Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp4022182imm; Mon, 30 Jul 2018 07:24:59 -0700 (PDT) X-Google-Smtp-Source: AAOMgpfeVvKaBze1z9plKMtaRjaRMm+ai2DHzpoQvi4jKm+RVyQa2bDUeE8FRXArbNJgJV+vsxtk X-Received: by 2002:a63:ba43:: with SMTP id l3-v6mr16062073pgu.295.1532960699762; Mon, 30 Jul 2018 07:24:59 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1532960699; cv=none; d=google.com; s=arc-20160816; b=ECdFbvNN4EI1p/o4iTuKms/lPqGw/U9V1Eua5FUfV/ncLoj8ExwORF5d/AUBuY3fKv TnNH+RjcsanXw0BK5j1X+euDy9x65xRqwZXFMlPVDFUA3r77LxeQcEo0bNELOg7e4Eoc 7ge2dUxUxeusgMUSK/Q+fAf1nTwczBniwsMvQro8lKnlWjXqNvat44xZIqLhm7pujshP qPf0M2rpxVIyeNRFDnxLMD4k1+1TdZKezU+qGwOZ1DSCbe07EHZUrVjXs39Vi23Qd2ye 0gGf46fZCwXWmBsQCqhPQu6MGF1Fosu3a4gDw1U6prhnTFQz69mysBy3TrIjWmRJLQsP a8Uw== 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=vU7K9lqhhJ8MM2ORw81eATcJN4dN6Cn3JlXhlWSdsEw=; b=DF9+KFCMhsGorPaMIpJk8dSWkakQ0MBF73gVP04y2eetFWB+Lz1mFiD4Dt69wTdwBu 1Kojv5+LJuJ1ibmOWWL9pC+lgVH8zRxViyf6V1pNdPXpCzkBqrZ5QsT+rxqxxmXrRbgY ZcdAM1gqksDs+uvQgnjtIliZg6whsPMvHUr1S04dEK15/0gxRP5XRmDBJ6K7boswB0ss kTOQc5pXgFLoOZOwVMHxg5+696G7gUxPefH4wk9NEOyf+WLZEnWstnqRKG7h81osnh8w 4foLmq9d5aLAAly8QM4Kza4XQm8UUa7fGzr5HH3zA98G693DL47h1K5OC7VDTA7Td+Z/ PX3g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=PKvZvas1; 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 d34-v6si10564325pla.195.2018.07.30.07.24.45; Mon, 30 Jul 2018 07:24:59 -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=PKvZvas1; 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 S1730553AbeG3P7J (ORCPT + 99 others); Mon, 30 Jul 2018 11:59:09 -0400 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 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 D44B91AC2; Mon, 30 Jul 2018 16:23:52 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1532960633; bh=8+Bn9XEADFTtw0p4G8fMF7AmBtTA0/+qnWbhXodrQUk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=PKvZvas1/PbeNAWUyJ+Ns2RXwlCuuyb73FA5rOZ0bdSvae5IcHze476feDwnnPgys gl1/L+F9xf2+d+6L9IHoTSUTbtcCF5jC6773wBKt1XYfbtStiKsW1CRdct+LysWTh5 fDptnUcBwt9eRL1L2UoP3ZE3fmA4O93R7pmVvYls= 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> Organization: Ideas on Board Oy 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-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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