Re: [PATCH v7 19/36] drm/connector: hdmi: Compute bpc and format automatically

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5455 bytes --]

Hi Alex,

On Sun, Feb 25, 2024 at 02:56:02PM +0100, Alex Bee wrote:
> Am 22.02.24 um 19:14 schrieb Maxime Ripard:
> > Now that we have all the infrastructure needed, we can add some code
> > that will, for a given connector state and mode, compute the best output
> > format and bpc.
> > 
> > The algorithm is equivalent to the one already found in i915 and vc4.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >   drivers/gpu/drm/drm_atomic_state_helper.c          | 184 ++++++++++++++++++++-
> >   .../gpu/drm/tests/drm_atomic_state_helper_test.c   |  25 ++-
> >   2 files changed, 197 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 448b4a73d1c8..9f517599f117 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -31,6 +31,7 @@
> >   #include <drm/drm_connector.h>
> >   #include <drm/drm_crtc.h>
> >   #include <drm/drm_device.h>
> > +#include <drm/drm_edid.h>
> >   #include <drm/drm_framebuffer.h>
> >   #include <drm/drm_plane.h>
> >   #include <drm/drm_print.h>
> > @@ -662,6 +663,96 @@ connector_state_get_mode(const struct drm_connector_state *conn_state)
> >   	return &crtc_state->mode;
> >   }
> > +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);
> > +		return false;
> > +	}
> > +
> > +	if (!info->is_hdmi &&
> > +	    (format != HDMI_COLORSPACE_RGB || bpc != 8)) {
> > +		drm_dbg(dev, "DVI Monitors require an RGB output at 8 bpc\n");
> > +		return false;
> > +	}
> > +
> > +	if (!(connector->hdmi.supported_formats & BIT(format))) {
> > +		drm_dbg(dev, "%s format unsupported by the connector.\n",
> > +			drm_hdmi_connector_get_output_format_name(format));
> > +		return false;
> > +	}
> > +
> > +	switch (format) {
> > +	case HDMI_COLORSPACE_RGB:
> > +		drm_dbg(dev, "RGB Format, checking the constraints.\n");
> > +
> > +		if (!(info->color_formats & DRM_COLOR_FORMAT_RGB444))
> > +			return false;
> > +
> > +		if (bpc == 10 && !(info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_30)) {
> > +			drm_dbg(dev, "10 BPC but sink doesn't support Deep Color 30.\n");
> > +			return false;
> > +		}
> > +
> > +		if (bpc == 12 && !(info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_36)) {
> > +			drm_dbg(dev, "12 BPC but sink doesn't support Deep Color 36.\n");
> > +			return false;
> > +		}
> > +
> > +		drm_dbg(dev, "RGB format supported in that configuration.\n");
> > +
> > +		return true;
> > +
> > +	case HDMI_COLORSPACE_YUV422:
> > +		drm_dbg(dev, "YUV422 format, checking the constraints.\n");
> > +
> > +		if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR422)) {
> > +			drm_dbg(dev, "Sink doesn't support YUV422.\n");
> > +			return false;
> > +		}
> > +
> > +		if (bpc != 12) {
> > +			drm_dbg(dev, "YUV422 only supports 12 bpc.\n");
> > +			return false;
> > +		}
> > +
> I'm not sure this check is really necessary/helpful.
> In [0] you are quoting HDMI specs which are saying that YUV422 is just
> always 12 bpc - which I guess is correct. The problem I'm seeing here:
> There are HDMI 1.4 controllers, like Rockchip Inno HDMI, that support
> YUV422 but do not support any other color depth than 8 bpc for RGB or
> YUV444. In drmm_connector_hdmi_init you are expecting to give the max bpc
> as parameter and (if I'm getting it correctly) I'd had to set it to 12 to
> also get YUV422 modes, but I'd also get RGB/YUV444 with bpc > 8 modes which
> are not supported by this controller. I guess the same applies to other
> HDMI 1.4 controllers that support YUV422. Or would I have to filter it out
> myself?
> So I guess the easiest way around is to drop the above check since it is
> just always 12 bpc for YUV422 and there is no need to filter out anything.
> (Same applies to the similar check in [0]).

So, let's tackle drm_connector_hdmi_compute_mode_clock() first, and then
I'll try to answer most of your question there.

If drm_connector_hdmi_compute_mode_clock() is called with the YCbCr422
format and a bpc != 12, what should we return if not an error?

It's the only bpc count allowed by the spec and for which we have a
formula for. I just can't return the character rate of YCbCr422 with 8
bpc, I have no idea what it should be.

And now pivoting to the block of code you commented on, there's two
things to consider here. Eventually, the userspace is in charge of
limiting the bpc count, and we have to take it into account.

If the userspace limits us to below 12bpc, we fall back to the
discussion above: we simply have no way to tell how it works out for
YCbCr422, and RGB is the only solution we have.

In your particular case, what you actually want is to prevent RGB 10 and
12bpc to be used. I guess we could create a new driver hook or extend
the one that checks for the code you pointed out to check whether the
driver supports it (possibly turned into a helper), but there's no other
way around it I think.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]