Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753813AbbKQTL3 (ORCPT ); Tue, 17 Nov 2015 14:11:29 -0500 Received: from mail-wm0-f50.google.com ([74.125.82.50]:38158 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752119AbbKQTL1 (ORCPT ); Tue, 17 Nov 2015 14:11:27 -0500 Date: Tue, 17 Nov 2015 20:11:23 +0100 From: Daniel Vetter To: John Keeping Cc: Daniel Vetter , Alex Deucher , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , LKML , Maling list - DRI developers , Michel =?iso-8859-1?Q?D=E4nzer?= Subject: Re: [PATCH v2] drm: support hotspot for universal plane cursors Message-ID: <20151117191123.GY16848@phenom.ffwll.local> Mail-Followup-To: John Keeping , Alex Deucher , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , LKML , Maling list - DRI developers , Michel =?iso-8859-1?Q?D=E4nzer?= References: <1447762218-11017-1-git-send-email-john@metanate.com> <1447772734-6480-1-git-send-email-john@metanate.com> <20151117153932.GU4437@intel.com> <20151117155943.1e83b053.john@metanate.com> <20151117162935.GK16848@phenom.ffwll.local> <20151117183132.GU16848@phenom.ffwll.local> <20151117184728.27252360.john@metanate.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20151117184728.27252360.john@metanate.com> X-Operating-System: Linux phenom 4.1.0-2-amd64 User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15483 Lines: 405 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 > > > 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 > > > >> > > --- > > > >> > > 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 > > > > 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 > #include > #include > > #include > > #include > #include > #include > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/