[jc-kynesim]

Adds power management and some controls that Chrome wated when scanning devices that seem like good things to have in any case. All these probably want to go into out next attempt to upstream.

[popcornmix]

Claude:

Review: Add power management to hevc_d, and add a few other controls

Looks good overall. A few points:

1. HEVC clock is now held at max from boot (power off doesn't drop the rate)

hevc_d_hw_power_on() does clk_set_min_rate(dev->clock, dev->max_clock_rate) and hevc_d_hw_power_off() never releases it. Before this PR that only happened after the first stream; now it happens in probe, so every boot leaves the HEVC clock at max even if the decoder is never used.

On rpi-6.18.y the HEVC firmware clock is CLK_IS_CRITICAL + minimize, so clk_disable_unprepare() never reaches the firmware and the rate is decided only by the min-rate request. Suggest:

void hevc_d_hw_power_off(struct hevc_d_dev *dev)
{
	clk_disable_unprepare(dev->clock);
	clk_set_min_rate(dev->clock, 0);
}

(Same applies to the clock "off" claim in the commit message: as things stand runtime suspend saves nothing, so the rate drop is where the saving actually comes from.)

2. S_FMT(OUTPUT) relaxation should still check the CAPTURE queue

hevc_d_s_fmt_vid_out() clears dst_fmt_set and changes dst_fmt colour. With only vb2_is_streaming(vq) checked, this can now happen while CAPTURE buffers are allocated, so the CAPTURE format can change under existing buffers. Hantro (which this is modelled on) does both:

	if (vb2_is_streaming(vq))
		return -EBUSY;

	peer_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
	if (vb2_is_busy(peer_vq))
		return -EBUSY;

OUTPUT buffer size itself is fine since buf_prepare ignores OUTPUT sizes.

3. Minor

• hevc_d_h265_sync() is just a wrapper for irq_sync(); fine, but the comment on hevc_d_suspend() already explains it so the wrapper's comment could go.
• Without CONFIG_PM the clock is left on from probe until remove. Acceptable given Pi configs, just noting it's a behaviour change from the old per-stream enable.
• Level control has no .def (defaults to LEVEL_1). Harmless for a decoder, but .def = V4L2_MPEG_VIDEO_HEVC_LEVEL_5_1 may be clearer.

PM paths (probe set_active/enable, remove, pm_runtime_force_suspend/resume, IRQ enable moved to power-on) look correct.

[6by9]

Do we actually handle odd widths/heights even with chroma subsampling?

Hantro claims _STEPWISE.
https://elixir.bootlin.com/linux/v7.3/source/drivers/media/platform/verisilicon/hantro_v4l2.c#L210
If I read it correctly, it sets the step to the tile macroblock size of 4.
https://elixir.bootlin.com/linux/v7.3/source/drivers/media/platform/verisilicon/imx8m_vpu_hw.c#L210

[jc-kynesim]

I wondered about that and I thought I had copied Hantros block but I'm happy to make it stepwise 4

[6by9]

Drop debug log messages.

[jc-kynesim]

OK

[6by9]

Comment not required.

[jc-kynesim]

OK

[jc-kynesim]

Claude:

Review: Add power management to hevc_d, and add a few other controls

Looks good overall. A few points:

1. HEVC clock is now held at max from boot (power off doesn't drop the rate)

hevc_d_hw_power_on() does clk_set_min_rate(dev->clock, dev->max_clock_rate) and hevc_d_hw_power_off() never releases it. Before this PR that only happened after the first stream; now it happens in probe, so every boot leaves the HEVC clock at max even if the decoder is never used.

On rpi-6.18.y the HEVC firmware clock is CLK_IS_CRITICAL + minimize, so clk_disable_unprepare() never reaches the firmware and the rate is decided only by the min-rate request. Suggest:

void hevc_d_hw_power_off(struct hevc_d_dev *dev)
{
	clk_disable_unprepare(dev->clock);
	clk_set_min_rate(dev->clock, 0);
}

(Same applies to the clock "off" claim in the commit message: as things stand runtime suspend saves nothing, so the rate drop is where the saving actually comes from.)

I'll do this if you want but I thought the outcome of our previous clock discussions was that we didn't want this as the firmware tended to become confused. The current code doesn't do it. Once you make clk on/off work the CLK_IS_CRITCAL should go away and it should all work.

2. S_FMT(OUTPUT) relaxation should still check the CAPTURE queue

hevc_d_s_fmt_vid_out() clears dst_fmt_set and changes dst_fmt colour. With only vb2_is_streaming(vq) checked, this can now happen while CAPTURE buffers are allocated, so the CAPTURE format can change under existing buffers. Hantro (which this is modelled on) does both:

	if (vb2_is_streaming(vq))
		return -EBUSY;

	peer_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
	if (vb2_is_busy(peer_vq))
		return -EBUSY;

OUTPUT buffer size itself is fine since buf_prepare ignores OUTPUT sizes.

Actually more recent Hantro code doesn't do that as it allows for size changes on the fly, The width / height set on OUTPUT are ignored. The values we care about are set in SPS.

3. Minor

• hevc_d_h265_sync() is just a wrapper for irq_sync(); fine, but the comment on hevc_d_suspend() already explains it so the wrapper's comment could go.

OK

• Without CONFIG_PM the clock is left on from probe until remove. Acceptable given Pi configs, just noting it's a behaviour change from the old per-stream enable.

I'll look at this - it seems we have CONFIG_PM set in our current tree - I did check that everything was called in the right order on a Pi4 on a current 6.18.y branch and it was.

• Level control has no .def (defaults to LEVEL_1). Harmless for a decoder, but .def = V4L2_MPEG_VIDEO_HEVC_LEVEL_5_1 may be clearer.

Can do - I just nicked what Hantro did.

PM paths (probe set_active/enable, remove, pm_runtime_force_suspend/resume, IRQ enable moved to power-on) look correct.

Good

[popcornmix]

I'll do this if you want but I thought the outcome of our previous clock discussions was that we didn't want this as the firmware tended to become confused. The current code doesn't do it. Once you make clk on/off work the CLK_IS_CRITCAL should go away and it should all work.

I think the plan is for the drivers to do the right thing - request clocks when needed and release them when no longer needed. I think having both lines is wanted. When you find the existing driver fails due to firmware issues, you use CRITICAL as the sticking plaster (so clocks don't really switch off). If this goes wrong, then let me know.

The hope is when better firmware arrives, removing CRITICAL will be possible and clocks will be at zero (or whatever firmware considers the min frequency) when not required.

[jc-kynesim]

I'm happy to do whatever I'm told is the right answer re. clocks, but I do feel that if I disable a clock it really shouldn't matter what frequency it was previously set to otherwise what does disable even mean?

[jc-kynesim]

Fixed up most of the above comments with the following exceptions:

1. Clocks, see my comments above. But if you are sure that we need disable and set to zero then I'll do that.
2. The comment re if we unset CONFIG_PM then the clock stays on.- actually if we unset CONFIG_PM then I think this is probably the expected behaviour - again I'm willing to be overridden on this.

[popcornmix]

I'm happy to do whatever I'm told is the right answer re. clocks, but I do feel that if I disable a clock it really shouldn't matter what frequency it was previously set to otherwise what does disable even mean?

I think the issue is you have said you want a min freq, of, say 500MHz. You enable then disable.
The clock is zero , but there is still a per client request of 500MHz.
If another client comes along and says, I want min freq of 250MHz and enable, they will pick up your 500MHz.

The clock rate chosen is the max of all clients min_rates.
The clock is on if any client wants it on.

[jc-kynesim]

Fair enough if the min_freq is global rather than bound to my device, but if it is global then if we have multiple devices with differing min_freq requests then no-one can ever set anything for fear of overwriting someone else's value. But if it is bound to the device then it should be ignored when that device is disabled (disabling should IMHO have an implicit min-freq 0).

[popcornmix]

@jc-kynesim I think this is a similar situation in vc4_v3d where it made a difference. e27916f

[jc-kynesim]

Added the clk_set_min_rate. I still feel it shouldn't be wanted, but I accept that it is.

[jc-kynesim]

Added a trivial extra commit to fix bad debug. Also to ping. Is there anything I still need to do here?

[popcornmix]

Claude:

Review of #7720: hevc_d power management and extra controls

The power management (PM) work looks right to me. I have one real concern, about the relaxed S_FMT check. Everything else is minor.

media/hevc_d: Relax restriction on OUTPUT S_FMT if buffers alloced

hevc_d_video.c:408: hevc_d_s_fmt_vid_out() sets ctx->dst_fmt_set = 0. After that, the next G_FMT/queue_setup on CAPTURE recalculates ctx->dst_fmt from the current SPS. Nothing stops this happening while CAPTURE buffers are allocated, or even while CAPTURE is streaming. The decoder then uses the new dst_fmt strides and colmv sizes (hevc_d_h265.c ~345, ~1660) with buffers that were sized for the old format. That can write past the end of the buffers.

Before this change, a busy OUTPUT queue blocked most of these paths. Relaxing the check opens them up. Cedrus handles this by also checking the other queue:

	if (vb2_is_streaming(vq))
		return -EBUSY;

	/* Changing OUTPUT format resets CAPTURE */
	peer_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
				  V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
	if (vb2_is_busy(peer_vq))
		return -EBUSY;

Does Chrome still work with this? As far as I know it frees its CAPTURE buffers before it changes the OUTPUT format.

A related older bug, not new in this PR: get_dst_fmt() can rewrite dst_fmt while CAPTURE is busy when dst_fmt_set == 0, i.e. when the app never set the CAPTURE format.

media/hevc_d: Add runtime and system suspend/resume PM support

Looks good:

• Probe/remove handle both cases, with and without CONFIG_PM.
• Restoring the IRQ enables in hevc_d_hw_power_on() is correct.
• Resetting the min clock rate on power off is an improvement, since the old code never released it.
• hevc_d_h265_sync() only touches software state, so it is safe to call while the hardware is powered off.

Nits:

• hevc_d_hw.c:454-455: the return value of pm_runtime_set_active() is ignored. Consider devm_pm_runtime_enable(). If you keep the explicit pm_runtime_disable() in remove, leave it as is.
• There is no autosuspend, so every seek (stream off, then on) turns the clock off and on again. The old code did the same. Autosuspend with a short delay (~100ms) would avoid the firmware calls on every seek.

media/hevc_d: Add enum_framesizes

• hevc_d_video.c:209: steps of 4 are fine. 8 (the smallest HEVC coding block) would be more accurate. Not important.

media/hevc_d: Add HEVC profile and level controls

• The profile range MAIN..MAIN_10 also includes MAIN_STILL_PICTURE. That is fine because the hardware can decode it.
• The commit says the code is copied from Hantro, but .def = LEVEL_4_1 is not in Hantro (Hantro gives no default). That's harmless; maybe mention it in the commit message.
• There's an unrelated whitespace fix in the same commit. That's fine, and the commit message mentions it.

media/hevc_d: Add vidioc_remove_bufs handling

OK. The aux/colmv entries are tracked by buffer index. A reused index reuses its entry, the same as normal re-queueing. CAPTURE S_FMT is blocked while buffers exist, so a reused index can't change size. (Once the S_FMT OUTPUT issue above is fixed, OUTPUT can't change it either.)

media/hevc_d: Fix debug if coeff bufs cannot be allocated

LGTM.

[jc-kynesim]

My Claude likes it now, yours may have a different opinion!

I've fixed up the get_dst_fmt issues. I'm not going to add a lazy clock off in this patchset - if it becomes an issue we can think about it later.

[popcornmix]

My Claude is less happy:

Re-review of #7720 (head 54843ac)

Clock, PM, framesizes, controls, remove_bufs and the debug fix all look good now. One remaining issue.

media/hevc_d: Relax restriction on OUTPUT S_FMT if buffers alloced

The get_dst_fmt() guard fixes the out-of-date-buffer problem. But dropping ctx->dst_fmt_set = 0 from hevc_d_s_fmt_vid_out() makes a CAPTURE format set by the client stick for the life of the file handle.

The stateless decoder spec (dev-stateless-decoder.rst, Initialization step 1) says:

Changing the OUTPUT format may change the currently set CAPTURE format. The driver will derive a new CAPTURE format from the OUTPUT format being set, including resolution [...]

Dynamic resolution change repeats that sequence. So with this patch:

1. Client sets OUTPUT 1920x1080, SPS, S_FMT(CAPTURE) (e.g. to pick SAND128), and decodes.
2. Stream switches to 3840x2160. Client stops streaming, frees CAPTURE buffers, S_FMT(OUTPUT) 3840x2160, sets the new SPS.
3. G_FMT(CAPTURE) still returns 1920x1080, because dst_fmt_set is still 1.

Clients that reopen the device on a resolution change won't notice. Clients that reuse the file handle get the old size. The size checks in hevc_d_h265.c:1678 stop this from writing out of bounds, but decoding then fails.

The new check in get_dst_fmt() already protects allocated buffers, so resetting the flag is safe whenever CAPTURE is idle:

	ctx->src_fmt = f->fmt.pix_mp;
	if (!vb2_is_busy(v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx)))
		ctx->dst_fmt_set = 0;  /* Setting src invalidates dst */

(Or return -EBUSY when CAPTURE is busy, like Cedrus does.)

The commit message says the linkage "was never required". Please reword it, since the spec does require it.

Nits

• hevc_d.c: the level default is LEVEL_4_1, which Hantro doesn't set, so the commit text "copied from Hantro" isn't quite accurate. This doesn't matter.

[jc-kynesim]

The important word in the spec is MAY. That is not MUST. So what I have is spec compliant. Claudes example also assumes inconsistent behaviour from the client where it sets the format the first time round and doesn't the second.
As it happens in both FFmpeg & gstreamer (I think) the stream is always reopened and the format is always set.
My Claude explicitly rejected your Claudes idea due to oddities where deallocating the CAPTURE buffers can cause their format to change.
I now regret inventing the dst_set flag at all. As stated the spec doesn't require it, but it is handy having CAPTURE set from SPS even if that really isn't in the spec.

[popcornmix]

Claude has backed off a little. I think we are into minor issues though, so happy for this to be merged if your are.

Claude's (reluctant) acceptance:

Fair enough. The next sentence in the spec ("The driver will derive a new CAPTURE format from the OUTPUT format being set") reads more firmly than "may", but if FFmpeg and GStreamer always reopen and always set the CAPTURE format, nobody will hit it. I'm happy to leave it.

On the "format changes when CAPTURE buffers are freed" oddity: I think that already happens with the current code when the client never sets CAPTURE (dst_fmt_set == 0). get_dst_fmt() holds the old format while buffers exist, then jumps to the SPS-derived one once they are freed. Clearing the flag only while CAPTURE is idle wouldn't add a new case. Not a blocker though.

[jc-kynesim]

Probably worth doing the traditional "try it out with Kodi" thing and see if it breaks anything. My fairly simple testing thinks it is OK, but I think your usage is probably a bit different to what I test.

[popcornmix]

Kodi seems to work with this.