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