> -----Original Message----- > From: Nikula, Jani > Sent: Tuesday, September 12, 2017 7:18 AM > To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@xxxxxxxxx>; Sripada, > Radhakrishna <radhakrishna.sripada@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; > nicholas.stommel@xxxxxxxxx > Subject: Re: [PATCH v2] drm/i915: Disable DRRS when PSR is > enabled > > On Tue, 12 Sep 2017, Jani Nikula <jani.nikula@xxxxxxxxx> wrote: > > On Sat, 09 Sep 2017, "Pandiyan, Dhinakaran" > <dhinakaran.pandiyan@xxxxxxxxx> 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@xxxxxxxxx> > >>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> > >>> Cc: Jani Nikula <jani.nikula@xxxxxxxxx> > >>> Cc: Clinton Taylor <clinton.a.taylor@xxxxxxxxx> > >>> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > >>> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx> > >>> --- > >>> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
