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@xxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> >From 4ea0c8839b47e846d46c613e38af475231994f0f Mon Sep 17 00:00:00 2001
>> From: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
>> 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