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

[Daniel Vetter]

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@xxxxxxxxxxxx>
> >>>> ---
> >>>> v2:
> >>>>   - add kerneldoc for hot_x and hot_y in struct drm_crtc
> >>>>
> >>>>  drivers/gpu/drm/drm_crtc.c | 11 +++++++----
> >>>>  include/drm/drm_crtc.h     |  6 ++++++
> >>>>  2 files changed, 13 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >>>> index 720a153..40f5b84 100644
> >>>> --- a/drivers/gpu/drm/drm_crtc.c
> >>>> +++ b/drivers/gpu/drm/drm_crtc.c
> >>>> @@ -2831,6 +2831,9 @@ static int drm_mode_cursor_universal(struct
> >>>> drm_crtc *crtc, DRM_DEBUG_KMS("failed to wrap cursor buffer in drm
> >>>> framebuffer\n"); return PTR_ERR(fb);
> >>>>  			}
> >>>> +
> >>>> +			crtc->hot_x = req->hot_x;
> >>>> +			crtc->hot_y = req->hot_y;
> >>>>  		} else {
> >>>>  			fb = NULL;
> >>>>  		}
> >>>> @@ -2841,11 +2844,11 @@ static int drm_mode_cursor_universal(struct
> >>>> drm_crtc *crtc, }
> >>>>  
> >>>>  	if (req->flags & DRM_MODE_CURSOR_MOVE) {
> >>>> -		crtc_x = req->x;
> >>>> -		crtc_y = req->y;
> >>>> +		crtc_x = req->x - crtc->hot_x;
> >>>> +		crtc_y = req->y - crtc->hot_y;
> >>>>  	} else {
> >>>> -		crtc_x = crtc->cursor_x;
> >>>> -		crtc_y = crtc->cursor_y;
> >>>> +		crtc_x = crtc->cursor_x - crtc->hot_x;
> >>>> +		crtc_y = crtc->cursor_y - crtc->hot_y;  
> >>>
> >>> Why does the location of the hotspot affect the plane position?
> >>
> >> hot_{x,y} specify the location of the active pixel within the cursor
> >> plane and cursor_{x,y} specify the location of the active pixel on the
> >> display so we need to offset the plane position in order for the active
> >> pixel to be in the correct place.
> > 
> > Nope, hot_x/y is just for virtual machines to indicate where the logical
> > cursor position is within the cursor plane. It should have 2 effect on how
> > something is displayed.
> 
> 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@xxxxxxx>
> > Date:   Tue Nov 18 18:00:08 2014 +0900
> > 
> >     drm/radeon: Use cursor_set2 hook for enabling / disabling the HW cursor
> > 
> > Michel/Alex, can you please shed some light onto this?
> 
> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/