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

Message ID 9c7d683907b9f9cf4a99f57f978671ec7f5a1dbc.1619191723.git.mchehab+huawei@kernel.org
State New
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.
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. | #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. | #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. | #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. | #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. | #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. | #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. | #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 --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;
 }