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

[Werner Sembach]

Am 04.05.21 um 11:54 schrieb 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.

That was actually an oversight on my side and not intended. Does a RGB fallback make sense?

Now that I think of it get to 2 scenarios:

- The screen is really 420_only, which causes a silent fail and a black screen I guess? Where before at least a log message was written.

- The screen falsely reports as 420_only and using RGB regardless makes it magically work

I think at least warning should be printed to the logs. Something along the lines of: "Display reports as 420 only, but port does not support 420, try forcing RGB, but this is likely to fail."
> 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