Am 07.05.21 um 19:52 schrieb Ville Syrjälä:
> On Fri, May 07, 2021 at 10:49:02AM +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.
>>
>> A slight change in behaviour occurs with this patch: If YCbCr420 is not
>> allowed but display is YCbCr420 only it no longer fails, but just prints
>> an error and tries to fallback on RGB.
>>
>> Signed-off-by: Werner Sembach <w...@tuxedocomputers.com>
>> ---
>> drivers/gpu/drm/i915/display/intel_hdmi.c | 65 ++++++++++++-----------
>> 1 file changed, 34 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
>> b/drivers/gpu/drm/i915/display/intel_hdmi.c
>> index 576d3d910d06..9f3da72dabee 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,30 @@ static bool intel_hdmi_has_audio(struct intel_encoder
>> *encoder,
>> return intel_conn_state->force_audio == HDMI_AUDIO_ON;
>> }
>>
>> +static int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
>> + 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;
>> + int ret;
>> + bool ycbcr_420_only;
>> +
>> + ycbcr_420_only = drm_mode_is_420_only(&connector->display_info,
>> adjusted_mode);
>> + if (connector->ycbcr_420_allowed && ycbcr_420_only) {
>> + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
>> + } else {
>> + if (!connector->ycbcr_420_allowed && ycbcr_420_only)
>> + drm_err(&i915->drm, "Display only supports YCbCr420
>> output, but connector does not allow it. Fallback to RGB, but this will
>> likely fail.\n");
> We can't let the user spam dmesg with errors freely. So this needs
> to be a drm_dbg_kms(). Also a bit long, so going to annoyingly wrap
> always. Could maybe shorten a bit to something like:
> "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB."
Done, but I don't have time anymore to test it today. I will test and submit v7
on Monday.
>
> With that sorted, and the intel_hdmi_port_clock() stuff restored,
> I believe this series is good to go.
>
> I think you confused our CI by replying to the old patch with a whole
> new series. It can generally deal with a whole new series as a new
> thread or replies to individual patches with updated versions of
> exactly that patch, but not full series as a reply to a patch.
> So I suggest just posting the final versions as a new series. Thanks.
>
>> + 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 +2153,25 @@ 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;
>>
>> + if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) {
>> + 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
