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

[Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 7348 bytes --]

Ville Syrjälä <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> writes:

> On Fri, May 11, 2018 at 09:47:49PM +0200, Boris Brezillon wrote:
>> On Fri, 11 May 2018 20:29:48 +0300
>> Ville Syrjälä <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
>> > > > > > > ---
>> > > > > > > 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.
>
> Now you're assuming the cursor is only ever used as a cursor. It can
> be used for other things and those may need to be positioned pixel
> perfect in relation to other planes/fb contents.
>
> We used to play is fast and loose in i915 when it came to the sprite
> plane coordinates. People generally hated that, at least when it came
> to the atomic ioctl. Basically we just adjusted the src/dst
> coordinates until the hw was happy with them, partially ignoring
> what the user had asked. Maarten recently nuked that code, and so
> now we either respect the user's choice or we return an error.
>
> I guess one way out of this conundrum would be to allow the cursor
> to violate the user's requested parameters when controlled via the
> legacy cursor ioctls. There are no atomicity guarantees there, so
> I guess we could also say there are no other correctness guarantees
> either. Not sure if the accuracy of the hotspot might become an issue
> though.
>
> Another option might be to just scale the cursor as well. If I
> understand correctly the "cursor can't be scaled" limitation just
> comes from the fact that some vblank synced resource needs to be
> reconfigured whenever the scaling changes. So doing that for
> unthrottled cursor updates is not easy. But in this case the
> underscan properties are what determines the scaling so maybe that
> resource could be reconfigured whenever the those props change
> to make sure the cursor can always be scaled appropriately?

Yeah, we had no application for it, so it wasn't supported.  I don't
think it would be hard to have a previously-scaled cursor move, it's
just that we need to fall back to a synchronous plane update if the
scaling parameters change.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau