[58/78] media: exynos-gsc: use pm_runtime_resume_and_get()
diff mbox series

Message ID 9c7d683907b9f9cf4a99f57f978671ec7f5a1dbc.1619191723.git.mchehab+huawei@kernel.org
State New, archived
Headers show
Series
  • media: use pm_runtime_resume_and_get() instead of pm_runtime_get_sync()
Related show

Commit Message

Mauro Carvalho Chehab April 24, 2021, 6:45 a.m. UTC
Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added pm_runtime_resume_and_get() in order to automatically handle
dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/exynos-gsc/gsc-core.c | 3 +--
 drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

Comments

Sylwester Nawrocki April 27, 2021, 8:18 a.m. UTC | #1
On 24.04.2021 08:45, Mauro Carvalho Chehab wrote:
> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> added pm_runtime_resume_and_get() in order to automatically handle
> dev->power.usage_count decrement on errors.
> 
> Use the new API, in order to cleanup the error check logic.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/platform/exynos-gsc/gsc-core.c | 3 +--
>  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 9f41c2e7097a..9d5841194f6b 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
>  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
>  	int i;
>  
> -	pm_runtime_get_sync(&pdev->dev);
> +	pm_runtime_resume_and_get(&pdev->dev);
>  
>  	gsc_unregister_m2m_device(gsc);
>  	v4l2_device_unregister(&gsc->v4l2_dev);
> @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
>  	for (i = 0; i < gsc->num_clocks; i++)
>  		clk_disable_unprepare(gsc->clock[i]);
>  
> -	pm_runtime_put_noidle(&pdev->dev);

If we do this then the device usage count will not get decremented
after the pm_runtime_resume_and_get() call above and after driver
unload/load cycle it will not be possible to suspend the device.
I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
works better in that case.

Regards,
Sylwester

>  	pm_runtime_disable(&pdev->dev);
>  
>  	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name
Mauro Carvalho Chehab April 27, 2021, 9:30 a.m. UTC | #2
Em Tue, 27 Apr 2021 10:18:12 +0200
Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:

> On 24.04.2021 08:45, Mauro Carvalho Chehab wrote:
> > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > added pm_runtime_resume_and_get() in order to automatically handle
> > dev->power.usage_count decrement on errors.
> > 
> > Use the new API, in order to cleanup the error check logic.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  drivers/media/platform/exynos-gsc/gsc-core.c | 3 +--
> >  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> > index 9f41c2e7097a..9d5841194f6b 100644
> > --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> > +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> > @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
> >  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
> >  	int i;
> >  
> > -	pm_runtime_get_sync(&pdev->dev);
> > +	pm_runtime_resume_and_get(&pdev->dev);
> >  
> >  	gsc_unregister_m2m_device(gsc);
> >  	v4l2_device_unregister(&gsc->v4l2_dev);
> > @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
> >  	for (i = 0; i < gsc->num_clocks; i++)
> >  		clk_disable_unprepare(gsc->clock[i]);
> >  
> > -	pm_runtime_put_noidle(&pdev->dev);  
> 
> If we do this then the device usage count will not get decremented
> after the pm_runtime_resume_and_get() call above and after driver
> unload/load cycle it will not be possible to suspend the device.
> I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
> works better in that case.

Good point.

Actually, I don't see any reason why to call a PM resume
function - either being pm_runtime_get_sync() or
pm_runtime_resume_and_get().

The code there could simply be:

    static int gsc_remove(struct platform_device *pdev)
    {
        struct gsc_dev *gsc = platform_get_drvdata(pdev);
        int i;

        gsc_unregister_m2m_device(gsc);
        v4l2_device_unregister(&gsc->v4l2_dev);

        vb2_dma_contig_clear_max_seg_size(&pdev->dev);
        for (i = 0; i < gsc->num_clocks; i++)
                clk_disable_unprepare(gsc->clock[i]);

        pm_runtime_disable(&pdev->dev);

        dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
        return 0;
    }

Eventually also adding:
	pm_runtime_suspended(&pdev->dev);

just after pm_runtime_disable().

Regards,
Mauro
Mauro Carvalho Chehab April 27, 2021, 9:42 a.m. UTC | #3
Em Tue, 27 Apr 2021 11:30:55 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Tue, 27 Apr 2021 10:18:12 +0200
> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
> 
> > On 24.04.2021 08:45, Mauro Carvalho Chehab wrote:  
> > > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > > added pm_runtime_resume_and_get() in order to automatically handle
> > > dev->power.usage_count decrement on errors.
> > > 
> > > Use the new API, in order to cleanup the error check logic.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > >  drivers/media/platform/exynos-gsc/gsc-core.c | 3 +--
> > >  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
> > >  2 files changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> > > index 9f41c2e7097a..9d5841194f6b 100644
> > > --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> > > +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> > > @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
> > >  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
> > >  	int i;
> > >  
> > > -	pm_runtime_get_sync(&pdev->dev);
> > > +	pm_runtime_resume_and_get(&pdev->dev);
> > >  
> > >  	gsc_unregister_m2m_device(gsc);
> > >  	v4l2_device_unregister(&gsc->v4l2_dev);
> > > @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
> > >  	for (i = 0; i < gsc->num_clocks; i++)
> > >  		clk_disable_unprepare(gsc->clock[i]);
> > >  
> > > -	pm_runtime_put_noidle(&pdev->dev);    
> > 
> > If we do this then the device usage count will not get decremented
> > after the pm_runtime_resume_and_get() call above and after driver
> > unload/load cycle it will not be possible to suspend the device.
> > I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
> > works better in that case.  
> 
> Good point.
> 
> Actually, I don't see any reason why to call a PM resume
> function - either being pm_runtime_get_sync() or
> pm_runtime_resume_and_get().
> 
> The code there could simply be:
> 
>     static int gsc_remove(struct platform_device *pdev)
>     {
>         struct gsc_dev *gsc = platform_get_drvdata(pdev);
>         int i;
> 
>         gsc_unregister_m2m_device(gsc);
>         v4l2_device_unregister(&gsc->v4l2_dev);
> 
>         vb2_dma_contig_clear_max_seg_size(&pdev->dev);
>         for (i = 0; i < gsc->num_clocks; i++)
>                 clk_disable_unprepare(gsc->clock[i]);
> 
>         pm_runtime_disable(&pdev->dev);
> 
>         dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
>         return 0;
>     }
> 
> Eventually also adding:
> 	pm_runtime_suspended(&pdev->dev);

In time: I actually meant:

	pm_runtime_set_suspended(&pdev->dev);

but after double-checking the PM runtime code, it sounds to me that
just calling pm_runtime_disable() would be enough. Not 100% sure
here. Btw, some media drivers call it after pm_runtime_disable(),
while others don't do.

Thanks,
Mauro
Sylwester Nawrocki April 27, 2021, 11:50 a.m. UTC | #4
On 27.04.2021 11:42, Mauro Carvalho Chehab wrote:
> Em Tue, 27 Apr 2021 11:30:55 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> 
>> Em Tue, 27 Apr 2021 10:18:12 +0200
>> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
>>
>>> On 24.04.2021 08:45, Mauro Carvalho Chehab wrote:  
>>>> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
>>>> added pm_runtime_resume_and_get() in order to automatically handle
>>>> dev->power.usage_count decrement on errors.
>>>>
>>>> Use the new API, in order to cleanup the error check logic.
>>>>
>>>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>>>> ---
>>>>  drivers/media/platform/exynos-gsc/gsc-core.c | 3 +--
>>>>  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
>>>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
>>>> index 9f41c2e7097a..9d5841194f6b 100644
>>>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
>>>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
>>>> @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
>>>>  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
>>>>  	int i;
>>>>  
>>>> -	pm_runtime_get_sync(&pdev->dev);
>>>> +	pm_runtime_resume_and_get(&pdev->dev);
>>>>  
>>>>  	gsc_unregister_m2m_device(gsc);
>>>>  	v4l2_device_unregister(&gsc->v4l2_dev);
>>>> @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
>>>>  	for (i = 0; i < gsc->num_clocks; i++)
>>>>  		clk_disable_unprepare(gsc->clock[i]);
>>>>  
>>>> -	pm_runtime_put_noidle(&pdev->dev);    
>>>
>>> If we do this then the device usage count will not get decremented
>>> after the pm_runtime_resume_and_get() call above and after driver
>>> unload/load cycle it will not be possible to suspend the device.
>>> I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
>>> works better in that case.  
>>
>> Good point.
>>
>> Actually, I don't see any reason why to call a PM resume
>> function - either being pm_runtime_get_sync() or
>> pm_runtime_resume_and_get().
>>
>> The code there could simply be:
>>
>>     static int gsc_remove(struct platform_device *pdev)
>>     {
>>         struct gsc_dev *gsc = platform_get_drvdata(pdev);
>>         int i;
>>
>>         gsc_unregister_m2m_device(gsc);
>>         v4l2_device_unregister(&gsc->v4l2_dev);
>>
>>         vb2_dma_contig_clear_max_seg_size(&pdev->dev);
>>         for (i = 0; i < gsc->num_clocks; i++)
>>                 clk_disable_unprepare(gsc->clock[i]);
>>
>>         pm_runtime_disable(&pdev->dev);
>>
>>         dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
>>         return 0;
>>     }
>>
>> Eventually also adding:
>> 	pm_runtime_suspended(&pdev->dev);
> 
> In time: I actually meant:
> 
> 	pm_runtime_set_suspended(&pdev->dev);
> 
> but after double-checking the PM runtime code, it sounds to me that
> just calling pm_runtime_disable() would be enough. Not 100% sure
> here. Btw, some media drivers call it after pm_runtime_disable(),
> while others don't do.

I think if the device is brought into suspended state (e.g. by
disabling clocks as above) the pm_runtime_set_suspended() call
should be there. IOW a following sequence: 

	pm_runtime_disable(dev);
	if (!pm_runtime_status_suspended(dev))
		/* put device into suspended state (disable clocks, 
		  voltage regulators, assert GPIOs, etc. */
	pm_runtime_set_suspended(dev);

--
Regards,
Sylwester
Mauro Carvalho Chehab April 28, 2021, 7:13 a.m. UTC | #5
Em Tue, 27 Apr 2021 13:50:44 +0200
Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:

> On 27.04.2021 11:42, Mauro Carvalho Chehab wrote:
> > Em Tue, 27 Apr 2021 11:30:55 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> >   
> >> Em Tue, 27 Apr 2021 10:18:12 +0200
> >> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
> >>  
> >>> On 24.04.2021 08:45, Mauro Carvalho Chehab wrote:    
> >>>> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> >>>> added pm_runtime_resume_and_get() in order to automatically handle
> >>>> dev->power.usage_count decrement on errors.
> >>>>
> >>>> Use the new API, in order to cleanup the error check logic.
> >>>>
> >>>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> >>>> ---
> >>>>  drivers/media/platform/exynos-gsc/gsc-core.c | 3 +--
> >>>>  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
> >>>>  2 files changed, 2 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> >>>> index 9f41c2e7097a..9d5841194f6b 100644
> >>>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> >>>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> >>>> @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
> >>>>  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
> >>>>  	int i;
> >>>>  
> >>>> -	pm_runtime_get_sync(&pdev->dev);
> >>>> +	pm_runtime_resume_and_get(&pdev->dev);
> >>>>  
> >>>>  	gsc_unregister_m2m_device(gsc);
> >>>>  	v4l2_device_unregister(&gsc->v4l2_dev);
> >>>> @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
> >>>>  	for (i = 0; i < gsc->num_clocks; i++)
> >>>>  		clk_disable_unprepare(gsc->clock[i]);
> >>>>  
> >>>> -	pm_runtime_put_noidle(&pdev->dev);      
> >>>
> >>> If we do this then the device usage count will not get decremented
> >>> after the pm_runtime_resume_and_get() call above and after driver
> >>> unload/load cycle it will not be possible to suspend the device.
> >>> I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
> >>> works better in that case.    
> >>
> >> Good point.
> >>
> >> Actually, I don't see any reason why to call a PM resume
> >> function - either being pm_runtime_get_sync() or
> >> pm_runtime_resume_and_get().
> >>
> >> The code there could simply be:
> >>
> >>     static int gsc_remove(struct platform_device *pdev)
> >>     {
> >>         struct gsc_dev *gsc = platform_get_drvdata(pdev);
> >>         int i;
> >>
> >>         gsc_unregister_m2m_device(gsc);
> >>         v4l2_device_unregister(&gsc->v4l2_dev);
> >>
> >>         vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> >>         for (i = 0; i < gsc->num_clocks; i++)
> >>                 clk_disable_unprepare(gsc->clock[i]);
> >>
> >>         pm_runtime_disable(&pdev->dev);
> >>
> >>         dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
> >>         return 0;
> >>     }
> >>
> >> Eventually also adding:
> >> 	pm_runtime_suspended(&pdev->dev);  
> > 
> > In time: I actually meant:
> > 
> > 	pm_runtime_set_suspended(&pdev->dev);
> > 
> > but after double-checking the PM runtime code, it sounds to me that
> > just calling pm_runtime_disable() would be enough. Not 100% sure
> > here. Btw, some media drivers call it after pm_runtime_disable(),
> > while others don't do.  
> 
> I think if the device is brought into suspended state (e.g. by
> disabling clocks as above) the pm_runtime_set_suspended() call
> should be there. IOW a following sequence: 
> 
> 	pm_runtime_disable(dev);
> 	if (!pm_runtime_status_suspended(dev))
> 		/* put device into suspended state (disable clocks, 
> 		  voltage regulators, assert GPIOs, etc. */
> 	pm_runtime_set_suspended(dev);

Not sure if this would work, as the clock framework would try
to do things like calling clk_pm_runtime_put().

Perhaps an alternative would be to just return an error if it
can't resume PM runtime, e. g.:

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 9f41c2e7097a..d47d02c75484 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1208,9 +1208,11 @@ static int gsc_probe(struct platform_device *pdev)
 static int gsc_remove(struct platform_device *pdev)
 {
        struct gsc_dev *gsc = platform_get_drvdata(pdev);
-       int i;
+       int ret, i;
 
-       pm_runtime_get_sync(&pdev->dev);
+       ret = pm_runtime_resume_and_get(&pdev->dev);
+       if (ret < 0)
+               return ret;
 
        gsc_unregister_m2m_device(gsc);
        v4l2_device_unregister(&gsc->v4l2_dev);


Thanks,
Mauro
Mauro Carvalho Chehab April 28, 2021, 7:17 a.m. UTC | #6
Em Wed, 28 Apr 2021 09:13:02 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Tue, 27 Apr 2021 13:50:44 +0200
> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
> 
> > On 27.04.2021 11:42, Mauro Carvalho Chehab wrote:  
> > > Em Tue, 27 Apr 2021 11:30:55 +0200
> > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> > >     
> > >> Em Tue, 27 Apr 2021 10:18:12 +0200
> > >> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
> > >>    
> > >>> On 24.04.2021 08:45, Mauro Carvalho Chehab wrote:      
> > >>>> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > >>>> added pm_runtime_resume_and_get() in order to automatically handle
> > >>>> dev->power.usage_count decrement on errors.
> > >>>>
> > >>>> Use the new API, in order to cleanup the error check logic.
> > >>>>
> > >>>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > >>>> ---
> > >>>>  drivers/media/platform/exynos-gsc/gsc-core.c | 3 +--
> > >>>>  drivers/media/platform/exynos-gsc/gsc-m2m.c  | 2 +-
> > >>>>  2 files changed, 2 insertions(+), 3 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> > >>>> index 9f41c2e7097a..9d5841194f6b 100644
> > >>>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> > >>>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> > >>>> @@ -1210,7 +1210,7 @@ static int gsc_remove(struct platform_device *pdev)
> > >>>>  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
> > >>>>  	int i;
> > >>>>  
> > >>>> -	pm_runtime_get_sync(&pdev->dev);
> > >>>> +	pm_runtime_resume_and_get(&pdev->dev);
> > >>>>  
> > >>>>  	gsc_unregister_m2m_device(gsc);
> > >>>>  	v4l2_device_unregister(&gsc->v4l2_dev);
> > >>>> @@ -1219,7 +1219,6 @@ static int gsc_remove(struct platform_device *pdev)
> > >>>>  	for (i = 0; i < gsc->num_clocks; i++)
> > >>>>  		clk_disable_unprepare(gsc->clock[i]);
> > >>>>  
> > >>>> -	pm_runtime_put_noidle(&pdev->dev);        
> > >>>
> > >>> If we do this then the device usage count will not get decremented
> > >>> after the pm_runtime_resume_and_get() call above and after driver
> > >>> unload/load cycle it will not be possible to suspend the device.
> > >>> I wouldn't be changing anything in gsc_remove(), pm_runtime_get_sync()
> > >>> works better in that case.      
> > >>
> > >> Good point.
> > >>
> > >> Actually, I don't see any reason why to call a PM resume
> > >> function - either being pm_runtime_get_sync() or
> > >> pm_runtime_resume_and_get().
> > >>
> > >> The code there could simply be:
> > >>
> > >>     static int gsc_remove(struct platform_device *pdev)
> > >>     {
> > >>         struct gsc_dev *gsc = platform_get_drvdata(pdev);
> > >>         int i;
> > >>
> > >>         gsc_unregister_m2m_device(gsc);
> > >>         v4l2_device_unregister(&gsc->v4l2_dev);
> > >>
> > >>         vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> > >>         for (i = 0; i < gsc->num_clocks; i++)
> > >>                 clk_disable_unprepare(gsc->clock[i]);
> > >>
> > >>         pm_runtime_disable(&pdev->dev);
> > >>
> > >>         dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
> > >>         return 0;
> > >>     }
> > >>
> > >> Eventually also adding:
> > >> 	pm_runtime_suspended(&pdev->dev);    
> > > 
> > > In time: I actually meant:
> > > 
> > > 	pm_runtime_set_suspended(&pdev->dev);
> > > 
> > > but after double-checking the PM runtime code, it sounds to me that
> > > just calling pm_runtime_disable() would be enough. Not 100% sure
> > > here. Btw, some media drivers call it after pm_runtime_disable(),
> > > while others don't do.    
> > 
> > I think if the device is brought into suspended state (e.g. by
> > disabling clocks as above) the pm_runtime_set_suspended() call
> > should be there. IOW a following sequence: 
> > 
> > 	pm_runtime_disable(dev);
> > 	if (!pm_runtime_status_suspended(dev))
> > 		/* put device into suspended state (disable clocks, 
> > 		  voltage regulators, assert GPIOs, etc. */
> > 	pm_runtime_set_suspended(dev);  
> 
> Not sure if this would work, as the clock framework would try
> to do things like calling clk_pm_runtime_put().
> 
> Perhaps an alternative would be to just return an error if it
> can't resume PM runtime, e. g.:
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 9f41c2e7097a..d47d02c75484 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -1208,9 +1208,11 @@ static int gsc_probe(struct platform_device *pdev)
>  static int gsc_remove(struct platform_device *pdev)
>  {
>         struct gsc_dev *gsc = platform_get_drvdata(pdev);
> -       int i;
> +       int ret, i;
>  
> -       pm_runtime_get_sync(&pdev->dev);
> +       ret = pm_runtime_resume_and_get(&pdev->dev);
> +       if (ret < 0)
> +               return ret;

Nah, forget about that. Despite the platform driver having a return code,
support for it bogus:


static int platform_remove(struct device *_dev)
{
        struct platform_driver *drv = to_platform_driver(_dev->driver);
        struct platform_device *dev = to_platform_device(_dev);

        if (drv->remove) {
                int ret = drv->remove(dev);

                if (ret)
                        dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
        }
        dev_pm_domain_detach(_dev, true);

        return 0;
}

Thanks,
Mauro
Sylwester Nawrocki April 28, 2021, 8:27 a.m. UTC | #7
On 28.04.2021 09:17, Mauro Carvalho Chehab wrote:
> Em Wed, 28 Apr 2021 09:13:02 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> 
>> Em Tue, 27 Apr 2021 13:50:44 +0200
>> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
>>
>>> On 27.04.2021 11:42, Mauro Carvalho Chehab wrote:

>>> I think if the device is brought into suspended state (e.g. by
>>> disabling clocks as above) the pm_runtime_set_suspended() call
>>> should be there. IOW a following sequence:
>>>
>>> 	pm_runtime_disable(dev);
>>> 	if (!pm_runtime_status_suspended(dev))
>>> 		/* put device into suspended state (disable clocks,
>>> 		  voltage regulators, assert GPIOs, etc. */
>>> 	pm_runtime_set_suspended(dev);
>>
>> Not sure if this would work, as the clock framework would try
>> to do things like calling clk_pm_runtime_put().

It's done in multiple drivers this way. clk_pm_runtime_put() operates
on different device - the clock supplier, not the consumer device.
We just need to disable runtime PM for GSC as the last step, to avoid
any possible v4l2 m2m device_run() call with runtime PM disabled.

>> Perhaps an alternative would be to just return an error if it
>> can't resume PM runtime, e. g.:
[...]
> Nah, forget about that. Despite the platform driver having a return code,
> support for it bogus:

Yes, we can't really stop remove() from driver level so as much complete
resource release is being done as possible.


Regards,
Sylwester

Patch
diff mbox series

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 9f41c2e7097a..9d5841194f6b 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1210,7 +1210,7 @@  static int gsc_remove(struct platform_device *pdev)
 	struct gsc_dev *gsc = platform_get_drvdata(pdev);
 	int i;
 
-	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_resume_and_get(&pdev->dev);
 
 	gsc_unregister_m2m_device(gsc);
 	v4l2_device_unregister(&gsc->v4l2_dev);
@@ -1219,7 +1219,6 @@  static int gsc_remove(struct platform_device *pdev)
 	for (i = 0; i < gsc->num_clocks; i++)
 		clk_disable_unprepare(gsc->clock[i]);
 
-	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index 27a3c92c73bc..09551e96ac15 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -58,7 +58,7 @@  static int gsc_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
 	struct gsc_ctx *ctx = q->drv_priv;
 	int ret;
 
-	ret = pm_runtime_get_sync(&ctx->gsc_dev->pdev->dev);
+	ret = pm_runtime_resume_and_get(&ctx->gsc_dev->pdev->dev);
 	return ret > 0 ? 0 : ret;
 }