From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from galahad.ideasonboard.com ([185.26.127.97]:59213 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752425AbdCEV6K (ORCPT ); Sun, 5 Mar 2017 16:58:10 -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: [PATCH v3 1/3] v4l: vsp1: Postpone frame end handling in event of display list race Date: Sun, 05 Mar 2017 23:58:45 +0200 Message-ID: <4368649.e29LHi5jnS@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 Sunday 05 Mar 2017 16:00:02 Kieran Bingham wrote: > If we try to commit the display list while an update is pending, we have > missed our opportunity. The display list manager will hold the commit > until the next interrupt. > > In this event, we skip the pipeline completion callback handler so that > the pipeline will not mistakenly report frame completion to the user. > > Signed-off-by: Kieran Bingham Reviewed-by: Laurent Pinchart > --- > drivers/media/platform/vsp1/vsp1_dl.c | 19 +++++++++++++++++-- > drivers/media/platform/vsp1/vsp1_dl.h | 2 +- > drivers/media/platform/vsp1/vsp1_pipe.c | 13 ++++++++++++- > 3 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/vsp1/vsp1_dl.c > b/drivers/media/platform/vsp1/vsp1_dl.c index b9e5027778ff..f449ca689554 > 100644 > --- a/drivers/media/platform/vsp1/vsp1_dl.c > +++ b/drivers/media/platform/vsp1/vsp1_dl.c > @@ -562,9 +562,19 @@ void vsp1_dlm_irq_display_start(struct vsp1_dl_manager > *dlm) spin_unlock(&dlm->lock); > } > > -void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm) > +/** > + * vsp1_dlm_irq_frame_end - Display list handler for the frame end > interrupt + * @dlm: the display list manager > + * > + * Return true if the previous display list has completed at frame end, or > false + * if it has been delayed by one frame because the display list > commit raced + * with the frame end interrupt. The function always returns > true in header mode + * as display list processing is then not continuous > and races never occur. + */ > +bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm) > { > struct vsp1_device *vsp1 = dlm->vsp1; > + bool completed = false; > > spin_lock(&dlm->lock); > > @@ -576,8 +586,10 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager > *dlm) * perform any operation as there can't be any new display list queued > * in that case. > */ > - if (dlm->mode == VSP1_DL_MODE_HEADER) > + if (dlm->mode == VSP1_DL_MODE_HEADER) { > + completed = true; > goto done; > + } > > /* > * The UPD bit set indicates that the commit operation raced with the > @@ -597,6 +609,7 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm) > if (dlm->queued) { > dlm->active = dlm->queued; > dlm->queued = NULL; > + completed = true; > } > > /* > @@ -619,6 +632,8 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm) > > done: > spin_unlock(&dlm->lock); > + > + return completed; > } > > /* Hardware Setup */ > diff --git a/drivers/media/platform/vsp1/vsp1_dl.h > b/drivers/media/platform/vsp1/vsp1_dl.h index 7131aa3c5978..6ec1380a10af > 100644 > --- a/drivers/media/platform/vsp1/vsp1_dl.h > +++ b/drivers/media/platform/vsp1/vsp1_dl.h > @@ -28,7 +28,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device > *vsp1, void vsp1_dlm_destroy(struct vsp1_dl_manager *dlm); > void vsp1_dlm_reset(struct vsp1_dl_manager *dlm); > void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm); > -void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm); > +bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm); > > struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm); > void vsp1_dl_list_put(struct vsp1_dl_list *dl); > diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c > b/drivers/media/platform/vsp1/vsp1_pipe.c index 35364f594e19..d15327701ad8 > 100644 > --- a/drivers/media/platform/vsp1/vsp1_pipe.c > +++ b/drivers/media/platform/vsp1/vsp1_pipe.c > @@ -304,10 +304,21 @@ bool vsp1_pipeline_ready(struct vsp1_pipeline *pipe) > > void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe) > { > + bool completed; > + > if (pipe == NULL) > return; > > - vsp1_dlm_irq_frame_end(pipe->output->dlm); > + completed = vsp1_dlm_irq_frame_end(pipe->output->dlm); > + if (!completed) { > + /* > + * If the DL commit raced with the frame end interrupt, the > + * commit ends up being postponed by one frame. Return > + * immediately without calling the pipeline's frame end handler > + * or incrementing the sequence number. > + */ > + return; > + } > > if (pipe->frame_end) > pipe->frame_end(pipe); -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v3 1/3] v4l: vsp1: Postpone frame end handling in event of display list race Date: Sun, 05 Mar 2017 23:58:45 +0200 Message-ID: <4368649.e29LHi5jnS@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 17BC16E302 for ; Sun, 5 Mar 2017 21:58:10 +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 SGkgS2llcmFuLAoKVGhhbmsgeW91IGZvciB0aGUgcGF0Y2guCgpPbiBTdW5kYXkgMDUgTWFyIDIw MTcgMTY6MDA6MDIgS2llcmFuIEJpbmdoYW0gd3JvdGU6Cj4gSWYgd2UgdHJ5IHRvIGNvbW1pdCB0 aGUgZGlzcGxheSBsaXN0IHdoaWxlIGFuIHVwZGF0ZSBpcyBwZW5kaW5nLCB3ZSBoYXZlCj4gbWlz c2VkIG91ciBvcHBvcnR1bml0eS4gVGhlIGRpc3BsYXkgbGlzdCBtYW5hZ2VyIHdpbGwgaG9sZCB0 aGUgY29tbWl0Cj4gdW50aWwgdGhlIG5leHQgaW50ZXJydXB0Lgo+IAo+IEluIHRoaXMgZXZlbnQs IHdlIHNraXAgdGhlIHBpcGVsaW5lIGNvbXBsZXRpb24gY2FsbGJhY2sgaGFuZGxlciBzbyB0aGF0 Cj4gdGhlIHBpcGVsaW5lIHdpbGwgbm90IG1pc3Rha2VubHkgcmVwb3J0IGZyYW1lIGNvbXBsZXRp b24gdG8gdGhlIHVzZXIuCj4gCj4gU2lnbmVkLW9mZi1ieTogS2llcmFuIEJpbmdoYW0gPGtpZXJh bi5iaW5naGFtK3JlbmVzYXNAaWRlYXNvbmJvYXJkLmNvbT4KClJldmlld2VkLWJ5OiBMYXVyZW50 IFBpbmNoYXJ0IDxsYXVyZW50LnBpbmNoYXJ0QGlkZWFzb25ib2FyZC5jb20+Cgo+IC0tLQo+ICBk cml2ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNwMV9kbC5jICAgfCAxOSArKysrKysrKysrKysr KysrKy0tCj4gIGRyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3AxX2RsLmggICB8ICAyICst Cj4gIGRyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3AxX3BpcGUuYyB8IDEzICsrKysrKysr KysrKy0KPiAgMyBmaWxlcyBjaGFuZ2VkLCAzMCBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9ucygt KQo+IAo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNwMV9kbC5j Cj4gYi9kcml2ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNwMV9kbC5jIGluZGV4IGI5ZTUwMjc3 NzhmZi4uZjQ0OWNhNjg5NTU0Cj4gMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy9tZWRpYS9wbGF0Zm9y bS92c3AxL3ZzcDFfZGwuYwo+ICsrKyBiL2RyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3Ax X2RsLmMKPiBAQCAtNTYyLDkgKzU2MiwxOSBAQCB2b2lkIHZzcDFfZGxtX2lycV9kaXNwbGF5X3N0 YXJ0KHN0cnVjdCB2c3AxX2RsX21hbmFnZXIKPiAqZGxtKSBzcGluX3VubG9jaygmZGxtLT5sb2Nr KTsKPiAgfQo+IAo+IC12b2lkIHZzcDFfZGxtX2lycV9mcmFtZV9lbmQoc3RydWN0IHZzcDFfZGxf bWFuYWdlciAqZGxtKQo+ICsvKioKPiArICogdnNwMV9kbG1faXJxX2ZyYW1lX2VuZCAtIERpc3Bs YXkgbGlzdCBoYW5kbGVyIGZvciB0aGUgZnJhbWUgZW5kCj4gaW50ZXJydXB0ICsgKiBAZGxtOiB0 aGUgZGlzcGxheSBsaXN0IG1hbmFnZXIKPiArICoKPiArICogUmV0dXJuIHRydWUgaWYgdGhlIHBy ZXZpb3VzIGRpc3BsYXkgbGlzdCBoYXMgY29tcGxldGVkIGF0IGZyYW1lIGVuZCwgb3IKPiBmYWxz ZSArICogaWYgaXQgaGFzIGJlZW4gZGVsYXllZCBieSBvbmUgZnJhbWUgYmVjYXVzZSB0aGUgZGlz cGxheSBsaXN0Cj4gY29tbWl0IHJhY2VkICsgKiB3aXRoIHRoZSBmcmFtZSBlbmQgaW50ZXJydXB0 LiBUaGUgZnVuY3Rpb24gYWx3YXlzIHJldHVybnMKPiB0cnVlIGluIGhlYWRlciBtb2RlICsgKiBh cyBkaXNwbGF5IGxpc3QgcHJvY2Vzc2luZyBpcyB0aGVuIG5vdCBjb250aW51b3VzCj4gYW5kIHJh Y2VzIG5ldmVyIG9jY3VyLiArICovCj4gK2Jvb2wgdnNwMV9kbG1faXJxX2ZyYW1lX2VuZChzdHJ1 Y3QgdnNwMV9kbF9tYW5hZ2VyICpkbG0pCj4gIHsKPiAgCXN0cnVjdCB2c3AxX2RldmljZSAqdnNw MSA9IGRsbS0+dnNwMTsKPiArCWJvb2wgY29tcGxldGVkID0gZmFsc2U7Cj4gCj4gIAlzcGluX2xv Y2soJmRsbS0+bG9jayk7Cj4gCj4gQEAgLTU3Niw4ICs1ODYsMTAgQEAgdm9pZCB2c3AxX2RsbV9p cnFfZnJhbWVfZW5kKHN0cnVjdCB2c3AxX2RsX21hbmFnZXIKPiAqZGxtKSAqIHBlcmZvcm0gYW55 IG9wZXJhdGlvbiBhcyB0aGVyZSBjYW4ndCBiZSBhbnkgbmV3IGRpc3BsYXkgbGlzdCBxdWV1ZWQK PiAqIGluIHRoYXQgY2FzZS4KPiAgCSAqLwo+IC0JaWYgKGRsbS0+bW9kZSA9PSBWU1AxX0RMX01P REVfSEVBREVSKQo+ICsJaWYgKGRsbS0+bW9kZSA9PSBWU1AxX0RMX01PREVfSEVBREVSKSB7Cj4g KwkJY29tcGxldGVkID0gdHJ1ZTsKPiAgCQlnb3RvIGRvbmU7Cj4gKwl9Cj4gCj4gIAkvKgo+ICAJ ICogVGhlIFVQRCBiaXQgc2V0IGluZGljYXRlcyB0aGF0IHRoZSBjb21taXQgb3BlcmF0aW9uIHJh Y2VkIHdpdGggdGhlCj4gQEAgLTU5Nyw2ICs2MDksNyBAQCB2b2lkIHZzcDFfZGxtX2lycV9mcmFt ZV9lbmQoc3RydWN0IHZzcDFfZGxfbWFuYWdlciAqZGxtKQo+IGlmIChkbG0tPnF1ZXVlZCkgewo+ ICAJCWRsbS0+YWN0aXZlID0gZGxtLT5xdWV1ZWQ7Cj4gIAkJZGxtLT5xdWV1ZWQgPSBOVUxMOwo+ ICsJCWNvbXBsZXRlZCA9IHRydWU7Cj4gIAl9Cj4gCj4gIAkvKgo+IEBAIC02MTksNiArNjMyLDgg QEAgdm9pZCB2c3AxX2RsbV9pcnFfZnJhbWVfZW5kKHN0cnVjdCB2c3AxX2RsX21hbmFnZXIgKmRs bSkKPiAKPiAgZG9uZToKPiAgCXNwaW5fdW5sb2NrKCZkbG0tPmxvY2spOwo+ICsKPiArCXJldHVy biBjb21wbGV0ZWQ7Cj4gIH0KPiAKPiAgLyogSGFyZHdhcmUgU2V0dXAgKi8KPiBkaWZmIC0tZ2l0 IGEvZHJpdmVycy9tZWRpYS9wbGF0Zm9ybS92c3AxL3ZzcDFfZGwuaAo+IGIvZHJpdmVycy9tZWRp YS9wbGF0Zm9ybS92c3AxL3ZzcDFfZGwuaCBpbmRleCA3MTMxYWEzYzU5NzguLjZlYzEzODBhMTBh Zgo+IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3AxX2RsLmgK PiArKysgYi9kcml2ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNwMV9kbC5oCj4gQEAgLTI4LDcg KzI4LDcgQEAgc3RydWN0IHZzcDFfZGxfbWFuYWdlciAqdnNwMV9kbG1fY3JlYXRlKHN0cnVjdCB2 c3AxX2RldmljZQo+ICp2c3AxLCB2b2lkIHZzcDFfZGxtX2Rlc3Ryb3koc3RydWN0IHZzcDFfZGxf bWFuYWdlciAqZGxtKTsKPiAgdm9pZCB2c3AxX2RsbV9yZXNldChzdHJ1Y3QgdnNwMV9kbF9tYW5h Z2VyICpkbG0pOwo+ICB2b2lkIHZzcDFfZGxtX2lycV9kaXNwbGF5X3N0YXJ0KHN0cnVjdCB2c3Ax X2RsX21hbmFnZXIgKmRsbSk7Cj4gLXZvaWQgdnNwMV9kbG1faXJxX2ZyYW1lX2VuZChzdHJ1Y3Qg dnNwMV9kbF9tYW5hZ2VyICpkbG0pOwo+ICtib29sIHZzcDFfZGxtX2lycV9mcmFtZV9lbmQoc3Ry dWN0IHZzcDFfZGxfbWFuYWdlciAqZGxtKTsKPiAKPiAgc3RydWN0IHZzcDFfZGxfbGlzdCAqdnNw MV9kbF9saXN0X2dldChzdHJ1Y3QgdnNwMV9kbF9tYW5hZ2VyICpkbG0pOwo+ICB2b2lkIHZzcDFf ZGxfbGlzdF9wdXQoc3RydWN0IHZzcDFfZGxfbGlzdCAqZGwpOwo+IGRpZmYgLS1naXQgYS9kcml2 ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNwMV9waXBlLmMKPiBiL2RyaXZlcnMvbWVkaWEvcGxh dGZvcm0vdnNwMS92c3AxX3BpcGUuYyBpbmRleCAzNTM2NGY1OTRlMTkuLmQxNTMyNzcwMWFkOAo+ IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3AxX3BpcGUuYwo+ ICsrKyBiL2RyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3AxX3BpcGUuYwo+IEBAIC0zMDQs MTAgKzMwNCwyMSBAQCBib29sIHZzcDFfcGlwZWxpbmVfcmVhZHkoc3RydWN0IHZzcDFfcGlwZWxp bmUgKnBpcGUpCj4gCj4gIHZvaWQgdnNwMV9waXBlbGluZV9mcmFtZV9lbmQoc3RydWN0IHZzcDFf cGlwZWxpbmUgKnBpcGUpCj4gIHsKPiArCWJvb2wgY29tcGxldGVkOwo+ICsKPiAgCWlmIChwaXBl ID09IE5VTEwpCj4gIAkJcmV0dXJuOwo+IAo+IC0JdnNwMV9kbG1faXJxX2ZyYW1lX2VuZChwaXBl LT5vdXRwdXQtPmRsbSk7Cj4gKwljb21wbGV0ZWQgPSB2c3AxX2RsbV9pcnFfZnJhbWVfZW5kKHBp cGUtPm91dHB1dC0+ZGxtKTsKPiArCWlmICghY29tcGxldGVkKSB7Cj4gKwkJLyoKPiArCQkgKiBJ ZiB0aGUgREwgY29tbWl0IHJhY2VkIHdpdGggdGhlIGZyYW1lIGVuZCBpbnRlcnJ1cHQsIHRoZQo+ ICsJCSAqIGNvbW1pdCBlbmRzIHVwIGJlaW5nIHBvc3Rwb25lZCBieSBvbmUgZnJhbWUuIFJldHVy bgo+ICsJCSAqIGltbWVkaWF0ZWx5IHdpdGhvdXQgY2FsbGluZyB0aGUgcGlwZWxpbmUncyBmcmFt ZSBlbmQgCmhhbmRsZXIKPiArCQkgKiBvciBpbmNyZW1lbnRpbmcgdGhlIHNlcXVlbmNlIG51bWJl ci4KPiArCQkgKi8KPiArCQlyZXR1cm47Cj4gKwl9Cj4gCj4gIAlpZiAocGlwZS0+ZnJhbWVfZW5k KQo+ICAJCXBpcGUtPmZyYW1lX2VuZChwaXBlKTsKCi0tIApSZWdhcmRzLAoKTGF1cmVudCBQaW5j aGFydAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJp LWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBz Oi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg==