Re: [PATCH v2] drm/i915: Disable DRRS when PSR is enabled

[Jani Nikula <jani.nikula@intel.com>]

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");

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx