[v2] drm: support hotspot for universal plane cursors

Message ID	1447772734-6480-1-git-send-email-john@metanate.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <dri-devel-bounces@lists.freedesktop.org> From: John Keeping <john@metanate.com> To: David Airlie <airlied@linux.ie> Subject: [PATCH v2] drm: support hotspot for universal plane cursors Date: Tue, 17 Nov 2015 15:05:34 +0000 Message-Id: <1447772734-6480-1-git-send-email-john@metanate.com> In-Reply-To: <1447762218-11017-1-git-send-email-john@metanate.com> References: <1447762218-11017-1-git-send-email-john@metanate.com> Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.org>

John Keeping Nov. 17, 2015, 3:05 p.m. UTC

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(-)

Ville Syrjala Nov. 17, 2015, 3:39 p.m. UTC | #1

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

John Keeping Nov. 17, 2015, 3:59 p.m. UTC | #2

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.

> >  	}
> >  
> >  	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  
>

Ville Syrjala Nov. 17, 2015, 4:09 p.m. UTC | #3

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

Daniel Vetter Nov. 17, 2015, 4:29 p.m. UTC | #4

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?

Cheers, Daniel

> 
> > >  	}
> > >  
> > >  	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  
> > 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

John Keeping Nov. 17, 2015, 4:58 p.m. UTC | #5

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.

Alex Deucher Nov. 17, 2015, 5:07 p.m. UTC | #6

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

Alex

>
> Cheers, Daniel
>
>>
>> > >   }
>> > >
>> > >   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
>> >
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Daniel Vetter Nov. 17, 2015, 6:31 p.m. UTC | #7

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

Daniel Vetter Nov. 17, 2015, 6:40 p.m. UTC | #8

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.

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

John Keeping Nov. 17, 2015, 6:47 p.m. UTC | #9

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

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.

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;
}

Daniel Vetter Nov. 17, 2015, 7:11 p.m. UTC | #10

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=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).
-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;
> }

Alex Deucher Nov. 17, 2015, 7:11 p.m. UTC | #11

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/gpu/drm/radeon/radeon_cursor.c#n204

amdgpu programs it as well:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c#n2440

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

Michel Dänzer Nov. 18, 2015, 8:39 a.m. UTC | #12

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.

Daniel Vetter Nov. 18, 2015, 8:51 a.m. UTC | #13

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

Michel Dänzer Nov. 18, 2015, 8:59 a.m. UTC | #14

On 18.11.2015 17:51, Daniel Vetter wrote:
> On Wed, Nov 18, 2015 at 05:39:39PM +0900, Michel Dänzer wrote:
>> On 18.11.2015 01:29, Daniel Vetter wrote:
>>>
>>> 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.

Well, the legacy cursor interfaces currently don't allow the driver to
prevent the hardware from updating the cursor between the cursor_set /
cursor_move calls. Anyway, I tried adding a cursor_lock hook for that
purpose and adapting userspace accordingly, but it still doesn't seem to
fully fix the problem. So I'm leaving it to somebody else / another day. :)

John Keeping Nov. 18, 2015, 10:12 a.m. UTC | #15

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

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

Daniel Vetter Nov. 18, 2015, 11:08 a.m. UTC | #16

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

[v2] drm: support hotspot for universal plane cursors

Commit Message

Comments

Patch