On Fri, 27 May 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > It seem that when dealing with VRR capable eDP panels we need > to accept fixed modes with variable vblank length (which is what > VRR varies dynamically). Typically the preferred mode seems to be > a non-VRR more (lowish dotclock/refresh rate + short vblank). > > We also have examples where it looks like even the hblank length > is a bit different between the preferred mode vs. VRR mode(s). > So let's just accept anything that has matching hdisp+vdisp+flags. > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/125 > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 3 +- > drivers/gpu/drm/i915/display/intel_lvds.c | 3 +- > drivers/gpu/drm/i915/display/intel_panel.c | 48 ++++++++++++++++------ > drivers/gpu/drm/i915/display/intel_panel.h | 3 +- > drivers/gpu/drm/i915/display/intel_sdvo.c | 2 +- > 5 files changed, 43 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 1bc1f6458e81..b8e2d3cd4d68 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -5217,7 +5217,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > IS_ERR(edid) ? NULL : edid); > > intel_panel_add_edid_fixed_modes(intel_connector, > - intel_connector->panel.vbt.drrs_type != DRRS_TYPE_NONE); > + intel_connector->panel.vbt.drrs_type != DRRS_TYPE_NONE, > + intel_vrr_is_capable(intel_connector)); > > /* MSO requires information from the EDID */ > intel_edp_mso_init(intel_dp); > diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c > index 595f03343939..e55802b45461 100644 > --- a/drivers/gpu/drm/i915/display/intel_lvds.c > +++ b/drivers/gpu/drm/i915/display/intel_lvds.c > @@ -972,7 +972,8 @@ void intel_lvds_init(struct drm_i915_private *dev_priv) > > /* Try EDID first */ > intel_panel_add_edid_fixed_modes(intel_connector, > - intel_connector->panel.vbt.drrs_type != DRRS_TYPE_NONE); > + intel_connector->panel.vbt.drrs_type != DRRS_TYPE_NONE, > + false); > > /* Failed to get EDID, what about VBT? */ > if (!intel_panel_preferred_fixed_mode(intel_connector)) > diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c > index f55e4eafd74e..963b24293b50 100644 > --- a/drivers/gpu/drm/i915/display/intel_panel.c > +++ b/drivers/gpu/drm/i915/display/intel_panel.c > @@ -71,6 +71,27 @@ intel_panel_fixed_mode(struct intel_connector *connector, > return best_mode; > } > > +static bool is_alt_drrs_mode(const struct drm_display_mode *mode, > + const struct drm_display_mode *preferred_mode) > +{ > + return drm_mode_match(mode, preferred_mode, > + DRM_MODE_MATCH_TIMINGS | > + DRM_MODE_MATCH_FLAGS | > + DRM_MODE_MATCH_3D_FLAGS) && > + mode->clock != preferred_mode->clock; > +} > + > +static bool is_alt_vrr_mode(const struct drm_display_mode *mode, > + const struct drm_display_mode *preferred_mode) > +{ > + return drm_mode_match(mode, preferred_mode, > + DRM_MODE_MATCH_FLAGS | > + DRM_MODE_MATCH_3D_FLAGS) && > + mode->hdisplay == preferred_mode->hdisplay && > + mode->vdisplay == preferred_mode->vdisplay && > + mode->clock != preferred_mode->clock; > +} > + > const struct drm_display_mode * > intel_panel_downclock_mode(struct intel_connector *connector, > const struct drm_display_mode *adjusted_mode) > @@ -83,7 +104,8 @@ intel_panel_downclock_mode(struct intel_connector *connector, > list_for_each_entry(fixed_mode, &connector->panel.fixed_modes, head) { > int vrefresh = drm_mode_vrefresh(fixed_mode); > > - if (vrefresh >= min_vrefresh && vrefresh < max_vrefresh) { > + if (is_alt_drrs_mode(fixed_mode, adjusted_mode) && > + vrefresh >= min_vrefresh && vrefresh < max_vrefresh) { > max_vrefresh = vrefresh; > best_mode = fixed_mode; > } > @@ -151,16 +173,17 @@ int intel_panel_compute_config(struct intel_connector *connector, > } > > static bool is_alt_fixed_mode(const struct drm_display_mode *mode, > - const struct drm_display_mode *preferred_mode) > + const struct drm_display_mode *preferred_mode, > + bool has_vrr) > { > - return drm_mode_match(mode, preferred_mode, > - DRM_MODE_MATCH_TIMINGS | > - DRM_MODE_MATCH_FLAGS | > - DRM_MODE_MATCH_3D_FLAGS) && > - mode->clock != preferred_mode->clock; > + if (has_vrr) > + return is_alt_vrr_mode(mode, preferred_mode); > + else > + return is_alt_drrs_mode(mode, preferred_mode); This assumes is_alt_drrs_mode() accepts a subset of is_alt_vrr_mode(). Which is true and fine. But maybe deserves a comment, as otherwise I believe we should try both if the vrr one fails. A really defensive solution would be to WARN_ON(!is_alt_vrr_mode()) in is_alt_drrs_mode() if everything else is true. Maybe overkill. > } > > -static void intel_panel_add_edid_alt_fixed_modes(struct intel_connector *connector) > +static void intel_panel_add_edid_alt_fixed_modes(struct intel_connector *connector, > + bool has_vrr) > { > struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > const struct drm_display_mode *preferred_mode = > @@ -168,7 +191,7 @@ static void intel_panel_add_edid_alt_fixed_modes(struct intel_connector *connect > struct drm_display_mode *mode, *next; > > list_for_each_entry_safe(mode, next, &connector->base.probed_modes, head) { > - if (!is_alt_fixed_mode(mode, preferred_mode)) > + if (!is_alt_fixed_mode(mode, preferred_mode, has_vrr)) > continue; Should we perhaps debug log if it was vrr or drrs? Anyway, with the comment added, Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx> The rest is up to you. > > drm_dbg_kms(&dev_priv->drm, > @@ -226,11 +249,12 @@ static void intel_panel_destroy_probed_modes(struct intel_connector *connector) > } > } > > -void intel_panel_add_edid_fixed_modes(struct intel_connector *connector, bool has_drrs) > +void intel_panel_add_edid_fixed_modes(struct intel_connector *connector, > + bool has_drrs, bool has_vrr) > { > intel_panel_add_edid_preferred_mode(connector); > - if (intel_panel_preferred_fixed_mode(connector) && has_drrs) > - intel_panel_add_edid_alt_fixed_modes(connector); > + if (intel_panel_preferred_fixed_mode(connector) && (has_drrs || has_vrr)) > + intel_panel_add_edid_alt_fixed_modes(connector, has_vrr); > intel_panel_destroy_probed_modes(connector); > } > > diff --git a/drivers/gpu/drm/i915/display/intel_panel.h b/drivers/gpu/drm/i915/display/intel_panel.h > index 2e32bb728beb..b087c0c3cc6d 100644 > --- a/drivers/gpu/drm/i915/display/intel_panel.h > +++ b/drivers/gpu/drm/i915/display/intel_panel.h > @@ -40,7 +40,8 @@ int intel_panel_fitting(struct intel_crtc_state *crtc_state, > const struct drm_connector_state *conn_state); > int intel_panel_compute_config(struct intel_connector *connector, > struct drm_display_mode *adjusted_mode); > -void intel_panel_add_edid_fixed_modes(struct intel_connector *connector, bool has_drrs); > +void intel_panel_add_edid_fixed_modes(struct intel_connector *connector, > + bool has_drrs, bool has_vrr); > void intel_panel_add_vbt_lfp_fixed_mode(struct intel_connector *connector); > void intel_panel_add_vbt_sdvo_fixed_mode(struct intel_connector *connector); > void intel_panel_add_encoder_fixed_mode(struct intel_connector *connector, > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c > index d9de2c4d67a7..2b78a790e1b6 100644 > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c > @@ -2911,7 +2911,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device) > > if (!intel_panel_preferred_fixed_mode(intel_connector)) { > intel_ddc_get_modes(connector, &intel_sdvo->ddc); > - intel_panel_add_edid_fixed_modes(intel_connector, false); > + intel_panel_add_edid_fixed_modes(intel_connector, false, false); > } > > intel_panel_init(intel_connector); -- Jani Nikula, Intel Open Source Graphics Center
