From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from perceval.ideasonboard.com ([213.167.242.64]:52742 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730518AbeG1VrO (ORCPT ); Sat, 28 Jul 2018 17:47:14 -0400 From: Laurent Pinchart To: Souptick Joarder Cc: Vaishali Thakkar , airlied@linux.ie, Ajit Linux , dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, Linux Kernel Mailing List , Daniel Vetter Subject: Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() Date: Sat, 28 Jul 2018 23:20:07 +0300 Message-ID: <10502231.oSnn9dME1M@avalon> In-Reply-To: References: <20180728154007.GA28426@jordon-HP-15-Notebook-PC> 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 Souptick, Thank you for the patch. On Saturday, 28 July 2018 21:50:58 EEST Souptick Joarder wrote: > On Sat, Jul 28, 2018 at 11:20 PM, Vaishali Thakkar wrote: > > On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder wrote: > >> convert drm_atomic_helper_suspend/resume() to use > >> drm_mode_config_helper_suspend/resume(). > > > > Hi Souptick, > > > > Thanks for your patch. > > > >> Signed-off-by: Souptick Joarder > >> Signed-off-by: Ajit Negi > >> --- > >> > >> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++------------------- > >> 1 file changed, 2 insertions(+), 19 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > >> @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device > >> *dev) > >> > >> static int rcar_du_pm_suspend(struct device *dev) > >> { > >> struct rcar_du_device *rcdu = dev_get_drvdata(dev); > >> - struct drm_atomic_state *state; > >> > >> - drm_kms_helper_poll_disable(rcdu->ddev); > >> - drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, true); > >> - > >> - state = drm_atomic_helper_suspend(rcdu->ddev); > >> - if (IS_ERR(state)) { > >> - drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false); > > > > I don't think we can use drm_mode_config_helper_(suspend/resume) > > API here as this file uses CMA functions. > > drm_fbdev_cma_set_suspend_unlocked() is wrapper function which > invokes drm_fb_helper_set_suspend_unlocked(). > > Where the new API drm_mode_config_helper_suspend/resume() directly invokes > drm_fb_helper_set_suspend_unlocked(). So it is safe to replace exiting > code with API drm_mode_config_helper_suspend/resume(). I agree that they're functionally equivalent for now, but what if drm_fbdev_cma_set_suspend_unlocked() gets extended later ? This change risks introducing a breakage that could could unnoticed at that point. At the very least you should add a comment in drm_fbdev_cma_set_suspend_unlocked() to explain that any extension of the function should also address all drivers using drm_mode_config_helper_suspend() and drm_mode_config_helper_resume(). > > And from git grep it seems that there are very few drivers using it at the > > moment, so not sure if introducing new API functions similar to > > drm_mode_config will make sense or not. > > https://www.kernel.org/doc/html/latest/gpu/todo.html > > It was picked up from TODO list after discussing with > Daniel. > > > Thanks. > > > >> - drm_kms_helper_poll_enable(rcdu->ddev); > >> - return PTR_ERR(state); > >> - } > >> - > >> - rcdu->suspend_state = state; Additionally, I think you can remove the suspend_state field from the rcdu structure. > >> - return 0; > >> + return drm_mode_config_helper_suspend(rcdu->ddev); > >> } > >> > >> static int rcar_du_pm_resume(struct device *dev) > >> { > >> struct rcar_du_device *rcdu = dev_get_drvdata(dev); > >> > >> - drm_atomic_helper_resume(rcdu->ddev, rcdu->suspend_state); > >> - drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false); > >> - drm_kms_helper_poll_enable(rcdu->ddev); > >> - > >> - return 0; > >> + return drm_mode_config_helper_resume(rcdu->ddev); > >> } > >> #endif -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() Date: Sat, 28 Jul 2018 23:20:07 +0300 Message-ID: <10502231.oSnn9dME1M@avalon> References: <20180728154007.GA28426@jordon-HP-15-Notebook-PC> 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 [213.167.242.64]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4F6586E27A for ; Sat, 28 Jul 2018 20:19:34 +0000 (UTC) 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: Souptick Joarder Cc: Ajit Linux , airlied@linux.ie, Linux Kernel Mailing List , dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, Vaishali Thakkar List-Id: dri-devel@lists.freedesktop.org SGkgU291cHRpY2ssCgpUaGFuayB5b3UgZm9yIHRoZSBwYXRjaC4KCk9uIFNhdHVyZGF5LCAyOCBK dWx5IDIwMTggMjE6NTA6NTggRUVTVCBTb3VwdGljayBKb2FyZGVyIHdyb3RlOgo+IE9uIFNhdCwg SnVsIDI4LCAyMDE4IGF0IDExOjIwIFBNLCBWYWlzaGFsaSBUaGFra2FyIHdyb3RlOgo+ID4gT24g U2F0LCBKdWwgMjgsIDIwMTggYXQgOToxMCBQTSwgU291cHRpY2sgSm9hcmRlciB3cm90ZToKPiA+ PiBjb252ZXJ0IGRybV9hdG9taWNfaGVscGVyX3N1c3BlbmQvcmVzdW1lKCkgdG8gdXNlCj4gPj4g ZHJtX21vZGVfY29uZmlnX2hlbHBlcl9zdXNwZW5kL3Jlc3VtZSgpLgo+ID4gCj4gPiBIaSBTb3Vw dGljaywKPiA+IAo+ID4gVGhhbmtzIGZvciB5b3VyIHBhdGNoLgo+ID4gCj4gPj4gU2lnbmVkLW9m Zi1ieTogU291cHRpY2sgSm9hcmRlciA8anJkci5saW51eEBnbWFpbC5jb20+Cj4gPj4gU2lnbmVk LW9mZi1ieTogQWppdCBOZWdpIDxhaml0bi5saW51eEBnbWFpbC5jb20+Cj4gPj4gLS0tCj4gPj4g Cj4gPj4gIGRyaXZlcnMvZ3B1L2RybS9yY2FyLWR1L3JjYXJfZHVfZHJ2LmMgfCAyMSArKy0tLS0t LS0tLS0tLS0tLS0tLS0KPiA+PiAgMSBmaWxlIGNoYW5nZWQsIDIgaW5zZXJ0aW9ucygrKSwgMTkg ZGVsZXRpb25zKC0pCj4gPj4gCj4gPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9yY2Fy LWR1L3JjYXJfZHVfZHJ2LmMKPiA+PiBiL2RyaXZlcnMvZ3B1L2RybS9yY2FyLWR1L3JjYXJfZHVf ZHJ2LmMgaW5kZXggMDJhZWU2Yy4uMjg4MjIwZiAxMDA2NDQKPiA+PiAtLS0gYS9kcml2ZXJzL2dw dS9kcm0vcmNhci1kdS9yY2FyX2R1X2Rydi5jCj4gPj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL3Jj YXItZHUvcmNhcl9kdV9kcnYuYwo+ID4+IEBAIC0zNTcsMzIgKzM1NywxNSBAQCBzdGF0aWMgdm9p ZCByY2FyX2R1X2xhc3RjbG9zZShzdHJ1Y3QgZHJtX2RldmljZQo+ID4+ICpkZXYpCj4gPj4gCj4g Pj4gIHN0YXRpYyBpbnQgcmNhcl9kdV9wbV9zdXNwZW5kKHN0cnVjdCBkZXZpY2UgKmRldikKPiA+ PiAgewo+ID4+ICAgICAgICAgc3RydWN0IHJjYXJfZHVfZGV2aWNlICpyY2R1ID0gZGV2X2dldF9k cnZkYXRhKGRldik7Cj4gPj4gLSAgICAgICBzdHJ1Y3QgZHJtX2F0b21pY19zdGF0ZSAqc3RhdGU7 Cj4gPj4gCj4gPj4gLSAgICAgICBkcm1fa21zX2hlbHBlcl9wb2xsX2Rpc2FibGUocmNkdS0+ZGRl dik7Cj4gPj4gLSAgICAgICBkcm1fZmJkZXZfY21hX3NldF9zdXNwZW5kX3VubG9ja2VkKHJjZHUt PmZiZGV2LCB0cnVlKTsKPiA+PiAtCj4gPj4gLSAgICAgICBzdGF0ZSA9IGRybV9hdG9taWNfaGVs cGVyX3N1c3BlbmQocmNkdS0+ZGRldik7Cj4gPj4gLSAgICAgICBpZiAoSVNfRVJSKHN0YXRlKSkg ewo+ID4+IC0gICAgICAgICAgICAgICBkcm1fZmJkZXZfY21hX3NldF9zdXNwZW5kX3VubG9ja2Vk KHJjZHUtPmZiZGV2LCBmYWxzZSk7Cj4gPiAKPiA+IEkgZG9uJ3QgdGhpbmsgd2UgY2FuIHVzZSBk cm1fbW9kZV9jb25maWdfaGVscGVyXyhzdXNwZW5kL3Jlc3VtZSkKPiA+IEFQSSBoZXJlIGFzIHRo aXMgZmlsZSB1c2VzIENNQSBmdW5jdGlvbnMuCj4gCj4gZHJtX2ZiZGV2X2NtYV9zZXRfc3VzcGVu ZF91bmxvY2tlZCgpIGlzIHdyYXBwZXIgZnVuY3Rpb24gd2hpY2gKPiBpbnZva2VzIGRybV9mYl9o ZWxwZXJfc2V0X3N1c3BlbmRfdW5sb2NrZWQoKS4KPiAKPiBXaGVyZSB0aGUgbmV3IEFQSSBkcm1f bW9kZV9jb25maWdfaGVscGVyX3N1c3BlbmQvcmVzdW1lKCkgZGlyZWN0bHkgaW52b2tlcwo+IGRy bV9mYl9oZWxwZXJfc2V0X3N1c3BlbmRfdW5sb2NrZWQoKS4gU28gaXQgaXMgc2FmZSB0byByZXBs YWNlIGV4aXRpbmcKPiBjb2RlIHdpdGggQVBJIGRybV9tb2RlX2NvbmZpZ19oZWxwZXJfc3VzcGVu ZC9yZXN1bWUoKS4KCkkgYWdyZWUgdGhhdCB0aGV5J3JlIGZ1bmN0aW9uYWxseSBlcXVpdmFsZW50 IGZvciBub3csIGJ1dCB3aGF0IGlmIApkcm1fZmJkZXZfY21hX3NldF9zdXNwZW5kX3VubG9ja2Vk KCkgZ2V0cyBleHRlbmRlZCBsYXRlciA/IFRoaXMgY2hhbmdlIHJpc2tzIAppbnRyb2R1Y2luZyBh IGJyZWFrYWdlIHRoYXQgY291bGQgY291bGQgdW5ub3RpY2VkIGF0IHRoYXQgcG9pbnQuIEF0IHRo ZSB2ZXJ5IApsZWFzdCB5b3Ugc2hvdWxkIGFkZCBhIGNvbW1lbnQgaW4gZHJtX2ZiZGV2X2NtYV9z ZXRfc3VzcGVuZF91bmxvY2tlZCgpIHRvIApleHBsYWluIHRoYXQgYW55IGV4dGVuc2lvbiBvZiB0 aGUgZnVuY3Rpb24gc2hvdWxkIGFsc28gYWRkcmVzcyBhbGwgZHJpdmVycyAKdXNpbmcgZHJtX21v ZGVfY29uZmlnX2hlbHBlcl9zdXNwZW5kKCkgYW5kIGRybV9tb2RlX2NvbmZpZ19oZWxwZXJfcmVz dW1lKCkuCgo+ID4gQW5kIGZyb20gZ2l0IGdyZXAgaXQgc2VlbXMgdGhhdCB0aGVyZSBhcmUgdmVy eSBmZXcgZHJpdmVycyB1c2luZyBpdCBhdCB0aGUKPiA+IG1vbWVudCwgc28gbm90IHN1cmUgaWYg aW50cm9kdWNpbmcgbmV3IEFQSSBmdW5jdGlvbnMgc2ltaWxhciB0byAKPiA+IGRybV9tb2RlX2Nv bmZpZyB3aWxsIG1ha2Ugc2Vuc2Ugb3Igbm90Lgo+IAo+IGh0dHBzOi8vd3d3Lmtlcm5lbC5vcmcv ZG9jL2h0bWwvbGF0ZXN0L2dwdS90b2RvLmh0bWwKPiAKPiBJdCB3YXMgcGlja2VkIHVwIGZyb20g VE9ETyBsaXN0IGFmdGVyIGRpc2N1c3Npbmcgd2l0aAo+IERhbmllbC4KPiAKPiA+IFRoYW5rcy4K PiA+IAo+ID4+IC0gICAgICAgICAgICAgICBkcm1fa21zX2hlbHBlcl9wb2xsX2VuYWJsZShyY2R1 LT5kZGV2KTsKPiA+PiAtICAgICAgICAgICAgICAgcmV0dXJuIFBUUl9FUlIoc3RhdGUpOwo+ID4+ IC0gICAgICAgfQo+ID4+IC0KPiA+PiAtICAgICAgIHJjZHUtPnN1c3BlbmRfc3RhdGUgPSBzdGF0 ZTsKCkFkZGl0aW9uYWxseSwgSSB0aGluayB5b3UgY2FuIHJlbW92ZSB0aGUgc3VzcGVuZF9zdGF0 ZSBmaWVsZCBmcm9tIHRoZSByY2R1IApzdHJ1Y3R1cmUuCgo+ID4+IC0gICAgICAgcmV0dXJuIDA7 Cj4gPj4gKyAgICAgICByZXR1cm4gZHJtX21vZGVfY29uZmlnX2hlbHBlcl9zdXNwZW5kKHJjZHUt PmRkZXYpOwo+ID4+ICB9Cj4gPj4gIAo+ID4+ICBzdGF0aWMgaW50IHJjYXJfZHVfcG1fcmVzdW1l KHN0cnVjdCBkZXZpY2UgKmRldikKPiA+PiAgewo+ID4+ICAgICAgICAgc3RydWN0IHJjYXJfZHVf ZGV2aWNlICpyY2R1ID0gZGV2X2dldF9kcnZkYXRhKGRldik7Cj4gPj4gCj4gPj4gLSAgICAgICBk cm1fYXRvbWljX2hlbHBlcl9yZXN1bWUocmNkdS0+ZGRldiwgcmNkdS0+c3VzcGVuZF9zdGF0ZSk7 Cj4gPj4gLSAgICAgICBkcm1fZmJkZXZfY21hX3NldF9zdXNwZW5kX3VubG9ja2VkKHJjZHUtPmZi ZGV2LCBmYWxzZSk7Cj4gPj4gLSAgICAgICBkcm1fa21zX2hlbHBlcl9wb2xsX2VuYWJsZShyY2R1 LT5kZGV2KTsKPiA+PiAtCj4gPj4gLSAgICAgICByZXR1cm4gMDsKPiA+PiArICAgICAgIHJldHVy biBkcm1fbW9kZV9jb25maWdfaGVscGVyX3Jlc3VtZShyY2R1LT5kZGV2KTsKPiA+PiAgfQo+ID4+ ICAjZW5kaWYKCi0tIApSZWdhcmRzLAoKTGF1cmVudCBQaW5jaGFydAoKCgpfX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0 CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3Rv cC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK