* [PATCH 1/5] drm/i915: kill intel_display_power_well_is_enabled()
@ 2018-08-20 23:31 Paulo Zanoni
2018-08-20 23:31 ` [PATCH 2/5] drm/i915: WARN() if we can't lookup_power_well() Paulo Zanoni
` (6 more replies)
0 siblings, 7 replies; 11+ messages in thread
From: Paulo Zanoni @ 2018-08-20 23:31 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
Use the same coding pattern as we use in the other functions of the
same file: just call lookup_power_well() directly in the only caller.
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++-----------------
1 file changed, 3 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index ff3fd8dbd2b4..f75837e45144 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -49,9 +49,6 @@
* present for a given platform.
*/
-bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
- enum i915_power_well_id power_well_id);
-
static struct i915_power_well *
lookup_power_well(struct drm_i915_private *dev_priv,
enum i915_power_well_id power_well_id);
@@ -654,8 +651,9 @@ static void assert_csr_loaded(struct drm_i915_private *dev_priv)
static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
{
- bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv,
- SKL_DISP_PW_2);
+ struct i915_power_well *pg2 = lookup_power_well(dev_priv,
+ SKL_DISP_PW_2);
+ bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv, pg2);
WARN_ONCE(pg2_enabled, "PG2 not disabled to enable DC5.\n");
@@ -2278,18 +2276,6 @@ static const struct i915_power_well_desc chv_power_wells[] = {
},
};
-bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
- enum i915_power_well_id power_well_id)
-{
- struct i915_power_well *power_well;
- bool ret;
-
- power_well = lookup_power_well(dev_priv, power_well_id);
- ret = power_well->desc->ops->is_enabled(dev_priv, power_well);
-
- return ret;
-}
-
static const struct i915_power_well_desc skl_power_wells[] = {
{
.name = "always-on",
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/5] drm/i915: WARN() if we can't lookup_power_well()
2018-08-20 23:31 [PATCH 1/5] drm/i915: kill intel_display_power_well_is_enabled() Paulo Zanoni
@ 2018-08-20 23:31 ` Paulo Zanoni
2018-08-23 13:41 ` Imre Deak
2018-08-20 23:31 ` [PATCH 3/5] drm/i915: use for_each_power_well in lookup_power_well() Paulo Zanoni
` (5 subsequent siblings)
6 siblings, 1 reply; 11+ messages in thread
From: Paulo Zanoni @ 2018-08-20 23:31 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
None of the current lookup_power_well() callers are actually checking
for NULL return values, they all just use the pointer right away. The
first idea was to replace these theoretical segfaults with a BUG()
since this would at least make our code a little more explicit to the
reader. It was suggested that just converting the BUG() to a WARN()
and returning any power well would probably be better since it would
still keep the system running while at the same time exposing the
driver bug.
We can only hit this NULL/BUG()/WARN() condition if we try to lookup a
power well that isn't defined on a given platform. If that ever
happens, we have to fix our code, making it lookup the correct power
well. Because of this, I don't think it's worth trying to implement
error checking in every caller. Improving our CI system will be a
better use of our time once a bug is found in the wild.
v2: Avoid the BUG() with a WARN() return a random PW (Michal).
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_runtime_pm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index f75837e45144..f07ed70b839f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1096,7 +1096,15 @@ lookup_power_well(struct drm_i915_private *dev_priv,
return power_well;
}
- return NULL;
+ /*
+ * It's not feasible to add error checking code to the callers since
+ * this condition really shouldn't happen and it doesn't even make sense
+ * to abort things like display initialization sequences. Just return
+ * the first power well and hope the WARN gets reported so we can fix
+ * our driver.
+ */
+ WARN(1, "Power well %d not defined for this platform\n", power_well_id);
+ return &power_domains->power_wells[0];
}
#define BITS_SET(val, bits) (((val) & (bits)) == (bits))
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/5] drm/i915: use for_each_power_well in lookup_power_well()
2018-08-20 23:31 [PATCH 1/5] drm/i915: kill intel_display_power_well_is_enabled() Paulo Zanoni
2018-08-20 23:31 ` [PATCH 2/5] drm/i915: WARN() if we can't lookup_power_well() Paulo Zanoni
@ 2018-08-20 23:31 ` Paulo Zanoni
2018-08-20 23:31 ` [PATCH 4/5] drm/i915: move lookup_power_well() up Paulo Zanoni
` (4 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Paulo Zanoni @ 2018-08-20 23:31 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
Use the nice helper function to make the implementation simpler.
v2: Rebase.
Cc: Imre Deak <imre.deak@intel.com>
Reviewed-by: José Roberto de Souza <jose.souza@intel.com> (v1)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_runtime_pm.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index f07ed70b839f..6f5a2f00a12d 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1085,16 +1085,11 @@ static struct i915_power_well *
lookup_power_well(struct drm_i915_private *dev_priv,
enum i915_power_well_id power_well_id)
{
- struct i915_power_domains *power_domains = &dev_priv->power_domains;
- int i;
-
- for (i = 0; i < power_domains->power_well_count; i++) {
- struct i915_power_well *power_well;
+ struct i915_power_well *power_well;
- power_well = &power_domains->power_wells[i];
+ for_each_power_well(dev_priv, power_well)
if (power_well->desc->id == power_well_id)
return power_well;
- }
/*
* It's not feasible to add error checking code to the callers since
@@ -1104,7 +1099,7 @@ lookup_power_well(struct drm_i915_private *dev_priv,
* our driver.
*/
WARN(1, "Power well %d not defined for this platform\n", power_well_id);
- return &power_domains->power_wells[0];
+ return &dev_priv->power_domains.power_wells[0];
}
#define BITS_SET(val, bits) (((val) & (bits)) == (bits))
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/5] drm/i915: move lookup_power_well() up
2018-08-20 23:31 [PATCH 1/5] drm/i915: kill intel_display_power_well_is_enabled() Paulo Zanoni
2018-08-20 23:31 ` [PATCH 2/5] drm/i915: WARN() if we can't lookup_power_well() Paulo Zanoni
2018-08-20 23:31 ` [PATCH 3/5] drm/i915: use for_each_power_well in lookup_power_well() Paulo Zanoni
@ 2018-08-20 23:31 ` Paulo Zanoni
2018-08-23 13:44 ` Imre Deak
2018-08-20 23:31 ` [PATCH 5/5] drm/i915: use the SW-based pw->hw_enabled check instead of reading registers Paulo Zanoni
` (3 subsequent siblings)
6 siblings, 1 reply; 11+ messages in thread
From: Paulo Zanoni @ 2018-08-20 23:31 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
There's no need for that forward declaration.
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_runtime_pm.c | 46 +++++++++++++++------------------
1 file changed, 21 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6f5a2f00a12d..f38a049861e6 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -49,10 +49,6 @@
* present for a given platform.
*/
-static struct i915_power_well *
-lookup_power_well(struct drm_i915_private *dev_priv,
- enum i915_power_well_id power_well_id);
-
const char *
intel_display_power_domain_str(enum intel_display_power_domain domain)
{
@@ -649,6 +645,27 @@ static void assert_csr_loaded(struct drm_i915_private *dev_priv)
WARN_ONCE(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n");
}
+static struct i915_power_well *
+lookup_power_well(struct drm_i915_private *dev_priv,
+ enum i915_power_well_id power_well_id)
+{
+ struct i915_power_well *power_well;
+
+ for_each_power_well(dev_priv, power_well)
+ if (power_well->desc->id == power_well_id)
+ return power_well;
+
+ /*
+ * It's not feasible to add error checking code to the callers since
+ * this condition really shouldn't happen and it doesn't even make sense
+ * to abort things like display initialization sequences. Just return
+ * the first power well and hope the WARN gets reported so we can fix
+ * our driver.
+ */
+ WARN(1, "Power well %d not defined for this platform\n", power_well_id);
+ return &dev_priv->power_domains.power_wells[0];
+}
+
static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
{
struct i915_power_well *pg2 = lookup_power_well(dev_priv,
@@ -1081,27 +1098,6 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
#define POWER_DOMAIN_MASK (GENMASK_ULL(POWER_DOMAIN_NUM - 1, 0))
-static struct i915_power_well *
-lookup_power_well(struct drm_i915_private *dev_priv,
- enum i915_power_well_id power_well_id)
-{
- struct i915_power_well *power_well;
-
- for_each_power_well(dev_priv, power_well)
- if (power_well->desc->id == power_well_id)
- return power_well;
-
- /*
- * It's not feasible to add error checking code to the callers since
- * this condition really shouldn't happen and it doesn't even make sense
- * to abort things like display initialization sequences. Just return
- * the first power well and hope the WARN gets reported so we can fix
- * our driver.
- */
- WARN(1, "Power well %d not defined for this platform\n", power_well_id);
- return &dev_priv->power_domains.power_wells[0];
-}
-
#define BITS_SET(val, bits) (((val) & (bits)) == (bits))
static void assert_chv_phy_status(struct drm_i915_private *dev_priv)
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 5/5] drm/i915: use the SW-based pw->hw_enabled check instead of reading registers
2018-08-20 23:31 [PATCH 1/5] drm/i915: kill intel_display_power_well_is_enabled() Paulo Zanoni
` (2 preceding siblings ...)
2018-08-20 23:31 ` [PATCH 4/5] drm/i915: move lookup_power_well() up Paulo Zanoni
@ 2018-08-20 23:31 ` Paulo Zanoni
2018-08-23 13:48 ` Imre Deak
2018-08-20 23:37 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/5] drm/i915: kill intel_display_power_well_is_enabled() Patchwork
` (2 subsequent siblings)
6 siblings, 1 reply; 11+ messages in thread
From: Paulo Zanoni @ 2018-08-20 23:31 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
I can't find a reason why we would want to call is_enabled(), which
does a register read, instead of just relying on our tracking with
hw_enabled. Let's try to trust our hardware sync.
Cc: Imre Deak <imre.deak@intel.com>
Requested-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index f38a049861e6..76bb2e06fef1 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -670,9 +670,8 @@ static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
{
struct i915_power_well *pg2 = lookup_power_well(dev_priv,
SKL_DISP_PW_2);
- bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv, pg2);
- WARN_ONCE(pg2_enabled, "PG2 not disabled to enable DC5.\n");
+ WARN_ONCE(pg2->hw_enabled, "PG2 not disabled to enable DC5.\n");
WARN_ONCE((I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5),
"DC5 already programmed to be enabled.\n");
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/5] drm/i915: kill intel_display_power_well_is_enabled()
2018-08-20 23:31 [PATCH 1/5] drm/i915: kill intel_display_power_well_is_enabled() Paulo Zanoni
` (3 preceding siblings ...)
2018-08-20 23:31 ` [PATCH 5/5] drm/i915: use the SW-based pw->hw_enabled check instead of reading registers Paulo Zanoni
@ 2018-08-20 23:37 ` Patchwork
2018-08-20 23:55 ` ✓ Fi.CI.BAT: success " Patchwork
2018-08-21 0:45 ` ✓ Fi.CI.IGT: " Patchwork
6 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2018-08-20 23:37 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/5] drm/i915: kill intel_display_power_well_is_enabled()
URL : https://patchwork.freedesktop.org/series/48466/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
20b826691fa5 drm/i915: kill intel_display_power_well_is_enabled()
c740d932780f drm/i915: WARN() if we can't lookup_power_well()
7d5567ea0755 drm/i915: use for_each_power_well in lookup_power_well()
49dc6855112d drm/i915: move lookup_power_well() up
5e7669b1dc9b drm/i915: use the SW-based pw->hw_enabled check instead of reading registers
-:15: WARNING:BAD_SIGN_OFF: Non-standard signature: Requested-by:
#15:
Requested-by: José Roberto de Souza <jose.souza@intel.com>
total: 0 errors, 1 warnings, 0 checks, 10 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/5] drm/i915: kill intel_display_power_well_is_enabled()
2018-08-20 23:31 [PATCH 1/5] drm/i915: kill intel_display_power_well_is_enabled() Paulo Zanoni
` (4 preceding siblings ...)
2018-08-20 23:37 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/5] drm/i915: kill intel_display_power_well_is_enabled() Patchwork
@ 2018-08-20 23:55 ` Patchwork
2018-08-21 0:45 ` ✓ Fi.CI.IGT: " Patchwork
6 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2018-08-20 23:55 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/5] drm/i915: kill intel_display_power_well_is_enabled()
URL : https://patchwork.freedesktop.org/series/48466/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4691 -> Patchwork_9978 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/48466/revisions/1/mbox/
== Known issues ==
Here are the changes found in Patchwork_9978 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@drv_selftest@live_coherency:
fi-gdg-551: PASS -> DMESG-FAIL (fdo#107164)
igt@drv_selftest@live_hangcheck:
fi-bxt-j4205: PASS -> DMESG-FAIL (fdo#106560)
igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
{fi-byt-clapper}: PASS -> FAIL (fdo#107362)
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
{fi-byt-clapper}: PASS -> FAIL (fdo#103191, fdo#107362)
==== Possible fixes ====
igt@drv_selftest@live_hangcheck:
fi-skl-guc: DMESG-FAIL (fdo#107174) -> PASS
fi-kbl-guc: DMESG-FAIL (fdo#106947) -> PASS
fi-bxt-dsi: DMESG-FAIL (fdo#106560) -> PASS
{igt@kms_psr@primary_mmap_gtt}:
fi-cnl-psr: DMESG-WARN (fdo#107372) -> PASS
{igt@pm_rpm@module-reload}:
fi-cnl-psr: WARN (fdo#107602) -> PASS
==== Warnings ====
{igt@kms_psr@primary_page_flip}:
fi-cnl-psr: DMESG-WARN (fdo#107372) -> DMESG-FAIL (fdo#107372)
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372
fdo#107602 https://bugs.freedesktop.org/show_bug.cgi?id=107602
== Participating hosts (51 -> 48) ==
Additional (1): fi-snb-2520m
Missing (4): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u
== Build changes ==
* Linux: CI_DRM_4691 -> Patchwork_9978
CI_DRM_4691: 2d75266982a5dae956c10e683a1a74d977d88e09 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4607: 6e0b3e7a2d241af36f8c6b1cc335aa1db3532d29 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9978: 5e7669b1dc9b95218d7deb4709a83ed1905b54b3 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
5e7669b1dc9b drm/i915: use the SW-based pw->hw_enabled check instead of reading registers
49dc6855112d drm/i915: move lookup_power_well() up
7d5567ea0755 drm/i915: use for_each_power_well in lookup_power_well()
c740d932780f drm/i915: WARN() if we can't lookup_power_well()
20b826691fa5 drm/i915: kill intel_display_power_well_is_enabled()
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9978/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [1/5] drm/i915: kill intel_display_power_well_is_enabled()
2018-08-20 23:31 [PATCH 1/5] drm/i915: kill intel_display_power_well_is_enabled() Paulo Zanoni
` (5 preceding siblings ...)
2018-08-20 23:55 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-08-21 0:45 ` Patchwork
6 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2018-08-21 0:45 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/5] drm/i915: kill intel_display_power_well_is_enabled()
URL : https://patchwork.freedesktop.org/series/48466/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4691_full -> Patchwork_9978_full =
== Summary - SUCCESS ==
No regressions found.
== Known issues ==
Here are the changes found in Patchwork_9978_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_ppgtt@blt-vs-render-ctxn:
shard-kbl: PASS -> INCOMPLETE (fdo#103665, fdo#106023)
igt@kms_fbcon_fbt@fbc-suspend:
shard-kbl: PASS -> DMESG-WARN (fdo#103313)
igt@kms_setmode@basic:
shard-kbl: PASS -> FAIL (fdo#99912)
==== Possible fixes ====
igt@drv_suspend@shrink:
shard-glk: FAIL (fdo#106886) -> PASS
igt@kms_setmode@basic:
shard-apl: FAIL (fdo#99912) -> PASS
fdo#103313 https://bugs.freedesktop.org/show_bug.cgi?id=103313
fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
== Participating hosts (5 -> 5) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_4691 -> Patchwork_9978
CI_DRM_4691: 2d75266982a5dae956c10e683a1a74d977d88e09 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4607: 6e0b3e7a2d241af36f8c6b1cc335aa1db3532d29 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9978: 5e7669b1dc9b95218d7deb4709a83ed1905b54b3 @ 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_9978/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/5] drm/i915: WARN() if we can't lookup_power_well()
2018-08-20 23:31 ` [PATCH 2/5] drm/i915: WARN() if we can't lookup_power_well() Paulo Zanoni
@ 2018-08-23 13:41 ` Imre Deak
0 siblings, 0 replies; 11+ messages in thread
From: Imre Deak @ 2018-08-23 13:41 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
On Mon, Aug 20, 2018 at 04:31:36PM -0700, Paulo Zanoni wrote:
> None of the current lookup_power_well() callers are actually checking
> for NULL return values, they all just use the pointer right away. The
> first idea was to replace these theoretical segfaults with a BUG()
> since this would at least make our code a little more explicit to the
> reader. It was suggested that just converting the BUG() to a WARN()
> and returning any power well would probably be better since it would
> still keep the system running while at the same time exposing the
> driver bug.
>
> We can only hit this NULL/BUG()/WARN() condition if we try to lookup a
> power well that isn't defined on a given platform. If that ever
> happens, we have to fix our code, making it lookup the correct power
> well. Because of this, I don't think it's worth trying to implement
> error checking in every caller. Improving our CI system will be a
> better use of our time once a bug is found in the wild.
>
> v2: Avoid the BUG() with a WARN() return a random PW (Michal).
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_runtime_pm.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index f75837e45144..f07ed70b839f 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1096,7 +1096,15 @@ lookup_power_well(struct drm_i915_private *dev_priv,
> return power_well;
> }
>
> - return NULL;
> + /*
> + * It's not feasible to add error checking code to the callers since
> + * this condition really shouldn't happen and it doesn't even make sense
> + * to abort things like display initialization sequences. Just return
> + * the first power well and hope the WARN gets reported so we can fix
> + * our driver.
> + */
The first power well is the always-on power well on all platforms, which
has nop get/put handlers so comes handy to use it here, making side effects
less likely. The change makes sense:
Reviewed-by: Imre Deak <imre.deak@intel.com>
> + WARN(1, "Power well %d not defined for this platform\n", power_well_id);
> + return &power_domains->power_wells[0];
> }
>
> #define BITS_SET(val, bits) (((val) & (bits)) == (bits))
> --
> 2.14.4
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/5] drm/i915: move lookup_power_well() up
2018-08-20 23:31 ` [PATCH 4/5] drm/i915: move lookup_power_well() up Paulo Zanoni
@ 2018-08-23 13:44 ` Imre Deak
0 siblings, 0 replies; 11+ messages in thread
From: Imre Deak @ 2018-08-23 13:44 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
On Mon, Aug 20, 2018 at 04:31:38PM -0700, Paulo Zanoni wrote:
> There's no need for that forward declaration.
>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/intel_runtime_pm.c | 46 +++++++++++++++------------------
> 1 file changed, 21 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6f5a2f00a12d..f38a049861e6 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -49,10 +49,6 @@
> * present for a given platform.
> */
>
> -static struct i915_power_well *
> -lookup_power_well(struct drm_i915_private *dev_priv,
> - enum i915_power_well_id power_well_id);
> -
> const char *
> intel_display_power_domain_str(enum intel_display_power_domain domain)
> {
> @@ -649,6 +645,27 @@ static void assert_csr_loaded(struct drm_i915_private *dev_priv)
> WARN_ONCE(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n");
> }
>
> +static struct i915_power_well *
> +lookup_power_well(struct drm_i915_private *dev_priv,
> + enum i915_power_well_id power_well_id)
> +{
> + struct i915_power_well *power_well;
> +
> + for_each_power_well(dev_priv, power_well)
> + if (power_well->desc->id == power_well_id)
> + return power_well;
> +
> + /*
> + * It's not feasible to add error checking code to the callers since
> + * this condition really shouldn't happen and it doesn't even make sense
> + * to abort things like display initialization sequences. Just return
> + * the first power well and hope the WARN gets reported so we can fix
> + * our driver.
> + */
> + WARN(1, "Power well %d not defined for this platform\n", power_well_id);
> + return &dev_priv->power_domains.power_wells[0];
> +}
> +
> static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
> {
> struct i915_power_well *pg2 = lookup_power_well(dev_priv,
> @@ -1081,27 +1098,6 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>
> #define POWER_DOMAIN_MASK (GENMASK_ULL(POWER_DOMAIN_NUM - 1, 0))
>
> -static struct i915_power_well *
> -lookup_power_well(struct drm_i915_private *dev_priv,
> - enum i915_power_well_id power_well_id)
> -{
> - struct i915_power_well *power_well;
> -
> - for_each_power_well(dev_priv, power_well)
> - if (power_well->desc->id == power_well_id)
> - return power_well;
> -
> - /*
> - * It's not feasible to add error checking code to the callers since
> - * this condition really shouldn't happen and it doesn't even make sense
> - * to abort things like display initialization sequences. Just return
> - * the first power well and hope the WARN gets reported so we can fix
> - * our driver.
> - */
> - WARN(1, "Power well %d not defined for this platform\n", power_well_id);
> - return &dev_priv->power_domains.power_wells[0];
> -}
> -
> #define BITS_SET(val, bits) (((val) & (bits)) == (bits))
>
> static void assert_chv_phy_status(struct drm_i915_private *dev_priv)
> --
> 2.14.4
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 5/5] drm/i915: use the SW-based pw->hw_enabled check instead of reading registers
2018-08-20 23:31 ` [PATCH 5/5] drm/i915: use the SW-based pw->hw_enabled check instead of reading registers Paulo Zanoni
@ 2018-08-23 13:48 ` Imre Deak
0 siblings, 0 replies; 11+ messages in thread
From: Imre Deak @ 2018-08-23 13:48 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
On Mon, Aug 20, 2018 at 04:31:39PM -0700, Paulo Zanoni wrote:
> I can't find a reason why we would want to call is_enabled(), which
> does a register read, instead of just relying on our tracking with
> hw_enabled. Let's try to trust our hardware sync.
The software state is indirect, so not sure why we should check that
instead. The register read doesn't have considerable overhead and it's
not on a speed critical path anyway.
>
> Cc: Imre Deak <imre.deak@intel.com>
> Requested-by: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index f38a049861e6..76bb2e06fef1 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -670,9 +670,8 @@ static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
> {
> struct i915_power_well *pg2 = lookup_power_well(dev_priv,
> SKL_DISP_PW_2);
> - bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv, pg2);
>
> - WARN_ONCE(pg2_enabled, "PG2 not disabled to enable DC5.\n");
> + WARN_ONCE(pg2->hw_enabled, "PG2 not disabled to enable DC5.\n");
>
> WARN_ONCE((I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5),
> "DC5 already programmed to be enabled.\n");
> --
> 2.14.4
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-08-23 13:49 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-20 23:31 [PATCH 1/5] drm/i915: kill intel_display_power_well_is_enabled() Paulo Zanoni
2018-08-20 23:31 ` [PATCH 2/5] drm/i915: WARN() if we can't lookup_power_well() Paulo Zanoni
2018-08-23 13:41 ` Imre Deak
2018-08-20 23:31 ` [PATCH 3/5] drm/i915: use for_each_power_well in lookup_power_well() Paulo Zanoni
2018-08-20 23:31 ` [PATCH 4/5] drm/i915: move lookup_power_well() up Paulo Zanoni
2018-08-23 13:44 ` Imre Deak
2018-08-20 23:31 ` [PATCH 5/5] drm/i915: use the SW-based pw->hw_enabled check instead of reading registers Paulo Zanoni
2018-08-23 13:48 ` Imre Deak
2018-08-20 23:37 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/5] drm/i915: kill intel_display_power_well_is_enabled() Patchwork
2018-08-20 23:55 ` ✓ Fi.CI.BAT: success " Patchwork
2018-08-21 0:45 ` ✓ Fi.CI.IGT: " Patchwork
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.