public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2] drm/i915: Disable DRRS when PSR is enabled
@ 2017-08-31 21:17 Radhakrishna Sripada
  2017-08-31 21:38 ` ✓ Fi.CI.BAT: success for drm/i915: Disable DRRS when PSR is enabled (rev2) Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Radhakrishna Sripada @ 2017-08-31 21:17 UTC (permalink / raw)
  To: intel-gfx
  Cc: Jani Nikula, Nicholas Stommel, Dhinakaran Pandiyan, Rodrigo Vivi

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(-)

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Disable DRRS when PSR is enabled (rev2)
  2017-08-31 21:17 [PATCH v2] drm/i915: Disable DRRS when PSR is enabled Radhakrishna Sripada
@ 2017-08-31 21:38 ` Patchwork
  2017-09-01  0:27 ` ✗ Fi.CI.IGT: warning " Patchwork
  2017-09-08 22:02 ` [PATCH v2] drm/i915: Disable DRRS when PSR is enabled Pandiyan, Dhinakaran
  2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2017-08-31 21:38 UTC (permalink / raw)
  To: Radhakrishna Sripada; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Disable DRRS when PSR is enabled (rev2)
URL   : https://patchwork.freedesktop.org/series/29577/
State : success

== Summary ==

Series 29577v2 drm/i915: Disable DRRS when PSR is enabled
https://patchwork.freedesktop.org/api/1.0/series/29577/revisions/2/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test kms_cursor_legacy:
        Subgroup basic-flip-after-cursor-varying-size:
                fail       -> PASS       (fi-hsw-4770) fdo#102402 +1

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#102402 https://bugs.freedesktop.org/show_bug.cgi?id=102402

fi-bdw-5557u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-bdw-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:443s
fi-blb-e6850     total:288  pass:224  dwarn:1   dfail:0   fail:0   skip:63  time:364s
fi-bsw-n3050     total:288  pass:243  dwarn:0   dfail:0   fail:0   skip:45  time:556s
fi-bwr-2160      total:288  pass:184  dwarn:0   dfail:0   fail:0   skip:104 time:253s
fi-bxt-j4205     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:525s
fi-byt-j1900     total:288  pass:254  dwarn:1   dfail:0   fail:0   skip:33  time:522s
fi-byt-n2820     total:288  pass:250  dwarn:1   dfail:0   fail:0   skip:37  time:518s
fi-elk-e7500     total:288  pass:230  dwarn:0   dfail:0   fail:0   skip:58  time:437s
fi-glk-2a        total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:619s
fi-hsw-4770      total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  time:456s
fi-hsw-4770r     total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  time:423s
fi-ilk-650       total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:430s
fi-ivb-3520m     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:508s
fi-ivb-3770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:476s
fi-kbl-7500u     total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  time:517s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:599s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:598s
fi-pnv-d510      total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:526s
fi-skl-6260u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:469s
fi-skl-6700k     total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:535s
fi-skl-6770hq    total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:486s
fi-skl-gvtdvm    total:288  pass:266  dwarn:0   dfail:0   fail:0   skip:22  time:446s
fi-skl-x1585l    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:489s
fi-snb-2520m     total:288  pass:251  dwarn:0   dfail:0   fail:0   skip:37  time:552s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:2   skip:38  time:407s

ccf4ca2d93383fe1a234aba83df9c21400216433 drm-tip: 2017y-08m-31d-18h-37m-46s UTC integration manifest
cba2ebe39662 drm/i915: Disable DRRS when PSR is enabled

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5553/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* ✗ Fi.CI.IGT: warning for drm/i915: Disable DRRS when PSR is enabled (rev2)
  2017-08-31 21:17 [PATCH v2] drm/i915: Disable DRRS when PSR is enabled Radhakrishna Sripada
  2017-08-31 21:38 ` ✓ Fi.CI.BAT: success for drm/i915: Disable DRRS when PSR is enabled (rev2) Patchwork
@ 2017-09-01  0:27 ` Patchwork
  2017-09-08 22:02 ` [PATCH v2] drm/i915: Disable DRRS when PSR is enabled Pandiyan, Dhinakaran
  2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2017-09-01  0:27 UTC (permalink / raw)
  To: Radhakrishna Sripada; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Disable DRRS when PSR is enabled (rev2)
URL   : https://patchwork.freedesktop.org/series/29577/
State : warning

== Summary ==

Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912
Test perf:
        Subgroup polling:
                fail       -> PASS       (shard-hsw) fdo#102252 +1
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-spr-indfb-move:
                pass       -> SKIP       (shard-hsw)
Test kms_chv_cursor_fail:
        Subgroup pipe-C-256x256-right-edge:
                pass       -> SKIP       (shard-hsw)
        Subgroup pipe-C-128x128-top-edge:
                pass       -> SKIP       (shard-hsw)
Test pm_rpm:
        Subgroup legacy-planes:
                pass       -> SKIP       (shard-hsw)
Test kms_plane:
        Subgroup plane-position-hole-dpms-pipe-A-planes:
                pass       -> SKIP       (shard-hsw)
Test kms_cursor_legacy:
        Subgroup basic-flip-after-cursor-legacy:
                pass       -> SKIP       (shard-hsw)

fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-hsw        total:2265 pass:1229 dwarn:0   dfail:0   fail:14  skip:1022 time:9618s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5553/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] drm/i915: Disable DRRS when PSR is enabled
  2017-08-31 21:17 [PATCH v2] drm/i915: Disable DRRS when PSR is enabled Radhakrishna Sripada
  2017-08-31 21:38 ` ✓ Fi.CI.BAT: success for drm/i915: Disable DRRS when PSR is enabled (rev2) Patchwork
  2017-09-01  0:27 ` ✗ Fi.CI.IGT: warning " Patchwork
@ 2017-09-08 22:02 ` Pandiyan, Dhinakaran
  2017-09-12 14:16   ` Jani Nikula
  2 siblings, 1 reply; 7+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-09-08 22:02 UTC (permalink / raw)
  To: Sripada, Radhakrishna
  Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org,
	nicholas.stommel@gmail.com, Vivi, Rodrigo

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");
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] drm/i915: Disable DRRS when PSR is enabled
  2017-09-08 22:02 ` [PATCH v2] drm/i915: Disable DRRS when PSR is enabled Pandiyan, Dhinakaran
@ 2017-09-12 14:16   ` Jani Nikula
  2017-09-12 14:17     ` Jani Nikula
  0 siblings, 1 reply; 7+ messages in thread
From: Jani Nikula @ 2017-09-12 14:16 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran, Sripada, Radhakrishna
  Cc: intel-gfx@lists.freedesktop.org, nicholas.stommel@gmail.com,
	Vivi, Rodrigo

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] drm/i915: Disable DRRS when PSR is enabled
  2017-09-12 14:16   ` Jani Nikula
@ 2017-09-12 14:17     ` Jani Nikula
  2017-09-13 23:32       ` Sripada, Radhakrishna
  0 siblings, 1 reply; 7+ messages in thread
From: Jani Nikula @ 2017-09-12 14:17 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran, Sripada, Radhakrishna
  Cc: intel-gfx@lists.freedesktop.org, nicholas.stommel@gmail.com,
	Vivi, Rodrigo

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] drm/i915: Disable DRRS when PSR is enabled
  2017-09-12 14:17     ` Jani Nikula
@ 2017-09-13 23:32       ` Sripada, Radhakrishna
  0 siblings, 0 replies; 7+ messages in thread
From: Sripada, Radhakrishna @ 2017-09-13 23:32 UTC (permalink / raw)
  To: Nikula, Jani, Pandiyan, Dhinakaran
  Cc: intel-gfx@lists.freedesktop.org, nicholas.stommel@gmail.com,
	Vivi, Rodrigo



> -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-09-13 23:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-31 21:17 [PATCH v2] drm/i915: Disable DRRS when PSR is enabled Radhakrishna Sripada
2017-08-31 21:38 ` ✓ Fi.CI.BAT: success for drm/i915: Disable DRRS when PSR is enabled (rev2) Patchwork
2017-09-01  0:27 ` ✗ Fi.CI.IGT: warning " Patchwork
2017-09-08 22:02 ` [PATCH v2] drm/i915: Disable DRRS when PSR is enabled Pandiyan, Dhinakaran
2017-09-12 14:16   ` Jani Nikula
2017-09-12 14:17     ` Jani Nikula
2017-09-13 23:32       ` Sripada, Radhakrishna

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox