Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp4105489imm; Mon, 30 Jul 2018 08:45:45 -0700 (PDT) X-Google-Smtp-Source: AAOMgpfcBWpJ/v1NdjSvXIWtAeO0seeKuJr6fyKeJ3zNLwkoY6gg1nz4xQ0JeHi4QrJvxssx64Il X-Received: by 2002:a17:902:d706:: with SMTP id w6-v6mr1718014ply.158.1532965545019; Mon, 30 Jul 2018 08:45:45 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1532965544; cv=none; d=google.com; s=arc-20160816; b=dZStlvOwTJongR5F3bDlQ5sAQvHd0YMD056ahrM+15clMZGgvITOvRm+RcWZRMl1us qgrBCvrGzUq7QEEB3tzsElWbxXLJ2t2T9pMfW8qx7aZKld7wB1h9vwUehHiQS8YT4ey8 NqceJ3hFt1P7FXCREyaTb1MEgvhQgs9cLi1c/7Q21zpcGQIpdt/xwb0S75wJA2cnN1T4 h/ktcCdouUeSEtzIbzywsMFOmQ/zq9xEtVFQtxHLcxdCdZQip/6EE7X6K2Gy8/ky0gp5 pRqnSaHSZHq6GZ/GA7NWLjnin/Fvr3b0k2xkXB7Plb2WZ1RUMxd6ZULI7w1tE+rU1NXk swHA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:organization:message-id:date:subject:cc:to :from:dkim-signature:arc-authentication-results; bh=ykjIVDdPuoejNP8dxUFXGN1+BbryhzrGylrRHPzHXP4=; b=xB4srWgm6UIny2Q7M5zQ++KMwQlUWvHkLKO/ENkk9ZEROSJkrqY+Vk3tMZiRdbkSsr 6KBjAnasAgvQbaNoes2IYUCvIIUjOUqw7r7t7adO02f7xyHyY2+Y9DRp/5tgFOn+8bFT v2Xz9J4RnfLcCbCgR0MZrH84fS5bJKHB6QVoVUVUMwUHxdJjuj2C4Be6vKIJ6So62cut 3iDUwmyVySozJmbpKVMMU5MrlalpY36nma/o96PE4R6dIcrYi6LXb7plJabzIvFCzAuz mfVVk1+55DOUB+ina5c0YEb+pUpzWngueTm3yQGLx1OgNX6VBuhj2lg0dXhDwCH9lemv 3FOg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=MNUp+46d; 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 Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id b18-v6si10144355pgd.185.2018.07.30.08.45.30; Mon, 30 Jul 2018 08:45:44 -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 (test mode) header.i=@ideasonboard.com header.s=mail header.b=MNUp+46d; 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 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731535AbeG3RUA (ORCPT + 99 others); Mon, 30 Jul 2018 13:20:00 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:40854 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726843AbeG3RUA (ORCPT ); Mon, 30 Jul 2018 13:20:00 -0400 Received: from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi [IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id DD0C41AC2; Mon, 30 Jul 2018 17:44:23 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1532965464; bh=XoDmzvRvDitoyRYwPEkPnrJqNqJ+FTjXBnoQvA5Mak0=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=MNUp+46dEbLJX/16saSUSv2+XJJ7ybfZ/GC8bwCx5x8EjQpQD+AB+UK8p6UeylOql 93EGvZA4z3WVO4ABvkfDhV3cQabIAnsnduiNks3b3RtOmNBmzQNW4JtNmB7OTl0eFJ +kciO6gr43HJ3pCZKNT2udt764TaUfiD9zxrgjxM= From: Laurent Pinchart To: Souptick Joarder 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 Subject: Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume() Date: Mon, 30 Jul 2018 18:45:03 +0300 Message-ID: <1691790.jmkE8kBUkl@avalon> Organization: Ideas on Board Oy In-Reply-To: References: <20180728154007.GA28426@jordon-HP-15-Notebook-PC> <2566982.Bc5DNITNHH@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 ? > 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