Re: [PATCH v6 2/3] drm/i915/display: Restructure output format computation for better expandability

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel