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

[John Keeping]

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.
--
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/