From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from galahad.ideasonboard.com ([185.26.127.97]:35709 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750948AbdISIqd (ORCPT ); Tue, 19 Sep 2017 04:46:33 -0400 From: Laurent Pinchart To: kieran.bingham@ideasonboard.com Cc: linux-renesas-soc@vger.kernel.org, linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v1 1/3] media: vsp1: Prevent resuming DRM pipelines Date: Tue, 19 Sep 2017 11:46:36 +0300 Message-ID: <2503926.ERnN7xSWg0@avalon> In-Reply-To: <50ab72f2-0d08-f0ff-e4f5-45cd32d74894@ideasonboard.com> References: <1514508.N9eClCS3vr@avalon> <50ab72f2-0d08-f0ff-e4f5-45cd32d74894@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 Monday, 18 September 2017 03:04:13 EEST Kieran Bingham wrote: > On 15/09/17 17:58, Laurent Pinchart wrote: > > On Friday, 15 September 2017 19:42:05 EEST Kieran Bingham wrote: > >> DRM pipelines utilising the VSP must stop all frame processing as part > >> of the suspend operation to ensure the hardware is idle. Upon resume, > >> the pipeline must not be started until the DU performs an atomic flush > >> to restore the hardware configuration and state. > >> > >> Therefore the vsp1_pipeline_resume() call is not needed for DRM > >> pipelines, and we can disable it in this instance. > > > > Being familiar with the issue I certainly understand the commit message, > > but I think it can be a bit confusing to a reader not familiar to the > > VSP/DU. How about something similar to the following ? > > > > "When used as part of a display pipeline, the VSP is stopped and restarted > > explicitly by the DU from its suspend and resume handlers. There is thus > > no need to stop or restart pipelines in the VSP suspend and resume > > handlers." > > That's fine with me. > > >> CC: linux-media@vger.kernel.org > >> > >> Signed-off-by: Kieran Bingham > >> --- > >> > >> drivers/media/platform/vsp1/vsp1_drv.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c > >> b/drivers/media/platform/vsp1/vsp1_drv.c index 962e4c304076..7604c7994c74 > >> 100644 > >> --- a/drivers/media/platform/vsp1/vsp1_drv.c > >> +++ b/drivers/media/platform/vsp1/vsp1_drv.c > >> @@ -582,7 +582,13 @@ static int __maybe_unused vsp1_pm_resume(struct > >> device > >> *dev) struct vsp1_device *vsp1 = dev_get_drvdata(dev); > >> > >> pm_runtime_force_resume(vsp1->dev); > >> > >> - vsp1_pipelines_resume(vsp1); > >> + > >> + /* > >> + * DRM pipelines are stopped before suspend, and will be resumed after > >> + * the DRM subsystem has reconfigured its pipeline with an atomic flush > >> + */ > > > > I would also adapt this comment similarly to the commit message. > > > >> + if (!vsp1->drm) > >> + vsp1_pipelines_resume(vsp1); > > > > Should we do the same in vsp1_pm_suspend() ? I know it shouldn't be > > strictly needed at the moment as vsp1_pipelines_suspend() should be a > > no-op when the pipelines are already stopped, but a symmetrical > > implementation sounds better to me. > > I'm OK with that, it's not needed - but it doesn't hurt to be symmetrical. > > (Updated locally for a v1.1 repost or such) I've taken patches 2/3 and 3/3 in my tree so feel free to report 1/3 only. > > I also wonder whether the check shouldn't be moved inside the > > vsp1_pipelines_suspend() and vsp1_pipelines_resume() functions as we will > > likely need to handle suspend/resume of display pipelines when adding > > writeback support, but we could do so later. > > I'll have to retest the writeback implementation - but I think that has got > quite stale now anyway and will need some rework. Agreed, I don't expect it to work out of the box. We can move the check later when rebasing the writeback patches. > >> return 0; > >> > >> } -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v1 1/3] media: vsp1: Prevent resuming DRM pipelines Date: Tue, 19 Sep 2017 11:46:36 +0300 Message-ID: <2503926.ERnN7xSWg0@avalon> References: <1514508.N9eClCS3vr@avalon> <50ab72f2-0d08-f0ff-e4f5-45cd32d74894@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 652AF89145 for ; Tue, 19 Sep 2017 08:46:33 +0000 (UTC) In-Reply-To: <50ab72f2-0d08-f0ff-e4f5-45cd32d74894@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@ideasonboard.com Cc: linux-renesas-soc@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org SGkgS2llcmFuLAoKT24gTW9uZGF5LCAxOCBTZXB0ZW1iZXIgMjAxNyAwMzowNDoxMyBFRVNUIEtp ZXJhbiBCaW5naGFtIHdyb3RlOgo+IE9uIDE1LzA5LzE3IDE3OjU4LCBMYXVyZW50IFBpbmNoYXJ0 IHdyb3RlOgo+ID4gT24gRnJpZGF5LCAxNSBTZXB0ZW1iZXIgMjAxNyAxOTo0MjowNSBFRVNUIEtp ZXJhbiBCaW5naGFtIHdyb3RlOgo+ID4+IERSTSBwaXBlbGluZXMgdXRpbGlzaW5nIHRoZSBWU1Ag bXVzdCBzdG9wIGFsbCBmcmFtZSBwcm9jZXNzaW5nIGFzIHBhcnQKPiA+PiBvZiB0aGUgc3VzcGVu ZCBvcGVyYXRpb24gdG8gZW5zdXJlIHRoZSBoYXJkd2FyZSBpcyBpZGxlLiBVcG9uIHJlc3VtZSwK PiA+PiB0aGUgcGlwZWxpbmUgbXVzdCBub3QgYmUgc3RhcnRlZCB1bnRpbCB0aGUgRFUgcGVyZm9y bXMgYW4gYXRvbWljIGZsdXNoCj4gPj4gdG8gcmVzdG9yZSB0aGUgaGFyZHdhcmUgY29uZmlndXJh dGlvbiBhbmQgc3RhdGUuCj4gPj4gCj4gPj4gVGhlcmVmb3JlIHRoZSB2c3AxX3BpcGVsaW5lX3Jl c3VtZSgpIGNhbGwgaXMgbm90IG5lZWRlZCBmb3IgRFJNCj4gPj4gcGlwZWxpbmVzLCBhbmQgd2Ug Y2FuIGRpc2FibGUgaXQgaW4gdGhpcyBpbnN0YW5jZS4KPiA+IAo+ID4gQmVpbmcgZmFtaWxpYXIg d2l0aCB0aGUgaXNzdWUgSSBjZXJ0YWlubHkgdW5kZXJzdGFuZCB0aGUgY29tbWl0IG1lc3NhZ2Us Cj4gPiBidXQgSSB0aGluayBpdCBjYW4gYmUgYSBiaXQgY29uZnVzaW5nIHRvIGEgcmVhZGVyIG5v dCBmYW1pbGlhciB0byB0aGUKPiA+IFZTUC9EVS4gSG93IGFib3V0IHNvbWV0aGluZyBzaW1pbGFy IHRvIHRoZSBmb2xsb3dpbmcgPwo+ID4gCj4gPiAiV2hlbiB1c2VkIGFzIHBhcnQgb2YgYSBkaXNw bGF5IHBpcGVsaW5lLCB0aGUgVlNQIGlzIHN0b3BwZWQgYW5kIHJlc3RhcnRlZAo+ID4gZXhwbGlj aXRseSBieSB0aGUgRFUgZnJvbSBpdHMgc3VzcGVuZCBhbmQgcmVzdW1lIGhhbmRsZXJzLiBUaGVy ZSBpcyB0aHVzCj4gPiBubyBuZWVkIHRvIHN0b3Agb3IgcmVzdGFydCBwaXBlbGluZXMgaW4gdGhl IFZTUCBzdXNwZW5kIGFuZCByZXN1bWUKPiA+IGhhbmRsZXJzLiIKPiAKPiBUaGF0J3MgZmluZSB3 aXRoIG1lLgo+IAo+ID4+IENDOiBsaW51eC1tZWRpYUB2Z2VyLmtlcm5lbC5vcmcKPiA+PiAKPiA+ PiBTaWduZWQtb2ZmLWJ5OiBLaWVyYW4gQmluZ2hhbSA8a2llcmFuLmJpbmdoYW0rcmVuZXNhc0Bp ZGVhc29uYm9hcmQuY29tPgo+ID4+IC0tLQo+ID4+IAo+ID4+ICBkcml2ZXJzL21lZGlhL3BsYXRm b3JtL3ZzcDEvdnNwMV9kcnYuYyB8IDggKysrKysrKy0KPiA+PiAgMSBmaWxlIGNoYW5nZWQsIDcg aW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQo+ID4+IAo+ID4+IGRpZmYgLS1naXQgYS9kcml2 ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNwMV9kcnYuYwo+ID4+IGIvZHJpdmVycy9tZWRpYS9w bGF0Zm9ybS92c3AxL3ZzcDFfZHJ2LmMgaW5kZXggOTYyZTRjMzA0MDc2Li43NjA0Yzc5OTRjNzQK PiA+PiAxMDA2NDQKPiA+PiAtLS0gYS9kcml2ZXJzL21lZGlhL3BsYXRmb3JtL3ZzcDEvdnNwMV9k cnYuYwo+ID4+ICsrKyBiL2RyaXZlcnMvbWVkaWEvcGxhdGZvcm0vdnNwMS92c3AxX2Rydi5jCj4g Pj4gQEAgLTU4Miw3ICs1ODIsMTMgQEAgc3RhdGljIGludCBfX21heWJlX3VudXNlZCB2c3AxX3Bt X3Jlc3VtZShzdHJ1Y3QKPiA+PiBkZXZpY2UKPiA+PiAqZGV2KSBzdHJ1Y3QgdnNwMV9kZXZpY2Ug KnZzcDEgPSBkZXZfZ2V0X2RydmRhdGEoZGV2KTsKPiA+PiAKPiA+PiAgCXBtX3J1bnRpbWVfZm9y Y2VfcmVzdW1lKHZzcDEtPmRldik7Cj4gPj4gCj4gPj4gLQl2c3AxX3BpcGVsaW5lc19yZXN1bWUo dnNwMSk7Cj4gPj4gKwo+ID4+ICsJLyoKPiA+PiArCSAqIERSTSBwaXBlbGluZXMgYXJlIHN0b3Bw ZWQgYmVmb3JlIHN1c3BlbmQsIGFuZCB3aWxsIGJlIHJlc3VtZWQgYWZ0ZXIKPiA+PiArCSAqIHRo ZSBEUk0gc3Vic3lzdGVtIGhhcyByZWNvbmZpZ3VyZWQgaXRzIHBpcGVsaW5lIHdpdGggYW4gYXRv bWljIGZsdXNoCj4gPj4gKwkgKi8KPiA+IAo+ID4gSSB3b3VsZCBhbHNvIGFkYXB0IHRoaXMgY29t bWVudCBzaW1pbGFybHkgdG8gdGhlIGNvbW1pdCBtZXNzYWdlLgo+ID4gCj4gPj4gKwlpZiAoIXZz cDEtPmRybSkKPiA+PiArCQl2c3AxX3BpcGVsaW5lc19yZXN1bWUodnNwMSk7Cj4gPiAKPiA+IFNo b3VsZCB3ZSBkbyB0aGUgc2FtZSBpbiB2c3AxX3BtX3N1c3BlbmQoKSA/IEkga25vdyBpdCBzaG91 bGRuJ3QgYmUKPiA+IHN0cmljdGx5IG5lZWRlZCBhdCB0aGUgbW9tZW50IGFzIHZzcDFfcGlwZWxp bmVzX3N1c3BlbmQoKSBzaG91bGQgYmUgYQo+ID4gbm8tb3Agd2hlbiB0aGUgcGlwZWxpbmVzIGFy ZSBhbHJlYWR5IHN0b3BwZWQsIGJ1dCBhIHN5bW1ldHJpY2FsCj4gPiBpbXBsZW1lbnRhdGlvbiBz b3VuZHMgYmV0dGVyIHRvIG1lLgo+IAo+IEknbSBPSyB3aXRoIHRoYXQsIGl0J3Mgbm90IG5lZWRl ZCAtIGJ1dCBpdCBkb2Vzbid0IGh1cnQgdG8gYmUgc3ltbWV0cmljYWwuCj4gCj4gKFVwZGF0ZWQg bG9jYWxseSBmb3IgYSB2MS4xIHJlcG9zdCBvciBzdWNoKQoKSSd2ZSB0YWtlbiBwYXRjaGVzIDIv MyBhbmQgMy8zIGluIG15IHRyZWUgc28gZmVlbCBmcmVlIHRvIHJlcG9ydCAxLzMgb25seS4KCj4g PiBJIGFsc28gd29uZGVyIHdoZXRoZXIgdGhlIGNoZWNrIHNob3VsZG4ndCBiZSBtb3ZlZCBpbnNp ZGUgdGhlCj4gPiB2c3AxX3BpcGVsaW5lc19zdXNwZW5kKCkgYW5kIHZzcDFfcGlwZWxpbmVzX3Jl c3VtZSgpIGZ1bmN0aW9ucyBhcyB3ZSB3aWxsCj4gPiBsaWtlbHkgbmVlZCB0byBoYW5kbGUgc3Vz cGVuZC9yZXN1bWUgb2YgZGlzcGxheSBwaXBlbGluZXMgd2hlbiBhZGRpbmcKPiA+IHdyaXRlYmFj ayBzdXBwb3J0LCBidXQgd2UgY291bGQgZG8gc28gbGF0ZXIuCj4gCj4gSSdsbCBoYXZlIHRvIHJl dGVzdCB0aGUgd3JpdGViYWNrIGltcGxlbWVudGF0aW9uIC0gYnV0IEkgdGhpbmsgdGhhdCBoYXMg Z290Cj4gcXVpdGUgc3RhbGUgbm93IGFueXdheSBhbmQgd2lsbCBuZWVkIHNvbWUgcmV3b3JrLgoK QWdyZWVkLCBJIGRvbid0IGV4cGVjdCBpdCB0byB3b3JrIG91dCBvZiB0aGUgYm94LiBXZSBjYW4g bW92ZSB0aGUgY2hlY2sgbGF0ZXIgCndoZW4gcmViYXNpbmcgdGhlIHdyaXRlYmFjayBwYXRjaGVz LgoKPiA+PiAgCXJldHVybiAwOwo+ID4+ICAKPiA+PiAgfQoKLS0gClJlZ2FyZHMsCgpMYXVyZW50 IFBpbmNoYXJ0CgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f XwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcK aHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK