On Fri, 27 May 2022, Ville Syrjala <ville.syrj...@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> 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.syrj...@linux.intel.com>
> ---
> 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.nik...@intel.com>
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
