| Message ID | 20210507084903.28877-3-wse@tuxedocomputers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Try YCbCr420 color when RGB fails | expand |
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
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;
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(-)