Re: [PATCH v13 15/28] drm/connector: hdmi: Compute bpc and format automatically

[Maxime Ripard]

Hi again,

On Sun, May 12, 2024 at 04:18:38PM +0800, Andy Yan wrote:
> 在 2024-05-07 21:17:33，"Maxime Ripard" <mripard@xxxxxxxxxx> 写道：
> >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.
> >
> >Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >Signed-off-by: Maxime Ripard <mripard@xxxxxxxxxx>
> >---
> > drivers/gpu/drm/display/drm_hdmi_state_helper.c    | 199 ++++++++++++++++++++-
> > drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c |  25 ++-
> > 2 files changed, 212 insertions(+), 12 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> >index 063421835dba..f20dcfecb6b8 100644
> >--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> >+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> >@@ -1,9 +1,11 @@
> > // SPDX-License-Identifier: MIT
> > 
> > #include <drm/drm_atomic.h>
> > #include <drm/drm_connector.h>
> >+#include <drm/drm_edid.h>
> >+#include <drm/drm_print.h>
> > 
> > #include <drm/display/drm_hdmi_helper.h>
> > #include <drm/display/drm_hdmi_state_helper.h>
> > 
> > /**
> >@@ -46,10 +48,112 @@ connector_state_get_mode(const struct drm_connector_state *conn_state)
> > 		return NULL;
> > 
> > 	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);
> >+
> >+	/*
> >+	 * CTA-861-F, section 5.4 - Color Coding & Quantization states
> >+	 * that the bpc must be 8, 10, 12 or 16 except for the default
> >+	 * 640x480 VIC1 where the value must be 8.
> >+	 *
> >+	 * The definition of default here is ambiguous but the spec
> >+	 * refers to VIC1 being the default timing in several occasions
> >+	 * so our understanding is that for the default timing (ie,
> >+	 * VIC1), the bpc must be 8.
> >+	 */
> >+	if (vic == 1 && bpc != 8) {
> >+		drm_dbg_kms(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_kms(dev, "DVI Monitors require an RGB output at 8 bpc\n");
> >+		return false;
> >+	}
> >+
> >+	if (!(connector->hdmi.supported_formats & BIT(format))) {
> >+		drm_dbg_kms(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_kms(dev, "RGB Format, checking the constraints.\n");
> >+
> >+		if (!(info->color_formats & DRM_COLOR_FORMAT_RGB444)) {
> >+			drm_dbg_kms(dev, "Sink doesn't support RGB.\n");
> >+			return false;
> >+		}
> >+
> As I reported in V12,  the HDMI output on my rk3036-kylin was lost after apply this series.
> This is because there is something wrong with the DDC on my board, the edid read always failed
> on first bootup. That means inno_hdmi_connector_get_modes will return 0.
> 
> and in function drm_helper_probe_single_connector_modes:
> 
>          count = drm_helper_probe_get_modes(connector);
> 
>          if (count == 0 && (connector->status == connector_status_connected ||
>                             connector->status == connector_status_unknown)) {
>                  count = drm_add_modes_noedid(connector, 1024, 768);
> 
>                  /*
>                   * Section 4.2.2.6 (EDID Corruption Detection) of the DP 1.4a
>                   * Link CTS specifies that 640x480 (the official "failsafe"
>                   * mode) needs to be the default if there's no EDID.
>                   */
>                  if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
>                          drm_set_preferred_mode(connector, 640, 480);
>          }
> drm_add_modes_noedid will not initialize display_info. So the check about display info will always failed here:
> 
> [    4.205368] rockchip-drm display-subsystem: [drm:drm_atomic_check_only] checking (ptrval)
> [    4.205410] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] mode changed
> [    4.205439] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] enable changed
> [    4.205464] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CRTC:35:crtc-0] active changed
> [    4.205490] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] Updating routing for [CONNECTOR:37:HDMI-A-1]
> [    4.205517] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CONNECTOR:37:HDMI-A-1] using [ENCODER:36:TMDS-36] on [CRTC:35:crtc-0]
> [    4.205545] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Trying with a 8 bpc output
> [    4.205575] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Trying RGB output format
> [    4.205670] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] RGB Format, checking the constraints.
> [    4.205696] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Sink doesn't support RGB.
> [    4.205720] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] RGB output format not supported with 8 bpc
> [    4.205747] rockchip-drm display-subsystem: [drm:drm_atomic_helper_connector_hdmi_check] Failed. No Format Supported for that bpc count.
> [    4.205772] rockchip-drm display-subsystem: [drm:drm_atomic_helper_check_modeset] [CONNECTOR:37:HDMI-A-1] driver check failed
> [    4.205796] rockchip-drm display-subsystem: [drm:drm_atomic_check_only] atomic driver check for (ptrval) failed: -22
> 
> My reply for your email in V12[0] was bounced, so I think you didn't read it.
> 
> [0]https://patchwork.kernel.org/project/linux-rockchip/patch/20240423-kms-hdmi-connector-state-v12-27-3338e4c0b189@xxxxxxxxxx/

Indeed, I never received it, sorry.

Thanks for looking into it, it's very valuable.

I can see several things that interact and could go wrong:

* The DDC readout should not fail like that. From a quick look at the
  driver, I'm wondering if it's not due to the fact that the DDC
  controller isn't powered until the first modeset happens. Since the
  first get_modes call is done with the controller disabled, it's
  probably not initialized enough yet. The first modeset then comes and
  will initialize the controller enough for the subsequent get_modes to
  work. Is it something you could look into?

* drm_display_info not being filled to some sane default when there's no
  EDID is indeed an issue. I can't be made generic, but the HDMI spec
  provides us with some minimum requirements we can probably set in this
  case (RGB supported, 8bpc supported, etc.) I'll work on that.

Thanks again,
Maxime

Attachment: signature.asc
Description: PGP signature