From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from perceval.ideasonboard.com ([213.167.242.64]:34698 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726843AbeG3P7J (ORCPT ); Mon, 30 Jul 2018 11:59:09 -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: Mon, 30 Jul 2018 17:24:31 +0300 Message-ID: <2566982.Bc5DNITNHH@avalon> In-Reply-To: References: <20180728154007.GA28426@jordon-HP-15-Notebook-PC> <10502231.oSnn9dME1M@avalon> 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, On Monday, 30 July 2018 16:58:09 EEST Souptick Joarder wrote: > On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart wrote: > > 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. > > No, any extention of drm_fbdev_cma_set_suspend_unlocked() will not have > any impact on driver because with this patch we will be retaining the > original suspend/resume logic of the rcar-du driver and further this driver > is not going to use drm_fbdev_cma_set_suspend_unlocked(). My point is that if the fb cma helpers gets later extended with a feature that need special handling and suspend/resume time, with the drm_fbdev_cma_set_suspend_unlocked() function properly updated to take that feature into account, driver using those helpers but converted to drm_atomic_helper_suspend/resume() will break. > > 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(). > > The consumers of drm_fbdev_cma_set_suspend_unlocked() are - > drivers/gpu/drm/arm/hdlcd_drv.c > drivers/gpu/drm/drm_fb_cma_helper.c > > and both will be converted to use API > drm_mode_config_helper_suspend/resume(). As there will be no more consumer > of drm_fbdev_cma_set_suspend_unlocked() , we can remove this wrapper API > forever :) OK, if you remove the function completely then anyone wanting to extend the fbdev cma helpers in the way described above will notice that something will need to be done, so it's fine. Please thus make sure that you go all the way to removing that function. > >>> 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. > >> > >> >> - 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. > > Sure, I will remove it in v2. > > >>>> - 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: Mon, 30 Jul 2018 17:24:31 +0300 Message-ID: <2566982.Bc5DNITNHH@avalon> References: <20180728154007.GA28426@jordon-HP-15-Notebook-PC> <10502231.oSnn9dME1M@avalon> 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 998CB6E1B4 for ; Mon, 30 Jul 2018 14:23:55 +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 SGkgU291cHRpY2ssCgpPbiBNb25kYXksIDMwIEp1bHkgMjAxOCAxNjo1ODowOSBFRVNUIFNvdXB0 aWNrIEpvYXJkZXIgd3JvdGU6Cj4gT24gU3VuLCBKdWwgMjksIDIwMTggYXQgMTo1MCBBTSwgTGF1 cmVudCBQaW5jaGFydCB3cm90ZToKPiA+IE9uIFNhdHVyZGF5LCAyOCBKdWx5IDIwMTggMjE6NTA6 NTggRUVTVCBTb3VwdGljayBKb2FyZGVyIHdyb3RlOgo+ID4+IE9uIFNhdCwgSnVsIDI4LCAyMDE4 IGF0IDExOjIwIFBNLCBWYWlzaGFsaSBUaGFra2FyIHdyb3RlOgo+ID4+PiBPbiBTYXQsIEp1bCAy OCwgMjAxOCBhdCA5OjEwIFBNLCBTb3VwdGljayBKb2FyZGVyIHdyb3RlOgo+ID4+Pj4gY29udmVy dCBkcm1fYXRvbWljX2hlbHBlcl9zdXNwZW5kL3Jlc3VtZSgpIHRvIHVzZQo+ID4+Pj4gZHJtX21v ZGVfY29uZmlnX2hlbHBlcl9zdXNwZW5kL3Jlc3VtZSgpLgo+ID4+PiAKPiA+Pj4gSGkgU291cHRp Y2ssCj4gPj4+IAo+ID4+PiBUaGFua3MgZm9yIHlvdXIgcGF0Y2guCj4gPj4+IAo+ID4+Pj4gU2ln bmVkLW9mZi1ieTogU291cHRpY2sgSm9hcmRlciA8anJkci5saW51eEBnbWFpbC5jb20+Cj4gPj4+ PiBTaWduZWQtb2ZmLWJ5OiBBaml0IE5lZ2kgPGFqaXRuLmxpbnV4QGdtYWlsLmNvbT4KPiA+Pj4+ IC0tLQo+ID4+Pj4gCj4gPj4+PiAgZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV9kcnYu YyB8IDIxICsrLS0tLS0tLS0tLS0tLS0tLS0tLQo+ID4+Pj4gIDEgZmlsZSBjaGFuZ2VkLCAyIGlu c2VydGlvbnMoKyksIDE5IGRlbGV0aW9ucygtKQo+ID4+Pj4gCj4gPj4+PiBkaWZmIC0tZ2l0IGEv ZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV9kcnYuYwo+ID4+Pj4gYi9kcml2ZXJzL2dw dS9kcm0vcmNhci1kdS9yY2FyX2R1X2Rydi5jIGluZGV4IDAyYWVlNmMuLjI4ODIyMGYgMTAwNjQ0 Cj4gPj4+PiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vcmNhci1kdS9yY2FyX2R1X2Rydi5jCj4gPj4+ PiArKysgYi9kcml2ZXJzL2dwdS9kcm0vcmNhci1kdS9yY2FyX2R1X2Rydi5jCj4gPj4+PiBAQCAt MzU3LDMyICszNTcsMTUgQEAgc3RhdGljIHZvaWQgcmNhcl9kdV9sYXN0Y2xvc2Uoc3RydWN0IGRy bV9kZXZpY2UKPiA+Pj4+ICpkZXYpCj4gPj4+PiAKPiA+Pj4+ICBzdGF0aWMgaW50IHJjYXJfZHVf cG1fc3VzcGVuZChzdHJ1Y3QgZGV2aWNlICpkZXYpCj4gPj4+PiAgewo+ID4+Pj4gICAgICAgICBz dHJ1Y3QgcmNhcl9kdV9kZXZpY2UgKnJjZHUgPSBkZXZfZ2V0X2RydmRhdGEoZGV2KTsKPiA+Pj4+ IC0gICAgICAgc3RydWN0IGRybV9hdG9taWNfc3RhdGUgKnN0YXRlOwo+ID4+Pj4gCj4gPj4+PiAt ICAgICAgIGRybV9rbXNfaGVscGVyX3BvbGxfZGlzYWJsZShyY2R1LT5kZGV2KTsKPiA+Pj4+IC0g ICAgICAgZHJtX2ZiZGV2X2NtYV9zZXRfc3VzcGVuZF91bmxvY2tlZChyY2R1LT5mYmRldiwgdHJ1 ZSk7Cj4gPj4+PiAtCj4gPj4+PiAtICAgICAgIHN0YXRlID0gZHJtX2F0b21pY19oZWxwZXJfc3Vz cGVuZChyY2R1LT5kZGV2KTsKPiA+Pj4+IC0gICAgICAgaWYgKElTX0VSUihzdGF0ZSkpIHsKPiA+ Pj4+IC0gICAgICAgICAgICAgICBkcm1fZmJkZXZfY21hX3NldF9zdXNwZW5kX3VubG9ja2VkKHJj ZHUtPmZiZGV2LAo+ID4+Pj4gZmFsc2UpOwo+ID4+PiAKPiA+Pj4gSSBkb24ndCB0aGluayB3ZSBj YW4gdXNlIGRybV9tb2RlX2NvbmZpZ19oZWxwZXJfKHN1c3BlbmQvcmVzdW1lKQo+ID4+PiBBUEkg aGVyZSBhcyB0aGlzIGZpbGUgdXNlcyBDTUEgZnVuY3Rpb25zLgo+ID4+IAo+ID4+IGRybV9mYmRl dl9jbWFfc2V0X3N1c3BlbmRfdW5sb2NrZWQoKSBpcyB3cmFwcGVyIGZ1bmN0aW9uIHdoaWNoCj4g Pj4gaW52b2tlcyBkcm1fZmJfaGVscGVyX3NldF9zdXNwZW5kX3VubG9ja2VkKCkuCj4gPj4gCj4g Pj4gV2hlcmUgdGhlIG5ldyBBUEkgZHJtX21vZGVfY29uZmlnX2hlbHBlcl9zdXNwZW5kL3Jlc3Vt ZSgpIGRpcmVjdGx5Cj4gPj4gaW52b2tlcwo+ID4+IGRybV9mYl9oZWxwZXJfc2V0X3N1c3BlbmRf dW5sb2NrZWQoKS4gU28gaXQgaXMgc2FmZSB0byByZXBsYWNlIGV4aXRpbmcKPiA+PiBjb2RlIHdp dGggQVBJIGRybV9tb2RlX2NvbmZpZ19oZWxwZXJfc3VzcGVuZC9yZXN1bWUoKS4KPiA+IAo+ID4g SSBhZ3JlZSB0aGF0IHRoZXkncmUgZnVuY3Rpb25hbGx5IGVxdWl2YWxlbnQgZm9yIG5vdywgYnV0 IHdoYXQgaWYKPiA+IGRybV9mYmRldl9jbWFfc2V0X3N1c3BlbmRfdW5sb2NrZWQoKSBnZXRzIGV4 dGVuZGVkIGxhdGVyID8gVGhpcyBjaGFuZ2UKPiA+IHJpc2tzIGludHJvZHVjaW5nIGEgYnJlYWth Z2UgdGhhdCBjb3VsZCBjb3VsZCB1bm5vdGljZWQgYXQgdGhhdCBwb2ludC4KPiAKPiBObywgYW55 IGV4dGVudGlvbiBvZiBkcm1fZmJkZXZfY21hX3NldF9zdXNwZW5kX3VubG9ja2VkKCkgd2lsbCBu b3QgaGF2ZQo+IGFueSBpbXBhY3Qgb24gZHJpdmVyIGJlY2F1c2Ugd2l0aCB0aGlzIHBhdGNoIHdl IHdpbGwgYmUgcmV0YWluaW5nIHRoZQo+IG9yaWdpbmFsIHN1c3BlbmQvcmVzdW1lIGxvZ2ljIG9m IHRoZSByY2FyLWR1IGRyaXZlciBhbmQgZnVydGhlciB0aGlzIGRyaXZlcgo+IGlzIG5vdCBnb2lu ZyB0byB1c2UgZHJtX2ZiZGV2X2NtYV9zZXRfc3VzcGVuZF91bmxvY2tlZCgpLgoKTXkgcG9pbnQg aXMgdGhhdCBpZiB0aGUgZmIgY21hIGhlbHBlcnMgZ2V0cyBsYXRlciBleHRlbmRlZCB3aXRoIGEg ZmVhdHVyZSB0aGF0IApuZWVkIHNwZWNpYWwgaGFuZGxpbmcgYW5kIHN1c3BlbmQvcmVzdW1lIHRp bWUsIHdpdGggdGhlIApkcm1fZmJkZXZfY21hX3NldF9zdXNwZW5kX3VubG9ja2VkKCkgZnVuY3Rp b24gcHJvcGVybHkgdXBkYXRlZCB0byB0YWtlIHRoYXQgCmZlYXR1cmUgaW50byBhY2NvdW50LCBk cml2ZXIgdXNpbmcgdGhvc2UgaGVscGVycyBidXQgY29udmVydGVkIHRvIApkcm1fYXRvbWljX2hl bHBlcl9zdXNwZW5kL3Jlc3VtZSgpIHdpbGwgYnJlYWsuCgo+ID4gQXQgdGhlIHZlcnkKPiA+IGxl YXN0IHlvdSBzaG91bGQgYWRkIGEgY29tbWVudCBpbiBkcm1fZmJkZXZfY21hX3NldF9zdXNwZW5k X3VubG9ja2VkKCkgdG8KPiA+IGV4cGxhaW4gdGhhdCBhbnkgZXh0ZW5zaW9uIG9mIHRoZSBmdW5j dGlvbiBzaG91bGQgYWxzbyBhZGRyZXNzIGFsbCBkcml2ZXJzCj4gPiB1c2luZyBkcm1fbW9kZV9j b25maWdfaGVscGVyX3N1c3BlbmQoKSBhbmQKPiA+IGRybV9tb2RlX2NvbmZpZ19oZWxwZXJfcmVz dW1lKCkuCj4gCj4gVGhlIGNvbnN1bWVycyBvZiBkcm1fZmJkZXZfY21hX3NldF9zdXNwZW5kX3Vu bG9ja2VkKCkgYXJlIC0KPiBkcml2ZXJzL2dwdS9kcm0vYXJtL2hkbGNkX2Rydi5jCj4gZHJpdmVy cy9ncHUvZHJtL2RybV9mYl9jbWFfaGVscGVyLmMKPiAKPiBhbmQgYm90aCB3aWxsIGJlIGNvbnZl cnRlZCB0byB1c2UgQVBJCj4gZHJtX21vZGVfY29uZmlnX2hlbHBlcl9zdXNwZW5kL3Jlc3VtZSgp LiBBcyB0aGVyZSB3aWxsIGJlIG5vIG1vcmUgY29uc3VtZXIKPiBvZiBkcm1fZmJkZXZfY21hX3Nl dF9zdXNwZW5kX3VubG9ja2VkKCkgLCB3ZSBjYW4gcmVtb3ZlIHRoaXMgd3JhcHBlciBBUEkKPiBm b3JldmVyIDopCgpPSywgaWYgeW91IHJlbW92ZSB0aGUgZnVuY3Rpb24gY29tcGxldGVseSB0aGVu IGFueW9uZSB3YW50aW5nIHRvIGV4dGVuZCB0aGUgCmZiZGV2IGNtYSBoZWxwZXJzIGluIHRoZSB3 YXkgZGVzY3JpYmVkIGFib3ZlIHdpbGwgbm90aWNlIHRoYXQgc29tZXRoaW5nIHdpbGwgCm5lZWQg dG8gYmUgZG9uZSwgc28gaXQncyBmaW5lLiBQbGVhc2UgdGh1cyBtYWtlIHN1cmUgdGhhdCB5b3Ug Z28gYWxsIHRoZSB3YXkgCnRvIHJlbW92aW5nIHRoYXQgZnVuY3Rpb24uCgo+ID4+PiBBbmQgZnJv bSBnaXQgZ3JlcCBpdCBzZWVtcyB0aGF0IHRoZXJlIGFyZSB2ZXJ5IGZldyBkcml2ZXJzIHVzaW5n IGl0IGF0Cj4gPj4+IHRoZSBtb21lbnQsIHNvIG5vdCBzdXJlIGlmIGludHJvZHVjaW5nIG5ldyBB UEkgZnVuY3Rpb25zIHNpbWlsYXIgdG8KPiA+Pj4gZHJtX21vZGVfY29uZmlnIHdpbGwgbWFrZSBz ZW5zZSBvciBub3QuCj4gPj4gCj4gPj4gaHR0cHM6Ly93d3cua2VybmVsLm9yZy9kb2MvaHRtbC9s YXRlc3QvZ3B1L3RvZG8uaHRtbAo+ID4+IAo+ID4+IEl0IHdhcyBwaWNrZWQgdXAgZnJvbSBUT0RP IGxpc3QgYWZ0ZXIgZGlzY3Vzc2luZyB3aXRoCj4gPj4gRGFuaWVsLgo+ID4+IAo+ID4+ID4+IC0g ICAgICAgICAgICAgICBkcm1fa21zX2hlbHBlcl9wb2xsX2VuYWJsZShyY2R1LT5kZGV2KTsKPiA+ PiA+PiAtICAgICAgICAgICAgICAgcmV0dXJuIFBUUl9FUlIoc3RhdGUpOwo+ID4+ID4+IC0gICAg ICAgfQo+ID4+ID4+IC0KPiA+PiA+PiAtICAgICAgIHJjZHUtPnN1c3BlbmRfc3RhdGUgPSBzdGF0 ZTsKPiA+IAo+ID4gQWRkaXRpb25hbGx5LCBJIHRoaW5rIHlvdSBjYW4gcmVtb3ZlIHRoZSBzdXNw ZW5kX3N0YXRlIGZpZWxkIGZyb20gdGhlIHJjZHUKPiA+IHN0cnVjdHVyZS4KPiAKPiBTdXJlLCBJ IHdpbGwgcmVtb3ZlIGl0IGluIHYyLgo+IAo+ID4+Pj4gLSAgICAgICByZXR1cm4gMDsKPiA+Pj4+ ICsgICAgICAgcmV0dXJuIGRybV9tb2RlX2NvbmZpZ19oZWxwZXJfc3VzcGVuZChyY2R1LT5kZGV2 KTsKPiA+Pj4+ICB9Cj4gPj4+PiAgCj4gPj4+PiAgc3RhdGljIGludCByY2FyX2R1X3BtX3Jlc3Vt ZShzdHJ1Y3QgZGV2aWNlICpkZXYpCj4gPj4+PiAgewo+ID4+Pj4gICAgICAgICBzdHJ1Y3QgcmNh cl9kdV9kZXZpY2UgKnJjZHUgPSBkZXZfZ2V0X2RydmRhdGEoZGV2KTsKPiA+Pj4+IAo+ID4+Pj4g LSAgICAgICBkcm1fYXRvbWljX2hlbHBlcl9yZXN1bWUocmNkdS0+ZGRldiwgcmNkdS0+c3VzcGVu ZF9zdGF0ZSk7Cj4gPj4+PiAtICAgICAgIGRybV9mYmRldl9jbWFfc2V0X3N1c3BlbmRfdW5sb2Nr ZWQocmNkdS0+ZmJkZXYsIGZhbHNlKTsKPiA+Pj4+IC0gICAgICAgZHJtX2ttc19oZWxwZXJfcG9s bF9lbmFibGUocmNkdS0+ZGRldik7Cj4gPj4+PiAtCj4gPj4+PiAtICAgICAgIHJldHVybiAwOwo+ ID4+Pj4gKyAgICAgICByZXR1cm4gZHJtX21vZGVfY29uZmlnX2hlbHBlcl9yZXN1bWUocmNkdS0+ ZGRldik7Cj4gPj4+PiAgfQo+ID4+Pj4gICNlbmRpZgoKLS0gClJlZ2FyZHMsCgpMYXVyZW50IFBp bmNoYXJ0CgoKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f CmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpo dHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo=