Re: [PATCH 3/4] Restructure output format computation for better expandability

[Ville Syrjälä]

On Mon, May 03, 2021 at 08:21:47PM +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.
> 
> Signed-off-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
> ---
> 
> >From 57e42ec6e34ac32da29eb7bc3c691cbeb2534396 Mon Sep 17 00:00:00 2001
> From: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
> Date: Mon, 3 May 2021 15:30:40 +0200
> Subject: [PATCH 3/4] Restructure output format computation for better
>  expandability
> 
> ---
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 57 +++++++++++------------
>  1 file changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index ce165ef28e88..e2553ac6fd13 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,24 @@ static bool intel_hdmi_has_audio(struct intel_encoder *encoder,
>  		return intel_conn_state->force_audio == HDMI_AUDIO_ON;
>  }
>  
> +int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
> +				     struct intel_crtc_state *crtc_state,
> +				     const struct drm_connector_state *conn_state)
> +{
> +	const struct drm_connector *connector = conn_state->connector;
> +	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +	int ret;
> +
> +	if (connector->ycbcr_420_allowed && drm_mode_is_420_only(&connector->display_info, adjusted_mode))
> +		crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
> +	else
> +		crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB;

Slight change in behaviour here since we used to reject 420_only modes
if ycbcr_420_allowed wasn't set. But I think this should be OK, and in
fact I believe the DP counterpart code always used an RGB fallback
rather than failing. So this lines up better with that.

Needs at least a note in the commit message to indicate that
there is a functional change buried within. Though it would be
better to split this functional change into a separate prep patch.

> +
> +	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 +2147,23 @@ 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;
>  
> +	ret = intel_pch_panel_fitting(pipe_config, conn_state);
> +	if (ret)
> +		return ret;

We probably want to still wrap this call in a
if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) {...}

In theory calling intel_pch_panel_fitting() should be a nop for
the !420 case, but I think we have some issues there at least when
it comes to bigjoiner. So the 420 check is probably needed to avoid
mistakenly turning on the panel fitter when not needed.


> +
> +	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