From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic. Date: Tue, 27 Nov 2018 08:56:02 +0100 Message-ID: <20181127075602.GV4266@phenom.ffwll.local> References: <20181119190805.19139-1-helen.koike@collabora.com> <1d10a4323a49ca09bbb7dc865aaaf5b8c5e3b3d5.camel@crowfest.net> <87o9abbmq4.fsf@anholt.net> <20181126224102.1efbf2d1@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20181126224102.1efbf2d1@bbrezillon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Boris Brezillon Cc: =?iso-8859-1?Q?St=E9phane?= Marchesin , Sean Paul , Gustavo Padovan , David Airlie , Linux Kernel Mailing List , "list@263.net:IOMMU DRIVERS , Joerg Roedel , " , Tomasz Figa , helen.koike@collabora.com, dri-devel , Enric Balletbo i Serra , "open list:ARM/Rockchip SoC..." , kernel@collabora.com, Michael Zoran , "list@263.net:IOMMU DRIVERS , Joerg Roedel , " List-Id: iommu@lists.linux-foundation.org T24gTW9uLCBOb3YgMjYsIDIwMTggYXQgMTA6NDE6MDJQTSArMDEwMCwgQm9yaXMgQnJlemlsbG9u IHdyb3RlOgo+IE9uIE1vbiwgMjYgTm92IDIwMTggMTI6MzY6MDMgLTA4MDAKPiBFcmljIEFuaG9s dCA8ZXJpY0BhbmhvbHQubmV0PiB3cm90ZToKPiAKPiA+IE1pY2hhZWwgWm9yYW4gPG16b3JhbkBj cm93ZmVzdC5uZXQ+IHdyaXRlczoKPiA+IAo+ID4gPiBPbiBGcmksIDIwMTgtMTEtMjMgYXQgMTE6 MjcgKzA5MDAsIFRvbWFzeiBGaWdhIHdyb3RlOiAgCj4gPiA+PiAKPiA+ID4+IFRoZSBwb2ludCBo ZXJlIGlzIG5vdCBhYm91dCBzZXR0aW5nIGFuZCByZXNldHRpbmcgdGhlIHBsYW5lLT5mYgo+ID4g Pj4gcG9pbnRlci4gSXQncyBhYm91dCB3aGF0IGhhcHBlbnMgaW5zaWRlCj4gPiA+PiBkcm1fYXRv bWljX3NldF9mYl9mb3JfcGxhbmUoKS4KPiA+ID4+IAo+ID4gPj4gSXQgY2FsbHMgZHJtX2ZyYW1l YnVmZmVyX2dldCgpIGZvciB0aGUgbmV3IGZiIGFuZAo+ID4gPj4gZHJtX2ZyYW1lYnVmZmVyX3B1 dCgpIGZvciB0aGUgb2xkIGZiLiBJbiByZXN1bHQsIGlmIHRoZSBmYiBjaGFuZ2VzLAo+ID4gPj4g dGhlIG9sZCBmYiwgd2hpY2ggaGFkIGl0cyByZWZlcmVuY2UgY291bnQgaW5jcmVtZW50ZWQgaW4g dGhlIGF0b21pYwo+ID4gPj4gY29tbWl0IHRoYXQgc2V0IGl0IHRvIHRoZSBwbGFuZSBiZWZvcmUs IGhhcyBpdHMgcmVmZXJlbmNlIGNvdW50Cj4gPiA+PiBkZWNyZW1lbnRlZC4gTW9yZW92ZXIsIGlm IHRoZSBuZXcgcmVmZXJlbmNlIGNvdW50IGJlY29tZXMgMCwKPiA+ID4+IGRybV9mcmFtZWJ1ZmZl cl9wdXQoKSB3aWxsIGltbWVkaWF0ZWx5IGZyZWUgdGhlIGJ1ZmZlci4KPiA+ID4+IAo+ID4gPj4g RnJlZWluZyBhIGJ1ZmZlciB3aGVuIHRoZSBoYXJkd2FyZSBpcyBzdGlsbCBzY2FubmluZyBvdXQg b2YgaXQgaXNuJ3QKPiA+ID4+IGEKPiA+ID4+IGdvb2QgaWRlYSwgaXMgaXQ/ICAKPiA+ID4KPiA+ ID4gTm8sIGl0J3Mgbm90LiAgQnV0IHRoZSBib2FyZCBJIHN1Ym1pdHRlZCB0aGUgcGF0Y2ggZm9y IGRvZXNuJ3QgaGF2ZQo+ID4gPiBhbnl0aGluZyBsaWtlIGhvdCBzd2FwYWJsZSByYW0uICBUaGUg cmFtIGFjY2VzcyBpcyBzdGlsbCBnb2luZyB0byB3b3JrLAo+ID4gPiBqdXN0IGl0IG1pZ2h0IGRp c3BsYXkgc29tZXRoaW5nIGl0IHNob3VsZG4ndC4gU2F5IGZvciBleGFtcGxlIGlmIHRoYXQKPiA+ ID4gZnJhbWUgYnVmZmVyIGdvdCByZXVzZWQgYnkgc29tZXRoaWcgZWxzZSBhbmQgZmlsbGVkIHdp dGggbmV3IGRhdGEgaW4KPiA+ID4gdGhlIHZlcnkgc21hbGwgd2luZG93Lgo+ID4gPgo+ID4gPiBC dXQgeWVzLCBJIGFncmVlIHRoZSBiZXN0IHNvbHV0aW9uIHdvdWxkIGJlIHRvIG5vdCByZWxlYXNl IHRoZSBidWZmZXIKPiA+ID4gdW50aWwgdGhlIG5leHQgdmJsYW5rLgo+ID4gPgo+ID4gPiBQZXJo YXBzIGEgZ29vZCBzb2x1dGlvbiB3b3VsZCBiZSBmb3IgdGhlIERSTSBhcGkgdG8gaGF2ZSB0aGUg Y29uY2VwdCBvZgo+ID4gPiBhIGRlZmVycmVkIHJlbGVhc2U/ICBNZWFuaW5nIGlmIHRoZSBwdXQo KSBjYWxsIGp1c3QgYWRkZWQgdGhlIGZyYW1lCj4gPiA+IGJ1ZmZlciB0byBhIGxpc3QgdGhhdCBE Uk0gY29yZSBjb3VsZCB3YWxrIGR1cmluZyB0aGUgdmJsYW5rLiAgVGhhdAo+ID4gPiBtaWdodCBi ZSBiZXR0ZXIgdGhlbiBldmVyeSBzaW5nbGUgZHJpdmVyIHRyeWluZyB0byB3b3JrIHVwIGEgY3Vz dG9tCj4gPiA+IHNvbHV0aW9uLgo+ID4gPiAgCj4gPiA+PiBUaGUgdmM0IGRyaXZlciBzZWVtcyB0 byBiZSBhYmxlIHRvIHByb2dyYW0gdGhlIGhhcmR3YXJlIHRvIHN3aXRjaCB0aGUKPiA+ID4+IHNj YW5vdXQgdG8gdGhlIG5ldyBidWZmZXIgaW1tZWRpYXRlbHk6Cj4gPiA+PiAKPiA+ID4+IGh0dHBz Oi8vZWxpeGlyLmJvb3RsaW4uY29tL2xpbnV4L3Y0LjIwLXJjMy9zb3VyY2UvZHJpdmVycy9ncHUv ZHJtL3ZjNC92YzRfcGxhbmUuYyNMNzk0Cj4gPiA+PiAKPiA+ID4+IEFsdGhvdWdoIEkgd29uZGVy IGlmIHRoZXJlIGlzbid0IHN0aWxsIGEgdGlueSByYWNlIHRoZXJlIC0gdGhlCj4gPiA+PiBoYXJk d2FyZSBtYXkgaGF2ZSBqdXN0IHN0YXJ0ZWQgcmVmaWxsaW5nIHRoZSBGSUZPIGZyb20gdGhlIG9s ZAo+ID4gPj4gYWRkcmVzcy4gU3RpbGwsIGlmIHRoZSBGSUZPIGlzIHNtYWxsLCB0aGUgRklGTyBy ZWZpbGwgb3BlcmF0aW9uIG1heQo+ID4gPj4gYmUKPiA+ID4+IG11Y2ggc2hvcnRlciB0aGFuIGl0 IHRha2VzIGZvciB0aGUga2VybmVsIGNvZGUgdG8gYWN0dWFsbHkgZnJlZSB0aGUKPiA+ID4+IGJ1 ZmZlci4gRXJpYyBhbmQgTWljaGFlbCwgY291bGQgeW91IGNvbmZpcm0/Cj4gPiA+PiAgIAo+ID4g Pgo+ID4gPiBJIGRvbid0IGhhdmUgdGhvc2UgYm9hcmRzIGFueW1vcmUsIGFuZCBJIGRvbid0IGhh dmUgYWNjZXNzIHRvIGFueQo+ID4gPiB0ZWNobmljYWwgZG9jdW1lbnRhdGlvbiBvbiB0aGUgR1BV IHNvIEkgY2FuJ3QgcmVhbGx5IGFkZCBtdWNoIGhlcmUuICAKPiA+ID4gRXJpYyBjYW4gcHJvYmFi bHkgcHJvdmlkZSB0aGUgYmVzdCBpbmZvcm1hdGlvbi4gIAo+ID4gCj4gPiBJIGRvbid0IHRoaW5r IEkgdW5kZXJzdG9vZCBteSBzY2Fub3V0IGhhcmR3YXJlIHdlbGwgZW5vdWdoIHdoZW4gSQo+ID4g c3RhcnRlZCBvbiB0aGUgYXN5bmMgdXBkYXRlIHN0dWZmIGZvciBycGkuICB2YzQgcHJvYmFibHkg bmVlZHMgdG8gd2FpdAo+ID4gdW50aWwgdGhlIEhXIHN0YXJ0cyBzY2FubmluZyBvdXQgYSBuZXcg bGluZSBiZWZvcmUgbGV0dGluZyB0aGUgb2xkIEJPCj4gPiBnZXQgZnJlZWQuCj4gCj4gVGhhdCdz IGFsc28gbXkgdW5kZXJzdGFuZGluZy4gTm90ZSB0aGF0IGV2ZW4gaWYgdGhlIEJPIGlzIGZyZWVk IGJlZm9yZQo+IHRoZSBIVlMgaGFzIGZpbmlzaGVkIGdlbmVyYXRpbmcgYSBsaW5lIGl0IHNob3Vs ZG4ndCBjcmFzaCwgYmVjYXVzZQo+IGFjY2Vzc2VzIGFyZSBkb25lIHRocm91Z2ggRE1BLiBNaWdo dCBiZSBhIHNlY3VyaXR5IGlzc3VlIHRob3VnaCBpZiB3ZQo+IHN0YXJ0IHJlLXVzaW5nIHRoZSBt ZW1vcnkgZm9yIHNvbWV0aGluZyBlbHNlIHdoaWxlIGl0J3Mgc3RpbGwgYmVpbmcKPiBhY2Nlc3Nl ZC4KPiAKPiBUaGUgc29sdXRpb24gd291bGQgYmUgdG8gd2FpdCBmb3IgdGhlIEVPTCAoRW5kIE9m IExpbmUpIGludGVycnVwdCBpbgo+IHRoZSBhc3luYyB1cGRhdGUgcGF0aCwgYnV0IEknbSBub3Qg c3VyZSB0aGlzIGlzIGFsbG93ZWQuCgpGb3Igc3Vic3VtaW5nIGFzeW5jIHBhZ2VfZmxpcCBpbnRv IHRoZSBhc3luYyBhdG9taWMgY29tbWl0IHN0dWZmIHdlIG5lZWQKY29tcGxldGlvbiBldmVudHMg YW55d2F5LiBGaXJpbmcgdGhvc2UgZnJvbSB0aGUgRU9MIChvciBzb21lIG90aGVyCmh3LWRlcGVu ZGVudCBpbnRlcnJ1cHQsIGNvdWxkIGFsc28gYmUgdGhlIG5leHQgdmJsYW5rKSBzaG91bGQgYmUg aGFyZCB0bwphcnJhbmdlIGZvciBkcml2ZXJzLiBBbmQgdGhlIGhlbGVwcnMgY291bGQgdGhlbiBl YXNpbHkgb2ZmbG9hZCB0aGUKY2xlYW51cF9wbGFuZXMgd29yayB0byBhIHdvcmtlciBhbmQgZGVs YXkgaXQgdW50aWwgYWZ0ZXIgdGhlIGV2ZW50IGhhcwpmaXJlZC4gTGlrZSBpdCBhbHJlYWR5IGRv ZXMgZm9yIG5vcm1hbCBhdG9taWMgY29tbWl0cy4KCkJ1dCB5ZWFoIHJpZ2h0IG5vdyB0aGF0J3Mg YWxsIG1pc3NpbmcuIFRoZSBiaWdnZXN0IGlzc3VlIGhlcmUgaXMgZmlndXJpbmcKb3V0IHdoYXQg dGhlc2UgZXZlbnRzIHNob3VsZCBsb29rIGxpa2UsIGFuZCBob3cgZ2VuZXJpYyB1c2Vyc3BhY2Ug bmVlZHMgdG8KdXNlIHRoZW0uCi1EYW5pZWwKLS0gCkRhbmllbCBWZXR0ZXIKU29mdHdhcmUgRW5n aW5lZXIsIEludGVsIENvcnBvcmF0aW9uCmh0dHA6Ly9ibG9nLmZmd2xsLmNoCl9fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxp c3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNr dG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel@ffwll.ch (Daniel Vetter) Date: Tue, 27 Nov 2018 08:56:02 +0100 Subject: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic. In-Reply-To: <20181126224102.1efbf2d1@bbrezillon> References: <20181119190805.19139-1-helen.koike@collabora.com> <1d10a4323a49ca09bbb7dc865aaaf5b8c5e3b3d5.camel@crowfest.net> <87o9abbmq4.fsf@anholt.net> <20181126224102.1efbf2d1@bbrezillon> Message-ID: <20181127075602.GV4266@phenom.ffwll.local> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Nov 26, 2018 at 10:41:02PM +0100, Boris Brezillon wrote: > 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. For subsuming async page_flip into the async atomic commit stuff we need completion events anyway. Firing those from the EOL (or some other hw-dependent interrupt, could also be the next vblank) should be hard to arrange for drivers. And the heleprs could then easily offload the cleanup_planes work to a worker and delay it until after the event has fired. Like it already does for normal atomic commits. But yeah right now that's all missing. The biggest issue here is figuring out what these events should look like, and how generic userspace needs to use them. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch 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=-2.3 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED, USER_AGENT_MUTT 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 947BDC43441 for ; Tue, 27 Nov 2018 07:56:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4C4C02133F for ; Tue, 27 Nov 2018 07:56:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="E7qfJwCF" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4C4C02133F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch 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 S1729344AbeK0SxI (ORCPT ); Tue, 27 Nov 2018 13:53:08 -0500 Received: from mail-ed1-f66.google.com ([209.85.208.66]:45925 "EHLO mail-ed1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728838AbeK0SxI (ORCPT ); Tue, 27 Nov 2018 13:53:08 -0500 Received: by mail-ed1-f66.google.com with SMTP id d39so18132199edb.12 for ; Mon, 26 Nov 2018 23:56:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=sender:date:from:to:cc:subject:message-id:mail-followup-to :references:mime-version:content-disposition:in-reply-to:user-agent; bh=DO/QdORUV6GcvyHJ+4q6a8+und2kGMWyqSETaGIku6Y=; b=E7qfJwCFbpzQH6baP1IZgIZ8gMWd/hRW74HEDpO01TdqZVEjk+C/HjY68eapttRPOp xNm/Gvnzgzcv+c9v242IRdb42QuNFD6bI8BD/3sdYelV7NfM2oDxlxb4LhvG6Yqv7F/u 4L1Ah+gy5e5424shNfu8D/xBUnCnwEXkhkO88= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to:user-agent; bh=DO/QdORUV6GcvyHJ+4q6a8+und2kGMWyqSETaGIku6Y=; b=OolS//I8aiymj/Tk8jEDAp6y0oQeJTeHduqP67MJiQsWlGsXE0Zpr580MdiK9wSTMg 5rM/JduUuBai1VjtavUvdB32nY82wqonFn9EDHnEcgqcsuOIw56UH1voV6cQ+xHbYXel XE6cS+dh+ByILng1AgTmNosdkloupPdXtbyfy+5gUXGLzjmdU1YS3eQVMtxKHzFgVLn9 crfhUvcuvZ2prdGTf4oqnTFOjTtCUzy/+R3Quqrhn80ZSWbvFa9B9tgTPHEBY7+ntfEn HVBHC7P5Ht4Eop953IaJUvnTjbaHhWya/nnJV7VuaLC/b0BIRRe0z4ERWo/e7BDhJ8Kt jwYA== X-Gm-Message-State: AA+aEWYQ+ykleTsD8/3k1y6lzAlVQROt80VEhIaFGsUVfui/wm9oPnrY CyhL3QCnJiZQsFbNwgyCkTcxfw== X-Google-Smtp-Source: AFSGD/VBrgT/WfzeY6lRKhswhsMxNulpcM0CHnEf4jEic+DTWoJG/X2m6Mazqlid8ZGMws/JtRZFGg== X-Received: by 2002:aa7:c0d0:: with SMTP id j16mr25254356edp.173.1543305365183; Mon, 26 Nov 2018 23:56:05 -0800 (PST) Received: from phenom.ffwll.local ([2a02:168:569e:0:3106:d637:d723:e855]) by smtp.gmail.com with ESMTPSA id s12sm797511edb.43.2018.11.26.23.56.03 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 26 Nov 2018 23:56:04 -0800 (PST) Date: Tue, 27 Nov 2018 08:56:02 +0100 From: Daniel Vetter To: Boris Brezillon Cc: Eric Anholt , =?iso-8859-1?Q?St=E9phane?= 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 ," Subject: Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic. Message-ID: <20181127075602.GV4266@phenom.ffwll.local> Mail-Followup-To: Boris Brezillon , Eric Anholt , =?iso-8859-1?Q?St=E9phane?= 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 ," References: <20181119190805.19139-1-helen.koike@collabora.com> <1d10a4323a49ca09bbb7dc865aaaf5b8c5e3b3d5.camel@crowfest.net> <87o9abbmq4.fsf@anholt.net> <20181126224102.1efbf2d1@bbrezillon> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181126224102.1efbf2d1@bbrezillon> X-Operating-System: Linux phenom 4.18.0-2-amd64 User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 26, 2018 at 10:41:02PM +0100, Boris Brezillon wrote: > 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. For subsuming async page_flip into the async atomic commit stuff we need completion events anyway. Firing those from the EOL (or some other hw-dependent interrupt, could also be the next vblank) should be hard to arrange for drivers. And the heleprs could then easily offload the cleanup_planes work to a worker and delay it until after the event has fired. Like it already does for normal atomic commits. But yeah right now that's all missing. The biggest issue here is figuring out what these events should look like, and how generic userspace needs to use them. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch