| 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: <20250410184637.5e0613d2@collabora.com>
Date: Thu, 10 Apr 2025 18:46:37 +0200
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Karunika Choo <karunika.choo@....com>
Cc: dri-devel@...ts.freedesktop.org, nd@....com, Steven Price
<steven.price@....com>, Liviu Dudau <liviu.dudau@....com>, Maarten
Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
<mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
<airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/panthor: Add 64-bit and poll register accessors
On Thu, 10 Apr 2025 17:35:46 +0100
Karunika Choo <karunika.choo@....com> wrote:
> This patch adds 64-bit register accessors to simplify register access in
> Panthor. It also adds 32-bit and 64-bit variants for read_poll_timeout.
>
> This patch also updates Panthor to use the new 64-bit accessors and poll
> functions.
>
> Signed-off-by: Karunika Choo <karunika.choo@....com>
> ---
> drivers/gpu/drm/panthor/panthor_device.h | 71 ++++++++++++
> drivers/gpu/drm/panthor/panthor_fw.c | 9 +-
> drivers/gpu/drm/panthor/panthor_gpu.c | 142 ++++++-----------------
> drivers/gpu/drm/panthor/panthor_mmu.c | 34 ++----
> drivers/gpu/drm/panthor/panthor_regs.h | 6 -
> 5 files changed, 124 insertions(+), 138 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index da6574021664..5028e25f5e0d 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -428,4 +428,75 @@ static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev, \
>
> extern struct workqueue_struct *panthor_cleanup_wq;
>
> +static inline void gpu_write(struct panthor_device *ptdev, u32 reg, u32 data)
> +{
> + writel(data, ptdev->iomem + reg);
> +}
> +
> +static inline u32 gpu_read(struct panthor_device *ptdev, u32 reg)
> +{
> + return readl(ptdev->iomem + reg);
> +}
> +
> +static inline u32 gpu_read_relaxed(struct panthor_device *ptdev, u32 reg)
> +{
> + return readl_relaxed(ptdev->iomem + reg);
> +}
> +
> +static inline void gpu_write64(struct panthor_device *ptdev, u32 reg, u64 data)
> +{
> + gpu_write(ptdev, reg, lower_32_bits(data));
> + gpu_write(ptdev, reg + 4, upper_32_bits(data));
> +}
> +
> +static inline u64 gpu_read64(struct panthor_device *ptdev, u32 reg)
> +{
> + return (gpu_read(ptdev, reg) | ((u64)gpu_read(ptdev, reg + 4) << 32));
> +}
> +
> +static inline u64 gpu_read64_relaxed(struct panthor_device *ptdev, u32 reg)
> +{
> + return (gpu_read_relaxed(ptdev, reg) |
> + ((u64)gpu_read_relaxed(ptdev, reg + 4) << 32));
> +}
> +
> +static inline u64 gpu_read64_counter(struct panthor_device *ptdev, u32 reg)
> +{
> + u32 lo, hi1, hi2;
> + do {
> + hi1 = gpu_read(ptdev, reg + 4);
> + lo = gpu_read(ptdev, reg);
> + hi2 = gpu_read(ptdev, reg + 4);
> + } while (hi1 != hi2);
> + return lo | ((u64)hi2 << 32);
> +}
> +
> +#define gpu_read_poll_timeout(dev, reg, val, cond, delay_us, timeout_us) \
> + read_poll_timeout(gpu_read, val, cond, delay_us, timeout_us, false, \
> + dev, reg)
nit: can use use tabs to pad till the '\' at the end of the line so we
can have a consistent formatting across these definitions?
> +
> +#define gpu_read_poll_timeout_atomic(dev, reg, val, cond, delay_us, \
> + timeout_us) \
> + read_poll_timeout_atomic(gpu_read, val, cond, delay_us, timeout_us, \
> + false, dev, reg)
> +
> +#define gpu_read64_poll_timeout(dev, reg, val, cond, delay_us, timeout_us) \
> + read_poll_timeout(gpu_read64, val, cond, delay_us, timeout_us, false, \
> + dev, reg)
> +
> +#define gpu_read64_poll_timeout_atomic(dev, reg, val, cond, delay_us, \
> + timeout_us) \
> + read_poll_timeout_atomic(gpu_read64, val, cond, delay_us, timeout_us, \
> + false, dev, reg)
> +
> +#define gpu_read_relaxed_poll_timeout_atomic(dev, reg, val, cond, delay_us, \
> + timeout_us) \
> + read_poll_timeout_atomic(gpu_read_relaxed, val, cond, delay_us, \
> + timeout_us, false, dev, reg)
> +
> +#define gpu_read64_relaxed_poll_timeout(dev, reg, val, cond, delay_us, \
> + timeout_us) \
> + read_poll_timeout(gpu_read64_relaxed, val, cond, delay_us, timeout_us, \
> + false, dev, reg)
> +
> #endif
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 0f52766a3120..ecfbe0456f89 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -1059,8 +1059,8 @@ static void panthor_fw_stop(struct panthor_device *ptdev)
> u32 status;
>
> gpu_write(ptdev, MCU_CONTROL, MCU_CONTROL_DISABLE);
> - if (readl_poll_timeout(ptdev->iomem + MCU_STATUS, status,
> - status == MCU_STATUS_DISABLED, 10, 100000))
> + if (gpu_read_poll_timeout(ptdev, MCU_STATUS, status,
> + status == MCU_STATUS_DISABLED, 10, 100000))
> drm_err(&ptdev->base, "Failed to stop MCU");
> }
>
> @@ -1085,8 +1085,9 @@ void panthor_fw_pre_reset(struct panthor_device *ptdev, bool on_hang)
>
> panthor_fw_update_reqs(glb_iface, req, GLB_HALT, GLB_HALT);
> gpu_write(ptdev, CSF_DOORBELL(CSF_GLB_DOORBELL_ID), 1);
> - if (!readl_poll_timeout(ptdev->iomem + MCU_STATUS, status,
> - status == MCU_STATUS_HALT, 10, 100000)) {
> + if (!gpu_read_poll_timeout(ptdev, MCU_STATUS, status,
> + status == MCU_STATUS_HALT, 10,
> + 100000)) {
> ptdev->reset.fast = true;
> } else {
> drm_warn(&ptdev->base, "Failed to cleanly suspend MCU");
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index 671049020afa..fd09f0928019 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -108,14 +108,9 @@ static void panthor_gpu_init_info(struct panthor_device *ptdev)
>
> ptdev->gpu_info.as_present = gpu_read(ptdev, GPU_AS_PRESENT);
>
> - ptdev->gpu_info.shader_present = gpu_read(ptdev, GPU_SHADER_PRESENT_LO);
> - ptdev->gpu_info.shader_present |= (u64)gpu_read(ptdev, GPU_SHADER_PRESENT_HI) << 32;
> -
> - ptdev->gpu_info.tiler_present = gpu_read(ptdev, GPU_TILER_PRESENT_LO);
> - ptdev->gpu_info.tiler_present |= (u64)gpu_read(ptdev, GPU_TILER_PRESENT_HI) << 32;
> -
> - ptdev->gpu_info.l2_present = gpu_read(ptdev, GPU_L2_PRESENT_LO);
> - ptdev->gpu_info.l2_present |= (u64)gpu_read(ptdev, GPU_L2_PRESENT_HI) << 32;
> + ptdev->gpu_info.shader_present = gpu_read64(ptdev, GPU_SHADER_PRESENT_LO);
> + ptdev->gpu_info.tiler_present = gpu_read64(ptdev, GPU_TILER_PRESENT_LO);
> + ptdev->gpu_info.l2_present = gpu_read64(ptdev, GPU_L2_PRESENT_LO);
Now that we have proper 64-bit accessors, I think I would drop the
_LO/_HI definitions and just go a single def per register that replaces
the _LO one.
Powered by blists - more mailing lists