From mboxrd@z Thu Jan 1 00:00:00 1970 From: robin.murphy@arm.com (Robin Murphy) Date: Thu, 17 Mar 2016 10:52:44 +0000 Subject: [PATCH] drm/fb_cma_helper: Implement fb_mmap callback In-Reply-To: <20160316191450.GY19428@n2100.arm.linux.org.uk> References: <11eb10d95109abc104beb94c0409d49644ea8517.1458140105.git.robin.murphy@arm.com> <20160316152825.GR14170@phenom.ffwll.local> <20160316191450.GY19428@n2100.arm.linux.org.uk> Message-ID: <56EA8C7C.4040303@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 16/03/16 19:14, Russell King - ARM Linux wrote: > On Wed, Mar 16, 2016 at 04:28:25PM +0100, Daniel Vetter wrote: >> On Wed, Mar 16, 2016 at 02:57:49PM +0000, Robin Murphy wrote: >>> In the absence of an fb_mmap callback, the fbdev code falls back to a >>> naive implementation which relies upon the DMA address being the same >>> as the physical address, and the buffer being physically contiguous >>> from there. Whilst this often holds for standard CMA allocations via >>> the platform's regular DMA ops, if the allocation is provided by an >>> IOMMU then such assumptions can fall apart spectacularly. >>> >>> To resolve this, reroute the fb_mmap call to the appropriate DMA API >>> implementation, as per the other cma_helper calls. >>> >>> Signed-off-by: Robin Murphy >>> --- >>> >>> Hi dri-devel, >>> >>> This is an empirical fix for something I tickled via the newly-added >>> ARM HDLCD driver on a Juno platform - I have no idea whatsoever about >>> how "proper" it is in terms of the DRM infrastructure, so feel free to >>> treat this as a bug report rather than an actual patch if appropriate ;) >> >> I think the best case would be if we could have a generic fbdev helper >> that remaps to dumb mmap support. But that's a bit tricky to pull of: >> 1. from fb_info we can get at the fbdev drm_framebuffer. >> 2. from a drm_framebuffer we can get at the underlying backing storage >> object using fb->funcs->get_handle. >> 3. With that handle we could go into the dumb mmap support (using als the >> vma) and create the mmap. >> >> Except that ->get_handle needs a file_priv, and that just exist for the >> fbdev emulation kms client. I guess we could fix that by creating a >> minimal fake drm file_priv for the fbdev emulation (and treat it more like >> any other kms client), but I think that's way too much work when this >> simple patch here gets the job done. > > I think first, a different question needs to be answered: > > include/uapi/linux/fb.h: > > struct fb_fix_screeninfo { > char id[16]; /* identification string eg "TT Builtin" */ > unsigned long smem_start; /* Start of frame buffer mem */ > /* (physical address) */ > > Should a DMA address be exposed through smem_start, rather than a > physical address as the long-standing documentation quoted above > has stated? > > Is it, in fact, a driver bug to store something that isn't a physical > address there? We could also go into whether it's right to store even a physical address in something which isn't necessarily big enough... After the time I spent in the bowels of the fbdev code figuring out this crash, I fear that if we dig too deep we may awaken something in the darkness ;) Robin. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robin Murphy Subject: Re: [PATCH] drm/fb_cma_helper: Implement fb_mmap callback Date: Thu, 17 Mar 2016 10:52:44 +0000 Message-ID: <56EA8C7C.4040303@arm.com> References: <11eb10d95109abc104beb94c0409d49644ea8517.1458140105.git.robin.murphy@arm.com> <20160316152825.GR14170@phenom.ffwll.local> <20160316191450.GY19428@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by gabe.freedesktop.org (Postfix) with ESMTP id 228DE6E06B for ; Thu, 17 Mar 2016 10:52:48 +0000 (UTC) In-Reply-To: <20160316191450.GY19428@n2100.arm.linux.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Russell King - ARM Linux , Daniel Vetter Cc: liviu.dudau@arm.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org T24gMTYvMDMvMTYgMTk6MTQsIFJ1c3NlbGwgS2luZyAtIEFSTSBMaW51eCB3cm90ZToKPiBPbiBX ZWQsIE1hciAxNiwgMjAxNiBhdCAwNDoyODoyNVBNICswMTAwLCBEYW5pZWwgVmV0dGVyIHdyb3Rl Ogo+PiBPbiBXZWQsIE1hciAxNiwgMjAxNiBhdCAwMjo1Nzo0OVBNICswMDAwLCBSb2JpbiBNdXJw aHkgd3JvdGU6Cj4+PiBJbiB0aGUgYWJzZW5jZSBvZiBhbiBmYl9tbWFwIGNhbGxiYWNrLCB0aGUg ZmJkZXYgY29kZSBmYWxscyBiYWNrIHRvIGEKPj4+IG5haXZlIGltcGxlbWVudGF0aW9uIHdoaWNo IHJlbGllcyB1cG9uIHRoZSBETUEgYWRkcmVzcyBiZWluZyB0aGUgc2FtZQo+Pj4gYXMgdGhlIHBo eXNpY2FsIGFkZHJlc3MsIGFuZCB0aGUgYnVmZmVyIGJlaW5nIHBoeXNpY2FsbHkgY29udGlndW91 cwo+Pj4gZnJvbSB0aGVyZS4gV2hpbHN0IHRoaXMgb2Z0ZW4gaG9sZHMgZm9yIHN0YW5kYXJkIENN QSBhbGxvY2F0aW9ucyB2aWEKPj4+IHRoZSBwbGF0Zm9ybSdzIHJlZ3VsYXIgRE1BIG9wcywgaWYg dGhlIGFsbG9jYXRpb24gaXMgcHJvdmlkZWQgYnkgYW4KPj4+IElPTU1VIHRoZW4gc3VjaCBhc3N1 bXB0aW9ucyBjYW4gZmFsbCBhcGFydCBzcGVjdGFjdWxhcmx5Lgo+Pj4KPj4+IFRvIHJlc29sdmUg dGhpcywgcmVyb3V0ZSB0aGUgZmJfbW1hcCBjYWxsIHRvIHRoZSBhcHByb3ByaWF0ZSBETUEgQVBJ Cj4+PiBpbXBsZW1lbnRhdGlvbiwgYXMgcGVyIHRoZSBvdGhlciBjbWFfaGVscGVyIGNhbGxzLgo+ Pj4KPj4+IFNpZ25lZC1vZmYtYnk6IFJvYmluIE11cnBoeSA8cm9iaW4ubXVycGh5QGFybS5jb20+ Cj4+PiAtLS0KPj4+Cj4+PiBIaSBkcmktZGV2ZWwsCj4+Pgo+Pj4gVGhpcyBpcyBhbiBlbXBpcmlj YWwgZml4IGZvciBzb21ldGhpbmcgSSB0aWNrbGVkIHZpYSB0aGUgbmV3bHktYWRkZWQKPj4+IEFS TSBIRExDRCBkcml2ZXIgb24gYSBKdW5vIHBsYXRmb3JtIC0gSSBoYXZlIG5vIGlkZWEgd2hhdHNv ZXZlciBhYm91dAo+Pj4gaG93ICJwcm9wZXIiIGl0IGlzIGluIHRlcm1zIG9mIHRoZSBEUk0gaW5m cmFzdHJ1Y3R1cmUsIHNvIGZlZWwgZnJlZSB0bwo+Pj4gdHJlYXQgdGhpcyBhcyBhIGJ1ZyByZXBv cnQgcmF0aGVyIHRoYW4gYW4gYWN0dWFsIHBhdGNoIGlmIGFwcHJvcHJpYXRlIDspCj4+Cj4+IEkg dGhpbmsgdGhlIGJlc3QgY2FzZSB3b3VsZCBiZSBpZiB3ZSBjb3VsZCBoYXZlIGEgZ2VuZXJpYyBm YmRldiBoZWxwZXIKPj4gdGhhdCByZW1hcHMgdG8gZHVtYiBtbWFwIHN1cHBvcnQuIEJ1dCB0aGF0 J3MgYSBiaXQgdHJpY2t5IHRvIHB1bGwgb2Y6Cj4+IDEuIGZyb20gZmJfaW5mbyB3ZSBjYW4gZ2V0 IGF0IHRoZSBmYmRldiBkcm1fZnJhbWVidWZmZXIuCj4+IDIuIGZyb20gYSBkcm1fZnJhbWVidWZm ZXIgd2UgY2FuIGdldCBhdCB0aGUgdW5kZXJseWluZyBiYWNraW5nIHN0b3JhZ2UKPj4gb2JqZWN0 IHVzaW5nIGZiLT5mdW5jcy0+Z2V0X2hhbmRsZS4KPj4gMy4gV2l0aCB0aGF0IGhhbmRsZSB3ZSBj b3VsZCBnbyBpbnRvIHRoZSBkdW1iIG1tYXAgc3VwcG9ydCAodXNpbmcgYWxzIHRoZQo+PiB2bWEp IGFuZCBjcmVhdGUgdGhlIG1tYXAuCj4+Cj4+IEV4Y2VwdCB0aGF0IC0+Z2V0X2hhbmRsZSBuZWVk cyBhIGZpbGVfcHJpdiwgYW5kIHRoYXQganVzdCBleGlzdCBmb3IgdGhlCj4+IGZiZGV2IGVtdWxh dGlvbiBrbXMgY2xpZW50LiBJIGd1ZXNzIHdlIGNvdWxkIGZpeCB0aGF0IGJ5IGNyZWF0aW5nIGEK Pj4gbWluaW1hbCBmYWtlIGRybSBmaWxlX3ByaXYgZm9yIHRoZSBmYmRldiBlbXVsYXRpb24gKGFu ZCB0cmVhdCBpdCBtb3JlIGxpa2UKPj4gYW55IG90aGVyIGttcyBjbGllbnQpLCBidXQgSSB0aGlu ayB0aGF0J3Mgd2F5IHRvbyBtdWNoIHdvcmsgd2hlbiB0aGlzCj4+IHNpbXBsZSBwYXRjaCBoZXJl IGdldHMgdGhlIGpvYiBkb25lLgo+Cj4gSSB0aGluayBmaXJzdCwgYSBkaWZmZXJlbnQgcXVlc3Rp b24gbmVlZHMgdG8gYmUgYW5zd2VyZWQ6Cj4KPiBpbmNsdWRlL3VhcGkvbGludXgvZmIuaDoKPgo+ IHN0cnVjdCBmYl9maXhfc2NyZWVuaW5mbyB7Cj4gICAgICAgICAgY2hhciBpZFsxNl07ICAgICAg ICAgICAgICAgICAgICAvKiBpZGVudGlmaWNhdGlvbiBzdHJpbmcgZWcgIlRUIEJ1aWx0aW4iICov Cj4gICAgICAgICAgdW5zaWduZWQgbG9uZyBzbWVtX3N0YXJ0OyAgICAgICAvKiBTdGFydCBvZiBm cmFtZSBidWZmZXIgbWVtICovCj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAvKiAocGh5c2ljYWwgYWRkcmVzcykgKi8KPgo+IFNob3VsZCBhIERNQSBhZGRyZXNzIGJl IGV4cG9zZWQgdGhyb3VnaCBzbWVtX3N0YXJ0LCByYXRoZXIgdGhhbiBhCj4gcGh5c2ljYWwgYWRk cmVzcyBhcyB0aGUgbG9uZy1zdGFuZGluZyBkb2N1bWVudGF0aW9uIHF1b3RlZCBhYm92ZQo+IGhh cyBzdGF0ZWQ/Cj4KPiBJcyBpdCwgaW4gZmFjdCwgYSBkcml2ZXIgYnVnIHRvIHN0b3JlIHNvbWV0 aGluZyB0aGF0IGlzbid0IGEgcGh5c2ljYWwKPiBhZGRyZXNzIHRoZXJlPwoKV2UgY291bGQgYWxz byBnbyBpbnRvIHdoZXRoZXIgaXQncyByaWdodCB0byBzdG9yZSBldmVuIGEgcGh5c2ljYWwgCmFk ZHJlc3MgaW4gc29tZXRoaW5nIHdoaWNoIGlzbid0IG5lY2Vzc2FyaWx5IGJpZyBlbm91Z2guLi4K CkFmdGVyIHRoZSB0aW1lIEkgc3BlbnQgaW4gdGhlIGJvd2VscyBvZiB0aGUgZmJkZXYgY29kZSBm aWd1cmluZyBvdXQgdGhpcyAKY3Jhc2gsIEkgZmVhciB0aGF0IGlmIHdlIGRpZyB0b28gZGVlcCB3 ZSBtYXkgYXdha2VuIHNvbWV0aGluZyBpbiB0aGUgCmRhcmtuZXNzIDspCgpSb2Jpbi4KX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxp bmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJl ZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935668AbcCQKwx (ORCPT ); Thu, 17 Mar 2016 06:52:53 -0400 Received: from foss.arm.com ([217.140.101.70]:47445 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933112AbcCQKws (ORCPT ); Thu, 17 Mar 2016 06:52:48 -0400 Subject: Re: [PATCH] drm/fb_cma_helper: Implement fb_mmap callback To: Russell King - ARM Linux , Daniel Vetter References: <11eb10d95109abc104beb94c0409d49644ea8517.1458140105.git.robin.murphy@arm.com> <20160316152825.GR14170@phenom.ffwll.local> <20160316191450.GY19428@n2100.arm.linux.org.uk> Cc: airlied@linux.ie, liviu.dudau@arm.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org From: Robin Murphy Message-ID: <56EA8C7C.4040303@arm.com> Date: Thu, 17 Mar 2016 10:52:44 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160316191450.GY19428@n2100.arm.linux.org.uk> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 16/03/16 19:14, Russell King - ARM Linux wrote: > On Wed, Mar 16, 2016 at 04:28:25PM +0100, Daniel Vetter wrote: >> On Wed, Mar 16, 2016 at 02:57:49PM +0000, Robin Murphy wrote: >>> In the absence of an fb_mmap callback, the fbdev code falls back to a >>> naive implementation which relies upon the DMA address being the same >>> as the physical address, and the buffer being physically contiguous >>> from there. Whilst this often holds for standard CMA allocations via >>> the platform's regular DMA ops, if the allocation is provided by an >>> IOMMU then such assumptions can fall apart spectacularly. >>> >>> To resolve this, reroute the fb_mmap call to the appropriate DMA API >>> implementation, as per the other cma_helper calls. >>> >>> Signed-off-by: Robin Murphy >>> --- >>> >>> Hi dri-devel, >>> >>> This is an empirical fix for something I tickled via the newly-added >>> ARM HDLCD driver on a Juno platform - I have no idea whatsoever about >>> how "proper" it is in terms of the DRM infrastructure, so feel free to >>> treat this as a bug report rather than an actual patch if appropriate ;) >> >> I think the best case would be if we could have a generic fbdev helper >> that remaps to dumb mmap support. But that's a bit tricky to pull of: >> 1. from fb_info we can get at the fbdev drm_framebuffer. >> 2. from a drm_framebuffer we can get at the underlying backing storage >> object using fb->funcs->get_handle. >> 3. With that handle we could go into the dumb mmap support (using als the >> vma) and create the mmap. >> >> Except that ->get_handle needs a file_priv, and that just exist for the >> fbdev emulation kms client. I guess we could fix that by creating a >> minimal fake drm file_priv for the fbdev emulation (and treat it more like >> any other kms client), but I think that's way too much work when this >> simple patch here gets the job done. > > I think first, a different question needs to be answered: > > include/uapi/linux/fb.h: > > struct fb_fix_screeninfo { > char id[16]; /* identification string eg "TT Builtin" */ > unsigned long smem_start; /* Start of frame buffer mem */ > /* (physical address) */ > > Should a DMA address be exposed through smem_start, rather than a > physical address as the long-standing documentation quoted above > has stated? > > Is it, in fact, a driver bug to store something that isn't a physical > address there? We could also go into whether it's right to store even a physical address in something which isn't necessarily big enough... After the time I spent in the bowels of the fbdev code figuring out this crash, I fear that if we dig too deep we may awaken something in the darkness ;) Robin.