[PATCH v4 2/5] drm/connector: Clarify the unit of TV margins

[Boris Brezillon]

All margins are expressed in pixels. Clarify that in the doc.

Signed-off-by: Boris Brezillon <***@bootlin.com>
---
include/drm/drm_connector.h | 2 +-
include/drm/drm_mode_config.h | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 665b9cae7f43..9d28e8180cfd 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -394,7 +394,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
/**
* struct drm_tv_connector_state - TV connector related states
* @subconnector: selected subconnector
- * @margins: margins
+ * @margins: margins (all margins are expressed in pixels)
* @margins.left: left margin
* @margins.right: right margin
* @margins.top: top margin
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 5dbeabdbaf91..30bb3ce85921 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -674,22 +674,22 @@ struct drm_mode_config {
struct drm_property *tv_mode_property;
/**
* @tv_left_margin_property: Optional TV property to set the left
- * margin.
+ * margin (expressed in pixels).
*/
struct drm_property *tv_left_margin_property;
/**
* @tv_right_margin_property: Optional TV property to set the right
- * margin.
+ * margin (expressed in pixels).
*/
struct drm_property *tv_right_margin_property;
/**
* @tv_top_margin_property: Optional TV property to set the right
- * margin.
+ * margin (expressed in pixels).
*/
struct drm_property *tv_top_margin_property;
/**
* @tv_bottom_margin_property: Optional TV property to set the right
- * margin.
+ * margin (expressed in pixels).
*/
struct drm_property *tv_bottom_margin_property;
/**

[Boris Brezillon]

The in the kernel-doc header did not match the function name.

Signed-off-by: Boris Brezillon <***@bootlin.com>
---
drivers/gpu/drm/drm_connector.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index fa9baacc863b..408082a57171 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1138,7 +1138,7 @@ void drm_hdmi_avi_infoframe_content_type(struct hdmi_avi_infoframe *frame,
EXPORT_SYMBOL(drm_hdmi_avi_infoframe_content_type);

/**
- * drm_create_tv_properties - create TV specific connector properties
+ * drm_mode_create_tv_properties - create TV specific connector properties
* @dev: DRM device
* @num_modes: number of different TV formats (modes) supported
* @modes: array of pointers to strings containing name of each format

[Boris Brezillon]

TV margins properties can only be added as part of the SDTV TV
connector properties creation, but we might need those props for HDMI
TVs too, so let's move the margins props creation in a separate
function and expose it to drivers.

We also add an helper to attach margins props to a connector.

Signed-off-by: Boris Brezillon <***@bootlin.com>
---
drivers/gpu/drm/drm_connector.c | 81 +++++++++++++++++++++++++--------
include/drm/drm_connector.h | 2 +
2 files changed, 65 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 408082a57171..ea616fb1501c 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1137,6 +1137,68 @@ void drm_hdmi_avi_infoframe_content_type(struct hdmi_avi_infoframe *frame,
}
EXPORT_SYMBOL(drm_hdmi_avi_infoframe_content_type);

+/**
+ * drm_mode_attach_tv_margin_properties - attach TV connector margin properties
+ * @connector: DRM connector
+ *
+ * Called by a driver when it needs to attach TV margin props to a connector.
+ * Typically used on SDTV and HDMI connectors.
+ */
+void drm_connector_attach_tv_margin_properties(struct drm_connector *connector)
+{
+ struct drm_device *dev = connector->dev;
+
+ drm_object_attach_property(&connector->base,
+ dev->mode_config.tv_left_margin_property,
+ 0);
+ drm_object_attach_property(&connector->base,
+ dev->mode_config.tv_right_margin_property,
+ 0);
+ drm_object_attach_property(&connector->base,
+ dev->mode_config.tv_top_margin_property,
+ 0);
+ drm_object_attach_property(&connector->base,
+ dev->mode_config.tv_bottom_margin_property,
+ 0);
+}
+
+/**
+ * drm_mode_create_tv_margin_properties - create TV connector margin properties
+ * @dev: DRM device
+ *
+ * Called by a driver's HDMI connector initialization routine, this function
+ * creates the TV margin properties for a given device. No need to call this
+ * function for an SDTV connector, it's already called from
+ * drm_mode_create_tv_properties().
+ */
+int drm_mode_create_tv_margin_properties(struct drm_device *dev)
+{
+ if (dev->mode_config.tv_left_margin_property)
+ return 0;
+
+ dev->mode_config.tv_left_margin_property =
+ drm_property_create_range(dev, 0, "left margin", 0, 100);
+ if (!dev->mode_config.tv_left_margin_property)
+ return -ENOMEM;
+
+ dev->mode_config.tv_right_margin_property =
+ drm_property_create_range(dev, 0, "right margin", 0, 100);
+ if (!dev->mode_config.tv_right_margin_property)
+ return -ENOMEM;
+
+ dev->mode_config.tv_top_margin_property =
+ drm_property_create_range(dev, 0, "top margin", 0, 100);
+ if (!dev->mode_config.tv_top_margin_property)
+ return -ENOMEM;
+
+ dev->mode_config.tv_bottom_margin_property =
+ drm_property_create_range(dev, 0, "bottom margin", 0, 100);
+ if (!dev->mode_config.tv_bottom_margin_property)
+ return -ENOMEM;
+
+ return 0;
+}
+
/**
* drm_mode_create_tv_properties - create TV specific connector properties
* @dev: DRM device
@@ -1183,24 +1245,7 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
/*
* Other, TV specific properties: margins & TV modes.
*/
- dev->mode_config.tv_left_margin_property =
- drm_property_create_range(dev, 0, "left margin", 0, 100);
- if (!dev->mode_config.tv_left_margin_property)
- goto nomem;
-
- dev->mode_config.tv_right_margin_property =
- drm_property_create_range(dev, 0, "right margin", 0, 100);
- if (!dev->mode_config.tv_right_margin_property)
- goto nomem;
-
- dev->mode_config.tv_top_margin_property =
- drm_property_create_range(dev, 0, "top margin", 0, 100);
- if (!dev->mode_config.tv_top_margin_property)
- goto nomem;
-
- dev->mode_config.tv_bottom_margin_property =
- drm_property_create_range(dev, 0, "bottom margin", 0, 100);
- if (!dev->mode_config.tv_bottom_margin_property)
+ if (drm_mode_create_tv_margin_properties(dev))
goto nomem;

dev->mode_config.tv_mode_property =
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 9d28e8180cfd..594f8d33a61f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1238,9 +1238,11 @@ const char *drm_get_tv_select_name(int val);
const char *drm_get_content_protection_name(int val);

int drm_mode_create_dvi_i_properties(struct drm_device *dev);
+int drm_mode_create_tv_margin_properties(struct drm_device *dev);
int drm_mode_create_tv_properties(struct drm_device *dev,
unsigned int num_modes,
const char * const modes[]);
+void drm_connector_attach_tv_margin_properties(struct drm_connector *conn);
int drm_mode_create_scaling_mode_property(struct drm_device *dev);
int drm_connector_attach_content_type_property(struct drm_connector *dev);
int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,

[Boris Brezillon]

Applyin margins is just a matter of scaling all planes appropriately
and adjusting the CRTC X/Y offset to account for the
left/right/top/bottom borders.

Create a vc4_plane_margins_adj() function doing that and call it from
vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
margins properties to the HDMI connector.

Signed-off-by: Boris Brezillon <***@bootlin.com>
Reviewed-by: Eric Anholt <***@anholt.net>
---
Changes in v5:
- Use the margins props instead of the underscan ones

Changes in v4:
- Add Eric's R-b

Changes in v3:
- Rebase on top of the "cursor rescaling" changes

Changes in v2:
- Take changes on hborder/vborder meaning into account
---
drivers/gpu/drm/vc4/vc4_crtc.c | 43 ++++++++++++++++++++++++++++
drivers/gpu/drm/vc4/vc4_drv.h | 3 ++
drivers/gpu/drm/vc4/vc4_plane.c | 50 +++++++++++++++++++++++++++++++++
3 files changed, 96 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 3ce136ba8791..97caf1671dd0 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -49,6 +49,13 @@ struct vc4_crtc_state {
struct drm_mm_node mm;
bool feed_txp;
bool txp_armed;
+
+ struct {
+ unsigned int left;
+ unsigned int right;
+ unsigned int top;
+ unsigned int bottom;
+ } margins;
};

static inline struct vc4_crtc_state *
@@ -624,6 +631,37 @@ static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc,
return MODE_OK;
}

+void vc4_crtc_get_margins(struct drm_crtc_state *state,
+ unsigned int *left, unsigned int *right,
+ unsigned int *top, unsigned int *bottom)
+{
+ struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
+ struct drm_connector_state *conn_state;
+ struct drm_connector *conn;
+ int i;
+
+ *left = vc4_state->margins.left;
+ *right = vc4_state->margins.right;
+ *top = vc4_state->margins.top;
+ *bottom = vc4_state->margins.bottom;
+
+ /* We have to interate over all new connector states because
+ * vc4_crtc_get_margins() might be called before
+ * vc4_crtc_atomic_check() which means margins info in vc4_crtc_state
+ * might be outdated.
+ */
+ for_each_new_connector_in_state(state->state, conn, conn_state, i) {
+ if (conn_state->crtc != state->crtc)
+ continue;
+
+ *left = conn_state->tv.margins.left;
+ *right = conn_state->tv.margins.right;
+ *top = conn_state->tv.margins.top;
+ *bottom = conn_state->tv.margins.bottom;
+ break;
+ }
+}
+
static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_crtc_state *state)
{
@@ -671,6 +709,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
vc4_state->feed_txp = false;
}

+ vc4_state->margins.left = conn_state->tv.margins.left;
+ vc4_state->margins.right = conn_state->tv.margins.right;
+ vc4_state->margins.top = conn_state->tv.margins.top;
+ vc4_state->margins.bottom = conn_state->tv.margins.bottom;
break;
}

@@ -972,6 +1014,7 @@ static struct drm_crtc_state *vc4_crtc_duplicate_state(struct drm_crtc *crtc)

old_vc4_state = to_vc4_crtc_state(crtc->state);
vc4_state->feed_txp = old_vc4_state->feed_txp;
+ vc4_state->margins = old_vc4_state->margins;

__drm_atomic_helper_crtc_duplicate_state(crtc, &vc4_state->base);
return &vc4_state->base;
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 4f87b03f837d..c24b078f0593 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -707,6 +707,9 @@ bool vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
const struct drm_display_mode *mode);
void vc4_crtc_handle_vblank(struct vc4_crtc *crtc);
void vc4_crtc_txp_armed(struct drm_crtc_state *state);
+void vc4_crtc_get_margins(struct drm_crtc_state *state,
+ unsigned int *right, unsigned int *left,
+ unsigned int *top, unsigned int *bottom);

/* vc4_debugfs.c */
int vc4_debugfs_init(struct drm_minor *minor);
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 75db62cbe468..f73fd35dfcff 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -258,6 +258,52 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
}
}

+static int vc4_plane_margins_adj(struct drm_plane_state *pstate)
+{
+ struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
+ unsigned int left, right, top, bottom, adjhdisplay, adjvdisplay;
+ struct drm_crtc_state *crtc_state;
+
+ crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
+ pstate->crtc);
+
+ vc4_crtc_get_margins(crtc_state, &left, &right, &top, &bottom);
+ if (!left && !right && !top && !bottom)
+ return 0;
+
+ if (left + right >= crtc_state->mode.hdisplay ||
+ top + bottom >= crtc_state->mode.vdisplay)
+ return -EINVAL;
+
+ adjhdisplay = crtc_state->mode.hdisplay - (left + right);
+ vc4_pstate->crtc_x = DIV_ROUND_CLOSEST(vc4_pstate->crtc_x *
+ adjhdisplay,
+ crtc_state->mode.hdisplay);
+ vc4_pstate->crtc_x += left;
+ if (vc4_pstate->crtc_x > crtc_state->mode.hdisplay - left)
+ vc4_pstate->crtc_x = crtc_state->mode.hdisplay - left;
+
+ adjvdisplay = crtc_state->mode.vdisplay - (top + bottom);
+ vc4_pstate->crtc_y = DIV_ROUND_CLOSEST(vc4_pstate->crtc_y *
+ adjvdisplay,
+ crtc_state->mode.vdisplay);
+ vc4_pstate->crtc_y += top;
+ if (vc4_pstate->crtc_y > crtc_state->mode.vdisplay - top)
+ vc4_pstate->crtc_y = crtc_state->mode.vdisplay - top;
+
+ vc4_pstate->crtc_w = DIV_ROUND_CLOSEST(vc4_pstate->crtc_w *
+ adjhdisplay,
+ crtc_state->mode.hdisplay);
+ vc4_pstate->crtc_h = DIV_ROUND_CLOSEST(vc4_pstate->crtc_h *
+ adjvdisplay,
+ crtc_state->mode.vdisplay);
+
+ if (!vc4_pstate->crtc_w || !vc4_pstate->crtc_h)
+ return -EINVAL;
+
+ return 0;
+}
+
static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
{
struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
@@ -306,6 +352,10 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
vc4_state->crtc_w = state->dst.x2 - state->dst.x1;
vc4_state->crtc_h = state->dst.y2 - state->dst.y1;

+ ret = vc4_plane_margins_adj(state);
+ if (ret)
+ return ret;
+
vc4_state->x_scaling[0] = vc4_get_scaling_mode(vc4_state->src_w[0],
vc4_state->crtc_w);
vc4_state->y_scaling[0] = vc4_get_scaling_mode(vc4_state->src_h[0],

[Boris Brezillon]

Now that the plane code takes the margins setup into account, we can
safely attach margin props to the HDMI connector.

We also take care of filling AVI infoframes correctly to expose the
top/botton/left/right bar.

Note that those margin props match pretty well the
overscan_{left,right,top,bottom} properties defined in config.txt and
parsed by the VC4 firmware.

Signed-off-by: Boris Brezillon <***@bootlin.com>
Reviewed-by: Eric Anholt <***@anholt.net>
---
Changes in v5:
- Use margin props instead of underscan ones

Changes in v4:
- Add Eric's R-b

Changes in v3:
- none

Changes in v2:
- none
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index fd5522fd179e..2f276222e30f 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -310,6 +310,7 @@ static struct drm_connector *vc4_hdmi_connector_init(struct drm_device *dev,
{
struct drm_connector *connector;
struct vc4_hdmi_connector *hdmi_connector;
+ int ret;

hdmi_connector = devm_kzalloc(dev->dev, sizeof(*hdmi_connector),
GFP_KERNEL);
@@ -323,6 +324,13 @@ static struct drm_connector *vc4_hdmi_connector_init(struct drm_device *dev,
DRM_MODE_CONNECTOR_HDMIA);
drm_connector_helper_add(connector, &vc4_hdmi_connector_helper_funcs);

+ /* Create and attach TV margin props to this connector. */
+ ret = drm_mode_create_tv_margin_properties(dev);
+ if (ret)
+ return ERR_PTR(ret);
+
+ drm_connector_attach_tv_margin_properties(connector);
+
connector->polled = (DRM_CONNECTOR_POLL_CONNECT |
DRM_CONNECTOR_POLL_DISCONNECT);

@@ -408,6 +416,9 @@ static void vc4_hdmi_write_infoframe(struct drm_encoder *encoder,
static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)
{
struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
+ struct vc4_dev *vc4 = encoder->dev->dev_private;
+ struct vc4_hdmi *hdmi = vc4->hdmi;
+ struct drm_connector_state *cstate = hdmi->connector->state;
struct drm_crtc *crtc = encoder->crtc;
const struct drm_display_mode *mode = &crtc->state->adjusted_mode;
union hdmi_infoframe frame;
@@ -426,6 +437,11 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)
vc4_encoder->rgb_range_selectable,
false);

+ frame.avi.right_bar = cstate->tv.margins.right;
+ frame.avi.left_bar = cstate->tv.margins.left;
+ frame.avi.top_bar = cstate->tv.margins.top;
+ frame.avi.bottom_bar = cstate->tv.margins.bottom;
+
vc4_hdmi_write_infoframe(encoder, &frame);
}

[Eric Anholt]

Hello,
As suggested by Ville, this version uses the already available TV
margin props to define left/right/top/bottom margins insted of adding
new underscan props.
This works pretty well for what we want to do in VC4: allow one to
define the visible area on a TV.
The first 2 patches are fixing the existing doc, patch 3 is allowing
one to create and attach TV margins independently of the TV connector
props. Finally, patch 4 and 5 add support for those props to the VC4
driver.

This series is:

Reviewed-by: Eric Anholt <***@anholt.net>