From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from galahad.ideasonboard.com ([185.26.127.97]:35189 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932196AbdCFPKl (ORCPT ); Mon, 6 Mar 2017 10:10:41 -0500 From: Laurent Pinchart To: Daniel Vetter Cc: Laurent Pinchart , dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, Kieran Bingham Subject: Re: [PATCH v2] drm: rcar-du: Arm the page flip event after queuing the page flip Date: Mon, 06 Mar 2017 17:11:17 +0200 Message-ID: <1783628.YCkISaBNKo@avalon> In-Reply-To: <20170306150200.wpl3ya6kcybgtpij@phenom.ffwll.local> References: <20170305011032.32747-1-laurent.pinchart+renesas@ideasonboard.com> <20170306150200.wpl3ya6kcybgtpij@phenom.ffwll.local> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: Hi Daniel, On Monday 06 Mar 2017 16:02:00 Daniel Vetter wrote: > On Sun, Mar 05, 2017 at 03:10:32AM +0200, Laurent Pinchart wrote: > > The page flip event is armed in the atomic begin handler, creating a > > race condition with the frame end interrupt that could send the event > > before the atomic operation actually completes. To avoid that, arm the > > event in the atomic flush handler after queuing the page flip. > > > > This change doesn't fully close the race window, as the frame end > > interrupt could be generated before the page flip is committed to > > hardware but only handled after the event is armed. However, the race > > window is now much smaller. > > > > The event must however be armed before calling the VSP atomic commit > > function, otherwise the completion callback could arrive before we arm > > the event, resulting in a deadlock. > > > > Signed-off-by: Laurent Pinchart > > > > Since your hw is not the only one where this seems fundamentally > unfixable, should we document a recommended way to handle/minize the race > window? Sure, that's a good idea. I think it boils down to one of those two options: - If the device can notify of page flip completion, the event should be armed just before the page flip is committed. At the hardware level this can be as simple as an interrupt that will only be fired when a queued page flip is processed (and not at every vblank), or a combination of a GO bit with vblank (which then requires a more complex dance). The R-Car Gen3 hardware falls in this category, the external VSP compositor that handles planes has a GO bit and a vblank-like interrupt. - If the device can't notify of page flip completion, the event should be armed just after the page flip is committed. In the worst case the driver will send the event to userspace one frame too late. This doesn't allow for a real atomic update, but it should avoid tearing. The R-Car Gen2 hardware falls in this category. Feel free to turn this into a kerneldoc patch :-) > > --- > > > > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 22 +++++++++++----------- > > 1 file changed, 11 insertions(+), 11 deletions(-) > > > > Changes since v1: > > > > - Arm the event before calling the VSP atomic commit function > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 7391dd95c733..2aceb84fc15d > > 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > > @@ -502,17 +502,6 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc > > *crtc,> > > struct drm_crtc_state *old_crtc_state) > > > > { > > > > struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); > > > > - struct drm_device *dev = rcrtc->crtc.dev; > > - unsigned long flags; > > - > > - if (crtc->state->event) { > > - WARN_ON(drm_crtc_vblank_get(crtc) != 0); > > - > > - spin_lock_irqsave(&dev->event_lock, flags); > > - rcrtc->event = crtc->state->event; > > - crtc->state->event = NULL; > > - spin_unlock_irqrestore(&dev->event_lock, flags); > > - } > > > > if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) > > > > rcar_du_vsp_atomic_begin(rcrtc); > > > > @@ -522,9 +511,20 @@ static void rcar_du_crtc_atomic_flush(struct drm_crtc > > *crtc,> > > struct drm_crtc_state *old_crtc_state) > > > > { > > > > struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); > > > > + struct drm_device *dev = rcrtc->crtc.dev; > > + unsigned long flags; > > > > rcar_du_crtc_update_planes(rcrtc); > > > > + if (crtc->state->event) { > > + WARN_ON(drm_crtc_vblank_get(crtc) != 0); > > + > > + spin_lock_irqsave(&dev->event_lock, flags); > > + rcrtc->event = crtc->state->event; > > + crtc->state->event = NULL; > > + spin_unlock_irqrestore(&dev->event_lock, flags); > > + } > > + > > > > if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) > > > > rcar_du_vsp_atomic_flush(rcrtc); > > > > } -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v2] drm: rcar-du: Arm the page flip event after queuing the page flip Date: Mon, 06 Mar 2017 17:11:17 +0200 Message-ID: <1783628.YCkISaBNKo@avalon> References: <20170305011032.32747-1-laurent.pinchart+renesas@ideasonboard.com> <20170306150200.wpl3ya6kcybgtpij@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [IPv6:2001:4b98:dc2:45:216:3eff:febb:480d]) by gabe.freedesktop.org (Postfix) with ESMTPS id 76F906E459 for ; Mon, 6 Mar 2017 15:10:41 +0000 (UTC) In-Reply-To: <20170306150200.wpl3ya6kcybgtpij@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: Daniel Vetter Cc: linux-renesas-soc@vger.kernel.org, Laurent Pinchart , Kieran Bingham , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org SGkgRGFuaWVsLAoKT24gTW9uZGF5IDA2IE1hciAyMDE3IDE2OjAyOjAwIERhbmllbCBWZXR0ZXIg d3JvdGU6Cj4gT24gU3VuLCBNYXIgMDUsIDIwMTcgYXQgMDM6MTA6MzJBTSArMDIwMCwgTGF1cmVu dCBQaW5jaGFydCB3cm90ZToKPiA+IFRoZSBwYWdlIGZsaXAgZXZlbnQgaXMgYXJtZWQgaW4gdGhl IGF0b21pYyBiZWdpbiBoYW5kbGVyLCBjcmVhdGluZyBhCj4gPiByYWNlIGNvbmRpdGlvbiB3aXRo IHRoZSBmcmFtZSBlbmQgaW50ZXJydXB0IHRoYXQgY291bGQgc2VuZCB0aGUgZXZlbnQKPiA+IGJl Zm9yZSB0aGUgYXRvbWljIG9wZXJhdGlvbiBhY3R1YWxseSBjb21wbGV0ZXMuIFRvIGF2b2lkIHRo YXQsIGFybSB0aGUKPiA+IGV2ZW50IGluIHRoZSBhdG9taWMgZmx1c2ggaGFuZGxlciBhZnRlciBx dWV1aW5nIHRoZSBwYWdlIGZsaXAuCj4gPiAKPiA+IFRoaXMgY2hhbmdlIGRvZXNuJ3QgZnVsbHkg Y2xvc2UgdGhlIHJhY2Ugd2luZG93LCBhcyB0aGUgZnJhbWUgZW5kCj4gPiBpbnRlcnJ1cHQgY291 bGQgYmUgZ2VuZXJhdGVkIGJlZm9yZSB0aGUgcGFnZSBmbGlwIGlzIGNvbW1pdHRlZCB0bwo+ID4g aGFyZHdhcmUgYnV0IG9ubHkgaGFuZGxlZCBhZnRlciB0aGUgZXZlbnQgaXMgYXJtZWQuIEhvd2V2 ZXIsIHRoZSByYWNlCj4gPiB3aW5kb3cgaXMgbm93IG11Y2ggc21hbGxlci4KPiA+IAo+ID4gVGhl IGV2ZW50IG11c3QgaG93ZXZlciBiZSBhcm1lZCBiZWZvcmUgY2FsbGluZyB0aGUgVlNQIGF0b21p YyBjb21taXQKPiA+IGZ1bmN0aW9uLCBvdGhlcndpc2UgdGhlIGNvbXBsZXRpb24gY2FsbGJhY2sg Y291bGQgYXJyaXZlIGJlZm9yZSB3ZSBhcm0KPiA+IHRoZSBldmVudCwgcmVzdWx0aW5nIGluIGEg ZGVhZGxvY2suCj4gPiAKPiA+IFNpZ25lZC1vZmYtYnk6IExhdXJlbnQgUGluY2hhcnQKPiA+IDxs YXVyZW50LnBpbmNoYXJ0K3JlbmVzYXNAaWRlYXNvbmJvYXJkLmNvbT4KPiAKPiBTaW5jZSB5b3Vy IGh3IGlzIG5vdCB0aGUgb25seSBvbmUgd2hlcmUgdGhpcyBzZWVtcyBmdW5kYW1lbnRhbGx5Cj4g dW5maXhhYmxlLCBzaG91bGQgd2UgZG9jdW1lbnQgYSByZWNvbW1lbmRlZCB3YXkgdG8gaGFuZGxl L21pbml6ZSB0aGUgcmFjZQo+IHdpbmRvdz8KClN1cmUsIHRoYXQncyBhIGdvb2QgaWRlYS4KCkkg dGhpbmsgaXQgYm9pbHMgZG93biB0byBvbmUgb2YgdGhvc2UgdHdvIG9wdGlvbnM6CgotIElmIHRo ZSBkZXZpY2UgY2FuIG5vdGlmeSBvZiBwYWdlIGZsaXAgY29tcGxldGlvbiwgdGhlIGV2ZW50IHNo b3VsZCBiZSBhcm1lZCAKanVzdCBiZWZvcmUgdGhlIHBhZ2UgZmxpcCBpcyBjb21taXR0ZWQuIEF0 IHRoZSBoYXJkd2FyZSBsZXZlbCB0aGlzIGNhbiBiZSBhcyAKc2ltcGxlIGFzIGFuIGludGVycnVw dCB0aGF0IHdpbGwgb25seSBiZSBmaXJlZCB3aGVuIGEgcXVldWVkIHBhZ2UgZmxpcCBpcyAKcHJv Y2Vzc2VkIChhbmQgbm90IGF0IGV2ZXJ5IHZibGFuayksIG9yIGEgY29tYmluYXRpb24gb2YgYSBH TyBiaXQgd2l0aCB2YmxhbmsgCih3aGljaCB0aGVuIHJlcXVpcmVzIGEgbW9yZSBjb21wbGV4IGRh bmNlKS4gVGhlIFItQ2FyIEdlbjMgaGFyZHdhcmUgZmFsbHMgaW4gCnRoaXMgY2F0ZWdvcnksIHRo ZSBleHRlcm5hbCBWU1AgY29tcG9zaXRvciB0aGF0IGhhbmRsZXMgcGxhbmVzIGhhcyBhIEdPIGJp dCAKYW5kIGEgdmJsYW5rLWxpa2UgaW50ZXJydXB0LgoKLSBJZiB0aGUgZGV2aWNlIGNhbid0IG5v dGlmeSBvZiBwYWdlIGZsaXAgY29tcGxldGlvbiwgdGhlIGV2ZW50IHNob3VsZCBiZSAKYXJtZWQg anVzdCBhZnRlciB0aGUgcGFnZSBmbGlwIGlzIGNvbW1pdHRlZC4gSW4gdGhlIHdvcnN0IGNhc2Ug dGhlIGRyaXZlciB3aWxsIApzZW5kIHRoZSBldmVudCB0byB1c2Vyc3BhY2Ugb25lIGZyYW1lIHRv byBsYXRlLiBUaGlzIGRvZXNuJ3QgYWxsb3cgZm9yIGEgcmVhbCAKYXRvbWljIHVwZGF0ZSwgYnV0 IGl0IHNob3VsZCBhdm9pZCB0ZWFyaW5nLiBUaGUgUi1DYXIgR2VuMiBoYXJkd2FyZSBmYWxscyBp biAKdGhpcyBjYXRlZ29yeS4KCkZlZWwgZnJlZSB0byB0dXJuIHRoaXMgaW50byBhIGtlcm5lbGRv YyBwYXRjaCA6LSkKCj4gPiAtLS0KPiA+IAo+ID4gIGRyaXZlcnMvZ3B1L2RybS9yY2FyLWR1L3Jj YXJfZHVfY3J0Yy5jIHwgMjIgKysrKysrKysrKystLS0tLS0tLS0tLQo+ID4gIDEgZmlsZSBjaGFu Z2VkLCAxMSBpbnNlcnRpb25zKCspLCAxMSBkZWxldGlvbnMoLSkKPiA+IAo+ID4gQ2hhbmdlcyBz aW5jZSB2MToKPiA+IAo+ID4gLSBBcm0gdGhlIGV2ZW50IGJlZm9yZSBjYWxsaW5nIHRoZSBWU1Ag YXRvbWljIGNvbW1pdCBmdW5jdGlvbgo+ID4gCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUv ZHJtL3JjYXItZHUvcmNhcl9kdV9jcnRjLmMKPiA+IGIvZHJpdmVycy9ncHUvZHJtL3JjYXItZHUv cmNhcl9kdV9jcnRjLmMgaW5kZXggNzM5MWRkOTVjNzMzLi4yYWNlYjg0ZmMxNWQKPiA+IDEwMDY0 NAo+ID4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV9jcnRjLmMKPiA+ICsr KyBiL2RyaXZlcnMvZ3B1L2RybS9yY2FyLWR1L3JjYXJfZHVfY3J0Yy5jCj4gPiBAQCAtNTAyLDE3 ICs1MDIsNiBAQCBzdGF0aWMgdm9pZCByY2FyX2R1X2NydGNfYXRvbWljX2JlZ2luKHN0cnVjdCBk cm1fY3J0Ywo+ID4gKmNydGMsPiAKPiA+ICAJCQkJICAgICAgc3RydWN0IGRybV9jcnRjX3N0YXRl ICpvbGRfY3J0Y19zdGF0ZSkKPiA+ICAKPiA+ICB7Cj4gPiAgCj4gPiAgCXN0cnVjdCByY2FyX2R1 X2NydGMgKnJjcnRjID0gdG9fcmNhcl9jcnRjKGNydGMpOwo+ID4gCj4gPiAtCXN0cnVjdCBkcm1f ZGV2aWNlICpkZXYgPSByY3J0Yy0+Y3J0Yy5kZXY7Cj4gPiAtCXVuc2lnbmVkIGxvbmcgZmxhZ3M7 Cj4gPiAtCj4gPiAtCWlmIChjcnRjLT5zdGF0ZS0+ZXZlbnQpIHsKPiA+IC0JCVdBUk5fT04oZHJt X2NydGNfdmJsYW5rX2dldChjcnRjKSAhPSAwKTsKPiA+IC0KPiA+IC0JCXNwaW5fbG9ja19pcnFz YXZlKCZkZXYtPmV2ZW50X2xvY2ssIGZsYWdzKTsKPiA+IC0JCXJjcnRjLT5ldmVudCA9IGNydGMt PnN0YXRlLT5ldmVudDsKPiA+IC0JCWNydGMtPnN0YXRlLT5ldmVudCA9IE5VTEw7Cj4gPiAtCQlz cGluX3VubG9ja19pcnFyZXN0b3JlKCZkZXYtPmV2ZW50X2xvY2ssIGZsYWdzKTsKPiA+IC0JfQo+ ID4gCj4gPiAgCWlmIChyY2FyX2R1X2hhcyhyY3J0Yy0+Z3JvdXAtPmRldiwgUkNBUl9EVV9GRUFU VVJFX1ZTUDFfU09VUkNFKSkKPiA+ICAJCj4gPiAgCQlyY2FyX2R1X3ZzcF9hdG9taWNfYmVnaW4o cmNydGMpOwo+ID4gCj4gPiBAQCAtNTIyLDkgKzUxMSwyMCBAQCBzdGF0aWMgdm9pZCByY2FyX2R1 X2NydGNfYXRvbWljX2ZsdXNoKHN0cnVjdCBkcm1fY3J0Ywo+ID4gKmNydGMsPiAKPiA+ICAJCQkJ ICAgICAgc3RydWN0IGRybV9jcnRjX3N0YXRlICpvbGRfY3J0Y19zdGF0ZSkKPiA+ICAKPiA+ICB7 Cj4gPiAgCj4gPiAgCXN0cnVjdCByY2FyX2R1X2NydGMgKnJjcnRjID0gdG9fcmNhcl9jcnRjKGNy dGMpOwo+ID4gCj4gPiArCXN0cnVjdCBkcm1fZGV2aWNlICpkZXYgPSByY3J0Yy0+Y3J0Yy5kZXY7 Cj4gPiArCXVuc2lnbmVkIGxvbmcgZmxhZ3M7Cj4gPiAKPiA+ICAJcmNhcl9kdV9jcnRjX3VwZGF0 ZV9wbGFuZXMocmNydGMpOwo+ID4gCj4gPiArCWlmIChjcnRjLT5zdGF0ZS0+ZXZlbnQpIHsKPiA+ ICsJCVdBUk5fT04oZHJtX2NydGNfdmJsYW5rX2dldChjcnRjKSAhPSAwKTsKPiA+ICsKPiA+ICsJ CXNwaW5fbG9ja19pcnFzYXZlKCZkZXYtPmV2ZW50X2xvY2ssIGZsYWdzKTsKPiA+ICsJCXJjcnRj LT5ldmVudCA9IGNydGMtPnN0YXRlLT5ldmVudDsKPiA+ICsJCWNydGMtPnN0YXRlLT5ldmVudCA9 IE5VTEw7Cj4gPiArCQlzcGluX3VubG9ja19pcnFyZXN0b3JlKCZkZXYtPmV2ZW50X2xvY2ssIGZs YWdzKTsKPiA+ICsJfQo+ID4gKwo+ID4gCj4gPiAgCWlmIChyY2FyX2R1X2hhcyhyY3J0Yy0+Z3Jv dXAtPmRldiwgUkNBUl9EVV9GRUFUVVJFX1ZTUDFfU09VUkNFKSkKPiA+ICAJCj4gPiAgCQlyY2Fy X2R1X3ZzcF9hdG9taWNfZmx1c2gocmNydGMpOwo+ID4gIAo+ID4gIH0KCi0tIApSZWdhcmRzLAoK TGF1cmVudCBQaW5jaGFydAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0 b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJp LWRldmVsCg==