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

[Boris Brezillon]

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).