Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

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

On Fri, 11 May 2018 19:54:02 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote:
> > On Fri, 11 May 2018 18:34:50 +0300
> > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >   
> > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:  
> > > > Applying an underscan setup is just a matter of scaling all planes
> > > > appropriately and adjusting the CRTC X/Y offset to account for the
> > > > horizontal and vertical border.
> > > > 
> > > > Create an vc4_plane_underscan_adj() function doing that and call it from
> > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
> > > > underscan properties to the HDMI connector.
> > > > 
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > ---
> > > > Changes in v2:
> > > > - Take changes on hborder/vborder meaning into account
> > > > ---
> > > >  drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 48 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > > > index 71d44c357d35..61ed60841cd6 100644
> > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
> > > >  	}
> > > >  }
> > > >  
> > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
> > > > +{
> > > > +	struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
> > > > +	struct drm_connector_state *conn_state = NULL;
> > > > +	struct drm_connector *conn;
> > > > +	struct drm_crtc_state *crtc_state;
> > > > +	int i;
> > > > +
> > > > +	for_each_new_connector_in_state(pstate->state, conn, conn_state, i) {
> > > > +		if (conn_state->crtc == pstate->crtc)
> > > > +			break;
> > > > +	}
> > > > +
> > > > +	if (i == pstate->state->num_connector)
> > > > +		return 0;
> > > > +
> > > > +	if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
> > > > +		return 0;
> > > > +
> > > > +	crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
> > > > +						   pstate->crtc);
> > > > +
> > > > +	if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay ||
> > > > +	    conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
> > > > +		return -EINVAL;    
> > > 
> > > border * 2 ?  
> > 
> > Oops, indeed. I'll fix that.
> >   
> > >   
> > > > +
> > > > +	vc4_pstate->crtc_x += conn_state->underscan.hborder;
> > > > +	vc4_pstate->crtc_y += conn_state->underscan.vborder;
> > > > +	vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
> > > > +			      (crtc_state->mode.hdisplay -
> > > > +			       (conn_state->underscan.hborder * 2))) /
> > > > +			     crtc_state->mode.hdisplay;
> > > > +	vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
> > > > +			      (crtc_state->mode.vdisplay -
> > > > +			       (conn_state->underscan.vborder * 2))) /
> > > > +			     crtc_state->mode.vdisplay;    
> > > 
> > > So you're now scaling all planes? The code seems to reject scaling for
> > > the cursor plane, how are you dealing with that? Or just no cursor
> > > allowed when underscanning?  
> > 
> > No, I just didn't test with a cursor plane. We should probably avoid
> > scaling the cursor plane and just adjust its position. Eric any opinion
> > on that?  
> 
> I don't think you can just not scale it. The user asked for the cursor
> to be at a specific place with a specific size. Can't just ignore
> that and do something else. Also eg. i915 would definitely scale the
> cursor since we'd just scale the entire crtc instead of scaling
> individual planes. Different drivers doing different things wouldn't
> be good.

Except in our case the scaling takes place before the composition, so
we don't have a choice.

> 
> >   
> > > 
> > > I'm also wondering if there's any way we can reconcile these border
> > > props with the scaling mode prop, should we ever wish to expose
> > > these props on connectors that already have the scaling mode prop.  
> > 
> > Nouveau seems to expose both, and I don't see why we couldn't.  
> 
> First of all the interaction of these borders with panels that have
> a fixed mode would need to be properly specified. Do we apply the
> borders against the user mode or the panel's native mode?

Hm, I'm a bit lost. Isn't crtc_state->mode supposed to contain the mode
that is about to be applied, no matter if it's a usermode or one of the
mode returned by the display? 

> I guess
> the latter would be a bit easier (would also match how the TV margins
> behave I suppose). But that does leave open the question of how
> would generic userspace know which case it's dealing with? Eg. if it
> wants to underscan by some specific percentage it has to calculate
> the borders based on the correct mode, otherwise the results aren't
> going to match the expectations.

I don't get it, sorry. Borders are relative to the mode applied by
userspace. So if it needs to express borders as a ratio of the
resolution, then for sure userspace will have to do the math, but I
don't see the problem here.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx