| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230906092140.3993d40a@collabora.com>
Date: Wed, 6 Sep 2023 09:21:40 +0200
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Adrián Larumbe <adrian.larumbe@...labora.com>
Cc: maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
tzimmermann@...e.de, airlied@...il.com, daniel@...ll.ch,
robdclark@...il.com, quic_abhinavk@...cinc.com,
dmitry.baryshkov@...aro.org, sean@...rly.run,
marijn.suijten@...ainline.org, robh@...nel.org,
steven.price@....com, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
healych@...zon.com, kernel@...labora.com,
freedreno@...ts.freedesktop.org
Subject: Re: [PATCH v3 2/8] drm/panfrost: Enable cycle counter register upon
job submission
On Tue, 5 Sep 2023 19:45:18 +0100
Adrián Larumbe <adrian.larumbe@...labora.com> 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@...labora.com>
> ---
> 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.
> +
> 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.
> +}
> +
> +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.
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);
Powered by blists - more mailing lists