drm/vc4: Remove request for min clocks when hdmi output is disabled

[popcornmix]

Currently, booting with no hdmi connected has:
pi@pi4:~ $ vcgencmd measure_clock hdmi pixel
frequency(9)=120010256
frequency(29)=74988280

After connecting hdmi we get:
pi@pi4:~ $ vcgencmd measure_clock hdmi pixel
frequency(9)=300005856
frequency(29)=149989744

and that persists after disconnecting hdmi

I can measure this on a power supply as [email protected] (52mW).

We should always remove clk_set_min_rate requests
when we no longer need them.

[6by9]

We do call clk_disable_unprepare for pixel_bvb_clock in vc4_hdmi_encoder_post_crtc_powerdown, and for hsm_clock in vc4_hdmi_runtime_suspend.

I would have expected the clock framework to ignore any min clock request for a disabled consumer. That may be a false expectation, but feels worth checking out.
(I have a slight niggle in my mind that setting up clocks typically sets the rate and then prepares/enables the clock, but that the caller often wants to validate the new rate before enabling. That can't happen if a disabled clock is ignored)

[popcornmix]

I put some logging in, and I don't believe vc4_hdmi_encoder_post_crtc_powerdown is called when unplugging.
I only saw the log for vc4_hdmi_encoder_post_crtc_disable.

[popcornmix]

I put some logging in, and I don't believe vc4_hdmi_encoder_post_crtc_powerdown is called when unplugging. I only saw the log for vc4_hdmi_encoder_post_crtc_disable.

Ah - printk without a \n means I was seeing the first message (disable) but not the second (powerdown).

Calling clk_set_min_rate with 0 just before the corresponding clk_disable_unprepare in powerdown also works.
Removing the clk_set_min_rate calls does not lower the clocks.

So, clk_disable_unprepare doesn't implicitly remove the min rate request.

[popcornmix]

@mripard is it required for clients to call clk_set_min_rate(clk, 0) before calling clk_disable_unprepare() or should the clock framework do that itself?

[mripard]

Yeah, there's no link between a rate boundary and a whether the clock is enabled or not. Both can happen independently. So you need to explicitly call clk_set_min_rate here indeed.

[popcornmix]

It looks like vc4_hvs and vc4_v3d also have clocks that are left with uncleared min clock requests, so added those.

rpivid_video, bcm2835-unicam also use clk_set_min_rate, but appear to cleat the request correctly anyway.

(make this draft until I've done more testing on additional changes)

[6by9]

Typo in the comments of we not longer have a requirement, but otherwise looks good to me.

[popcornmix]

Updated comments, and tested on Pi3, Pi4 and Pi5 (with a range of hdmi hotplug/removes and v3d use).

[pelwell]

Can you explain why the pairing of this line with the following line (which reference hsm_clock and pixel_clock) doesn't match other clk_set_min_rate and clk_disable_unprepare pairs - or at least say that it is expected and correct?

[popcornmix]

Good point. vc4_hdmi->hsm_clock and vc4_hdmi->pixel_bvb_clock are the clocks that previously were used with clk_set_min_rate, so the lines added are as intended.

But, when I moved them from disable to destroy I assumed they lined up with the clocks being disabled, but as you say that is not quite matching. Let me have a closer look.

[pelwell]

Thanks - I can believe that does what you want.

[popcornmix]

I've moved the clearing of hsm_clock to vc4_hdmi_runtime_suspend where its clock was unprepared.
Still seems to work the same.