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

[Daniel Vetter]

On Wed, Nov 18, 2015 at 10:12:16AM +0000, John Keeping wrote:
> On Tue, 17 Nov 2015 20:11:23 +0100, Daniel Vetter wrote:
> 
> > On Tue, Nov 17, 2015 at 06:47:28PM +0000, John Keeping wrote:
> > > On Tue, 17 Nov 2015 19:31:32 +0100, Daniel Vetter wrote:
> > >   
> > > > On Tue, Nov 17, 2015 at 12:07:24PM -0500, Alex Deucher wrote:  
> > > > > On Tue, Nov 17, 2015 at 11:29 AM, Daniel Vetter
> > > > > <daniel@xxxxxxxx> 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. And no, I
> > > > > > have absolutely no idea why radeon is pulling some tricks
> > > > > > here, which have been added in
> > > > > >
> > > > > > commit 78b1a6010b46a69bcd47b723a80f92693f26d17b
> > > > > > Author: Michel Dänzer <michel.daenzer@xxxxxxx>
> > > > > > Date:   Tue Nov 18 18:00:08 2014 +0900
> > > > > >
> > > > > >     drm/radeon: Use cursor_set2 hook for enabling / disabling
> > > > > > the HW cursor
> > > > > >
> > > > > > Michel/Alex, can you please shed some light onto this? radeon
> > > > > > is the only driver doing this, making this interface
> > > > > > inconsistent. Is the ddx doing something funny compared to
> > > > > > -modeseting or weston?
> > > > > >
> > > > > > At least a quick look in the -ati sources didn't even show a
> > > > > > user for this, it's still using the old cursor ioctl. And
> > > > > > there's no other indication in the commit of a bug or
> > > > > > anything. Can we perhaps just revert this?    
> > > > > 
> > > > > We use this is xf86-video-ati.  See:
> > > > > http://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=c9f8f642fd495937400618a4fc25ecae3f8888fc
> > > > > Our hw cursor has a hotspot register that needs to be
> > > > > programmed for proper operation.  I don't remember the details
> > > > > of the specific bug. Michel can provide more info.    
> > > > 
> > > > Yeah I was blind and didn't spot this. But I can't find your
> > > > hotspot cursor reg - it's only used to adjust the x/y offsets
> > > > (similar to John's patch). And amdgpu doesn't do this at all,
> > > > it's only radoen.ko. Still confused.  
> > > 
> > > Having investigated a bit more, I think xserver handles the hotspot
> > > itself without using the kernel hotspot handling and xorg-video-qxl
> > > carefully reverses this in order to take advantage of
> > > drmModeSetCursor2(), so with X11 drivers you don't notice that the
> > > kernel handling of this is broken in nearly all drivers.  
> > 
> > Where did you spot this code in -qxl? Afaics it has the exact same
> > copy of the usual drmmode.c code as everyone else. No adjustments of
> > x/yhot seems to be going on there, nor in the qxl.ko kernel driver.
> > Or at least I didn't find it.
> > 
> > > Here's a small test program that demonstrates whether or not cursor
> > > hotspots work.  There should be a single colored pixel immediately
> > > to the left of the pointer but with a broken driver the white pixel
> > > will be 64 pixels to the left of the pointer.  
> > 
> > Is there also a way to test this with X? In the end that's what will
> > matter for most end users, and if there's difference in behaviour in
> > the various X drivers we're really screwed (since essentially we
> > can't ever fix it properly for the legacy ioctl).
> 
> I've now tested with X and QXL and it seems that the bug is in the QXL
> DRM driver, since the cursor appears in the wrong place when using both
> xorg-video-qxl and xorg-video-modesetting.
> 
> I think what's happening is that X and the DRM cursor ioctls always use
> cursor_{x,y} as the location of the top-left of the cursor image, but
> the SPICE protocol uses cursor_{x,y} as the location of the active pixel
> in the cursor image (the specification [1] uses the term "position of
> the mouse pointer").  So this patch is wrong and I withdraw it.

Yup, x/y being the top-left corner of the cursor image is how all other
drivers work.

> This means that the QXL driver needs to apply the hotspot offset to the
> location whenever it generated QXL_CURSOR_SET and QXL_CURSOR_MOVE
> commands.
> 
> [1] http://www.spice-space.org/docs/spice_protocol.pdf

Excellent that we've figured this out. Care to write a patch for qxl since
you've figured this all out?

Thanks, 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/