Message-ID: <20151117184728.27252360.john@metanate.com>
Date:	Tue, 17 Nov 2015 18:47:28 +0000
From:	John Keeping <john@...anate.com>
To:	Daniel Vetter <daniel@...ll.ch>
Cc:	Alex Deucher <alexdeucher@...il.com>,
	Ville Syrjälä <ville.syrjala@...ux.intel.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Maling list - DRI developers 
	<dri-devel@...ts.freedesktop.org>,
	Michel Dänzer <michel.daenzer@....com>
Subject: Re: [PATCH v2] drm: support hotspot for universal plane cursors

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@...ll.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@...anate.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@....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.

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.

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.

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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists