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
>
