Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755420AbbKRLIZ (ORCPT ); Wed, 18 Nov 2015 06:08:25 -0500 Received: from mail-wm0-f41.google.com ([74.125.82.41]:34430 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751447AbbKRLIX (ORCPT ); Wed, 18 Nov 2015 06:08:23 -0500 Date: Wed, 18 Nov 2015 12:08:19 +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: <20151118110819.GQ20799@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> <20151117191123.GY16848@phenom.ffwll.local> <20151118101216.4cd01306.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: <20151118101216.4cd01306.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: 7515 Lines: 153 On Wed, Nov 18, 2015 at 10:12:16AM +0000, John Keeping wrote: > 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. Yup, x/y being the top-left corner of the cursor image is how all other drivers work. > 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 Excellent that we've figured this out. Care to write a patch for qxl since you've figured this all out? Thanks, 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/