* [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR
@ 2018-10-05 23:35 José Roberto de Souza
2018-10-05 23:35 ` [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen José Roberto de Souza
` (6 more replies)
0 siblings, 7 replies; 18+ messages in thread
From: José Roberto de Souza @ 2018-10-05 23:35 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi
It should always wait for idle state when disabling PSR because PSR
could be inactive due a call to intel_psr_exit() and while PSR is
still being disabled asynchronously userspace could change the
modeset causing a call to psr_disable() that will not wait for PSR
idle and then PSR will be enabled again while PSR is still not idle.
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_psr.c | 43 +++++++++++++++-----------------
1 file changed, 20 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 423cdf84059c..cd9a60d1efa1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -661,40 +661,37 @@ static void
intel_psr_disable_source(struct intel_dp *intel_dp)
{
struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+ i915_reg_t psr_status;
+ u32 psr_status_mask;
if (dev_priv->psr.active) {
- i915_reg_t psr_status;
- u32 psr_status_mask;
-
- if (dev_priv->psr.psr2_enabled) {
- psr_status = EDP_PSR2_STATUS;
- psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
-
+ if (dev_priv->psr.psr2_enabled)
I915_WRITE(EDP_PSR2_CTL,
- I915_READ(EDP_PSR2_CTL) &
- ~(EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE));
-
- } else {
- psr_status = EDP_PSR_STATUS;
- psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
-
+ I915_READ(EDP_PSR2_CTL) & ~EDP_PSR2_ENABLE);
+ else
I915_WRITE(EDP_PSR_CTL,
I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
- }
-
- /* Wait till PSR is idle */
- if (intel_wait_for_register(dev_priv,
- psr_status, psr_status_mask, 0,
- 2000))
- DRM_ERROR("Timed out waiting for PSR Idle State\n");
-
- dev_priv->psr.active = false;
} else {
if (dev_priv->psr.psr2_enabled)
WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
else
WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
}
+
+ if (dev_priv->psr.psr2_enabled) {
+ psr_status = EDP_PSR2_STATUS;
+ psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
+ } else {
+ psr_status = EDP_PSR_STATUS;
+ psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
+ }
+
+ /* Wait till PSR is idle */
+ if (intel_wait_for_register(dev_priv, psr_status, psr_status_mask, 0,
+ 2000))
+ DRM_ERROR("Timed out waiting for PSR Idle State\n");
+
+ dev_priv->psr.active = false;
}
static void intel_psr_disable_locked(struct intel_dp *intel_dp)
--
2.19.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen
2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza
@ 2018-10-05 23:35 ` José Roberto de Souza
2018-10-09 0:14 ` Dhinakaran Pandiyan
2018-10-05 23:35 ` [PATCH 3/4] drm/i915: Cache sink_count for eDP José Roberto de Souza
` (5 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: José Roberto de Souza @ 2018-10-05 23:35 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan
While PSR is active hardware will do aux transactions by it self to
wakeup sink to receive a new frame when necessary. If that
transaction is not acked by sink, hardware will trigger this
interruption.
So let's disable PSR as it is a hint that there is problem with this
sink.
The removed FIXME was asking to manually train the link but we don't
need to do that as by spec sink should do a short pulse when it is
out of sync with source, we just need to make sure it is awaken and
the SDP header with PSR disable will trigger this condition.
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_psr.c | 39 ++++++++++++++++++++++++++++----
2 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 794a8a03c7e6..efbebe1c2ba3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -638,6 +638,7 @@ struct i915_psr {
u8 sink_sync_latency;
ktime_t last_entry_attempt;
ktime_t last_exit;
+ u32 irq_aux_error;
};
enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index cd9a60d1efa1..74090fffea23 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -159,10 +159,16 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
BIT(TRANSCODER_C);
for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
- /* FIXME: Exit PSR and link train manually when this happens. */
- if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
- DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
- transcoder_name(cpu_transcoder));
+ if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
+ DRM_WARN("[transcoder %s] PSR aux error\n",
+ transcoder_name(cpu_transcoder));
+
+ spin_lock(&dev_priv->irq_lock);
+ dev_priv->psr.irq_aux_error |= BIT(cpu_transcoder);
+ spin_unlock(&dev_priv->irq_lock);
+
+ schedule_work(&dev_priv->psr.work);
+ }
if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
dev_priv->psr.last_entry_attempt = time_ns;
@@ -891,11 +897,36 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
return ret;
}
+static void intel_psr_handle_irq(struct drm_i915_private *dev_priv)
+{
+ struct i915_psr *psr = &dev_priv->psr;
+ u32 irq_aux_error;
+
+ spin_lock_irq(&dev_priv->irq_lock);
+ irq_aux_error = psr->irq_aux_error;
+ psr->irq_aux_error = 0;
+ spin_unlock_irq(&dev_priv->irq_lock);
+
+ /* right now PSR is only enabled in eDP */
+ WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
+
+ mutex_lock(&psr->lock);
+
+ intel_psr_disable_locked(psr->dp);
+ /* let's make sure that sink is awaken */
+ drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
+
+ mutex_unlock(&dev_priv->psr.lock);
+}
+
static void intel_psr_work(struct work_struct *work)
{
struct drm_i915_private *dev_priv =
container_of(work, typeof(*dev_priv), psr.work);
+ if (READ_ONCE(dev_priv->psr.irq_aux_error))
+ intel_psr_handle_irq(dev_priv);
+
mutex_lock(&dev_priv->psr.lock);
if (!dev_priv->psr.enabled)
--
2.19.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/4] drm/i915: Cache sink_count for eDP
2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza
2018-10-05 23:35 ` [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen José Roberto de Souza
@ 2018-10-05 23:35 ` José Roberto de Souza
2018-10-09 0:19 ` Dhinakaran Pandiyan
2018-10-05 23:35 ` [PATCH 4/4] drm/i915: Check PSR errors instead of retrain while PSR is enabled José Roberto de Souza
` (4 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: José Roberto de Souza @ 2018-10-05 23:35 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan
For eDP panels all the DPCD and EDID data is cached when initializing
the eDP connector so in futher detection it do not call
intel_dp_detect_dpcd() for eDP.
The problem is on the first short pulse interruption it calls
intel_dp_get_dpcd() for eDP and DP and it will read and set the sink
count, causing a mismatch between old sink count and the new one
triggering a full detection without needed.
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 19f0c3f59cbe..4a1c31ec9065 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
{
struct drm_i915_private *dev_priv =
to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
+ u8 val;
/* this function is meant to be called only once */
WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
@@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
intel_dp_set_common_rates(intel_dp);
+ if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <= 0)
+ return false;
+ intel_dp->sink_count = DP_GET_SINK_COUNT(val);
+
return true;
}
--
2.19.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/4] drm/i915: Check PSR errors instead of retrain while PSR is enabled
2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza
2018-10-05 23:35 ` [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen José Roberto de Souza
2018-10-05 23:35 ` [PATCH 3/4] drm/i915: Cache sink_count for eDP José Roberto de Souza
@ 2018-10-05 23:35 ` José Roberto de Souza
2018-10-06 0:50 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR Patchwork
` (3 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: José Roberto de Souza @ 2018-10-05 23:35 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan
When a PSR error happens sink also update the link status values
while source do not retrain link as required in the PSR exit
sequence.
So in the short pulse handling it was returning earlier and doing a
full detection and attempting to retrain but it fails because for
most sinks the main link is disabled while PSR is active, before even
check PSR errors.
Just call intel_psr_short_pulse() before
intel_dp_needs_link_retrain() is also not the right fix as
intel_dp_needs_link_retrain() would return true and trigger a full
detection and trying to retrain link while PSR HW is also doing that.
Check for PSR active is also not safe as it could be inactive due a
frontbuffer invalidate and still doing the PSR exit sequence.
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 7 +++++++
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_psr.c | 15 +++++++++++++++
3 files changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4a1c31ec9065..f89802bdf525 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4363,6 +4363,13 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
if (!intel_dp->link_trained)
return false;
+ /*
+ * While PSR is enabled, HW will control main-link and retrain when
+ * exiting PSR
+ */
+ if (intel_psr_enabled(intel_dp))
+ return false;
+
if (!intel_dp_get_link_status(intel_dp, link_status))
return false;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 43190c6e9ef2..e2076ec9d965 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1958,6 +1958,7 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
void intel_psr_short_pulse(struct intel_dp *intel_dp);
int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
u32 *out_value);
+bool intel_psr_enabled(struct intel_dp *intel_dp);
/* intel_runtime_pm.c */
int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 74090fffea23..63c00c802fa7 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1155,3 +1155,18 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
exit:
mutex_unlock(&psr->lock);
}
+
+bool intel_psr_enabled(struct intel_dp *intel_dp)
+{
+ struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+ bool ret;
+
+ if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
+ return false;
+
+ mutex_lock(&dev_priv->psr.lock);
+ ret = (dev_priv->psr.dp == intel_dp && dev_priv->psr.enabled);
+ mutex_unlock(&dev_priv->psr.lock);
+
+ return ret;
+}
--
2.19.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR
2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza
` (2 preceding siblings ...)
2018-10-05 23:35 ` [PATCH 4/4] drm/i915: Check PSR errors instead of retrain while PSR is enabled José Roberto de Souza
@ 2018-10-06 0:50 ` Patchwork
2018-10-06 1:10 ` ✓ Fi.CI.BAT: success " Patchwork
` (2 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: Patchwork @ 2018-10-06 0:50 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR
URL : https://patchwork.freedesktop.org/series/50654/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/psr: Always wait for idle state when disabling PSR
Okay!
Commit: drm/i915: Disable PSR when a PSR aux error happen
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3727:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3728:16: warning: expression using sizeof(void)
Commit: drm/i915: Cache sink_count for eDP
Okay!
Commit: drm/i915: Check PSR errors instead of retrain while PSR is enabled
Okay!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR
2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza
` (3 preceding siblings ...)
2018-10-06 0:50 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR Patchwork
@ 2018-10-06 1:10 ` Patchwork
2018-10-06 8:52 ` ✓ Fi.CI.IGT: " Patchwork
2018-10-08 22:43 ` [PATCH 1/4] " Dhinakaran Pandiyan
6 siblings, 0 replies; 18+ messages in thread
From: Patchwork @ 2018-10-06 1:10 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR
URL : https://patchwork.freedesktop.org/series/50654/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4945 -> Patchwork_10386 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/50654/revisions/1/mbox/
== Known issues ==
Here are the changes found in Patchwork_10386 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@pm_rpm@module-reload:
fi-glk-j4005: NOTRUN -> DMESG-WARN (fdo#107726)
==== Possible fixes ====
igt@kms_flip@basic-flip-vs-dpms:
fi-hsw-4770r: DMESG-WARN (fdo#105602) -> PASS
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
fdo#107726 https://bugs.freedesktop.org/show_bug.cgi?id=107726
== Participating hosts (44 -> 39) ==
Additional (1): fi-glk-j4005
Missing (6): fi-ilk-m540 fi-bxt-dsi fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-ctg-p8600
== Build changes ==
* Linux: CI_DRM_4945 -> Patchwork_10386
CI_DRM_4945: d9b2bbbdba15cadca76ffd3ff0476e71222d671b @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4671: b121f7d42c260ae3a050c3f440d1c11f7cff7d1a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10386: a80ef2c1e92629cd69081e088ded4a58b609b5f3 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
a80ef2c1e926 drm/i915: Check PSR errors instead of retrain while PSR is enabled
52e9bd524423 drm/i915: Cache sink_count for eDP
39febff6f09c drm/i915: Disable PSR when a PSR aux error happen
ba05b8343469 drm/i915/psr: Always wait for idle state when disabling PSR
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10386/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR
2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza
` (4 preceding siblings ...)
2018-10-06 1:10 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-10-06 8:52 ` Patchwork
2018-10-08 22:43 ` [PATCH 1/4] " Dhinakaran Pandiyan
6 siblings, 0 replies; 18+ messages in thread
From: Patchwork @ 2018-10-06 8:52 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR
URL : https://patchwork.freedesktop.org/series/50654/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4945_full -> Patchwork_10386_full =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_10386_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10386_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_10386_full:
=== IGT changes ===
==== Warnings ====
igt@pm_rc6_residency@rc6-accuracy:
shard-snb: PASS -> SKIP
== Known issues ==
Here are the changes found in Patchwork_10386_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_exec_big:
shard-hsw: PASS -> TIMEOUT (fdo#107937)
igt@gem_exec_schedule@pi-ringfull-blt:
shard-apl: NOTRUN -> FAIL (fdo#103158)
igt@gem_ppgtt@blt-vs-render-ctx0:
shard-skl: NOTRUN -> TIMEOUT (fdo#108039)
igt@gem_softpin@noreloc-s3:
shard-skl: PASS -> INCOMPLETE (fdo#107773, fdo#104108)
igt@kms_available_modes_crc@available_mode_test_crc:
shard-apl: PASS -> FAIL (fdo#106641)
igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
shard-apl: NOTRUN -> DMESG-WARN (fdo#107956) +3
igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
shard-apl: NOTRUN -> FAIL (fdo#105458, fdo#106510)
igt@kms_cursor_crc@cursor-128x128-onscreen:
shard-apl: PASS -> FAIL (fdo#103232)
igt@kms_cursor_crc@cursor-256x256-random:
shard-skl: NOTRUN -> FAIL (fdo#103232)
igt@kms_cursor_legacy@cursora-vs-flipa-toggle:
shard-glk: PASS -> DMESG-WARN (fdo#105763, fdo#106538)
igt@kms_flip@flip-vs-fences-interruptible:
shard-snb: NOTRUN -> INCOMPLETE (fdo#105411)
igt@kms_flip@plain-flip-ts-check-interruptible:
shard-skl: PASS -> FAIL (fdo#100368)
igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-mmap-gtt:
shard-glk: PASS -> DMESG-FAIL (fdo#103167, fdo#106538)
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
shard-apl: PASS -> FAIL (fdo#103167) +2
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
shard-glk: PASS -> FAIL (fdo#103167) +6
igt@kms_plane@pixel-format-pipe-c-planes:
shard-skl: NOTRUN -> DMESG-WARN (fdo#106885)
{igt@kms_plane_alpha_blend@pipe-a-alpha-basic}:
shard-skl: NOTRUN -> FAIL (fdo#108145) +1
{igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max}:
shard-glk: PASS -> FAIL (fdo#108145) +1
{igt@kms_plane_alpha_blend@pipe-a-coverage-7efc}:
shard-glk: PASS -> DMESG-FAIL (fdo#106538)
{igt@kms_plane_alpha_blend@pipe-b-alpha-7efc}:
shard-apl: NOTRUN -> FAIL (fdo#108146) +1
{igt@kms_plane_alpha_blend@pipe-b-alpha-transparant-fb}:
shard-apl: NOTRUN -> FAIL (fdo#108145) +1
{igt@kms_plane_alpha_blend@pipe-c-alpha-7efc}:
shard-kbl: NOTRUN -> FAIL (fdo#108146)
{igt@kms_plane_alpha_blend@pipe-c-coverage-7efc}:
shard-skl: NOTRUN -> FAIL (fdo#108146)
igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
shard-glk: PASS -> FAIL (fdo#103166) +1
shard-apl: PASS -> FAIL (fdo#103166)
igt@kms_setmode@basic:
shard-skl: NOTRUN -> FAIL (fdo#99912)
shard-snb: NOTRUN -> FAIL (fdo#99912)
==== Possible fixes ====
igt@kms_ccs@pipe-a-crc-primary-rotation-180:
shard-skl: FAIL -> PASS
igt@kms_color@pipe-b-legacy-gamma:
shard-apl: FAIL (fdo#104782) -> PASS
igt@kms_cursor_crc@cursor-256x256-dpms:
shard-glk: FAIL (fdo#103232) -> PASS +2
igt@kms_cursor_crc@cursor-64x21-random:
shard-apl: FAIL (fdo#103232) -> PASS
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-wc:
shard-apl: FAIL (fdo#103167) -> PASS
igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
shard-glk: FAIL (fdo#103167) -> PASS +7
igt@kms_plane_multiple@atomic-pipe-b-tiling-x:
shard-apl: FAIL (fdo#103166) -> PASS
igt@perf_pmu@busy-accuracy-98-rcs0:
shard-snb: INCOMPLETE (fdo#105411) -> SKIP
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
fdo#104782 https://bugs.freedesktop.org/show_bug.cgi?id=104782
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
fdo#105458 https://bugs.freedesktop.org/show_bug.cgi?id=105458
fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
fdo#106510 https://bugs.freedesktop.org/show_bug.cgi?id=106510
fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
fdo#106885 https://bugs.freedesktop.org/show_bug.cgi?id=106885
fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
fdo#107937 https://bugs.freedesktop.org/show_bug.cgi?id=107937
fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
fdo#108039 https://bugs.freedesktop.org/show_bug.cgi?id=108039
fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
fdo#108146 https://bugs.freedesktop.org/show_bug.cgi?id=108146
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
== Participating hosts (6 -> 6) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_4945 -> Patchwork_10386
CI_DRM_4945: d9b2bbbdba15cadca76ffd3ff0476e71222d671b @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4671: b121f7d42c260ae3a050c3f440d1c11f7cff7d1a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10386: a80ef2c1e92629cd69081e088ded4a58b609b5f3 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10386/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR
2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza
` (5 preceding siblings ...)
2018-10-06 8:52 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-10-08 22:43 ` Dhinakaran Pandiyan
2018-10-10 1:12 ` Souza, Jose
6 siblings, 1 reply; 18+ messages in thread
From: Dhinakaran Pandiyan @ 2018-10-08 22:43 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx, Ville Syrjälä
Cc: Rodrigo Vivi
On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> It should always wait for idle state when disabling PSR because PSR
> could be inactive due a call to intel_psr_exit() and while PSR is
> still being disabled asynchronously userspace could change the
> modeset causing a call to psr_disable() that will not wait for PSR
> idle and then PSR will be enabled again while PSR is still not idle.
Agreed.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/intel_psr.c | 43 +++++++++++++++---------------
> --
> 1 file changed, 20 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 423cdf84059c..cd9a60d1efa1 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -661,40 +661,37 @@ static void
> intel_psr_disable_source(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> + i915_reg_t psr_status;
> + u32 psr_status_mask;
>
> if (dev_priv->psr.active) {
> - i915_reg_t psr_status;
> - u32 psr_status_mask;
> -
> - if (dev_priv->psr.psr2_enabled) {
> - psr_status = EDP_PSR2_STATUS;
> - psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> -
> + if (dev_priv->psr.psr2_enabled)
> I915_WRITE(EDP_PSR2_CTL,
> - I915_READ(EDP_PSR2_CTL) &
> - ~(EDP_PSR2_ENABLE |
> EDP_SU_TRACK_ENABLE));
> -
> - } else {
> - psr_status = EDP_PSR_STATUS;
> - psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> -
> + I915_READ(EDP_PSR2_CTL) &
> ~EDP_PSR2_ENABLE);
Is there a way to reuse psr_exit() and move rest of the stuff to the
caller? We won't not need disable_source() if that can be done?
> + else
> I915_WRITE(EDP_PSR_CTL,
> I915_READ(EDP_PSR_CTL) &
> ~EDP_PSR_ENABLE);
> - }
> -
> - /* Wait till PSR is idle */
> - if (intel_wait_for_register(dev_priv,
> - psr_status,
> psr_status_mask, 0,
> - 2000))
> - DRM_ERROR("Timed out waiting for PSR Idle
> State\n");
> -
> - dev_priv->psr.active = false;
> } else {
> if (dev_priv->psr.psr2_enabled)
> WARN_ON(I915_READ(EDP_PSR2_CTL) &
> EDP_PSR2_ENABLE);
> else
> WARN_ON(I915_READ(EDP_PSR_CTL) &
> EDP_PSR_ENABLE);
> }
> +
> + if (dev_priv->psr.psr2_enabled) {
> + psr_status = EDP_PSR2_STATUS;
> + psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> + } else {
> + psr_status = EDP_PSR_STATUS;
> + psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> + }
> +
> + /* Wait till PSR is idle */
> + if (intel_wait_for_register(dev_priv, psr_status,
> psr_status_mask, 0,
> + 2000))
> + DRM_ERROR("Timed out waiting for PSR Idle State\n");
> +
> + dev_priv->psr.active = false;
> }
>
> static void intel_psr_disable_locked(struct intel_dp *intel_dp)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen
2018-10-05 23:35 ` [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen José Roberto de Souza
@ 2018-10-09 0:14 ` Dhinakaran Pandiyan
2018-10-09 0:30 ` Souza, Jose
0 siblings, 1 reply; 18+ messages in thread
From: Dhinakaran Pandiyan @ 2018-10-09 0:14 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx
On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> While PSR is active hardware will do aux transactions by it self to
> wakeup sink to receive a new frame when necessary. If that
> transaction is not acked by sink, hardware will trigger this
> interruption.
>
> So let's disable PSR as it is a hint that there is problem with this
> sink.
>
> The removed FIXME was asking to manually train the link but we don't
> need to do that as by spec sink should do a short pulse when it is
> out of sync with source, we just need to make sure it is awaken and
> the SDP header with PSR disable will trigger this condition.
That's a good point, the short pulse handler does handle retraining.
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 39 ++++++++++++++++++++++++++++
> ----
> 2 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 794a8a03c7e6..efbebe1c2ba3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -638,6 +638,7 @@ struct i915_psr {
> u8 sink_sync_latency;
> ktime_t last_entry_attempt;
> ktime_t last_exit;
> + u32 irq_aux_error;
> };
>
> enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index cd9a60d1efa1..74090fffea23 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -159,10 +159,16 @@ void intel_psr_irq_handler(struct
> drm_i915_private *dev_priv, u32 psr_iir)
> BIT(TRANSCODER_C);
>
> for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> transcoders) {
> - /* FIXME: Exit PSR and link train manually when this
> happens. */
> - if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> - DRM_DEBUG_KMS("[transcoder %s] PSR aux
> error\n",
> - transcoder_name(cpu_transcoder));
> + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> + DRM_WARN("[transcoder %s] PSR aux error\n",
> + transcoder_name(cpu_transcoder));
> +
> + spin_lock(&dev_priv->irq_lock);
> + dev_priv->psr.irq_aux_error |=
> BIT(cpu_transcoder);
> + spin_unlock(&dev_priv->irq_lock);
> +
> + schedule_work(&dev_priv->psr.work);
> + }
>
> if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> dev_priv->psr.last_entry_attempt = time_ns;
> @@ -891,11 +897,36 @@ int intel_psr_set_debugfs_mode(struct
> drm_i915_private *dev_priv,
> return ret;
> }
>
> +static void intel_psr_handle_irq(struct drm_i915_private *dev_priv)
> +{
> + struct i915_psr *psr = &dev_priv->psr;
> + u32 irq_aux_error;
> +
> + spin_lock_irq(&dev_priv->irq_lock);
> + irq_aux_error = psr->irq_aux_error;
> + psr->irq_aux_error = 0;
> + spin_unlock_irq(&dev_priv->irq_lock);
> +
> + /* right now PSR is only enabled in eDP */
> + WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
> +
> + mutex_lock(&psr->lock);
> +
> + intel_psr_disable_locked(psr->dp);
> + /* let's make sure that sink is awaken */
> + drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER,
> DP_SET_POWER_D0);
We should be making sure the sink exits PSR after psr_invalidate() ->
psr_exit() too? Which means, we have to figure out a cleaner way to
handle all of this. I am not sure, at this point, what a cleaner
solution will like. However, I'd like PSR disable from invalidate, PSR
disable from a modeset and PSR disable due to an error share code and
behavior. All of them should basically be
1) Disable PSR in PSR_CTL
2) Wait for idle in PSR_STATUS
3) Write to sink DP_SET_POWER
> +
> + mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> static void intel_psr_work(struct work_struct *work)
> {
> struct drm_i915_private *dev_priv =
> container_of(work, typeof(*dev_priv), psr.work);
>
> + if (READ_ONCE(dev_priv->psr.irq_aux_error))
> + intel_psr_handle_irq(dev_priv);
> +
> mutex_lock(&dev_priv->psr.lock);
>
> if (!dev_priv->psr.enabled)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP
2018-10-05 23:35 ` [PATCH 3/4] drm/i915: Cache sink_count for eDP José Roberto de Souza
@ 2018-10-09 0:19 ` Dhinakaran Pandiyan
2018-10-09 0:35 ` Souza, Jose
0 siblings, 1 reply; 18+ messages in thread
From: Dhinakaran Pandiyan @ 2018-10-09 0:19 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx
On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> For eDP panels all the DPCD and EDID data is cached when initializing
> the eDP connector so in futher detection it do not call
> intel_dp_detect_dpcd() for eDP.
> The problem is on the first short pulse interruption it calls
> intel_dp_get_dpcd() for eDP and DP and it will read and set the sink
> count, causing a mismatch between old sink count and the new one
> triggering a full detection without needed.
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 19f0c3f59cbe..4a1c31ec9065 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv =
> to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> + u8 val;
>
> /* this function is meant to be called only once */
> WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>
> intel_dp_set_common_rates(intel_dp);
>
> + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> 0)
> + return false;
> + intel_dp->sink_count = DP_GET_SINK_COUNT(val);
Is this even relevant for eDPs? Seems unnecessary to read or compare
sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for eDP.
-DK
> +
> return true;
> }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen
2018-10-09 0:14 ` Dhinakaran Pandiyan
@ 2018-10-09 0:30 ` Souza, Jose
2018-10-09 0:49 ` Dhinakaran Pandiyan
0 siblings, 1 reply; 18+ messages in thread
From: Souza, Jose @ 2018-10-09 0:30 UTC (permalink / raw)
To: intel-gfx, Pandiyan, Dhinakaran
On Mon, 2018-10-08 at 17:14 -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > While PSR is active hardware will do aux transactions by it self to
> > wakeup sink to receive a new frame when necessary. If that
> > transaction is not acked by sink, hardware will trigger this
> > interruption.
> >
> > So let's disable PSR as it is a hint that there is problem with
> > this
> > sink.
> >
> > The removed FIXME was asking to manually train the link but we
> > don't
> > need to do that as by spec sink should do a short pulse when it is
> > out of sync with source, we just need to make sure it is awaken and
> > the SDP header with PSR disable will trigger this condition.
> That's a good point, the short pulse handler does handle retraining.
>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_psr.c | 39 ++++++++++++++++++++++++++++
> > ----
> > 2 files changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 794a8a03c7e6..efbebe1c2ba3 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -638,6 +638,7 @@ struct i915_psr {
> > u8 sink_sync_latency;
> > ktime_t last_entry_attempt;
> > ktime_t last_exit;
> > + u32 irq_aux_error;
> > };
> >
> > enum intel_pch {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index cd9a60d1efa1..74090fffea23 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -159,10 +159,16 @@ void intel_psr_irq_handler(struct
> > drm_i915_private *dev_priv, u32 psr_iir)
> > BIT(TRANSCODER_C);
> >
> > for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > transcoders) {
> > - /* FIXME: Exit PSR and link train manually when this
> > happens. */
> > - if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > - DRM_DEBUG_KMS("[transcoder %s] PSR aux
> > error\n",
> > - transcoder_name(cpu_transcoder));
> > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > + DRM_WARN("[transcoder %s] PSR aux error\n",
> > + transcoder_name(cpu_transcoder));
> > +
> > + spin_lock(&dev_priv->irq_lock);
> > + dev_priv->psr.irq_aux_error |=
> > BIT(cpu_transcoder);
> > + spin_unlock(&dev_priv->irq_lock);
> > +
> > + schedule_work(&dev_priv->psr.work);
> > + }
> >
> > if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > dev_priv->psr.last_entry_attempt = time_ns;
> > @@ -891,11 +897,36 @@ int intel_psr_set_debugfs_mode(struct
> > drm_i915_private *dev_priv,
> > return ret;
> > }
> >
> > +static void intel_psr_handle_irq(struct drm_i915_private
> > *dev_priv)
> > +{
> > + struct i915_psr *psr = &dev_priv->psr;
> > + u32 irq_aux_error;
> > +
> > + spin_lock_irq(&dev_priv->irq_lock);
> > + irq_aux_error = psr->irq_aux_error;
> > + psr->irq_aux_error = 0;
> > + spin_unlock_irq(&dev_priv->irq_lock);
> > +
> > + /* right now PSR is only enabled in eDP */
> > + WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
> > +
> > + mutex_lock(&psr->lock);
> > +
> > + intel_psr_disable_locked(psr->dp);
> > + /* let's make sure that sink is awaken */
> > + drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER,
> > DP_SET_POWER_D0);
>
> We should be making sure the sink exits PSR after psr_invalidate() ->
> psr_exit() too? Which means, we have to figure out a cleaner way to
> handle all of this. I am not sure, at this point, what a cleaner
> solution will like. However, I'd like PSR disable from invalidate,
> PSR
> disable from a modeset and PSR disable due to an error share code and
> behavior. All of them should basically be
> 1) Disable PSR in PSR_CTL
> 2) Wait for idle in PSR_STATUS
> 3) Write to sink DP_SET_POWER
We don't need to wait PSR to be disabled for invalidate(), hardware
will do the exit sequence including write to DP_SET_POWER, also in this
case we want activate PSR as soon as all frontbuffer changes was
commited and PSR is inactive.
>
>
>
> > +
> > + mutex_unlock(&dev_priv->psr.lock);
> > +}
> > +
> > static void intel_psr_work(struct work_struct *work)
> > {
> > struct drm_i915_private *dev_priv =
> > container_of(work, typeof(*dev_priv), psr.work);
> >
> > + if (READ_ONCE(dev_priv->psr.irq_aux_error))
> > + intel_psr_handle_irq(dev_priv);
> > +
> > mutex_lock(&dev_priv->psr.lock);
> >
> > if (!dev_priv->psr.enabled)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP
2018-10-09 0:19 ` Dhinakaran Pandiyan
@ 2018-10-09 0:35 ` Souza, Jose
2018-10-09 0:54 ` Dhinakaran Pandiyan
0 siblings, 1 reply; 18+ messages in thread
From: Souza, Jose @ 2018-10-09 0:35 UTC (permalink / raw)
To: intel-gfx, Pandiyan, Dhinakaran
On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > For eDP panels all the DPCD and EDID data is cached when
> > initializing
> > the eDP connector so in futher detection it do not call
> > intel_dp_detect_dpcd() for eDP.
> > The problem is on the first short pulse interruption it calls
> > intel_dp_get_dpcd() for eDP and DP and it will read and set the
> > sink
> > count, causing a mismatch between old sink count and the new one
> > triggering a full detection without needed.
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 19f0c3f59cbe..4a1c31ec9065 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > *intel_dp)
> > {
> > struct drm_i915_private *dev_priv =
> > to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> > + u8 val;
> >
> > /* this function is meant to be called only once */
> > WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > *intel_dp)
> >
> > intel_dp_set_common_rates(intel_dp);
> >
> > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> > 0)
> > + return false;
> > + intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> Is this even relevant for eDPs? Seems unnecessary to read or compare
> sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for
> eDP.
I'm not sure as DP specs for DP_SINK_COUNT says:
The Sink device shall add one more if it has a local Rendering
Function.
and eDP spec do not redefine or alter this, so I guess is more safe
also read for eDP too.
>
>
> -DK
>
> > +
> > return true;
> > }
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen
2018-10-09 0:30 ` Souza, Jose
@ 2018-10-09 0:49 ` Dhinakaran Pandiyan
2018-10-09 0:57 ` Souza, Jose
0 siblings, 1 reply; 18+ messages in thread
From: Dhinakaran Pandiyan @ 2018-10-09 0:49 UTC (permalink / raw)
To: Souza, Jose, intel-gfx
On Mon, 2018-10-08 at 17:30 -0700, Souza, Jose wrote:
> On Mon, 2018-10-08 at 17:14 -0700, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > While PSR is active hardware will do aux transactions by it self
> > > to
> > > wakeup sink to receive a new frame when necessary. If that
> > > transaction is not acked by sink, hardware will trigger this
> > > interruption.
> > >
> > > So let's disable PSR as it is a hint that there is problem with
> > > this
> > > sink.
> > >
> > > The removed FIXME was asking to manually train the link but we
> > > don't
> > > need to do that as by spec sink should do a short pulse when it
> > > is
> > > out of sync with source, we just need to make sure it is awaken
> > > and
> > > the SDP header with PSR disable will trigger this condition.
> >
> > That's a good point, the short pulse handler does handle
> > retraining.
> >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > > drivers/gpu/drm/i915/intel_psr.c | 39
> > > ++++++++++++++++++++++++++++
> > > ----
> > > 2 files changed, 36 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 794a8a03c7e6..efbebe1c2ba3 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -638,6 +638,7 @@ struct i915_psr {
> > > u8 sink_sync_latency;
> > > ktime_t last_entry_attempt;
> > > ktime_t last_exit;
> > > + u32 irq_aux_error;
> > > };
> > >
> > > enum intel_pch {
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index cd9a60d1efa1..74090fffea23 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -159,10 +159,16 @@ void intel_psr_irq_handler(struct
> > > drm_i915_private *dev_priv, u32 psr_iir)
> > > BIT(TRANSCODER_C);
> > >
> > > for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > > transcoders) {
> > > - /* FIXME: Exit PSR and link train manually when this
> > > happens. */
> > > - if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > > - DRM_DEBUG_KMS("[transcoder %s] PSR aux
> > > error\n",
> > > - transcoder_name(cpu_transcoder));
> > > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > > + DRM_WARN("[transcoder %s] PSR aux error\n",
> > > + transcoder_name(cpu_transcoder));
> > > +
> > > + spin_lock(&dev_priv->irq_lock);
> > > + dev_priv->psr.irq_aux_error |=
> > > BIT(cpu_transcoder);
> > > + spin_unlock(&dev_priv->irq_lock);
> > > +
> > > + schedule_work(&dev_priv->psr.work);
> > > + }
> > >
> > > if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > dev_priv->psr.last_entry_attempt = time_ns;
> > > @@ -891,11 +897,36 @@ int intel_psr_set_debugfs_mode(struct
> > > drm_i915_private *dev_priv,
> > > return ret;
> > > }
> > >
> > > +static void intel_psr_handle_irq(struct drm_i915_private
> > > *dev_priv)
> > > +{
> > > + struct i915_psr *psr = &dev_priv->psr;
> > > + u32 irq_aux_error;
> > > +
> > > + spin_lock_irq(&dev_priv->irq_lock);
> > > + irq_aux_error = psr->irq_aux_error;
> > > + psr->irq_aux_error = 0;
> > > + spin_unlock_irq(&dev_priv->irq_lock);
> > > +
> > > + /* right now PSR is only enabled in eDP */
> > > + WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
> > > +
> > > + mutex_lock(&psr->lock);
> > > +
> > > + intel_psr_disable_locked(psr->dp);
> > > + /* let's make sure that sink is awaken */
> > > + drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER,
> > > DP_SET_POWER_D0);
> >
> > We should be making sure the sink exits PSR after psr_invalidate()
> > ->
> > psr_exit() too? Which means, we have to figure out a cleaner way to
> > handle all of this. I am not sure, at this point, what a cleaner
> > solution will like. However, I'd like PSR disable from invalidate,
> > PSR
> > disable from a modeset and PSR disable due to an error share code
> > and
> > behavior. All of them should basically be
> > 1) Disable PSR in PSR_CTL
> > 2) Wait for idle in PSR_STATUS
> > 3) Write to sink DP_SET_POWER
>
> We don't need to wait PSR to be disabled for invalidate(), hardware
> will do the exit sequence including write to DP_SET_POWER, also in
> this
Yeah, but doesn't work consistently on the panel that I have, writing
to the sink DP_SET_POWER dpcd is needed. I guess, we could add a panel
specific quirk to work around it.
-DK
> case we want activate PSR as soon as all frontbuffer changes was
> commited and PSR is inactive.
>
> >
> >
> >
> > > +
> > > + mutex_unlock(&dev_priv->psr.lock);
> > > +}
> > > +
> > > static void intel_psr_work(struct work_struct *work)
> > > {
> > > struct drm_i915_private *dev_priv =
> > > container_of(work, typeof(*dev_priv), psr.work);
> > >
> > > + if (READ_ONCE(dev_priv->psr.irq_aux_error))
> > > + intel_psr_handle_irq(dev_priv);
> > > +
> > > mutex_lock(&dev_priv->psr.lock);
> > >
> > > if (!dev_priv->psr.enabled)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP
2018-10-09 0:35 ` Souza, Jose
@ 2018-10-09 0:54 ` Dhinakaran Pandiyan
2018-10-09 13:27 ` Ville Syrjälä
0 siblings, 1 reply; 18+ messages in thread
From: Dhinakaran Pandiyan @ 2018-10-09 0:54 UTC (permalink / raw)
To: Souza, Jose, intel-gfx
On Mon, 2018-10-08 at 17:35 -0700, Souza, Jose wrote:
> On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > For eDP panels all the DPCD and EDID data is cached when
> > > initializing
> > > the eDP connector so in futher detection it do not call
> > > intel_dp_detect_dpcd() for eDP.
> > > The problem is on the first short pulse interruption it calls
> > > intel_dp_get_dpcd() for eDP and DP and it will read and set the
> > > sink
> > > count, causing a mismatch between old sink count and the new one
> > > triggering a full detection without needed.
> > >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 19f0c3f59cbe..4a1c31ec9065 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > > *intel_dp)
> > > {
> > > struct drm_i915_private *dev_priv =
> > > to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> > > + u8 val;
> > >
> > > /* this function is meant to be called only once */
> > > WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > > *intel_dp)
> > >
> > > intel_dp_set_common_rates(intel_dp);
> > >
> > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> > > 0)
> > > + return false;
> > > + intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> >
> > Is this even relevant for eDPs? Seems unnecessary to read or
> > compare
> > sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for
> > eDP.
>
> I'm not sure as DP specs for DP_SINK_COUNT says:
>
> The Sink device shall add one more if it has a local Rendering
> Function.
>
> and eDP spec do not redefine or alter this, so I guess is more safe
> also read for eDP too.
>
We already special case eDP in several places, for example, don't
update link rates from the short pulse handler etc. And also don't
support hotplug, I don't see a point.
-DK
>
> >
> >
> > -DK
> >
> > > +
> > > return true;
> > > }
> > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen
2018-10-09 0:49 ` Dhinakaran Pandiyan
@ 2018-10-09 0:57 ` Souza, Jose
0 siblings, 0 replies; 18+ messages in thread
From: Souza, Jose @ 2018-10-09 0:57 UTC (permalink / raw)
To: intel-gfx, Pandiyan, Dhinakaran
On Mon, 2018-10-08 at 17:49 -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-10-08 at 17:30 -0700, Souza, Jose wrote:
> > On Mon, 2018-10-08 at 17:14 -0700, Dhinakaran Pandiyan wrote:
> > > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > > While PSR is active hardware will do aux transactions by it
> > > > self
> > > > to
> > > > wakeup sink to receive a new frame when necessary. If that
> > > > transaction is not acked by sink, hardware will trigger this
> > > > interruption.
> > > >
> > > > So let's disable PSR as it is a hint that there is problem with
> > > > this
> > > > sink.
> > > >
> > > > The removed FIXME was asking to manually train the link but we
> > > > don't
> > > > need to do that as by spec sink should do a short pulse when it
> > > > is
> > > > out of sync with source, we just need to make sure it is awaken
> > > > and
> > > > the SDP header with PSR disable will trigger this condition.
> > >
> > > That's a good point, the short pulse handler does handle
> > > retraining.
> > >
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > > > drivers/gpu/drm/i915/intel_psr.c | 39
> > > > ++++++++++++++++++++++++++++
> > > > ----
> > > > 2 files changed, 36 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 794a8a03c7e6..efbebe1c2ba3 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -638,6 +638,7 @@ struct i915_psr {
> > > > u8 sink_sync_latency;
> > > > ktime_t last_entry_attempt;
> > > > ktime_t last_exit;
> > > > + u32 irq_aux_error;
> > > > };
> > > >
> > > > enum intel_pch {
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index cd9a60d1efa1..74090fffea23 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -159,10 +159,16 @@ void intel_psr_irq_handler(struct
> > > > drm_i915_private *dev_priv, u32 psr_iir)
> > > > BIT(TRANSCODER_C);
> > > >
> > > > for_each_cpu_transcoder_masked(dev_priv,
> > > > cpu_transcoder,
> > > > transcoders) {
> > > > - /* FIXME: Exit PSR and link train manually when
> > > > this
> > > > happens. */
> > > > - if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > > > - DRM_DEBUG_KMS("[transcoder %s] PSR aux
> > > > error\n",
> > > > - transcoder_name(cpu_trans
> > > > coder));
> > > > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > > > + DRM_WARN("[transcoder %s] PSR aux
> > > > error\n",
> > > > + transcoder_name(cpu_transcoder
> > > > ));
> > > > +
> > > > + spin_lock(&dev_priv->irq_lock);
> > > > + dev_priv->psr.irq_aux_error |=
> > > > BIT(cpu_transcoder);
> > > > + spin_unlock(&dev_priv->irq_lock);
> > > > +
> > > > + schedule_work(&dev_priv->psr.work);
> > > > + }
> > > >
> > > > if (psr_iir &
> > > > EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > > dev_priv->psr.last_entry_attempt =
> > > > time_ns;
> > > > @@ -891,11 +897,36 @@ int intel_psr_set_debugfs_mode(struct
> > > > drm_i915_private *dev_priv,
> > > > return ret;
> > > > }
> > > >
> > > > +static void intel_psr_handle_irq(struct drm_i915_private
> > > > *dev_priv)
> > > > +{
> > > > + struct i915_psr *psr = &dev_priv->psr;
> > > > + u32 irq_aux_error;
> > > > +
> > > > + spin_lock_irq(&dev_priv->irq_lock);
> > > > + irq_aux_error = psr->irq_aux_error;
> > > > + psr->irq_aux_error = 0;
> > > > + spin_unlock_irq(&dev_priv->irq_lock);
> > > > +
> > > > + /* right now PSR is only enabled in eDP */
> > > > + WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
> > > > +
> > > > + mutex_lock(&psr->lock);
> > > > +
> > > > + intel_psr_disable_locked(psr->dp);
> > > > + /* let's make sure that sink is awaken */
> > > > + drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER,
> > > > DP_SET_POWER_D0);
> > >
> > > We should be making sure the sink exits PSR after
> > > psr_invalidate()
> > > ->
> > > psr_exit() too? Which means, we have to figure out a cleaner way
> > > to
> > > handle all of this. I am not sure, at this point, what a cleaner
> > > solution will like. However, I'd like PSR disable from
> > > invalidate,
> > > PSR
> > > disable from a modeset and PSR disable due to an error share code
> > > and
> > > behavior. All of them should basically be
> > > 1) Disable PSR in PSR_CTL
> > > 2) Wait for idle in PSR_STATUS
> > > 3) Write to sink DP_SET_POWER
> >
> > We don't need to wait PSR to be disabled for invalidate(), hardware
> > will do the exit sequence including write to DP_SET_POWER, also in
> > this
> Yeah, but doesn't work consistently on the panel that I have, writing
> to the sink DP_SET_POWER dpcd is needed. I guess, we could add a
> panel
> specific quirk to work around it.
The DP_SET_POWER writes that HW does while PSR is enabled in HW should
also fail, that is really strange that only the psr_exit() fails.
>
> -DK
>
> > case we want activate PSR as soon as all frontbuffer changes was
> > commited and PSR is inactive.
> >
> > >
> > >
> > > > +
> > > > + mutex_unlock(&dev_priv->psr.lock);
> > > > +}
> > > > +
> > > > static void intel_psr_work(struct work_struct *work)
> > > > {
> > > > struct drm_i915_private *dev_priv =
> > > > container_of(work, typeof(*dev_priv),
> > > > psr.work);
> > > >
> > > > + if (READ_ONCE(dev_priv->psr.irq_aux_error))
> > > > + intel_psr_handle_irq(dev_priv);
> > > > +
> > > > mutex_lock(&dev_priv->psr.lock);
> > > >
> > > > if (!dev_priv->psr.enabled)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP
2018-10-09 0:54 ` Dhinakaran Pandiyan
@ 2018-10-09 13:27 ` Ville Syrjälä
2018-10-10 1:09 ` Souza, Jose
0 siblings, 1 reply; 18+ messages in thread
From: Ville Syrjälä @ 2018-10-09 13:27 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx
On Mon, Oct 08, 2018 at 05:54:17PM -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-10-08 at 17:35 -0700, Souza, Jose wrote:
> > On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> > > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > > For eDP panels all the DPCD and EDID data is cached when
> > > > initializing
> > > > the eDP connector so in futher detection it do not call
> > > > intel_dp_detect_dpcd() for eDP.
> > > > The problem is on the first short pulse interruption it calls
> > > > intel_dp_get_dpcd() for eDP and DP and it will read and set the
> > > > sink
> > > > count, causing a mismatch between old sink count and the new one
> > > > triggering a full detection without needed.
> > > >
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 19f0c3f59cbe..4a1c31ec9065 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > > {
> > > > struct drm_i915_private *dev_priv =
> > > > to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> > > > + u8 val;
> > > >
> > > > /* this function is meant to be called only once */
> > > > WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > > > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > >
> > > > intel_dp_set_common_rates(intel_dp);
> > > >
> > > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> > > > 0)
> > > > + return false;
> > > > + intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> > >
> > > Is this even relevant for eDPs? Seems unnecessary to read or
> > > compare
> > > sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for
> > > eDP.
> >
> > I'm not sure as DP specs for DP_SINK_COUNT says:
> >
> > The Sink device shall add one more if it has a local Rendering
> > Function.
> >
> > and eDP spec do not redefine or alter this, so I guess is more safe
> > also read for eDP too.
> >
>
> We already special case eDP in several places, for example, don't
> update link rates from the short pulse handler etc. And also don't
> support hotplug, I don't see a point.
IIRC some conformance test or something required that we read this.
I guess what we could do is still read it but just not update
intel_dp->sink_count. We already seem to have a special case which
ignores a zero sink_count on eDP. Might as well extend that a bit
I suppose.
In general I think special cases are bad, so IMO we should try
hard not add more unless really necessary. In this case it seems
the special case is warranted. Unfortunately commit 1034ce707b57
("drm/i915: Fixing eDP detection on certain platforms") failed to add
a comment explaining why. I'd appreciate if someone could add that
comment now so that we don't forget this in the future.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP
2018-10-09 13:27 ` Ville Syrjälä
@ 2018-10-10 1:09 ` Souza, Jose
0 siblings, 0 replies; 18+ messages in thread
From: Souza, Jose @ 2018-10-10 1:09 UTC (permalink / raw)
To: ville.syrjala, Pandiyan, Dhinakaran; +Cc: intel-gfx
On Tue, 2018-10-09 at 16:27 +0300, Ville Syrjälä wrote:
> On Mon, Oct 08, 2018 at 05:54:17PM -0700, Dhinakaran Pandiyan wrote:
> > On Mon, 2018-10-08 at 17:35 -0700, Souza, Jose wrote:
> > > On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> > > > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > > > For eDP panels all the DPCD and EDID data is cached when
> > > > > initializing
> > > > > the eDP connector so in futher detection it do not call
> > > > > intel_dp_detect_dpcd() for eDP.
> > > > > The problem is on the first short pulse interruption it calls
> > > > > intel_dp_get_dpcd() for eDP and DP and it will read and set
> > > > > the
> > > > > sink
> > > > > count, causing a mismatch between old sink count and the new
> > > > > one
> > > > > triggering a full detection without needed.
> > > > >
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> > > > > 1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 19f0c3f59cbe..4a1c31ec9065 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > > > > *intel_dp)
> > > > > {
> > > > > struct drm_i915_private *dev_priv =
> > > > > to_i915(dp_to_dig_port(intel_dp)-
> > > > > >base.base.dev);
> > > > > + u8 val;
> > > > >
> > > > > /* this function is meant to be called only once */
> > > > > WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > > > > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > > > > *intel_dp)
> > > > >
> > > > > intel_dp_set_common_rates(intel_dp);
> > > > >
> > > > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT,
> > > > > &val) <=
> > > > > 0)
> > > > > + return false;
> > > > > + intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> > > >
> > > > Is this even relevant for eDPs? Seems unnecessary to read or
> > > > compare
> > > > sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks
> > > > for
> > > > eDP.
> > >
> > > I'm not sure as DP specs for DP_SINK_COUNT says:
> > >
> > > The Sink device shall add one more if it has a local Rendering
> > > Function.
> > >
> > > and eDP spec do not redefine or alter this, so I guess is more
> > > safe
> > > also read for eDP too.
> > >
> >
> > We already special case eDP in several places, for example, don't
> > update link rates from the short pulse handler etc. And also don't
> > support hotplug, I don't see a point.
>
> IIRC some conformance test or something required that we read this.
> I guess what we could do is still read it but just not update
> intel_dp->sink_count. We already seem to have a special case which
> ignores a zero sink_count on eDP. Might as well extend that a bit
> I suppose.
Okay, I will skip the comparison for eDP.
>
> In general I think special cases are bad, so IMO we should try
> hard not add more unless really necessary. In this case it seems
> the special case is warranted. Unfortunately commit 1034ce707b57
> ("drm/i915: Fixing eDP detection on certain platforms") failed to add
> a comment explaining why. I'd appreciate if someone could add that
> comment now so that we don't forget this in the future.
Sure, I will add this comment.
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR
2018-10-08 22:43 ` [PATCH 1/4] " Dhinakaran Pandiyan
@ 2018-10-10 1:12 ` Souza, Jose
0 siblings, 0 replies; 18+ messages in thread
From: Souza, Jose @ 2018-10-10 1:12 UTC (permalink / raw)
To: ville.syrjala, intel-gfx, Pandiyan, Dhinakaran; +Cc: Vivi, Rodrigo
On Mon, 2018-10-08 at 15:43 -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > It should always wait for idle state when disabling PSR because PSR
> > could be inactive due a call to intel_psr_exit() and while PSR is
> > still being disabled asynchronously userspace could change the
> > modeset causing a call to psr_disable() that will not wait for PSR
> > idle and then PSR will be enabled again while PSR is still not
> > idle.
> Agreed.
>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_psr.c | 43 +++++++++++++++-------------
> > --
> > --
> > 1 file changed, 20 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 423cdf84059c..cd9a60d1efa1 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -661,40 +661,37 @@ static void
> > intel_psr_disable_source(struct intel_dp *intel_dp)
> > {
> > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > + i915_reg_t psr_status;
> > + u32 psr_status_mask;
> >
> > if (dev_priv->psr.active) {
> > - i915_reg_t psr_status;
> > - u32 psr_status_mask;
> > -
> > - if (dev_priv->psr.psr2_enabled) {
> > - psr_status = EDP_PSR2_STATUS;
> > - psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > -
> > + if (dev_priv->psr.psr2_enabled)
> > I915_WRITE(EDP_PSR2_CTL,
> > - I915_READ(EDP_PSR2_CTL) &
> > - ~(EDP_PSR2_ENABLE |
> > EDP_SU_TRACK_ENABLE));
> > -
> > - } else {
> > - psr_status = EDP_PSR_STATUS;
> > - psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> > -
> > + I915_READ(EDP_PSR2_CTL) &
> > ~EDP_PSR2_ENABLE);
>
> Is there a way to reuse psr_exit() and move rest of the stuff to the
> caller? We won't not need disable_source() if that can be done?
We could move to the caller and reuse psr_exit(), I will do that in
another patch.
>
>
>
> > + else
> > I915_WRITE(EDP_PSR_CTL,
> > I915_READ(EDP_PSR_CTL) &
> > ~EDP_PSR_ENABLE);
> > - }
> > -
> > - /* Wait till PSR is idle */
> > - if (intel_wait_for_register(dev_priv,
> > - psr_status,
> > psr_status_mask, 0,
> > - 2000))
> > - DRM_ERROR("Timed out waiting for PSR Idle
> > State\n");
> > -
> > - dev_priv->psr.active = false;
> > } else {
> > if (dev_priv->psr.psr2_enabled)
> > WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > EDP_PSR2_ENABLE);
> > else
> > WARN_ON(I915_READ(EDP_PSR_CTL) &
> > EDP_PSR_ENABLE);
> > }
> > +
> > + if (dev_priv->psr.psr2_enabled) {
> > + psr_status = EDP_PSR2_STATUS;
> > + psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > + } else {
> > + psr_status = EDP_PSR_STATUS;
> > + psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> > + }
> > +
> > + /* Wait till PSR is idle */
> > + if (intel_wait_for_register(dev_priv, psr_status,
> > psr_status_mask, 0,
> > + 2000))
> > + DRM_ERROR("Timed out waiting for PSR Idle State\n");
> > +
> > + dev_priv->psr.active = false;
> > }
> >
> > static void intel_psr_disable_locked(struct intel_dp *intel_dp)
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-10-10 1:13 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza
2018-10-05 23:35 ` [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen José Roberto de Souza
2018-10-09 0:14 ` Dhinakaran Pandiyan
2018-10-09 0:30 ` Souza, Jose
2018-10-09 0:49 ` Dhinakaran Pandiyan
2018-10-09 0:57 ` Souza, Jose
2018-10-05 23:35 ` [PATCH 3/4] drm/i915: Cache sink_count for eDP José Roberto de Souza
2018-10-09 0:19 ` Dhinakaran Pandiyan
2018-10-09 0:35 ` Souza, Jose
2018-10-09 0:54 ` Dhinakaran Pandiyan
2018-10-09 13:27 ` Ville Syrjälä
2018-10-10 1:09 ` Souza, Jose
2018-10-05 23:35 ` [PATCH 4/4] drm/i915: Check PSR errors instead of retrain while PSR is enabled José Roberto de Souza
2018-10-06 0:50 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR Patchwork
2018-10-06 1:10 ` ✓ Fi.CI.BAT: success " Patchwork
2018-10-06 8:52 ` ✓ Fi.CI.IGT: " Patchwork
2018-10-08 22:43 ` [PATCH 1/4] " Dhinakaran Pandiyan
2018-10-10 1:12 ` Souza, Jose
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.