| Message ID | 20170831211750.27890-1-radhakrishna.sripada@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@gmail.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Clinton Taylor <clinton.a.taylor@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@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? 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. > + > mutex_lock(&dev_priv->drrs.mutex); > if (WARN_ON(dev_priv->drrs.dp)) { > DRM_ERROR("DRRS already enabled\n");
On Sat, 09 Sep 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@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@gmail.com> >> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Clinton Taylor <clinton.a.taylor@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@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. 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. BR, Jani. > > >> + >> mutex_lock(&dev_priv->drrs.mutex); >> if (WARN_ON(dev_priv->drrs.dp)) { >> DRM_ERROR("DRRS already enabled\n");
On Tue, 12 Sep 2017, Jani Nikula <jani.nikula@intel.com> wrote: > On Sat, 09 Sep 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@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@gmail.com> >>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Cc: Clinton Taylor <clinton.a.taylor@intel.com> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@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. > > BR, > Jani. > >> >> >>> + >>> mutex_lock(&dev_priv->drrs.mutex); >>> if (WARN_ON(dev_priv->drrs.dp)) { >>> DRM_ERROR("DRRS already enabled\n");
> -----Original Message----- > From: Nikula, Jani > Sent: Tuesday, September 12, 2017 7:18 AM > To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>; Sripada, > Radhakrishna <radhakrishna.sripada@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; > nicholas.stommel@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@intel.com> wrote: > > On Sat, 09 Sep 2017, "Pandiyan, Dhinakaran" > <dhinakaran.pandiyan@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@gmail.com> > >>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > >>> Cc: Jani Nikula <jani.nikula@intel.com> > >>> Cc: Clinton Taylor <clinton.a.taylor@intel.com> > >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > >>> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@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
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; + } + mutex_lock(&dev_priv->drrs.mutex); if (WARN_ON(dev_priv->drrs.dp)) { DRM_ERROR("DRRS already enabled\n");
Some platforms donot support PSR and DRRS simultaneously. 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@gmail.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Clinton Taylor <clinton.a.taylor@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)