Re: [PATCH v13 13/35] drm/tegra: gr2d: Support generic power domain and runtime PM

[Dmitry Osipenko]

04.10.2021 14:01, Ulf Hansson пишет:
> On Fri, 1 Oct 2021 at 21:00, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>>
>> 01.10.2021 17:55, Ulf Hansson пишет:
>>> On Fri, 1 Oct 2021 at 16:29, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>>>>
>>>> 01.10.2021 16:39, Ulf Hansson пишет:
>>>>> On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>>>>>>
>>>>>> Add runtime power management and support generic power domains.
>>>>>>
>>>>>> Tested-by: Peter Geis <pgwipeout@xxxxxxxxx> # Ouya T30
>>>>>> Tested-by: Paul Fertser <fercerpav@xxxxxxxxx> # PAZ00 T20
>>>>>> Tested-by: Nicolas Chauvet <kwizart@xxxxxxxxx> # PAZ00 T20 and TK1 T124
>>>>>> Tested-by: Matt Merhar <mattmerhar@xxxxxxxxxxxxxx> # Ouya T30
>>>>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>>>>> ---
>>>>>>  drivers/gpu/drm/tegra/gr2d.c | 155 +++++++++++++++++++++++++++++++++--
>>>>>
>>>>> [...]
>>>>>
>>>>>>  static int gr2d_remove(struct platform_device *pdev)
>>>>>> @@ -259,15 +312,101 @@ static int gr2d_remove(struct platform_device *pdev)
>>>>>>                 return err;
>>>>>>         }
>>>>>>
>>>>>> +       pm_runtime_dont_use_autosuspend(&pdev->dev);
>>>>>> +       pm_runtime_disable(&pdev->dev);
>>>>>
>>>>> There is no guarantee that the ->runtime_suspend() has been invoked
>>>>> here, which means that clock may be left prepared/enabled beyond this
>>>>> point.
>>>>>
>>>>> I suggest you call pm_runtime_force_suspend(), instead of
>>>>> pm_runtime_disable(), to make sure that gets done.
>>>>
>>>> The pm_runtime_disable() performs the final synchronization, please see [1].
>>>>
>>>> [1]
>>>> https://elixir.bootlin.com/linux/v5.15-rc3/source/drivers/base/power/runtime.c#L1412
>>>
>>> pm_runtime_disable() end up calling _pm_runtime_barrier(), which calls
>>> cancel_work_sync() if dev->power.request_pending has been set.
>>>
>>> If the work that was punted to the pm_wq in rpm_idle() has not been
>>> started yet, we end up just canceling it. In other words, there are no
>>> guarantees it runs to completion.
>>
>> You're right. Although, in a case of this particular patch, the syncing
>> is actually implicitly done by pm_runtime_dont_use_autosuspend().
>>
>> But for drivers which don't use auto-suspend, there is no sync. This
>> looks like a disaster, it's a very common pattern for drivers to
>> 'put+disable'.
>>
>>> Moreover, use space may have bumped the usage count via sysfs for the
>>> device (pm_runtime_forbid()) to keep the device runtime resumed.
>>
>> Right, this is also a disaster in a case of driver removal.
>>
>>>> Calling pm_runtime_force_suspend() isn't correct because each 'enable'
>>>> must have the corresponding 'disable'. Hence there is no problem here.
>>>
>>> pm_runtime_force_suspend() calls pm_runtime_disable(), so I think that
>>> should be fine. No?
>>
>> [adding Rafael]
>>
>> Rafael, could you please explain how drivers are supposed to properly
>> suspend and disable RPM to cut off power and reset state that was
>> altered by the driver's resume callback? What we're missing? Is Ulf's
>> suggestion acceptable?
>>
>> The RPM state of a device is getting reset on driver's removal, hence
>> all refcounts that were bumped by the rpm-resume callback of the device
>> driver will be screwed up if device is kept resumed after removal. I
>> just verified that it's true in practice.
> 
> Note that, what makes the Tegra drivers a bit special is that they are
> always built with CONFIG_PM being set (selected from the "SoC"
> Kconfig).
> 
> Therefore, pm_runtime_force_suspend() can work for some of these
> cases. Using this, would potentially avoid the driver from having to
> runtime resume the device in ->remove(), according to the below
> generic sequence, which is used in many drivers.
> 
> pm_runtime_get_sync()
> clk_disable_unprepare() (+ additional things to turn off the device)
> pm_runtime_disable()
> pm_runtime_put_noidle()

It's not a problem to change this patchset. The problem is that if
you'll grep mainline for 'pm_runtime_disable', you will find that there
are a lot of drivers in a potential trouble.

I'm proposing that we should change pm_runtime_disable() to perform the
syncing with this oneliner:

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index ec94049442b9..5c9f28165824 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1380,6 +1380,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_barrier);
  */
 void __pm_runtime_disable(struct device *dev, bool check_resume)
 {
+	flush_work(&dev->power.work);
+
 	spin_lock_irq(&dev->power.lock);

 	if (dev->power.disable_depth > 0) {

Objections?

The sysfs rpm-forbid is a separate problem and it's less troublesome
since it requires root privileges. It's also not something that
userspace touches casually. For now I don't know what could be done
about it.