From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 30 Jul 2018 09:29:06 -0300 From: Mauro Carvalho Chehab To: Kieran Bingham Cc: Laurent Pinchart , linux-media@vger.kernel.org, Mauro Carvalho Chehab , dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org Subject: Re: [PATCH] v4l: vsp1: Fix deadlock in VSPDL DRM pipelines Message-ID: <20180730092906.4da35890@coco.lan> In-Reply-To: <3163968a-e24a-b8ad-5b3c-0a94aef12755@ideasonboard.com> References: <20180727171945.25603-1-laurent.pinchart+renesas@ideasonboard.com> <3163968a-e24a-b8ad-5b3c-0a94aef12755@ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Em Sat, 28 Jul 2018 20:13:05 +0100 Kieran Bingham escreveu: > Hi Laurent, Mauro, > > I've cast my eyes through this, and the driver code it affects > > On 27/07/18 18:19, Laurent Pinchart wrote: > > The VSP uses a lock to protect the BRU and BRS assignment when > > configuring pipelines. The lock is taken in vsp1_du_atomic_begin() and > > released in vsp1_du_atomic_flush(), as well as taken and released in > > vsp1_du_setup_lif(). This guards against multiple pipelines trying to > > assign the same BRU and BRS at the same time. > > > > The DRM framework calls the .atomic_begin() operations in a loop over > > all CRTCs included in an atomic commit. On a VSPDL (the only VSP type > > where this matters), a single VSP instance handles two CRTCs, with a > > single lock. This results in a deadlock when the .atomic_begin() > > operation is called on the second CRTC. > > > > The DRM framework serializes atomic commits that affect the same CRTCs, > > but doesn't know about two CRTCs sharing the same VSPDL. Two commits > > affecting the VSPDL LIF0 and LIF1 respectively can thus race each other, > > hence the need for a lock. > > > > This could be fixed on the DRM side by forcing serialization of commits > > affecting CRTCs backed by the same VSPDL, but that would negatively > > affect performances, as the locking is only needed when the BRU and BRS > > need to be reassigned, which is an uncommon case. > > > > The lock protects the whole .atomic_begin() to .atomic_flush() sequence. > > The only operation that can occur in-between is vsp1_du_atomic_update(), > > which doesn't touch the BRU and BRS, and thus doesn't need to be > > protected by the lock. We can thus only take the lock around the > > And I almost replied to say ... but what about vsp1_du_atomic_update() > before re-reading this paragraph :) > > > > pipeline setup calls in vsp1_du_atomic_flush(), which fixes the > > deadlock. > > > > Fixes: f81f9adc4ee1 ("media: v4l: vsp1: Assign BRU and BRS to pipelines dynamically") > > Signed-off-by: Laurent Pinchart > > It makes me very happy to see the lock/unlock across separate functions > removed :) > > Reviewed-by: Kieran Bingham > > > > --- > > drivers/media/platform/vsp1/vsp1_drm.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > I've successfully tested the patch with kmstest --flip running with four > > outputs on a Salvator-XS board, as well as with the DU kms-test-brxalloc.py > > test. The deadlock is gone, and no race has been observed. > > > > Mauro, this is a v4.18 regression fix. I'm sorry for sending it so late, > > I haven't noticed the issue earlier. Once Kieran reviews it (which should > > happen in the next few days), could you send it to Linus ? The breakage is > > pretty bad otherwise for people using both the VGA and LVDS outputs at the > > same time. > > > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c > > index edb35a5c57ea..a99fc0ced7a7 100644 > > --- a/drivers/media/platform/vsp1/vsp1_drm.c > > +++ b/drivers/media/platform/vsp1/vsp1_drm.c > > @@ -728,9 +728,6 @@ EXPORT_SYMBOL_GPL(vsp1_du_setup_lif); > > */ > > void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index) > > { > > - struct vsp1_device *vsp1 = dev_get_drvdata(dev); > > - > > - mutex_lock(&vsp1->drm->lock); > > } As this is a regression fix, I'll apply it. However, this function now does nothing. Please remove it on a next Kernel version, if it is not needed anymore (or add whatever it is needed to replace the lock). Regards, Mauro Thanks, Mauro From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mauro Carvalho Chehab Subject: Re: [PATCH] v4l: vsp1: Fix deadlock in VSPDL DRM pipelines Date: Mon, 30 Jul 2018 09:29:06 -0300 Message-ID: <20180730092906.4da35890@coco.lan> References: <20180727171945.25603-1-laurent.pinchart+renesas@ideasonboard.com> <3163968a-e24a-b8ad-5b3c-0a94aef12755@ideasonboard.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from bombadil.infradead.org (bombadil.infradead.org [IPv6:2607:7c80:54:e::133]) by gabe.freedesktop.org (Postfix) with ESMTPS id 880136E131 for ; Mon, 30 Jul 2018 12:29:12 +0000 (UTC) In-Reply-To: <3163968a-e24a-b8ad-5b3c-0a94aef12755@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, Mauro Carvalho Chehab , Laurent Pinchart , dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org RW0gU2F0LCAyOCBKdWwgMjAxOCAyMDoxMzowNSArMDEwMApLaWVyYW4gQmluZ2hhbSA8a2llcmFu LmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT4gZXNjcmV2ZXU6Cgo+IEhpIExhdXJlbnQsIE1hdXJv LAo+IAo+IEkndmUgY2FzdCBteSBleWVzIHRocm91Z2ggdGhpcywgYW5kIHRoZSBkcml2ZXIgY29k ZSBpdCBhZmZlY3RzCj4gCj4gT24gMjcvMDcvMTggMTg6MTksIExhdXJlbnQgUGluY2hhcnQgd3Jv dGU6Cj4gPiBUaGUgVlNQIHVzZXMgYSBsb2NrIHRvIHByb3RlY3QgdGhlIEJSVSBhbmQgQlJTIGFz c2lnbm1lbnQgd2hlbgo+ID4gY29uZmlndXJpbmcgcGlwZWxpbmVzLiBUaGUgbG9jayBpcyB0YWtl biBpbiB2c3AxX2R1X2F0b21pY19iZWdpbigpIGFuZAo+ID4gcmVsZWFzZWQgaW4gdnNwMV9kdV9h dG9taWNfZmx1c2goKSwgYXMgd2VsbCBhcyB0YWtlbiBhbmQgcmVsZWFzZWQgaW4KPiA+IHZzcDFf ZHVfc2V0dXBfbGlmKCkuIFRoaXMgZ3VhcmRzIGFnYWluc3QgbXVsdGlwbGUgcGlwZWxpbmVzIHRy eWluZyB0bwo+ID4gYXNzaWduIHRoZSBzYW1lIEJSVSBhbmQgQlJTIGF0IHRoZSBzYW1lIHRpbWUu Cj4gPiAKPiA+IFRoZSBEUk0gZnJhbWV3b3JrIGNhbGxzIHRoZSAuYXRvbWljX2JlZ2luKCkgb3Bl cmF0aW9ucyBpbiBhIGxvb3Agb3Zlcgo+ID4gYWxsIENSVENzIGluY2x1ZGVkIGluIGFuIGF0b21p YyBjb21taXQuIE9uIGEgVlNQREwgKHRoZSBvbmx5IFZTUCB0eXBlCj4gPiB3aGVyZSB0aGlzIG1h dHRlcnMpLCBhIHNpbmdsZSBWU1AgaW5zdGFuY2UgaGFuZGxlcyB0d28gQ1JUQ3MsIHdpdGggYQo+ ID4gc2luZ2xlIGxvY2suIFRoaXMgcmVzdWx0cyBpbiBhIGRlYWRsb2NrIHdoZW4gdGhlIC5hdG9t aWNfYmVnaW4oKQo+ID4gb3BlcmF0aW9uIGlzIGNhbGxlZCBvbiB0aGUgc2Vjb25kIENSVEMuCj4g PiAKPiA+IFRoZSBEUk0gZnJhbWV3b3JrIHNlcmlhbGl6ZXMgYXRvbWljIGNvbW1pdHMgdGhhdCBh ZmZlY3QgdGhlIHNhbWUgQ1JUQ3MsCj4gPiBidXQgZG9lc24ndCBrbm93IGFib3V0IHR3byBDUlRD cyBzaGFyaW5nIHRoZSBzYW1lIFZTUERMLiBUd28gY29tbWl0cwo+ID4gYWZmZWN0aW5nIHRoZSBW U1BETCBMSUYwIGFuZCBMSUYxIHJlc3BlY3RpdmVseSBjYW4gdGh1cyByYWNlIGVhY2ggb3RoZXIs Cj4gPiBoZW5jZSB0aGUgbmVlZCBmb3IgYSBsb2NrLgo+ID4gCj4gPiBUaGlzIGNvdWxkIGJlIGZp eGVkIG9uIHRoZSBEUk0gc2lkZSBieSBmb3JjaW5nIHNlcmlhbGl6YXRpb24gb2YgY29tbWl0cwo+ ID4gYWZmZWN0aW5nIENSVENzIGJhY2tlZCBieSB0aGUgc2FtZSBWU1BETCwgYnV0IHRoYXQgd291 bGQgbmVnYXRpdmVseQo+ID4gYWZmZWN0IHBlcmZvcm1hbmNlcywgYXMgdGhlIGxvY2tpbmcgaXMg b25seSBuZWVkZWQgd2hlbiB0aGUgQlJVIGFuZCBCUlMKPiA+IG5lZWQgdG8gYmUgcmVhc3NpZ25l ZCwgd2hpY2ggaXMgYW4gdW5jb21tb24gY2FzZS4KPiA+IAo+ID4gVGhlIGxvY2sgcHJvdGVjdHMg dGhlIHdob2xlIC5hdG9taWNfYmVnaW4oKSB0byAuYXRvbWljX2ZsdXNoKCkgc2VxdWVuY2UuCj4g PiBUaGUgb25seSBvcGVyYXRpb24gdGhhdCBjYW4gb2NjdXIgaW4tYmV0d2VlbiBpcyB2c3AxX2R1 X2F0b21pY191cGRhdGUoKSwKPiA+IHdoaWNoIGRvZXNuJ3QgdG91Y2ggdGhlIEJSVSBhbmQgQlJT LCBhbmQgdGh1cyBkb2Vzbid0IG5lZWQgdG8gYmUKPiA+IHByb3RlY3RlZCBieSB0aGUgbG9jay4g V2UgY2FuIHRodXMgb25seSB0YWtlIHRoZSBsb2NrIGFyb3VuZCB0aGUgIAo+IAo+IEFuZCBJIGFs bW9zdCByZXBsaWVkIHRvIHNheSAuLi4gYnV0IHdoYXQgYWJvdXQgdnNwMV9kdV9hdG9taWNfdXBk YXRlKCkKPiBiZWZvcmUgcmUtcmVhZGluZyB0aGlzIHBhcmFncmFwaCA6KQo+IAo+IAo+ID4gcGlw ZWxpbmUgc2V0dXAgY2FsbHMgaW4gdnNwMV9kdV9hdG9taWNfZmx1c2goKSwgd2hpY2ggZml4ZXMg dGhlCj4gPiBkZWFkbG9jay4KPiA+IAo+ID4gRml4ZXM6IGY4MWY5YWRjNGVlMSAoIm1lZGlhOiB2 NGw6IHZzcDE6IEFzc2lnbiBCUlUgYW5kIEJSUyB0byBwaXBlbGluZXMgZHluYW1pY2FsbHkiKQo+ ID4gU2lnbmVkLW9mZi1ieTogTGF1cmVudCBQaW5jaGFydCA8bGF1cmVudC5waW5jaGFydCtyZW5l c2FzQGlkZWFzb25ib2FyZC5jb20+ICAKPiAKPiBJdCBtYWtlcyBtZSB2ZXJ5IGhhcHB5IHRvIHNl ZSB0aGUgbG9jay91bmxvY2sgYWNyb3NzIHNlcGFyYXRlIGZ1bmN0aW9ucwo+IHJlbW92ZWQgOikK PiAKPiBSZXZpZXdlZC1ieTogS2llcmFuIEJpbmdoYW0gPGtpZXJhbi5iaW5naGFtK3JlbmVzYXNA aWRlYXNvbmJvYXJkLmNvbT4KPiAKPiAKPiA+IC0tLQo+ID4gIGRyaXZlcnMvbWVkaWEvcGxhdGZv cm0vdnNwMS92c3AxX2RybS5jIHwgNCArLS0tCj4gPiAgMSBmaWxlIGNoYW5nZWQsIDEgaW5zZXJ0 aW9uKCspLCAzIGRlbGV0aW9ucygtKQo+ID4gCj4gPiBJJ3ZlIHN1Y2Nlc3NmdWxseSB0ZXN0ZWQg dGhlIHBhdGNoIHdpdGgga21zdGVzdCAtLWZsaXAgcnVubmluZyB3aXRoIGZvdXIKPiA+IG91dHB1 dHMgb24gYSBTYWx2YXRvci1YUyBib2FyZCwgYXMgd2VsbCBhcyB3aXRoIHRoZSBEVSBrbXMtdGVz dC1icnhhbGxvYy5weQo+ID4gdGVzdC4gVGhlIGRlYWRsb2NrIGlzIGdvbmUsIGFuZCBubyByYWNl IGhhcyBiZWVuIG9ic2VydmVkLgo+ID4gCj4gPiBNYXVybywgdGhpcyBpcyBhIHY0LjE4IHJlZ3Jl c3Npb24gZml4LiBJJ20gc29ycnkgZm9yIHNlbmRpbmcgaXQgc28gbGF0ZSwKPiA+IEkgaGF2ZW4n dCBub3RpY2VkIHRoZSBpc3N1ZSBlYXJsaWVyLiBPbmNlIEtpZXJhbiByZXZpZXdzIGl0ICh3aGlj aCBzaG91bGQKPiA+IGhhcHBlbiBpbiB0aGUgbmV4dCBmZXcgZGF5cyksIGNvdWxkIHlvdSBzZW5k IGl0IHRvIExpbnVzID8gVGhlIGJyZWFrYWdlIGlzCj4gPiBwcmV0dHkgYmFkIG90aGVyd2lzZSBm b3IgcGVvcGxlIHVzaW5nIGJvdGggdGhlIFZHQSBhbmQgTFZEUyBvdXRwdXRzIGF0IHRoZQo+ID4g c2FtZSB0aW1lLgo+ID4gCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9tZWRpYS9wbGF0Zm9ybS92 c3AxL3ZzcDFfZHJtLmMgYi9kcml2ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNwMV9kcm0uYwo+ ID4gaW5kZXggZWRiMzVhNWM1N2VhLi5hOTlmYzBjZWQ3YTcgMTAwNjQ0Cj4gPiAtLS0gYS9kcml2 ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNwMV9kcm0uYwo+ID4gKysrIGIvZHJpdmVycy9tZWRp YS9wbGF0Zm9ybS92c3AxL3ZzcDFfZHJtLmMKPiA+IEBAIC03MjgsOSArNzI4LDYgQEAgRVhQT1JU X1NZTUJPTF9HUEwodnNwMV9kdV9zZXR1cF9saWYpOwo+ID4gICAqLwo+ID4gIHZvaWQgdnNwMV9k dV9hdG9taWNfYmVnaW4oc3RydWN0IGRldmljZSAqZGV2LCB1bnNpZ25lZCBpbnQgcGlwZV9pbmRl eCkKPiA+ICB7Cj4gPiAtCXN0cnVjdCB2c3AxX2RldmljZSAqdnNwMSA9IGRldl9nZXRfZHJ2ZGF0 YShkZXYpOwo+ID4gLQo+ID4gLQltdXRleF9sb2NrKCZ2c3AxLT5kcm0tPmxvY2spOwo+ID4gIH0K CkFzIHRoaXMgaXMgYSByZWdyZXNzaW9uIGZpeCwgSSdsbCBhcHBseSBpdC4KCkhvd2V2ZXIsIHRo aXMgZnVuY3Rpb24gbm93IGRvZXMgbm90aGluZy4gUGxlYXNlIHJlbW92ZSBpdCBvbiBhIG5leHQK S2VybmVsIHZlcnNpb24sIGlmIGl0IGlzIG5vdCBuZWVkZWQgYW55bW9yZSAob3IgYWRkIHdoYXRl dmVyIGl0IGlzCm5lZWRlZCB0byByZXBsYWNlIHRoZSBsb2NrKS4KClJlZ2FyZHMsCk1hdXJvCgpU aGFua3MsCk1hdXJvCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9y ZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZl bAo=