On Sat, 09 Sep 2017, "Pandiyan, Dhinakaran" <[email protected]>
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 <[email protected]>
>> Cc: Dhinakaran Pandiyan <[email protected]>
>> Cc: Jani Nikula <[email protected]>
>> Cc: Clinton Taylor <[email protected]>
>> Cc: Rodrigo Vivi <[email protected]>
>> Signed-off-by: Radhakrishna Sripada <[email protected]>
>> ---
>> 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");
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
