On Thu, Mar 10, 2022 at 12:30:06PM +0200, Jani Nikula wrote:
> On Thu, 10 Mar 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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.
This part is a bit different since here we just want to list all
the supported modes. So we just duplicate our whole "fixed mode list"
which for now just comprises of fixed_mode and downclock_mode.
>
> > + 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.
Shrug.
>
> 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.
Disabling the seamless DRRS stuff is IMO an orthogonal issue. And I'm
not sure there is any point in disabling it actually. All the use cases
I can think of would just keep it in the high refresh rate mode all
the time anyway, and thus disabling it wouldn't achieve anything extra.
Also it's an i915 specific feature so coming up an acceptable uapi for
it is perhaps an uphill battle.
> > + 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?
I was thinking of it as 'the thing we chose as our "fixed mode"'. But
we could just as well call it "adjusted_mode" I suppose.
>
> > + return downclock_mode;
> > + else
> > + return NULL;
> > }
> >
> > int intel_panel_compute_config(struct intel_connector *connector,
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Ville Syrjälä
Intel
