From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users Date: Fri, 14 Jul 2017 02:43:12 +0300 Message-ID: <1823154.LT2zYSFJ4j@avalon> References: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: 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: Maxime Ripard Cc: linux-renesas-soc@vger.kernel.org, linux-samsung-soc@vger.kernel.org, dri-devel@lists.freedesktop.org, Seung-Woo Kim , Chen-Yu Tsai , linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, Kyungmin Park , Kukjin Kim , Krzysztof Kozlowski , Daniel Vetter , linux-arm-kernel@lists.infradead.org List-Id: linux-rockchip.vger.kernel.org SGkgTWF4aW1lLAoKVGhhbmsgeW91IGZvciB0aGUgcGF0Y2guCgpPbiBUaHVyc2RheSAxMyBKdWwg MjAxNyAxNjo0MToxMyBNYXhpbWUgUmlwYXJkIHdyb3RlOgo+IFRoZSBjdXJyZW50IGRybV9hdG9t aWNfaGVscGVyX2NvbW1pdF90YWlsIGhlbHBlciB3b3JrcyBvbmx5IGlmIHRoZSBDUlRDIGlzCj4g YWNjZXNzaWJsZSwgYW5kIGRvY3VtZW50cyBhbiBhbHRlcm5hdGl2ZSBpbXBsZW1lbnRhdGlvbiB0 aGF0IGlzIHN1cHBvc2VkIHRvCj4gYmUgdXNlZCBpZiB0aGF0IGhhcHBlbnMuCj4gCj4gVGhhdCBp bXBsZW1lbnRhdGlvbiBpcyB0aGVuIGR1cGxpY2F0ZWQgYnkgc29tZSBkcml2ZXJzLiBJbnN0ZWFk IG9mCj4gZG9jdW1lbnRpbmcgaXQsIGxldCdzIGltcGxlbWVudCBhbiBoZWxwZXIgdGhhdCBhbGwg dGhlIHJlbGV2YW50IHVzZXJzIGNhbgo+IHVzZSBkaXJlY3RseS4KPiAKPiBTaWduZWQtb2ZmLWJ5 OiBNYXhpbWUgUmlwYXJkIDxtYXhpbWUucmlwYXJkQGZyZWUtZWxlY3Ryb25zLmNvbT4KPiAtLS0K PiAgZHJpdmVycy9ncHUvZHJtL2RybV9hdG9taWNfaGVscGVyLmMgICAgICAgIHwgNDcgKysrKysr KysrKysrKysrLS0tLS0tLS0KPiAgZHJpdmVycy9ncHUvZHJtL2V4eW5vcy9leHlub3NfZHJtX2Zi LmMgICAgIHwgMjcgKy0tLS0tLS0tLS0tLS0KPiAgZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNh cl9kdV9rbXMuYyAgICAgIHwgMTggKy0tLS0tLS0tLQoKSSd2ZSBzdWJtaXR0ZWQgIltQQVRDSF0g ZHJtOiByY2FyLWR1OiBTZXR1cCBwbGFuZXMgYmVmb3JlIGVuYWJsaW5nIENSVEMgdG8gCmF2b2lk IGZsaWNrZXIiIHRoYXQgY2hhbmdlcyB0aGUgcmNhci1kdSBpbXBsZW1lbnRhdGlvbiB0byB0aGUg c3RhbmRhcmQgCmRpc2FibGUvdXBkYXRlIHBsYW5lcy9lbmFibGUgb3JkZXIsIHNvIEknZCBhcHBy ZWNpYXRlIGlmIHlvdSBjb3VsZCBkcm9wIHRoZSAKcmNhci1kdSBwYXJ0IG9mIHRoaXMgcGF0Y2gg dG8gYXZvaWQgY29uZmxpY3RzLgoKVGhpcyBiZWluZyBzYWlkLCB0aGUgcmVhc29uIHdoeSBJIHN3 aXRjaGVkIGJhY2sgZnJvbSB0aGUgInJ1bnRpbWUgUE0iIHRvIHRoZSAKInN0YW5kYXJkIiBvcmRl ciBpcyBwcm9iYWJseSBvZiBpbnRlcmVzdCB0byB5b3UuIFF1b3RpbmcgdGhlIGNvbW1pdCBtZXNz YWdlLAoKPiBDb21taXQgNTIwNTViYWZhMWZmICgiZHJtOiByY2FyLWR1OiBNb3ZlIHBsYW5lIGNv bW1pdCBjb2RlIGZyb20gQ1JUQwo+IHN0YXJ0IHRvIENSVEMgcmVzdW1lIikgY2hhbmdlZCB0aGUg b3JkZXIgb2YgdGhlIHBsYW5lIGNvbW1pdCBhbmQgQ1JUQwo+IGVuYWJsZSBvcGVyYXRpb25zIHRv IGFjY29tbW9kYXRlIHRoZSBydW50aW1lIFBNIHJlcXVpcmVtZW50cy4gSG93ZXZlciwKPiB0aGlz IGludHJvZHVjZWQgY29ycnVwdGlvbiBpbiB0aGUgZmlyc3QgZGlzcGxheWVkIGZyYW1lLCBhcyB0 aGUgQ1JUQyBpcwo+IG5vdyBlbmFibGVkIHdpdGhvdXQgYW55IHBsYW5lIGNvbmZpZ3VyZWQuIE9u IEdlbjIgaGFyZHdhcmUgdGhlIGZpcnN0Cj4gZnJhbWUgd2lsbCBiZSBibGFjayBhbmQgbGlrZWx5 IHVubm90aWNlZCwgYnV0IG9uIEdlbjMgaGFyZHdhcmUgd2UgZW5kIHVwCj4gc3RhcnRpbmcgdGhl IGRpc3BsYXkgYmVmb3JlIHRoZSBWU1AgY29tcG9zaXRvciwgd2hpY2ggaXMgbW9yZQo+IG5vdGlj ZWFibGUuCj4gCj4gVG8gZml4IHRoaXMsIHJldmVydCB0aGUgb3JkZXIgb2YgdGhlIGNvbW1pdCBv cGVyYXRpb25zIGJhY2ssIGFuZCBoYW5kbGUKPiBydW50aW1lIFBNIHJlcXVpcmVtZW50cyBpbiB0 aGUgQ1JUQyAuYXRvbWljX2JlZ2luKCkgYW5kIC5hdG9taWNfZW5hYmxlKCkKPiBoZWxwZXIgb3Bl cmF0aW9uIGhhbmRsZXJzLgoKSSBiZWxpZXZlIHRoYXQgdGhlICJydW50aW1lIFBNIiBvcmRlciBp cyBwcm9ibGVtYXRpYyBpbiBtb3N0IGRyaXZlcnMuIFRoZSAKcHJvYmxlbSB1c3VhbGx5IGdvZXMg dW5ub3RpY2VkIGFzIG1vc3QgbW9uaXRvcnMgd2lsbCBub3QgZXZlbiBkaXNwbGF5IHRoZSAKZmly c3QgZnJhbWUsIGFuZCBJIGFzc3VtZSBtYW55IGRldmljZXMgd2lsbCBqdXN0IG91dHB1dCBpdCBi bGFjaywgYnV0IGl0J3MgYW4gCmlzc3VlIG5vbmV0aGVsZXNzLgoKTm90ZSB0aGF0IG15IGRyaXZl ciBoYXNuJ3QgbG9zdCB0aGUgInJ1bnRpbWUgUE0iIHJlcXVpcmVtZW50cywgc28gSSBoYWQgdG8g CnN1cHBvcnQgdGhlbSB3aXRoIHRoZSAic3RhbmRhcmQiIG9yZGVyLiBUaGUgYmVzdCB3YXkgSSd2 ZSBmb3VuZCB3YXMgdG8gcnVudGltZSAKcmVzdW1lIGluIHRoZSBvbmUgb2YgLmF0b21pY19iZWdp bigpIGFuZCAuZW5hYmxlKCkgdGhhdCBpcyBydW4gZmlyc3QuIE5vdCB2ZXJ5IApuZWF0LCBhcyBz aW1pbGFyIGNvZGUgd291bGQgYmUgbmVlZGVkIGluIG1vc3QgZHJpdmVycy4gSSB3b25kZXIgd2hl dGhlciBpdCAKd291bGRuJ3QgYmUgdXNlZnVsIHRvIGFkZCByZXN1bWUvc3VzcGVuZCBoZWxwZXIg Y2FsbGJhY2tzIGZvciB0aGUgQ1JUQy4KCj4gIGRyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2Nr Y2hpcF9kcm1fZmIuYyB8IDIxICstLS0tLS0tLS0tCj4gIGluY2x1ZGUvZHJtL2RybV9hdG9taWNf aGVscGVyLmggICAgICAgICAgICB8ICAxICstCj4gIDUgZmlsZXMgY2hhbmdlZCwgMzYgaW5zZXJ0 aW9ucygrKSwgNzggZGVsZXRpb25zKC0pCgotLSAKUmVnYXJkcywKCkxhdXJlbnQgUGluY2hhcnQK Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZl bCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xp c3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: From: Laurent Pinchart To: Maxime Ripard Cc: Daniel Vetter , David Airlie , Jani Nikula , Sean Paul , Inki Dae , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Kukjin Kim , Krzysztof Kozlowski , Mark Yao , Heiko Stuebner , Chen-Yu Tsai , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-rockchip@lists.infradead.org Subject: Re: [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users Date: Fri, 14 Jul 2017 02:43:12 +0300 Message-ID: <1823154.LT2zYSFJ4j@avalon> In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: Hi Maxime, Thank you for the patch. On Thursday 13 Jul 2017 16:41:13 Maxime Ripard wrote: > The current drm_atomic_helper_commit_tail helper works only if the CRTC is > accessible, and documents an alternative implementation that is supposed to > be used if that happens. > > That implementation is then duplicated by some drivers. Instead of > documenting it, let's implement an helper that all the relevant users can > use directly. > > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++-------- > drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +--------- I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker" that changes the rcar-du implementation to the standard disable/update planes/enable order, so I'd appreciate if you could drop the rcar-du part of this patch to avoid conflicts. This being said, the reason why I switched back from the "runtime PM" to the "standard" order is probably of interest to you. Quoting the commit message, > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC > start to CRTC resume") changed the order of the plane commit and CRTC > enable operations to accommodate the runtime PM requirements. However, > this introduced corruption in the first displayed frame, as the CRTC is > now enabled without any plane configured. On Gen2 hardware the first > frame will be black and likely unnoticed, but on Gen3 hardware we end up > starting the display before the VSP compositor, which is more > noticeable. > > To fix this, revert the order of the commit operations back, and handle > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable() > helper operation handlers. I believe that the "runtime PM" order is problematic in most drivers. The problem usually goes unnoticed as most monitors will not even display the first frame, and I assume many devices will just output it black, but it's an issue nonetheless. Note that my driver hasn't lost the "runtime PM" requirements, so I had to support them with the "standard" order. The best way I've found was to runtime resume in the one of .atomic_begin() and .enable() that is run first. Not very neat, as similar code would be needed in most drivers. I wonder whether it wouldn't be useful to add resume/suspend helper callbacks for the CRTC. > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +---------- > include/drm/drm_atomic_helper.h | 1 +- > 5 files changed, 36 insertions(+), 78 deletions(-) -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Fri, 14 Jul 2017 02:43:12 +0300 Subject: [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users In-Reply-To: References: Message-ID: <1823154.LT2zYSFJ4j@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Maxime, Thank you for the patch. On Thursday 13 Jul 2017 16:41:13 Maxime Ripard wrote: > The current drm_atomic_helper_commit_tail helper works only if the CRTC is > accessible, and documents an alternative implementation that is supposed to > be used if that happens. > > That implementation is then duplicated by some drivers. Instead of > documenting it, let's implement an helper that all the relevant users can > use directly. > > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++-------- > drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +------------- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +--------- I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker" that changes the rcar-du implementation to the standard disable/update planes/enable order, so I'd appreciate if you could drop the rcar-du part of this patch to avoid conflicts. This being said, the reason why I switched back from the "runtime PM" to the "standard" order is probably of interest to you. Quoting the commit message, > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC > start to CRTC resume") changed the order of the plane commit and CRTC > enable operations to accommodate the runtime PM requirements. However, > this introduced corruption in the first displayed frame, as the CRTC is > now enabled without any plane configured. On Gen2 hardware the first > frame will be black and likely unnoticed, but on Gen3 hardware we end up > starting the display before the VSP compositor, which is more > noticeable. > > To fix this, revert the order of the commit operations back, and handle > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable() > helper operation handlers. I believe that the "runtime PM" order is problematic in most drivers. The problem usually goes unnoticed as most monitors will not even display the first frame, and I assume many devices will just output it black, but it's an issue nonetheless. Note that my driver hasn't lost the "runtime PM" requirements, so I had to support them with the "standard" order. The best way I've found was to runtime resume in the one of .atomic_begin() and .enable() that is run first. Not very neat, as similar code would be needed in most drivers. I wonder whether it wouldn't be useful to add resume/suspend helper callbacks for the CRTC. > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +---------- > include/drm/drm_atomic_helper.h | 1 +- > 5 files changed, 36 insertions(+), 78 deletions(-) -- Regards, Laurent Pinchart