When encoder validation of a display mode fails, retry with less bandwidth heavy YCbCr420 color mode, if available. This enables some HDMI 1.4 setups to support 4k60Hz output, which previously failed silently.
AMDGPU had nearly the exact same issue. This problem description is therefore copied from my commit message of the AMDGPU patch.
On some setups, while the monitor and the gpu support display modes with pixel clocks of up to 600MHz, the link encoder might not. This prevents YCbCr444 and RGB encoding for 4k60Hz, but YCbCr420 encoding might still be possible. However, which color mode is used is decided before the link encoder capabilities are checked. This patch fixes the problem by retrying to find a display mode with YCbCr420 enforced and using it, if it is valid.
This patchset is revision 5. Only change to 4 is a small whitespace error fix. Resend because automation tools did not work with wrong e-mail format used before.
Moves some checks that later will be performed 2 times to an own fuction. This avoids duplicate code later on.
Signed-off-by: Werner Sembach wse@tuxedocomputers.com --- drivers/gpu/drm/i915/display/intel_hdmi.c | 41 ++++++++++++++--------- 1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 46de56af33db..576d3d910d06 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1861,6 +1861,31 @@ static int intel_hdmi_port_clock(int clock, int bpc) return clock * bpc / 8; }
+static enum drm_mode_status +intel_hdmi_mode_clock_valid(struct intel_hdmi *hdmi, int clock, bool has_hdmi_sink) +{ + struct drm_device *dev = intel_hdmi_to_dev(hdmi); + struct drm_i915_private *dev_priv = to_i915(dev); + enum drm_mode_status status; + + /* check if we can do 8bpc */ + status = hdmi_port_clock_valid(hdmi, clock, true, has_hdmi_sink); + + if (has_hdmi_sink) { + /* if we can't do 8bpc we may still be able to do 12bpc */ + if (status != MODE_OK && !HAS_GMCH(dev_priv)) + status = hdmi_port_clock_valid(hdmi, clock * 3 / 2, + true, has_hdmi_sink); + + /* if we can't do 8,12bpc we may still be able to do 10bpc */ + if (status != MODE_OK && INTEL_GEN(dev_priv) >= 11) + status = hdmi_port_clock_valid(hdmi, clock * 5 / 4, + true, has_hdmi_sink); + } + + return status; +} + static enum drm_mode_status intel_hdmi_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) @@ -1891,21 +1916,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector, if (drm_mode_is_420_only(&connector->display_info, mode)) clock /= 2;
- /* check if we can do 8bpc */ - status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 8), - true, has_hdmi_sink); - - if (has_hdmi_sink) { - /* if we can't do 8bpc we may still be able to do 12bpc */ - if (status != MODE_OK && !HAS_GMCH(dev_priv)) - status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 12), - true, has_hdmi_sink); - - /* if we can't do 8,12bpc we may still be able to do 10bpc */ - if (status != MODE_OK && DISPLAY_VER(dev_priv) >= 11) - status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 10), - true, has_hdmi_sink); - } + status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink); if (status != MODE_OK) return status;
Couples the decission between RGB and YCbCr420 mode and the check if the port clock can archive the required frequency. Other checks and configuration steps that where previously done in between can also be done before or after.
This allows for are cleaner implementation of retrying different color encodings.
Slight change in behaviour: If YCbCr420 is not allowed but display is YCbCr420 only it no longer fails, but just prints an error and tries to fallback on RGB.
Signed-off-by: Werner Sembach wse@tuxedocomputers.com --- drivers/gpu/drm/i915/display/intel_hdmi.c | 65 ++++++++++++----------- 1 file changed, 34 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 576d3d910d06..b0201d4f27eb 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1999,29 +1999,6 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, INTEL_OUTPUT_FORMAT_YCBCR420); }
-static int -intel_hdmi_ycbcr420_config(struct intel_crtc_state *crtc_state, - const struct drm_connector_state *conn_state) -{ - struct drm_connector *connector = conn_state->connector; - struct drm_i915_private *i915 = to_i915(connector->dev); - const struct drm_display_mode *adjusted_mode = - &crtc_state->hw.adjusted_mode; - - if (!drm_mode_is_420_only(&connector->display_info, adjusted_mode)) - return 0; - - if (!connector->ycbcr_420_allowed) { - drm_err(&i915->drm, - "Platform doesn't support YCBCR420 output\n"); - return -EINVAL; - } - - crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420; - - return intel_pch_panel_fitting(crtc_state, conn_state); -} - static int intel_hdmi_compute_bpc(struct intel_encoder *encoder, struct intel_crtc_state *crtc_state, int clock) @@ -2128,6 +2105,30 @@ static bool intel_hdmi_has_audio(struct intel_encoder *encoder, return intel_conn_state->force_audio == HDMI_AUDIO_ON; }
+static int intel_hdmi_compute_output_format(struct intel_encoder *encoder, + struct intel_crtc_state *crtc_state, + const struct drm_connector_state *conn_state) +{ + struct drm_connector *connector = conn_state->connector; + struct drm_i915_private *i915 = to_i915(connector->dev); + const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; + int ret; + bool ycbcr_420_only; + + ycbcr_420_only = drm_mode_is_420_only(&connector->display_info, adjusted_mode); + if (connector->ycbcr_420_allowed && ycbcr_420_only) + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420; + else { + if (!connector->ycbcr_420_allowed && ycbcr_420_only) + drm_err(&i915->drm, "Display only supports YCbCr420 output, but connector does not allow it. Fallback to RGB, but this will likely fail.\n"); + crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB; + } + + ret = intel_hdmi_compute_clock(encoder, crtc_state); + + return ret; +} + int intel_hdmi_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) @@ -2152,23 +2153,25 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder, if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) pipe_config->pixel_multiplier = 2;
- ret = intel_hdmi_ycbcr420_config(pipe_config, conn_state); - if (ret) - return ret; - - pipe_config->limited_color_range = - intel_hdmi_limited_color_range(pipe_config, conn_state); - if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv)) pipe_config->has_pch_encoder = true;
pipe_config->has_audio = intel_hdmi_has_audio(encoder, pipe_config, conn_state);
- ret = intel_hdmi_compute_clock(encoder, pipe_config); + ret = intel_hdmi_compute_output_format(encoder, pipe_config, conn_state); if (ret) return ret;
+ if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) { + ret = intel_pch_panel_fitting(pipe_config, conn_state); + if (ret) + return ret; + } + + pipe_config->limited_color_range = + intel_hdmi_limited_color_range(pipe_config, conn_state); + if (conn_state->picture_aspect_ratio) adjusted_mode->picture_aspect_ratio = conn_state->picture_aspect_ratio;
When encoder validation of a display mode fails, retry with less bandwidth heavy YCbCr420 color mode, if available. This enables some HDMI 1.4 setups to support 4k60Hz output, which previously failed silently.
AMDGPU had nearly the exact same issue. This problem description is therefore copied from my commit message of the AMDGPU patch.
On some setups, while the monitor and the gpu support display modes with pixel clocks of up to 600MHz, the link encoder might not. This prevents YCbCr444 and RGB encoding for 4k60Hz, but YCbCr420 encoding might still be possible. However, which color mode is used is decided before the link encoder capabilities are checked. This patch fixes the problem by retrying to find a display mode with YCbCr420 enforced and using it, if it is valid.
Signed-off-by: Werner Sembach wse@tuxedocomputers.com --- drivers/gpu/drm/i915/display/intel_hdmi.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index b0201d4f27eb..7815569267e3 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1897,6 +1897,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector, int clock = mode->clock; int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; bool has_hdmi_sink = intel_has_hdmi_sink(hdmi, connector->state); + bool ycbcr_420_only;
if (mode->flags & DRM_MODE_FLAG_DBLSCAN) return MODE_NO_DBLESCAN; @@ -1913,12 +1914,20 @@ intel_hdmi_mode_valid(struct drm_connector *connector, clock *= 2; }
- if (drm_mode_is_420_only(&connector->display_info, mode)) + ycbcr_420_only = drm_mode_is_420_only(&connector->display_info, mode); + if (ycbcr_420_only) clock /= 2;
status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink); - if (status != MODE_OK) - return status; + if (status != MODE_OK) { + if (ycbcr_420_only || !connector->ycbcr_420_allowed || !drm_mode_is_420_also(&connector->display_info, mode)) + return status; + + clock /= 2; + status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink); + if (status != MODE_OK) + return status; + }
return intel_mode_valid_max_plane_size(dev_priv, mode, false); } @@ -2125,6 +2134,14 @@ static int intel_hdmi_compute_output_format(struct intel_encoder *encoder, }
ret = intel_hdmi_compute_clock(encoder, crtc_state); + if (ret) { + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_YCBCR420 && + connector->ycbcr_420_allowed && + drm_mode_is_420_also(&connector->display_info, adjusted_mode)) { + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420; + ret = intel_hdmi_compute_clock(encoder, crtc_state); + } + }
return ret; }
When encoder validation of a display mode fails, retry with less bandwidth heavy YCbCr420 color mode, if available. This enables some HDMI 1.4 setups to support 4k60Hz output, which previously failed silently.
AMDGPU had nearly the exact same issue. This problem description is therefore copied from my commit message of the AMDGPU patch.
On some setups, while the monitor and the gpu support display modes with pixel clocks of up to 600MHz, the link encoder might not. This prevents YCbCr444 and RGB encoding for 4k60Hz, but YCbCr420 encoding might still be possible. However, which color mode is used is decided before the link encoder capabilities are checked. This patch fixes the problem by retrying to find a display mode with YCbCr420 enforced and using it, if it is valid.
This patchset is revision 6. Fixed only some coding style issues.
Moves some checks that later will be performed 2 times to an own function. This avoids duplicate code later on.
Signed-off-by: Werner Sembach wse@tuxedocomputers.com --- drivers/gpu/drm/i915/display/intel_hdmi.c | 41 ++++++++++++++--------- 1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 46de56af33db..576d3d910d06 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1861,6 +1861,31 @@ static int intel_hdmi_port_clock(int clock, int bpc) return clock * bpc / 8; }
+static enum drm_mode_status +intel_hdmi_mode_clock_valid(struct intel_hdmi *hdmi, int clock, bool has_hdmi_sink) +{ + struct drm_device *dev = intel_hdmi_to_dev(hdmi); + struct drm_i915_private *dev_priv = to_i915(dev); + enum drm_mode_status status; + + /* check if we can do 8bpc */ + status = hdmi_port_clock_valid(hdmi, clock, true, has_hdmi_sink); + + if (has_hdmi_sink) { + /* if we can't do 8bpc we may still be able to do 12bpc */ + if (status != MODE_OK && !HAS_GMCH(dev_priv)) + status = hdmi_port_clock_valid(hdmi, clock * 3 / 2, + true, has_hdmi_sink); + + /* if we can't do 8,12bpc we may still be able to do 10bpc */ + if (status != MODE_OK && INTEL_GEN(dev_priv) >= 11) + status = hdmi_port_clock_valid(hdmi, clock * 5 / 4, + true, has_hdmi_sink); + } + + return status; +} + static enum drm_mode_status intel_hdmi_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) @@ -1891,21 +1916,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector, if (drm_mode_is_420_only(&connector->display_info, mode)) clock /= 2;
- /* check if we can do 8bpc */ - status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 8), - true, has_hdmi_sink); - - if (has_hdmi_sink) { - /* if we can't do 8bpc we may still be able to do 12bpc */ - if (status != MODE_OK && !HAS_GMCH(dev_priv)) - status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 12), - true, has_hdmi_sink); - - /* if we can't do 8,12bpc we may still be able to do 10bpc */ - if (status != MODE_OK && DISPLAY_VER(dev_priv) >= 11) - status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 10), - true, has_hdmi_sink); - } + status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink); if (status != MODE_OK) return status;
On Fri, May 07, 2021 at 10:49:01AM +0200, Werner Sembach wrote:
Moves some checks that later will be performed 2 times to an own function. This avoids duplicate code later on.
Signed-off-by: Werner Sembach wse@tuxedocomputers.com
drivers/gpu/drm/i915/display/intel_hdmi.c | 41 ++++++++++++++--------- 1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 46de56af33db..576d3d910d06 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1861,6 +1861,31 @@ static int intel_hdmi_port_clock(int clock, int bpc) return clock * bpc / 8; }
+static enum drm_mode_status +intel_hdmi_mode_clock_valid(struct intel_hdmi *hdmi, int clock, bool has_hdmi_sink) +{
- struct drm_device *dev = intel_hdmi_to_dev(hdmi);
- struct drm_i915_private *dev_priv = to_i915(dev);
- enum drm_mode_status status;
- /* check if we can do 8bpc */
- status = hdmi_port_clock_valid(hdmi, clock, true, has_hdmi_sink);
- if (has_hdmi_sink) {
/* if we can't do 8bpc we may still be able to do 12bpc */if (status != MODE_OK && !HAS_GMCH(dev_priv))status = hdmi_port_clock_valid(hdmi, clock * 3 / 2,
Seems we've lost intel_hdmi_port_clock() here somehow.
true, has_hdmi_sink);/* if we can't do 8,12bpc we may still be able to do 10bpc */if (status != MODE_OK && INTEL_GEN(dev_priv) >= 11)status = hdmi_port_clock_valid(hdmi, clock * 5 / 4,
Also here.
true, has_hdmi_sink);- }
- return status;
+}
static enum drm_mode_status intel_hdmi_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) @@ -1891,21 +1916,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector, if (drm_mode_is_420_only(&connector->display_info, mode)) clock /= 2;
- /* check if we can do 8bpc */
- status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 8),
true, has_hdmi_sink);- if (has_hdmi_sink) {
/* if we can't do 8bpc we may still be able to do 12bpc */if (status != MODE_OK && !HAS_GMCH(dev_priv))status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 12),true, has_hdmi_sink);/* if we can't do 8,12bpc we may still be able to do 10bpc */if (status != MODE_OK && DISPLAY_VER(dev_priv) >= 11)status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 10),true, has_hdmi_sink);- }
- status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink); if (status != MODE_OK) return status;
-- 2.25.1
Am 07.05.21 um 19:47 schrieb Ville Syrjälä:
On Fri, May 07, 2021 at 10:49:01AM +0200, Werner Sembach wrote:
Moves some checks that later will be performed 2 times to an own function. This avoids duplicate code later on.
Signed-off-by: Werner Sembach wse@tuxedocomputers.com
drivers/gpu/drm/i915/display/intel_hdmi.c | 41 ++++++++++++++--------- 1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 46de56af33db..576d3d910d06 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1861,6 +1861,31 @@ static int intel_hdmi_port_clock(int clock, int bpc) return clock * bpc / 8; }
+static enum drm_mode_status +intel_hdmi_mode_clock_valid(struct intel_hdmi *hdmi, int clock, bool has_hdmi_sink) +{
- struct drm_device *dev = intel_hdmi_to_dev(hdmi);
- struct drm_i915_private *dev_priv = to_i915(dev);
- enum drm_mode_status status;
- /* check if we can do 8bpc */
- status = hdmi_port_clock_valid(hdmi, clock, true, has_hdmi_sink);
- if (has_hdmi_sink) {
/* if we can't do 8bpc we may still be able to do 12bpc */if (status != MODE_OK && !HAS_GMCH(dev_priv))status = hdmi_port_clock_valid(hdmi, clock * 3 / 2,Seems we've lost intel_hdmi_port_clock() here somehow.
Yes, I think it happened when I rebased from torvalds/master to drm-tip/drm-tip.
Thanks for pointing it out. Fixed.
true, has_hdmi_sink);/* if we can't do 8,12bpc we may still be able to do 10bpc */if (status != MODE_OK && INTEL_GEN(dev_priv) >= 11)status = hdmi_port_clock_valid(hdmi, clock * 5 / 4,Also here.
true, has_hdmi_sink);- }
- return status;
+}
static enum drm_mode_status intel_hdmi_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) @@ -1891,21 +1916,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector, if (drm_mode_is_420_only(&connector->display_info, mode)) clock /= 2;
- /* check if we can do 8bpc */
- status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 8),
true, has_hdmi_sink);- if (has_hdmi_sink) {
/* if we can't do 8bpc we may still be able to do 12bpc */if (status != MODE_OK && !HAS_GMCH(dev_priv))status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 12),true, has_hdmi_sink);/* if we can't do 8,12bpc we may still be able to do 10bpc */if (status != MODE_OK && DISPLAY_VER(dev_priv) >= 11)status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 10),true, has_hdmi_sink);- }
- status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink); if (status != MODE_OK) return status;
-- 2.25.1
Couples the decission between RGB and YCbCr420 mode and the check if the port clock can archive the required frequency. Other checks and configuration steps that where previously done in between can also be done before or after.
This allows for are cleaner implementation of retrying different color encodings.
A slight change in behaviour occurs with this patch: If YCbCr420 is not allowed but display is YCbCr420 only it no longer fails, but just prints an error and tries to fallback on RGB.
Signed-off-by: Werner Sembach wse@tuxedocomputers.com --- drivers/gpu/drm/i915/display/intel_hdmi.c | 65 ++++++++++++----------- 1 file changed, 34 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 576d3d910d06..9f3da72dabee 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1999,29 +1999,6 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, INTEL_OUTPUT_FORMAT_YCBCR420); }
-static int -intel_hdmi_ycbcr420_config(struct intel_crtc_state *crtc_state, - const struct drm_connector_state *conn_state) -{ - struct drm_connector *connector = conn_state->connector; - struct drm_i915_private *i915 = to_i915(connector->dev); - const struct drm_display_mode *adjusted_mode = - &crtc_state->hw.adjusted_mode; - - if (!drm_mode_is_420_only(&connector->display_info, adjusted_mode)) - return 0; - - if (!connector->ycbcr_420_allowed) { - drm_err(&i915->drm, - "Platform doesn't support YCBCR420 output\n"); - return -EINVAL; - } - - crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420; - - return intel_pch_panel_fitting(crtc_state, conn_state); -} - static int intel_hdmi_compute_bpc(struct intel_encoder *encoder, struct intel_crtc_state *crtc_state, int clock) @@ -2128,6 +2105,30 @@ static bool intel_hdmi_has_audio(struct intel_encoder *encoder, return intel_conn_state->force_audio == HDMI_AUDIO_ON; }
+static int intel_hdmi_compute_output_format(struct intel_encoder *encoder, + struct intel_crtc_state *crtc_state, + const struct drm_connector_state *conn_state) +{ + struct drm_connector *connector = conn_state->connector; + struct drm_i915_private *i915 = to_i915(connector->dev); + const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; + int ret; + bool ycbcr_420_only; + + ycbcr_420_only = drm_mode_is_420_only(&connector->display_info, adjusted_mode); + if (connector->ycbcr_420_allowed && ycbcr_420_only) { + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420; + } else { + if (!connector->ycbcr_420_allowed && ycbcr_420_only) + drm_err(&i915->drm, "Display only supports YCbCr420 output, but connector does not allow it. Fallback to RGB, but this will likely fail.\n"); + crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB; + } + + ret = intel_hdmi_compute_clock(encoder, crtc_state); + + return ret; +} + int intel_hdmi_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) @@ -2152,23 +2153,25 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder, if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) pipe_config->pixel_multiplier = 2;
- ret = intel_hdmi_ycbcr420_config(pipe_config, conn_state); - if (ret) - return ret; - - pipe_config->limited_color_range = - intel_hdmi_limited_color_range(pipe_config, conn_state); - if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv)) pipe_config->has_pch_encoder = true;
pipe_config->has_audio = intel_hdmi_has_audio(encoder, pipe_config, conn_state);
- ret = intel_hdmi_compute_clock(encoder, pipe_config); + ret = intel_hdmi_compute_output_format(encoder, pipe_config, conn_state); if (ret) return ret;
+ if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) { + ret = intel_pch_panel_fitting(pipe_config, conn_state); + if (ret) + return ret; + } + + pipe_config->limited_color_range = + intel_hdmi_limited_color_range(pipe_config, conn_state); + if (conn_state->picture_aspect_ratio) adjusted_mode->picture_aspect_ratio = conn_state->picture_aspect_ratio;
On Fri, May 07, 2021 at 10:49:02AM +0200, Werner Sembach wrote:
Couples the decission between RGB and YCbCr420 mode and the check if the port clock can archive the required frequency. Other checks and configuration steps that where previously done in between can also be done before or after.
This allows for are cleaner implementation of retrying different color encodings.
A slight change in behaviour occurs with this patch: If YCbCr420 is not allowed but display is YCbCr420 only it no longer fails, but just prints an error and tries to fallback on RGB.
Signed-off-by: Werner Sembach wse@tuxedocomputers.com
drivers/gpu/drm/i915/display/intel_hdmi.c | 65 ++++++++++++----------- 1 file changed, 34 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 576d3d910d06..9f3da72dabee 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1999,29 +1999,6 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, INTEL_OUTPUT_FORMAT_YCBCR420); }
-static int -intel_hdmi_ycbcr420_config(struct intel_crtc_state *crtc_state,
const struct drm_connector_state *conn_state)-{
- struct drm_connector *connector = conn_state->connector;
- struct drm_i915_private *i915 = to_i915(connector->dev);
- const struct drm_display_mode *adjusted_mode =
&crtc_state->hw.adjusted_mode;- if (!drm_mode_is_420_only(&connector->display_info, adjusted_mode))
return 0;- if (!connector->ycbcr_420_allowed) {
drm_err(&i915->drm,"Platform doesn't support YCBCR420 output\n");return -EINVAL;- }
- crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
- return intel_pch_panel_fitting(crtc_state, conn_state);
-}
static int intel_hdmi_compute_bpc(struct intel_encoder *encoder, struct intel_crtc_state *crtc_state, int clock) @@ -2128,6 +2105,30 @@ static bool intel_hdmi_has_audio(struct intel_encoder *encoder, return intel_conn_state->force_audio == HDMI_AUDIO_ON; }
+static int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
struct intel_crtc_state *crtc_state,const struct drm_connector_state *conn_state)+{
- struct drm_connector *connector = conn_state->connector;
- struct drm_i915_private *i915 = to_i915(connector->dev);
- const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
- int ret;
- bool ycbcr_420_only;
- ycbcr_420_only = drm_mode_is_420_only(&connector->display_info, adjusted_mode);
- if (connector->ycbcr_420_allowed && ycbcr_420_only) {
crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;- } else {
if (!connector->ycbcr_420_allowed && ycbcr_420_only)drm_err(&i915->drm, "Display only supports YCbCr420 output, but connector does not allow it. Fallback to RGB, but this will likely fail.\n");
We can't let the user spam dmesg with errors freely. So this needs to be a drm_dbg_kms(). Also a bit long, so going to annoyingly wrap always. Could maybe shorten a bit to something like: "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB."
With that sorted, and the intel_hdmi_port_clock() stuff restored, I believe this series is good to go.
I think you confused our CI by replying to the old patch with a whole new series. It can generally deal with a whole new series as a new thread or replies to individual patches with updated versions of exactly that patch, but not full series as a reply to a patch. So I suggest just posting the final versions as a new series. Thanks.
crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB;- }
- ret = intel_hdmi_compute_clock(encoder, crtc_state);
- return ret;
+}
int intel_hdmi_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) @@ -2152,23 +2153,25 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder, if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) pipe_config->pixel_multiplier = 2;
ret = intel_hdmi_ycbcr420_config(pipe_config, conn_state);
if (ret)
return ret;pipe_config->limited_color_range =
intel_hdmi_limited_color_range(pipe_config, conn_state);if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv)) pipe_config->has_pch_encoder = true;
pipe_config->has_audio = intel_hdmi_has_audio(encoder, pipe_config, conn_state);
ret = intel_hdmi_compute_clock(encoder, pipe_config);
ret = intel_hdmi_compute_output_format(encoder, pipe_config, conn_state); if (ret) return ret;
if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) {
ret = intel_pch_panel_fitting(pipe_config, conn_state);if (ret)return ret;}
pipe_config->limited_color_range =
intel_hdmi_limited_color_range(pipe_config, conn_state);if (conn_state->picture_aspect_ratio) adjusted_mode->picture_aspect_ratio = conn_state->picture_aspect_ratio;
-- 2.25.1
On 5/7/21 10:52 AM, Ville Syrjälä wrote:
On Fri, May 07, 2021 at 10:49:02AM +0200, Werner Sembach wrote:
Couples the decission between RGB and YCbCr420 mode and the check if the port clock can archive the required frequency. Other checks and configuration steps that where previously done in between can also be done before or after.
This allows for are cleaner implementation of retrying different color encodings.
A slight change in behaviour occurs with this patch: If YCbCr420 is not allowed but display is YCbCr420 only it no longer fails, but just prints an error and tries to fallback on RGB.
Signed-off-by: Werner Sembach wse@tuxedocomputers.com
drivers/gpu/drm/i915/display/intel_hdmi.c | 65 ++++++++++++----------- 1 file changed, 34 insertions(+), 31 deletions(-)
We can't let the user spam dmesg with errors freely. So this needs to be a drm_dbg_kms(). Also a bit long, so going to annoyingly wrap always. Could maybe shorten a bit to something like: "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB."
With that sorted, and the intel_hdmi_port_clock() stuff restored, I believe this series is good to go.
I think you confused our CI by replying to the old patch with a whole new series. It can generally deal with a whole new series as a new thread or replies to individual patches with updated versions of exactly that patch, but not full series as a reply to a patch. So I suggest just posting the final versions as a new series. Thanks.
Yeah, we try to say "don't do that," but maybe we need to say that more strongly. See Documentation/process/submitting-patches.rst:
<<< However, for a multi-patch series, it is generally best to avoid using In-Reply-To: to link to older versions of the series. This way multiple versions of the patch don't become an unmanageable forest of references in email clients. If a link is helpful, you can use the https://lkml.kernel.org/ redirector (e.g., in the cover email text) to link to an earlier version of the patch series.
Am 07.05.21 um 19:52 schrieb Ville Syrjälä:
On Fri, May 07, 2021 at 10:49:02AM +0200, Werner Sembach wrote:
Couples the decission between RGB and YCbCr420 mode and the check if the port clock can archive the required frequency. Other checks and configuration steps that where previously done in between can also be done before or after.
This allows for are cleaner implementation of retrying different color encodings.
A slight change in behaviour occurs with this patch: If YCbCr420 is not allowed but display is YCbCr420 only it no longer fails, but just prints an error and tries to fallback on RGB.
Signed-off-by: Werner Sembach wse@tuxedocomputers.com
drivers/gpu/drm/i915/display/intel_hdmi.c | 65 ++++++++++++----------- 1 file changed, 34 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 576d3d910d06..9f3da72dabee 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1999,29 +1999,6 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, INTEL_OUTPUT_FORMAT_YCBCR420); }
-static int -intel_hdmi_ycbcr420_config(struct intel_crtc_state *crtc_state,
const struct drm_connector_state *conn_state)-{
- struct drm_connector *connector = conn_state->connector;
- struct drm_i915_private *i915 = to_i915(connector->dev);
- const struct drm_display_mode *adjusted_mode =
&crtc_state->hw.adjusted_mode;- if (!drm_mode_is_420_only(&connector->display_info, adjusted_mode))
return 0;- if (!connector->ycbcr_420_allowed) {
drm_err(&i915->drm,"Platform doesn't support YCBCR420 output\n");return -EINVAL;- }
- crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
- return intel_pch_panel_fitting(crtc_state, conn_state);
-}
static int intel_hdmi_compute_bpc(struct intel_encoder *encoder, struct intel_crtc_state *crtc_state, int clock) @@ -2128,6 +2105,30 @@ static bool intel_hdmi_has_audio(struct intel_encoder *encoder, return intel_conn_state->force_audio == HDMI_AUDIO_ON; }
+static int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
struct intel_crtc_state *crtc_state,const struct drm_connector_state *conn_state)+{
- struct drm_connector *connector = conn_state->connector;
- struct drm_i915_private *i915 = to_i915(connector->dev);
- const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
- int ret;
- bool ycbcr_420_only;
- ycbcr_420_only = drm_mode_is_420_only(&connector->display_info, adjusted_mode);
- if (connector->ycbcr_420_allowed && ycbcr_420_only) {
crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;- } else {
if (!connector->ycbcr_420_allowed && ycbcr_420_only)drm_err(&i915->drm, "Display only supports YCbCr420 output, but connector does not allow it. Fallback to RGB, but this will likely fail.\n");We can't let the user spam dmesg with errors freely. So this needs to be a drm_dbg_kms(). Also a bit long, so going to annoyingly wrap always. Could maybe shorten a bit to something like: "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB."
Done, but I don't have time anymore to test it today. I will test and submit v7 on Monday.
With that sorted, and the intel_hdmi_port_clock() stuff restored, I believe this series is good to go.
I think you confused our CI by replying to the old patch with a whole new series. It can generally deal with a whole new series as a new thread or replies to individual patches with updated versions of exactly that patch, but not full series as a reply to a patch. So I suggest just posting the final versions as a new series. Thanks.
crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB;- }
- ret = intel_hdmi_compute_clock(encoder, crtc_state);
- return ret;
+}
int intel_hdmi_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) @@ -2152,23 +2153,25 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder, if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) pipe_config->pixel_multiplier = 2;
ret = intel_hdmi_ycbcr420_config(pipe_config, conn_state);
if (ret)
return ret;pipe_config->limited_color_range =
intel_hdmi_limited_color_range(pipe_config, conn_state);if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv)) pipe_config->has_pch_encoder = true;
pipe_config->has_audio = intel_hdmi_has_audio(encoder, pipe_config, conn_state);
ret = intel_hdmi_compute_clock(encoder, pipe_config);
ret = intel_hdmi_compute_output_format(encoder, pipe_config, conn_state); if (ret) return ret;
if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) {
ret = intel_pch_panel_fitting(pipe_config, conn_state);if (ret)return ret;}
pipe_config->limited_color_range =
intel_hdmi_limited_color_range(pipe_config, conn_state);if (conn_state->picture_aspect_ratio) adjusted_mode->picture_aspect_ratio = conn_state->picture_aspect_ratio;
-- 2.25.1
When encoder validation of a display mode fails, retry with less bandwidth heavy YCbCr420 color mode, if available. This enables some HDMI 1.4 setups to support 4k60Hz output, which previously failed silently.
AMDGPU had nearly the exact same issue. This problem description is therefore copied from my commit message of the AMDGPU patch.
On some setups, while the monitor and the gpu support display modes with pixel clocks of up to 600MHz, the link encoder might not. This prevents YCbCr444 and RGB encoding for 4k60Hz, but YCbCr420 encoding might still be possible. However, which color mode is used is decided before the link encoder capabilities are checked. This patch fixes the problem by retrying to find a display mode with YCbCr420 enforced and using it, if it is valid.
Signed-off-by: Werner Sembach wse@tuxedocomputers.com --- drivers/gpu/drm/i915/display/intel_hdmi.c | 25 ++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 9f3da72dabee..3c4d7a3e0969 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1897,6 +1897,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector, int clock = mode->clock; int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; bool has_hdmi_sink = intel_has_hdmi_sink(hdmi, connector->state); + bool ycbcr_420_only;
if (mode->flags & DRM_MODE_FLAG_DBLSCAN) return MODE_NO_DBLESCAN; @@ -1913,12 +1914,22 @@ intel_hdmi_mode_valid(struct drm_connector *connector, clock *= 2; }
- if (drm_mode_is_420_only(&connector->display_info, mode)) + ycbcr_420_only = drm_mode_is_420_only(&connector->display_info, mode); + if (ycbcr_420_only) clock /= 2;
status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink); - if (status != MODE_OK) - return status; + if (status != MODE_OK) { + if (ycbcr_420_only || + !connector->ycbcr_420_allowed || + !drm_mode_is_420_also(&connector->display_info, mode)) + return status; + + clock /= 2; + status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink); + if (status != MODE_OK) + return status; + }
return intel_mode_valid_max_plane_size(dev_priv, mode, false); } @@ -2125,6 +2136,14 @@ static int intel_hdmi_compute_output_format(struct intel_encoder *encoder, }
ret = intel_hdmi_compute_clock(encoder, crtc_state); + if (ret) { + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_YCBCR420 && + connector->ycbcr_420_allowed && + drm_mode_is_420_also(&connector->display_info, adjusted_mode)) { + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420; + ret = intel_hdmi_compute_clock(encoder, crtc_state); + } + }
return ret; }
dri-devel@lists.freedesktop.org
-
Randy Dunlap -
Ville Syrjälä -
Werner Sembach