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

[Alex Deucher]

On Tue, Nov 17, 2015 at 1:40 PM, Daniel Vetter <daniel@xxxxxxxx> 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@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.
>>
>> 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
--
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/