BCM2711 KMS YUV output #4201
BCM2711 KMS YUV output #4201
Conversation
drivers/gpu/drm/vc4/vc4_hdmi.c
Outdated
| @@ -1221,8 +1230,7 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, | |||
| struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); | |||
| struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); | |||
|
|
|||
| if (vc4_encoder->hdmi_monitor && | |||
| drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED) { | |||
| if (vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode) { | |||
There was a problem hiding this comment.
Hasn't this condition been inverted?
The original checked for being limited range, but now we're looking for being full range RGB.
drivers/gpu/drm/vc4/vc4_hdmi.c
Outdated
| vc4_encoder->limited_rgb_range = false; | ||
| } | ||
| if (vc4_hdmi->variant->csc_setup) | ||
| vc4_hdmi->variant->csc_setup(vc4_hdmi, true); |
There was a problem hiding this comment.
csc_setup had just changed to taking the mode, but now we're back to true. Think that should have been mode.
drivers/gpu/drm/vc4/vc4_hdmi.c
Outdated
| { | ||
| u32 csc_ctl; | ||
|
|
||
| csc_ctl = 0x07; /* RGB_CONVERT_MODE = custom matrix, || USE_RGB_TO_YCBCR */ | ||
|
|
||
| if (enable) { | ||
| if (vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode)) { |
There was a problem hiding this comment.
Inverted. CSC is limited range, check is for full range.
| * [ -0.117208 -0.394207 0.511416 128 ] | ||
| * [ 0.511416 -0.464524 -0.046891 128 ] | ||
| * [ 0.212639 0.715169 0.072192 0 ] | ||
| * |
There was a problem hiding this comment.
Oof, does it really swap the channels around. How annoying.
Do we ever have to worry about 601? It is in the HDMI spec, and can be advertised through the C1/C0 colorimetry bit in the AVI infoframe data byte 2. Park for later?
mripard
force-pushed
the
rpi/5.10-hdmi-yuv
branch
from
c5c7717
to
e48421b
Compare
|
Following the discussion with upstream, I reworked that branch to support what the current consensus seems to be.
The bugs reported by @6by9 should also be fixed, and it's based on the 4k PR I just sent, with 4k@60 with 12 bpc and in YUV422 that should work fine (unlike what @popcornmix reported previously). Since it requires the userspace cooperation, we'll still need the work on Kodi to merge this |
|
Thanks. I had a patch for kodi that worked (for lower resolutions) with previous patch. I'll try to get something similar that queries the available output formats. Assuming it works with this PR and gracefully handles the lack of these properties then we should be able to get it merged into Kodi. |
|
We don't really need to have it merged at that point, since it could still change due to the review process. What we need at this point is a statement from Kodi that they are ok with merging it in its current form :) |
|
@mripard (and I've also seen |
|
While gives me a picture on monitor is reports 36bits (good) 20Hz (not so good). So I don't think the clocks are right. I haven't worked out what effect 4:2:2 + 12bit should have on the pixel clock. |
|
Slightly odd one as selecting "max bpc:8" with YCbCr-422 ("output format:2") is making my analyser report 36bpp. GCP packets certainly say that too. Confusingly which seems to select 36bpp gives correct colours (greyscale gradient) which tries to force 36bpp, gives a serious red tinge to everything. My analyser appears not to give frame rate or timing info on the HDMI2.0 interface, but does on the HDMI1.4 one. In that mode it does confirm that I am getting 45fps instead of my requested 60. for max bpc:12. This sounds very familiar to when I was looking at similar things. Now if only I could remember which clock needs to be scaled up to sort the issue! |
mripard
force-pushed
the
rpi/5.10-hdmi-yuv
branch
from
e48421b
to
1c3eb58
Compare
|
@mripard after a long discussion with Broadcom I think popcornmix@735899f is what we want. Basically YCC422 is always 36-bit, and does not require deep colour signalling (from HDMI 1.4 spec). |
In the case where we have a CEA861 extension block in the EDID that sets the YCbCr bits in the byte 3, and a HDMI Vendor-Specific Data Block in that same extension block, DRM will in drm_parse_cea_ext first look at the byte 3, set the struct display_info color_formats field accordingly, and then reset it in drm_parse_hdmi_vsdb_video. The latter will only set RGB444 and YCbCr444 if found though, so if the display was reporting in the byte 3 that it supported YCbCr422, it's essentially cleared and color_formats will not report it anymore. Signed-off-by: Maxime Ripard <maxime@cerno.tech>
HDMI allows for either an RGB444 output which has been supported so far, but also YUV444, YUV422 and YUV420. Let's make the switch between all those variants available through a connector property that defaults to RGB444. Signed-off-by: Maxime Ripard <maxime@cerno.tech>
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 <maxime@cerno.tech>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
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 <tzimmermann@suse.de> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
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 <tzimmermann@suse.de> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
The limited_rgb_range field in the vc4_hdmi_encoder structure is used to tell whether we're supposed to output with a full or limited RGB range. This is redundant with the new helper we introduced, so let's convert to that helper and drop that field. Acked-by: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
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 <tzimmermann@suse.de> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
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 <tzimmermann@suse.de> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
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 <tzimmermann@suse.de> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
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 <tzimmermann@suse.de> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
The HDMI controllers in the BCM2711 support YUV444 and YUV420 outputs, let's add support for it. Signed-off-by: Maxime Ripard <maxime@cerno.tech>
mripard
force-pushed
the
rpi/5.10-hdmi-yuv
branch
from
1c3eb58
to
a64ced7
Compare
|
I just pushed a new version with your commit merged (thanks for figuring it out!), and a fix for the missing YCbCr422 in the bitmask |
|
This work has been superseded by PR #4754, which has been merged in the RPi tree and upstream. Closing. |
Hi,
This is a PR to support the YUV 444 and 420 output from the KMS driver on the BCM2711.
While it works, it's marked as a draft since it introduces a new KMS property. Indeed, DRM requires for any new property that it's been agreed on and used by an open-source project: https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements
I was thinking Kodi could be that vessel. The process next would be to make sure we have some patches to support it from within Kodi, and that the Kodi maintainers have reviewed it and agreed to it. However, it shouldn't be merged at this point, since this interface isn't stable from the kernel point-of-view, so it could change.
Once there, we can submit it to the kernel, get it merged, and then we will be able to merge it within Kodi.