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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Nov 17, 2015 at 12:07:24PM -0500, Alex Deucher wrote:
> On Tue, Nov 17, 2015 at 11:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Nov 17, 2015 at 03:59:43PM +0000, John Keeping wrote:
> >> On Tue, 17 Nov 2015 17:39:32 +0200, Ville Syrjälä wrote:
> >>
> >> > On Tue, Nov 17, 2015 at 03:05:34PM +0000, John Keeping wrote:
> >> > > The request's hot_x and hot_y are set correctly for both
> >> > > DRM_IOCTL_MODE_CURSOR and DRM_IOCTL_MODE_CURSOR2 so we just need to
> >> > > save the values and then apply the offset to the cursor plane when
> >> > > the cursor moves.
> >> > >
> >> > > Signed-off-by: John Keeping <john@metanate.com>
> >> > > ---
> >> > > v2:
> >> > >   - add kerneldoc for hot_x and hot_y in struct drm_crtc
> >> > >
> >> > >  drivers/gpu/drm/drm_crtc.c | 11 +++++++----
> >> > >  include/drm/drm_crtc.h     |  6 ++++++
> >> > >  2 files changed, 13 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> > > index 720a153..40f5b84 100644
> >> > > --- a/drivers/gpu/drm/drm_crtc.c
> >> > > +++ b/drivers/gpu/drm/drm_crtc.c
> >> > > @@ -2831,6 +2831,9 @@ static int drm_mode_cursor_universal(struct
> >> > > drm_crtc *crtc, DRM_DEBUG_KMS("failed to wrap cursor buffer in drm
> >> > > framebuffer\n"); return PTR_ERR(fb);
> >> > >                   }
> >> > > +
> >> > > +                 crtc->hot_x = req->hot_x;
> >> > > +                 crtc->hot_y = req->hot_y;
> >> > >           } else {
> >> > >                   fb = NULL;
> >> > >           }
> >> > > @@ -2841,11 +2844,11 @@ static int drm_mode_cursor_universal(struct
> >> > > drm_crtc *crtc, }
> >> > >
> >> > >   if (req->flags & DRM_MODE_CURSOR_MOVE) {
> >> > > -         crtc_x = req->x;
> >> > > -         crtc_y = req->y;
> >> > > +         crtc_x = req->x - crtc->hot_x;
> >> > > +         crtc_y = req->y - crtc->hot_y;
> >> > >   } else {
> >> > > -         crtc_x = crtc->cursor_x;
> >> > > -         crtc_y = crtc->cursor_y;
> >> > > +         crtc_x = crtc->cursor_x - crtc->hot_x;
> >> > > +         crtc_y = crtc->cursor_y - crtc->hot_y;
> >> >
> >> > Why does the location of the hotspot affect the plane position?
> >>
> >> hot_{x,y} specify the location of the active pixel within the cursor
> >> plane and cursor_{x,y} specify the location of the active pixel on the
> >> display so we need to offset the plane position in order for the active
> >> pixel to be in the correct place.
> >
> > Nope, hot_x/y is just for virtual machines to indicate where the logical
> > cursor position is within the cursor plane. It should have 2 effect on how
> > something is displayed. And no, I have absolutely no idea why radeon is
> > pulling some tricks here, which have been added in
> >
> > commit 78b1a6010b46a69bcd47b723a80f92693f26d17b
> > Author: Michel Dänzer <michel.daenzer@amd.com>
> > Date:   Tue Nov 18 18:00:08 2014 +0900
> >
> >     drm/radeon: Use cursor_set2 hook for enabling / disabling the HW cursor
> >
> > Michel/Alex, can you please shed some light onto this? radeon is the only
> > driver doing this, making this interface inconsistent. Is the ddx doing
> > something funny compared to -modeseting or weston?
> >
> > At least a quick look in the -ati sources didn't even show a user for
> > this, it's still using the old cursor ioctl. And there's no other
> > indication in the commit of a bug or anything. Can we perhaps just revert
> > this?
> 
> We use this is xf86-video-ati.  See:
> http://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=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
Software Engineer, Intel Corporation
http://blog.ffwll.ch