[PATCH v2 0/4] drm/connector: Provide generic support for underscan

[PATCH v2 0/4] drm/connector: Provide generic support for underscan

[Boris Brezillon]

Hello,

This is an attempt at providing generic support for underscan connector
props. We already have 3 drivers defining the same underscan, underscan
vborder and underscan hborder properties (amd, radeon and nouveau) and
I am about to add a new one, hence my proposal to put the prop parsing
code in the core and add ->underscan fields to drm_connector_state.

In this v2, I also converted the nouveau driver to the generic
approach. The amdgpu and radeon ones can't be converted since they're
still not converted to the atomic interface.

Regards,

Boris 

Boris Brezillon (4):
  drm/connector: Add generic underscan properties
  drm/vc4: Take underscan setup into account when updating planes
  drm/vc4: Attach underscan props to the HDMI connector
  drm/nouveau: Switch to the generic underscan props

 drivers/gpu/drm/drm_atomic.c                |  12 +++
 drivers/gpu/drm/drm_connector.c             | 127 ++++++++++++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_connector.c |  39 ++-------
 drivers/gpu/drm/nouveau/nouveau_connector.h |   9 --
 drivers/gpu/drm/nouveau/nouveau_display.c   |  14 ---
 drivers/gpu/drm/nouveau/nouveau_display.h   |   3 -
 drivers/gpu/drm/nouveau/nv50_display.c      |  17 ++--
 drivers/gpu/drm/vc4/vc4_hdmi.c              |  25 ++++++
 drivers/gpu/drm/vc4/vc4_plane.c             |  49 ++++++++++-
 include/drm/drm_connector.h                 |  80 ++++++++++++++++++
 10 files changed, 310 insertions(+), 65 deletions(-)

-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/4] drm/connector: Add generic underscan properties

[Boris Brezillon]

We have 3 drivers defining the "underscan", "underscan hborder" and
"underscan vborder" properties (radeon, amd and nouveau) and we are
about to add the same kind of thing in VC4.

Define generic underscan props and add new fields to the drm_connector
state so that the property parsing logic can be shared by all DRM
drivers.

A driver can now attach underscan properties to its connector through
the drm_connector_attach_underscan_properties() helper, and can
check/apply the underscan setup based on the
drm_connector_state->underscan fields.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v2:
- Add a new section in the connector props doc to describe underscan
  props
- Fix description of auto mode (auto means apply underscan for HDMI
  monitors only)
- Fix description of vborder/hborder:
    right_border = left_border = hborder
    top_border = bottom_border = vborder
  not
    right_border = left_border = hborder / 2
    top_border = bottom_border = vborder / 2
- Rename drm_underscan into drm_underscan_state
---
 drivers/gpu/drm/drm_atomic.c    |  12 ++++
 drivers/gpu/drm/drm_connector.c | 127 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h     |  80 +++++++++++++++++++++++++
 3 files changed, 219 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index dc850b4b6e21..b7312bd172c9 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1278,6 +1278,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 			return -EINVAL;
 		}
 		state->content_protection = val;
+	} else if (property == connector->underscan_mode_property) {
+		state->underscan.mode = val;
+	} else if (property == connector->underscan_hborder_property) {
+		state->underscan.hborder = val;
+	} else if (property == connector->underscan_vborder_property) {
+		state->underscan.vborder = val;
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -1359,6 +1365,12 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = state->scaling_mode;
 	} else if (property == connector->content_protection_property) {
 		*val = state->content_protection;
+	} else if (property == connector->underscan_mode_property) {
+		*val = state->underscan.mode;
+	} else if (property == connector->underscan_hborder_property) {
+		*val = state->underscan.hborder;
+	} else if (property == connector->underscan_vborder_property) {
+		*val = state->underscan.vborder;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 9b9ba5d5ec0c..826af6267a4f 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -914,6 +914,38 @@ DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
  *	can also expose this property to external outputs, in which case they
  *	must support "None", which should be the default (since external screens
  *	have a built-in scaler).
+ *
+ * Connectors for non-analog outputs may also have standardized underscan
+ * properties (drivers can set this up by calling
+ * drm_connector_attach_content_protection_property() on initialization):
+ *
+ * underscan:
+ *	This properties defines whether underscan is activated or not, and when
+ *	it is activated, how the horizontal and vertical borders are calculated:
+ *
+ *	off:
+ *		Underscan is disabled. The output image shouldn't be scaled to
+ *		take screen borders into account.
+ *	on:
+ *		Underscan is activated and horizontal and vertical borders are
+ *		specified through the "underscan hborder" and
+ *		"underscan vborder" properties.
+ *	auto:
+ *		Underscan is activated only for HDMI monitors.
+ *
+ * underscan hborder:
+ *	Horizontal border expressed in pixels. The border is symmetric, which
+ *	means you'll have a border of 'hborder' pixels on the left and on the
+ *	same border on the right.
+ *	When this value is 0 and underscan is "on" or "auto", the driver will
+ *	pick a default value (the default value is driver dependent).
+ *
+ * underscan vborder:
+ *	Vertical border expressed in pixels. The border is symmetric, which
+ *	means you'll have a border of 'vborder' pixels on the top and the same
+ *	border on the bottom.
+ *	When this value is 0 and underscan is "on" or "auto", the driver will
+ *	pick a default value (the default value is driver dependent).
  */
 
 int drm_connector_create_standard_properties(struct drm_device *dev)
@@ -1108,6 +1140,101 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_mode_create_tv_properties);
 
+static const struct drm_prop_enum_list drm_underscan_mode_enum_list[] = {
+	{ DRM_UNDERSCAN_OFF, "off" },
+	{ DRM_UNDERSCAN_ON, "on" },
+	{ DRM_UNDERSCAN_AUTO, "auto" },
+};
+
+/**
+ * drm_connector_attach_underscan_properties - attach atomic underscan
+ *					       properties
+ * @connector: connector to attach underscan mode properties on.
+ * @mode_mask: bitmask of %DRM_UNDERSCAN_XX modes encoding the supported
+ *	       underscan modes.
+ * @max_hborder: maximum size of the horizontal border expressed in pixels.
+ *		 Should be > 0.
+ * @max_vborder: maximum size of the vertical border expressed in pixels.
+ *		 Should be > 0.
+ *
+ * This is used to add support for underscan to atomic drivers.
+ * The underscan config will be set to &drm_connector_state.underscan
+ * and can be used from &drm_connector_helper_funcs->atomic_check for
+ * validation.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_underscan_properties(struct drm_connector *connector,
+					      u32 mode_mask, u64 max_hborder,
+					      u64 max_vborder)
+{
+	unsigned int i, nmodes = ARRAY_SIZE(drm_underscan_mode_enum_list);
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	if (!max_hborder || !max_vborder)
+		return -EINVAL;
+
+	if (!hweight32(mode_mask) || (mode_mask & ~GENMASK(nmodes - 1, 0)))
+		return -EINVAL;
+
+	prop = drm_property_create(dev, DRM_MODE_PROP_ENUM, "underscan",
+				   hweight32(mode_mask));
+	if (!prop)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(drm_underscan_mode_enum_list); i++) {
+		const struct drm_prop_enum_list *entry;
+		int ret;
+
+		if (!(BIT(i) & mode_mask))
+			continue;
+
+		entry = &drm_underscan_mode_enum_list[i];
+		ret = drm_property_add_enum(prop, entry->type, entry->name);
+		if (ret)
+			goto err_free_mode_prop;
+	}
+
+	connector->underscan_mode_property = prop;
+
+	prop = drm_property_create_range(dev, 0, "underscan hborder", 0,
+					 max_hborder);
+	if (!prop)
+		goto err_free_mode_prop;
+
+	connector->underscan_hborder_property = prop;
+
+	prop = drm_property_create_range(dev, 0, "underscan vborder", 0,
+					 max_vborder);
+	if (!prop)
+		goto err_free_hborder_prop;
+
+	connector->underscan_vborder_property = prop;
+
+	drm_object_attach_property(&connector->base,
+				   connector->underscan_mode_property,
+				   DRM_UNDERSCAN_OFF);
+	drm_object_attach_property(&connector->base,
+				   connector->underscan_hborder_property, 0);
+	drm_object_attach_property(&connector->base,
+				   connector->underscan_vborder_property, 0);
+
+	return 0;
+
+err_free_hborder_prop:
+	drm_property_destroy(dev, connector->underscan_hborder_property);
+	connector->underscan_hborder_property = NULL;
+
+err_free_mode_prop:
+	drm_property_destroy(dev, connector->underscan_mode_property);
+	connector->underscan_mode_property = NULL;
+
+	return -ENOMEM;
+}
+EXPORT_SYMBOL(drm_connector_attach_underscan_properties);
+
 /**
  * drm_mode_create_scaling_mode_property - create scaling mode property
  * @dev: DRM device
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 675cc3f8cf85..5ad968dc48c3 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -372,6 +372,53 @@ struct drm_tv_connector_state {
 	unsigned int hue;
 };
 
+/**
+ * enum drm_underscan_mode - Underscan mode
+ *
+ * This enum is used to track the underscan mode.
+ *
+ * @DRM_UNDERSCAN_OFF: No underscan applied, the output image will be unchanged
+ * @DRM_UNDERSCAN_ON: Underscan is enabled, and horizontal/vertical border size
+ *		      are specified through the &struct_drm_underscan->hborder
+ *		      and &struct_drm_underscan->vborder fields.
+ * @DRM_UNDERSCAN_AUTO: Underscan is enabled and &struct_drm_underscan->hborder
+ *			and &struct_drm_underscan->vborder are guessed by the
+ *			driver.
+ */
+enum drm_underscan_mode {
+	DRM_UNDERSCAN_OFF,
+	DRM_UNDERSCAN_ON,
+	DRM_UNDERSCAN_AUTO,
+};
+
+/**
+ * struct drm_underscan_state - Underscan properties attached to a connector
+ *				state
+ *
+ * This can be used to tell the CRTC how the image should be scaled/placed in
+ * order to cover fit in the display connected through this connector. Most of
+ * the time used to address situations where the display borders are hidden.
+ * Can also be used to compensate overscan done on the display side.
+ */
+struct drm_underscan_state {
+	/**
+	 * @mode: Underscan mode.
+	 */
+	enum drm_underscan_mode mode;
+
+	/**
+	 * @hborder: Horizontal border. This values encodes both the left and
+	 * right borders: left_border = right_border = hborder / 2.
+	 */
+	unsigned int hborder;
+
+	/**
+	 * @vborder: Vertical border. This values encodes both the top and
+	 * bottom borders: top_border = bottom_border = vborder / 2.
+	 */
+	unsigned int vborder;
+};
+
 /**
  * struct drm_connector_state - mutable connector state
  * @connector: backpointer to the connector
@@ -429,6 +476,13 @@ struct drm_connector_state {
 	 * protection. This is most commonly used for HDCP.
 	 */
 	unsigned int content_protection;
+
+	/**
+	 * @underscan: Underscan information. Most commonly used to adjust
+	 * image when the display has borders covering part of the image of
+	 * when it's doing overscan on its own.
+	 */
+	struct drm_underscan_state underscan;
 };
 
 /**
@@ -892,6 +946,29 @@ struct drm_connector {
 	 */
 	struct drm_property_blob *tile_blob_ptr;
 
+	/**
+	 * @underscan_mode_property: Optional connector underscan mode. Used by
+	 * the driver to scale the output image and compensate an overscan done
+	 * on the display side.
+	 */
+	struct drm_property *underscan_mode_property;
+
+	/**
+	 * @underscan_hborder_property: Optional connector underscan horizontal
+	 * border (expressed in pixels). Used by the driver to adjust the
+	 * output image position and compensate an overscan done on the display
+	 * side.
+	 */
+	struct drm_property *underscan_hborder_property;
+
+	/**
+	 * @underscan_hborder_property: Optional connector underscan vertical
+	 * border (expressed in pixels). Used by the driver to adjust the
+	 * output image position and compensate an overscan done on the display
+	 * side.
+	 */
+	struct drm_property *underscan_vborder_property;
+
 /* should we poll this connector for connects and disconnects */
 /* hot plug detectable */
 #define DRM_CONNECTOR_POLL_HPD (1 << 0)
@@ -1088,6 +1165,9 @@ int drm_mode_create_dvi_i_properties(struct drm_device *dev);
 int drm_mode_create_tv_properties(struct drm_device *dev,
 				  unsigned int num_modes,
 				  const char * const modes[]);
+int drm_connector_attach_underscan_properties(struct drm_connector *connector,
+					      u32 mode_mask, u64 max_hborder,
+					      u64 max_vborder);
 int drm_mode_create_scaling_mode_property(struct drm_device *dev);
 int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
 					       u32 scaling_mode_mask);
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Boris Brezillon]

Applying an underscan setup is just a matter of scaling all planes
appropriately and adjusting the CRTC X/Y offset to account for the
horizontal and vertical border.

Create an vc4_plane_underscan_adj() function doing that and call it from
vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
underscan properties to the HDMI connector.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v2:
- Take changes on hborder/vborder meaning into account
---
 drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 71d44c357d35..61ed60841cd6 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
 	}
 }
 
+static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
+{
+	struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
+	struct drm_connector_state *conn_state = NULL;
+	struct drm_connector *conn;
+	struct drm_crtc_state *crtc_state;
+	int i;
+
+	for_each_new_connector_in_state(pstate->state, conn, conn_state, i) {
+		if (conn_state->crtc == pstate->crtc)
+			break;
+	}
+
+	if (i == pstate->state->num_connector)
+		return 0;
+
+	if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
+		return 0;
+
+	crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
+						   pstate->crtc);
+
+	if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay ||
+	    conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
+		return -EINVAL;
+
+	vc4_pstate->crtc_x += conn_state->underscan.hborder;
+	vc4_pstate->crtc_y += conn_state->underscan.vborder;
+	vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
+			      (crtc_state->mode.hdisplay -
+			       (conn_state->underscan.hborder * 2))) /
+			     crtc_state->mode.hdisplay;
+	vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
+			      (crtc_state->mode.vdisplay -
+			       (conn_state->underscan.vborder * 2))) /
+			     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 drm_plane *plane = state->plane;
@@ -269,7 +312,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
 	int num_planes = fb->format->num_planes;
 	u32 h_subsample = 1;
 	u32 v_subsample = 1;
-	int i;
+	int i, ret;
 
 	for (i = 0; i < num_planes; i++)
 		vc4_state->offsets[i] = bo->paddr + fb->offsets[i];
@@ -292,6 +335,10 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
 	vc4_state->crtc_w = state->crtc_w;
 	vc4_state->crtc_h = state->crtc_h;
 
+	ret = vc4_plane_underscan_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],
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Ville Syrjälä]

On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:
> Applying an underscan setup is just a matter of scaling all planes
> appropriately and adjusting the CRTC X/Y offset to account for the
> horizontal and vertical border.
> 
> Create an vc4_plane_underscan_adj() function doing that and call it from
> vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
> underscan properties to the HDMI connector.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> Changes in v2:
> - Take changes on hborder/vborder meaning into account
> ---
>  drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index 71d44c357d35..61ed60841cd6 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
>  	}
>  }
>  
> +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
> +{
> +	struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
> +	struct drm_connector_state *conn_state = NULL;
> +	struct drm_connector *conn;
> +	struct drm_crtc_state *crtc_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(pstate->state, conn, conn_state, i) {
> +		if (conn_state->crtc == pstate->crtc)
> +			break;
> +	}
> +
> +	if (i == pstate->state->num_connector)
> +		return 0;
> +
> +	if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
> +		return 0;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
> +						   pstate->crtc);
> +
> +	if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay ||
> +	    conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
> +		return -EINVAL;

border * 2 ?

> +
> +	vc4_pstate->crtc_x += conn_state->underscan.hborder;
> +	vc4_pstate->crtc_y += conn_state->underscan.vborder;
> +	vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
> +			      (crtc_state->mode.hdisplay -
> +			       (conn_state->underscan.hborder * 2))) /
> +			     crtc_state->mode.hdisplay;
> +	vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
> +			      (crtc_state->mode.vdisplay -
> +			       (conn_state->underscan.vborder * 2))) /
> +			     crtc_state->mode.vdisplay;

So you're now scaling all planes? The code seems to reject scaling for
the cursor plane, how are you dealing with that? Or just no cursor
allowed when underscanning?

I'm also wondering if there's any way we can reconcile these border
props with the scaling mode prop, should we ever wish to expose
these props on connectors that already have the scaling mode prop.
Maybe we just have to require the scaling mode to be set to fullscreen 
to allow borders to be specified explictly?

> +
> +	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 drm_plane *plane = state->plane;
> @@ -269,7 +312,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
>  	int num_planes = fb->format->num_planes;
>  	u32 h_subsample = 1;
>  	u32 v_subsample = 1;
> -	int i;
> +	int i, ret;
>  
>  	for (i = 0; i < num_planes; i++)
>  		vc4_state->offsets[i] = bo->paddr + fb->offsets[i];
> @@ -292,6 +335,10 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
>  	vc4_state->crtc_w = state->crtc_w;
>  	vc4_state->crtc_h = state->crtc_h;
>  
> +	ret = vc4_plane_underscan_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],
> -- 
> 2.14.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Boris Brezillon]

On Fri, 11 May 2018 18:34:50 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:
> > Applying an underscan setup is just a matter of scaling all planes
> > appropriately and adjusting the CRTC X/Y offset to account for the
> > horizontal and vertical border.
> > 
> > Create an vc4_plane_underscan_adj() function doing that and call it from
> > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
> > underscan properties to the HDMI connector.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> > Changes in v2:
> > - Take changes on hborder/vborder meaning into account
> > ---
> >  drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > index 71d44c357d35..61ed60841cd6 100644
> > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
> >  	}
> >  }
> >  
> > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
> > +{
> > +	struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
> > +	struct drm_connector_state *conn_state = NULL;
> > +	struct drm_connector *conn;
> > +	struct drm_crtc_state *crtc_state;
> > +	int i;
> > +
> > +	for_each_new_connector_in_state(pstate->state, conn, conn_state, i) {
> > +		if (conn_state->crtc == pstate->crtc)
> > +			break;
> > +	}
> > +
> > +	if (i == pstate->state->num_connector)
> > +		return 0;
> > +
> > +	if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
> > +		return 0;
> > +
> > +	crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
> > +						   pstate->crtc);
> > +
> > +	if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay ||
> > +	    conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
> > +		return -EINVAL;  
> 
> border * 2 ?

Oops, indeed. I'll fix that.

> 
> > +
> > +	vc4_pstate->crtc_x += conn_state->underscan.hborder;
> > +	vc4_pstate->crtc_y += conn_state->underscan.vborder;
> > +	vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
> > +			      (crtc_state->mode.hdisplay -
> > +			       (conn_state->underscan.hborder * 2))) /
> > +			     crtc_state->mode.hdisplay;
> > +	vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
> > +			      (crtc_state->mode.vdisplay -
> > +			       (conn_state->underscan.vborder * 2))) /
> > +			     crtc_state->mode.vdisplay;  
> 
> So you're now scaling all planes? The code seems to reject scaling for
> the cursor plane, how are you dealing with that? Or just no cursor
> allowed when underscanning?

No, I just didn't test with a cursor plane. We should probably avoid
scaling the cursor plane and just adjust its position. Eric any opinion
on that?

> 
> I'm also wondering if there's any way we can reconcile these border
> props with the scaling mode prop, should we ever wish to expose
> these props on connectors that already have the scaling mode prop.

Nouveau seems to expose both, and I don't see why we couldn't.

> Maybe we just have to require the scaling mode to be set to fullscreen 
> to allow borders to be specified explictly?
> 
> > +
> > +	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 drm_plane *plane = state->plane;
> > @@ -269,7 +312,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
> >  	int num_planes = fb->format->num_planes;
> >  	u32 h_subsample = 1;
> >  	u32 v_subsample = 1;
> > -	int i;
> > +	int i, ret;
> >  
> >  	for (i = 0; i < num_planes; i++)
> >  		vc4_state->offsets[i] = bo->paddr + fb->offsets[i];
> > @@ -292,6 +335,10 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
> >  	vc4_state->crtc_w = state->crtc_w;
> >  	vc4_state->crtc_h = state->crtc_h;
> >  
> > +	ret = vc4_plane_underscan_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],
> > -- 
> > 2.14.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel  
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Ville Syrjälä]

On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote:
> On Fri, 11 May 2018 18:34:50 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:
> > > Applying an underscan setup is just a matter of scaling all planes
> > > appropriately and adjusting the CRTC X/Y offset to account for the
> > > horizontal and vertical border.
> > > 
> > > Create an vc4_plane_underscan_adj() function doing that and call it from
> > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
> > > underscan properties to the HDMI connector.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > ---
> > > Changes in v2:
> > > - Take changes on hborder/vborder meaning into account
> > > ---
> > >  drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 48 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > > index 71d44c357d35..61ed60841cd6 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
> > >  	}
> > >  }
> > >  
> > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
> > > +{
> > > +	struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
> > > +	struct drm_connector_state *conn_state = NULL;
> > > +	struct drm_connector *conn;
> > > +	struct drm_crtc_state *crtc_state;
> > > +	int i;
> > > +
> > > +	for_each_new_connector_in_state(pstate->state, conn, conn_state, i) {
> > > +		if (conn_state->crtc == pstate->crtc)
> > > +			break;
> > > +	}
> > > +
> > > +	if (i == pstate->state->num_connector)
> > > +		return 0;
> > > +
> > > +	if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
> > > +		return 0;
> > > +
> > > +	crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
> > > +						   pstate->crtc);
> > > +
> > > +	if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay ||
> > > +	    conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
> > > +		return -EINVAL;  
> > 
> > border * 2 ?
> 
> Oops, indeed. I'll fix that.
> 
> > 
> > > +
> > > +	vc4_pstate->crtc_x += conn_state->underscan.hborder;
> > > +	vc4_pstate->crtc_y += conn_state->underscan.vborder;
> > > +	vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
> > > +			      (crtc_state->mode.hdisplay -
> > > +			       (conn_state->underscan.hborder * 2))) /
> > > +			     crtc_state->mode.hdisplay;
> > > +	vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
> > > +			      (crtc_state->mode.vdisplay -
> > > +			       (conn_state->underscan.vborder * 2))) /
> > > +			     crtc_state->mode.vdisplay;  
> > 
> > So you're now scaling all planes? The code seems to reject scaling for
> > the cursor plane, how are you dealing with that? Or just no cursor
> > allowed when underscanning?
> 
> No, I just didn't test with a cursor plane. We should probably avoid
> scaling the cursor plane and just adjust its position. Eric any opinion
> on that?

I don't think you can just not scale it. The user asked for the cursor
to be at a specific place with a specific size. Can't just ignore
that and do something else. Also eg. i915 would definitely scale the
cursor since we'd just scale the entire crtc instead of scaling
individual planes. Different drivers doing different things wouldn't
be good.

> 
> > 
> > I'm also wondering if there's any way we can reconcile these border
> > props with the scaling mode prop, should we ever wish to expose
> > these props on connectors that already have the scaling mode prop.
> 
> Nouveau seems to expose both, and I don't see why we couldn't.

First of all the interaction of these borders with panels that have
a fixed mode would need to be properly specified. Do we apply the
borders against the user mode or the panel's native mode? I guess
the latter would be a bit easier (would also match how the TV margins
behave I suppose). But that does leave open the question of how
would generic userspace know which case it's dealing with? Eg. if it
wants to underscan by some specific percentage it has to calculate
the borders based on the correct mode, otherwise the results aren't
going to match the expectations.

> 
> > Maybe we just have to require the scaling mode to be set to fullscreen 
> > to allow borders to be specified explictly?
> > 
> > > +
> > > +	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 drm_plane *plane = state->plane;
> > > @@ -269,7 +312,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
> > >  	int num_planes = fb->format->num_planes;
> > >  	u32 h_subsample = 1;
> > >  	u32 v_subsample = 1;
> > > -	int i;
> > > +	int i, ret;
> > >  
> > >  	for (i = 0; i < num_planes; i++)
> > >  		vc4_state->offsets[i] = bo->paddr + fb->offsets[i];
> > > @@ -292,6 +335,10 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
> > >  	vc4_state->crtc_w = state->crtc_w;
> > >  	vc4_state->crtc_h = state->crtc_h;
> > >  
> > > +	ret = vc4_plane_underscan_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],
> > > -- 
> > > 2.14.1
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel  
> > 

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Boris Brezillon]

On Fri, 11 May 2018 19:54:02 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote:
> > On Fri, 11 May 2018 18:34:50 +0300
> > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >   
> > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:  
> > > > Applying an underscan setup is just a matter of scaling all planes
> > > > appropriately and adjusting the CRTC X/Y offset to account for the
> > > > horizontal and vertical border.
> > > > 
> > > > Create an vc4_plane_underscan_adj() function doing that and call it from
> > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
> > > > underscan properties to the HDMI connector.
> > > > 
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > ---
> > > > Changes in v2:
> > > > - Take changes on hborder/vborder meaning into account
> > > > ---
> > > >  drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 48 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > > > index 71d44c357d35..61ed60841cd6 100644
> > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
> > > >  	}
> > > >  }
> > > >  
> > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
> > > > +{
> > > > +	struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
> > > > +	struct drm_connector_state *conn_state = NULL;
> > > > +	struct drm_connector *conn;
> > > > +	struct drm_crtc_state *crtc_state;
> > > > +	int i;
> > > > +
> > > > +	for_each_new_connector_in_state(pstate->state, conn, conn_state, i) {
> > > > +		if (conn_state->crtc == pstate->crtc)
> > > > +			break;
> > > > +	}
> > > > +
> > > > +	if (i == pstate->state->num_connector)
> > > > +		return 0;
> > > > +
> > > > +	if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
> > > > +		return 0;
> > > > +
> > > > +	crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
> > > > +						   pstate->crtc);
> > > > +
> > > > +	if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay ||
> > > > +	    conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
> > > > +		return -EINVAL;    
> > > 
> > > border * 2 ?  
> > 
> > Oops, indeed. I'll fix that.
> >   
> > >   
> > > > +
> > > > +	vc4_pstate->crtc_x += conn_state->underscan.hborder;
> > > > +	vc4_pstate->crtc_y += conn_state->underscan.vborder;
> > > > +	vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
> > > > +			      (crtc_state->mode.hdisplay -
> > > > +			       (conn_state->underscan.hborder * 2))) /
> > > > +			     crtc_state->mode.hdisplay;
> > > > +	vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
> > > > +			      (crtc_state->mode.vdisplay -
> > > > +			       (conn_state->underscan.vborder * 2))) /
> > > > +			     crtc_state->mode.vdisplay;    
> > > 
> > > So you're now scaling all planes? The code seems to reject scaling for
> > > the cursor plane, how are you dealing with that? Or just no cursor
> > > allowed when underscanning?  
> > 
> > No, I just didn't test with a cursor plane. We should probably avoid
> > scaling the cursor plane and just adjust its position. Eric any opinion
> > on that?  
> 
> I don't think you can just not scale it. The user asked for the cursor
> to be at a specific place with a specific size. Can't just ignore
> that and do something else. Also eg. i915 would definitely scale the
> cursor since we'd just scale the entire crtc instead of scaling
> individual planes. Different drivers doing different things wouldn't
> be good.

Except in our case the scaling takes place before the composition, so
we don't have a choice.

> 
> >   
> > > 
> > > I'm also wondering if there's any way we can reconcile these border
> > > props with the scaling mode prop, should we ever wish to expose
> > > these props on connectors that already have the scaling mode prop.  
> > 
> > Nouveau seems to expose both, and I don't see why we couldn't.  
> 
> First of all the interaction of these borders with panels that have
> a fixed mode would need to be properly specified. Do we apply the
> borders against the user mode or the panel's native mode?

Hm, I'm a bit lost. Isn't crtc_state->mode supposed to contain the mode
that is about to be applied, no matter if it's a usermode or one of the
mode returned by the display? 

> I guess
> the latter would be a bit easier (would also match how the TV margins
> behave I suppose). But that does leave open the question of how
> would generic userspace know which case it's dealing with? Eg. if it
> wants to underscan by some specific percentage it has to calculate
> the borders based on the correct mode, otherwise the results aren't
> going to match the expectations.

I don't get it, sorry. Borders are relative to the mode applied by
userspace. So if it needs to express borders as a ratio of the
resolution, then for sure userspace will have to do the math, but I
don't see the problem here.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Ville Syrjälä]

On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote:
> On Fri, 11 May 2018 19:54:02 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote:
> > > On Fri, 11 May 2018 18:34:50 +0300
> > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > >   
> > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:  
> > > > > Applying an underscan setup is just a matter of scaling all planes
> > > > > appropriately and adjusting the CRTC X/Y offset to account for the
> > > > > horizontal and vertical border.
> > > > > 
> > > > > Create an vc4_plane_underscan_adj() function doing that and call it from
> > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
> > > > > underscan properties to the HDMI connector.
> > > > > 
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > > ---
> > > > > Changes in v2:
> > > > > - Take changes on hborder/vborder meaning into account
> > > > > ---
> > > > >  drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 48 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > > > > index 71d44c357d35..61ed60841cd6 100644
> > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
> > > > > +{
> > > > > +	struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
> > > > > +	struct drm_connector_state *conn_state = NULL;
> > > > > +	struct drm_connector *conn;
> > > > > +	struct drm_crtc_state *crtc_state;
> > > > > +	int i;
> > > > > +
> > > > > +	for_each_new_connector_in_state(pstate->state, conn, conn_state, i) {
> > > > > +		if (conn_state->crtc == pstate->crtc)
> > > > > +			break;
> > > > > +	}
> > > > > +
> > > > > +	if (i == pstate->state->num_connector)
> > > > > +		return 0;
> > > > > +
> > > > > +	if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
> > > > > +		return 0;
> > > > > +
> > > > > +	crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
> > > > > +						   pstate->crtc);
> > > > > +
> > > > > +	if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay ||
> > > > > +	    conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
> > > > > +		return -EINVAL;    
> > > > 
> > > > border * 2 ?  
> > > 
> > > Oops, indeed. I'll fix that.
> > >   
> > > >   
> > > > > +
> > > > > +	vc4_pstate->crtc_x += conn_state->underscan.hborder;
> > > > > +	vc4_pstate->crtc_y += conn_state->underscan.vborder;
> > > > > +	vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
> > > > > +			      (crtc_state->mode.hdisplay -
> > > > > +			       (conn_state->underscan.hborder * 2))) /
> > > > > +			     crtc_state->mode.hdisplay;
> > > > > +	vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
> > > > > +			      (crtc_state->mode.vdisplay -
> > > > > +			       (conn_state->underscan.vborder * 2))) /
> > > > > +			     crtc_state->mode.vdisplay;    
> > > > 
> > > > So you're now scaling all planes? The code seems to reject scaling for
> > > > the cursor plane, how are you dealing with that? Or just no cursor
> > > > allowed when underscanning?  
> > > 
> > > No, I just didn't test with a cursor plane. We should probably avoid
> > > scaling the cursor plane and just adjust its position. Eric any opinion
> > > on that?  
> > 
> > I don't think you can just not scale it. The user asked for the cursor
> > to be at a specific place with a specific size. Can't just ignore
> > that and do something else. Also eg. i915 would definitely scale the
> > cursor since we'd just scale the entire crtc instead of scaling
> > individual planes. Different drivers doing different things wouldn't
> > be good.
> 
> Except in our case the scaling takes place before the composition, so
> we don't have a choice.

The choice is to either do what userspace asked, or return an error.

> 
> > 
> > >   
> > > > 
> > > > I'm also wondering if there's any way we can reconcile these border
> > > > props with the scaling mode prop, should we ever wish to expose
> > > > these props on connectors that already have the scaling mode prop.  
> > > 
> > > Nouveau seems to expose both, and I don't see why we couldn't.  
> > 
> > First of all the interaction of these borders with panels that have
> > a fixed mode would need to be properly specified. Do we apply the
> > borders against the user mode or the panel's native mode?
> 
> Hm, I'm a bit lost. Isn't crtc_state->mode supposed to contain the mode
> that is about to be applied, no matter if it's a usermode or one of the
> mode returned by the display? 

No. With fixed mode panels the user mode is a lie. Only
hdisplay/vdisplay mean anything. The actual timings are known only to
the kernel. Well, userspace can perhaps make an educated guess based on
the connector's mode list and the presence of the scaling mode property.

> 
> > I guess
> > the latter would be a bit easier (would also match how the TV margins
> > behave I suppose). But that does leave open the question of how
> > would generic userspace know which case it's dealing with? Eg. if it
> > wants to underscan by some specific percentage it has to calculate
> > the borders based on the correct mode, otherwise the results aren't
> > going to match the expectations.
> 
> I don't get it, sorry. Borders are relative to the mode applied by
> userspace. So if it needs to express borders as a ratio of the
> resolution, then for sure userspace will have to do the math, but I
> don't see the problem here.

-- 
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Boris Brezillon]

On Fri, 11 May 2018 20:29:48 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote:
> > On Fri, 11 May 2018 19:54:02 +0300
> > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >   
> > > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote:  
> > > > On Fri, 11 May 2018 18:34:50 +0300
> > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > >     
> > > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:    
> > > > > > Applying an underscan setup is just a matter of scaling all planes
> > > > > > appropriately and adjusting the CRTC X/Y offset to account for the
> > > > > > horizontal and vertical border.
> > > > > > 
> > > > > > Create an vc4_plane_underscan_adj() function doing that and call it from
> > > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
> > > > > > underscan properties to the HDMI connector.
> > > > > > 
> > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > - Take changes on hborder/vborder meaning into account
> > > > > > ---
> > > > > >  drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++-
> > > > > >  1 file changed, 48 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > > > > > index 71d44c357d35..61ed60841cd6 100644
> > > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
> > > > > >  	}
> > > > > >  }
> > > > > >  
> > > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
> > > > > > +{
> > > > > > +	struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
> > > > > > +	struct drm_connector_state *conn_state = NULL;
> > > > > > +	struct drm_connector *conn;
> > > > > > +	struct drm_crtc_state *crtc_state;
> > > > > > +	int i;
> > > > > > +
> > > > > > +	for_each_new_connector_in_state(pstate->state, conn, conn_state, i) {
> > > > > > +		if (conn_state->crtc == pstate->crtc)
> > > > > > +			break;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (i == pstate->state->num_connector)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
> > > > > > +						   pstate->crtc);
> > > > > > +
> > > > > > +	if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay ||
> > > > > > +	    conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
> > > > > > +		return -EINVAL;      
> > > > > 
> > > > > border * 2 ?    
> > > > 
> > > > Oops, indeed. I'll fix that.
> > > >     
> > > > >     
> > > > > > +
> > > > > > +	vc4_pstate->crtc_x += conn_state->underscan.hborder;
> > > > > > +	vc4_pstate->crtc_y += conn_state->underscan.vborder;
> > > > > > +	vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
> > > > > > +			      (crtc_state->mode.hdisplay -
> > > > > > +			       (conn_state->underscan.hborder * 2))) /
> > > > > > +			     crtc_state->mode.hdisplay;
> > > > > > +	vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
> > > > > > +			      (crtc_state->mode.vdisplay -
> > > > > > +			       (conn_state->underscan.vborder * 2))) /
> > > > > > +			     crtc_state->mode.vdisplay;      
> > > > > 
> > > > > So you're now scaling all planes? The code seems to reject scaling for
> > > > > the cursor plane, how are you dealing with that? Or just no cursor
> > > > > allowed when underscanning?    
> > > > 
> > > > No, I just didn't test with a cursor plane. We should probably avoid
> > > > scaling the cursor plane and just adjust its position. Eric any opinion
> > > > on that?    
> > > 
> > > I don't think you can just not scale it. The user asked for the cursor
> > > to be at a specific place with a specific size. Can't just ignore
> > > that and do something else. Also eg. i915 would definitely scale the
> > > cursor since we'd just scale the entire crtc instead of scaling
> > > individual planes. Different drivers doing different things wouldn't
> > > be good.  
> > 
> > Except in our case the scaling takes place before the composition, so
> > we don't have a choice.  
> 
> The choice is to either do what userspace asked, or return an error.

Come on! If we can't use underscan when there's a cursor plane enabled
this feature is pretty much useless. But let's take a real use case to
show you how negligible the lack of scaling on the cursor plane will
be. Say you have borders taking 10% of you screen (which is already a
lot), and your cursor is a plane of 64x64 pixels, you'll end up with a
64x64 cursor instead of 58x58. Quite frankly, I doubt you'll notice
the difference.

Anyway, I'd like to hear back from Eric on that, since he is the one
who asked me to work on this feature.

> 
> >   
> > >   
> > > >     
> > > > > 
> > > > > I'm also wondering if there's any way we can reconcile these border
> > > > > props with the scaling mode prop, should we ever wish to expose
> > > > > these props on connectors that already have the scaling mode prop.    
> > > > 
> > > > Nouveau seems to expose both, and I don't see why we couldn't.    
> > > 
> > > First of all the interaction of these borders with panels that have
> > > a fixed mode would need to be properly specified. Do we apply the
> > > borders against the user mode or the panel's native mode?  
> > 
> > Hm, I'm a bit lost. Isn't crtc_state->mode supposed to contain the mode
> > that is about to be applied, no matter if it's a usermode or one of the
> > mode returned by the display?   
> 
> No. With fixed mode panels the user mode is a lie. Only
> hdisplay/vdisplay mean anything. The actual timings are known only to
> the kernel.

So now you say that the driver can silently ignore what the userspace
asked for and set what he thinks is appropriate based on what the HW
supports. This clearly contradicts the statement you make a few lines
above: driver should apply what userspace asked for or return an error
when it's not possible.

> Well, userspace can perhaps make an educated guess based on
> the connector's mode list and the presence of the scaling mode property.

But guess what? Where is the actual 'HW' mode stored if it's not in
crtc_state->mode? I mean, the drivers needs to know what to apply, and
almost all drivers are using atomic states to extract this information.

On a side note, I'd like to repeat what I already said before: I don't
care how this "define borders" feature is exposed to userspace, as long
as we have a way to express that the display we're using has borders
and the output image should be scaled+moved accordingly.

So now it's time to reach a consensus, because I'm not going to try and
guess what you expect. When I asked Daniel earlier this week whether he
wanted me to do things differently than I did in my v1 or try to match
what other drivers were already doing with the underscan prop, he told
me we should try to converge to a generic underscan solution that keeps
the existing userspace interface unchanged (basically what radeon,
amdgpu and nouveau drivers expose today). You don't seem to agree with
that. So now I'd like a clear answer, what should I do?
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Ville Syrjälä]

On Fri, May 11, 2018 at 09:47:49PM +0200, Boris Brezillon wrote:
> On Fri, 11 May 2018 20:29:48 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote:
> > > On Fri, 11 May 2018 19:54:02 +0300
> > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > >   
> > > > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote:  
> > > > > On Fri, 11 May 2018 18:34:50 +0300
> > > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > >     
> > > > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:    
> > > > > > > Applying an underscan setup is just a matter of scaling all planes
> > > > > > > appropriately and adjusting the CRTC X/Y offset to account for the
> > > > > > > horizontal and vertical border.
> > > > > > > 
> > > > > > > Create an vc4_plane_underscan_adj() function doing that and call it from
> > > > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
> > > > > > > underscan properties to the HDMI connector.
> > > > > > > 
> > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > > > > ---
> > > > > > > Changes in v2:
> > > > > > > - Take changes on hborder/vborder meaning into account
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++-
> > > > > > >  1 file changed, 48 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > > > > > > index 71d44c357d35..61ed60841cd6 100644
> > > > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > > > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > > > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
> > > > > > >  	}
> > > > > > >  }
> > > > > > >  
> > > > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
> > > > > > > +{
> > > > > > > +	struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
> > > > > > > +	struct drm_connector_state *conn_state = NULL;
> > > > > > > +	struct drm_connector *conn;
> > > > > > > +	struct drm_crtc_state *crtc_state;
> > > > > > > +	int i;
> > > > > > > +
> > > > > > > +	for_each_new_connector_in_state(pstate->state, conn, conn_state, i) {
> > > > > > > +		if (conn_state->crtc == pstate->crtc)
> > > > > > > +			break;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (i == pstate->state->num_connector)
> > > > > > > +		return 0;
> > > > > > > +
> > > > > > > +	if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
> > > > > > > +		return 0;
> > > > > > > +
> > > > > > > +	crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
> > > > > > > +						   pstate->crtc);
> > > > > > > +
> > > > > > > +	if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay ||
> > > > > > > +	    conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
> > > > > > > +		return -EINVAL;      
> > > > > > 
> > > > > > border * 2 ?    
> > > > > 
> > > > > Oops, indeed. I'll fix that.
> > > > >     
> > > > > >     
> > > > > > > +
> > > > > > > +	vc4_pstate->crtc_x += conn_state->underscan.hborder;
> > > > > > > +	vc4_pstate->crtc_y += conn_state->underscan.vborder;
> > > > > > > +	vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
> > > > > > > +			      (crtc_state->mode.hdisplay -
> > > > > > > +			       (conn_state->underscan.hborder * 2))) /
> > > > > > > +			     crtc_state->mode.hdisplay;
> > > > > > > +	vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
> > > > > > > +			      (crtc_state->mode.vdisplay -
> > > > > > > +			       (conn_state->underscan.vborder * 2))) /
> > > > > > > +			     crtc_state->mode.vdisplay;      
> > > > > > 
> > > > > > So you're now scaling all planes? The code seems to reject scaling for
> > > > > > the cursor plane, how are you dealing with that? Or just no cursor
> > > > > > allowed when underscanning?    
> > > > > 
> > > > > No, I just didn't test with a cursor plane. We should probably avoid
> > > > > scaling the cursor plane and just adjust its position. Eric any opinion
> > > > > on that?    
> > > > 
> > > > I don't think you can just not scale it. The user asked for the cursor
> > > > to be at a specific place with a specific size. Can't just ignore
> > > > that and do something else. Also eg. i915 would definitely scale the
> > > > cursor since we'd just scale the entire crtc instead of scaling
> > > > individual planes. Different drivers doing different things wouldn't
> > > > be good.  
> > > 
> > > Except in our case the scaling takes place before the composition, so
> > > we don't have a choice.  
> > 
> > The choice is to either do what userspace asked, or return an error.
> 
> Come on! If we can't use underscan when there's a cursor plane enabled
> this feature is pretty much useless. But let's take a real use case to
> show you how negligible the lack of scaling on the cursor plane will
> be. Say you have borders taking 10% of you screen (which is already a
> lot), and your cursor is a plane of 64x64 pixels, you'll end up with a
> 64x64 cursor instead of 58x58. Quite frankly, I doubt you'll notice
> the difference.

Now you're assuming the cursor is only ever used as a cursor. It can
be used for other things and those may need to be positioned pixel
perfect in relation to other planes/fb contents.

We used to play is fast and loose in i915 when it came to the sprite
plane coordinates. People generally hated that, at least when it came
to the atomic ioctl. Basically we just adjusted the src/dst
coordinates until the hw was happy with them, partially ignoring
what the user had asked. Maarten recently nuked that code, and so
now we either respect the user's choice or we return an error.

I guess one way out of this conundrum would be to allow the cursor
to violate the user's requested parameters when controlled via the
legacy cursor ioctls. There are no atomicity guarantees there, so
I guess we could also say there are no other correctness guarantees
either. Not sure if the accuracy of the hotspot might become an issue
though.

Another option might be to just scale the cursor as well. If I
understand correctly the "cursor can't be scaled" limitation just
comes from the fact that some vblank synced resource needs to be
reconfigured whenever the scaling changes. So doing that for
unthrottled cursor updates is not easy. But in this case the
underscan properties are what determines the scaling so maybe that
resource could be reconfigured whenever the those props change
to make sure the cursor can always be scaled appropriately?

> 
> Anyway, I'd like to hear back from Eric on that, since he is the one
> who asked me to work on this feature.
> 
> > 
> > >   
> > > >   
> > > > >     
> > > > > > 
> > > > > > I'm also wondering if there's any way we can reconcile these border
> > > > > > props with the scaling mode prop, should we ever wish to expose
> > > > > > these props on connectors that already have the scaling mode prop.    
> > > > > 
> > > > > Nouveau seems to expose both, and I don't see why we couldn't.    
> > > > 
> > > > First of all the interaction of these borders with panels that have
> > > > a fixed mode would need to be properly specified. Do we apply the
> > > > borders against the user mode or the panel's native mode?  
> > > 
> > > Hm, I'm a bit lost. Isn't crtc_state->mode supposed to contain the mode
> > > that is about to be applied, no matter if it's a usermode or one of the
> > > mode returned by the display?   
> > 
> > No. With fixed mode panels the user mode is a lie. Only
> > hdisplay/vdisplay mean anything. The actual timings are known only to
> > the kernel.
> 
> So now you say that the driver can silently ignore what the userspace
> asked for and set what he thinks is appropriate based on what the HW
> supports. This clearly contradicts the statement you make a few lines
> above: driver should apply what userspace asked for or return an error
> when it's not possible.

It's an unfortunate legacy uapi design we have to live with. I agree
that it would be much better if userspace had more explicit control
over the crtc scaling/borders/etc.

> 
> > Well, userspace can perhaps make an educated guess based on
> > the connector's mode list and the presence of the scaling mode property.
> 
> But guess what? Where is the actual 'HW' mode stored if it's not in
> crtc_state->mode? I mean, the drivers needs to know what to apply, and
> almost all drivers are using atomic states to extract this information.

adjusted_mode is there for the driver to do as they wish. Eg. in i915
LVDS/eDP/DSI/etc. encoders will overwrite the adjusted_mode with the
panel's native mode at the appropriate time (for other encoder types
we just populate adjusted_mode with the user mode).

For the user it will still *look* like the mode they asked for (ie.
hdisplay/vdisplay determine how many visible pixels there are), but
internally we've replaced the timings with something different and
there may or may not scaling involved (depend on the scaling mode
property).

> 
> On a side note, I'd like to repeat what I already said before: I don't
> care how this "define borders" feature is exposed to userspace, as long
> as we have a way to express that the display we're using has borders
> and the output image should be scaled+moved accordingly.
> 
> So now it's time to reach a consensus, because I'm not going to try and
> guess what you expect. When I asked Daniel earlier this week whether he
> wanted me to do things differently than I did in my v1 or try to match
> what other drivers were already doing with the underscan prop, he told
> me we should try to converge to a generic underscan solution that keeps
> the existing userspace interface unchanged (basically what radeon,
> amdgpu and nouveau drivers expose today). You don't seem to agree with
> that. So now I'd like a clear answer, what should I do?

What do those drivers do when it comes to planes? I would expect that
they just scale the entire crtc instead of individual planes.

-- 
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 7348 bytes --]

Ville Syrjälä <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> writes:

> On Fri, May 11, 2018 at 09:47:49PM +0200, Boris Brezillon wrote:
>> On Fri, 11 May 2018 20:29:48 +0300
>> Ville Syrjälä <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> 
>> > On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote:
>> > > On Fri, 11 May 2018 19:54:02 +0300
>> > > Ville Syrjälä <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> > >   
>> > > > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote:  
>> > > > > On Fri, 11 May 2018 18:34:50 +0300
>> > > > > Ville Syrjälä <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> > > > >     
>> > > > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:    
>> > > > > > > Applying an underscan setup is just a matter of scaling all planes
>> > > > > > > appropriately and adjusting the CRTC X/Y offset to account for the
>> > > > > > > horizontal and vertical border.
>> > > > > > > 
>> > > > > > > Create an vc4_plane_underscan_adj() function doing that and call it from
>> > > > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
>> > > > > > > underscan properties to the HDMI connector.
>> > > > > > > 
>> > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
>> > > > > > > ---
>> > > > > > > Changes in v2:
>> > > > > > > - Take changes on hborder/vborder meaning into account
>> > > > > > > ---
>> > > > > > >  drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++-
>> > > > > > >  1 file changed, 48 insertions(+), 1 deletion(-)
>> > > > > > > 
>> > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > > > index 71d44c357d35..61ed60841cd6 100644
>> > > > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
>> > > > > > >  	}
>> > > > > > >  }
>> > > > > > >  
>> > > > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
>> > > > > > > +{
>> > > > > > > +	struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
>> > > > > > > +	struct drm_connector_state *conn_state = NULL;
>> > > > > > > +	struct drm_connector *conn;
>> > > > > > > +	struct drm_crtc_state *crtc_state;
>> > > > > > > +	int i;
>> > > > > > > +
>> > > > > > > +	for_each_new_connector_in_state(pstate->state, conn, conn_state, i) {
>> > > > > > > +		if (conn_state->crtc == pstate->crtc)
>> > > > > > > +			break;
>> > > > > > > +	}
>> > > > > > > +
>> > > > > > > +	if (i == pstate->state->num_connector)
>> > > > > > > +		return 0;
>> > > > > > > +
>> > > > > > > +	if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
>> > > > > > > +		return 0;
>> > > > > > > +
>> > > > > > > +	crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
>> > > > > > > +						   pstate->crtc);
>> > > > > > > +
>> > > > > > > +	if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay ||
>> > > > > > > +	    conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
>> > > > > > > +		return -EINVAL;      
>> > > > > > 
>> > > > > > border * 2 ?    
>> > > > > 
>> > > > > Oops, indeed. I'll fix that.
>> > > > >     
>> > > > > >     
>> > > > > > > +
>> > > > > > > +	vc4_pstate->crtc_x += conn_state->underscan.hborder;
>> > > > > > > +	vc4_pstate->crtc_y += conn_state->underscan.vborder;
>> > > > > > > +	vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
>> > > > > > > +			      (crtc_state->mode.hdisplay -
>> > > > > > > +			       (conn_state->underscan.hborder * 2))) /
>> > > > > > > +			     crtc_state->mode.hdisplay;
>> > > > > > > +	vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
>> > > > > > > +			      (crtc_state->mode.vdisplay -
>> > > > > > > +			       (conn_state->underscan.vborder * 2))) /
>> > > > > > > +			     crtc_state->mode.vdisplay;      
>> > > > > > 
>> > > > > > So you're now scaling all planes? The code seems to reject scaling for
>> > > > > > the cursor plane, how are you dealing with that? Or just no cursor
>> > > > > > allowed when underscanning?    
>> > > > > 
>> > > > > No, I just didn't test with a cursor plane. We should probably avoid
>> > > > > scaling the cursor plane and just adjust its position. Eric any opinion
>> > > > > on that?    
>> > > > 
>> > > > I don't think you can just not scale it. The user asked for the cursor
>> > > > to be at a specific place with a specific size. Can't just ignore
>> > > > that and do something else. Also eg. i915 would definitely scale the
>> > > > cursor since we'd just scale the entire crtc instead of scaling
>> > > > individual planes. Different drivers doing different things wouldn't
>> > > > be good.  
>> > > 
>> > > Except in our case the scaling takes place before the composition, so
>> > > we don't have a choice.  
>> > 
>> > The choice is to either do what userspace asked, or return an error.
>> 
>> Come on! If we can't use underscan when there's a cursor plane enabled
>> this feature is pretty much useless. But let's take a real use case to
>> show you how negligible the lack of scaling on the cursor plane will
>> be. Say you have borders taking 10% of you screen (which is already a
>> lot), and your cursor is a plane of 64x64 pixels, you'll end up with a
>> 64x64 cursor instead of 58x58. Quite frankly, I doubt you'll notice
>> the difference.
>
> Now you're assuming the cursor is only ever used as a cursor. It can
> be used for other things and those may need to be positioned pixel
> perfect in relation to other planes/fb contents.
>
> We used to play is fast and loose in i915 when it came to the sprite
> plane coordinates. People generally hated that, at least when it came
> to the atomic ioctl. Basically we just adjusted the src/dst
> coordinates until the hw was happy with them, partially ignoring
> what the user had asked. Maarten recently nuked that code, and so
> now we either respect the user's choice or we return an error.
>
> I guess one way out of this conundrum would be to allow the cursor
> to violate the user's requested parameters when controlled via the
> legacy cursor ioctls. There are no atomicity guarantees there, so
> I guess we could also say there are no other correctness guarantees
> either. Not sure if the accuracy of the hotspot might become an issue
> though.
>
> Another option might be to just scale the cursor as well. If I
> understand correctly the "cursor can't be scaled" limitation just
> comes from the fact that some vblank synced resource needs to be
> reconfigured whenever the scaling changes. So doing that for
> unthrottled cursor updates is not easy. But in this case the
> underscan properties are what determines the scaling so maybe that
> resource could be reconfigured whenever the those props change
> to make sure the cursor can always be scaled appropriately?

Yeah, we had no application for it, so it wasn't supported.  I don't
think it would be hard to have a previously-scaled cursor move, it's
just that we need to fall back to a synchronous plane update if the
scaling parameters change.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 5821 bytes --]

Ville Syrjälä <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> writes:

> On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote:
>> On Fri, 11 May 2018 19:54:02 +0300
>> Ville Syrjälä <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> 
>> > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote:
>> > > On Fri, 11 May 2018 18:34:50 +0300
>> > > Ville Syrjälä <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> > >   
>> > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:  
>> > > > > Applying an underscan setup is just a matter of scaling all planes
>> > > > > appropriately and adjusting the CRTC X/Y offset to account for the
>> > > > > horizontal and vertical border.
>> > > > > 
>> > > > > Create an vc4_plane_underscan_adj() function doing that and call it from
>> > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
>> > > > > underscan properties to the HDMI connector.
>> > > > > 
>> > > > > Signed-off-by: Boris Brezillon <boris.brezillon-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
>> > > > > ---
>> > > > > Changes in v2:
>> > > > > - Take changes on hborder/vborder meaning into account
>> > > > > ---
>> > > > >  drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++-
>> > > > >  1 file changed, 48 insertions(+), 1 deletion(-)
>> > > > > 
>> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > index 71d44c357d35..61ed60841cd6 100644
>> > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
>> > > > >  	}
>> > > > >  }
>> > > > >  
>> > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
>> > > > > +{
>> > > > > +	struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
>> > > > > +	struct drm_connector_state *conn_state = NULL;
>> > > > > +	struct drm_connector *conn;
>> > > > > +	struct drm_crtc_state *crtc_state;
>> > > > > +	int i;
>> > > > > +
>> > > > > +	for_each_new_connector_in_state(pstate->state, conn, conn_state, i) {
>> > > > > +		if (conn_state->crtc == pstate->crtc)
>> > > > > +			break;
>> > > > > +	}
>> > > > > +
>> > > > > +	if (i == pstate->state->num_connector)
>> > > > > +		return 0;
>> > > > > +
>> > > > > +	if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
>> > > > > +		return 0;
>> > > > > +
>> > > > > +	crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
>> > > > > +						   pstate->crtc);
>> > > > > +
>> > > > > +	if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay ||
>> > > > > +	    conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
>> > > > > +		return -EINVAL;    
>> > > > 
>> > > > border * 2 ?  
>> > > 
>> > > Oops, indeed. I'll fix that.
>> > >   
>> > > >   
>> > > > > +
>> > > > > +	vc4_pstate->crtc_x += conn_state->underscan.hborder;
>> > > > > +	vc4_pstate->crtc_y += conn_state->underscan.vborder;
>> > > > > +	vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
>> > > > > +			      (crtc_state->mode.hdisplay -
>> > > > > +			       (conn_state->underscan.hborder * 2))) /
>> > > > > +			     crtc_state->mode.hdisplay;
>> > > > > +	vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
>> > > > > +			      (crtc_state->mode.vdisplay -
>> > > > > +			       (conn_state->underscan.vborder * 2))) /
>> > > > > +			     crtc_state->mode.vdisplay;    
>> > > > 
>> > > > So you're now scaling all planes? The code seems to reject scaling for
>> > > > the cursor plane, how are you dealing with that? Or just no cursor
>> > > > allowed when underscanning?  
>> > > 
>> > > No, I just didn't test with a cursor plane. We should probably avoid
>> > > scaling the cursor plane and just adjust its position. Eric any opinion
>> > > on that?  
>> > 
>> > I don't think you can just not scale it. The user asked for the cursor
>> > to be at a specific place with a specific size. Can't just ignore
>> > that and do something else. Also eg. i915 would definitely scale the
>> > cursor since we'd just scale the entire crtc instead of scaling
>> > individual planes. Different drivers doing different things wouldn't
>> > be good.
>> 
>> Except in our case the scaling takes place before the composition, so
>> we don't have a choice.
>
> The choice is to either do what userspace asked, or return an error.
>
>> 
>> > 
>> > >   
>> > > > 
>> > > > I'm also wondering if there's any way we can reconcile these border
>> > > > props with the scaling mode prop, should we ever wish to expose
>> > > > these props on connectors that already have the scaling mode prop.  
>> > > 
>> > > Nouveau seems to expose both, and I don't see why we couldn't.  
>> > 
>> > First of all the interaction of these borders with panels that have
>> > a fixed mode would need to be properly specified. Do we apply the
>> > borders against the user mode or the panel's native mode?
>> 
>> Hm, I'm a bit lost. Isn't crtc_state->mode supposed to contain the mode
>> that is about to be applied, no matter if it's a usermode or one of the
>> mode returned by the display? 
>
> No. With fixed mode panels the user mode is a lie. Only
> hdisplay/vdisplay mean anything. The actual timings are known only to
> the kernel. Well, userspace can perhaps make an educated guess based on
> the connector's mode list and the presence of the scaling mode property.

Let's just not expose the property on devices with fixed mode panels.
Problem solved because all this property is is a way to work around HDMI
monitors doing something awful, and trying to build something more
general isn't serving anyone.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2 4/4] drm/nouveau: Switch to the generic underscan props

[Boris Brezillon]

Now that underscan props can be parsed by the core and assigned to
conn_state->underscan.xxx, we can rely on this implementation and get
rid of the nouveau-specific underscan props.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 39 +++++------------------------
 drivers/gpu/drm/nouveau/nouveau_connector.h |  9 -------
 drivers/gpu/drm/nouveau/nouveau_display.c   | 14 -----------
 drivers/gpu/drm/nouveau/nouveau_display.h   |  3 ---
 drivers/gpu/drm/nouveau/nv50_display.c      | 17 +++++++++----
 5 files changed, 18 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 6ed9cb053dfa..0ce055d3b89e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -105,12 +105,6 @@ nouveau_conn_atomic_get_property(struct drm_connector *connector,
 
 	if (property == dev->mode_config.scaling_mode_property)
 		*val = asyc->scaler.mode;
-	else if (property == disp->underscan_property)
-		*val = asyc->scaler.underscan.mode;
-	else if (property == disp->underscan_hborder_property)
-		*val = asyc->scaler.underscan.hborder;
-	else if (property == disp->underscan_vborder_property)
-		*val = asyc->scaler.underscan.vborder;
 	else if (property == disp->dithering_mode)
 		*val = asyc->dither.mode;
 	else if (property == disp->dithering_depth)
@@ -170,24 +164,6 @@ nouveau_conn_atomic_set_property(struct drm_connector *connector,
 			asyc->set.scaler = true;
 		}
 	} else
-	if (property == disp->underscan_property) {
-		if (asyc->scaler.underscan.mode != val) {
-			asyc->scaler.underscan.mode = val;
-			asyc->set.scaler = true;
-		}
-	} else
-	if (property == disp->underscan_hborder_property) {
-		if (asyc->scaler.underscan.hborder != val) {
-			asyc->scaler.underscan.hborder = val;
-			asyc->set.scaler = true;
-		}
-	} else
-	if (property == disp->underscan_vborder_property) {
-		if (asyc->scaler.underscan.vborder != val) {
-			asyc->scaler.underscan.vborder = val;
-			asyc->set.scaler = true;
-		}
-	} else
 	if (property == disp->dithering_mode) {
 		if (asyc->dither.mode != val) {
 			asyc->dither.mode = val;
@@ -256,7 +232,6 @@ nouveau_conn_reset(struct drm_connector *connector)
 	asyc->dither.mode = DITHERING_MODE_AUTO;
 	asyc->dither.depth = DITHERING_DEPTH_AUTO;
 	asyc->scaler.mode = DRM_MODE_SCALE_NONE;
-	asyc->scaler.underscan.mode = UNDERSCAN_OFF;
 	asyc->procamp.color_vibrance = 150;
 	asyc->procamp.vibrant_hue = 90;
 
@@ -285,18 +260,16 @@ nouveau_conn_attach_properties(struct drm_connector *connector)
 					   dvi_i_subconnector_property, 0);
 
 	/* Add overscan compensation options to digital outputs. */
-	if (disp->underscan_property &&
+	if (disp->disp.oclass >= NV50_DISP &&
 	    (connector->connector_type == DRM_MODE_CONNECTOR_DVID ||
 	     connector->connector_type == DRM_MODE_CONNECTOR_DVII ||
 	     connector->connector_type == DRM_MODE_CONNECTOR_HDMIA ||
 	     connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)) {
-		drm_object_attach_property(&connector->base,
-					   disp->underscan_property,
-					   UNDERSCAN_OFF);
-		drm_object_attach_property(&connector->base,
-					   disp->underscan_hborder_property, 0);
-		drm_object_attach_property(&connector->base,
-					   disp->underscan_vborder_property, 0);
+		WARN_ON(drm_connector_attach_underscan_properties(connector,
+						BIT(DRM_UNDERSCAN_OFF) |
+						BIT(DRM_UNDERSCAN_ON) |
+						BIT(DRM_UNDERSCAN_AUTO),
+						128, 128));
 	}
 
 	/* Add hue and saturation options. */
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h b/drivers/gpu/drm/nouveau/nouveau_connector.h
index a4d1a059bd3d..1d3ec65288e1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.h
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.h
@@ -111,15 +111,6 @@ struct nouveau_conn_atom {
 
 	struct {
 		int mode;	/* DRM_MODE_SCALE_* */
-		struct {
-			enum {
-				UNDERSCAN_OFF,
-				UNDERSCAN_ON,
-				UNDERSCAN_AUTO,
-			} mode;
-			u32 hborder;
-			u32 vborder;
-		} underscan;
 		bool full;
 	} scaler;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 7d0bec8dd03d..002c2b13571b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -303,13 +303,6 @@ struct nouveau_drm_prop_enum_list {
 	char *name;
 };
 
-static struct nouveau_drm_prop_enum_list underscan[] = {
-	{ 6, UNDERSCAN_AUTO, "auto" },
-	{ 6, UNDERSCAN_OFF, "off" },
-	{ 6, UNDERSCAN_ON, "on" },
-	{}
-};
-
 static struct nouveau_drm_prop_enum_list dither_mode[] = {
 	{ 7, DITHERING_MODE_AUTO, "auto" },
 	{ 7, DITHERING_MODE_OFF, "off" },
@@ -464,13 +457,6 @@ nouveau_display_create_properties(struct drm_device *dev)
 
 	PROP_ENUM(disp->dithering_mode, gen, "dithering mode", dither_mode);
 	PROP_ENUM(disp->dithering_depth, gen, "dithering depth", dither_depth);
-	PROP_ENUM(disp->underscan_property, gen, "underscan", underscan);
-
-	disp->underscan_hborder_property =
-		drm_property_create_range(dev, 0, "underscan hborder", 0, 128);
-
-	disp->underscan_vborder_property =
-		drm_property_create_range(dev, 0, "underscan vborder", 0, 128);
 
 	if (gen < 1)
 		return;
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
index 270ba56f2756..df2f57d72fa9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.h
+++ b/drivers/gpu/drm/nouveau/nouveau_display.h
@@ -42,9 +42,6 @@ struct nouveau_display {
 
 	struct drm_property *dithering_mode;
 	struct drm_property *dithering_depth;
-	struct drm_property *underscan_property;
-	struct drm_property *underscan_hborder_property;
-	struct drm_property *underscan_vborder_property;
 	/* not really hue and saturation: */
 	struct drm_property *vibrant_hue_property;
 	struct drm_property *color_vibrance_property;
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
index 8bd739cfd00d..7c7dbf3bd3a0 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -2057,11 +2057,11 @@ nv50_head_atomic_check_view(struct nv50_head_atom *armh,
 	 * ratio the same as the backend mode unless overridden by the
 	 * user setting both hborder and vborder properties.
 	 */
-	if ((asyc->scaler.underscan.mode == UNDERSCAN_ON ||
-	    (asyc->scaler.underscan.mode == UNDERSCAN_AUTO &&
+	if ((asyc->state.underscan.mode == DRM_UNDERSCAN_ON ||
+	    (asyc->state.underscan.mode == DRM_UNDERSCAN_AUTO &&
 	     drm_detect_hdmi_monitor(edid)))) {
-		u32 bX = asyc->scaler.underscan.hborder;
-		u32 bY = asyc->scaler.underscan.vborder;
+		u32 bX = asyc->state.underscan.hborder;
+		u32 bY = asyc->state.underscan.vborder;
 		u32 r = (asyh->view.oH << 19) / asyh->view.oW;
 
 		if (bX) {
@@ -2185,8 +2185,8 @@ nv50_head_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state)
 	struct nv50_head *head = nv50_head(crtc);
 	struct nv50_head_atom *armh = nv50_head_atom(crtc->state);
 	struct nv50_head_atom *asyh = nv50_head_atom(state);
+	struct drm_connector_state *conns, *oldconns;
 	struct nouveau_conn_atom *asyc = NULL;
-	struct drm_connector_state *conns;
 	struct drm_connector *conn;
 	int i;
 
@@ -2199,6 +2199,13 @@ nv50_head_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state)
 			}
 		}
 
+		for_each_oldnew_connector_in_state(asyh->state.state, conn,
+						   oldconns, conns, i) {
+			if (memcmp(&oldconns->underscan, &conns->underscan,
+				   sizeof(conns->underscan)))
+				asyc->set.scaler = true;
+		}
+
 		if (armh->state.active) {
 			if (asyc) {
 				if (asyh->state.mode_changed)
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v2 3/4] drm/vc4: Attach underscan props to the HDMI connector

[Boris Brezillon]

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

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

Note that these underscan 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 <boris.brezillon@bootlin.com>
---
Changes in v2:
- none
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 1a6db291d48b..17464b5981f9 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -323,6 +323,16 @@ 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);
 
+	/* The hborder and vborder limit is arbitrarily set to 1024 which
+	 * should be more than enough for real use cases. Note that the actual
+	 * limitation comes from the display mode:
+	 *	hborder < hdisplay && vborder < vdisplay
+	 */
+	drm_connector_attach_underscan_properties(connector,
+						  BIT(DRM_UNDERSCAN_OFF) |
+						  BIT(DRM_UNDERSCAN_ON),
+						  1024, 1024);
+
 	connector->polled = (DRM_CONNECTOR_POLL_CONNECT |
 			     DRM_CONNECTOR_POLL_DISCONNECT);
 
@@ -408,6 +418,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 +439,18 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)
 					   vc4_encoder->rgb_range_selectable,
 					   false);
 
+	if (cstate->underscan.mode == DRM_UNDERSCAN_ON) {
+		if (cstate->underscan.hborder) {
+			frame.avi.right_bar = cstate->underscan.hborder / 2;
+			frame.avi.left_bar = frame.avi.right_bar;
+		}
+
+		if (cstate->underscan.vborder) {
+			frame.avi.top_bar = cstate->underscan.vborder / 2;
+			frame.avi.bottom_bar = frame.avi.top_bar;
+		}
+	}
+
 	vc4_hdmi_write_infoframe(encoder, &frame);
 }
 
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel