BCM2711 KMS YUV Support #4754
BCM2711 KMS YUV Support #4754
Conversation
|
Before this PR: after this PR: Something has gone wrong with the fps timing. |
|
On LibreELEC modetest is behaving more sensibly but: So we are getting 4kp40 when requesting 12-bit 4kp60, so there is an issue with clocks. |
|
I think we need this logic for clocks: linux/drivers/gpu/drm/vc4/vc4_hdmi.c Lines 1541 to 1550 in a64ced7 |
|
As discussed on Slack there are some more things we need to concern:
|
drivers/gpu/drm/vc4/vc4_hdmi.c
Outdated
| @@ -1065,6 +1067,12 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, | |||
| break; | |||
| } | |||
|
|
|||
| // YCC422 is always 36-bit and not considered deep colour so doesn't signal in GCP | |||
| if (vc4_state->output_format == DRM_COLOR_FORMAT_YCRCB422) { | |||
There was a problem hiding this comment.
Should this be VC4_HDMI_OUTPUT_YUV422?
drivers/gpu/drm/vc4/vc4_hdmi.c
Outdated
| { | ||
| unsigned long clock = mode->crtc_clock * 1000; | ||
|
|
||
| if (mode->flags & DRM_MODE_FLAG_DBLCLK) | ||
| clock = clock * 2; | ||
|
|
||
| if (fmt == VC4_HDMI_OUTPUT_YUV422) | ||
| clock = clock * 2 / 3; |
There was a problem hiding this comment.
I think you want bcp=8 instead. The clock rate does not change in YCC422 modes. The bpc does not affect clock rate.
|
With the two fixes I suggested, all report 60fps (at least from a LibreELEC build). Without the changes the framerate lowers as bpc increases. |
popcornmix
force-pushed
the
rpi-5.15.y
branch
from
6a40ed4 to
6517064
Compare
|
Testing more with modetest: 1080p24/1080p25/1080p30/1080p50/1080p60 at 8bit or 10bit or 12bit working |
|
modetest with 4kp30 reports: which is surprising as I'd expect 4kp30 to be 297MHz for 8bit (and 297MHz * 1.5 for RGB) and so RGB should pass. mode->clock is 594MHz (e.g. 4kp60) rather than 297MHz I am expecting. Any idea why? |
|
I've reworked the current code and get a sane clock now for 4k at 30Hz, however, it won't enable the scrambler since we only check on the timings clock and don't take the bpc and format into account. I'll work on that. |
|
Sounds good. That was going to be my next question. |
mripard
force-pushed
the
rpi/hdmi-yuv-simpler
branch
from
26ff77f to
b56ce77
Compare
|
I've just pushed a new version that should address most of the issues raised so far. The one thing I haven't been able to fix yet is that, even though my monitor reports that it supports YUV422, it looks like the drm_display_info structure doesn't have the matching bit set and thus we always ignore it. |
|
I get the same issue results in: But the display does support this (and was outputting it with your previous version of this PR). Also the update means we default to 12bpc by default (previous version was 8bpc). It also means, for, say 1080p60 we will output 12bpc and have 50% higher clocks, which could result in complaints from users with less than ideal HDMI cables. I was hoping that we would default to 8bpc, and only switch to 12bpc if the user application sets the "max bpc" to 12. That can only be done when required - e.g. with a 10bpc (P030) video playing. |
|
Just spotted another issue: vc4_hdmi_encoder_clock_valid (or some other code parts) should check if a HDMI VSDB is present in EDID and if yes cap the clock at the TMDS limit - see HDMI 1.4b section 8.3
It might make sense to simply skip this check at 8bpc as I've seen EDIDs with nonsense or no values in there - as we'd do for DVI devices (which usually don't report an HDMI VSDB) |
|
Ah, drm_display_info already has parsed max_tmds_clock, so I'd suggest doing the capping like in the intel driver https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/gpu/drm/i915/display/intel_hdmi.c#L1818-L1820 |
|
Quick heads up, there's a bug in drm_edid.c's deep color parsing function, it incorrectly removes YCRCB422: HDMI 1.3 (section 6.2.4) doesn't state that YCRCB422 isn't allowed at all if a sink supports deep color, only that this is not a valid deep color mode HDMI 1.4 (section 6.2.4) clarifies this further |
|
@HiassofT so the line should be: ? |
mripard
force-pushed
the
rpi/hdmi-yuv-simpler
branch
from
b56ce77 to
88471fd
Compare
|
I've pushed a version fixing the YUV422 issue like @HiassofT suggested, took the max_tmds_clock into account and dropped the part raising the default max_bpc property |
|
@popcornmix yes, that's fine DRM_COLOR_FORMAT_RGB444 will already be set if a CEA extension block is found https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/gpu/drm/drm_edid.c#L5093-L5094 so while it's not strictly necessary to add it again it's more readable and safer if we explicitly enable it here |
|
Functionally this is looking good. I seem to be getting the expected output formats and clocks for a wide range of resolution/bpc/framerate. But LE is starting up with a blank screen: when run without |
|
So the place where the blank screen diverges is: (without hdmi_enable_4kp60=1) |
| if (mode->flags & DRM_MODE_FLAG_DBLCLK) | ||
| clock = clock * 2; | ||
|
|
||
| return clock * bpc / 8; |
There was a problem hiding this comment.
This overflows resulting in scrambling not getting enabled. (e.g. clock=594000000 bpc=8)
Need something like return (u64)clock * bpc / 8
mripard
force-pushed
the
rpi/hdmi-yuv-simpler
branch
from
88471fd to
694813e
Compare
|
Indeed, thanks for the report. I've fixed it by using an unsigned long long everywhere for the clock since it's what we were using in the vc4_hdmi_connector_state to store it anyway. |
drivers/gpu/drm/vc4/vc4_hdmi.c
Outdated
| return MODE_CLOCK_HIGH; | ||
|
|
||
| return MODE_OK; | ||
| return vc4_hdmi_encoder_clock_valid(vc4_hdmi, mode->crtc_clock * 1000); |
There was a problem hiding this comment.
This should be mode->clock not mode->crtc_clock.
It's the cause of blank screen when booting LE at 4kp30.
Logging shows mode->clock is set to plausible value but mode->crtc_clock is often 0 here.
There was a problem hiding this comment.
Indeed, it's filled by drm_mode_set_crtcinfo() which is called right after .mode_valid. I've fixed it.
mripard
force-pushed
the
rpi/hdmi-yuv-simpler
branch
from
694813e to
ad829ab
Compare
popcornmix
force-pushed
the
rpi-5.15.y
branch
from
be49bc5 to
77b27ab
Compare
The current code, when parsing the EDID Deep Color depths, that the YUV422 cannot be used, referring to the HDMI 1.3 Specification. This specification, in its section 6.2.4, indeed states: For each supported Deep Color mode, RGB 4:4:4 shall be supported and optionally YCBCR 4:4:4 may be supported. YCBCR 4:2:2 is not permitted for any Deep Color mode. This indeed can be interpreted like the code does, but the HDMI 1.4 specification further clarifies that statement in its section 6.2.4: For each supported Deep Color mode, RGB 4:4:4 shall be supported and optionally YCBCR 4:4:4 may be supported. YCBCR 4:2:2 is also 36-bit mode but does not require the further use of the Deep Color modes described in section 6.5.2 and 6.5.3. This means that, even though YUV422 can be used with 12 bit per color, it shouldn't be treated as a deep color mode. This deviates from the interpretation of the code and comment, so let's fix those. Fixes: d0c9469 ("drm/edid: Parse and handle HDMI deep color modes.") Signed-off-by: Maxime Ripard <[email protected]>
The drm_hdmi_avi_infoframe_colorspace() function actually sets the colorimetry and extended_colorimetry fields in the hdmi_avi_infoframe structure with DRM_MODE_COLORIMETRY_* values. To make things worse, the hdmi_avi_infoframe structure also has a colorspace field used to signal whether an RGB or YUV output is being used. Let's remove the inconsistency and allow for the colorspace usage by renaming the function. Signed-off-by: Maxime Ripard <[email protected]>
We're going to need to tell whether we want to run with a full or limited range RGB output in multiple places in the code, so let's create a helper that will return whether we need with full range or not. Acked-by: Thomas Zimmermann <[email protected]> Signed-off-by: Maxime Ripard <[email protected]>
The CSC callbacks takes a boolean as an argument to tell whether we're using the full range or limited range RGB. However, with the upcoming YUV support, the logic will be a bit more complex. In order to address this, let's make the callbacks take the entire mode, and call our new helper to tell whether the full or limited range RGB should be used. Acked-by: Thomas Zimmermann <[email protected]> Signed-off-by: Maxime Ripard <[email protected]>
On the BCM2711, the HDMI_VEC_INTERFACE_XBAR register configuration depends on whether we're using an RGB or YUV output. Let's move that configuration to the CSC setup. Acked-by: Thomas Zimmermann <[email protected]> Signed-off-by: Maxime Ripard <[email protected]>
On BCM2711, the HDMI_CSC_CTL register value has been hardcoded to an opaque value. Let's replace it with properly defined values. Acked-by: Thomas Zimmermann <[email protected]> Signed-off-by: Maxime Ripard <[email protected]>
The current CSC setup code for the BCM2711 uses a sequence of register writes to configure the CSC depending on whether we output using a full or limited range. However, with the upcoming introduction of the YUV output, we're going to add new matrices to perform the conversions, so we should switch to something a bit more flexible that takes the matrix as an argument and programs the CSC accordingly. Acked-by: Thomas Zimmermann <[email protected]> Signed-off-by: Maxime Ripard <[email protected]>
In order to support the YUV output, we'll need the atomic state to know what is the state of the associated property in the CSC setup callback. Let's change the prototype of that callback to allow us to access it. Acked-by: Thomas Zimmermann <[email protected]> Signed-off-by: Maxime Ripard <[email protected]>
Our code is doing the same clock rate validation in multiple instances. Let's create a helper to share the rate validation. Signed-off-by: Maxime Ripard <[email protected]>
The code to compute our clock rate for a given setup will be called in multiple places in the next patches, so let's create a separate function for it. Signed-off-by: Maxime Ripard <[email protected]>
In the function that validates that the clock isn't too high, we've only taken our controller limitations into account so far. However, the sink can have a limit on the maximum TMDS clock it can deal with too which is exposed through the EDID and the drm_display_info. Make sure we check it. Signed-off-by: Maxime Ripard <[email protected]>
The current code only base its decision for whether the scrambler must be enabled or not on the pixel clock of the mode, but doesn't take the bits per color into account. Let's leverage the new function to compute the clock rate in the scrambler setup code. Signed-off-by: Maxime Ripard <[email protected]>
Currently we take the max_bpc property as the bpc value and do not try anything else. However, what the other drivers seem to be doing is that they would try with the highest bpc allowed by the max_bpc property and the hardware capabilities, test if it results in an acceptable configuration, and if not decrease the bpc and try again. Let's use the same logic. Signed-off-by: Maxime Ripard <[email protected]>
In addition to the RGB444 output, the BCM2711 HDMI controller supports the YUV444 and YUV422 output formats. Let's add support for them in the driver, but still use RGB as the preferred format. Signed-off-by: Maxime Ripard <[email protected]>
mripard
force-pushed
the
rpi/hdmi-yuv-simpler
branch
from
ad829ab to
daeede0
Compare
|
I'm happy with this after quite a lot of testing. Also tested by @HiassofT. |
|
I get no display with these commits: |
|
PR reverted for now. |
|
I believe that warn is due to the lack of #4770 |
|
|
I think should be as the max_tmds_clock looks to be optional in edid. |
|
I agree - |
|
Reapplied with the fix included. |
Hi,
This is another attempt of supporting YUV output in the HDMI driver following the discussions made on slack recently.
For reference, the other two attempts were #4201 and #4280.
This series tries to mimic what the Intel driver is doing, and will try to use the RGB format first, and if we're above our clock limits will try again using YUV422. If both fails, we'll reject the commit entirely.
This is based on 5.15, let me know if you would prefer a branch based on 5.10. I also plan on sending it today upstream to start the discussion and see if it gets more traction than the other attempts.
Maxime