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@xxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> >From 57e42ec6e34ac32da29eb7bc3c691cbeb2534396 Mon Sep 17 00:00:00 2001
>> From: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
>> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
