| Message ID | 20240507-kms-hdmi-connector-state-v13-15-8fafc5efe8be@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/connector: Create HDMI Connector infrastructure | expand |
Hi Maxime， 在 2024-05-07 21:17:33，"Maxime Ripard" <mripard@kernel.org> 写道： >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@linux.intel.com> >Signed-off-by: Maxime Ripard <mripard@kernel.org> >--- > 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@kernel.org/ >+ if (bpc == 10 && !(info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_30)) { >+ drm_dbg_kms(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_kms(dev, "12 BPC but sink doesn't support Deep Color 36.\n"); >+ return false; >+ } >+ >+ drm_dbg_kms(dev, "RGB format supported in that configuration.\n"); >+ >+ return true; >+ >+ case HDMI_COLORSPACE_YUV422: >+ drm_dbg_kms(dev, "YUV422 format, checking the constraints.\n"); >+ >+ if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR422)) { >+ drm_dbg_kms(dev, "Sink doesn't support YUV422.\n"); >+ return false; >+ } >+ >+ if (bpc != 12) { >+ drm_dbg_kms(dev, "YUV422 only supports 12 bpc.\n"); >+ return false; >+ } >+ >+ drm_dbg_kms(dev, "YUV422 format supported in that configuration.\n"); >+ >+ return true; >+ >+ case HDMI_COLORSPACE_YUV444: >+ drm_dbg_kms(dev, "YUV444 format, checking the constraints.\n"); >+ >+ if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR444)) { >+ drm_dbg_kms(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_kms(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_kms(dev, "12 BPC but sink doesn't support Deep Color 36.\n"); >+ return false; >+ } >+ >+ drm_dbg_kms(dev, "YUV444 format supported in that configuration.\n"); >+ >+ return true; >+ } >+ >+ return false; >+} >+ >-- >2.45.0 > > >_______________________________________________ >linux-arm-kernel mailing list >linux-arm-kernel@lists.infradead.org >http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi again, On Sun, May 12, 2024 at 04:18:38PM +0800, Andy Yan wrote: > 在 2024-05-07 21:17:33，"Maxime Ripard" <mripard@kernel.org> 写道： > >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@linux.intel.com> > >Signed-off-by: Maxime Ripard <mripard@kernel.org> > >--- > > 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@kernel.org/ 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
Hi Maxime, On 5/16/24 17:45, Maxime Ripard wrote: > Hi again, > > On Sun, May 12, 2024 at 04:18:38PM +0800, Andy Yan wrote: >> 在 2024-05-07 21:17:33，"Maxime Ripard" <mripard@kernel.org> 写道： >>> 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@linux.intel.com> >>> Signed-off-by: Maxime Ripard <mripard@kernel.org> >>> --- >>> 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@kernel.org/ > > 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? I have a same feeling about it. I will check it later. > > * 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. > > Thanks again, > Maxime
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; + } + + if (bpc == 10 && !(info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_30)) { + drm_dbg_kms(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_kms(dev, "12 BPC but sink doesn't support Deep Color 36.\n"); + return false; + } + + drm_dbg_kms(dev, "RGB format supported in that configuration.\n"); + + return true; + + case HDMI_COLORSPACE_YUV422: + drm_dbg_kms(dev, "YUV422 format, checking the constraints.\n"); + + if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR422)) { + drm_dbg_kms(dev, "Sink doesn't support YUV422.\n"); + return false; + } + + if (bpc != 12) { + drm_dbg_kms(dev, "YUV422 only supports 12 bpc.\n"); + return false; + } + + drm_dbg_kms(dev, "YUV422 format supported in that configuration.\n"); + + return true; + + case HDMI_COLORSPACE_YUV444: + drm_dbg_kms(dev, "YUV444 format, checking the constraints.\n"); + + if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR444)) { + drm_dbg_kms(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_kms(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_kms(dev, "12 BPC but sink doesn't support Deep Color 36.\n"); + return false; + } + + drm_dbg_kms(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) { @@ -90,10 +194,101 @@ hdmi_compute_clock(const struct drm_connector *connector, conn_state->hdmi.tmds_char_rate = clock; return 0; } +static bool +hdmi_try_format_bpc(const struct drm_connector *connector, + struct drm_connector_state *conn_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_kms(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_kms(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, conn_state, mode, bpc, fmt); + if (ret) { + drm_dbg_kms(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_kms(dev, "%s output format supported with %u (TMDS char rate: %llu Hz)\n", + drm_hdmi_connector_get_output_format_name(fmt), + bpc, conn_state->hdmi.tmds_char_rate); + + return true; +} + +static int +hdmi_compute_format(const struct drm_connector *connector, + struct drm_connector_state *conn_state, + const struct drm_display_mode *mode, + unsigned int bpc) +{ + struct drm_device *dev = connector->dev; + + /* + * TODO: Add support for YCbCr420 output for HDMI 2.0 capable + * devices, for modes that only support YCbCr420. + */ + if (hdmi_try_format_bpc(connector, conn_state, mode, bpc, HDMI_COLORSPACE_RGB)) { + conn_state->hdmi.output_format = HDMI_COLORSPACE_RGB; + return 0; + } + + drm_dbg_kms(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 *conn_state, + const struct drm_display_mode *mode) +{ + struct drm_device *dev = connector->dev; + unsigned int max_bpc = clamp_t(unsigned int, + conn_state->max_bpc, + 8, connector->max_bpc); + unsigned int bpc; + int ret; + + for (bpc = max_bpc; bpc >= 8; bpc -= 2) { + drm_dbg_kms(dev, "Trying with a %d bpc output\n", bpc); + + ret = hdmi_compute_format(connector, conn_state, mode, bpc); + if (ret) + continue; + + conn_state->hdmi.output_bpc = bpc; + + drm_dbg_kms(dev, + "Mode %ux%u @ %uHz: Found configuration: bpc: %u, fmt: %s, clock: %llu\n", + mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode), + conn_state->hdmi.output_bpc, + drm_hdmi_connector_get_output_format_name(conn_state->hdmi.output_format), + conn_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 * @@ -113,13 +308,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_conn_state); int ret; - ret = hdmi_compute_clock(connector, new_conn_state, mode, - new_conn_state->hdmi.output_bpc, - new_conn_state->hdmi.output_format); + ret = hdmi_compute_config(connector, new_conn_state, mode); if (ret) return ret; if (old_conn_state->hdmi.output_bpc != new_conn_state->hdmi.output_bpc || old_conn_state->hdmi.output_format != new_conn_state->hdmi.output_format) { diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c index ead998a691e7..a49a544d7b49 100644 --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c @@ -70,13 +70,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); @@ -251,14 +248,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; @@ -272,15 +274,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); @@ -320,14 +322,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; @@ -670,11 +677,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. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Maxime Ripard <mripard@kernel.org> --- 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(-)