Re: [Intel-gfx] [PATCH 13/13] drm/i915: Implement static DRRS

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 10 Mar 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Let's start supporting static DRRS by trying to match the refresh
> rate the user has requested, assuming the panel supports suitable
> timings.
>
> For now we stick to just our current two timings:
> - fixed_mode: the panel's preferred mode
> - downclock_mode: the lowest refresh rate mode we found
> Some panels may support more timings than that, but we'll
> have to convert our fixed_mode/downclock_mode pointers
> into a full list before we can handle that.

So this won't fix panels with, say, a 120 Hz mode that isn't the panel's
preferred mode?

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c    | 11 +++++++++++
>  drivers/gpu/drm/i915/display/intel_drrs.c  |  8 +++++---
>  drivers/gpu/drm/i915/display/intel_panel.c | 20 ++++++++++++++++++--
>  3 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index af659320c02e..9bd958377a54 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4599,6 +4599,17 @@ static int intel_dp_get_modes(struct drm_connector *connector)
>  			num_modes++;
>  		}
>  	}
> +	if (intel_dp_is_edp(intel_attached_dp(intel_connector)) &&
> +	    intel_connector->panel.downclock_mode) {

I thought you wanted abstract access to connector->panel.downclock_mode
in the previous patch.

> +		struct drm_display_mode *mode;
> +
> +		mode = drm_mode_duplicate(connector->dev,
> +					  intel_connector->panel.downclock_mode);
> +		if (mode) {
> +			drm_mode_probed_add(connector, mode);
> +			num_modes++;
> +		}
> +	}
>  
>  	if (num_modes)
>  		return num_modes;
> diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
> index 5b2eb55c1340..dc1733c9abab 100644
> --- a/drivers/gpu/drm/i915/display/intel_drrs.c
> +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
> @@ -383,7 +383,7 @@ intel_drrs_init(struct intel_connector *connector,
>  		return NULL;
>  	}
>  
> -	if (dev_priv->vbt.drrs_type != DRRS_TYPE_SEAMLESS) {
> +	if (dev_priv->vbt.drrs_type == DRRS_TYPE_NONE) {
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "[CONNECTOR:%d:%s] DRRS not supported according to VBT\n",
>  			    connector->base.base.id, connector->base.name);
> @@ -399,8 +399,10 @@ intel_drrs_init(struct intel_connector *connector,
>  	}
>  
>  	drm_dbg_kms(&dev_priv->drm,
> -		    "[CONNECTOR:%d:%s] seamless DRRS supported\n",
> -		    connector->base.base.id, connector->base.name);
> +		    "[CONNECTOR:%d:%s] %s DRRS supported\n",
> +		    connector->base.base.id, connector->base.name,
> +		    dev_priv->vbt.drrs_type == DRRS_TYPE_SEAMLESS ?
> +		    "seamless" : "static");
>  
>  	return downclock_mode;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index 127ad9643360..6ddbb69dcfdc 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -49,14 +49,30 @@ const struct drm_display_mode *
>  intel_panel_fixed_mode(struct intel_connector *connector,
>  		       const struct drm_display_mode *mode)
>  {
> -	return connector->panel.fixed_mode;
> +	const struct drm_display_mode *fixed_mode = connector->panel.fixed_mode;
> +	const struct drm_display_mode *downclock_mode = connector->panel.downclock_mode;
> +
> +	/* pick the one that is closer in terms of vrefresh */
> +	/* FIXME make this a a list of modes so we can have more than two */

Indeed feels a bit hackish...

This being the last and arguably fairly simple part of the series, I do
wonder about the pros and cons of merging this as an interim
solution.

IIUC if the user wanted to do static/manual drrs while seamless is
supported, this does not disable seamless when the user chooses
preferred vrefresh and a downclock vrefresh is available. So you can
choose lower vrefresh and stay there, but you can't choose higher
vrefresh and stay there.

> +	if (fixed_mode && downclock_mode &&
> +	    abs(drm_mode_vrefresh(downclock_mode) - drm_mode_vrefresh(mode)) <
> +	    abs(drm_mode_vrefresh(fixed_mode) - drm_mode_vrefresh(mode)))
> +		return downclock_mode;
> +	else
> +		return fixed_mode;
>  }
>  
>  const struct drm_display_mode *
>  intel_panel_downclock_mode(struct intel_connector *connector,
>  			   const struct drm_display_mode *fixed_mode)
>  {
> -	return connector->panel.downclock_mode;
> +	const struct drm_display_mode *downclock_mode = connector->panel.downclock_mode;
> +
> +	if (downclock_mode &&
> +	    drm_mode_vrefresh(downclock_mode) < drm_mode_vrefresh(fixed_mode))

With this use, I think the fixed_mode name here is a bit misleading. The
downclock mode will always have a vrefresh lower than the fixed
mode. But you're not always passing the fixed mode, right?

> +		return downclock_mode;
> +	else
> +		return NULL;
>  }
>  
>  int intel_panel_compute_config(struct intel_connector *connector,

-- 
Jani Nikula, Intel Open Source Graphics Center