From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 12/15] v4l: vsp1: Generalize detection of entity removal from DRM pipeline Date: Mon, 02 Apr 2018 15:57:13 +0300 Message-ID: <2504001.NRtzAvQP2M@avalon> In-Reply-To: <4841fc82-0201-2a53-53ce-6da7c144a75e@ideasonboard.com> References: <20180226214516.11559-1-laurent.pinchart+renesas@ideasonboard.com> <20180226214516.11559-13-laurent.pinchart+renesas@ideasonboard.com> <4841fc82-0201-2a53-53ce-6da7c144a75e@ideasonboard.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: Hi Kieran, On Thursday, 29 March 2018 14:37:07 EEST Kieran Bingham wrote: > On 26/02/18 21:45, Laurent Pinchart wrote: > > When disabling a DRM plane, the corresponding RPF is only marked as > > removed from the pipeline in the atomic update handler, with the actual > > removal happening when configuring the pipeline at atomic commit time. > > This is required as the RPF has to be disabled in the hardware, which > > can't be done from the atomic update handler. > > > > The current implementation is RPF-specific. Make it independent of the > > entity type by using the entity's pipe pointer to mark removal from the > > pipeline. This will allow using the mechanism to remove BRU instances. > > Nice improvement ... > > > Signed-off-by: Laurent Pinchart > > > > --- > > > > drivers/media/platform/vsp1/vsp1_drm.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c > > b/drivers/media/platform/vsp1/vsp1_drm.c index d705a6e9fa1d..6c60b72b6f50 > > 100644 > > --- a/drivers/media/platform/vsp1/vsp1_drm.c > > +++ b/drivers/media/platform/vsp1/vsp1_drm.c > > @@ -346,13 +346,12 @@ static void vsp1_du_pipeline_configure(struct > > vsp1_pipeline *pipe) > > dl = vsp1_dl_list_get(pipe->output->dlm); > > > > list_for_each_entry_safe(entity, next, &pipe->entities, list_pipe) { > > - /* Disconnect unused RPFs from the pipeline. */ > > - if (entity->type == VSP1_ENTITY_RPF && > > - !pipe->inputs[entity->index]) { > > + /* Disconnect unused entities from the pipeline. */ > > + if (!entity->pipe) { > > vsp1_dl_list_write(dl, entity->route->reg, > > VI6_DPR_NODE_UNUSED); > > I don't think it's a problem, as we don't unset the entity->pipe for > arbitrary entities, but what happens if we set an HGO/HGT entity to NULL > (these currently have 0 as the route->reg. This would risk writing > VI6_DPR_NODE_UNUSED to the VI6_CMD(0) register? > > In fact any entity in the pipeline with a null pipe pointer could cause > this... so we'd best be sure that they are right. Otherwise this could > cause some crazy bugs manifesting with the hardware doing something > unexpected. First of all, this code handles the DRM pipeline only, which can't contain an HGO or HGT. If we ever have to add an HGO or HGT to the DRM pipeline (I don't see a use case for now, but who knows), this will sure need to be fixed, among other things because the VI6_DPR_HGO_SMPPT and VI6_DPR_HGT_SMPPT registers would need to be configured. This being said, the idea behind this code is that when an entity is added to a pipeline, it will be added to the pipeline's entities list, and its pipe field will be set to point to the pipeline object. Removing an entity from a pipeline thus requires the opposite actions, removing it from the entities list and setting the pipe field to NULL. Due to the architecture of the DRM pipeline handling code, there is a need to reconfigure routing when an entity is removed from a pipeline. We can't reconfigure routing at the point where we get notified that entities need to be removed from the pipeline. We thus need to track removed entities, and then reconfigure routing later. This patch implements such a mechanism by using the pipe pointer to track entity removal, and then removes them from the pipeline's list of entities later after reconfiguring routing. You are worried that any entity in the pipeline with a NULL pipe pointer could cause issues. The pipe pointer is set to a non-NULL value when an entity is added to the pipeline, so the only way for the pipe pointer to be NULL here is for an entity that was part of a pipeline to be removed from it. As the HGO and HGT entities are never added to the pipeline, that's not an issue. > Assuming that won't be a problem, I believe that assumption to be correct :-) > Reviewed-by: Kieran Bingham > > > - entity->pipe = NULL; > > + entity->sink = NULL; > > list_del(&entity->list_pipe); > > > > continue; > > @@ -569,10 +568,11 @@ int vsp1_du_atomic_update(struct device *dev, > > unsigned int pipe_index, > > rpf_index); > > > > /* > > - * Remove the RPF from the pipe's inputs. The atomic flush > > - * handler will disable the input and remove the entity from the > > - * pipe's entities list. > > + * Remove the RPF from the pipeline's inputs. Keep it in the > > + * pipeline's entity list to let vsp1_du_pipeline_configure() > > + * remove it from the hardware pipeline. > > */ > > + rpf->entity.pipe = NULL; > > drm_pipe->pipe.inputs[rpf_index] = NULL; > > return 0; > > } -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH 12/15] v4l: vsp1: Generalize detection of entity removal from DRM pipeline Date: Mon, 02 Apr 2018 15:57:13 +0300 Message-ID: <2504001.NRtzAvQP2M@avalon> References: <20180226214516.11559-1-laurent.pinchart+renesas@ideasonboard.com> <20180226214516.11559-13-laurent.pinchart+renesas@ideasonboard.com> <4841fc82-0201-2a53-53ce-6da7c144a75e@ideasonboard.com> 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 3CC546E148 for ; Mon, 2 Apr 2018 12:57:07 +0000 (UTC) In-Reply-To: <4841fc82-0201-2a53-53ce-6da7c144a75e@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 SGkgS2llcmFuLAoKT24gVGh1cnNkYXksIDI5IE1hcmNoIDIwMTggMTQ6Mzc6MDcgRUVTVCBLaWVy YW4gQmluZ2hhbSB3cm90ZToKPiBPbiAyNi8wMi8xOCAyMTo0NSwgTGF1cmVudCBQaW5jaGFydCB3 cm90ZToKPiA+IFdoZW4gZGlzYWJsaW5nIGEgRFJNIHBsYW5lLCB0aGUgY29ycmVzcG9uZGluZyBS UEYgaXMgb25seSBtYXJrZWQgYXMKPiA+IHJlbW92ZWQgZnJvbSB0aGUgcGlwZWxpbmUgaW4gdGhl IGF0b21pYyB1cGRhdGUgaGFuZGxlciwgd2l0aCB0aGUgYWN0dWFsCj4gPiByZW1vdmFsIGhhcHBl bmluZyB3aGVuIGNvbmZpZ3VyaW5nIHRoZSBwaXBlbGluZSBhdCBhdG9taWMgY29tbWl0IHRpbWUu Cj4gPiBUaGlzIGlzIHJlcXVpcmVkIGFzIHRoZSBSUEYgaGFzIHRvIGJlIGRpc2FibGVkIGluIHRo ZSBoYXJkd2FyZSwgd2hpY2gKPiA+IGNhbid0IGJlIGRvbmUgZnJvbSB0aGUgYXRvbWljIHVwZGF0 ZSBoYW5kbGVyLgo+ID4gCj4gPiBUaGUgY3VycmVudCBpbXBsZW1lbnRhdGlvbiBpcyBSUEYtc3Bl Y2lmaWMuIE1ha2UgaXQgaW5kZXBlbmRlbnQgb2YgdGhlCj4gPiBlbnRpdHkgdHlwZSBieSB1c2lu ZyB0aGUgZW50aXR5J3MgcGlwZSBwb2ludGVyIHRvIG1hcmsgcmVtb3ZhbCBmcm9tIHRoZQo+ID4g cGlwZWxpbmUuIFRoaXMgd2lsbCBhbGxvdyB1c2luZyB0aGUgbWVjaGFuaXNtIHRvIHJlbW92ZSBC UlUgaW5zdGFuY2VzLgo+IAo+IE5pY2UgaW1wcm92ZW1lbnQgLi4uCj4gCj4gPiBTaWduZWQtb2Zm LWJ5OiBMYXVyZW50IFBpbmNoYXJ0Cj4gPiA8bGF1cmVudC5waW5jaGFydCtyZW5lc2FzQGlkZWFz b25ib2FyZC5jb20+Cj4gPiAtLS0KPiA+IAo+ID4gIGRyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNw MS92c3AxX2RybS5jIHwgMTQgKysrKysrKy0tLS0tLS0KPiA+ICAxIGZpbGUgY2hhbmdlZCwgNyBp bnNlcnRpb25zKCspLCA3IGRlbGV0aW9ucygtKQo+ID4gCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVy cy9tZWRpYS9wbGF0Zm9ybS92c3AxL3ZzcDFfZHJtLmMKPiA+IGIvZHJpdmVycy9tZWRpYS9wbGF0 Zm9ybS92c3AxL3ZzcDFfZHJtLmMgaW5kZXggZDcwNWE2ZTlmYTFkLi42YzYwYjcyYjZmNTAKPiA+ IDEwMDY0NAo+ID4gLS0tIGEvZHJpdmVycy9tZWRpYS9wbGF0Zm9ybS92c3AxL3ZzcDFfZHJtLmMK PiA+ICsrKyBiL2RyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3AxX2RybS5jCj4gPiBAQCAt MzQ2LDEzICszNDYsMTIgQEAgc3RhdGljIHZvaWQgdnNwMV9kdV9waXBlbGluZV9jb25maWd1cmUo c3RydWN0Cj4gPiB2c3AxX3BpcGVsaW5lICpwaXBlKQo+ID4gIAlkbCA9IHZzcDFfZGxfbGlzdF9n ZXQocGlwZS0+b3V0cHV0LT5kbG0pOwo+ID4gIAkKPiA+ICAJbGlzdF9mb3JfZWFjaF9lbnRyeV9z YWZlKGVudGl0eSwgbmV4dCwgJnBpcGUtPmVudGl0aWVzLCBsaXN0X3BpcGUpIHsKPiA+IC0JCS8q IERpc2Nvbm5lY3QgdW51c2VkIFJQRnMgZnJvbSB0aGUgcGlwZWxpbmUuICovCj4gPiAtCQlpZiAo ZW50aXR5LT50eXBlID09IFZTUDFfRU5USVRZX1JQRiAmJgo+ID4gLQkJICAgICFwaXBlLT5pbnB1 dHNbZW50aXR5LT5pbmRleF0pIHsKPiA+ICsJCS8qIERpc2Nvbm5lY3QgdW51c2VkIGVudGl0aWVz IGZyb20gdGhlIHBpcGVsaW5lLiAqLwo+ID4gKwkJaWYgKCFlbnRpdHktPnBpcGUpIHsKPiA+ICAJ CQl2c3AxX2RsX2xpc3Rfd3JpdGUoZGwsIGVudGl0eS0+cm91dGUtPnJlZywKPiA+ICAJCQkJCSAg IFZJNl9EUFJfTk9ERV9VTlVTRUQpOwo+IAo+IEkgZG9uJ3QgdGhpbmsgaXQncyBhIHByb2JsZW0s IGFzIHdlIGRvbid0IHVuc2V0IHRoZSBlbnRpdHktPnBpcGUgZm9yCj4gYXJiaXRyYXJ5IGVudGl0 aWVzLCBidXQgd2hhdCBoYXBwZW5zIGlmIHdlIHNldCBhbiBIR08vSEdUIGVudGl0eSB0byBOVUxM Cj4gKHRoZXNlIGN1cnJlbnRseSBoYXZlIDAgYXMgdGhlIHJvdXRlLT5yZWcuIFRoaXMgd291bGQg cmlzayB3cml0aW5nCj4gVkk2X0RQUl9OT0RFX1VOVVNFRCB0byB0aGUgVkk2X0NNRCgwKSByZWdp c3Rlcj8KPiAKPiBJbiBmYWN0IGFueSBlbnRpdHkgaW4gdGhlIHBpcGVsaW5lIHdpdGggYSBudWxs IHBpcGUgcG9pbnRlciBjb3VsZCBjYXVzZQo+IHRoaXMuLi4gc28gd2UnZCBiZXN0IGJlIHN1cmUg dGhhdCB0aGV5IGFyZSByaWdodC4gT3RoZXJ3aXNlIHRoaXMgY291bGQKPiBjYXVzZSBzb21lIGNy YXp5IGJ1Z3MgbWFuaWZlc3Rpbmcgd2l0aCB0aGUgaGFyZHdhcmUgZG9pbmcgc29tZXRoaW5nCj4g dW5leHBlY3RlZC4KCkZpcnN0IG9mIGFsbCwgdGhpcyBjb2RlIGhhbmRsZXMgdGhlIERSTSBwaXBl bGluZSBvbmx5LCB3aGljaCBjYW4ndCBjb250YWluIGFuIApIR08gb3IgSEdULiBJZiB3ZSBldmVy IGhhdmUgdG8gYWRkIGFuIEhHTyBvciBIR1QgdG8gdGhlIERSTSBwaXBlbGluZSAoSSBkb24ndCAK c2VlIGEgdXNlIGNhc2UgZm9yIG5vdywgYnV0IHdobyBrbm93cyksIHRoaXMgd2lsbCBzdXJlIG5l ZWQgdG8gYmUgZml4ZWQsIGFtb25nIApvdGhlciB0aGluZ3MgYmVjYXVzZSB0aGUgVkk2X0RQUl9I R09fU01QUFQgYW5kIFZJNl9EUFJfSEdUX1NNUFBUIHJlZ2lzdGVycyAKd291bGQgbmVlZCB0byBi ZSBjb25maWd1cmVkLgoKVGhpcyBiZWluZyBzYWlkLCB0aGUgaWRlYSBiZWhpbmQgdGhpcyBjb2Rl IGlzIHRoYXQgd2hlbiBhbiBlbnRpdHkgaXMgYWRkZWQgdG8gCmEgcGlwZWxpbmUsIGl0IHdpbGwg YmUgYWRkZWQgdG8gdGhlIHBpcGVsaW5lJ3MgZW50aXRpZXMgbGlzdCwgYW5kIGl0cyBwaXBlIApm aWVsZCB3aWxsIGJlIHNldCB0byBwb2ludCB0byB0aGUgcGlwZWxpbmUgb2JqZWN0LiBSZW1vdmlu ZyBhbiBlbnRpdHkgZnJvbSBhIApwaXBlbGluZSB0aHVzIHJlcXVpcmVzIHRoZSBvcHBvc2l0ZSBh Y3Rpb25zLCByZW1vdmluZyBpdCBmcm9tIHRoZSBlbnRpdGllcyAKbGlzdCBhbmQgc2V0dGluZyB0 aGUgcGlwZSBmaWVsZCB0byBOVUxMLgoKRHVlIHRvIHRoZSBhcmNoaXRlY3R1cmUgb2YgdGhlIERS TSBwaXBlbGluZSBoYW5kbGluZyBjb2RlLCB0aGVyZSBpcyBhIG5lZWQgdG8gCnJlY29uZmlndXJl IHJvdXRpbmcgd2hlbiBhbiBlbnRpdHkgaXMgcmVtb3ZlZCBmcm9tIGEgcGlwZWxpbmUuIFdlIGNh bid0IApyZWNvbmZpZ3VyZSByb3V0aW5nIGF0IHRoZSBwb2ludCB3aGVyZSB3ZSBnZXQgbm90aWZp ZWQgdGhhdCBlbnRpdGllcyBuZWVkIHRvIApiZSByZW1vdmVkIGZyb20gdGhlIHBpcGVsaW5lLiBX ZSB0aHVzIG5lZWQgdG8gdHJhY2sgcmVtb3ZlZCBlbnRpdGllcywgYW5kIHRoZW4gCnJlY29uZmln dXJlIHJvdXRpbmcgbGF0ZXIuIFRoaXMgcGF0Y2ggaW1wbGVtZW50cyBzdWNoIGEgbWVjaGFuaXNt IGJ5IHVzaW5nIHRoZSAKcGlwZSBwb2ludGVyIHRvIHRyYWNrIGVudGl0eSByZW1vdmFsLCBhbmQg dGhlbiByZW1vdmVzIHRoZW0gZnJvbSB0aGUgCnBpcGVsaW5lJ3MgbGlzdCBvZiBlbnRpdGllcyBs YXRlciBhZnRlciByZWNvbmZpZ3VyaW5nIHJvdXRpbmcuCgpZb3UgYXJlIHdvcnJpZWQgdGhhdCBh bnkgZW50aXR5IGluIHRoZSBwaXBlbGluZSB3aXRoIGEgTlVMTCBwaXBlIHBvaW50ZXIgY291bGQg CmNhdXNlIGlzc3Vlcy4gVGhlIHBpcGUgcG9pbnRlciBpcyBzZXQgdG8gYSBub24tTlVMTCB2YWx1 ZSB3aGVuIGFuIGVudGl0eSBpcyAKYWRkZWQgdG8gdGhlIHBpcGVsaW5lLCBzbyB0aGUgb25seSB3 YXkgZm9yIHRoZSBwaXBlIHBvaW50ZXIgdG8gYmUgTlVMTCBoZXJlIGlzIApmb3IgYW4gZW50aXR5 IHRoYXQgd2FzIHBhcnQgb2YgYSBwaXBlbGluZSB0byBiZSByZW1vdmVkIGZyb20gaXQuIEFzIHRo ZSBIR08gCmFuZCBIR1QgZW50aXRpZXMgYXJlIG5ldmVyIGFkZGVkIHRvIHRoZSBwaXBlbGluZSwg dGhhdCdzIG5vdCBhbiBpc3N1ZS4KCj4gQXNzdW1pbmcgdGhhdCB3b24ndCBiZSBhIHByb2JsZW0s CgpJIGJlbGlldmUgdGhhdCBhc3N1bXB0aW9uIHRvIGJlIGNvcnJlY3QgOi0pCgo+IFJldmlld2Vk LWJ5OiBLaWVyYW4gQmluZ2hhbSA8a2llcmFuLmJpbmdoYW0rcmVuZXNhc0BpZGVhc29uYm9hcmQu Y29tPgo+IAo+ID4gLQkJCWVudGl0eS0+cGlwZSA9IE5VTEw7Cj4gPiArCQkJZW50aXR5LT5zaW5r ID0gTlVMTDsKPiA+ICAJCQlsaXN0X2RlbCgmZW50aXR5LT5saXN0X3BpcGUpOwo+ID4gIAkJCQo+ ID4gIAkJCWNvbnRpbnVlOwo+ID4gQEAgLTU2OSwxMCArNTY4LDExIEBAIGludCB2c3AxX2R1X2F0 b21pY191cGRhdGUoc3RydWN0IGRldmljZSAqZGV2LAo+ID4gdW5zaWduZWQgaW50IHBpcGVfaW5k ZXgsCj4gPiAgCQkJcnBmX2luZGV4KTsKPiA+ICAJCQo+ID4gIAkJLyoKPiA+IC0JCSAqIFJlbW92 ZSB0aGUgUlBGIGZyb20gdGhlIHBpcGUncyBpbnB1dHMuIFRoZSBhdG9taWMgZmx1c2gKPiA+IC0J CSAqIGhhbmRsZXIgd2lsbCBkaXNhYmxlIHRoZSBpbnB1dCBhbmQgcmVtb3ZlIHRoZSBlbnRpdHkg ZnJvbSB0aGUKPiA+IC0JCSAqIHBpcGUncyBlbnRpdGllcyBsaXN0Lgo+ID4gKwkJICogUmVtb3Zl IHRoZSBSUEYgZnJvbSB0aGUgcGlwZWxpbmUncyBpbnB1dHMuIEtlZXAgaXQgaW4gdGhlCj4gPiAr CQkgKiBwaXBlbGluZSdzIGVudGl0eSBsaXN0IHRvIGxldCB2c3AxX2R1X3BpcGVsaW5lX2NvbmZp Z3VyZSgpCj4gPiArCQkgKiByZW1vdmUgaXQgZnJvbSB0aGUgaGFyZHdhcmUgcGlwZWxpbmUuCj4g PiAgCQkgKi8KPiA+ICsJCXJwZi0+ZW50aXR5LnBpcGUgPSBOVUxMOwo+ID4gIAkJZHJtX3BpcGUt PnBpcGUuaW5wdXRzW3JwZl9pbmRleF0gPSBOVUxMOwo+ID4gIAkJcmV0dXJuIDA7Cj4gPiAgCX0K Ci0tIApSZWdhcmRzLAoKTGF1cmVudCBQaW5jaGFydAoKCgpfX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZl bEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFp bG1hbi9saXN0aW5mby9kcmktZGV2ZWwK