linux-kernel - Re: [PATCH 02/10] drm/tidss: Use PM autosuspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c0c735f7-e00a-4574-9edc-07bc83012070@ideasonboard.com>
Date:   Mon, 6 Nov 2023 09:54:20 +0200
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Aradhya Bhatia <a-bhatia1@...com>,
        Devarsh Thakkar <devarsht@...com>,
        Jyri Sarha <jyri.sarha@....fi>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Sakari Ailus <sakari.ailus@....fi>
Subject: Re: [PATCH 02/10] drm/tidss: Use PM autosuspend

On 06/11/2023 00:53, Laurent Pinchart wrote:
> Hi Tomi,
> 
> CC'ing Sakari for his expertise on runtime PM (I think he will soon
> start wishing he would be ignorant in this area).
> 
> On Thu, Nov 02, 2023 at 08:34:45AM +0200, Tomi Valkeinen wrote:
>> On 01/11/2023 15:54, Laurent Pinchart wrote:
>>> On Wed, Nov 01, 2023 at 11:17:39AM +0200, Tomi Valkeinen wrote:
>>>> Use runtime PM autosuspend feature, with 1s timeout, to avoid
>>>> unnecessary suspend-resume cycles when, e.g. the userspace temporarily
>>>> turns off the crtcs when configuring the outputs.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>>>> ---
>>>>    drivers/gpu/drm/tidss/tidss_drv.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
>>>> index f403db11b846..64914331715a 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_drv.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
>>>> @@ -43,7 +43,9 @@ void tidss_runtime_put(struct tidss_device *tidss)
>>>>    
>>>>    	dev_dbg(tidss->dev, "%s\n", __func__);
>>>>    
>>>> -	r = pm_runtime_put_sync(tidss->dev);
>>>> +	pm_runtime_mark_last_busy(tidss->dev);
>>>> +
>>>> +	r = pm_runtime_put_autosuspend(tidss->dev);
>>>>    	WARN_ON(r < 0);
>>>>    }
>>>>    
>>>> @@ -144,6 +146,9 @@ static int tidss_probe(struct platform_device *pdev)
>>>>    
>>>>    	pm_runtime_enable(dev);
>>>>    
>>>> +	pm_runtime_set_autosuspend_delay(dev, 1000);
>>>> +	pm_runtime_use_autosuspend(dev);
>>>> +
>>>>    #ifndef CONFIG_PM
>>>>    	/* If we don't have PM, we need to call resume manually */
>>>>    	dispc_runtime_resume(tidss->dispc);
>>>
>>> By the way, there's a way to handle this without any ifdef:
>>>
>>> 	dispc_runtime_resume(tidss->dispc);
>>>
>>> 	pm_runtime_set_active(dev);
>>> 	pm_runtime_get_noresume(dev);
>>> 	pm_runtime_enable(dev);
>>> 	pm_runtime_set_autosuspend_delay(dev, 1000);
>>> 	pm_runtime_use_autosuspend(dev);
>>
>> I'm not sure I follow what you are trying to do here. The call to
>> dispc_runtime_resume() would crash if we have PM, as the HW would not be
>> enabled at that point.
> 
> Isn't dispc_runtime_resume() meant to enable the hardware ?
> 
> The idea is to enable the hardware, then enable runtime PM, and tell the
> runtime PM framework that the device is enabled. If CONFIG_PM is not
> set, the RPM calls will be no-ops, and the device will stay enable. If
> CONFIG_PM is set, the device will be enabled, and will get disabled at
> end of probe by a call to pm_runtime_put_autosuspend().

(The text below is more about the end result of this series, rather than 
this specific patch):

Hmm, no, I don't think that's how it works. My understanding is this:

There are multiple parts "enabling the hardware", and I think they 
usually need to be done in this order: 1) enabling the parent devices, 
2) system level HW module enable (this is possibly really part of the 
1), 3) clk/regulator/register setup.

3) is handled by the driver, but 1) and 2) are handled via the runtime 
PM framework. Calling dispc_runtime_resume() as the first thing could 
mean that DSS's parents are not enabled or that the DSS HW module is not 
enabled at the system control level.

That's why I first call pm_runtime_set_active(), which should handle 1) 
and 2).

The only thing dispc_runtime_resume() does wrt. enabling the hardware is 
enabling the fclk. It does a lot more, but all the rest is just 
configuring the hardware to settings that we always want to use (e.g. 
fifo management).

Now, if the bootloader had enabled the display, and the driver did:

- pm_runtime_enable()
- pm_runtime_get()
- dispc_reset()

it would cause dispc_runtime_resume() to be called before the reset. 
This would mean that the dispc_runtime_resume() would be changing 
settings that must not be changed while streaming is enabled.

We could do a DSS reset always as the first thing in 
dispc_runtime_resume() (after enabling the fclk), but that feels a bit 
pointless as after the first reset the DSS is in a known state.

Also, if we don't do a reset at probe time, there are things we need to 
take care of: at least we need to mask the IRQs (presuming we register 
the DSS interrupt at probe time). But generally speaking, I feel a bit 
uncomfortable leaving an IP possibly running in an unknown state after 
probe. I'd much rather just reset it at probe.

  Tomi