From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from perceval.ideasonboard.com ([213.167.242.64]:54206 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751151AbeD1TPX (ORCPT ); Sat, 28 Apr 2018 15:15:23 -0400 From: Laurent Pinchart To: Kieran Bingham Cc: Laurent Pinchart , linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org Subject: Re: [PATCH v2 7/8] v4l: vsp1: Integrate DISCOM in display pipeline Date: Sat, 28 Apr 2018 22:15:38 +0300 Message-ID: <2726723.Sl6OlPdl7O@avalon> In-Reply-To: <759b7675-d0d5-4a31-0949-2affd4019f77@ideasonboard.com> References: <20180422223430.16407-1-laurent.pinchart+renesas@ideasonboard.com> <20180422223430.16407-8-laurent.pinchart+renesas@ideasonboard.com> <759b7675-d0d5-4a31-0949-2affd4019f77@ideasonboard.com> 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, On Saturday, 28 April 2018 21:58:53 EEST Kieran Bingham wrote: > On 22/04/18 23:34, Laurent Pinchart wrote: > > The DISCOM is used to compute CRCs on display frames. Integrate it in > > the display pipeline at the output of the blending unit to process > > output frames. > > > > Computing CRCs on input frames is possible by positioning the DISCOM at > > a different point in the pipeline. This use case isn't supported at the > > moment and could be implemented by extending the API between the VSP1 > > and DU drivers if needed. > > > > Signed-off-by: Laurent Pinchart > > > > Only a couple of small questions - but nothing to block an RB tag. > > Reviewed-by: Kieran Bingham > > > --- > > > > drivers/media/platform/vsp1/vsp1_drm.c | 115 +++++++++++++++++++++++++++- > > drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++ > > 2 files changed, 124 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c > > b/drivers/media/platform/vsp1/vsp1_drm.c index 5fc31578f9b0..7864b43a90e1 > > 100644 > > --- a/drivers/media/platform/vsp1/vsp1_drm.c > > +++ b/drivers/media/platform/vsp1/vsp1_drm.c [snip] > > @@ -748,6 +847,9 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned > > int pipe_index,> > > struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index]; > > struct vsp1_pipeline *pipe = &drm_pipe->pipe; > > > > + drm_pipe->crc.source = cfg->crc.source; > > + drm_pipe->crc.index = cfg->crc.index; > > I think this could be shortened to > > drm_pipe->crc = cfg->crc; > > Or is that a GCC extension. Either way, it's just a matter of taste, and you > might prefer to be more explicit. That's what I had written in the first place, only to have gcc throwing an error because the crc fields are anonymous structures. > > + > > vsp1_du_pipeline_setup_inputs(vsp1, pipe); > > vsp1_du_pipeline_configure(pipe); > > mutex_unlock(&vsp1->drm->lock); [snip] > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.h > > b/drivers/media/platform/vsp1/vsp1_drm.h index e5b88b28806c..1e7670955ef0 > > 100644 > > --- a/drivers/media/platform/vsp1/vsp1_drm.h > > +++ b/drivers/media/platform/vsp1/vsp1_drm.h > > @@ -13,6 +13,8 @@ > > #include > > #include > > > > +#include > > + > > #include "vsp1_pipe.h" > > > > /** > > @@ -22,6 +24,9 @@ > > * @height: output display height > > * @force_brx_release: when set, release the BRx during the next > > reconfiguration > > * @wait_queue: wait queue to wait for BRx release completion > > + * @uif: UIF entity if available for the pipeline > > + * @crc.source: source for CRC calculation > > + * @crc.index: index of the CRC source plane (when crc.source is set to > > plane) > > * @du_complete: frame completion callback for the DU driver (optional) > > * @du_private: data to be passed to the du_complete callback > > */ > > @@ -34,6 +39,13 @@ struct vsp1_drm_pipeline { > > bool force_brx_release; > > wait_queue_head_t wait_queue; > > > > + struct vsp1_entity *uif; > > + > > + struct { > > + enum vsp1_du_crc_source source; > > + unsigned int index; > > + } crc; > > Does this have to be duplicated ? Or can it be included from the API > header... It's a good point. I thought it might not be worth it just for two fields, but I see no compelling reason against it. I'll introduce a new structure. > > + > > /* Frame synchronisation */ > > void (*du_complete)(void *data, bool completed, u32 crc); > > void *du_private; -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v2 7/8] v4l: vsp1: Integrate DISCOM in display pipeline Date: Sat, 28 Apr 2018 22:15:38 +0300 Message-ID: <2726723.Sl6OlPdl7O@avalon> References: <20180422223430.16407-1-laurent.pinchart+renesas@ideasonboard.com> <20180422223430.16407-8-laurent.pinchart+renesas@ideasonboard.com> <759b7675-d0d5-4a31-0949-2affd4019f77@ideasonboard.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by gabe.freedesktop.org (Postfix) with ESMTPS id 682FB6E068 for ; Sat, 28 Apr 2018 19:15:23 +0000 (UTC) In-Reply-To: <759b7675-d0d5-4a31-0949-2affd4019f77@ideasonboard.com> 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, Laurent Pinchart , dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org SGkgS2llcmFuLAoKT24gU2F0dXJkYXksIDI4IEFwcmlsIDIwMTggMjE6NTg6NTMgRUVTVCBLaWVy YW4gQmluZ2hhbSB3cm90ZToKPiBPbiAyMi8wNC8xOCAyMzozNCwgTGF1cmVudCBQaW5jaGFydCB3 cm90ZToKPiA+IFRoZSBESVNDT00gaXMgdXNlZCB0byBjb21wdXRlIENSQ3Mgb24gZGlzcGxheSBm cmFtZXMuIEludGVncmF0ZSBpdCBpbgo+ID4gdGhlIGRpc3BsYXkgcGlwZWxpbmUgYXQgdGhlIG91 dHB1dCBvZiB0aGUgYmxlbmRpbmcgdW5pdCB0byBwcm9jZXNzCj4gPiBvdXRwdXQgZnJhbWVzLgo+ ID4gCj4gPiBDb21wdXRpbmcgQ1JDcyBvbiBpbnB1dCBmcmFtZXMgaXMgcG9zc2libGUgYnkgcG9z aXRpb25pbmcgdGhlIERJU0NPTSBhdAo+ID4gYSBkaWZmZXJlbnQgcG9pbnQgaW4gdGhlIHBpcGVs aW5lLiBUaGlzIHVzZSBjYXNlIGlzbid0IHN1cHBvcnRlZCBhdCB0aGUKPiA+IG1vbWVudCBhbmQg Y291bGQgYmUgaW1wbGVtZW50ZWQgYnkgZXh0ZW5kaW5nIHRoZSBBUEkgYmV0d2VlbiB0aGUgVlNQ MQo+ID4gYW5kIERVIGRyaXZlcnMgaWYgbmVlZGVkLgo+ID4gCj4gPiBTaWduZWQtb2ZmLWJ5OiBM YXVyZW50IFBpbmNoYXJ0Cj4gPiA8bGF1cmVudC5waW5jaGFydCtyZW5lc2FzQGlkZWFzb25ib2Fy ZC5jb20+Cj4gCj4gT25seSBhIGNvdXBsZSBvZiBzbWFsbCBxdWVzdGlvbnMgLSBidXQgbm90aGlu ZyB0byBibG9jayBhbiBSQiB0YWcuCj4gCj4gUmV2aWV3ZWQtYnk6IEtpZXJhbiBCaW5naGFtIDxr aWVyYW4uYmluZ2hhbStyZW5lc2FzQGlkZWFzb25ib2FyZC5jb20+Cj4gCj4gPiAtLS0KPiA+IAo+ ID4gIGRyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3AxX2RybS5jIHwgMTE1ICsrKysrKysr KysrKysrKysrKysrKysrKysrKy0KPiA+ICBkcml2ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNw MV9kcm0uaCB8ICAxMiArKysrCj4gPiAgMiBmaWxlcyBjaGFuZ2VkLCAxMjQgaW5zZXJ0aW9ucygr KSwgMyBkZWxldGlvbnMoLSkKPiA+IAo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvbWVkaWEvcGxh dGZvcm0vdnNwMS92c3AxX2RybS5jCj4gPiBiL2RyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92 c3AxX2RybS5jIGluZGV4IDVmYzMxNTc4ZjliMC4uNzg2NGI0M2E5MGUxCj4gPiAxMDA2NDQKPiA+ IC0tLSBhL2RyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3AxX2RybS5jCj4gPiArKysgYi9k cml2ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNwMV9kcm0uYwoKW3NuaXBdCgo+ID4gQEAgLTc0 OCw2ICs4NDcsOSBAQCB2b2lkIHZzcDFfZHVfYXRvbWljX2ZsdXNoKHN0cnVjdCBkZXZpY2UgKmRl diwgdW5zaWduZWQKPiA+IGludCBwaXBlX2luZGV4LD4gCj4gPiAgCXN0cnVjdCB2c3AxX2RybV9w aXBlbGluZSAqZHJtX3BpcGUgPSAmdnNwMS0+ZHJtLT5waXBlW3BpcGVfaW5kZXhdOwo+ID4gIAlz dHJ1Y3QgdnNwMV9waXBlbGluZSAqcGlwZSA9ICZkcm1fcGlwZS0+cGlwZTsKPiA+IAo+ID4gKwlk cm1fcGlwZS0+Y3JjLnNvdXJjZSA9IGNmZy0+Y3JjLnNvdXJjZTsKPiA+ICsJZHJtX3BpcGUtPmNy Yy5pbmRleCA9IGNmZy0+Y3JjLmluZGV4Owo+IAo+IEkgdGhpbmsgdGhpcyBjb3VsZCBiZSBzaG9y dGVuZWQgdG8KPiAKPiAJZHJtX3BpcGUtPmNyYyA9IGNmZy0+Y3JjOwo+IAo+IE9yIGlzIHRoYXQg YSBHQ0MgZXh0ZW5zaW9uLiBFaXRoZXIgd2F5LCBpdCdzIGp1c3QgYSBtYXR0ZXIgb2YgdGFzdGUs IGFuZCB5b3UKPiBtaWdodCBwcmVmZXIgdG8gYmUgbW9yZSBleHBsaWNpdC4KClRoYXQncyB3aGF0 IEkgaGFkIHdyaXR0ZW4gaW4gdGhlIGZpcnN0IHBsYWNlLCBvbmx5IHRvIGhhdmUgZ2NjIHRocm93 aW5nIGFuIAplcnJvciBiZWNhdXNlIHRoZSBjcmMgZmllbGRzIGFyZSBhbm9ueW1vdXMgc3RydWN0 dXJlcy4KCj4gPiArCj4gPiAgCXZzcDFfZHVfcGlwZWxpbmVfc2V0dXBfaW5wdXRzKHZzcDEsIHBp cGUpOwo+ID4gIAl2c3AxX2R1X3BpcGVsaW5lX2NvbmZpZ3VyZShwaXBlKTsKPiA+ICAJbXV0ZXhf dW5sb2NrKCZ2c3AxLT5kcm0tPmxvY2spOwoKW3NuaXBdCgo+ID4gZGlmZiAtLWdpdCBhL2RyaXZl cnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3AxX2RybS5oCj4gPiBiL2RyaXZlcnMvbWVkaWEvcGxh dGZvcm0vdnNwMS92c3AxX2RybS5oIGluZGV4IGU1Yjg4YjI4ODA2Yy4uMWU3NjcwOTU1ZWYwCj4g PiAxMDA2NDQKPiA+IC0tLSBhL2RyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3AxX2RybS5o Cj4gPiArKysgYi9kcml2ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNwMV9kcm0uaAo+ID4gQEAg LTEzLDYgKzEzLDggQEAKPiA+ICAjaW5jbHVkZSA8bGludXgvdmlkZW9kZXYyLmg+Cj4gPiAgI2lu Y2x1ZGUgPGxpbnV4L3dhaXQuaD4KPiA+IAo+ID4gKyNpbmNsdWRlIDxtZWRpYS92c3AxLmg+Cj4g PiArCj4gPiAgI2luY2x1ZGUgInZzcDFfcGlwZS5oIgo+ID4gIAo+ID4gIC8qKgo+ID4gQEAgLTIy LDYgKzI0LDkgQEAKPiA+ICAgKiBAaGVpZ2h0OiBvdXRwdXQgZGlzcGxheSBoZWlnaHQKPiA+ICAg KiBAZm9yY2VfYnJ4X3JlbGVhc2U6IHdoZW4gc2V0LCByZWxlYXNlIHRoZSBCUnggZHVyaW5nIHRo ZSBuZXh0Cj4gPiAgIHJlY29uZmlndXJhdGlvbgo+ID4gICAqIEB3YWl0X3F1ZXVlOiB3YWl0IHF1 ZXVlIHRvIHdhaXQgZm9yIEJSeCByZWxlYXNlIGNvbXBsZXRpb24KPiA+ICsgKiBAdWlmOiBVSUYg ZW50aXR5IGlmIGF2YWlsYWJsZSBmb3IgdGhlIHBpcGVsaW5lCj4gPiArICogQGNyYy5zb3VyY2U6 IHNvdXJjZSBmb3IgQ1JDIGNhbGN1bGF0aW9uCj4gPiArICogQGNyYy5pbmRleDogaW5kZXggb2Yg dGhlIENSQyBzb3VyY2UgcGxhbmUgKHdoZW4gY3JjLnNvdXJjZSBpcyBzZXQgdG8KPiA+IHBsYW5l KQo+ID4gICAqIEBkdV9jb21wbGV0ZTogZnJhbWUgY29tcGxldGlvbiBjYWxsYmFjayBmb3IgdGhl IERVIGRyaXZlciAob3B0aW9uYWwpCj4gPiAgICogQGR1X3ByaXZhdGU6IGRhdGEgdG8gYmUgcGFz c2VkIHRvIHRoZSBkdV9jb21wbGV0ZSBjYWxsYmFjawo+ID4gICAqLwo+ID4gQEAgLTM0LDYgKzM5 LDEzIEBAIHN0cnVjdCB2c3AxX2RybV9waXBlbGluZSB7Cj4gPiAgCWJvb2wgZm9yY2VfYnJ4X3Jl bGVhc2U7Cj4gPiAgCXdhaXRfcXVldWVfaGVhZF90IHdhaXRfcXVldWU7Cj4gPiAKPiA+ICsJc3Ry dWN0IHZzcDFfZW50aXR5ICp1aWY7Cj4gPiArCj4gPiArCXN0cnVjdCB7Cj4gPiArCQllbnVtIHZz cDFfZHVfY3JjX3NvdXJjZSBzb3VyY2U7Cj4gPiArCQl1bnNpZ25lZCBpbnQgaW5kZXg7Cj4gPiAr CX0gY3JjOwo+IAo+IERvZXMgdGhpcyBoYXZlIHRvIGJlIGR1cGxpY2F0ZWQgPyBPciBjYW4gaXQg YmUgaW5jbHVkZWQgZnJvbSB0aGUgQVBJCj4gaGVhZGVyLi4uCgpJdCdzIGEgZ29vZCBwb2ludC4g SSB0aG91Z2h0IGl0IG1pZ2h0IG5vdCBiZSB3b3J0aCBpdCBqdXN0IGZvciB0d28gZmllbGRzLCBi dXQgCkkgc2VlIG5vIGNvbXBlbGxpbmcgcmVhc29uIGFnYWluc3QgaXQuIEknbGwgaW50cm9kdWNl IGEgbmV3IHN0cnVjdHVyZS4KCj4gPiArCj4gPiAgCS8qIEZyYW1lIHN5bmNocm9uaXNhdGlvbiAq Lwo+ID4gIAl2b2lkICgqZHVfY29tcGxldGUpKHZvaWQgKmRhdGEsIGJvb2wgY29tcGxldGVkLCB1 MzIgY3JjKTsKPiA+ICAJdm9pZCAqZHVfcHJpdmF0ZTsKCi0tIApSZWdhcmRzLAoKTGF1cmVudCBQ aW5jaGFydAoKCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f XwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcK aHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK