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 <[email protected]>
---
From 4ea0c8839b47e846d46c613e38af475231994f0f Mon Sep 17 00:00:00 2001
From: Werner Sembach <[email protected]>
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
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 <[email protected]>
> ---
>
> >From 4ea0c8839b47e846d46c613e38af475231994f0f Mon Sep 17 00:00:00 2001
> From: Werner Sembach <[email protected]>
> 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
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 <[email protected]>
>> ---
>>
>> >From 4ea0c8839b47e846d46c613e38af475231994f0f Mon Sep 17 00:00:00 2001
>> From: Werner Sembach <[email protected]>
>> 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