Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp4168274imm; Mon, 30 Jul 2018 09:46:35 -0700 (PDT) X-Google-Smtp-Source: AAOMgpd+fyjNiiR+X1IRVDODztoF6sh14E2PGO5QDrv8EJjrXynhYtT2/HoklC3jpINl2RcBtUnF X-Received: by 2002:a63:d5b:: with SMTP id 27-v6mr16813953pgn.107.1532969195385; Mon, 30 Jul 2018 09:46:35 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1532969195; cv=none; d=google.com; s=arc-20160816; b=hmsyldaimwSdq5cIfgJhbYCESh44mU5qwq8QiwIN+NHYQ7yILa3J5nedjWZu+cb7aM 2ELarXiiNHOWG8MKEf4EAEhqKHUTeqzY4v1PyryQ8Phai3xyuv9ZJtroRwC1L7h3LDl/ dJsMhtlZ7usqJKotf3387rek6Ph5jzkiFJ43sm9ofYSXyXut9Nbiio7jjYSnvUppm9XF TZbpF78Z0cag7qNKqucZqTLV3bY7Z8/rdabEZFp2hoXwgbokgqFuR760wvAJhStA0sr1 RvkN8PyuWwLeDXnmOnOaNRV+eQ59Idzfxysvxd7cl+sHMIKjMyzbPEbgMCcbAbPk4nVD K8gw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:dkim-signature :arc-authentication-results; bh=kgMLEhNM9b7GmY1Ba1NrSJFNV5Rqd5iFcnLxhP1/1no=; b=wT57AVgy70k+TqJbmaayYUW1+70Y2lGlTq7VcvB3nV8FarmBeZjBIv4JNutGeyie4D A0Q1kmUzAW8+Zr7KAExyYtMbqOMDWPoLmffYjbo2GMEuw1UuTziwV2vNaVFhNHWh6jBT 1rM1eQv/T41Mx3rv4RS99PG9tvxO7UoO1GYd5umPdmUbhsqjJgQlWpr0tBvwVT/ZGNPf WcxhVpjQClLBY79jXwyXt6fag5yrFKod7Ey4zKe4/Ih9y+5sQPZtH4VkRc5dJKXaNHc/ G8vFdHSOs3UDjU3Q422WcTr7g2FG2oUrEIe8HZ1W39/xjTdEqzAtvRw/3XJ4jb2NVbes kI6Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=HCHUfKir; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id a9-v6si10904713pgf.380.2018.07.30.09.46.21; Mon, 30 Jul 2018 09:46:35 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=HCHUfKir; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731930AbeG3SUp (ORCPT + 99 others); Mon, 30 Jul 2018 14:20:45 -0400 Received: from mail-lf1-f65.google.com ([209.85.167.65]:34182 "EHLO mail-lf1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729523AbeG3SUp (ORCPT ); Mon, 30 Jul 2018 14:20:45 -0400 Received: by mail-lf1-f65.google.com with SMTP id n96-v6so8623412lfi.1; Mon, 30 Jul 2018 09:44:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=kgMLEhNM9b7GmY1Ba1NrSJFNV5Rqd5iFcnLxhP1/1no=; b=HCHUfKirPpAmNjlosZ8UEbzo8LjmsswFTuo55Dmc9P1uyr5j/JB4k/3sTQ8In67mLO 87ihqcP8M8+KrP18q5nInGs3amBBXIxc88TiC5B+qCUjb7MW1S21uGunTpTIXVCbD423 +pzmH63s4DYp3fhhkmwsNm4D8kWzxqbEJybDa2d2zWwXT5Ex6XK5331gGQ0TkddEKr+5 h/rp7j3dEzLPWe5SX+gqNRQDgkg/z4+Txq1pHZPh1ELyF76qTlWCUZfPa7eF+LL07TeB UScdSznoXi02hqq2WxUToIJaxEyVkfQfbfbK4YQneu6DjdqEctWJcRtMwpQQ5c+xToZY 3Weg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=kgMLEhNM9b7GmY1Ba1NrSJFNV5Rqd5iFcnLxhP1/1no=; b=PC84oIvUFS+icWbFqOYIAbM6Wg9wdcQHuHbGtf18Vap1Gw6/4iKF1hcsjtANQLWL5/ eIAXAj8tfrH9cZJ11Tdcgv57VJY0OL1oCQYLsgMVfVfO9D17gTQrQYgqukH7Al0h9UIF pCQWqJrP+DbPghGkfApEamc5FZ8Jvv0juokQFgO2y+za56ip6QbwDu+yBFVFTvfwED/W GTg5LbTST3e+XnJ5SwV1kdiMvJyWDtktIe0qoMA6hmaxBpi63FipL4Jhs2RWj8yQTNs0 DtOVDADEtn9MBh//T6GwWodT0vuY54DII+WEVfws4Flp3Ratm4sAlW77Ax07PUWVtTdX LEmQ== X-Gm-Message-State: AOUpUlH3qD3bAWO7fBbAat+LURUgEOWP/bA5sd26mODp6h+TkqI8QO5N ZSH6LynU2w3Re7K070oTrHjJK7B45KaET8JgXuc= X-Received: by 2002:a19:e119:: with SMTP id y25-v6mr11604039lfg.3.1532969093009; Mon, 30 Jul 2018 09:44:53 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a19:2b14:0:0:0:0:0 with HTTP; Mon, 30 Jul 2018 09:44:52 -0700 (PDT) In-Reply-To: <1691790.jmkE8kBUkl@avalon> References: <20180728154007.GA28426@jordon-HP-15-Notebook-PC> <2566982.Bc5DNITNHH@avalon> <1691790.jmkE8kBUkl@avalon> From: Souptick Joarder Date: Mon, 30 Jul 2018 22:14:52 +0530 Message-ID: Subject: Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() To: Laurent Pinchart Cc: airlied@linux.ie, Daniel Vetter , Vaishali Thakkar , Ajit Linux , dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 30, 2018 at 9:15 PM, Laurent Pinchart wrote: > Hi Souptick, > > On Monday, 30 July 2018 18:13:13 EEST Souptick Joarder wrote: >> On Mon, Jul 30, 2018 at 7:54 PM, Laurent Pinchart wrote: >> > 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. >> >> Sure, once both the drivers are converted to use >> drm_mode_config_helper_suspend/resume() >> and goes into linus's tree, then we can remove it. > > Could we get the two driver changes and the function removal merged all > together ? Sure, I will send it. > >> But will wait for some more feedback before concluding on this. >> >> Dave/ Daniel, Would you like to add any feedback ? >> >> >>>>> 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 > > >