Re: [PATCH v2] drm: support hotspot for universal plane cursors
From: Daniel Vetter
Date: Tue Nov 17 2015 - 14:11:32 EST
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@xxxxxxxx>
> > > 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. 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? 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;
> }
--
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/