Re: [PATCH v3 2/8] drm/panfrost: Enable cycle counter register upon job submission

[Adrián Larumbe]

On 06.09.2023 09:21, Boris Brezillon wrote:
>On Tue,  5 Sep 2023 19:45:18 +0100
>Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx> wrote:
>
>> In a future development, we will want to keep track of the number of GPU
>> cycles spent on a given job. That means we should enable it only when the
>> GPU has work to do, and switch it off whenever it is idle to avoid power
>> waste.
>> 
>> To avoid race conditions during enablement/disabling, a reference counting
>> mechanism was introduced, and a job flag that tells us whether a given job
>> increased the refcount. This is necessary, because a future development
>> will let user space toggle cycle counting through a debugfs file, and a
>> given job might have been in flight by the time cycle counting was
>> disabled.
>> 
>> Toggling of GPU cycle counting has to be done through a module parameter.
>> 
>> Signed-off-by: Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/panfrost/panfrost_device.c |  5 +++
>>  drivers/gpu/drm/panfrost/panfrost_device.h |  6 +++
>>  drivers/gpu/drm/panfrost/panfrost_gpu.c    | 43 ++++++++++++++++++++++
>>  drivers/gpu/drm/panfrost/panfrost_gpu.h    |  6 +++
>>  drivers/gpu/drm/panfrost/panfrost_job.c    | 10 +++++
>>  drivers/gpu/drm/panfrost/panfrost_job.h    |  1 +
>>  6 files changed, 71 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
>> index fa1a086a862b..1ea2ac3804f0 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
>> @@ -18,6 +18,9 @@
>>  #include "panfrost_mmu.h"
>>  #include "panfrost_perfcnt.h"
>>  
>> +static bool profile;
>> +module_param(profile, bool, 0600);
>
>Not sure if we should make that a module parameter. Might be better
>exposed as a debugfs knob attached to the device (even if having
>multiple Mali devices is rather unlikely, I think I'd prefer to make
>this toggle per-device).
>
>> +
>>  static int panfrost_reset_init(struct panfrost_device *pfdev)
>>  {
>>  	pfdev->rstc = devm_reset_control_array_get_optional_exclusive(pfdev->dev);
>> @@ -207,6 +210,8 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>>  
>>  	spin_lock_init(&pfdev->as_lock);
>>  
>> +	atomic_set(&pfdev->profile_mode, profile);
>
>So, profile_mode can only be set at probe time, meaning any changes to
>the profile module param is not taken into account after that point.

Not in this patch in the series, that's why I thought of enabling debugfs
toggling in a later patch.  But I suppose it's best to coalesce them into a
single one and do away with the module param altogether.

>> +
>>  	err = panfrost_clk_init(pfdev);
>>  	if (err) {
>>  		dev_err(pfdev->dev, "clk init failed %d\n", err);
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
>> index b0126b9fbadc..5c09c9f3ae08 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
>> @@ -107,6 +107,7 @@ struct panfrost_device {
>>  	struct list_head scheduled_jobs;
>>  
>>  	struct panfrost_perfcnt *perfcnt;
>> +	atomic_t profile_mode;
>>  
>>  	struct mutex sched_lock;
>>  
>> @@ -121,6 +122,11 @@ struct panfrost_device {
>>  	struct shrinker shrinker;
>>  
>>  	struct panfrost_devfreq pfdevfreq;
>> +
>> +	struct {
>> +		atomic_t use_count;
>> +		spinlock_t lock;
>> +	} cycle_counter;
>>  };
>>  
>>  struct panfrost_mmu {
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> index 2faa344d89ee..fddbc72bf093 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> @@ -73,6 +73,8 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev)
>>  	gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_MASK_ALL);
>>  	gpu_write(pfdev, GPU_INT_MASK, GPU_IRQ_MASK_ALL);
>>  
>> +	atomic_set(&pfdev->cycle_counter.use_count, 0);
>
>I think I'd prefer if the jobs that were in-flight at the time a GPU
>hang occurred explicitly release their reference on use_count. So maybe
>something like
>
>	/* When we reset the GPU we should have no cycle-counter users
>	 * left.
>	 */
>	if (drm_WARN_ON(cycle_counter.use_count != 0))
>		atomic_set(&pfdev->cycle_counter.use_count, 0);
>
>to catch unbalanced get/put situations.
>
>> +
>>  	return 0;
>>  }
>>  
>> @@ -321,6 +323,46 @@ static void panfrost_gpu_init_features(struct panfrost_device *pfdev)
>>  		 pfdev->features.shader_present, pfdev->features.l2_present);
>>  }
>>  
>> +void panfrost_cycle_counter_get(struct panfrost_device *pfdev)
>> +{
>> +	if (atomic_inc_not_zero(&pfdev->cycle_counter.use_count))
>> +		return;
>> +
>> +	spin_lock(&pfdev->cycle_counter.lock);
>> +	if (atomic_inc_return(&pfdev->cycle_counter.use_count) == 1)
>> +		gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_START);
>> +	spin_unlock(&pfdev->cycle_counter.lock);
>> +}
>> +
>> +void panfrost_cycle_counter_put(struct panfrost_device *pfdev)
>> +{
>> +	if (atomic_add_unless(&pfdev->cycle_counter.use_count, -1, 1))
>> +		return;
>> +
>> +	spin_lock(&pfdev->cycle_counter.lock);
>> +	if (atomic_dec_return(&pfdev->cycle_counter.use_count) == 0)
>> +		gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_STOP);
>> +	spin_unlock(&pfdev->cycle_counter.lock);
>> +}
>> +
>> +void panfrost_cycle_counter_stop(struct panfrost_device *pfdev)
>> +{
>> +	atomic_set(&pfdev->profile_mode, 0);
>> +	gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_STOP);
>
>Why do we need to issue a STOP here. Setting profile_mode to false
>should be enough to prevent future jobs from enabling the
>cycle-counter, and the counter will be naturally disabled when all
>in-flight jobs that had profiling enabled are done.
>
>Actually I'm not even sure I understand why this function exists.

I thought it might be good to stop the cycle counter at once even if there
were still in-flight jobs, but now that you mention this perhaps it would
run the risk of disabling it even in the case of a broken refcount.

>> +}
>> +
>> +unsigned long long panfrost_cycle_counter_read(struct panfrost_device *pfdev)
>> +{
>> +	U32 hi, lo;
>> +
>> +	do {
>> +		hi = gpu_read(pfdev, GPU_CYCLE_COUNT_HI);
>> +		lo = gpu_read(pfdev, GPU_CYCLE_COUNT_LO);
>> +	} while (hi != gpu_read(pfdev, GPU_CYCLE_COUNT_HI));
>> +
>> +	return ((u64)hi << 32) | lo;
>> +}
>> +
>>  void panfrost_gpu_power_on(struct panfrost_device *pfdev)
>>  {
>>  	int ret;
>> @@ -367,6 +409,7 @@ void panfrost_gpu_power_on(struct panfrost_device *pfdev)
>>  
>>  void panfrost_gpu_power_off(struct panfrost_device *pfdev)
>>  {
>> +	panfrost_cycle_counter_stop(pfdev);
>
>So, you're setting profile_mode = 0 in the suspend path, but AFAICT,
>it's not set back to the module param profile value on resume, which
>means it's disabled on suspend and never re-enabled after that.

Yep, this is wrong, I missed this path altogether.

>Besides, I don't really see a reason to change the pfdev->profile_mode
>value in this path. If we suspend the device, that means we have no
>jobs running, and the use_count refcount should have dropped to zero,
>thus disabling cycle counting.
>
>>  	gpu_write(pfdev, TILER_PWROFF_LO, 0);
>>  	gpu_write(pfdev, SHADER_PWROFF_LO, 0);
>>  	gpu_write(pfdev, L2_PWROFF_LO, 0);
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.h b/drivers/gpu/drm/panfrost/panfrost_gpu.h
>> index 468c51e7e46d..4d62e8901c79 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.h
>> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.h
>> @@ -16,6 +16,12 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev);
>>  void panfrost_gpu_power_on(struct panfrost_device *pfdev);
>>  void panfrost_gpu_power_off(struct panfrost_device *pfdev);
>>  
>> +void panfrost_stop_cycle_counter(struct panfrost_device *pfdev);
>> +void panfrost_cycle_counter_get(struct panfrost_device *pfdev);
>> +void panfrost_cycle_counter_stop(struct panfrost_device *pfdev);
>> +void panfrost_cycle_counter_put(struct panfrost_device *pfdev);
>> +unsigned long long panfrost_cycle_counter_read(struct panfrost_device *pfdev);
>> +
>>  void panfrost_gpu_amlogic_quirk(struct panfrost_device *pfdev);
>>  
>>  #endif
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
>> index 033f5e684707..8b1bf6ac48f8 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
>> @@ -297,6 +297,11 @@ int panfrost_job_push(struct panfrost_job *job)
>>  
>>  	kref_get(&job->refcount); /* put by scheduler job completion */
>>  
>> +	if (atomic_read(&pfdev->profile_mode)) {
>> +		panfrost_cycle_counter_get(pfdev);
>> +		job->is_profiled = true;
>> +	}
>> +
>>  	drm_sched_entity_push_job(&job->base);
>>  
>>  	mutex_unlock(&pfdev->sched_lock);
>> @@ -351,6 +356,9 @@ static void panfrost_job_free(struct drm_sched_job *sched_job)
>>  
>>  	drm_sched_job_cleanup(sched_job);
>>  
>> +	if (job->is_profiled)
>> +		panfrost_cycle_counter_put(job->pfdev);
>> +
>>  	panfrost_job_put(job);
>>  }
>>  
>> @@ -842,6 +850,8 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>>  		}
>>  	}
>>  
>> +	spin_lock_init(&pfdev->cycle_counter.lock);
>> +
>>  	panfrost_job_enable_interrupts(pfdev);
>>  
>>  	return 0;
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
>> index 8becc1ba0eb9..2aa0add35459 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_job.h
>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
>> @@ -32,6 +32,7 @@ struct panfrost_job {
>>  
>>  	/* Fence to be signaled by drm-sched once its done with the job */
>>  	struct dma_fence *render_done_fence;
>> +	bool is_profiled;
>>  };
>>  
>>  int panfrost_job_init(struct panfrost_device *pfdev);