From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from galahad.ideasonboard.com ([185.26.127.97]:35588 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750779AbdCCCZL (ORCPT ); Thu, 2 Mar 2017 21:25:11 -0500 From: Laurent Pinchart To: Kieran Bingham Cc: linux-renesas-soc@vger.kernel.org, linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [RFC PATCH 3/3] drm: rcar-du: Register a completion callback with VSP1 Date: Fri, 03 Mar 2017 04:17:39 +0200 Message-ID: <1896383.LZRWDRHL8Z@avalon> In-Reply-To: References: 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 Kieran, Thank you for the patch. On Wednesday 01 Mar 2017 13:12:56 Kieran Bingham wrote: > Updating the state in a running VSP1 requires two interrupts from the > VSP. Initially, the updated state will be committed - but only after the > VSP1 has completed processing it's current frame will the new state be > taken into account. As such, the committed state will only be 'completed' > after an extra frame completion interrupt. > > Track this delay, by passing the frame flip event through the VSP > module; It will be returned only when the frame has completed and can be > returned to the caller. I'll check the interrupt sequence logic tomorrow, it's a bit too late now. Please see below for additional comments. > Signed-off-by: Kieran Bingham > --- > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 8 +++++- > drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 1 +- > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 34 ++++++++++++++++++++++++++- > 3 files changed, 41 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 7391dd95c733..0a824633a012 > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > @@ -328,7 +328,7 @@ static bool rcar_du_crtc_page_flip_pending(struct > rcar_du_crtc *rcrtc) bool pending; > > spin_lock_irqsave(&dev->event_lock, flags); > - pending = rcrtc->event != NULL; > + pending = (rcrtc->event != NULL) || (rcrtc->pending != NULL); > spin_unlock_irqrestore(&dev->event_lock, flags); > > return pending; > @@ -578,6 +578,12 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg) > rcar_du_crtc_write(rcrtc, DSRCR, status & DSRCR_MASK); > > if (status & DSSR_FRM) { > + > + if (rcrtc->pending) { > + trace_printk("VBlank loss due to VSP Overrun\n"); > + return IRQ_HANDLED; > + } > + > drm_crtc_handle_vblank(&rcrtc->crtc); > rcar_du_crtc_finish_page_flip(rcrtc); > ret = IRQ_HANDLED; > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index a7194812997e..8374a858446a > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > @@ -46,6 +46,7 @@ struct rcar_du_crtc { > bool started; > > struct drm_pending_vblank_event *event; > + struct drm_pending_vblank_event *pending; > wait_queue_head_t flip_wait; > > unsigned int outputs; > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 71e70e1e0881..408375aff1a0 > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > @@ -28,6 +28,26 @@ > #include "rcar_du_kms.h" > #include "rcar_du_vsp.h" > > +static void rcar_du_vsp_complete(void *private, void *data) > +{ > + struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private; > + struct drm_device *dev = crtc->crtc.dev; > + struct drm_pending_vblank_event *event; > + bool match; > + unsigned long flags; > + > + spin_lock_irqsave(&dev->event_lock, flags); > + event = crtc->event; > + crtc->event = data; > + match = (crtc->event == crtc->pending); > + crtc->pending = NULL; > + spin_unlock_irqrestore(&dev->event_lock, flags); > + > + /* Safety checks */ > + WARN(event, "Event lost by VSP completion callback\n"); > + WARN(!match, "Stored pending event, does not match completion\n"); I understand you want to be safe, and I assume these have never been triggered in your tests. I'd rather replace them by a mechanism that doesn't require passing the event to the VSP driver, and that wouldn't require adding a pending field to the rcar_du_crtc structure. Wouldn't adding a WARN_ON(rcrtc- >event) in rcar_du_crtc_atomic_begin() in the if (crtc->state->event) block do the job ? Would this get in the way of your trace_printk() debugging in rcar_du_crtc_irq() ? Could you implement the debugging in a separate patch without requiring to pass the event to the VSP driver ? > +} > + > void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) > { > const struct drm_display_mode *mode = &crtc->crtc.state- >adjusted_mode; > @@ -66,6 +86,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) > */ > crtc->group->need_restart = true; > > + vsp1_du_register_callback(crtc->vsp->vsp, rcar_du_vsp_complete, crtc); > + > vsp1_du_setup_lif(crtc->vsp->vsp, mode->hdisplay, mode->vdisplay); > } > > @@ -81,7 +103,17 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) > > void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc) > { > - vsp1_du_atomic_flush(crtc->vsp->vsp, NULL); > + struct drm_device *dev = crtc->crtc.dev; > + struct drm_pending_vblank_event *event; > + unsigned long flags; > + > + /* Move the event to the VSP, track it locally as 'pending' */ > + spin_lock_irqsave(&dev->event_lock, flags); > + event = crtc->pending = crtc->event; > + crtc->event = NULL; > + spin_unlock_irqrestore(&dev->event_lock, flags); > + > + vsp1_du_atomic_flush(crtc->vsp->vsp, event); > } > > /* Keep the two tables in sync. */ -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [RFC PATCH 3/3] drm: rcar-du: Register a completion callback with VSP1 Date: Fri, 03 Mar 2017 04:17:39 +0200 Message-ID: <1896383.LZRWDRHL8Z@avalon> References: 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 [185.26.127.97]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8A6DF6E25A for ; Fri, 3 Mar 2017 02:17:04 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Kieran Bingham Cc: linux-renesas-soc@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org SGkgS2llcmFuLAoKVGhhbmsgeW91IGZvciB0aGUgcGF0Y2guCgpPbiBXZWRuZXNkYXkgMDEgTWFy IDIwMTcgMTM6MTI6NTYgS2llcmFuIEJpbmdoYW0gd3JvdGU6Cj4gVXBkYXRpbmcgdGhlIHN0YXRl IGluIGEgcnVubmluZyBWU1AxIHJlcXVpcmVzIHR3byBpbnRlcnJ1cHRzIGZyb20gdGhlCj4gVlNQ LiBJbml0aWFsbHksIHRoZSB1cGRhdGVkIHN0YXRlIHdpbGwgYmUgY29tbWl0dGVkIC0gYnV0IG9u bHkgYWZ0ZXIgdGhlCj4gVlNQMSBoYXMgY29tcGxldGVkIHByb2Nlc3NpbmcgaXQncyBjdXJyZW50 IGZyYW1lIHdpbGwgdGhlIG5ldyBzdGF0ZSBiZQo+IHRha2VuIGludG8gYWNjb3VudC4gQXMgc3Vj aCwgdGhlIGNvbW1pdHRlZCBzdGF0ZSB3aWxsIG9ubHkgYmUgJ2NvbXBsZXRlZCcKPiBhZnRlciBh biBleHRyYSBmcmFtZSBjb21wbGV0aW9uIGludGVycnVwdC4KPiAKPiBUcmFjayB0aGlzIGRlbGF5 LCBieSBwYXNzaW5nIHRoZSBmcmFtZSBmbGlwIGV2ZW50IHRocm91Z2ggdGhlIFZTUAo+IG1vZHVs ZTsgSXQgd2lsbCBiZSByZXR1cm5lZCBvbmx5IHdoZW4gdGhlIGZyYW1lIGhhcyBjb21wbGV0ZWQg YW5kIGNhbiBiZQo+IHJldHVybmVkIHRvIHRoZSBjYWxsZXIuCgpJJ2xsIGNoZWNrIHRoZSBpbnRl cnJ1cHQgc2VxdWVuY2UgbG9naWMgdG9tb3Jyb3csIGl0J3MgYSBiaXQgdG9vIGxhdGUgbm93LiAK UGxlYXNlIHNlZSBiZWxvdyBmb3IgYWRkaXRpb25hbCBjb21tZW50cy4KCj4gU2lnbmVkLW9mZi1i eTogS2llcmFuIEJpbmdoYW0gPGtpZXJhbi5iaW5naGFtK3JlbmVzYXNAaWRlYXNvbmJvYXJkLmNv bT4KPiAtLS0KPiAgZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV9jcnRjLmMgfCAgOCAr KysrKy0KPiAgZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV9jcnRjLmggfCAgMSArLQo+ ICBkcml2ZXJzL2dwdS9kcm0vcmNhci1kdS9yY2FyX2R1X3ZzcC5jICB8IDM0ICsrKysrKysrKysr KysrKysrKysrKysrKysrLQo+ICAzIGZpbGVzIGNoYW5nZWQsIDQxIGluc2VydGlvbnMoKyksIDIg ZGVsZXRpb25zKC0pCj4gCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9yY2FyLWR1L3Jj YXJfZHVfY3J0Yy5jCj4gYi9kcml2ZXJzL2dwdS9kcm0vcmNhci1kdS9yY2FyX2R1X2NydGMuYyBp bmRleCA3MzkxZGQ5NWM3MzMuLjBhODI0NjMzYTAxMgo+IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMv Z3B1L2RybS9yY2FyLWR1L3JjYXJfZHVfY3J0Yy5jCj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL3Jj YXItZHUvcmNhcl9kdV9jcnRjLmMKPiBAQCAtMzI4LDcgKzMyOCw3IEBAIHN0YXRpYyBib29sIHJj YXJfZHVfY3J0Y19wYWdlX2ZsaXBfcGVuZGluZyhzdHJ1Y3QKPiByY2FyX2R1X2NydGMgKnJjcnRj KSBib29sIHBlbmRpbmc7Cj4gCj4gIAlzcGluX2xvY2tfaXJxc2F2ZSgmZGV2LT5ldmVudF9sb2Nr LCBmbGFncyk7Cj4gLQlwZW5kaW5nID0gcmNydGMtPmV2ZW50ICE9IE5VTEw7Cj4gKwlwZW5kaW5n ID0gKHJjcnRjLT5ldmVudCAhPSBOVUxMKSB8fCAocmNydGMtPnBlbmRpbmcgIT0gTlVMTCk7Cj4g IAlzcGluX3VubG9ja19pcnFyZXN0b3JlKCZkZXYtPmV2ZW50X2xvY2ssIGZsYWdzKTsKPiAKPiAg CXJldHVybiBwZW5kaW5nOwo+IEBAIC01NzgsNiArNTc4LDEyIEBAIHN0YXRpYyBpcnFyZXR1cm5f dCByY2FyX2R1X2NydGNfaXJxKGludCBpcnEsIHZvaWQgKmFyZykKPiByY2FyX2R1X2NydGNfd3Jp dGUocmNydGMsIERTUkNSLCBzdGF0dXMgJiBEU1JDUl9NQVNLKTsKPiAKPiAgCWlmIChzdGF0dXMg JiBEU1NSX0ZSTSkgewo+ICsKPiArCQlpZiAocmNydGMtPnBlbmRpbmcpIHsKPiArCQkJdHJhY2Vf cHJpbnRrKCJWQmxhbmsgbG9zcyBkdWUgdG8gVlNQIE92ZXJydW5cbiIpOwo+ICsJCQlyZXR1cm4g SVJRX0hBTkRMRUQ7Cj4gKwkJfQo+ICsKPiAgCQlkcm1fY3J0Y19oYW5kbGVfdmJsYW5rKCZyY3J0 Yy0+Y3J0Yyk7Cj4gIAkJcmNhcl9kdV9jcnRjX2ZpbmlzaF9wYWdlX2ZsaXAocmNydGMpOwo+ICAJ CXJldCA9IElSUV9IQU5ETEVEOwo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vcmNhci1k dS9yY2FyX2R1X2NydGMuaAo+IGIvZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV9jcnRj LmggaW5kZXggYTcxOTQ4MTI5OTdlLi44Mzc0YTg1ODQ0NmEKPiAxMDA2NDQKPiAtLS0gYS9kcml2 ZXJzL2dwdS9kcm0vcmNhci1kdS9yY2FyX2R1X2NydGMuaAo+ICsrKyBiL2RyaXZlcnMvZ3B1L2Ry bS9yY2FyLWR1L3JjYXJfZHVfY3J0Yy5oCj4gQEAgLTQ2LDYgKzQ2LDcgQEAgc3RydWN0IHJjYXJf ZHVfY3J0YyB7Cj4gIAlib29sIHN0YXJ0ZWQ7Cj4gCj4gIAlzdHJ1Y3QgZHJtX3BlbmRpbmdfdmJs YW5rX2V2ZW50ICpldmVudDsKPiArCXN0cnVjdCBkcm1fcGVuZGluZ192YmxhbmtfZXZlbnQgKnBl bmRpbmc7Cj4gIAl3YWl0X3F1ZXVlX2hlYWRfdCBmbGlwX3dhaXQ7Cj4gCj4gIAl1bnNpZ25lZCBp bnQgb3V0cHV0czsKPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9k dV92c3AuYwo+IGIvZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV92c3AuYyBpbmRleCA3 MWU3MGUxZTA4ODEuLjQwODM3NWFmZjFhMAo+IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvZ3B1L2Ry bS9yY2FyLWR1L3JjYXJfZHVfdnNwLmMKPiArKysgYi9kcml2ZXJzL2dwdS9kcm0vcmNhci1kdS9y Y2FyX2R1X3ZzcC5jCj4gQEAgLTI4LDYgKzI4LDI2IEBACj4gICNpbmNsdWRlICJyY2FyX2R1X2tt cy5oIgo+ICAjaW5jbHVkZSAicmNhcl9kdV92c3AuaCIKPiAKPiArc3RhdGljIHZvaWQgcmNhcl9k dV92c3BfY29tcGxldGUodm9pZCAqcHJpdmF0ZSwgdm9pZCAqZGF0YSkKPiArewo+ICsJc3RydWN0 IHJjYXJfZHVfY3J0YyAqY3J0YyA9IChzdHJ1Y3QgcmNhcl9kdV9jcnRjICopcHJpdmF0ZTsKPiAr CXN0cnVjdCBkcm1fZGV2aWNlICpkZXYgPSBjcnRjLT5jcnRjLmRldjsKPiArCXN0cnVjdCBkcm1f cGVuZGluZ192YmxhbmtfZXZlbnQgKmV2ZW50Owo+ICsJYm9vbCBtYXRjaDsKPiArCXVuc2lnbmVk IGxvbmcgZmxhZ3M7Cj4gKwo+ICsJc3Bpbl9sb2NrX2lycXNhdmUoJmRldi0+ZXZlbnRfbG9jaywg ZmxhZ3MpOwo+ICsJZXZlbnQgPSBjcnRjLT5ldmVudDsKPiArCWNydGMtPmV2ZW50ID0gZGF0YTsK PiArCW1hdGNoID0gKGNydGMtPmV2ZW50ID09IGNydGMtPnBlbmRpbmcpOwo+ICsJY3J0Yy0+cGVu ZGluZyA9IE5VTEw7Cj4gKwlzcGluX3VubG9ja19pcnFyZXN0b3JlKCZkZXYtPmV2ZW50X2xvY2ss IGZsYWdzKTsKPiArCj4gKwkvKiBTYWZldHkgY2hlY2tzICovCj4gKwlXQVJOKGV2ZW50LCAiRXZl bnQgbG9zdCBieSBWU1AgY29tcGxldGlvbiBjYWxsYmFja1xuIik7Cj4gKwlXQVJOKCFtYXRjaCwg IlN0b3JlZCBwZW5kaW5nIGV2ZW50LCBkb2VzIG5vdCBtYXRjaCBjb21wbGV0aW9uXG4iKTsKCkkg dW5kZXJzdGFuZCB5b3Ugd2FudCB0byBiZSBzYWZlLCBhbmQgSSBhc3N1bWUgdGhlc2UgaGF2ZSBu ZXZlciBiZWVuIHRyaWdnZXJlZCAKaW4geW91ciB0ZXN0cy4gSSdkIHJhdGhlciByZXBsYWNlIHRo ZW0gYnkgYSBtZWNoYW5pc20gdGhhdCBkb2Vzbid0IHJlcXVpcmUgCnBhc3NpbmcgdGhlIGV2ZW50 IHRvIHRoZSBWU1AgZHJpdmVyLCBhbmQgdGhhdCB3b3VsZG4ndCByZXF1aXJlIGFkZGluZyBhIApw ZW5kaW5nIGZpZWxkIHRvIHRoZSByY2FyX2R1X2NydGMgc3RydWN0dXJlLiBXb3VsZG4ndCBhZGRp bmcgYSBXQVJOX09OKHJjcnRjLQo+ZXZlbnQpIGluIHJjYXJfZHVfY3J0Y19hdG9taWNfYmVnaW4o KSBpbiB0aGUgaWYgKGNydGMtPnN0YXRlLT5ldmVudCkgYmxvY2sgZG8gCnRoZSBqb2IgPwoKV291 bGQgdGhpcyBnZXQgaW4gdGhlIHdheSBvZiB5b3VyIHRyYWNlX3ByaW50aygpIGRlYnVnZ2luZyBp biAKcmNhcl9kdV9jcnRjX2lycSgpID8gQ291bGQgeW91IGltcGxlbWVudCB0aGUgZGVidWdnaW5n IGluIGEgc2VwYXJhdGUgcGF0Y2ggCndpdGhvdXQgcmVxdWlyaW5nIHRvIHBhc3MgdGhlIGV2ZW50 IHRvIHRoZSBWU1AgZHJpdmVyID8KCj4gK30KPiArCj4gIHZvaWQgcmNhcl9kdV92c3BfZW5hYmxl KHN0cnVjdCByY2FyX2R1X2NydGMgKmNydGMpCj4gIHsKPiAgCWNvbnN0IHN0cnVjdCBkcm1fZGlz cGxheV9tb2RlICptb2RlID0gJmNydGMtPmNydGMuc3RhdGUtCj5hZGp1c3RlZF9tb2RlOwo+IEBA IC02Niw2ICs4Niw4IEBAIHZvaWQgcmNhcl9kdV92c3BfZW5hYmxlKHN0cnVjdCByY2FyX2R1X2Ny dGMgKmNydGMpCj4gIAkgKi8KPiAgCWNydGMtPmdyb3VwLT5uZWVkX3Jlc3RhcnQgPSB0cnVlOwo+ IAo+ICsJdnNwMV9kdV9yZWdpc3Rlcl9jYWxsYmFjayhjcnRjLT52c3AtPnZzcCwgcmNhcl9kdV92 c3BfY29tcGxldGUsIGNydGMpOwo+ICsKPiAgCXZzcDFfZHVfc2V0dXBfbGlmKGNydGMtPnZzcC0+ dnNwLCBtb2RlLT5oZGlzcGxheSwgbW9kZS0+dmRpc3BsYXkpOwo+ICB9Cj4gCj4gQEAgLTgxLDcg KzEwMywxNyBAQCB2b2lkIHJjYXJfZHVfdnNwX2F0b21pY19iZWdpbihzdHJ1Y3QgcmNhcl9kdV9j cnRjICpjcnRjKQo+IAo+ICB2b2lkIHJjYXJfZHVfdnNwX2F0b21pY19mbHVzaChzdHJ1Y3QgcmNh cl9kdV9jcnRjICpjcnRjKQo+ICB7Cj4gLQl2c3AxX2R1X2F0b21pY19mbHVzaChjcnRjLT52c3At PnZzcCwgTlVMTCk7Cj4gKwlzdHJ1Y3QgZHJtX2RldmljZSAqZGV2ID0gY3J0Yy0+Y3J0Yy5kZXY7 Cj4gKwlzdHJ1Y3QgZHJtX3BlbmRpbmdfdmJsYW5rX2V2ZW50ICpldmVudDsKPiArCXVuc2lnbmVk IGxvbmcgZmxhZ3M7Cj4gKwo+ICsJLyogTW92ZSB0aGUgZXZlbnQgdG8gdGhlIFZTUCwgdHJhY2sg aXQgbG9jYWxseSBhcyAncGVuZGluZycgKi8KPiArCXNwaW5fbG9ja19pcnFzYXZlKCZkZXYtPmV2 ZW50X2xvY2ssIGZsYWdzKTsKPiArCWV2ZW50ID0gY3J0Yy0+cGVuZGluZyA9IGNydGMtPmV2ZW50 Owo+ICsJY3J0Yy0+ZXZlbnQgPSBOVUxMOwo+ICsJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmZGV2 LT5ldmVudF9sb2NrLCBmbGFncyk7Cj4gKwo+ICsJdnNwMV9kdV9hdG9taWNfZmx1c2goY3J0Yy0+ dnNwLT52c3AsIGV2ZW50KTsKPiAgfQo+IAo+ICAvKiBLZWVwIHRoZSB0d28gdGFibGVzIGluIHN5 bmMuICovCgotLSAKUmVnYXJkcywKCkxhdXJlbnQgUGluY2hhcnQKCl9fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJp LWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9y Zy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo=