Nouveau - May 2018 - [PATCH 1/3] drm/connector: Add generic underscan properties

On Mon, May 07, 2018 at 04:44:32PM +0200, Boris Brezillon
wrote:
> 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 at bootlin.com>
> ---
>  drivers/gpu/drm/drm_atomic.c    |  12 ++++
>  drivers/gpu/drm/drm_connector.c | 120
++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h     |  78 ++++++++++++++++++++++++++
>  3 files changed, 210 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 dfc8ca1e9413..9937390b8a25 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -914,6 +914,31 @@ 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).
I think a new section here would be good, to make it more obvious this is
a group of properties. Plus a reference to
drm_connector_attach_underscan_properties().
> + *
> + * 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 and horizontal and vertical borders are
> + *		automatically chosen by the driver.
> + *
> + * underscan hborder:
> + *	Horizontal border expressed in pixels. The border is symmetric, which
> + *	means you'll have half of this value placed on the left and the
other
> + *	half on the right.
> + *
> + * underscan vborder:
> + *	Vertical border expressed in pixels. The border is symmetric, which
> + *	means you'll have half of this value placed on the top and the
other
> + *	half on the bottom.
>   */
>  
>  int drm_connector_create_standard_properties(struct drm_device *dev)
> @@ -1108,6 +1133,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..46380ee6d4ee 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -372,6 +372,52 @@ 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 - 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 {
I'd call this drm_underscan_state, similar to how we have
drm_tv_connector_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 +475,12 @@ 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 is doing overscan.
> +	 */
> +	struct drm_underscan underscan;
>  };
>  
>  /**
> @@ -892,6 +944,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;
Since you don't allow drivers to have a per-connector value range for
these properties these should be in drm_mode_config I think.

Yes the content_protection_property should probably have the same
treatment. Note that drm_property_blob does not derive from drm_property,
so that one is ok to be there. And the scaling property is generally
per-output.
-Daniel
> +
>  /* should we poll this connector for connects and disconnects */
>  /* hot plug detectable */
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> @@ -1088,6 +1163,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
> 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Mon, May 07, 2018 at 05:15:33PM +0200, Daniel Vetter
wrote:
> On Mon, May 07, 2018 at 04:44:32PM +0200, Boris Brezillon wrote:
> > 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 at bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_atomic.c    |  12 ++++
> >  drivers/gpu/drm/drm_connector.c | 120
++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h     |  78 ++++++++++++++++++++++++++
> >  3 files changed, 210 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 dfc8ca1e9413..9937390b8a25 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -914,6 +914,31 @@ 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).
> 
> I think a new section here would be good, to make it more obvious this is
> a group of properties. Plus a reference to
> drm_connector_attach_underscan_properties().
> 
> > + *
> > + * 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 and horizontal and vertical borders are
> > + *		automatically chosen by the driver.
> > + *
> > + * underscan hborder:
> > + *	Horizontal border expressed in pixels. The border is symmetric,
which
> > + *	means you'll have half of this value placed on the left and
the other
> > + *	half on the right.
> > + *
> > + * underscan vborder:
> > + *	Vertical border expressed in pixels. The border is symmetric,
which
> > + *	means you'll have half of this value placed on the top and the
other
> > + *	half on the bottom.
> >   */
> >  
> >  int drm_connector_create_standard_properties(struct drm_device *dev)
> > @@ -1108,6 +1133,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..46380ee6d4ee 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -372,6 +372,52 @@ 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 - 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 {
> 
> I'd call this drm_underscan_state, similar to how we have
> drm_tv_connector_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 +475,12 @@ 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 is doing overscan.
> > +	 */
> > +	struct drm_underscan underscan;
> >  };
> >  
> >  /**
> > @@ -892,6 +944,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;
> 
> Since you don't allow drivers to have a per-connector value range for
> these properties these should be in drm_mode_config I think.
> 
> Yes the content_protection_property should probably have the same
> treatment. Note that drm_property_blob does not derive from drm_property,
> so that one is ok to be there. And the scaling property is generally
> per-output.
Ah just noticed that the underscan_mode_property is potentially
per-connector. I guess we can keep them here then.
-Daniel
> -Daniel
> 
> > +
> >  /* should we poll this connector for connects and disconnects */
> >  /* hot plug detectable */
> >  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> > @@ -1088,6 +1163,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
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Mon, 7 May 2018 17:25:30 +0200
Daniel Vetter <daniel at ffwll.ch> wrote:
> On Mon, May 07, 2018 at 05:15:33PM +0200, Daniel Vetter wrote:
> > On Mon, May 07, 2018 at 04:44:32PM +0200, Boris Brezillon wrote:  
> > > 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 at
bootlin.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c    |  12 ++++
> > >  drivers/gpu/drm/drm_connector.c | 120
++++++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_connector.h     |  78 ++++++++++++++++++++++++++
> > >  3 files changed, 210 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 dfc8ca1e9413..9937390b8a25 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -914,6 +914,31 @@
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).  
> > 
> > I think a new section here would be good, to make it more obvious this
is
> > a group of properties. Plus a reference to
> > drm_connector_attach_underscan_properties().
> >   
> > > + *
> > > + * 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 and horizontal and vertical borders
are
> > > + *		automatically chosen by the driver.
> > > + *
> > > + * underscan hborder:
> > > + *	Horizontal border expressed in pixels. The border is
symmetric, which
> > > + *	means you'll have half of this value placed on the left
and the other
> > > + *	half on the right.
> > > + *
> > > + * underscan vborder:
> > > + *	Vertical border expressed in pixels. The border is symmetric,
which
> > > + *	means you'll have half of this value placed on the top
and the other
> > > + *	half on the bottom.
> > >   */
> > >  
> > >  int drm_connector_create_standard_properties(struct drm_device
*dev)
> > > @@ -1108,6 +1133,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..46380ee6d4ee 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -372,6 +372,52 @@ 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 - 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 {  
> > 
> > I'd call this drm_underscan_state, similar to how we have
> > drm_tv_connector_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 +475,12 @@ 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 is doing overscan.
> > > +	 */
> > > +	struct drm_underscan underscan;
> > >  };
> > >  
> > >  /**
> > > @@ -892,6 +944,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;  
> > 
> > Since you don't allow drivers to have a per-connector value range
for
> > these properties these should be in drm_mode_config I think.
I do allow that. See the max_vborder, max_hborder args in the 
drm_connector_attach_underscan_properties() prototype.