| Message ID | 20240311-kms-hdmi-connector-state-v9-14-d45890323344@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/connector: Create HDMI Connector infrastructure | expand |
On Mon, Mar 11, 2024 at 03:49:42PM +0100, Maxime Ripard 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 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 > @@ -29,10 +29,11 @@ > #include <drm/drm_blend.h> > #include <drm/drm_bridge.h> > #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> > #include <drm/drm_vblank.h> > #include <drm/drm_writeback.h> > @@ -660,10 +661,100 @@ 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); > + > + 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. > + > + 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; > + } > + > + 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, > unsigned long long clock) > { > @@ -704,10 +795,99 @@ hdmi_compute_clock(const struct drm_connector *connector, > state->hdmi.tmds_char_rate = clock; > > return 0; > } > > +static bool > +hdmi_try_format_bpc(const struct drm_connector *connector, > + struct drm_connector_state *state, "state" is IMO not a great naming choice. It forces you to go and look up the definition whenever you're not sure what 'state' actually is when reading the code. Als at some point you may want to plumb different kinds of states into these, or just switch to passing in the whole drm_atomic_state. So I recommend using more specific names for the different kinds of states (eg. "crtc_state"/"conn_state"/etc.) everywhere. > + 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; > + } Looks like you're preferring YCbCr 4:2:2 over RGB 8bpc. Not sure if that's a good tradeoff to make. 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. > + > + 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. > + 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 > * @state: the DRM State object > * > @@ -727,13 +907,11 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, > drm_atomic_get_new_connector_state(state, connector); > const struct drm_display_mode *mode = > connector_state_get_mode(new_state); > int ret; > > - 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; > > if (old_state->hdmi.output_bpc != new_state->hdmi.output_bpc || > old_state->hdmi.output_format != new_state->hdmi.output_format) { > 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 5a8750153510..f010fde0eb69 100644 > --- a/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c > +++ b/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c > @@ -68,13 +68,10 @@ static int light_up_connector(struct kunit *test, > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); > > 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); > > crtc_state = drm_atomic_get_crtc_state(state, crtc); > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state); > @@ -249,14 +246,19 @@ static void drm_test_check_output_bpc_crtc_mode_changed(struct kunit *test) > priv = drm_atomic_helper_connector_hdmi_init(test, > BIT(HDMI_COLORSPACE_RGB), > 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); > > drm = &priv->drm; > crtc = priv->crtc; > @@ -270,15 +272,15 @@ static void drm_test_check_output_bpc_crtc_mode_changed(struct kunit *test) > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > > 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); > > old_conn_state = drm_atomic_get_old_connector_state(state, conn); > @@ -318,14 +320,19 @@ static void drm_test_check_output_bpc_crtc_mode_not_changed(struct kunit *test) > priv = drm_atomic_helper_connector_hdmi_init(test, > BIT(HDMI_COLORSPACE_RGB), > 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); > > drm = &priv->drm; > crtc = priv->crtc; > @@ -668,11 +675,11 @@ static void drm_test_check_format_value(struct kunit *test) > 8); > KUNIT_ASSERT_NOT_NULL(test, priv); > > 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); > } > > /* > * Test that the value of the output format property out of reset is set > * to 0, and will be computed at atomic_check time. > > -- > 2.43.2
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
On Mon, Mar 18, 2024 at 01:05:22PM +0100, Maxime Ripard wrote: > 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. 8bpc RGB is in fact mandatory. > > 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. Ideally I'd like to know the actual color depth of the panel independently of the supported signal color depths. Unfortunately I don't think EDID gives us that. Can't recall if DisplayID might have something a bit more sensible. Given how info->bpc works right now, I suppose it would make sense to bump it up to 12 when 4:2:2 is supported. But I've not thought through the actual implications such a change. > 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? Since 8bpc RGB must always be supported this looks like dead code to me.
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 @@ -29,10 +29,11 @@ #include <drm/drm_blend.h> #include <drm/drm_bridge.h> #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> #include <drm/drm_vblank.h> #include <drm/drm_writeback.h> @@ -660,10 +661,100 @@ 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); + + 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; + } + + 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, unsigned long long clock) { @@ -704,10 +795,99 @@ hdmi_compute_clock(const struct drm_connector *connector, state->hdmi.tmds_char_rate = clock; 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 * @state: the DRM State object * @@ -727,13 +907,11 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, drm_atomic_get_new_connector_state(state, connector); const struct drm_display_mode *mode = connector_state_get_mode(new_state); int ret; - 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; if (old_state->hdmi.output_bpc != new_state->hdmi.output_bpc || old_state->hdmi.output_format != new_state->hdmi.output_format) { 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 5a8750153510..f010fde0eb69 100644 --- a/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c +++ b/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c @@ -68,13 +68,10 @@ static int light_up_connector(struct kunit *test, KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); 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); crtc_state = drm_atomic_get_crtc_state(state, crtc); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state); @@ -249,14 +246,19 @@ static void drm_test_check_output_bpc_crtc_mode_changed(struct kunit *test) priv = drm_atomic_helper_connector_hdmi_init(test, BIT(HDMI_COLORSPACE_RGB), 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); drm = &priv->drm; crtc = priv->crtc; @@ -270,15 +272,15 @@ static void drm_test_check_output_bpc_crtc_mode_changed(struct kunit *test) KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); 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); old_conn_state = drm_atomic_get_old_connector_state(state, conn); @@ -318,14 +320,19 @@ static void drm_test_check_output_bpc_crtc_mode_not_changed(struct kunit *test) priv = drm_atomic_helper_connector_hdmi_init(test, BIT(HDMI_COLORSPACE_RGB), 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); drm = &priv->drm; crtc = priv->crtc; @@ -668,11 +675,11 @@ static void drm_test_check_format_value(struct kunit *test) 8); KUNIT_ASSERT_NOT_NULL(test, priv); 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); } /* * Test that the value of the output format property out of reset is set * to 0, and will be computed at atomic_check time.
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(-)