[PATCH 0/2] Panfrost devfreq and GEM status fixes

[PATCH 0/2] Panfrost devfreq and GEM status fixes

[Adrián Larumbe]

During recent development of the Mali CSF GPU Panthor driver, a user
noticed that GPU frequency values as reported by fdinfo were
incorrect. This was traced back to incorrect handling of frequency value
updates. The same problem was seen in Panfrost.

Also one should consider GEM objects from a dma-buf import as being
resident in system memory, so that this can be reflected on fdinfo.

Adrián Larumbe (2):
  drm/panfrost: Consider dma-buf imported objects as resident
  drm/panfrost: Fix incorrect updating of current device frequency

 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 17 +++++++++++++++--
 drivers/gpu/drm/panfrost/panfrost_gem.c     |  2 +-
 2 files changed, 16 insertions(+), 3 deletions(-)


base-commit: 38f922a563aac3148ac73e73689805917f034cb5
-- 
2.42.0

[PATCH 1/2] drm/panfrost: Consider dma-buf imported objects as resident

[Adrián Larumbe]

A GEM object constructed from a dma-buf imported sgtable should be regarded
as being memory resident, because the dma-buf API mandates backing storage
to be allocated when attachment succeeds.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Fixes: 9ccdac7aa822 ("drm/panfrost: Add fdinfo support for memory stats")
Reported-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 0cf64456e29a..d47b40b82b0b 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -200,7 +200,7 @@ static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj
 	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
 	enum drm_gem_object_status res = 0;
 
-	if (bo->base.pages)
+	if (bo->base.base.import_attach || bo->base.pages)
 		res |= DRM_GEM_OBJECT_RESIDENT;
 
 	if (bo->base.madv == PANFROST_MADV_DONTNEED)
-- 
2.42.0

Re: [PATCH 1/2] drm/panfrost: Consider dma-buf imported objects as resident

[Steven Price]

On 25/11/2023 20:52, Adrián Larumbe wrote:
> A GEM object constructed from a dma-buf imported sgtable should be regarded
> as being memory resident, because the dma-buf API mandates backing storage
> to be allocated when attachment succeeds.

This obviously can cause a bit of double-counting system wide (both the
exporter and importer could show the memory usage). But I think we're
better off over-counting rather than under-counting.

Reviewed-by: Steven Price <steven.price@arm.com>

> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Fixes: 9ccdac7aa822 ("drm/panfrost: Add fdinfo support for memory stats")
> Reported-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 0cf64456e29a..d47b40b82b0b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -200,7 +200,7 @@ static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj
>  	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
>  	enum drm_gem_object_status res = 0;
>  
> -	if (bo->base.pages)
> +	if (bo->base.base.import_attach || bo->base.pages)
>  		res |= DRM_GEM_OBJECT_RESIDENT;
>  
>  	if (bo->base.madv == PANFROST_MADV_DONTNEED)

[PATCH 2/2] drm/panfrost: Fix incorrect updating of current device frequency

[Adrián Larumbe]

It was noticed when setting the Panfrost's DVFS device to the performant
governor, GPU frequency as reported by fdinfo had dropped to 0 permamently.

There are two separate issues causing this behaviour:
 - Not initialising the device's current_frequency variable to its original
 value during device probe().
 - Updating said variable in Panfrost devfreq's get_dev_status() rather
 than after the new OPP's frequency had been retrieved in target(), which
 meant the old frequency would be assigned instead.

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 | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index f59c82ea8870..2d30da38c2c3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -29,14 +29,20 @@ static void panfrost_devfreq_update_utilization(struct panfrost_devfreq *pfdevfr
 static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 				   u32 flags)
 {
+	struct panfrost_device *ptdev = dev_get_drvdata(dev);
 	struct dev_pm_opp *opp;
+	int err;
 
 	opp = devfreq_recommended_opp(dev, freq, flags);
 	if (IS_ERR(opp))
 		return PTR_ERR(opp);
 	dev_pm_opp_put(opp);
 
-	return dev_pm_opp_set_rate(dev, *freq);
+	err =  dev_pm_opp_set_rate(dev, *freq);
+	if (!err)
+		ptdev->pfdevfreq.current_frequency = *freq;
+
+	return err;
 }
 
 static void panfrost_devfreq_reset(struct panfrost_devfreq *pfdevfreq)
@@ -58,7 +64,6 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
 	spin_lock_irqsave(&pfdevfreq->lock, irqflags);
 
 	panfrost_devfreq_update_utilization(pfdevfreq);
-	pfdevfreq->current_frequency = status->current_frequency;
 
 	status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time,
 						   pfdevfreq->idle_time));
@@ -164,6 +169,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 
 	panfrost_devfreq_profile.initial_freq = cur_freq;
 
+	/*
+	 * We could wait until panfrost_devfreq_target() to set this value, but
+	 * since the simple_ondemand governor works asynchronously, there's a
+	 * chance by the time someone opens the device's fdinfo file, current
+	 * frequency hasn't been updated yet, so let's just do an early set.
+	 */
+	pfdevfreq->current_frequency = cur_freq;
+
 	/*
 	 * Set the recommend OPP this will enable and configure the regulator
 	 * if any and will avoid a switch off by regulator_late_cleanup()
-- 
2.42.0

Re: [PATCH 2/2] drm/panfrost: Fix incorrect updating of current device frequency

[Steven Price]

On 25/11/2023 20:52, Adrián Larumbe wrote:
> It was noticed when setting the Panfrost's DVFS device to the performant
> governor, GPU frequency as reported by fdinfo had dropped to 0 permamently.
> 
> There are two separate issues causing this behaviour:
>  - Not initialising the device's current_frequency variable to its original
>  value during device probe().
>  - Updating said variable in Panfrost devfreq's get_dev_status() rather
>  than after the new OPP's frequency had been retrieved in target(), which
>  meant the old frequency would be assigned instead.

Good catch - I'd not looked at the performance governor. I'd assumed
that one was "too simple to be wrong" ;)

Reviewed-by: Steven Price <steven.price@arm.com>

> 
> 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 | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index f59c82ea8870..2d30da38c2c3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -29,14 +29,20 @@ static void panfrost_devfreq_update_utilization(struct panfrost_devfreq *pfdevfr
>  static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
>  				   u32 flags)
>  {
> +	struct panfrost_device *ptdev = dev_get_drvdata(dev);
>  	struct dev_pm_opp *opp;
> +	int err;
>  
>  	opp = devfreq_recommended_opp(dev, freq, flags);
>  	if (IS_ERR(opp))
>  		return PTR_ERR(opp);
>  	dev_pm_opp_put(opp);
>  
> -	return dev_pm_opp_set_rate(dev, *freq);
> +	err =  dev_pm_opp_set_rate(dev, *freq);
> +	if (!err)
> +		ptdev->pfdevfreq.current_frequency = *freq;
> +
> +	return err;
>  }
>  
>  static void panfrost_devfreq_reset(struct panfrost_devfreq *pfdevfreq)
> @@ -58,7 +64,6 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
>  	spin_lock_irqsave(&pfdevfreq->lock, irqflags);
>  
>  	panfrost_devfreq_update_utilization(pfdevfreq);
> -	pfdevfreq->current_frequency = status->current_frequency;
>  
>  	status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time,
>  						   pfdevfreq->idle_time));
> @@ -164,6 +169,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>  
>  	panfrost_devfreq_profile.initial_freq = cur_freq;
>  
> +	/*
> +	 * We could wait until panfrost_devfreq_target() to set this value, but
> +	 * since the simple_ondemand governor works asynchronously, there's a
> +	 * chance by the time someone opens the device's fdinfo file, current
> +	 * frequency hasn't been updated yet, so let's just do an early set.
> +	 */
> +	pfdevfreq->current_frequency = cur_freq;
> +
>  	/*
>  	 * 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 0/2] Panfrost devfreq and GEM status fixes

[Steven Price]

On 25/11/2023 20:52, Adrián Larumbe wrote:
> During recent development of the Mali CSF GPU Panthor driver, a user
> noticed that GPU frequency values as reported by fdinfo were
> incorrect. This was traced back to incorrect handling of frequency value
> updates. The same problem was seen in Panfrost.
> 
> Also one should consider GEM objects from a dma-buf import as being
> resident in system memory, so that this can be reflected on fdinfo.
> 
> Adrián Larumbe (2):
>   drm/panfrost: Consider dma-buf imported objects as resident
>   drm/panfrost: Fix incorrect updating of current device frequency
> 
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 17 +++++++++++++++--
>  drivers/gpu/drm/panfrost/panfrost_gem.c     |  2 +-
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> 
> base-commit: 38f922a563aac3148ac73e73689805917f034cb5

Pushed to drm-misc-fixes

Thanks,

Steve