On Thu, 10 Mar 2022, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> 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.
Yeah, no strong opinions, really, but we do need to get the
non-preferred mode high refresh rate panels working sooner rather than
later. So this would just be a hopefully short lived intermediate step.
Forgot to add
Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
as in seems to do what it says on the box.
BR,
Jani.
>
>>
>> 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
--
Jani Nikula, Intel Open Source Graphics Center
