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. > > 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
