drm/panfrost: Add missing OPP table refcnt decremental

[PATCH] drm/panfrost: Add missing OPP table refcnt decremental

Posted by Adrián Larumbe 1 month, 3 weeks ago

Commit f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
retrieves the OPP for the maximum device clock frequency, but forgets to
keep the reference count balanced by putting the returned OPP object. This
eventually leads to an OPP core warning when removing the device.

Fix it by putting OPP objects as many times as they're retrieved.
Also remove an unnecessary whitespace.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Fixes: f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 2d30da38c2c3..c7d3f980f1e5 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -38,7 +38,7 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 		return PTR_ERR(opp);
 	dev_pm_opp_put(opp);
 
-	err =  dev_pm_opp_set_rate(dev, *freq);
+	err = dev_pm_opp_set_rate(dev, *freq);
 	if (!err)
 		ptdev->pfdevfreq.current_frequency = *freq;
 
@@ -177,6 +177,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 	 */
 	pfdevfreq->current_frequency = cur_freq;
 
+	dev_pm_opp_put(opp);
+
 	/*
 	 * Set the recommend OPP this will enable and configure the regulator
 	 * if any and will avoid a switch off by regulator_late_cleanup()
-- 
2.46.2

Re: [PATCH] drm/panfrost: Add missing OPP table refcnt decremental

Posted by Boris Brezillon 1 month, 3 weeks ago

On Thu,  3 Oct 2024 01:25:37 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Commit f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
> retrieves the OPP for the maximum device clock frequency, but forgets to
> keep the reference count balanced by putting the returned OPP object. This
> eventually leads to an OPP core warning when removing the device.

BTW, we do have opp leaks in the error paths of panthor_devfreq_init()
too.

> 
> Fix it by putting OPP objects as many times as they're retrieved.
> Also remove an unnecessary whitespace.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Fixes: f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
> ---
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 2d30da38c2c3..c7d3f980f1e5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -38,7 +38,7 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
>  		return PTR_ERR(opp);
>  	dev_pm_opp_put(opp);
>  
> -	err =  dev_pm_opp_set_rate(dev, *freq);
> +	err = dev_pm_opp_set_rate(dev, *freq);
>  	if (!err)
>  		ptdev->pfdevfreq.current_frequency = *freq;
>  
> @@ -177,6 +177,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>  	 */
>  	pfdevfreq->current_frequency = cur_freq;
>  
> +	dev_pm_opp_put(opp);
> +
>  	/*
>  	 * Set the recommend OPP this will enable and configure the regulator
>  	 * if any and will avoid a switch off by regulator_late_cleanup()

Re: [PATCH] drm/panfrost: Add missing OPP table refcnt decremental

Posted by Boris Brezillon 1 month, 3 weeks ago

On Thu,  3 Oct 2024 01:25:37 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Commit f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
> retrieves the OPP for the maximum device clock frequency, but forgets to
> keep the reference count balanced by putting the returned OPP object. This
> eventually leads to an OPP core warning when removing the device.
> 
> Fix it by putting OPP objects as many times as they're retrieved.
> Also remove an unnecessary whitespace.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Fixes: f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")

Reviewed-by: 

> ---
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 2d30da38c2c3..c7d3f980f1e5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -38,7 +38,7 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
>  		return PTR_ERR(opp);
>  	dev_pm_opp_put(opp);
>  
> -	err =  dev_pm_opp_set_rate(dev, *freq);
> +	err = dev_pm_opp_set_rate(dev, *freq);
>  	if (!err)
>  		ptdev->pfdevfreq.current_frequency = *freq;
>  
> @@ -177,6 +177,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>  	 */
>  	pfdevfreq->current_frequency = cur_freq;
>  
> +	dev_pm_opp_put(opp);
> +

Shouldn't this be moved after the dev_pm_opp_set_opp() that's
following?

>  	/*
>  	 * Set the recommend OPP this will enable and configure the regulator
>  	 * if any and will avoid a switch off by regulator_late_cleanup()

Re: [PATCH] drm/panfrost: Add missing OPP table refcnt decremental

Posted by Adrián Larumbe 1 month, 3 weeks ago

On 03.10.2024 09:17, Boris Brezillon wrote:
> On Thu,  3 Oct 2024 01:25:37 +0100
> Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> 
> > Commit f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
> > retrieves the OPP for the maximum device clock frequency, but forgets to
> > keep the reference count balanced by putting the returned OPP object. This
> > eventually leads to an OPP core warning when removing the device.
> > 
> > Fix it by putting OPP objects as many times as they're retrieved.
> > Also remove an unnecessary whitespace.
> > 
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > Fixes: f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
> 
> Reviewed-by: 
> 
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > index 2d30da38c2c3..c7d3f980f1e5 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > @@ -38,7 +38,7 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
> >  		return PTR_ERR(opp);
> >  	dev_pm_opp_put(opp);
> >  
> > -	err =  dev_pm_opp_set_rate(dev, *freq);
> > +	err = dev_pm_opp_set_rate(dev, *freq);
> >  	if (!err)
> >  		ptdev->pfdevfreq.current_frequency = *freq;
> >  
> > @@ -177,6 +177,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> >  	 */
> >  	pfdevfreq->current_frequency = cur_freq;
> >  
> > +	dev_pm_opp_put(opp);
> > +
> 
> Shouldn't this be moved after the dev_pm_opp_set_opp() that's
> following?

Yes, right now it's in the wrong place, thanks for catching this.

> >  	/*
> >  	 * Set the recommend OPP this will enable and configure the regulator
> >  	 * if any and will avoid a switch off by regulator_late_cleanup()


Adrian Larumbe

Re: [PATCH] drm/panfrost: Add missing OPP table refcnt decremental

Posted by Steven Price 1 month, 3 weeks ago

On 03/10/2024 08:17, Boris Brezillon wrote:
> On Thu,  3 Oct 2024 01:25:37 +0100
> Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> 
>> Commit f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
>> retrieves the OPP for the maximum device clock frequency, but forgets to
>> keep the reference count balanced by putting the returned OPP object. This
>> eventually leads to an OPP core warning when removing the device.
>>
>> Fix it by putting OPP objects as many times as they're retrieved.
>> Also remove an unnecessary whitespace.
>>
>> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
>> Fixes: f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
> 
> Reviewed-by: 

I assume that tag shouldn't be there ;)

>> ---
>>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
>> index 2d30da38c2c3..c7d3f980f1e5 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
>> @@ -38,7 +38,7 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
>>  		return PTR_ERR(opp);
>>  	dev_pm_opp_put(opp);
>>  
>> -	err =  dev_pm_opp_set_rate(dev, *freq);
>> +	err = dev_pm_opp_set_rate(dev, *freq);
>>  	if (!err)
>>  		ptdev->pfdevfreq.current_frequency = *freq;
>>  
>> @@ -177,6 +177,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>>  	 */
>>  	pfdevfreq->current_frequency = cur_freq;
>>  
>> +	dev_pm_opp_put(opp);
>> +
> 
> Shouldn't this be moved after the dev_pm_opp_set_opp() that's
> following?

I agree.

I'm not sure what the devfreq maintainers would think, but there's now a
few drivers that basically want find_available_max_freq() exported - if
you're interested in a wider cleanup then it might be worth looking at.

Steve

>>  	/*
>>  	 * Set the recommend OPP this will enable and configure the regulator
>>  	 * if any and will avoid a switch off by regulator_late_cleanup()
>