[PATCH v6 5/6] media: platform: Add Raspberry Pi HEVC decoder driver

[Maíra Canal]

Hi Dave,

On 3/4/26 11:05, Dave Stevenson wrote:
> From: John Cox <john.cox at raspberrypi.com>
> 
> 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@igalia.com/

> +
> +	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 at free-electrons.com>
> + * Copyright (C) 2018 Paul Kocialkowski <paul.kocialkowski at bootlin.com>
> + * 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