Hi Ville,
Thanks for your review !
On Fri, Mar 15, 2024 at 10:05:16AM +0200, Ville Syrjälä wrote:
> On Mon, Mar 11, 2024 at 03:49:42PM +0100, Maxime Ripard wrote:
> > +static bool
> > +sink_supports_format_bpc(const struct drm_connector *connector,
> > + const struct drm_display_info *info,
> > + const struct drm_display_mode *mode,
> > + unsigned int format, unsigned int bpc)
> > +{
> > + struct drm_device *dev = connector->dev;
> > + u8 vic = drm_match_cea_mode(mode);
> > +
> > + if (vic == 1 && bpc != 8) {
> > + drm_dbg(dev, "VIC1 requires a bpc of 8, got %u\n", bpc);
>
> Use of drm_dbg() for kms stuff is surprising.
>
> > + return false;
> > + }
>
> I don't think we have this in i915. My original impression was that you
> can use higher color depth if you can determine the sink capabilities,
> but all sinks are required to accept 640x480@8bpc as a fallback.
>
> but CTA-861-H says:
> "5.4 Color Coding & Quantization
> Component Depth: The coding shall be N-bit, where N = 8, 10, 12, or 16
> bits/component — except in the case of the default 640x480 Video Timing 1,
> where the value of N shall be 8."
>
> So that does seem to imply that you're supposed to use exactly 8bpc.
> Though the word "default" in there is confusing. Are they specifically
> using that to indicate that this is about the fallback behaviour, or
> is it just indicating that it is a "default mode that always has to
> be supported". Dunno. I guess no real harm in forcing 8bpc for 640x480
> since no one is likely to use that for any high fidelity stuff.
My understanding was that CTA-861 mandates that 640x480@60Hz is
supported, and mentions it being the default timing on a few occurences,
like in section 4 - Video Formats and Waveform Timings that states "This
section describes the default IT 640x480 Video Timing as well as all of
the standard CE Video Timings.", or Section 6.2 - Describing Video
Formats in EDID "The 640x480@60Hz flag, in the Established Timings area,
shall always be set, since the 640x480p format is a mandatory default
timing."
So my understanding is that default here applies to the timing itself,
and not the bpc, and is thus the second interpretation you suggested.
I'll add a comment to make it clearer.
> > +static int
> > +hdmi_compute_format(const struct drm_connector *connector,
> > + struct drm_connector_state *state,
> > + const struct drm_display_mode *mode,
> > + unsigned int bpc)
> > +{
> > + struct drm_device *dev = connector->dev;
> > +
> > + if (hdmi_try_format_bpc(connector, state, mode, bpc, HDMI_COLORSPACE_RGB)) {
> > + state->hdmi.output_format = HDMI_COLORSPACE_RGB;
> > + return 0;
> > + }
> > +
> > + if (hdmi_try_format_bpc(connector, state, mode, bpc, HDMI_COLORSPACE_YUV422)) {
> > + state->hdmi.output_format = HDMI_COLORSPACE_YUV422;
> > + return 0;
> > + }
>
> Looks like you're preferring YCbCr 4:2:2 over RGB 8bpc. Not sure
> if that's a good tradeoff to make.
Yeah, indeed. I guess it's a judgement call on whether we prioritise
lowering the bpc over selecting YUV422, but I guess I can try all
available RGB bpc before falling back to YUV422.
> In i915 we don't currently expose 4:2:2 at all because it doesn't
> help in getting a working display, and we have no uapi for the
> user to force it if they really want 4:2:2 over RGB.
I guess if the priority is given to lowering bpc, then it indeed doesn't
make sense to support YUV422, since the limiting factor is likely to be
the TMDS char rate and YUV422 12 bpc is equivalent to RGB 8bpc there.
dw-hdmi on the other hand will always put YUV422 and YUV444 before RGB
for a given bpc, which is weird to me:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c#L2696
What is even weirder to me is that YUV422 is explicitly stated to be
12bpc only, so there's some invalid configurations there (8 and 10 bpc).
And given that it's order by decreasing order of preference, I'm pretty
sure it'll never actually pick any YUV or RGB > 8bpc format since RGB
8bpc is super likely to be always available and thus picked first.
If we want to converge, I think we should amend this code to support
YUV420 for YUV420-only modes first, and then the RGB options like i915
is doing. And then if someone is interested in more, we can always
expand it to other formats.
> > +
> > + drm_dbg(dev, "Failed. No Format Supported for that bpc count.\n");
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int
> > +hdmi_compute_config(const struct drm_connector *connector,
> > + struct drm_connector_state *state,
> > + const struct drm_display_mode *mode)
> > +{
> > + struct drm_device *dev = connector->dev;
> > + unsigned int max_bpc = clamp_t(unsigned int,
> > + state->max_bpc,
> > + 8, connector->max_bpc);
> > + unsigned int bpc;
> > + int ret;
> > +
> > + for (bpc = max_bpc; bpc >= 8; bpc -= 2) {
> > + drm_dbg(dev, "Trying with a %d bpc output\n", bpc);
> > +
> > + ret = hdmi_compute_format(connector, state, mode, bpc);
>
> Hmm. Actually I'm not sure your 4:2:2 stuff even works since you
> check for bpc==12 in there and only call this based on the max_bpc.
> I'm not convinced max_bpc would actually be 12 for a sink that
> supports YCbCr 4:2:2 but not 12bpc RGB.
It's another discussion we had in an earlier version, but yeah we lack
the infrastructure to support those for now. I still believe it would
require an increased max_bpc to select YUV422, otherwise things would be
pretty inconsistent with other YUV formats.
But yeah, we need to provide a hook to report we don't support RGB >
8bpc for HDMI 1.4 devices. Which goes back to the previous question
actually, I believe it would still provide value to support YUV422 on
those devices, with something like:
for (bpc = max_bpc; bpc >= 8; bpc -= 2) {
if (!connector->hdmi->funcs->validate_config(mode, RGB, bpc))
continue;
// Select RGB with bpc
...
}
if (connector->hdmi->funcs->validate_config(mode, YUV) &&
hdmi_try_format_bpc(..., mode, 12, YUV422) {
// Select YUV422, 12 bpc
...
}
What do you think?
Maxime
Attachment:
signature.asc
Description: PGP signature
