Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755591AbbKRKM5 (ORCPT ); Wed, 18 Nov 2015 05:12:57 -0500 Received: from dougal.metanate.com ([90.155.101.14]:23139 "EHLO metanate.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752859AbbKRKMx convert rfc822-to-8bit (ORCPT ); Wed, 18 Nov 2015 05:12:53 -0500 Date: Wed, 18 Nov 2015 10:12:16 +0000 From: John Keeping To: Daniel Vetter Cc: Alex Deucher , Ville =?UTF-8?B?U3lyasOkbMOk?= , LKML , Maling list - DRI developers , Michel =?UTF-8?B?RMOkbnplcg==?= Subject: Re: [PATCH v2] drm: support hotspot for universal plane cursors Message-ID: <20151118101216.4cd01306.john@metanate.com> In-Reply-To: <20151117191123.GY16848@phenom.ffwll.local> 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> <20151117191123.GY16848@phenom.ffwll.local> Organization: Metanate Ltd X-Mailer: Claws Mail 3.13.0 (GTK+ 2.24.28; x86_64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6904 Lines: 140 On Tue, 17 Nov 2015 20:11:23 +0100, Daniel Vetter wrote: > 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). I've now tested with X and QXL and it seems that the bug is in the QXL DRM driver, since the cursor appears in the wrong place when using both xorg-video-qxl and xorg-video-modesetting. I think what's happening is that X and the DRM cursor ioctls always use cursor_{x,y} as the location of the top-left of the cursor image, but the SPICE protocol uses cursor_{x,y} as the location of the active pixel in the cursor image (the specification [1] uses the term "position of the mouse pointer"). So this patch is wrong and I withdraw it. This means that the QXL driver needs to apply the hotspot offset to the location whenever it generated QXL_CURSOR_SET and QXL_CURSOR_MOVE commands. [1] http://www.spice-space.org/docs/spice_protocol.pdf -- 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/