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@ffwll.ch 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@metanate.com > > > --- > > > 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@amd.com Date: Tue Nov 18 18:00:08 2014 +0900
drm/radeon: Use cursor_set2 hook for enabling / disablingthe 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=c9f8f642fd... 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.
Excellent that we've figured this out. Care to write a patch for qxl since you've figured this all out?
Thanks, Daniel