[v5,15/44] drm/connector: hdmi: Compute bpc and format automatically

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 the same one than the one already found in i915 and
vc4.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_state_helper.c          | 183 ++++++-
 .../gpu/drm/tests/drm_atomic_state_helper_test.c   | 529 ++++++++++++++++++++-
 drivers/gpu/drm/tests/drm_kunit_edid.h             | 160 +++++++
 3 files changed, 860 insertions(+), 12 deletions(-)

Hi Maxime

On Thu, 7 Dec 2023 at 15:50, Maxime Ripard <mripard@kernel.org> wrote:
>
> 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 the same one than the one already found in i915 and
> vc4.

We seem to have some extra words in this sentence.
"The algorithm is the same as that already found in i915 and vc4."?
Possibly "equivalent to" instead of "the same as", as i915 is slightly
different.

> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c          | 183 ++++++-
>  .../gpu/drm/tests/drm_atomic_state_helper_test.c   | 529 ++++++++++++++++++++-
>  drivers/gpu/drm/tests/drm_kunit_edid.h             | 160 +++++++
>  3 files changed, 860 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index a36edda590f8..2442b5a2d94f 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -682,6 +682,96 @@ static bool hdmi_is_full_range(const struct drm_connector *connector,
>         return drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_FULL ? true : false;
>  }
>
> +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;

We've dropped this check from vc4 in our downstream kernel as it stops
you using the prebaked EDIDs (eg drm.edid_firmware=edid/1024x768.bin),
or any other EDID that is defined as an analog monitor.
The EDID parsing bombs out at [1], so info->color_formats gets left at 0.

RGB is mandatory for both DVI and HDMI, so rejecting it seems overly fussy.

I don't see i915 making use of info->color_formats at all. The
equivalent function looks to be intel_hdmi_sink_bpc_possible [2] which
just accepts anything for 8bpc output.

  Dave

PS I'll have to defer looking at patch 16 for another day - it just
needs a bit more analysis than I can fit in today.

[1] https://elixir.free-electrons.com/linux/latest/source/drivers/gpu/drm/drm_edid.c#L6533
[2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/display/intel_hdmi.c#L1909

> +
> +               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;
> +               }
> +
> +               drm_dbg(dev, "YUV422 format supported in that configuration.\n");
> +
> +               return true;
> +
> +       case HDMI_COLORSPACE_YUV444:
> +               drm_dbg(dev, "YUV444 format, checking the constraints.\n");
> +
> +               if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR444)) {
> +                       drm_dbg(dev, "Sink doesn't support YUV444.\n");
> +                       return false;
> +               }
> +
> +               if (bpc == 10 && !(info->edid_hdmi_ycbcr444_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_ycbcr444_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, "YUV444 format supported in that configuration.\n");
> +
> +               return true;
> +       }
> +
> +       return false;
> +}
> +
>  static enum drm_mode_status
>  hdmi_clock_valid(const struct drm_connector *connector,
>                  const struct drm_display_mode *mode,
> @@ -726,6 +816,95 @@ hdmi_compute_clock(const struct drm_connector *connector,
>         return 0;
>  }
>
> +static bool
> +hdmi_try_format_bpc(const struct drm_connector *connector,
> +                   struct drm_connector_state *state,
> +                   const struct drm_display_mode *mode,
> +                   unsigned int bpc, enum hdmi_colorspace fmt)
> +{
> +       const struct drm_display_info *info = &connector->display_info;
> +       struct drm_device *dev = connector->dev;
> +       int ret;
> +
> +       drm_dbg(dev, "Trying %s output format\n",
> +               drm_hdmi_connector_get_output_format_name(fmt));
> +
> +       if (!sink_supports_format_bpc(connector, info, mode, fmt, bpc)) {
> +               drm_dbg(dev, "%s output format not supported with %u bpc\n",
> +                       drm_hdmi_connector_get_output_format_name(fmt), bpc);
> +               return false;
> +       }
> +
> +       ret = hdmi_compute_clock(connector, state, mode, bpc, fmt);
> +       if (ret) {
> +               drm_dbg(dev, "Couldn't compute clock for %s output format and %u bpc\n",
> +                       drm_hdmi_connector_get_output_format_name(fmt), bpc);
> +               return false;
> +       }
> +
> +       drm_dbg(dev, "%s output format supported with %u (TMDS char rate: %llu Hz)\n",
> +               drm_hdmi_connector_get_output_format_name(fmt), bpc, state->hdmi.tmds_char_rate);
> +
> +       return true;
> +}
> +
> +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;
> +       }
> +
> +       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);
> +               if (ret)
> +                       continue;
> +
> +               state->hdmi.output_bpc = bpc;
> +
> +               drm_dbg(dev,
> +                       "Mode %ux%u @ %uHz: Found configuration: bpc: %u, fmt: %s, clock: %llu\n",
> +                       mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode),
> +                       state->hdmi.output_bpc,
> +                       drm_hdmi_connector_get_output_format_name(state->hdmi.output_format),
> +                       state->hdmi.tmds_char_rate);
> +
> +               return 0;
> +       }
> +
> +       return -EINVAL;
> +}
> +
>  /**
>   * drm_atomic_helper_connector_hdmi_check() - Helper to check HDMI connector atomic state
>   * @connector: DRM Connector
> @@ -751,9 +930,7 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
>
>         new_state->hdmi.is_full_range = hdmi_is_full_range(connector, new_state);
>
> -       ret = hdmi_compute_clock(connector, new_state, mode,
> -                                new_state->hdmi.output_bpc,
> -                                new_state->hdmi.output_format);
> +       ret = hdmi_compute_config(connector, new_state, mode);
>         if (ret)
>                 return ret;
>
> diff --git a/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c b/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c
> index e7dbdd4a4e7f..860e34b00fee 100644
> --- a/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c
> @@ -70,9 +70,6 @@ static int light_up_connector(struct kunit *test,
>         conn_state = drm_atomic_get_connector_state(state, connector);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
>
> -       conn_state->hdmi.output_bpc = connector->max_bpc;
> -       conn_state->hdmi.output_format = HDMI_COLORSPACE_RGB;
> -
>         ret = drm_atomic_set_crtc_for_connector(conn_state, crtc);
>         KUNIT_EXPECT_EQ(test, ret, 0);
>
> @@ -720,10 +717,15 @@ static void drm_test_check_output_bpc_crtc_mode_changed(struct kunit *test)
>                                                      10);
>         KUNIT_ASSERT_NOT_NULL(test, priv);
>
> +       conn = &priv->connector;
> +       ret = set_connector_edid(test, conn,
> +                                test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
> +                                ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
>         ctx = drm_kunit_helper_acquire_ctx_alloc(test);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
>
> -       conn = &priv->connector;
>         preferred = find_preferred_mode(conn);
>         KUNIT_ASSERT_NOT_NULL(test, preferred);
>
> @@ -741,11 +743,11 @@ static void drm_test_check_output_bpc_crtc_mode_changed(struct kunit *test)
>         old_conn_state = drm_atomic_get_old_connector_state(state, conn);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state);
>
> -       new_conn_state->hdmi.output_bpc = 8;
> +       new_conn_state->max_requested_bpc = 8;
>
>         KUNIT_ASSERT_NE(test,
> -                       old_conn_state->hdmi.output_bpc,
> -                       new_conn_state->hdmi.output_bpc);
> +                       old_conn_state->max_requested_bpc,
> +                       new_conn_state->max_requested_bpc);
>
>         ret = drm_atomic_check_only(state);
>         KUNIT_ASSERT_EQ(test, ret, 0);
> @@ -789,10 +791,15 @@ static void drm_test_check_output_bpc_crtc_mode_not_changed(struct kunit *test)
>                                                      10);
>         KUNIT_ASSERT_NOT_NULL(test, priv);
>
> +       conn = &priv->connector;
> +       ret = set_connector_edid(test, conn,
> +                                test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
> +                                ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
>         ctx = drm_kunit_helper_acquire_ctx_alloc(test);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
>
> -       conn = &priv->connector;
>         preferred = find_preferred_mode(conn);
>         KUNIT_ASSERT_NOT_NULL(test, preferred);
>
> @@ -832,6 +839,56 @@ static void drm_test_check_output_bpc_crtc_mode_not_changed(struct kunit *test)
>         KUNIT_EXPECT_FALSE(test, crtc_state->mode_changed);
>  }
>
> +/*
> + * Test that if we have an HDMI connector but a !HDMI display, we always
> + * output RGB with 8 bpc.
> + */
> +static void drm_test_check_output_bpc_dvi(struct kunit *test)
> +{
> +       struct drm_atomic_helper_connector_hdmi_priv *priv;
> +       struct drm_modeset_acquire_ctx *ctx;
> +       struct drm_connector_state *conn_state;
> +       struct drm_display_info *info;
> +       struct drm_display_mode *preferred;
> +       struct drm_connector *conn;
> +       struct drm_device *drm;
> +       struct drm_crtc *crtc;
> +       int ret;
> +
> +       priv = drm_atomic_helper_connector_hdmi_init(test,
> +                                                    BIT(HDMI_COLORSPACE_RGB) |
> +                                                    BIT(HDMI_COLORSPACE_YUV422) |
> +                                                    BIT(HDMI_COLORSPACE_YUV444),
> +                                                    12);
> +       KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +       conn = &priv->connector;
> +       ret = set_connector_edid(test, conn,
> +                                test_edid_dvi_1080p,
> +                                ARRAY_SIZE(test_edid_dvi_1080p));
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       info = &conn->display_info;
> +       KUNIT_ASSERT_FALSE(test, info->is_hdmi);
> +
> +       ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +       preferred = find_preferred_mode(conn);
> +       KUNIT_ASSERT_NOT_NULL(test, preferred);
> +
> +       drm = &priv->drm;
> +       crtc = priv->crtc;
> +       ret = light_up_connector(test, drm, crtc, conn, preferred, ctx);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       conn_state = conn->state;
> +       KUNIT_ASSERT_NOT_NULL(test, conn_state);
> +
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 8);
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
> +}
> +
>  /*
>   * Test that when doing a commit which would use RGB 8bpc, the TMDS
>   * clock rate stored in the connector state is equal to the mode clock
> @@ -1024,6 +1081,452 @@ static void drm_test_check_hdmi_funcs_reject_rate(struct kunit *test)
>         KUNIT_EXPECT_LT(test, ret, 0);
>  }
>
> +/*
> + * Test that if:
> + * - We have an HDMI connector supporting RGB only
> + * - The chosen mode has a TMDS character rate higher than the display
> + *   supports in RGB/12bpc
> + * - The chosen mode has a TMDS character rate lower than the display
> + *   supports in RGB/10bpc.
> + *
> + * Then we will pick the latter, and the computed TMDS character rate
> + * will be equal to 1.25 times the mode pixel clock.
> + */
> +static void drm_test_check_max_tmds_rate_bpc_fallback(struct kunit *test)
> +{
> +       struct drm_atomic_helper_connector_hdmi_priv *priv;
> +       struct drm_modeset_acquire_ctx *ctx;
> +       struct drm_connector_state *conn_state;
> +       struct drm_display_info *info;
> +       struct drm_display_mode *preferred;
> +       unsigned long long rate;
> +       struct drm_connector *conn;
> +       struct drm_device *drm;
> +       struct drm_crtc *crtc;
> +       int ret;
> +
> +       priv = drm_atomic_helper_connector_hdmi_init(test,
> +                                                    BIT(HDMI_COLORSPACE_RGB),
> +                                                    12);
> +       KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +       conn = &priv->connector;
> +       ret = set_connector_edid(test, conn,
> +                                test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
> +                                ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       info = &conn->display_info;
> +       KUNIT_ASSERT_TRUE(test, info->is_hdmi);
> +       KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
> +
> +       ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +       preferred = find_preferred_mode(conn);
> +       KUNIT_ASSERT_NOT_NULL(test, preferred);
> +       KUNIT_ASSERT_FALSE(test, preferred->flags & DRM_MODE_FLAG_DBLCLK);
> +
> +       rate = drm_connector_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_RGB);
> +       KUNIT_ASSERT_GT(test, rate, info->max_tmds_clock * 1000);
> +
> +       rate = drm_connector_hdmi_compute_mode_clock(preferred, 10, HDMI_COLORSPACE_RGB);
> +       KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
> +
> +       drm = &priv->drm;
> +       crtc = priv->crtc;
> +       ret = light_up_connector(test, drm, crtc, conn, preferred, ctx);
> +       KUNIT_EXPECT_EQ(test, ret, 0);
> +
> +       conn_state = conn->state;
> +       KUNIT_ASSERT_NOT_NULL(test, conn_state);
> +
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 10);
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.tmds_char_rate, preferred->clock * 1250);
> +}
> +
> +/*
> + * Test that if:
> + * - We have an HDMI connector supporting both RGB and YUV422 and up to
> + *   12 bpc
> + * - The chosen mode has a TMDS character rate higher than the display
> + *   supports in RGB/12bpc
> + * - The chosen mode has a TMDS character rate lower than the display
> + *   supports in YUV422/12bpc.
> + *
> + * Then we will pick the latter, and the computed TMDS character rate
> + * will be equal to the mode pixel clock.
> + */
> +static void drm_test_check_max_tmds_rate_format_fallback(struct kunit *test)
> +{
> +       struct drm_atomic_helper_connector_hdmi_priv *priv;
> +       struct drm_modeset_acquire_ctx *ctx;
> +       struct drm_connector_state *conn_state;
> +       struct drm_display_info *info;
> +       struct drm_display_mode *preferred;
> +       unsigned long long rate;
> +       struct drm_connector *conn;
> +       struct drm_device *drm;
> +       struct drm_crtc *crtc;
> +       int ret;
> +
> +       priv = drm_atomic_helper_connector_hdmi_init(test,
> +                                                    BIT(HDMI_COLORSPACE_RGB) |
> +                                                    BIT(HDMI_COLORSPACE_YUV422) |
> +                                                    BIT(HDMI_COLORSPACE_YUV444),
> +                                                    12);
> +       KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +       conn = &priv->connector;
> +       ret = set_connector_edid(test, conn,
> +                                test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
> +                                ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       info = &conn->display_info;
> +       KUNIT_ASSERT_TRUE(test, info->is_hdmi);
> +       KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
> +
> +       ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +       preferred = find_preferred_mode(conn);
> +       KUNIT_ASSERT_NOT_NULL(test, preferred);
> +       KUNIT_ASSERT_FALSE(test, preferred->flags & DRM_MODE_FLAG_DBLCLK);
> +
> +       rate = drm_connector_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_RGB);
> +       KUNIT_ASSERT_GT(test, rate, info->max_tmds_clock * 1000);
> +
> +       rate = drm_connector_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_YUV422);
> +       KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
> +
> +       drm = &priv->drm;
> +       crtc = priv->crtc;
> +       ret = light_up_connector(test, drm, crtc, conn, preferred, ctx);
> +       KUNIT_EXPECT_EQ(test, ret, 0);
> +
> +       conn_state = conn->state;
> +       KUNIT_ASSERT_NOT_NULL(test, conn_state);
> +
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 12);
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_YUV422);
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.tmds_char_rate, preferred->clock * 1000);
> +}
> +
> +/*
> + * Test that if a driver and screen supports RGB and YUV formats, and we
> + * try to set the VIC 1 mode, we end up with 8bpc RGB even if we could
> + * have had a higher bpc.
> + */
> +static void drm_test_check_output_bpc_format_vic_1(struct kunit *test)
> +{
> +       struct drm_atomic_helper_connector_hdmi_priv *priv;
> +       struct drm_modeset_acquire_ctx *ctx;
> +       struct drm_connector_state *conn_state;
> +       struct drm_display_info *info;
> +       struct drm_display_mode *mode;
> +       unsigned long long rate;
> +       struct drm_connector *conn;
> +       struct drm_device *drm;
> +       struct drm_crtc *crtc;
> +       int ret;
> +
> +       priv = drm_atomic_helper_connector_hdmi_init(test,
> +                                                    BIT(HDMI_COLORSPACE_RGB) |
> +                                                    BIT(HDMI_COLORSPACE_YUV422) |
> +                                                    BIT(HDMI_COLORSPACE_YUV444),
> +                                                    12);
> +       KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +       conn = &priv->connector;
> +       ret = set_connector_edid(test, conn,
> +                                test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
> +                                ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       info = &conn->display_info;
> +       KUNIT_ASSERT_TRUE(test, info->is_hdmi);
> +       KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
> +
> +       ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +       mode = drm_display_mode_from_cea_vic(drm, 1);
> +       KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> +       /*
> +        * NOTE: We can't use drm_connector_hdmi_compute_mode_clock()
> +        * here because we're trying to get the rate of an invalid
> +        * configuration.
> +        *
> +        * Thus, we have to calculate the rate by hand.
> +        */
> +       rate = mode->clock * 1500;
> +       KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
> +
> +       drm = &priv->drm;
> +       crtc = priv->crtc;
> +       ret = light_up_connector(test, drm, crtc, conn, mode, ctx);
> +       KUNIT_EXPECT_EQ(test, ret, 0);
> +
> +       conn_state = conn->state;
> +       KUNIT_ASSERT_NOT_NULL(test, conn_state);
> +
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 8);
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
> +}
> +
> +/*
> + * Test that if a driver supports only RGB but the screen also supports
> + * YUV formats, we only end up with an RGB format.
> + */
> +static void drm_test_check_output_bpc_format_driver_rgb_only(struct kunit *test)
> +{
> +       struct drm_atomic_helper_connector_hdmi_priv *priv;
> +       struct drm_modeset_acquire_ctx *ctx;
> +       struct drm_connector_state *conn_state;
> +       struct drm_display_info *info;
> +       struct drm_display_mode *preferred;
> +       unsigned long long rate;
> +       struct drm_connector *conn;
> +       struct drm_device *drm;
> +       struct drm_crtc *crtc;
> +       int ret;
> +
> +       priv = drm_atomic_helper_connector_hdmi_init(test,
> +                                                    BIT(HDMI_COLORSPACE_RGB),
> +                                                    12);
> +       KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +       conn = &priv->connector;
> +       ret = set_connector_edid(test, conn,
> +                                test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
> +                                ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       info = &conn->display_info;
> +       KUNIT_ASSERT_TRUE(test, info->is_hdmi);
> +       KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
> +
> +       ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +       preferred = find_preferred_mode(conn);
> +       KUNIT_ASSERT_NOT_NULL(test, preferred);
> +
> +       /*
> +        * We're making sure that YUV422 would be the preferred option
> +        * here: we're always favouring higher bpc, we can't have RGB
> +        * because the TMDS character rate exceeds the maximum supported
> +        * by the display, and YUV422 works for that display.
> +        *
> +        * But since the driver only supports RGB, we should fallback to
> +        * a lower bpc with RGB.
> +        */
> +       rate = drm_connector_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_RGB);
> +       KUNIT_ASSERT_GT(test, rate, info->max_tmds_clock * 1000);
> +
> +       rate = drm_connector_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_YUV422);
> +       KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
> +
> +       drm = &priv->drm;
> +       crtc = priv->crtc;
> +       ret = light_up_connector(test, drm, crtc, conn, preferred, ctx);
> +       KUNIT_EXPECT_EQ(test, ret, 0);
> +
> +       conn_state = conn->state;
> +       KUNIT_ASSERT_NOT_NULL(test, conn_state);
> +
> +       KUNIT_EXPECT_LT(test, conn_state->hdmi.output_bpc, 12);
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
> +}
> +
> +/*
> + * Test that if a screen supports only RGB but the driver also supports
> + * YUV formats, we only end up with an RGB format.
> + */
> +static void drm_test_check_output_bpc_format_display_rgb_only(struct kunit *test)
> +{
> +       struct drm_atomic_helper_connector_hdmi_priv *priv;
> +       struct drm_modeset_acquire_ctx *ctx;
> +       struct drm_connector_state *conn_state;
> +       struct drm_display_info *info;
> +       struct drm_display_mode *preferred;
> +       unsigned long long rate;
> +       struct drm_connector *conn;
> +       struct drm_device *drm;
> +       struct drm_crtc *crtc;
> +       int ret;
> +
> +       priv = drm_atomic_helper_connector_hdmi_init(test,
> +                                                    BIT(HDMI_COLORSPACE_RGB) |
> +                                                    BIT(HDMI_COLORSPACE_YUV422) |
> +                                                    BIT(HDMI_COLORSPACE_YUV444),
> +                                                    12);
> +       KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +       conn = &priv->connector;
> +       ret = set_connector_edid(test, conn,
> +                                test_edid_hdmi_1080p_rgb_max_200mhz,
> +                                ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz));
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       info = &conn->display_info;
> +       KUNIT_ASSERT_TRUE(test, info->is_hdmi);
> +       KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
> +
> +       ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +       preferred = find_preferred_mode(conn);
> +       KUNIT_ASSERT_NOT_NULL(test, preferred);
> +
> +       /*
> +        * We're making sure that YUV422 would be the preferred option
> +        * here: we're always favouring higher bpc, we can't have RGB
> +        * because the TMDS character rate exceeds the maximum supported
> +        * by the display, and YUV422 works for that display.
> +        *
> +        * But since the display only supports RGB, we should fallback to
> +        * a lower bpc with RGB.
> +        */
> +       rate = drm_connector_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_RGB);
> +       KUNIT_ASSERT_GT(test, rate, info->max_tmds_clock * 1000);
> +
> +       rate = drm_connector_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_YUV422);
> +       KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
> +
> +       drm = &priv->drm;
> +       crtc = priv->crtc;
> +       ret = light_up_connector(test, drm, crtc, conn, preferred, ctx);
> +       KUNIT_EXPECT_EQ(test, ret, 0);
> +
> +       conn_state = conn->state;
> +       KUNIT_ASSERT_NOT_NULL(test, conn_state);
> +
> +       KUNIT_EXPECT_LT(test, conn_state->hdmi.output_bpc, 12);
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
> +}
> +
> +/*
> + * Test that if a display supports higher bpc but the driver only
> + * supports 8 bpc, we only end up with 8 bpc even if we could have had a
> + * higher bpc.
> + */
> +static void drm_test_check_output_bpc_format_driver_8bpc_only(struct kunit *test)
> +{
> +       struct drm_atomic_helper_connector_hdmi_priv *priv;
> +       struct drm_modeset_acquire_ctx *ctx;
> +       struct drm_connector_state *conn_state;
> +       struct drm_display_info *info;
> +       struct drm_display_mode *preferred;
> +       unsigned long long rate;
> +       struct drm_connector *conn;
> +       struct drm_device *drm;
> +       struct drm_crtc *crtc;
> +       int ret;
> +
> +       priv = drm_atomic_helper_connector_hdmi_init(test,
> +                                                    BIT(HDMI_COLORSPACE_RGB),
> +                                                    8);
> +       KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +       conn = &priv->connector;
> +       ret = set_connector_edid(test, conn,
> +                                test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz,
> +                                ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz));
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       info = &conn->display_info;
> +       KUNIT_ASSERT_TRUE(test, info->is_hdmi);
> +       KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
> +
> +       ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +       preferred = find_preferred_mode(conn);
> +       KUNIT_ASSERT_NOT_NULL(test, preferred);
> +
> +       /*
> +        * We're making sure that we have headroom on the TMDS character
> +        * clock to actually use 12bpc.
> +        */
> +       rate = drm_connector_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_RGB);
> +       KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
> +
> +       drm = &priv->drm;
> +       crtc = priv->crtc;
> +       ret = light_up_connector(test, drm, crtc, conn, preferred, ctx);
> +       KUNIT_EXPECT_EQ(test, ret, 0);
> +
> +       conn_state = conn->state;
> +       KUNIT_ASSERT_NOT_NULL(test, conn_state);
> +
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 8);
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
> +}
> +
> +/*
> + * Test that if a driver supports higher bpc but the display only
> + * supports 8 bpc, we only end up with 8 bpc even if we could have had a
> + * higher bpc.
> + */
> +static void drm_test_check_output_bpc_format_display_8bpc_only(struct kunit *test)
> +{
> +       struct drm_atomic_helper_connector_hdmi_priv *priv;
> +       struct drm_modeset_acquire_ctx *ctx;
> +       struct drm_connector_state *conn_state;
> +       struct drm_display_info *info;
> +       struct drm_display_mode *preferred;
> +       unsigned long long rate;
> +       struct drm_connector *conn;
> +       struct drm_device *drm;
> +       struct drm_crtc *crtc;
> +       int ret;
> +
> +       priv = drm_atomic_helper_connector_hdmi_init(test,
> +                                                    BIT(HDMI_COLORSPACE_RGB) |
> +                                                    BIT(HDMI_COLORSPACE_YUV422) |
> +                                                    BIT(HDMI_COLORSPACE_YUV444),
> +                                                    12);
> +       KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +       conn = &priv->connector;
> +       ret = set_connector_edid(test, conn,
> +                                test_edid_hdmi_1080p_rgb_max_340mhz,
> +                                ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_340mhz));
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       info = &conn->display_info;
> +       KUNIT_ASSERT_TRUE(test, info->is_hdmi);
> +       KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
> +
> +       ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +       preferred = find_preferred_mode(conn);
> +       KUNIT_ASSERT_NOT_NULL(test, preferred);
> +
> +       /*
> +        * We're making sure that we have headroom on the TMDS character
> +        * clock to actually use 12bpc.
> +        */
> +       rate = drm_connector_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_RGB);
> +       KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
> +
> +       drm = &priv->drm;
> +       crtc = priv->crtc;
> +       ret = light_up_connector(test, drm, crtc, conn, preferred, ctx);
> +       KUNIT_EXPECT_EQ(test, ret, 0);
> +
> +       conn_state = conn->state;
> +       KUNIT_ASSERT_NOT_NULL(test, conn_state);
> +
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 8);
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
> +}
> +
>  static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
>         KUNIT_CASE(drm_test_check_broadcast_rgb_auto_cea_mode),
>         KUNIT_CASE(drm_test_check_broadcast_rgb_auto_cea_mode_vic_1),
> @@ -1034,8 +1537,16 @@ static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
>         KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_changed),
>         KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_not_changed),
>         KUNIT_CASE(drm_test_check_hdmi_funcs_reject_rate),
> +       KUNIT_CASE(drm_test_check_max_tmds_rate_bpc_fallback),
> +       KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback),
>         KUNIT_CASE(drm_test_check_output_bpc_crtc_mode_changed),
>         KUNIT_CASE(drm_test_check_output_bpc_crtc_mode_not_changed),
> +       KUNIT_CASE(drm_test_check_output_bpc_dvi),
> +       KUNIT_CASE(drm_test_check_output_bpc_format_vic_1),
> +       KUNIT_CASE(drm_test_check_output_bpc_format_display_8bpc_only),
> +       KUNIT_CASE(drm_test_check_output_bpc_format_display_rgb_only),
> +       KUNIT_CASE(drm_test_check_output_bpc_format_driver_8bpc_only),
> +       KUNIT_CASE(drm_test_check_output_bpc_format_driver_rgb_only),
>         KUNIT_CASE(drm_test_check_tmds_char_rate_rgb_8bpc),
>         KUNIT_CASE(drm_test_check_tmds_char_rate_rgb_10bpc),
>         KUNIT_CASE(drm_test_check_tmds_char_rate_rgb_12bpc),
> @@ -1167,7 +1678,7 @@ static void drm_test_check_format_value(struct kunit *test)
>
>         conn = &priv->connector;
>         conn_state = conn->state;
> -       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
> +       KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, 0);
>  }
>
>  /*
> diff --git a/drivers/gpu/drm/tests/drm_kunit_edid.h b/drivers/gpu/drm/tests/drm_kunit_edid.h
> index 24f3377ef0f0..3e3527c58c31 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_edid.h
> +++ b/drivers/gpu/drm/tests/drm_kunit_edid.h
> @@ -1,6 +1,64 @@
>  #ifndef DRM_KUNIT_EDID_H_
>  #define DRM_KUNIT_EDID_H_
>
> +/*
> + * edid-decode (hex):
> + *
> + * 00 ff ff ff ff ff ff 00 31 d8 2a 00 00 00 00 00
> + * 00 21 01 03 81 a0 5a 78 0a 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 01 01 01 01 01 01 01 01 01 01
> + * 01 01 01 01 01 01 02 3a 80 18 71 38 2d 40 58 2c
> + * 45 00 40 84 63 00 00 1e 00 00 00 fc 00 54 65 73
> + * 74 20 45 44 49 44 0a 20 20 20 00 00 00 fd 00 32
> + * 46 1e 46 0f 00 0a 20 20 20 20 20 20 00 00 00 10
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ab
> + *
> + * ----------------
> + *
> + * Block 0, Base EDID:
> + *   EDID Structure Version & Revision: 1.3
> + *   Vendor & Product Identification:
> + *     Manufacturer: LNX
> + *     Model: 42
> + *     Made in: 2023
> + *   Basic Display Parameters & Features:
> + *     Digital display
> + *     DFP 1.x compatible TMDS
> + *     Maximum image size: 160 cm x 90 cm
> + *     Gamma: 2.20
> + *     RGB color display
> + *     First detailed timing is the preferred timing
> + *   Color Characteristics:
> + *     Red  : 0.0000, 0.0000
> + *     Green: 0.0000, 0.0000
> + *     Blue : 0.0000, 0.0000
> + *     White: 0.0000, 0.0000
> + *   Established Timings I & II: none
> + *   Standard Timings: none
> + *   Detailed Timing Descriptors:
> + *     DTD 1:  1920x1080   60.000000 Hz  16:9     67.500 kHz    148.500000 MHz (1600 mm x 900 mm)
> + *                  Hfront   88 Hsync  44 Hback  148 Hpol P
> + *                  Vfront    4 Vsync   5 Vback   36 Vpol P
> + *     Display Product Name: 'Test EDID'
> + *     Display Range Limits:
> + *       Monitor ranges (GTF): 50-70 Hz V, 30-70 kHz H, max dotclock 150 MHz
> + *     Dummy Descriptor:
> + * Checksum: 0xab
> + */
> +const unsigned char test_edid_dvi_1080p[] = {
> +  0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x2a, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x21, 0x01, 0x03, 0x81, 0xa0, 0x5a, 0x78,
> +  0x0a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
> +  0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02, 0x3a, 0x80, 0x18, 0x71, 0x38,
> +  0x2d, 0x40, 0x58, 0x2c, 0x45, 0x00, 0x40, 0x84, 0x63, 0x00, 0x00, 0x1e,
> +  0x00, 0x00, 0x00, 0xfc, 0x00, 0x54, 0x65, 0x73, 0x74, 0x20, 0x45, 0x44,
> +  0x49, 0x44, 0x0a, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x32,
> +  0x46, 0x1e, 0x46, 0x0f, 0x00, 0x0a, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> +  0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xab
> +};
> +
>  /*
>   * edid-decode (hex):
>   *
> @@ -103,6 +161,108 @@ const unsigned char test_edid_hdmi_1080p_rgb_max_200mhz[] = {
>    0x00, 0x00, 0x00, 0xd0
>  };
>
> +/*
> + * edid-decode (hex):
> + *
> + * 00 ff ff ff ff ff ff 00 31 d8 2a 00 00 00 00 00
> + * 00 21 01 03 81 a0 5a 78 02 00 00 00 00 00 00 00
> + * 00 00 00 20 00 00 01 01 01 01 01 01 01 01 01 01
> + * 01 01 01 01 01 01 02 3a 80 18 71 38 2d 40 58 2c
> + * 45 00 40 84 63 00 00 1e 00 00 00 fc 00 54 65 73
> + * 74 20 45 44 49 44 0a 20 20 20 00 00 00 fd 00 32
> + * 46 1e 46 0f 00 0a 20 20 20 20 20 20 00 00 00 10
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 92
> + *
> + * 02 03 1b 81 e3 05 00 20 41 10 e2 00 4a 6d 03 0c
> + * 00 12 34 00 28 20 00 00 00 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0
> + *
> + * ----------------
> + *
> + * Block 0, Base EDID:
> + *   EDID Structure Version & Revision: 1.3
> + *   Vendor & Product Identification:
> + *     Manufacturer: LNX
> + *     Model: 42
> + *     Made in: 2023
> + *   Basic Display Parameters & Features:
> + *     Digital display
> + *     DFP 1.x compatible TMDS
> + *     Maximum image size: 160 cm x 90 cm
> + *     Gamma: 2.20
> + *     Monochrome or grayscale display
> + *     First detailed timing is the preferred timing
> + *   Color Characteristics:
> + *     Red  : 0.0000, 0.0000
> + *     Green: 0.0000, 0.0000
> + *     Blue : 0.0000, 0.0000
> + *     White: 0.0000, 0.0000
> + *   Established Timings I & II:
> + *     DMT 0x04:   640x480    59.940476 Hz   4:3     31.469 kHz     25.175000 MHz
> + *   Standard Timings: none
> + *   Detailed Timing Descriptors:
> + *     DTD 1:  1920x1080   60.000000 Hz  16:9     67.500 kHz    148.500000 MHz (1600 mm x 900 mm)
> + *                  Hfront   88 Hsync  44 Hback  148 Hpol P
> + *                  Vfront    4 Vsync   5 Vback   36 Vpol P
> + *     Display Product Name: 'Test EDID'
> + *     Display Range Limits:
> + *       Monitor ranges (GTF): 50-70 Hz V, 30-70 kHz H, max dotclock 150 MHz
> + *     Dummy Descriptor:
> + *   Extension blocks: 1
> + * Checksum: 0x92
> + *
> + * ----------------
> + *
> + * Block 1, CTA-861 Extension Block:
> + *   Revision: 3
> + *   Underscans IT Video Formats by default
> + *   Native detailed modes: 1
> + *   Colorimetry Data Block:
> + *     sRGB
> + *   Video Data Block:
> + *     VIC  16:  1920x1080   60.000000 Hz  16:9     67.500 kHz    148.500000 MHz
> + *   Video Capability Data Block:
> + *     YCbCr quantization: No Data
> + *     RGB quantization: Selectable (via AVI Q)
> + *     PT scan behavior: No Data
> + *     IT scan behavior: Always Underscanned
> + *     CE scan behavior: Always Underscanned
> + *   Vendor-Specific Data Block (HDMI), OUI 00-0C-03:
> + *     Source physical address: 1.2.3.4
> + *     Maximum TMDS clock: 340 MHz
> + *     Extended HDMI video details:
> + * Checksum: 0xd0  Unused space in Extension Block: 100 bytes
> + */
> +const unsigned char test_edid_hdmi_1080p_rgb_max_340mhz[] = {
> +  0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x2a, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x21, 0x01, 0x03, 0x81, 0xa0, 0x5a, 0x78,
> +  0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20,
> +  0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
> +  0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02, 0x3a, 0x80, 0x18, 0x71, 0x38,
> +  0x2d, 0x40, 0x58, 0x2c, 0x45, 0x00, 0x40, 0x84, 0x63, 0x00, 0x00, 0x1e,
> +  0x00, 0x00, 0x00, 0xfc, 0x00, 0x54, 0x65, 0x73, 0x74, 0x20, 0x45, 0x44,
> +  0x49, 0x44, 0x0a, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x32,
> +  0x46, 0x00, 0x00, 0xc4, 0x00, 0x0a, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> +  0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x41, 0x02, 0x03, 0x1b, 0x81,
> +  0xe3, 0x05, 0x00, 0x20, 0x41, 0x10, 0xe2, 0x00, 0x4a, 0x6d, 0x03, 0x0c,
> +  0x00, 0x12, 0x34, 0x00, 0x44, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0xd0
> +};
> +
>  /*
>   * edid-decode (hex):
>   *
>
> --
> 2.43.0
>

Hi Maxime

On Thu, 1 Feb 2024 at 12:51, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Thu, Dec 14, 2023 at 03:10:43PM +0000, Dave Stevenson 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);
> > > +               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;
> >
> > We've dropped this check from vc4 in our downstream kernel as it stops
> > you using the prebaked EDIDs (eg drm.edid_firmware=edid/1024x768.bin),
> > or any other EDID that is defined as an analog monitor.
> > The EDID parsing bombs out at [1], so info->color_formats gets left at 0.
>
> Right, but it only does so if the display isn't defined as a digital display...
>
> > RGB is mandatory for both DVI and HDMI, so rejecting it seems overly fussy.
>
> ... which is required for both DVI and HDMI.
>
> And sure enough, if we decode that EDID:
>
> edid-decode (hex):
>
> 00 ff ff ff ff ff ff 00 31 d8 00 00 00 00 00 00
> 05 16 01 03 6d 23 1a 78 ea 5e c0 a4 59 4a 98 25
> 20 50 54 00 08 00 61 40 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 64 19 00 40 41 00 26 30 08 90
> 36 00 63 0a 11 00 00 18 00 00 00 ff 00 4c 69 6e
> 75 78 20 23 30 0a 20 20 20 20 00 00 00 fd 00 3b
> 3d 2f 31 07 00 0a 20 20 20 20 20 20 00 00 00 fc
> 00 4c 69 6e 75 78 20 58 47 41 0a 20 20 20 00 55
>
> ----------------
>
> Block 0, Base EDID:
>   EDID Structure Version & Revision: 1.3
>   Vendor & Product Identification:
>     Manufacturer: LNX
>     Model: 0
>     Made in: week 5 of 2012
>   Basic Display Parameters & Features:
>     Analog display
>     Signal Level Standard: 0.700 : 0.000 : 0.700 V p-p
>     Blank level equals black level
>     Sync: Separate Composite Serration
>     Maximum image size: 35 cm x 26 cm
>     Gamma: 2.20
>     DPMS levels: Standby Suspend Off
>     RGB color display
>     First detailed timing is the preferred timing
>   Color Characteristics:
>     Red  : 0.6416, 0.3486
>     Green: 0.2919, 0.5957
>     Blue : 0.1474, 0.1250
>     White: 0.3125, 0.3281
>   Established Timings I & II:
>     DMT 0x10:  1024x768    60.003840 Hz   4:3     48.363 kHz     65.000000 MHz
>   Standard Timings:
>     DMT 0x10:  1024x768    60.003840 Hz   4:3     48.363 kHz     65.000000 MHz
>   Detailed Timing Descriptors:
>     DTD 1:  1024x768    60.003840 Hz   4:3     48.363 kHz     65.000000 MHz (355 mm x 266 mm)
>                  Hfront    8 Hsync 144 Hback  168 Hpol N
>                  Vfront    3 Vsync   6 Vback   29 Vpol N
>     Display Product Serial Number: 'Linux #0'
>     Display Range Limits:
>       Monitor ranges (GTF): 59-61 Hz V, 47-49 kHz H, max dotclock 70 MHz
>     Display Product Name: 'Linux XGA'
> Checksum: 0x55
>
> ----------------
>
> Warnings:
>
> Block 0, Base EDID:
>   Detailed Timing Descriptor #1: DTD is similar but not identical to DMT 0x10.
>
> EDID conformity: PASS
>
> So, if anything, it's the EDID that needs to be updated, not the code there.

So are these EDIDs only valid for VGA outputs and another set needs to
be added for HDMI monitors?

Having drm.edid_firmware=edid/1024x768.bin works on an HDMI connector
prior to this patch, so presumably drm_edid_loader needs to
automatically switch between the existing (analog) and new (digital)
EDIDs based on the connector type? Or are you requiring users to
change the strings they use?

Cheers.
  Dave

On Thu, Feb 01, 2024 at 03:33:24PM +0000, Dave Stevenson wrote:
> Hi Maxime
> 
> On Thu, 1 Feb 2024 at 12:51, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Thu, Dec 14, 2023 at 03:10:43PM +0000, Dave Stevenson 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);
> > > > +               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;
> > >
> > > We've dropped this check from vc4 in our downstream kernel as it stops
> > > you using the prebaked EDIDs (eg drm.edid_firmware=edid/1024x768.bin),
> > > or any other EDID that is defined as an analog monitor.
> > > The EDID parsing bombs out at [1], so info->color_formats gets left at 0.
> >
> > Right, but it only does so if the display isn't defined as a digital display...
> >
> > > RGB is mandatory for both DVI and HDMI, so rejecting it seems overly fussy.
> >
> > ... which is required for both DVI and HDMI.
> >
> > And sure enough, if we decode that EDID:
> >
> > edid-decode (hex):
> >
> > 00 ff ff ff ff ff ff 00 31 d8 00 00 00 00 00 00
> > 05 16 01 03 6d 23 1a 78 ea 5e c0 a4 59 4a 98 25
> > 20 50 54 00 08 00 61 40 01 01 01 01 01 01 01 01
> > 01 01 01 01 01 01 64 19 00 40 41 00 26 30 08 90
> > 36 00 63 0a 11 00 00 18 00 00 00 ff 00 4c 69 6e
> > 75 78 20 23 30 0a 20 20 20 20 00 00 00 fd 00 3b
> > 3d 2f 31 07 00 0a 20 20 20 20 20 20 00 00 00 fc
> > 00 4c 69 6e 75 78 20 58 47 41 0a 20 20 20 00 55
> >
> > ----------------
> >
> > Block 0, Base EDID:
> >   EDID Structure Version & Revision: 1.3
> >   Vendor & Product Identification:
> >     Manufacturer: LNX
> >     Model: 0
> >     Made in: week 5 of 2012
> >   Basic Display Parameters & Features:
> >     Analog display
> >     Signal Level Standard: 0.700 : 0.000 : 0.700 V p-p
> >     Blank level equals black level
> >     Sync: Separate Composite Serration
> >     Maximum image size: 35 cm x 26 cm
> >     Gamma: 2.20
> >     DPMS levels: Standby Suspend Off
> >     RGB color display
> >     First detailed timing is the preferred timing
> >   Color Characteristics:
> >     Red  : 0.6416, 0.3486
> >     Green: 0.2919, 0.5957
> >     Blue : 0.1474, 0.1250
> >     White: 0.3125, 0.3281
> >   Established Timings I & II:
> >     DMT 0x10:  1024x768    60.003840 Hz   4:3     48.363 kHz     65.000000 MHz
> >   Standard Timings:
> >     DMT 0x10:  1024x768    60.003840 Hz   4:3     48.363 kHz     65.000000 MHz
> >   Detailed Timing Descriptors:
> >     DTD 1:  1024x768    60.003840 Hz   4:3     48.363 kHz     65.000000 MHz (355 mm x 266 mm)
> >                  Hfront    8 Hsync 144 Hback  168 Hpol N
> >                  Vfront    3 Vsync   6 Vback   29 Vpol N
> >     Display Product Serial Number: 'Linux #0'
> >     Display Range Limits:
> >       Monitor ranges (GTF): 59-61 Hz V, 47-49 kHz H, max dotclock 70 MHz
> >     Display Product Name: 'Linux XGA'
> > Checksum: 0x55
> >
> > ----------------
> >
> > Warnings:
> >
> > Block 0, Base EDID:
> >   Detailed Timing Descriptor #1: DTD is similar but not identical to DMT 0x10.
> >
> > EDID conformity: PASS
> >
> > So, if anything, it's the EDID that needs to be updated, not the code there.
> 
> So are these EDIDs only valid for VGA outputs and another set needs to
> be added for HDMI monitors?
> 
> Having drm.edid_firmware=edid/1024x768.bin works on an HDMI connector
> prior to this patch, so presumably drm_edid_loader needs to
> automatically switch between the existing (analog) and new (digital)
> EDIDs based on the connector type? Or are you requiring users to
> change the strings they use?

We've discussed that on IRC today. I'm not sure there was a conclusion
other than "well this doesn't seem right". I think we should at least
provide different EDIDs depending on the connector type indeed, but
there was also a few discussions that arose:

  - Is it useful to have embedded EDIDs in the kernel at all, and could
    we just get rid of it?

  - Should we expose those EDIDs to userspace, and what happens to the
    compositor when we do?

  - The current way to generate those EDIDs isn't... optimal? Should we
    get rid of that as well?

Anyway, all of those issues have been here for a while so I don't really
expect this series to fix that.

Maxime