drm/vc4_hdmi: Force modeset when bpc changes

[popcornmix]

See: https://forum.libreelec.tv/thread/25427-le-10-0-2-on-rpi4-not-playing-files-that-10-0-1-had-no-problems-with/

The issue is that kodi changes hdmi mode to 3840x2160@24 initially with "max bcp=8"
After decoding the first frame it does a modeset to same mode with "max bpc=12".

Now vc4_hdmi_encoder_compute_config chooses vc4_state->output_bpc = 12 with output_format=VC4_HDMI_OUTPUT_RGB
This requires scrambling as clock > 300MHz (and we have hdmi_enable_4kp60=1).

vc4_hdmi_encoder_atomic_mode_set (without this PR's assignment to mode_changed) is currenly not called so we don't assign:
vc4_hdmi->output_bpc = vc4_state->output_bpc

which means vc4_hdmi_enable_scrambling never enables scrambling (as vc4_hdmi->output_bpc is still 8).

But we do set the pixel clock in phy_init() to a clock frequency that requires scrambling.

The inconsistency of changing pixel clock, but not changing related state that is needed to enable scambling seems wrong.
I feel this PR works around the problem, rather than fixing it, so I'd be interested in hearing how it should work.

Signed-off-by: Dom Cobley popcornmix@gmail.com

[popcornmix]

ping @mripard @6by9

[pelwell]

Speaking as somebody less familiar with DRM, it seems wrong if atomic_mode_set is called to change the bpc and it doesn't result in mode_changed being set. Can you say more about why you think this is more like a workaround?

[mripard]

I think we should rather have it in encoder_atomic_check and compare vc4_connector_state->output_bpc.
Otherwise we will end up with a modeset even if we took the same bpc decision in vc4_hdmi_encoder_compute_config

Since it affects the tmds clock, I don't think we can get away with it without asking for a modeset. We'd need to do that for output_fmt too.

[popcornmix]

Updated to move to encoder_atomic_check.

I am seeing an issue (both with this and previous version).
It seems to reliably switch to 12 bpc mode, but I'm getting a blank screen when video stops (we are switching back to 1920x1080 8 bpc mode) some of the time (maybe 25%). Unplug/plug of hdmi gets picture back.

[mripard]

Even though it probably works because nothing modified it so far, we should directly compare with the old connector state here. You can retrieve it using

struct drm_connector_state *old_conn_state = drm_atomic_get_old_connector_state(conn_state->state, connector);
struct vc4_hdmi_connector_state *old_vc4_state = conn_state_to_vc4_hdmi_conn_state(old_conn_state);

[mripard]

And then here you can just compare vc4_state->output_bpc != old_vc4_state->output_bpc.

That way you don't rely on the fact that nothing updates output_bpc in the current state before that code runs, you always consider the old state value.

[popcornmix]

Speaking as somebody less familiar with DRM, it seems wrong if atomic_mode_set is called to change the bpc and it doesn't result in mode_changed being set. Can you say more about why you think this is more like a workaround?

@pelwell I think I was unhappy with vc4_state->pixel_rate being assigned (and used to set clocks in vc4_hdmi_encoder_pre_crtc_configure) unconditionally, and being based on vc4_state->output_format and vc4_state->output_bpc.

But vc4_hdmi->output_format and vc4_hdmi->output_bpc are only assigned conditionally in vc4_hdmi_encoder_atomic_mode_set, I believe only when mode_changed has been set, and they are used to determine if scrambling is enabled.

I believe this PR does ensure that this sequence will no longer occur, but it feels a little fragile. But I can't immediately think of a change that will break it, so I've removed the workaround comment from commit message.

@mripard I've updated to use the old state accessors.

[mripard]

Looks good to me now :)

[popcornmix]

There was a positive report by original bug reporter. https://forum.libreelec.tv/thread/25427-le-10-0-2-on-rpi4-not-playing-files-that-10-0-1-had-no-problems-with/