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).
> + *
> + * 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.
How is the output scaled? What does the user mode hdisplay/vdisplay mean
in this case? What if I want underscan without scaling?
> + *	auto:
> + *		Underscan is activated and horizontal and vertical borders are
> + *		automatically chosen by the driver.
Seems overly vague to be useful. You didn't even seem to implement it
for vc4.
> + *
> + * 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.
Seems like a slightly odd way to specify this. I think for the TV margins
we have one value for each edge.
> + *
> + * 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 {
> +	/**
> +	 * @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;
> +
>  /* 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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Ville Syrjälä
Intel

On Mon, 7 May 2018 18:01:44 +0300
Ville Syrjälä <ville.syrjala at linux.intel.com> 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).
> > + *
> > + * 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.  
> 
> How is the output scaled?
In HW. The formula is

	hfactor = (hdisplay - hborder) / hdisplay
	vfactor = (vdisplay - vborder) / vdisplay
> What does the user mode hdisplay/vdisplay mean
> in this case?
The same as before this patch: the output resolution. You just add
black margins.
> What if I want underscan without scaling?
Then don't involve the DRM driver and do that from userspace: just
fill the visible portion of the framebuffer and leave the rest black.
There nothing the DRM driver can do to help with that, except maybe
exposing the information about the active area of the screen. It would
be nice to do that, but that means patching all userspace libs to take
this into account.
> 
> > + *	auto:
> > + *		Underscan is activated and horizontal and vertical borders are
> > + *		automatically chosen by the driver.  
> 
> Seems overly vague to be useful. You didn't even seem to implement it
> for vc4.
Because I don't understand it either. I was just trying to keep things
working for drivers already exposing these properties.
> 
> > + *
> > + * 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.  
> 
> Seems like a slightly odd way to specify this. I think for the TV margins
> we have one value for each edge.
Again, I just wanted existing drivers to keep working with the generic
solution, but maybe we shouldn't care.

On 2018-05-07 12:19 PM, Boris Brezillon wrote:
> On Mon, 7 May 2018 18:01:44 +0300
> Ville Syrjälä <ville.syrjala at linux.intel.com> 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).
>>> + *
>>> + * 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.  
>>
>> How is the output scaled?
> 
> In HW. The formula is
> 
> 	hfactor = (hdisplay - hborder) / hdisplay
> 	vfactor = (vdisplay - vborder) / vdisplay
> 
>> What does the user mode hdisplay/vdisplay mean
>> in this case?
> 
> The same as before this patch: the output resolution. You just add
> black margins.
> 
>> What if I want underscan without scaling?
> 
> Then don't involve the DRM driver and do that from userspace: just
> fill the visible portion of the framebuffer and leave the rest black.
> There nothing the DRM driver can do to help with that, except maybe
> exposing the information about the active area of the screen. It would
> be nice to do that, but that means patching all userspace libs to take
> this into account.
> 
>>
>>> + *	auto:
>>> + *		Underscan is activated and horizontal and vertical borders are
>>> + *		automatically chosen by the driver.  
>>
>> Seems overly vague to be useful. You didn't even seem to implement
it
>> for vc4.
> 
FWIW, amdgpu treats UNDERSCAN_AUTO like UNDERSCAN_ON. radeon and nouveau seem to
do the same. So there's probably no need for auto.

Harry
> Because I don't understand it either. I was just trying to keep things
> working for drivers already exposing these properties.
> 
>>
>>> + *
>>> + * 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.  
>>
>> Seems like a slightly odd way to specify this. I think for the TV
margins
>> we have one value for each edge.
> 
> Again, I just wanted existing drivers to keep working with the generic
> solution, but maybe we shouldn't care.
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>