From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic. Date: Mon, 26 Nov 2018 22:41:02 +0100 Message-ID: <20181126224102.1efbf2d1@bbrezillon> References: <20181119190805.19139-1-helen.koike@collabora.com> <1d10a4323a49ca09bbb7dc865aaaf5b8c5e3b3d5.camel@crowfest.net> <87o9abbmq4.fsf@anholt.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <87o9abbmq4.fsf@anholt.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Eric Anholt Cc: =?UTF-8?B?U3TDqXBoYW5l?= Marchesin , Sean Paul , Gustavo Padovan , David Airlie , Linux Kernel Mailing List , helen.koike@collabora.com, "list@263.net:IOMMU DRIVERS , Joerg Roedel , " , Tomasz Figa , "open list:ARM/Rockchip SoC..." , Michael Zoran , dri-devel , Enric Balletbo i Serra , kernel@collabora.com, "list@263.net:IOMMU DRIVERS , Joerg Roedel ," List-Id: iommu@lists.linux-foundation.org T24gTW9uLCAyNiBOb3YgMjAxOCAxMjozNjowMyAtMDgwMApFcmljIEFuaG9sdCA8ZXJpY0Bhbmhv bHQubmV0PiB3cm90ZToKCj4gTWljaGFlbCBab3JhbiA8bXpvcmFuQGNyb3dmZXN0Lm5ldD4gd3Jp dGVzOgo+IAo+ID4gT24gRnJpLCAyMDE4LTExLTIzIGF0IDExOjI3ICswOTAwLCBUb21hc3ogRmln YSB3cm90ZTogIAo+ID4+IAo+ID4+IFRoZSBwb2ludCBoZXJlIGlzIG5vdCBhYm91dCBzZXR0aW5n IGFuZCByZXNldHRpbmcgdGhlIHBsYW5lLT5mYgo+ID4+IHBvaW50ZXIuIEl0J3MgYWJvdXQgd2hh dCBoYXBwZW5zIGluc2lkZQo+ID4+IGRybV9hdG9taWNfc2V0X2ZiX2Zvcl9wbGFuZSgpLgo+ID4+ IAo+ID4+IEl0IGNhbGxzIGRybV9mcmFtZWJ1ZmZlcl9nZXQoKSBmb3IgdGhlIG5ldyBmYiBhbmQK PiA+PiBkcm1fZnJhbWVidWZmZXJfcHV0KCkgZm9yIHRoZSBvbGQgZmIuIEluIHJlc3VsdCwgaWYg dGhlIGZiIGNoYW5nZXMsCj4gPj4gdGhlIG9sZCBmYiwgd2hpY2ggaGFkIGl0cyByZWZlcmVuY2Ug Y291bnQgaW5jcmVtZW50ZWQgaW4gdGhlIGF0b21pYwo+ID4+IGNvbW1pdCB0aGF0IHNldCBpdCB0 byB0aGUgcGxhbmUgYmVmb3JlLCBoYXMgaXRzIHJlZmVyZW5jZSBjb3VudAo+ID4+IGRlY3JlbWVu dGVkLiBNb3Jlb3ZlciwgaWYgdGhlIG5ldyByZWZlcmVuY2UgY291bnQgYmVjb21lcyAwLAo+ID4+ IGRybV9mcmFtZWJ1ZmZlcl9wdXQoKSB3aWxsIGltbWVkaWF0ZWx5IGZyZWUgdGhlIGJ1ZmZlci4K PiA+PiAKPiA+PiBGcmVlaW5nIGEgYnVmZmVyIHdoZW4gdGhlIGhhcmR3YXJlIGlzIHN0aWxsIHNj YW5uaW5nIG91dCBvZiBpdCBpc24ndAo+ID4+IGEKPiA+PiBnb29kIGlkZWEsIGlzIGl0PyAgCj4g Pgo+ID4gTm8sIGl0J3Mgbm90LiAgQnV0IHRoZSBib2FyZCBJIHN1Ym1pdHRlZCB0aGUgcGF0Y2gg Zm9yIGRvZXNuJ3QgaGF2ZQo+ID4gYW55dGhpbmcgbGlrZSBob3Qgc3dhcGFibGUgcmFtLiAgVGhl IHJhbSBhY2Nlc3MgaXMgc3RpbGwgZ29pbmcgdG8gd29yaywKPiA+IGp1c3QgaXQgbWlnaHQgZGlz cGxheSBzb21ldGhpbmcgaXQgc2hvdWxkbid0LiBTYXkgZm9yIGV4YW1wbGUgaWYgdGhhdAo+ID4g ZnJhbWUgYnVmZmVyIGdvdCByZXVzZWQgYnkgc29tZXRoaWcgZWxzZSBhbmQgZmlsbGVkIHdpdGgg bmV3IGRhdGEgaW4KPiA+IHRoZSB2ZXJ5IHNtYWxsIHdpbmRvdy4KPiA+Cj4gPiBCdXQgeWVzLCBJ IGFncmVlIHRoZSBiZXN0IHNvbHV0aW9uIHdvdWxkIGJlIHRvIG5vdCByZWxlYXNlIHRoZSBidWZm ZXIKPiA+IHVudGlsIHRoZSBuZXh0IHZibGFuay4KPiA+Cj4gPiBQZXJoYXBzIGEgZ29vZCBzb2x1 dGlvbiB3b3VsZCBiZSBmb3IgdGhlIERSTSBhcGkgdG8gaGF2ZSB0aGUgY29uY2VwdCBvZgo+ID4g YSBkZWZlcnJlZCByZWxlYXNlPyAgTWVhbmluZyBpZiB0aGUgcHV0KCkgY2FsbCBqdXN0IGFkZGVk IHRoZSBmcmFtZQo+ID4gYnVmZmVyIHRvIGEgbGlzdCB0aGF0IERSTSBjb3JlIGNvdWxkIHdhbGsg ZHVyaW5nIHRoZSB2YmxhbmsuICBUaGF0Cj4gPiBtaWdodCBiZSBiZXR0ZXIgdGhlbiBldmVyeSBz aW5nbGUgZHJpdmVyIHRyeWluZyB0byB3b3JrIHVwIGEgY3VzdG9tCj4gPiBzb2x1dGlvbi4KPiA+ ICAKPiA+PiBUaGUgdmM0IGRyaXZlciBzZWVtcyB0byBiZSBhYmxlIHRvIHByb2dyYW0gdGhlIGhh cmR3YXJlIHRvIHN3aXRjaCB0aGUKPiA+PiBzY2Fub3V0IHRvIHRoZSBuZXcgYnVmZmVyIGltbWVk aWF0ZWx5Ogo+ID4+IAo+ID4+IGh0dHBzOi8vZWxpeGlyLmJvb3RsaW4uY29tL2xpbnV4L3Y0LjIw LXJjMy9zb3VyY2UvZHJpdmVycy9ncHUvZHJtL3ZjNC92YzRfcGxhbmUuYyNMNzk0Cj4gPj4gCj4g Pj4gQWx0aG91Z2ggSSB3b25kZXIgaWYgdGhlcmUgaXNuJ3Qgc3RpbGwgYSB0aW55IHJhY2UgdGhl cmUgLSB0aGUKPiA+PiBoYXJkd2FyZSBtYXkgaGF2ZSBqdXN0IHN0YXJ0ZWQgcmVmaWxsaW5nIHRo ZSBGSUZPIGZyb20gdGhlIG9sZAo+ID4+IGFkZHJlc3MuIFN0aWxsLCBpZiB0aGUgRklGTyBpcyBz bWFsbCwgdGhlIEZJRk8gcmVmaWxsIG9wZXJhdGlvbiBtYXkKPiA+PiBiZQo+ID4+IG11Y2ggc2hv cnRlciB0aGFuIGl0IHRha2VzIGZvciB0aGUga2VybmVsIGNvZGUgdG8gYWN0dWFsbHkgZnJlZSB0 aGUKPiA+PiBidWZmZXIuIEVyaWMgYW5kIE1pY2hhZWwsIGNvdWxkIHlvdSBjb25maXJtPwo+ID4+ ICAgCj4gPgo+ID4gSSBkb24ndCBoYXZlIHRob3NlIGJvYXJkcyBhbnltb3JlLCBhbmQgSSBkb24n dCBoYXZlIGFjY2VzcyB0byBhbnkKPiA+IHRlY2huaWNhbCBkb2N1bWVudGF0aW9uIG9uIHRoZSBH UFUgc28gSSBjYW4ndCByZWFsbHkgYWRkIG11Y2ggaGVyZS4gIAo+ID4gRXJpYyBjYW4gcHJvYmFi bHkgcHJvdmlkZSB0aGUgYmVzdCBpbmZvcm1hdGlvbi4gIAo+IAo+IEkgZG9uJ3QgdGhpbmsgSSB1 bmRlcnN0b29kIG15IHNjYW5vdXQgaGFyZHdhcmUgd2VsbCBlbm91Z2ggd2hlbiBJCj4gc3RhcnRl ZCBvbiB0aGUgYXN5bmMgdXBkYXRlIHN0dWZmIGZvciBycGkuICB2YzQgcHJvYmFibHkgbmVlZHMg dG8gd2FpdAo+IHVudGlsIHRoZSBIVyBzdGFydHMgc2Nhbm5pbmcgb3V0IGEgbmV3IGxpbmUgYmVm b3JlIGxldHRpbmcgdGhlIG9sZCBCTwo+IGdldCBmcmVlZC4KClRoYXQncyBhbHNvIG15IHVuZGVy c3RhbmRpbmcuIE5vdGUgdGhhdCBldmVuIGlmIHRoZSBCTyBpcyBmcmVlZCBiZWZvcmUKdGhlIEhW UyBoYXMgZmluaXNoZWQgZ2VuZXJhdGluZyBhIGxpbmUgaXQgc2hvdWxkbid0IGNyYXNoLCBiZWNh dXNlCmFjY2Vzc2VzIGFyZSBkb25lIHRocm91Z2ggRE1BLiBNaWdodCBiZSBhIHNlY3VyaXR5IGlz c3VlIHRob3VnaCBpZiB3ZQpzdGFydCByZS11c2luZyB0aGUgbWVtb3J5IGZvciBzb21ldGhpbmcg ZWxzZSB3aGlsZSBpdCdzIHN0aWxsIGJlaW5nCmFjY2Vzc2VkLgoKVGhlIHNvbHV0aW9uIHdvdWxk IGJlIHRvIHdhaXQgZm9yIHRoZSBFT0wgKEVuZCBPZiBMaW5lKSBpbnRlcnJ1cHQgaW4KdGhlIGFz eW5jIHVwZGF0ZSBwYXRoLCBidXQgSSdtIG5vdCBzdXJlIHRoaXMgaXMgYWxsb3dlZC4KX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxp bmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJl ZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris.brezillon@bootlin.com (Boris Brezillon) Date: Mon, 26 Nov 2018 22:41:02 +0100 Subject: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic. In-Reply-To: <87o9abbmq4.fsf@anholt.net> References: <20181119190805.19139-1-helen.koike@collabora.com> <1d10a4323a49ca09bbb7dc865aaaf5b8c5e3b3d5.camel@crowfest.net> <87o9abbmq4.fsf@anholt.net> Message-ID: <20181126224102.1efbf2d1@bbrezillon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, 26 Nov 2018 12:36:03 -0800 Eric Anholt wrote: > Michael Zoran writes: > > > On Fri, 2018-11-23 at 11:27 +0900, Tomasz Figa wrote: > >> > >> The point here is not about setting and resetting the plane->fb > >> pointer. It's about what happens inside > >> drm_atomic_set_fb_for_plane(). > >> > >> It calls drm_framebuffer_get() for the new fb and > >> drm_framebuffer_put() for the old fb. In result, if the fb changes, > >> the old fb, which had its reference count incremented in the atomic > >> commit that set it to the plane before, has its reference count > >> decremented. Moreover, if the new reference count becomes 0, > >> drm_framebuffer_put() will immediately free the buffer. > >> > >> Freeing a buffer when the hardware is still scanning out of it isn't > >> a > >> good idea, is it? > > > > No, it's not. But the board I submitted the patch for doesn't have > > anything like hot swapable ram. The ram access is still going to work, > > just it might display something it shouldn't. Say for example if that > > frame buffer got reused by somethig else and filled with new data in > > the very small window. > > > > But yes, I agree the best solution would be to not release the buffer > > until the next vblank. > > > > Perhaps a good solution would be for the DRM api to have the concept of > > a deferred release? Meaning if the put() call just added the frame > > buffer to a list that DRM core could walk during the vblank. That > > might be better then every single driver trying to work up a custom > > solution. > > > >> The vc4 driver seems to be able to program the hardware to switch the > >> scanout to the new buffer immediately: > >> > >> https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/gpu/drm/vc4/vc4_plane.c#L794 > >> > >> Although I wonder if there isn't still a tiny race there - the > >> hardware may have just started refilling the FIFO from the old > >> address. Still, if the FIFO is small, the FIFO refill operation may > >> be > >> much shorter than it takes for the kernel code to actually free the > >> buffer. Eric and Michael, could you confirm? > >> > > > > I don't have those boards anymore, and I don't have access to any > > technical documentation on the GPU so I can't really add much here. > > Eric can probably provide the best information. > > I don't think I understood my scanout hardware well enough when I > started on the async update stuff for rpi. vc4 probably needs to wait > until the HW starts scanning out a new line before letting the old BO > get freed. That's also my understanding. Note that even if the BO is freed before the HVS has finished generating a line it shouldn't crash, because accesses are done through DMA. Might be a security issue though if we start re-using the memory for something else while it's still being accessed. The solution would be to wait for the EOL (End Of Line) interrupt in the async update path, but I'm not sure this is allowed. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9495FC43441 for ; Mon, 26 Nov 2018 21:41:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5EBEC208E4 for ; Mon, 26 Nov 2018 21:41:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5EBEC208E4 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=bootlin.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727114AbeK0Igo (ORCPT ); Tue, 27 Nov 2018 03:36:44 -0500 Received: from mail.bootlin.com ([62.4.15.54]:43500 "EHLO mail.bootlin.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726315AbeK0Igo (ORCPT ); Tue, 27 Nov 2018 03:36:44 -0500 Received: by mail.bootlin.com (Postfix, from userid 110) id 8EC15207BB; Mon, 26 Nov 2018 22:41:13 +0100 (CET) Received: from bbrezillon (91-160-177-164.subs.proxad.net [91.160.177.164]) by mail.bootlin.com (Postfix) with ESMTPSA id 2B17E206D8; Mon, 26 Nov 2018 22:41:03 +0100 (CET) Date: Mon, 26 Nov 2018 22:41:02 +0100 From: Boris Brezillon To: Eric Anholt Cc: Michael Zoran , Tomasz Figa , helen.koike@collabora.com, Sandy Huang , Heiko =?UTF-8?B?U3TDvGJuZXI=?= , David Airlie , "list\@263.net\:IOMMU DRIVERS \\, Joerg Roedel \\," , "list\@263.net\:IOMMU DRIVERS \\, Joerg Roedel \\," , "list\@263.net\:IOMMU DRIVERS \\, Joerg Roedel \\," , Enric Balletbo i Serra , Linux Kernel Mailing List , dri-devel , "open list\:ARM\/Rockchip SoC..." , Gustavo Padovan , Sean Paul , kernel@collabora.com, =?UTF-8?B?U3TDqXBoYW5l?= Marchesin Subject: Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic. Message-ID: <20181126224102.1efbf2d1@bbrezillon> In-Reply-To: <87o9abbmq4.fsf@anholt.net> References: <20181119190805.19139-1-helen.koike@collabora.com> <1d10a4323a49ca09bbb7dc865aaaf5b8c5e3b3d5.camel@crowfest.net> <87o9abbmq4.fsf@anholt.net> X-Mailer: Claws Mail 3.16.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 26 Nov 2018 12:36:03 -0800 Eric Anholt wrote: > Michael Zoran writes: > > > On Fri, 2018-11-23 at 11:27 +0900, Tomasz Figa wrote: > >> > >> The point here is not about setting and resetting the plane->fb > >> pointer. It's about what happens inside > >> drm_atomic_set_fb_for_plane(). > >> > >> It calls drm_framebuffer_get() for the new fb and > >> drm_framebuffer_put() for the old fb. In result, if the fb changes, > >> the old fb, which had its reference count incremented in the atomic > >> commit that set it to the plane before, has its reference count > >> decremented. Moreover, if the new reference count becomes 0, > >> drm_framebuffer_put() will immediately free the buffer. > >> > >> Freeing a buffer when the hardware is still scanning out of it isn't > >> a > >> good idea, is it? > > > > No, it's not. But the board I submitted the patch for doesn't have > > anything like hot swapable ram. The ram access is still going to work, > > just it might display something it shouldn't. Say for example if that > > frame buffer got reused by somethig else and filled with new data in > > the very small window. > > > > But yes, I agree the best solution would be to not release the buffer > > until the next vblank. > > > > Perhaps a good solution would be for the DRM api to have the concept of > > a deferred release? Meaning if the put() call just added the frame > > buffer to a list that DRM core could walk during the vblank. That > > might be better then every single driver trying to work up a custom > > solution. > > > >> The vc4 driver seems to be able to program the hardware to switch the > >> scanout to the new buffer immediately: > >> > >> https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/gpu/drm/vc4/vc4_plane.c#L794 > >> > >> Although I wonder if there isn't still a tiny race there - the > >> hardware may have just started refilling the FIFO from the old > >> address. Still, if the FIFO is small, the FIFO refill operation may > >> be > >> much shorter than it takes for the kernel code to actually free the > >> buffer. Eric and Michael, could you confirm? > >> > > > > I don't have those boards anymore, and I don't have access to any > > technical documentation on the GPU so I can't really add much here. > > Eric can probably provide the best information. > > I don't think I understood my scanout hardware well enough when I > started on the async update stuff for rpi. vc4 probably needs to wait > until the HW starts scanning out a new line before letting the old BO > get freed. That's also my understanding. Note that even if the BO is freed before the HVS has finished generating a line it shouldn't crash, because accesses are done through DMA. Might be a security issue though if we start re-using the memory for something else while it's still being accessed. The solution would be to wait for the EOL (End Of Line) interrupt in the async update path, but I'm not sure this is allowed.