[Intel-gfx] [PATCH v2] drm/i915: Disable DRRS when PSR is enabled

[Sripada, Radhakrishna]

> -----Original Message-----
> From: Nikula, Jani
> Sent: Tuesday, September 12, 2017 7:18 AM
> To: Pandiyan, Dhinakaran <dhinakaran.pandiyan at intel.com>; Sripada,
> Radhakrishna <radhakrishna.sripada at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi at intel.com>;
> nicholas.stommel at gmail.com
> Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Disable DRRS when PSR is
> enabled
> 
> On Tue, 12 Sep 2017, Jani Nikula <jani.nikula at intel.com> wrote:
> > On Sat, 09 Sep 2017, "Pandiyan, Dhinakaran"
> <dhinakaran.pandiyan at intel.com> wrote:
> >> On Thu, 2017-08-31 at 14:17 -0700, Radhakrishna Sripada wrote:
> >>> Some platforms donot support PSR and DRRS simultaneously.
> >>
> >> I could not verify which platforms support PSR + DRRS and which don't.
> >> But, seems safe to have DRRS disabled for all platforms when PSR is
> >> enabled.
> >>
> >>
> >>
> >>> Visual artifacts and flickering were reported on BDW HP Spectre
> >>> x360 Convertible. Deferring to PSR when both PSR and DRRS are
> >>> supported by the panel.
> >>>
> >>> V2: Minor code-style changes suggested by Rodrigo
> >>>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101111
> >>> Cc: Nicholas Stommel <nicholas.stommel at gmail.com>
> >>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> >>> Cc: Jani Nikula <jani.nikula at intel.com>
> >>> Cc: Clinton Taylor <clinton.a.taylor at intel.com>
> >>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> >>> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_dp.c | 10 +++++-----
> >>>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >>> b/drivers/gpu/drm/i915/intel_dp.c index 887953c0f495..aa5a69301257
> >>> 100644
> >>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>> @@ -5467,11 +5467,6 @@ static void intel_dp_set_drrs_state(struct
> drm_i915_private *dev_priv,
> >>>  		return;
> >>>  	}
> >>>
> >>> -	/*
> >>> -	 * FIXME: This needs proper synchronization with psr state for some
> >>> -	 * platforms that cannot have PSR and DRRS enabled at the same
> time.
> >>> -	 */
> >>> -
> >>>  	dig_port = dp_to_dig_port(intel_dp);
> >>>  	encoder = &dig_port->base;
> >>>  	intel_crtc = to_intel_crtc(encoder->base.crtc);
> >>> @@ -5555,6 +5550,11 @@ void intel_edp_drrs_enable(struct intel_dp
> *intel_dp,
> >>>  		return;
> >>>  	}
> >>>
> >>> +	if (dev_priv->psr.enabled) {
> >>> +		DRM_DEBUG_KMS("PSR enabled. Disabling DRRS.\n");
> >>> +		return;
> >>> +	}
> >>
> >> So every time a flush/invalidate happens, we end up taking the
> >> drrs.mutex and then returning because dev_priv->drrs.dp is NULL. That
> >> seems unnecessary. Have your considered drrs.type =
> DRRS_NOT_SUPPORTED?
> >
> > That would prevent DRRS testing by disabling PSR via module parameter.
> > I think this is fine.
> 
> I mean, I think the change in this patch is fine, preventing DRRS testing is not
> fine.
> 
> 
> > Although the debug message is misleading; it's "not enabling DRRS",
> > not "disabling DRRS". There's a difference.
> >
> > Side note, dev_priv->drrs.type is redundant and could be replaced with
> > direct use of dev_priv->vbt.drrs_type.
> >
> >> And this solution relies on the ordering that psr_enable() is done
> >> before drrs_enable(), we need a comment in enable_ddi to make a note
> >> of that. A WARN_ON in psr_enable() if drrs is already enabled might
> >> work too.
> >
> > I think a WARN_ON would be fine.

I will add a WARN_ON() and send a v3 of the patch.

-Radhakrishna
> >
> > BR,
> > Jani.
> >
> >>
> >>
> >>> +
> >>>  	mutex_lock(&dev_priv->drrs.mutex);
> >>>  	if (WARN_ON(dev_priv->drrs.dp)) {
> >>>  		DRM_ERROR("DRRS already enabled\n");
> 
> --
> Jani Nikula, Intel Open Source Technology Center