diff mbox series

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

Message ID 20210507084903.28877-3-wse@tuxedocomputers.com (mailing list archive)
State New, archived
Headers show
Series [v6,1/3] drm/i915/display: New function to avoid duplicate code in upcomming commits | expand

Commit Message

Werner Sembach May 7, 2021, 8:49 a.m. UTC
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(-)

Comments

Ville Syrjala May 7, 2021, 5:52 p.m. UTC | #1
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
Randy Dunlap May 7, 2021, 5:56 p.m. UTC | #2
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.
>>>
Werner Sembach May 7, 2021, 9:24 p.m. UTC | #3
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 mbox series

Patch

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;