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

[Ben Skeggs]

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