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@xxxxxxxxxxxxxxxxxxx>
> ---
> 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
--
Ville Syrjälä
Intel
