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

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
>

On 8 May 2018 at 04:26, Harry Wentland <harry.wentland at amd.com>
wrote:
>
>
> 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.
They're not the same.  UNDERSCAN_AUTO in both nouveau and radeon
attempt to enable it by default for HDMI displays that would otherwise
chop the edges off the displayed image.  Whereas UNDERSCAN_ON is
unconditional.

Ben.
>
> 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
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, 8 May 2018 10:18:10 +1000
Ben Skeggs <skeggsb at gmail.com> wrote:
> On 8 May 2018 at 04:26, Harry Wentland <harry.wentland at amd.com>
wrote:
> >
> >
> > 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.
> They're not the same.  UNDERSCAN_AUTO in both nouveau and radeon
> attempt to enable it by default for HDMI displays that would otherwise
> chop the edges off the displayed image.  Whereas UNDERSCAN_ON is
> unconditional.
Actually, It's also true for amdgpu, I just didn't notice that when I
first read the code (so many parenthesis that I mixed the || and &&
scope).