Re: [PATCH v2] drm: support hotspot for universal plane cursors

[Daniel Vetter]

On Tue, Nov 17, 2015 at 04:58:06PM +0000, John Keeping wrote:
> On Tue, 17 Nov 2015 17:29:35 +0100, Daniel Vetter wrote:
> 
> > On Tue, Nov 17, 2015 at 03:59:43PM +0000, John Keeping wrote:
> > > On Tue, 17 Nov 2015 17:39:32 +0200, Ville Syrjälä wrote:
> > >   
> > > > On Tue, Nov 17, 2015 at 03:05:34PM +0000, John Keeping wrote:  
> > > > > The request's hot_x and hot_y are set correctly for both
> > > > > DRM_IOCTL_MODE_CURSOR and DRM_IOCTL_MODE_CURSOR2 so we just
> > > > > need to save the values and then apply the offset to the cursor
> > > > > plane when the cursor moves.
> > > > > 
> > > > > Signed-off-by: John Keeping <john@xxxxxxxxxxxx>
> > > > > ---
> > > > > v2:
> > > > >   - add kerneldoc for hot_x and hot_y in struct drm_crtc
> > > > > 
> > > > >  drivers/gpu/drm/drm_crtc.c | 11 +++++++----
> > > > >  include/drm/drm_crtc.h     |  6 ++++++
> > > > >  2 files changed, 13 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_crtc.c
> > > > > b/drivers/gpu/drm/drm_crtc.c index 720a153..40f5b84 100644
> > > > > --- a/drivers/gpu/drm/drm_crtc.c
> > > > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > > > @@ -2831,6 +2831,9 @@ static int
> > > > > drm_mode_cursor_universal(struct drm_crtc *crtc,
> > > > > DRM_DEBUG_KMS("failed to wrap cursor buffer in drm
> > > > > framebuffer\n"); return PTR_ERR(fb); }
> > > > > +
> > > > > +			crtc->hot_x = req->hot_x;
> > > > > +			crtc->hot_y = req->hot_y;
> > > > >  		} else {
> > > > >  			fb = NULL;
> > > > >  		}
> > > > > @@ -2841,11 +2844,11 @@ static int
> > > > > drm_mode_cursor_universal(struct drm_crtc *crtc, }
> > > > >  
> > > > >  	if (req->flags & DRM_MODE_CURSOR_MOVE) {
> > > > > -		crtc_x = req->x;
> > > > > -		crtc_y = req->y;
> > > > > +		crtc_x = req->x - crtc->hot_x;
> > > > > +		crtc_y = req->y - crtc->hot_y;
> > > > >  	} else {
> > > > > -		crtc_x = crtc->cursor_x;
> > > > > -		crtc_y = crtc->cursor_y;
> > > > > +		crtc_x = crtc->cursor_x - crtc->hot_x;
> > > > > +		crtc_y = crtc->cursor_y - crtc->hot_y;    
> > > > 
> > > > Why does the location of the hotspot affect the plane position?  
> > > 
> > > hot_{x,y} specify the location of the active pixel within the cursor
> > > plane and cursor_{x,y} specify the location of the active pixel on
> > > the display so we need to offset the plane position in order for
> > > the active pixel to be in the correct place.  
> > 
> > Nope, hot_x/y is just for virtual machines to indicate where the
> > logical cursor position is within the cursor plane. It should have 2
> > effect on how something is displayed.
> 
> Hmm... I've run the same client code on QXL and Rockchip (which uses
> universal planes) and without this patch the behaviour is just plain
> wrong on Rockchip.
> 
> With a 32x32 cursor with the hotspot in the bottom-right using:
> 
>     drmModeSetCursor2(..., 0, 32)
>     drmModeMoveCursor(..., x, y)
> 
> then with QXL when I click I get an event at (x, y) and this is
> precisely under the bottom-right of the cursor.
> 
> With Rockchip the click appears to happen at the top-left of the cursor
> (as if the hotspot were (0, 0)).  This patch makes the behaviour match
> that on QXL.
> 
> I can't see how the hotspot can be ignored here unless you're saying
> that the client code needs to offset the cursor position by the hotspot,
> but in that case it will quite clearly be wrong on QXL.

I checked a bunch of X drivers and none of them adjust for the hotspot.
Also i915.ko always used x/y directly and never adjusted for the hotspot.
And as far as generic kms interfaces go i915 is pretty much the standard,
and that is what's been implemented in universal planes.

The only exception really seems to be radeon.ko, which does adjust the x/y
position of the cursor in the crtc space like your patch does above.

How can I test cursor hotspots? In the end we want a) something consistent
b) that doesn't break existing code and unfortunately b) trumps a). So I'd
like to figure out what's going on on the amd/intel hw I have here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/