Re: [PATCH v6 5/6] media: platform: Add Raspberry Pi HEVC decoder driver
From: Maíra Canal
Date: Mon Mar 09 2026 - 09:00:08 EST
Hi Dave,
On 3/4/26 11:05, Dave Stevenson wrote:
From: John Cox <john.cox@xxxxxxxxxxxxxxx>
The BCM2711 and BCM2712 SoCs used on Rapsberry Pi 4 and Raspberry
s/Rapsberry/Raspberry
diff --git a/drivers/media/platform/raspberrypi/hevc_dec/Kconfig b/drivers/media/platform/raspberrypi/hevc_dec/Kconfig
new file mode 100644
index 000000000000..ae1fd079e5c9
--- /dev/null
+++ b/drivers/media/platform/raspberrypi/hevc_dec/Kconfig
@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: GPL-2.0
+
+config VIDEO_RPI_HEVC_DEC
+ tristate "Rasperry Pi HEVC decoder"
s/Rapsberry/Raspberry
+ depends on VIDEO_DEV && VIDEO_DEV
VIDEO_DEV is listed twice.
[...]
+
+/*
+ * Stop the clock for this context
+ * clk_disable_unprepare does ref counting so this will not actually
+ * disable the clock if there are other running contexts
+ */
+void hevc_d_hw_stop_clock(struct hevc_d_dev *dev)
I believe it would be more idiomatic if you use runtime PM to handle
this stop_clock()/start_clock() semantics.
+{
+ clk_disable_unprepare(dev->clock);
In the case that the clock is actually disabled (no other running
contexts), I believe the IRQs should be also disabled before disabling
the clock.
+}
+
+/* Always starts the clock if it isn't already on this ctx */
+int hevc_d_hw_start_clock(struct hevc_d_dev *dev)
+{
+ int rv;
+
+ rv = clk_set_min_rate(dev->clock, dev->max_clock_rate);
+ if (rv) {
+ dev_err(dev->dev, "Failed to set clock rate\n");
+ return rv;
+ }
After I land [1], you will be able to drop this call and just add
`maximize = true` to the HEVC clock.
[1] https://lore.kernel.org/dri-devel/20260218-v3d-power-management-v6-1-40683fd39865@xxxxxxxxxx/
+
+ rv = clk_prepare_enable(dev->clock);
+ if (rv) {
+ dev_err(dev->dev, "Failed to enable clock\n");
+ return rv;
+ }
Considering that the clock was disabled, I believe you should re-enable
IRQs and reset any pending interrupts here, just like you do in
hw_setup().
+ return 0;
+}
+
[...]
diff --git a/drivers/media/platform/raspberrypi/hevc_dec/hevc_d_video.c b/drivers/media/platform/raspberrypi/hevc_dec/hevc_d_video.c
new file mode 100644
index 000000000000..d39a2e228595
--- /dev/null
+++ b/drivers/media/platform/raspberrypi/hevc_dec/hevc_d_video.c
@@ -0,0 +1,634 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Raspberry Pi HEVC driver
+ *
+ * Copyright (C) 2026 Raspberry Pi Ltd
+ *
+ * Based on the Cedrus VPU driver, that is:
+ *
+ * Copyright (C) 2016 Florent Revest <florent.revest@xxxxxxxxxxxxxxxxxx>
+ * Copyright (C) 2018 Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
+ * Copyright (C) 2018 Bootlin
+ */
+
+#include <media/videobuf2-dma-contig.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-ioctl.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-mem2mem.h>
+
+#include "hevc_d.h"
+#include "hevc_d_h265.h"
+#include "hevc_d_hw.h"
+#include "hevc_d_video.h"
+
+static inline struct hevc_d_ctx *hevc_d_file2ctx(struct file *file)
+{
+ return container_of(file->private_data, struct hevc_d_ctx, fh);
+}
+
+/* constrain x to y,y*2 */
+static inline unsigned int constrain2x(unsigned int x, unsigned int y)
+{
+ return (x < y) ?
+ y :
+ (x > y * 2) ? y : x;
+}
constrain2x() doesn't seem to be used anywhere in the driver.
[...]
+
+void hevc_d_prepare_src_format(struct v4l2_pix_format_mplane *pix_fmt)
+{
+ size_t size;
+ u32 w;
+ u32 h;
+
+ w = pix_fmt->width;
+ h = pix_fmt->height;
+ if (!w || !h) {
+ w = HEVC_D_DEFAULT_WIDTH;
+ h = HEVC_D_DEFAULT_HEIGHT;
+ }
+ if (w > HEVC_D_MAX_WIDTH)
+ w = HEVC_D_MAX_WIDTH;
+ if (h > HEVC_D_MAX_HEIGHT)
+ h = HEVC_D_MAX_HEIGHT;
+
+ if (!pix_fmt->plane_fmt[0].sizeimage ||
+ pix_fmt->plane_fmt[0].sizeimage > SZ_32M) {
+ /* Unspecified or way too big - pick max for size */
+ size = hevc_d_bit_buf_size(w, h, 2);
+ }
+ /* Set a minimum */
+ size = max_t(u32, SZ_4K, pix_fmt->plane_fmt[0].sizeimage);
The size computed by hevc_d_bit_buf_size() inside the if-block is
immediately overwritten here unconditionally.
Should the else case be explicit? Something like:
if (!pix_fmt->plane_fmt[0].sizeimage ||
pix_fmt->plane_fmt[0].sizeimage > SZ_32M) {
size = hevc_d_bit_buf_size(w, h, 2);
} else {
size = pix_fmt->plane_fmt[0].sizeimage;
}
size = max_t(u32, SZ_4K, size);
[...]
+
+static int hevc_d_start_streaming(struct vb2_queue *vq, unsigned int count)
+{
+ struct hevc_d_ctx *ctx = vb2_get_drv_priv(vq);
+ struct hevc_d_dev *dev = ctx->dev;
+ int ret = 0;
+
+ v4l2_m2m_update_start_streaming_state(ctx->fh.m2m_ctx, vq);
+
+ if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
+ ret = hevc_d_hw_start_clock(dev);
+ if (ret)
+ goto fail_cleanup;
+
+ ret = hevc_d_h265_start(ctx);
+ if (ret)
+ goto fail_stop_clock;
+ }
+
+ return 0;
+
+fail_stop_clock:
+ hevc_d_hw_stop_clock(dev);
+fail_cleanup:
+ v4l2_err(&dev->v4l2_dev, "%s: qtype=%d: FAIL\n", __func__, vq->type);
+ hevc_d_queue_cleanup(vq, VB2_BUF_STATE_QUEUED);
+ return ret;
+}
+
+static void hevc_d_stop_streaming(struct vb2_queue *vq)
+{
+ struct hevc_d_ctx *ctx = vb2_get_drv_priv(vq);
+ struct hevc_d_dev *dev = ctx->dev;
+
+ if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
+ hevc_d_h265_stop(ctx);
+ hevc_d_hw_stop_clock(dev);
+ }
+
+ hevc_d_queue_cleanup(vq, VB2_BUF_STATE_ERROR);
+
+ vb2_wait_for_all_buffers(vq);
+
+ v4l2_m2m_update_stop_streaming_state(ctx->fh.m2m_ctx, vq);
The order here looks a bit odd to me. Shouldn't we stop the clock after
we stop the streaming state and wait for all buffers?
Best regards,
- Maíra