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

[John Cox]

Hi

>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.

I'll fix that

>> +}
>> +
>> +/* 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;
>> +}
>> +

I'll fix that

>[...]
>
>> 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.

Good point - I'll remove it

>[...]
>
>> +
>> +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);

Yes that would be correct - I'll fix that

>[...]
>
>> +
>> +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?

I don't believe that is in fact broken. the call to
hevc_d_h265_stop(ctx) should ensure the hardware has stopped so it
should be safe to stop the clocks, the subsequent calls tidy up the
remaining state.

Many thanks

John Cox

>Best regards,
>- Maíra