[PATCH 0/4] drm/i915/display Try YCbCr420 color when RGB fails

[PATCH 0/4] drm/i915/display Try YCbCr420 color when RGB fails

[Werner Sembach]

When encoder validation of a display mode fails, retry with less bandwidth
heavy YCbCr420 color mode, if available. This enables some HDMI 1.4 setups
to support 4k60Hz output, which previously failed silently.

AMDGPU had nearly the exact same issue. This problem description is
therefore copied from my commit message of the AMDGPU patch.

On some setups, while the monitor and the gpu support display modes with
pixel clocks of up to 600MHz, the link encoder might not. This prevents
YCbCr444 and RGB encoding for 4k60Hz, but YCbCr420 encoding might still be
possible. However, which color mode is used is decided before the link
encoder capabilities are checked. This patch fixes the problem by retrying
to find a display mode with YCbCr420 enforced and using it, if it is
valid.

This patchset is revision 2, now split up in multiple parts with some
minor restructuring added für a cleaner implementation.

[PATCH 1/4] New function to avoid duplicate code in upcomming commits

[Werner Sembach]

Moves some checks that later will be performed 2 times to an own fuction. This
avoids duplicate code later on.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---

From 1c529783eb2ec02099d1ed2ab9257b008cb6f040 Mon Sep 17 00:00:00 2001
From: Werner Sembach <wse@tuxedocomputers.com>
Date: Mon, 3 May 2021 14:35:39 +0200
Subject: [PATCH 1/4] New function to avoid duplicate code in upcomming commits

---
 drivers/gpu/drm/i915/display/intel_hdmi.c | 41 ++++++++++++++---------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 46de56af33db..576d3d910d06 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -1861,6 +1861,31 @@ static int intel_hdmi_port_clock(int clock, int bpc)
 	return clock * bpc / 8;
 }
 
+static enum drm_mode_status
+intel_hdmi_mode_clock_valid(struct intel_hdmi *hdmi, int clock, bool has_hdmi_sink)
+{
+	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	enum drm_mode_status status;
+
+	/* check if we can do 8bpc */
+	status = hdmi_port_clock_valid(hdmi, clock, true, has_hdmi_sink);
+
+	if (has_hdmi_sink) {
+		/* if we can't do 8bpc we may still be able to do 12bpc */
+		if (status != MODE_OK && !HAS_GMCH(dev_priv))
+			status = hdmi_port_clock_valid(hdmi, clock * 3 / 2,
+						       true, has_hdmi_sink);
+
+		/* if we can't do 8,12bpc we may still be able to do 10bpc */
+		if (status != MODE_OK && INTEL_GEN(dev_priv) >= 11)
+			status = hdmi_port_clock_valid(hdmi, clock * 5 / 4,
+						       true, has_hdmi_sink);
+	}
+
+	return status;
+}
+
 static enum drm_mode_status
 intel_hdmi_mode_valid(struct drm_connector *connector,
 		      struct drm_display_mode *mode)
@@ -1891,21 +1916,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
 	if (drm_mode_is_420_only(&connector->display_info, mode))
 		clock /= 2;
 
-	/* check if we can do 8bpc */
-	status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 8),
-				       true, has_hdmi_sink);
-
-	if (has_hdmi_sink) {
-		/* if we can't do 8bpc we may still be able to do 12bpc */
-		if (status != MODE_OK && !HAS_GMCH(dev_priv))
-			status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 12),
-						       true, has_hdmi_sink);
-
-		/* if we can't do 8,12bpc we may still be able to do 10bpc */
-		if (status != MODE_OK && DISPLAY_VER(dev_priv) >= 11)
-			status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 10),
-						       true, has_hdmi_sink);
-	}
+	status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink);
 	if (status != MODE_OK)
 		return status;
 
-- 
2.25.1

[PATCH 2/4] Add missing check

[Werner Sembach]

Add a missing check that could potentially lead to an unarchivable mode being
validated.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---

From 54fa706f0a5f260a32af5d18b9622ceebb94c12e Mon Sep 17 00:00:00 2001
From: Werner Sembach <wse@tuxedocomputers.com>
Date: Mon, 3 May 2021 14:42:36 +0200
Subject: [PATCH 2/4] Add missing check

---
 drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 576d3d910d06..ce165ef28e88 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -1913,7 +1913,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
 		clock *= 2;
 	}
 
-	if (drm_mode_is_420_only(&connector->display_info, mode))
+	if (connector->ycbcr_420_allowed && drm_mode_is_420_only(&connector->display_info, mode))
 		clock /= 2;
 
 	status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink);
-- 
2.25.1

Re: [PATCH 2/4] Add missing check

[Ville Syrjälä]

On Mon, May 03, 2021 at 08:21:46PM +0200, Werner Sembach wrote:
> Add a missing check that could potentially lead to an unarchivable mode being
> validated.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
> 
> >From 54fa706f0a5f260a32af5d18b9622ceebb94c12e Mon Sep 17 00:00:00 2001
> From: Werner Sembach <wse@tuxedocomputers.com>
> Date: Mon, 3 May 2021 14:42:36 +0200
> Subject: [PATCH 2/4] Add missing check

I guess you did something a bit wonky with git format-patch/send-mail?

> 
> ---
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 576d3d910d06..ce165ef28e88 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -1913,7 +1913,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>  		clock *= 2;
>  	}
>  
> -	if (drm_mode_is_420_only(&connector->display_info, mode))
> +	if (connector->ycbcr_420_allowed && drm_mode_is_420_only(&connector->display_info, mode))

This one shouldn't be necessary. drm_mode_validate_ycbcr420() has
already checked it for us.

>  		clock /= 2;
>  
>  	status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink);
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/4] Add missing check

[Werner Sembach]

Am 04.05.21 um 11:41 schrieb Ville Syrjälä:

> On Mon, May 03, 2021 at 08:21:46PM +0200, Werner Sembach wrote:
>> Add a missing check that could potentially lead to an unarchivable mode being
>> validated.
>>
>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>> ---
>>
>> >From 54fa706f0a5f260a32af5d18b9622ceebb94c12e Mon Sep 17 00:00:00 2001
>> From: Werner Sembach <wse@tuxedocomputers.com>
>> Date: Mon, 3 May 2021 14:42:36 +0200
>> Subject: [PATCH 2/4] Add missing check
> I guess you did something a bit wonky with git format-patch/send-mail?
I have no idea how that timestamp happened, I will check when sending my next patch ^^.
>> ---
>>  drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
>> index 576d3d910d06..ce165ef28e88 100644
>> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
>> @@ -1913,7 +1913,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>>  		clock *= 2;
>>  	}
>>  
>> -	if (drm_mode_is_420_only(&connector->display_info, mode))
>> +	if (connector->ycbcr_420_allowed && drm_mode_is_420_only(&connector->display_info, mode))
> This one shouldn't be necessary. drm_mode_validate_ycbcr420() has
> already checked it for us.
I wasn't aware of drm_mode_validate_ycbcr420, thanks for the hint. In the "420_also"-patch I change drm_mode_is_420_only to drm_mode_is_420 (helper function: _only + _also), which is not checked by drm_mode_validate_ycbcr420. I can add this check to that patch, since its only required then.
>>  		clock /= 2;
>>  
>>  	status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink);
>> -- 
>> 2.25.1

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

[Werner Sembach]

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@tuxedocomputers.com>
---

From 57e42ec6e34ac32da29eb7bc3c691cbeb2534396 Mon Sep 17 00:00:00 2001
From: Werner Sembach <wse@tuxedocomputers.com>
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;
+
+	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;
+
+	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

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2556 bytes --]

Hi Werner,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20210503]
[cannot apply to v5.12]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Werner-Sembach/drm-i915-display-Try-YCbCr420-color-when-RGB-fails/20210504-022344
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-m021-20210503 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/8ff372234cbfe66eb5a59c2cda6a44961f5a9266
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Werner-Sembach/drm-i915-display-Try-YCbCr420-color-when-RGB-fails/20210504-022344
        git checkout 8ff372234cbfe66eb5a59c2cda6a44961f5a9266
        # save the attached .config to linux build tree
        make W=1 W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_hdmi.c:2108:5: error: no previous prototype for 'intel_hdmi_compute_output_format' [-Werror=missing-prototypes]
    2108 | int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors


vim +/intel_hdmi_compute_output_format +2108 drivers/gpu/drm/i915/display/intel_hdmi.c

  2107	
> 2108	int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
  2109					     struct intel_crtc_state *crtc_state,
  2110					     const struct drm_connector_state *conn_state)
  2111	{
  2112		const struct drm_connector *connector = conn_state->connector;
  2113		const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
  2114		int ret;
  2115	
  2116		if (connector->ycbcr_420_allowed && drm_mode_is_420_only(&connector->display_info, adjusted_mode))
  2117			crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
  2118		else
  2119			crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB;
  2120	
  2121		ret = intel_hdmi_compute_clock(encoder, crtc_state);
  2122	
  2123		return ret;
  2124	}
  2125	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35927 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3115 bytes --]

Hi Werner,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20210503]
[cannot apply to v5.12]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Werner-Sembach/drm-i915-display-Try-YCbCr420-color-when-RGB-fails/20210504-022344
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-r025-20210503 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8f5a2a5836cc8e4c1def2bdeb022e7b496623439)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/8ff372234cbfe66eb5a59c2cda6a44961f5a9266
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Werner-Sembach/drm-i915-display-Try-YCbCr420-color-when-RGB-fails/20210504-022344
        git checkout 8ff372234cbfe66eb5a59c2cda6a44961f5a9266
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_hdmi.c:2108:5: error: no previous prototype for function 'intel_hdmi_compute_output_format' [-Werror,-Wmissing-prototypes]
   int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
       ^
   drivers/gpu/drm/i915/display/intel_hdmi.c:2108:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
   ^
   static 
   1 error generated.


vim +/intel_hdmi_compute_output_format +2108 drivers/gpu/drm/i915/display/intel_hdmi.c

  2107	
> 2108	int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
  2109					     struct intel_crtc_state *crtc_state,
  2110					     const struct drm_connector_state *conn_state)
  2111	{
  2112		const struct drm_connector *connector = conn_state->connector;
  2113		const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
  2114		int ret;
  2115	
  2116		if (connector->ycbcr_420_allowed && drm_mode_is_420_only(&connector->display_info, adjusted_mode))
  2117			crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
  2118		else
  2119			crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB;
  2120	
  2121		ret = intel_hdmi_compute_clock(encoder, crtc_state);
  2122	
  2123		return ret;
  2124	}
  2125	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29693 bytes --]

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@tuxedocomputers.com>
> ---
> 
> >From 57e42ec6e34ac32da29eb7bc3c691cbeb2534396 Mon Sep 17 00:00:00 2001
> From: Werner Sembach <wse@tuxedocomputers.com>
> 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

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@tuxedocomputers.com>
>> ---
>>
>> >From 57e42ec6e34ac32da29eb7bc3c691cbeb2534396 Mon Sep 17 00:00:00 2001
>> From: Werner Sembach <wse@tuxedocomputers.com>
>> 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

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

[Ville Syrjälä]

On Wed, May 05, 2021 at 11:54:35AM +0200, Werner Sembach wrote:
> 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@tuxedocomputers.com>
> >> ---
> >>
> >> >From 57e42ec6e34ac32da29eb7bc3c691cbeb2534396 Mon Sep 17 00:00:00 2001
> >> From: Werner Sembach <wse@tuxedocomputers.com>
> >> 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."

I would just put it into the "user has decided to override the mode and
gets to keep both pieces if it breaks". Typical users would not hit that
since they will only use modes reported by the connector as supported.

So I think the RGB fallback is totally in line with existing behaviour
of the driver. We have other cases where we just ignore the reported
limits of the display if the user overrides the mode manually.

-- 
Ville Syrjälä
Intel

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

[Werner Sembach]

Am 05.05.21 um 14:15 schrieb Ville Syrjälä:
> On Wed, May 05, 2021 at 11:54:35AM +0200, Werner Sembach wrote:
>> 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@tuxedocomputers.com>
>>>> ---
>>>>
>>>> >From 57e42ec6e34ac32da29eb7bc3c691cbeb2534396 Mon Sep 17 00:00:00 2001
>>>> From: Werner Sembach <wse@tuxedocomputers.com>
>>>> 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."
> I would just put it into the "user has decided to override the mode and
> gets to keep both pieces if it breaks". Typical users would not hit that
> since they will only use modes reported by the connector as supported.
>
> So I think the RGB fallback is totally in line with existing behaviour
> of the driver. We have other cases where we just ignore the reported
> limits of the display if the user overrides the mode manually.
>
Did I get you right that "connector->ycbcr_420_allowed" is a user setting and not automatically filled configuration depending on hardware capabilities?

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

[Ville Syrjälä]

On Wed, May 05, 2021 at 03:02:53PM +0200, Werner Sembach wrote:
> 
> Am 05.05.21 um 14:15 schrieb Ville Syrjälä:
> > On Wed, May 05, 2021 at 11:54:35AM +0200, Werner Sembach wrote:
> >> 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@tuxedocomputers.com>
> >>>> ---
> >>>>
> >>>> >From 57e42ec6e34ac32da29eb7bc3c691cbeb2534396 Mon Sep 17 00:00:00 2001
> >>>> From: Werner Sembach <wse@tuxedocomputers.com>
> >>>> 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."
> > I would just put it into the "user has decided to override the mode and
> > gets to keep both pieces if it breaks". Typical users would not hit that
> > since they will only use modes reported by the connector as supported.
> >
> > So I think the RGB fallback is totally in line with existing behaviour
> > of the driver. We have other cases where we just ignore the reported
> > limits of the display if the user overrides the mode manually.
> >
> Did I get you right that "connector->ycbcr_420_allowed" is a user setting and not automatically filled configuration depending on hardware capabilities?

No, ycbcr_420_allowed is an automatic thing. But the user can
manually force the display mode to be whatever they want.

So we could have a case where the user forces a mode which the display
claims needs 4:2:0 but the GPU does not support 4:2:0 output. In that
case we could either reject it or just try to output it as RGB anyway.
The current policy for most things like this is "user knows best".
And sometimes they really do know best since some displays can in
fact do things they claim to not support.

-- 
Ville Syrjälä
Intel

[PATCH 4/4] Use YCbCr420 as fallback when RGB fails

[Werner Sembach]

When encoder validation of a display mode fails, retry with less bandwidth
heavy YCbCr420 color mode, if available. This enables some HDMI 1.4 setups
to support 4k60Hz output, which previously failed silently.

AMDGPU had nearly the exact same issue. This problem description is
therefore copied from my commit message of the AMDGPU patch.

On some setups, while the monitor and the gpu support display modes with
pixel clocks of up to 600MHz, the link encoder might not. This prevents
YCbCr444 and RGB encoding for 4k60Hz, but YCbCr420 encoding might still be
possible. However, which color mode is used is decided before the link
encoder capabilities are checked. This patch fixes the problem by retrying
to find a display mode with YCbCr420 enforced and using it, if it is
valid.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
---

From 4ea0c8839b47e846d46c613e38af475231994f0f Mon Sep 17 00:00:00 2001
From: Werner Sembach <wse@tuxedocomputers.com>
Date: Mon, 3 May 2021 16:23:17 +0200
Subject: [PATCH 4/4] Use YCbCr420 as fallback when RGB fails

---
 drivers/gpu/drm/i915/display/intel_hdmi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index e2553ac6fd13..20c800f2ed60 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -1913,7 +1913,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
 		clock *= 2;
 	}
 
-	if (connector->ycbcr_420_allowed && drm_mode_is_420_only(&connector->display_info, mode))
+	if (connector->ycbcr_420_allowed && drm_mode_is_420(&connector->display_info, mode))
 		clock /= 2;
 
 	status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink);
@@ -2119,6 +2119,14 @@ int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
 		crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB;
 
 	ret = intel_hdmi_compute_clock(encoder, crtc_state);
+	if (ret) {
+		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_YCBCR420 ||
+				connector->ycbcr_420_allowed ||
+				drm_mode_is_420_also(&connector->display_info, adjusted_mode)) {
+			crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
+			ret = intel_hdmi_compute_clock(encoder, crtc_state);
+		}
+	}
 
 	return ret;
 }
-- 
2.25.1

Re: [PATCH 4/4] Use YCbCr420 as fallback when RGB fails

[Ville Syrjälä]

On Mon, May 03, 2021 at 08:21:48PM +0200, Werner Sembach wrote:
> When encoder validation of a display mode fails, retry with less bandwidth
> heavy YCbCr420 color mode, if available. This enables some HDMI 1.4 setups
> to support 4k60Hz output, which previously failed silently.
> 
> AMDGPU had nearly the exact same issue. This problem description is
> therefore copied from my commit message of the AMDGPU patch.
> 
> On some setups, while the monitor and the gpu support display modes with
> pixel clocks of up to 600MHz, the link encoder might not. This prevents
> YCbCr444 and RGB encoding for 4k60Hz, but YCbCr420 encoding might still be
> possible. However, which color mode is used is decided before the link
> encoder capabilities are checked. This patch fixes the problem by retrying
> to find a display mode with YCbCr420 enforced and using it, if it is
> valid.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
> 
> >From 4ea0c8839b47e846d46c613e38af475231994f0f Mon Sep 17 00:00:00 2001
> From: Werner Sembach <wse@tuxedocomputers.com>
> Date: Mon, 3 May 2021 16:23:17 +0200
> Subject: [PATCH 4/4] Use YCbCr420 as fallback when RGB fails
> 
> ---
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index e2553ac6fd13..20c800f2ed60 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -1913,7 +1913,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>  		clock *= 2;
>  	}
>  
> -	if (connector->ycbcr_420_allowed && drm_mode_is_420_only(&connector->display_info, mode))
> +	if (connector->ycbcr_420_allowed && drm_mode_is_420(&connector->display_info, mode))
>  		clock /= 2;

This is too early. We want to keep clock as is for checking whether RGB
output is possible with 420_also modes.

So the structure you had in your original patch was the correct way to
go about it. Which I think was something along the lines of:

if (420_only)
	clock /= 2;

status = intel_hdmi_mode_clock_valid()
if (status != OK) {
	if (420_only || !420_also || !420_allowed)
		return status;
	
	clock /= 2;
	status = intel_hdmi_mode_clock_valid()
}


>  
>  	status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink);
> @@ -2119,6 +2119,14 @@ int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
>  		crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB;
>  
>  	ret = intel_hdmi_compute_clock(encoder, crtc_state);
> +	if (ret) {
> +		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_YCBCR420 ||
> +				connector->ycbcr_420_allowed ||
> +				drm_mode_is_420_also(&connector->display_info, adjusted_mode)) {

That needs s/||/&&/ or we flip the conditions around to:

if (ret) {
	if (output_format == 420 || !420_allowed || !420_also)
		return ret;

	output_format = 420;
	...
}

which would have the benefit of avoiding the extra indent level.

> +			crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
> +			ret = intel_hdmi_compute_clock(encoder, crtc_state);
> +		}
> +	}
>  
>  	return ret;
>  }
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [PATCH 4/4] Use YCbCr420 as fallback when RGB fails

[Werner Sembach]

Am 04.05.21 um 12:08 schrieb Ville Syrjälä:
> On Mon, May 03, 2021 at 08:21:48PM +0200, Werner Sembach wrote:
>> When encoder validation of a display mode fails, retry with less bandwidth
>> heavy YCbCr420 color mode, if available. This enables some HDMI 1.4 setups
>> to support 4k60Hz output, which previously failed silently.
>>
>> AMDGPU had nearly the exact same issue. This problem description is
>> therefore copied from my commit message of the AMDGPU patch.
>>
>> On some setups, while the monitor and the gpu support display modes with
>> pixel clocks of up to 600MHz, the link encoder might not. This prevents
>> YCbCr444 and RGB encoding for 4k60Hz, but YCbCr420 encoding might still be
>> possible. However, which color mode is used is decided before the link
>> encoder capabilities are checked. This patch fixes the problem by retrying
>> to find a display mode with YCbCr420 enforced and using it, if it is
>> valid.
>>
>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>> ---
>>
>> >From 4ea0c8839b47e846d46c613e38af475231994f0f Mon Sep 17 00:00:00 2001
>> From: Werner Sembach <wse@tuxedocomputers.com>
>> Date: Mon, 3 May 2021 16:23:17 +0200
>> Subject: [PATCH 4/4] Use YCbCr420 as fallback when RGB fails
>>
>> ---
>>  drivers/gpu/drm/i915/display/intel_hdmi.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
>> index e2553ac6fd13..20c800f2ed60 100644
>> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
>> @@ -1913,7 +1913,7 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>>  		clock *= 2;
>>  	}
>>  
>> -	if (connector->ycbcr_420_allowed && drm_mode_is_420_only(&connector->display_info, mode))
>> +	if (connector->ycbcr_420_allowed && drm_mode_is_420(&connector->display_info, mode))
>>  		clock /= 2;
> This is too early. We want to keep clock as is for checking whether RGB
> output is possible with 420_also modes.
>
> So the structure you had in your original patch was the correct way to
> go about it. Which I think was something along the lines of:
>
> if (420_only)
> 	clock /= 2;
>
> status = intel_hdmi_mode_clock_valid()
> if (status != OK) {
> 	if (420_only || !420_also || !420_allowed)
> 		return status;
> 	
> 	clock /= 2;
> 	status = intel_hdmi_mode_clock_valid()
> }
Does it make a difference?

In case !420_allowed only rgb is ever tested
In case 420_allowed && 420_only only 420 is ever tested
In case 420_allowed && 420_also the return value of the rgb test is discarded anyways
>
>>  
>>  	status = intel_hdmi_mode_clock_valid(hdmi, clock, has_hdmi_sink);
>> @@ -2119,6 +2119,14 @@ int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
>>  		crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB;
>>  
>>  	ret = intel_hdmi_compute_clock(encoder, crtc_state);
>> +	if (ret) {
>> +		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_YCBCR420 ||
>> +				connector->ycbcr_420_allowed ||
>> +				drm_mode_is_420_also(&connector->display_info, adjusted_mode)) {
> That needs s/||/&&/ or we flip the conditions around to:
>
> if (ret) {
> 	if (output_format == 420 || !420_allowed || !420_also)
> 		return ret;
>
> 	output_format = 420;
> 	...
> }
>
> which would have the benefit of avoiding the extra indent level.
>
>> +			crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
>> +			ret = intel_hdmi_compute_clock(encoder, crtc_state);
>> +		}
>> +	}
>>  
>>  	return ret;
>>  }
>> -- 
>> 2.25.1