From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Michel_D=c3=a4nzer?= Subject: Re: [PATCH v2] drm: support hotspot for universal plane cursors Date: Wed, 18 Nov 2015 17:39:39 +0900 Message-ID: <564C394B.8070408@daenzer.net> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail.gna.ch (darkcity.gna.ch [195.226.6.51]) by gabe.freedesktop.org (Postfix) with ESMTP id B89FF6E62D for ; Wed, 18 Nov 2015 00:39:48 -0800 (PST) In-Reply-To: <20151117162935.GK16848@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: John Keeping , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Alex Deucher Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org T24gMTguMTEuMjAxNSAwMToyOSwgRGFuaWVsIFZldHRlciB3cm90ZToKPiBPbiBUdWUsIE5vdiAx NywgMjAxNSBhdCAwMzo1OTo0M1BNICswMDAwLCBKb2huIEtlZXBpbmcgd3JvdGU6Cj4+IE9uIFR1 ZSwgMTcgTm92IDIwMTUgMTc6Mzk6MzIgKzAyMDAsIFZpbGxlIFN5cmrDpGzDpCB3cm90ZToKPj4K Pj4+IE9uIFR1ZSwgTm92IDE3LCAyMDE1IGF0IDAzOjA1OjM0UE0gKzAwMDAsIEpvaG4gS2VlcGlu ZyB3cm90ZToKPj4+PiBUaGUgcmVxdWVzdCdzIGhvdF94IGFuZCBob3RfeSBhcmUgc2V0IGNvcnJl Y3RseSBmb3IgYm90aAo+Pj4+IERSTV9JT0NUTF9NT0RFX0NVUlNPUiBhbmQgRFJNX0lPQ1RMX01P REVfQ1VSU09SMiBzbyB3ZSBqdXN0IG5lZWQgdG8KPj4+PiBzYXZlIHRoZSB2YWx1ZXMgYW5kIHRo ZW4gYXBwbHkgdGhlIG9mZnNldCB0byB0aGUgY3Vyc29yIHBsYW5lIHdoZW4KPj4+PiB0aGUgY3Vy c29yIG1vdmVzLgo+Pj4+Cj4+Pj4gU2lnbmVkLW9mZi1ieTogSm9obiBLZWVwaW5nIDxqb2huQG1l dGFuYXRlLmNvbT4KPj4+PiAtLS0KPj4+PiB2MjoKPj4+PiAgIC0gYWRkIGtlcm5lbGRvYyBmb3Ig aG90X3ggYW5kIGhvdF95IGluIHN0cnVjdCBkcm1fY3J0Ywo+Pj4+Cj4+Pj4gIGRyaXZlcnMvZ3B1 L2RybS9kcm1fY3J0Yy5jIHwgMTEgKysrKysrKy0tLS0KPj4+PiAgaW5jbHVkZS9kcm0vZHJtX2Ny dGMuaCAgICAgfCAgNiArKysrKysKPj4+PiAgMiBmaWxlcyBjaGFuZ2VkLCAxMyBpbnNlcnRpb25z KCspLCA0IGRlbGV0aW9ucygtKQo+Pj4+Cj4+Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2Ry bS9kcm1fY3J0Yy5jIGIvZHJpdmVycy9ncHUvZHJtL2RybV9jcnRjLmMKPj4+PiBpbmRleCA3MjBh MTUzLi40MGY1Yjg0IDEwMDY0NAo+Pj4+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fY3J0Yy5j Cj4+Pj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2RybV9jcnRjLmMKPj4+PiBAQCAtMjgzMSw2ICsy ODMxLDkgQEAgc3RhdGljIGludCBkcm1fbW9kZV9jdXJzb3JfdW5pdmVyc2FsKHN0cnVjdAo+Pj4+ IGRybV9jcnRjICpjcnRjLCBEUk1fREVCVUdfS01TKCJmYWlsZWQgdG8gd3JhcCBjdXJzb3IgYnVm ZmVyIGluIGRybQo+Pj4+IGZyYW1lYnVmZmVyXG4iKTsgcmV0dXJuIFBUUl9FUlIoZmIpOwo+Pj4+ ICAJCQl9Cj4+Pj4gKwo+Pj4+ICsJCQljcnRjLT5ob3RfeCA9IHJlcS0+aG90X3g7Cj4+Pj4gKwkJ CWNydGMtPmhvdF95ID0gcmVxLT5ob3RfeTsKPj4+PiAgCQl9IGVsc2Ugewo+Pj4+ICAJCQlmYiA9 IE5VTEw7Cj4+Pj4gIAkJfQo+Pj4+IEBAIC0yODQxLDExICsyODQ0LDExIEBAIHN0YXRpYyBpbnQg ZHJtX21vZGVfY3Vyc29yX3VuaXZlcnNhbChzdHJ1Y3QKPj4+PiBkcm1fY3J0YyAqY3J0YywgfQo+ Pj4+ICAKPj4+PiAgCWlmIChyZXEtPmZsYWdzICYgRFJNX01PREVfQ1VSU09SX01PVkUpIHsKPj4+ PiAtCQljcnRjX3ggPSByZXEtPng7Cj4+Pj4gLQkJY3J0Y195ID0gcmVxLT55Owo+Pj4+ICsJCWNy dGNfeCA9IHJlcS0+eCAtIGNydGMtPmhvdF94Owo+Pj4+ICsJCWNydGNfeSA9IHJlcS0+eSAtIGNy dGMtPmhvdF95Owo+Pj4+ICAJfSBlbHNlIHsKPj4+PiAtCQljcnRjX3ggPSBjcnRjLT5jdXJzb3Jf eDsKPj4+PiAtCQljcnRjX3kgPSBjcnRjLT5jdXJzb3JfeTsKPj4+PiArCQljcnRjX3ggPSBjcnRj LT5jdXJzb3JfeCAtIGNydGMtPmhvdF94Owo+Pj4+ICsJCWNydGNfeSA9IGNydGMtPmN1cnNvcl95 IC0gY3J0Yy0+aG90X3k7ICAKPj4+Cj4+PiBXaHkgZG9lcyB0aGUgbG9jYXRpb24gb2YgdGhlIGhv dHNwb3QgYWZmZWN0IHRoZSBwbGFuZSBwb3NpdGlvbj8KPj4KPj4gaG90X3t4LHl9IHNwZWNpZnkg dGhlIGxvY2F0aW9uIG9mIHRoZSBhY3RpdmUgcGl4ZWwgd2l0aGluIHRoZSBjdXJzb3IKPj4gcGxh bmUgYW5kIGN1cnNvcl97eCx5fSBzcGVjaWZ5IHRoZSBsb2NhdGlvbiBvZiB0aGUgYWN0aXZlIHBp eGVsIG9uIHRoZQo+PiBkaXNwbGF5IHNvIHdlIG5lZWQgdG8gb2Zmc2V0IHRoZSBwbGFuZSBwb3Np dGlvbiBpbiBvcmRlciBmb3IgdGhlIGFjdGl2ZQo+PiBwaXhlbCB0byBiZSBpbiB0aGUgY29ycmVj dCBwbGFjZS4KPiAKPiBOb3BlLCBob3RfeC95IGlzIGp1c3QgZm9yIHZpcnR1YWwgbWFjaGluZXMg dG8gaW5kaWNhdGUgd2hlcmUgdGhlIGxvZ2ljYWwKPiBjdXJzb3IgcG9zaXRpb24gaXMgd2l0aGlu IHRoZSBjdXJzb3IgcGxhbmUuIEl0IHNob3VsZCBoYXZlIDIgZWZmZWN0IG9uIGhvdwo+IHNvbWV0 aGluZyBpcyBkaXNwbGF5ZWQuCgpBZ3JlZWQ6IFNpbmNlIHRoZSBEUk1fSU9DVExfTU9ERV9DVVJT T1IgaW9jdGwgZG9lc24ndCBjb250YWluIGFueQppbmZvcm1hdGlvbiBhYm91dCB0aGUgaG90c3Bv dCwgdGhlIHgveSBjb29yZGluYXRlcyBwYXNzZWQgaW4gdGhlCkRSTV9JT0NUTF9NT0RFX0NVUlNP UigyKSBpb2N0bHMgY2FuIG9ubHkgcmVmZXIgdG8gdGhlIHBvc2l0aW9uIG9mIHRoZQp0b3AgbGVm dCBjb3JuZXIgb2YgdGhlIGN1cnNvci4KCgo+IEFuZCBubywgSSBoYXZlIGFic29sdXRlbHkgbm8g aWRlYSB3aHkgcmFkZW9uIGlzIHB1bGxpbmcgc29tZSB0cmlja3MgaGVyZSwKPiB3aGljaCBoYXZl IGJlZW4gYWRkZWQgaW4KPiAKPiBjb21taXQgNzhiMWE2MDEwYjQ2YTY5YmNkNDdiNzIzYTgwZjky NjkzZjI2ZDE3Ygo+IEF1dGhvcjogTWljaGVsIETDpG56ZXIgPG1pY2hlbC5kYWVuemVyQGFtZC5j b20+Cj4gRGF0ZTogICBUdWUgTm92IDE4IDE4OjAwOjA4IDIwMTQgKzA5MDAKPiAKPiAgICAgZHJt L3JhZGVvbjogVXNlIGN1cnNvcl9zZXQyIGhvb2sgZm9yIGVuYWJsaW5nIC8gZGlzYWJsaW5nIHRo ZSBIVyBjdXJzb3IKPiAKPiBNaWNoZWwvQWxleCwgY2FuIHlvdSBwbGVhc2Ugc2hlZCBzb21lIGxp Z2h0IG9udG8gdGhpcz8KCkFzIGRlc2NyaWJlZCBpbiB0aGUgcmVzdCBvZiB0aGUgY29tbWl0IGxv ZywgdGhlIGludGVudGlvbiB3YXMgdG8gYXZvaWQKdGhlIGN1cnNvciBpbnRlcm1pdHRlbnRseSBh cHBlYXJpbmcgaW4gdGhlIHdyb25nIGxvY2F0aW9uIHdpdGggZXhpc3RpbmcKdXNlcnNwYWNlIHdo aWNoIHNldHMgdGhlIGN1cnNvciBCTyBpbiBvbmUgaW9jdGwgY2FsbCBhbmQgdGhlIG5ldwpwb3Np dGlvbiBpbiBhbm90aGVyIGlvY3RsIGNhbGwuCgoKPiByYWRlb24gaXMgdGhlIG9ubHkgZHJpdmVy IGRvaW5nIHRoaXMsIG1ha2luZyB0aGlzIGludGVyZmFjZSBpbmNvbnNpc3RlbnQuCgpJdCdzIG9u bHkgaW5jb25zaXN0ZW50IGluIHRoZSBjYXNlIHRoYXQgdXNlcnNwYWNlIHVwZGF0ZXMgdGhlIGN1 cnNvcgpwb3NpdGlvbiB0byBhY2NvdW50IGZvciB0aGUgbmV3IGhvdHNwb3QgcG9zaXRpb24gaW4g b25lIGlvY3RsIGNhbGwKZmlyc3QsIGFuZCBvbmx5IHRoZW4gc2V0cyB0aGUgbmV3IEJPIGluIGFu b3RoZXIgaW9jdGwgY2FsbC4gSW4gYWxsIG90aGVyCmNhc2VzLCB0aGUgY3Vyc29yIHBvc2l0aW9u IHBhc3NlZCBpbiBieSB1c2Vyc3BhY2UgaXMgcHJlc2VydmVkLgoKQW55d2F5LCBpbiB0aGUgbWVh bnRpbWUgaXQgaGFzIGJlY29tZSBhcHBhcmVudCB0aGF0IHRoaXMgY2hhbmdlIGRpZG4ndApmdWxs eSBmaXggdGhlIHByb2JsZW0sIHNvIGZlZWwgZnJlZSB0byByZXZlcnQgaXQuCgoKLS0gCkVhcnRo bGluZyBNaWNoZWwgRMOkbnplciAgICAgICAgICAgICAgIHwgICAgICAgICAgICAgICBodHRwOi8v d3d3LmFtZC5jb20KTGlicmUgc29mdHdhcmUgZW50aHVzaWFzdCAgICAgICAgICAgICB8ICAgICAg ICAgICAgIE1lc2EgYW5kIFggZGV2ZWxvcGVyCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3Rz LmZyZWVkZXNrdG9wLm9yZwpodHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlz dGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755453AbbKRIjv (ORCPT ); Wed, 18 Nov 2015 03:39:51 -0500 Received: from darkcity.gna.ch ([195.226.6.51]:39510 "EHLO mail.gna.ch" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755048AbbKRIjt (ORCPT ); Wed, 18 Nov 2015 03:39:49 -0500 Subject: Re: [PATCH v2] drm: support hotspot for universal plane cursors To: John Keeping , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Alex Deucher 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> From: =?UTF-8?Q?Michel_D=c3=a4nzer?= X-Enigmail-Draft-Status: N1110 Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Message-ID: <564C394B.8070408@daenzer.net> Date: Wed, 18 Nov 2015 17:39:39 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.3.0 MIME-Version: 1.0 In-Reply-To: <20151117162935.GK16848@phenom.ffwll.local> 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 On 18.11.2015 01:29, 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. Agreed: Since the DRM_IOCTL_MODE_CURSOR ioctl doesn't contain any information about the hotspot, the x/y coordinates passed in the DRM_IOCTL_MODE_CURSOR(2) ioctls can only refer to the position of the top left corner of the cursor. > 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? As described in the rest of the commit log, the intention was to avoid the cursor intermittently appearing in the wrong location with existing userspace which sets the cursor BO in one ioctl call and the new position in another ioctl call. > radeon is the only driver doing this, making this interface inconsistent. It's only inconsistent in the case that userspace updates the cursor position to account for the new hotspot position in one ioctl call first, and only then sets the new BO in another ioctl call. In all other cases, the cursor position passed in by userspace is preserved. Anyway, in the meantime it has become apparent that this change didn't fully fix the problem, so feel free to revert it. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer