Am 05.05.21 um 14:15 schrieb Ville Syrjälä: > On Wed, May 05, 2021 at 11:54:35AM +0200, Werner Sembach wrote: >> 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." > I would just put it into the "user has decided to override the mode and > gets to keep both pieces if it breaks". Typical users would not hit that > since they will only use modes reported by the connector as supported. > > So I think the RGB fallback is totally in line with existing behaviour > of the driver. We have other cases where we just ignore the reported > limits of the display if the user overrides the mode manually. > Did I get you right that "connector->ycbcr_420_allowed" is a user setting and not automatically filled configuration depending on hardware capabilities? _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel