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

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

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