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

[Boris Brezillon <boris.brezillon-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>]

On Mon, 7 May 2018 18:01:44 +0300
Ville Syrjälä <ville.syrjala@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@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.

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.

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx