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?

Naturally there's no documentation in the uapi header for any of this stuff :( Would be nice if the pople excited about documentation would fix that part instead of just focusing on the internal stuff.

}

if (fb) { diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3f0c690..2be4414 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -404,6 +404,8 @@ struct drm_crtc_funcs {

• @cursor: cursor plane for this CRTC
• @cursor_x: current x position of the cursor, used for universal cursor planes
• @cursor_y: current y position of the cursor, used for universal cursor planes

• • @hot_x: x-coordinate of cursor hotspot, used for universal cursor planes
• • @hot_y: y-coordinate of cursor hotspot, used for universal cursor planes
  • @enabled: is this CRTC enabled?
  • @mode: current mode timings
  • @hwmode: mode timings as programmed to hw regs

@@ -445,6 +447,10 @@ struct drm_crtc { int cursor_x; int cursor_y;

• /* hotspot of cursor */

• int hot_x;

• int hot_y;

• bool enabled;

  /* Requested mode from modesetting. */

-- 2.6.3.462.gbe2c914

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Does it? Impossible to say without docs, or some reference to the canonical implementation. Looking at the logs, I guess qxl was the first to get hotspot support. Unfortunately looking at the qxl driver isn't much help. But at leeast it seems to just pass the x/y and hot_x/y straight through without offsetting anything.

so we need to offset the plane position in order for the active pixel to be in the correct place.

...

...

}

if (fb) { diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3f0c690..2be4414 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -404,6 +404,8 @@ struct drm_crtc_funcs {

• @cursor: cursor plane for this CRTC
• @cursor_x: current x position of the cursor, used for universal

cursor planes

• @cursor_y: current y position of the cursor, used for universal

cursor planes

• • @hot_x: x-coordinate of cursor hotspot, used for universal

cursor planes

• • @hot_y: y-coordinate of cursor hotspot, used for universal

cursor planes

• @enabled: is this CRTC enabled?
• @mode: current mode timings
• @hwmode: mode timings as programmed to hw regs

@@ -445,6 +447,10 @@ struct drm_crtc { int cursor_x; int cursor_y;

• /* hotspot of cursor */

• int hot_x;

• int hot_y;

• bool enabled;

  /* Requested mode from modesetting. */

-- 2.6.3.462.gbe2c914

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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.

On Tue, Nov 17, 2015 at 1:40 PM, Daniel Vetter daniel@ffwll.ch wrote:

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

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.

radeon hw has had the hotspot register since r5xx. http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/...

amdgpu programs it as well: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/...

Alex

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

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

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 / 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=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). -Daniel

Compile with:

cc -o test -I/usr/include/libdrm test.c -lkms -ldrm

-- >8 -- #include <err.h> #include <poll.h> #include <string.h>

#include <sys/mman.h>

#include <libkms/libkms.h> #include <xf86drm.h> #include <xf86drmMode.h>

static int drm_fd; static int crtc_id = -1; static drmModeModeInfo mode_info; static struct kms_driver *kms;

static struct { int id; size_t size; unsigned char *data; } framebuffer;

static struct { int x, y; struct kms_bo *buffer; } cursor;

#define CURSOR_WIDTH 21 #define CURSOR_HEIGHT 21 #define CURSOR_HOT_X 21 #define CURSOR_HOT_Y 0 unsigned char cursor_data[] = "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\377\0\0\0\377\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\0\0\0\377\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377" "\377\377\377\377\377\377\0\0\0\377\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\377\377\377\377" "\377\0\0\0\377\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377" "\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\0\0\0\377" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377" "\377\377\377\377\377\377\377\377\377\377\377\377\377\377\0\0\0\377\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\377\377" "\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\0\0\0\377\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\377\377\377" "\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377" "\0\0\0\377\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\377\377" "\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377" "\377\377\377\377\377\0\0\0\377\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377" "\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377" "\377\377\377\377\377\377\377\377\377\377\377\377\0\0\0\377\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\0\0" "\0\377\0\0\0\377\0\0\0\377\0\0\0\377\377\377\377\377\377\377\377\377\377" "\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\0\0\0\377\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\377" "\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\0\0\0\377" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\377" "\0\0\0\377\0\0\0\377\377\377\377\377\377\377\377\377\0\0\0\377\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\377\0\0\0\377\0\0\0" "\0\0\0\0\0\0\0\0\377\377\377\377\377\0\0\0\377\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\377\377\377\377\377\377\377\377\377\0\0\0\377\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\377\0\0\0\377\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377" "\377\377\0\0\0\377\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\377\0\0\0\377\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377" "\377\377\377\377\0\0\0\377\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\377\0\0\0\377\377\377\377\377\0\0\0\377\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\377\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0";

static void draw_cursor(void) { void *result; unsigned char *ptr; int x, y; if (kms_bo_map(cursor.buffer, &result)) err(1, "kms_bo_map");

ptr = result; memset(ptr, 0, 64 * 64 * 4);

for (y = 0; y < CURSOR_HEIGHT; y++) memcpy(ptr + 64 * 4 * y, cursor_data + 4 * CURSOR_WIDTH * y, CURSOR_WIDTH * 4);

kms_bo_unmap(cursor.buffer); }

static void setup_cursor(void) { const unsigned int attr[] = { KMS_BO_TYPE, KMS_BO_TYPE_CURSOR_64X64_A8R8G8B8, KMS_WIDTH, 64, KMS_HEIGHT, 64, KMS_TERMINATE_PROP_LIST }; unsigned int handle; if (kms_bo_create(kms, attr, &cursor.buffer)) err(1, "kms_bo_create");

draw_cursor();

if (kms_bo_get_prop(cursor.buffer, KMS_HANDLE, &handle)) err(1, "kms_bo_get_prop");

if (drmModeSetCursor2(drm_fd, crtc_id, handle, 64, 64, CURSOR_HOT_X, CURSOR_HOT_Y)) err(1, "drmModeSetCursor2");

if (drmModeMoveCursor(drm_fd, crtc_id, cursor.x, cursor.y)) err(1, "drmModeMoveCursor"); }

static void create_framebuffer(void) { struct drm_mode_create_dumb creq = { .width = mode_info.hdisplay, .height = mode_info.vdisplay, .bpp = 32, .flags = 0, }; struct drm_mode_map_dumb mreq = { 0 }; if (drmIoctl(drm_fd, DRM_IOCTL_MODE_CREATE_DUMB, &creq)) err(1, "DRM_IOCTL_MODE_CREATE_DUMB");

if (drmModeAddFB(drm_fd, creq.width, creq.height, 24, 32, creq.pitch, creq.handle, &framebuffer.id)) err(1, "drmModeAddFB");

mreq.handle = creq.handle; if (drmIoctl(drm_fd, DRM_IOCTL_MODE_MAP_DUMB, &mreq)) err(1, "DRM_IOCTL_MODE_MAP_DUMB");

framebuffer.data = mmap(0, creq.size, PROT_READ | PROT_WRITE, MAP_SHARED, drm_fd, mreq.offset); if (framebuffer.data == MAP_FAILED) err(1, "mmap");

framebuffer.size = creq.size; memset(framebuffer.data, 0, framebuffer.size);

/* Paint cursor location with a single colored pixel. */ framebuffer.data[1 + cursor.x * 4 + cursor.y * creq.pitch] = '\xff'; framebuffer.data[2 + cursor.x * 4 + cursor.y * creq.pitch] = '\xff'; }

static int setup_connector(drmModeConnector *conn, drmModeRes *res) { drmModeModeInfo *mode = NULL; int i;

if (conn->connection != DRM_MODE_CONNECTED) return -1;

for (int j = 0; j < conn->count_modes; j++) { if (!mode || (conn->modes[j].type & DRM_MODE_TYPE_PREFERRED)) { mode = &conn->modes[j]; if (conn->modes[j].type & DRM_MODE_TYPE_PREFERRED) break; } }

if (!mode) return -1;

for (i = 0; i < conn->count_encoders; i++) { int j; drmModeEncoder *enc = drmModeGetEncoder(drm_fd, conn->encoders[j]); if (!enc) continue;

for (j = 0; j < res->count_crtcs; j++) {
	if (enc->possible_crtcs & (1u << j)) {
		crtc_id = res->crtcs[j];
		break;
	}
}

drmModeFreeEncoder(enc);
if (crtc_id >= 0)
	break;

}

memcpy(&mode_info, mode, sizeof(mode_info)); cursor.x = mode->hdisplay / 2; cursor.y = mode->vdisplay / 2; create_framebuffer();

if (drmModeSetCrtc(drm_fd, crtc_id, framebuffer.id, 0, 0, &conn->connector_id, 1, mode)) err(1, "drmModeSetCrtc");

return 0; }

static void setup_screen(drmModeRes *res) { int i;

for (i = 0; i < res->count_connectors; i++) { int result; drmModeConnector *conn = drmModeGetConnector(drm_fd, res->connectors[i]); if (!conn) continue; result = setup_connector(conn, res); drmModeFreeConnector(conn); if (!result) break; }

if (crtc_id < 0) errx(1, "no screen found"); }

static void setup_drm(const char *module) { drmModeRes *res;

drm_fd = drmOpen(module, NULL); if (drm_fd < 0) err(1, "drmOpen");

if (kms_create(drm_fd, &kms) < 0) err(1, "kms_create");

res = drmModeGetResources(drm_fd); if (!res) err(1, "drmModeGetResources");

setup_screen(res); }

static void wait_for_input(void) { struct pollfd pfd = { .fd = 0, .events = POLLIN, };

poll(&pfd, 1, -1); }

int main(int argc, char *argv[]) { const char *module = "qxl"; if (argc > 1) module = argv[1];

setup_drm(module); setup_cursor();

wait_for_input();

return 0; }

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

[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

On Wed, Nov 18, 2015 at 05:39:39PM +0900, Michel Dänzer wrote:

On 18.11.2015 01:29, 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@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.

Agreed: Since the DRM_IOCTL_MODE_CURSOR ioctl doesn't contain any information about the hotspot, the x/y coordinates passed in the DRM_IOCTL_MODE_CURSOR(2) ioctls can only refer to the position of the top left corner of the cursor.

...

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 / disabling the HW cursor

Michel/Alex, can you please shed some light onto this?

As described in the rest of the commit log, the intention was to avoid the cursor intermittently appearing in the wrong location with existing userspace which sets the cursor BO in one ioctl call and the new position in another ioctl call.

...

radeon is the only driver doing this, making this interface inconsistent.

It's only inconsistent in the case that userspace updates the cursor position to account for the new hotspot position in one ioctl call first, and only then sets the new BO in another ioctl call. In all other cases, the cursor position passed in by userspace is preserved.

Anyway, in the meantime it has become apparent that this change didn't fully fix the problem, so feel free to revert it.

Yeah I read the commit message but didn't understand what it's doing. After some discussion with Alex on irc I realized that the fixup is only applied in when updating the cursor bo and changing the hotspot to avoid that kind of flickering. That problem is solved though on the kernel side with universal planes (where we don't artificially split up the cursor update into a move + bo-update for the driver interface any more). And it's fixable in userspace even with legacy cursor interfaces since the ioctl allows you to move + update at the same time too. It's just that X doesn't provide that interface to the driver in a useful way.

Anyway, conclusion is that radeon implements exactly the same behaviour as every other hw driver, with x/y pointing at the top-left corner of the cursor image (not the hotspot), in CRTC coordinates. I still have no idea what John is seeing though exactly, and why there's a difference between rockchip and qxl for him. -Daniel