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

[Harry Wentland]

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
>