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

Fri May 11 19:47:49 UTC 2018

On Fri, 11 May 2018 20:29:48 +0300
Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:

> On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote:
> > On Fri, 11 May 2018 19:54:02 +0300
> > Ville Syrjälä <ville.syrjala at 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 at 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 at 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.  
> 
> The choice is to either do what userspace asked, or return an error.

Come on! If we can't use underscan when there's a cursor plane enabled
this feature is pretty much useless. But let's take a real use case to
show you how negligible the lack of scaling on the cursor plane will
be. Say you have borders taking 10% of you screen (which is already a
lot), and your cursor is a plane of 64x64 pixels, you'll end up with a
64x64 cursor instead of 58x58. Quite frankly, I doubt you'll notice
the difference.

Anyway, I'd like to hear back from Eric on that, since he is the one
who asked me to work on this feature.

> 
> >   
> > >   
> > > >     
> > > > > 
> > > > > 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?   
> 
> No. With fixed mode panels the user mode is a lie. Only
> hdisplay/vdisplay mean anything. The actual timings are known only to
> the kernel.

So now you say that the driver can silently ignore what the userspace
asked for and set what he thinks is appropriate based on what the HW
supports. This clearly contradicts the statement you make a few lines
above: driver should apply what userspace asked for or return an error
when it's not possible.

> Well, userspace can perhaps make an educated guess based on
> the connector's mode list and the presence of the scaling mode property.

But guess what? Where is the actual 'HW' mode stored if it's not in
crtc_state->mode? I mean, the drivers needs to know what to apply, and
almost all drivers are using atomic states to extract this information.

On a side note, I'd like to repeat what I already said before: I don't
care how this "define borders" feature is exposed to userspace, as long
as we have a way to express that the display we're using has borders
and the output image should be scaled+moved accordingly.

So now it's time to reach a consensus, because I'm not going to try and
guess what you expect. When I asked Daniel earlier this week whether he
wanted me to do things differently than I did in my v1 or try to match
what other drivers were already doing with the underscan prop, he told
me we should try to converge to a generic underscan solution that keeps
the existing userspace interface unchanged (basically what radeon,
amdgpu and nouveau drivers expose today). You don't seem to agree with
that. So now I'd like a clear answer, what should I do?