* [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values
@ 2018-11-19 14:41 Imre Deak
2018-11-19 14:41 ` [PATCH 2/3] drm/i915: Make EDP PSR flags " Imre Deak
` (6 more replies)
0 siblings, 7 replies; 20+ messages in thread
From: Imre Deak @ 2018-11-19 14:41 UTC (permalink / raw)
To: intel-gfx
Depending on the transcoder enum values to translate from transcoder
to pipe/transcoder register addresses can easily break if we add a new
transcoder. So remove the dependency by using named initializers.
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_pci.c | 52 ++++++++++++++++++++++++++++++-----------
1 file changed, 38 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 983ae7fd8217..1b81d7cb209e 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -33,16 +33,30 @@
#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
#define GEN_DEFAULT_PIPEOFFSETS \
- .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
- PIPE_C_OFFSET, PIPE_EDP_OFFSET }, \
- .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
- TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET }
+ .pipe_offsets = { \
+ [TRANSCODER_A] = PIPE_A_OFFSET, \
+ [TRANSCODER_B] = PIPE_B_OFFSET, \
+ [TRANSCODER_C] = PIPE_C_OFFSET, \
+ [TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
+ }, \
+ .trans_offsets = { \
+ [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+ [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+ [TRANSCODER_C] = TRANSCODER_C_OFFSET, \
+ [TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
+ }
#define GEN_CHV_PIPEOFFSETS \
- .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
- CHV_PIPE_C_OFFSET }, \
- .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
- CHV_TRANSCODER_C_OFFSET }
+ .pipe_offsets = { \
+ [TRANSCODER_A] = PIPE_A_OFFSET, \
+ [TRANSCODER_B] = PIPE_B_OFFSET, \
+ [TRANSCODER_C] = CHV_PIPE_C_OFFSET, \
+ }, \
+ .trans_offsets = { \
+ [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+ [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+ [TRANSCODER_C] = CHV_TRANSCODER_C_OFFSET, \
+ }
#define CURSOR_OFFSETS \
.cursor_offsets = { CURSOR_A_OFFSET, CURSOR_B_OFFSET, CHV_CURSOR_C_OFFSET }
@@ -592,12 +606,22 @@ static const struct intel_device_info intel_cannonlake_info = {
#define GEN11_FEATURES \
GEN10_FEATURES, \
- .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
- PIPE_C_OFFSET, PIPE_EDP_OFFSET, \
- PIPE_DSI0_OFFSET, PIPE_DSI1_OFFSET }, \
- .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
- TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET, \
- TRANSCODER_DSI0_OFFSET, TRANSCODER_DSI1_OFFSET}, \
+ .pipe_offsets = { \
+ [TRANSCODER_A] = PIPE_A_OFFSET, \
+ [TRANSCODER_B] = PIPE_B_OFFSET, \
+ [TRANSCODER_C] = PIPE_C_OFFSET, \
+ [TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
+ [TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET, \
+ [TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET, \
+ }, \
+ .trans_offsets = { \
+ [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
+ [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
+ [TRANSCODER_C] = TRANSCODER_C_OFFSET, \
+ [TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
+ [TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
+ [TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
+ }, \
GEN(11), \
.ddb_size = 2048, \
.has_logical_ring_elsq = 1
--
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/3] drm/i915: Make EDP PSR flags not depend on enum values
2018-11-19 14:41 [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Imre Deak
@ 2018-11-19 14:41 ` Imre Deak
2018-11-19 20:46 ` [PATCH v2 " Imre Deak
2018-11-19 14:41 ` [PATCH " Imre Deak
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Imre Deak @ 2018-11-19 14:41 UTC (permalink / raw)
To: intel-gfx; +Cc: Lucas De Marchi
Depending on the transcoder enum values to translate from transcoder
to EDP PSR flags can easily break if we add a new transcoder. So remove
the dependency by using an explicit mapping.
While at it also add a WARN for unexpected trancoders.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 10 ++++--
drivers/gpu/drm/i915/intel_psr.c | 70 +++++++++++++++++++++++++++++-----------
2 files changed, 59 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index edb58af1e903..25b069175c2a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4150,9 +4150,13 @@ enum {
/* Bspec claims those aren't shifted but stay at 0x64800 */
#define EDP_PSR_IMR _MMIO(0x64834)
#define EDP_PSR_IIR _MMIO(0x64838)
-#define EDP_PSR_ERROR(trans) (1 << (((trans) * 8 + 10) & 31))
-#define EDP_PSR_POST_EXIT(trans) (1 << (((trans) * 8 + 9) & 31))
-#define EDP_PSR_PRE_ENTRY(trans) (1 << (((trans) * 8 + 8) & 31))
+#define _EDP_PSR_ERROR(idx) (1 << ((idx) * 8 + 2))
+#define _EDP_PSR_POST_EXIT(idx) (1 << ((idx) * 8 + 1))
+#define _EDP_PSR_PRE_ENTRY(idx) (1 << ((idx) * 8))
+#define _EDP_PSR_TRANSCODER_A_IDX 1
+#define _EDP_PSR_TRANSCODER_B_IDX 2
+#define _EDP_PSR_TRANSCODER_C_IDX 3
+#define _EDP_PSR_TRANSCODER_EDP_IDX 0
#define EDP_PSR_AUX_CTL _MMIO(dev_priv->psr_mmio_base + 0x10)
#define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 48df16a02fac..5fdc2f196045 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -83,25 +83,59 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
}
}
+static int edp_psr_transcoder_idx(enum transcoder trans)
+{
+ int trans_to_idx[] = {
+ [TRANSCODER_A] = _EDP_PSR_TRANSCODER_A_IDX,
+ [TRANSCODER_B] = _EDP_PSR_TRANSCODER_B_IDX,
+ [TRANSCODER_C] = _EDP_PSR_TRANSCODER_C_IDX,
+ [TRANSCODER_EDP] = _EDP_PSR_TRANSCODER_EDP_IDX,
+ };
+
+ switch (trans) {
+ case TRANSCODER_A:
+ case TRANSCODER_B:
+ case TRANSCODER_C:
+ case TRANSCODER_EDP:
+ return trans_to_idx[trans];
+ default:
+ MISSING_CASE(trans);
+ return trans_to_idx[TRANSCODER_EDP];
+ }
+}
+
+static u32 edp_psr_error(enum transcoder trans)
+{
+ return _EDP_PSR_ERROR(edp_psr_transcoder_idx(trans));
+}
+
+static u32 edp_psr_post_exit(enum transcoder trans)
+{
+ return _EDP_PSR_POST_EXIT(edp_psr_transcoder_idx(trans));
+}
+
+static u32 edp_psr_pre_entry(enum transcoder trans)
+{
+ return _EDP_PSR_PRE_ENTRY(edp_psr_transcoder_idx(trans));
+}
+
void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
{
u32 debug_mask, mask;
+ enum transcoder cpu_transcoder;
+ u32 transcoders = BIT(TRANSCODER_EDP);
- mask = EDP_PSR_ERROR(TRANSCODER_EDP);
- debug_mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
-
- if (INTEL_GEN(dev_priv) >= 8) {
- mask |= EDP_PSR_ERROR(TRANSCODER_A) |
- EDP_PSR_ERROR(TRANSCODER_B) |
- EDP_PSR_ERROR(TRANSCODER_C);
-
- debug_mask |= EDP_PSR_POST_EXIT(TRANSCODER_A) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
- EDP_PSR_POST_EXIT(TRANSCODER_B) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
- EDP_PSR_POST_EXIT(TRANSCODER_C) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_C);
+ if (INTEL_GEN(dev_priv) >= 8)
+ transcoders |= BIT(TRANSCODER_A) |
+ BIT(TRANSCODER_B) |
+ BIT(TRANSCODER_C);
+
+ debug_mask = 0;
+ mask = 0;
+ for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
+ mask |= edp_psr_error(cpu_transcoder);
+ debug_mask |= edp_psr_post_exit(cpu_transcoder) |
+ edp_psr_pre_entry(cpu_transcoder);
}
if (debug & I915_PSR_DEBUG_IRQ)
@@ -160,17 +194,17 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
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))
+ 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_PRE_ENTRY(cpu_transcoder)) {
+ if (psr_iir & edp_psr_pre_entry(cpu_transcoder)) {
dev_priv->psr.last_entry_attempt = time_ns;
DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
transcoder_name(cpu_transcoder));
}
- if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
+ if (psr_iir & edp_psr_post_exit(cpu_transcoder)) {
dev_priv->psr.last_exit = time_ns;
DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
transcoder_name(cpu_transcoder));
--
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/3] drm/i915: Add code comment on assumption of pipe==transcoder
2018-11-19 14:41 [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Imre Deak
2018-11-19 14:41 ` [PATCH 2/3] drm/i915: Make EDP PSR flags " Imre Deak
@ 2018-11-19 14:41 ` Imre Deak
2018-11-19 15:51 ` Ville Syrjälä
2018-11-19 15:11 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Patchwork
` (4 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Imre Deak @ 2018-11-19 14:41 UTC (permalink / raw)
To: intel-gfx; +Cc: Lucas De Marchi
Add a comment to the pipe and transcoder enum definitions about our
assumption in the code that pipe==transcoder for PIPE_A-C /
TRANSCODER_A-C. This means we have to keep the values for these
pipe/transcoder enums fixed.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index 43eb4ebbcc35..cbb5d79d6a4c 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -43,6 +43,10 @@ enum i915_gpio {
GPIOM,
};
+/*
+ * Keep the PIPE_A-C values fixed, we assume that pipe==transcoder for
+ * these pipes.
+ */
enum pipe {
INVALID_PIPE = -1,
@@ -56,6 +60,10 @@ enum pipe {
#define pipe_name(p) ((p) + 'A')
+/*
+ * Keep the TRANSCODER_A-C values fixed, we assume that pipe==transcoder for
+ * these transcoders.
+ */
enum transcoder {
TRANSCODER_A = 0,
TRANSCODER_B,
--
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values
2018-11-19 14:41 [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Imre Deak
2018-11-19 14:41 ` [PATCH 2/3] drm/i915: Make EDP PSR flags " Imre Deak
2018-11-19 14:41 ` [PATCH " Imre Deak
@ 2018-11-19 15:11 ` Patchwork
2018-11-19 15:29 ` [PATCH 1/3] " Ville Syrjälä
` (3 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-11-19 15:11 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values
URL : https://patchwork.freedesktop.org/series/52693/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_5160 -> Patchwork_10847 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/52693/revisions/1/mbox/
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_10847:
=== IGT changes ===
==== Possible regressions ====
{igt@runner@aborted}:
fi-icl-u: NOTRUN -> FAIL
== Known issues ==
Here are the changes found in Patchwork_10847 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_ctx_create@basic-files:
fi-icl-u2: PASS -> DMESG-WARN (fdo#107724)
igt@gem_exec_suspend@basic-s3:
fi-cfl-8109u: PASS -> FAIL (fdo#103375)
==== Possible fixes ====
igt@gem_ctx_create@basic-files:
fi-bsw-kefka: FAIL (fdo#108656) -> PASS
igt@gem_exec_suspend@basic-s3:
fi-icl-u2: DMESG-WARN (fdo#107724) -> PASS
igt@gem_mmap_gtt@basic-small-copy:
fi-glk-dsi: INCOMPLETE (fdo#103359, k.org#198133) -> PASS
igt@kms_frontbuffer_tracking@basic:
fi-icl-u: FAIL (fdo#103167) -> PASS
igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
fi-byt-clapper: FAIL (fdo#103191, fdo#107362) -> PASS
==== Warnings ====
igt@i915_selftest@live_contexts:
fi-icl-u: DMESG-FAIL (fdo#108569) -> INCOMPLETE (fdo#108315)
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315
fdo#108569 https://bugs.freedesktop.org/show_bug.cgi?id=108569
fdo#108656 https://bugs.freedesktop.org/show_bug.cgi?id=108656
k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133
== Participating hosts (51 -> 47) ==
Additional (1): fi-pnv-d510
Missing (5): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600
== Build changes ==
* Linux: CI_DRM_5160 -> Patchwork_10847
CI_DRM_5160: 80350f0233d79937675bdc76d7864f51843ccc63 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4720: c27aaca295d3ca2a38521e571c012449371e4bb5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10847: 799f685980adcac64afb2b90df4a0f38330553ff @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
799f685980ad drm/i915: Add code comment on assumption of pipe==transcoder
2dee3a985fdf drm/i915: Make EDP PSR flags not depend on enum values
15efd4a42d55 drm/i915: Make pipe/transcoder offsets not depend on enum values
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10847/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values
2018-11-19 14:41 [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Imre Deak
` (2 preceding siblings ...)
2018-11-19 15:11 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Patchwork
@ 2018-11-19 15:29 ` Ville Syrjälä
2018-11-19 18:54 ` Imre Deak
2018-11-19 19:33 ` ✓ Fi.CI.IGT: success for series starting with [1/3] " Patchwork
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2018-11-19 15:29 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
On Mon, Nov 19, 2018 at 04:41:07PM +0200, Imre Deak wrote:
> Depending on the transcoder enum values to translate from transcoder
> to pipe/transcoder register addresses can easily break if we add a new
> transcoder. So remove the dependency by using named initializers.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/i915_pci.c | 52 ++++++++++++++++++++++++++++++-----------
> 1 file changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 983ae7fd8217..1b81d7cb209e 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -33,16 +33,30 @@
> #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
>
> #define GEN_DEFAULT_PIPEOFFSETS \
> - .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> - PIPE_C_OFFSET, PIPE_EDP_OFFSET }, \
> - .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
> - TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET }
> + .pipe_offsets = { \
> + [TRANSCODER_A] = PIPE_A_OFFSET, \
> + [TRANSCODER_B] = PIPE_B_OFFSET, \
> + [TRANSCODER_C] = PIPE_C_OFFSET, \
> + [TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
Hmm. We do define this as int pipe_offsets[I915_MAX_TRANSCODERS], and
PIPECONF/TRANSCONF is per-transcoder so I suppose using TRANSCODER_foo
here make sense.
Couple of things that came to mind while thinking about this:
- pipe_offsets[] & co. could probably be u32 since we don't
need negative values
- _PIPE_EDP could be removed, which would maybe shrink a few
arrays based on I915_MAX_PIPES
Patch is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> + }, \
> + .trans_offsets = { \
> + [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> + [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> + [TRANSCODER_C] = TRANSCODER_C_OFFSET, \
> + [TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
> + }
>
> #define GEN_CHV_PIPEOFFSETS \
> - .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> - CHV_PIPE_C_OFFSET }, \
> - .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
> - CHV_TRANSCODER_C_OFFSET }
> + .pipe_offsets = { \
> + [TRANSCODER_A] = PIPE_A_OFFSET, \
> + [TRANSCODER_B] = PIPE_B_OFFSET, \
> + [TRANSCODER_C] = CHV_PIPE_C_OFFSET, \
> + }, \
> + .trans_offsets = { \
> + [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> + [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> + [TRANSCODER_C] = CHV_TRANSCODER_C_OFFSET, \
> + }
>
> #define CURSOR_OFFSETS \
> .cursor_offsets = { CURSOR_A_OFFSET, CURSOR_B_OFFSET, CHV_CURSOR_C_OFFSET }
> @@ -592,12 +606,22 @@ static const struct intel_device_info intel_cannonlake_info = {
>
> #define GEN11_FEATURES \
> GEN10_FEATURES, \
> - .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> - PIPE_C_OFFSET, PIPE_EDP_OFFSET, \
> - PIPE_DSI0_OFFSET, PIPE_DSI1_OFFSET }, \
> - .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
> - TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET, \
> - TRANSCODER_DSI0_OFFSET, TRANSCODER_DSI1_OFFSET}, \
> + .pipe_offsets = { \
> + [TRANSCODER_A] = PIPE_A_OFFSET, \
> + [TRANSCODER_B] = PIPE_B_OFFSET, \
> + [TRANSCODER_C] = PIPE_C_OFFSET, \
> + [TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
> + [TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET, \
> + [TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET, \
> + }, \
> + .trans_offsets = { \
> + [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> + [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> + [TRANSCODER_C] = TRANSCODER_C_OFFSET, \
> + [TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
> + [TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
> + [TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
> + }, \
> GEN(11), \
> .ddb_size = 2048, \
> .has_logical_ring_elsq = 1
> --
> 2.13.2
--
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] 20+ messages in thread
* Re: [PATCH 3/3] drm/i915: Add code comment on assumption of pipe==transcoder
2018-11-19 14:41 ` [PATCH " Imre Deak
@ 2018-11-19 15:51 ` Ville Syrjälä
2018-11-19 18:43 ` Imre Deak
0 siblings, 1 reply; 20+ messages in thread
From: Ville Syrjälä @ 2018-11-19 15:51 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx, Lucas De Marchi
On Mon, Nov 19, 2018 at 04:41:09PM +0200, Imre Deak wrote:
> Add a comment to the pipe and transcoder enum definitions about our
> assumption in the code that pipe==transcoder for PIPE_A-C /
> TRANSCODER_A-C. This means we have to keep the values for these
> pipe/transcoder enums fixed.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index 43eb4ebbcc35..cbb5d79d6a4c 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -43,6 +43,10 @@ enum i915_gpio {
> GPIOM,
> };
>
> +/*
> + * Keep the PIPE_A-C values fixed, we assume that pipe==transcoder for
> + * these pipes.
> + */
I suspect the A-C part is going to bitrot. Also I'm sure we
make more assupmtions about these values (PIPE_A == 0,
PIPE_N+1 == n+1, etc.). We should probably try to spell it
all out here.
> enum pipe {
> INVALID_PIPE = -1,
>
> @@ -56,6 +60,10 @@ enum pipe {
>
> #define pipe_name(p) ((p) + 'A')
>
> +/*
> + * Keep the TRANSCODER_A-C values fixed, we assume that pipe==transcoder for
> + * these transcoders.
> + */
Same issue with the A-C part perhaps.
> enum transcoder {
> TRANSCODER_A = 0,
> TRANSCODER_B,
> --
> 2.13.2
--
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] 20+ messages in thread
* Re: [PATCH 3/3] drm/i915: Add code comment on assumption of pipe==transcoder
2018-11-19 15:51 ` Ville Syrjälä
@ 2018-11-19 18:43 ` Imre Deak
2018-11-19 18:55 ` Ville Syrjälä
2018-11-19 22:06 ` Lucas De Marchi
0 siblings, 2 replies; 20+ messages in thread
From: Imre Deak @ 2018-11-19 18:43 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Lucas De Marchi
On Mon, Nov 19, 2018 at 05:51:31PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 19, 2018 at 04:41:09PM +0200, Imre Deak wrote:
> > Add a comment to the pipe and transcoder enum definitions about our
> > assumption in the code that pipe==transcoder for PIPE_A-C /
> > TRANSCODER_A-C. This means we have to keep the values for these
> > pipe/transcoder enums fixed.
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index 43eb4ebbcc35..cbb5d79d6a4c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -43,6 +43,10 @@ enum i915_gpio {
> > GPIOM,
> > };
> >
> > +/*
> > + * Keep the PIPE_A-C values fixed, we assume that pipe==transcoder for
> > + * these pipes.
> > + */
>
> I suspect the A-C part is going to bitrot. Also I'm sure we
> make more assupmtions about these values (PIPE_A == 0,
> PIPE_N+1 == n+1, etc.). We should probably try to spell it
> all out here.
Ok, can add instead:
/*
* Keep the pipe enum values fixed: the code assumes that PIPE_A=0, the
* rest have consecutive values and match the enum values of transcoders
* with a 1:1 transcoder->pipe mapping.
*/
>
> > enum pipe {
> > INVALID_PIPE = -1,
> >
> > @@ -56,6 +60,10 @@ enum pipe {
> >
> > #define pipe_name(p) ((p) + 'A')
> >
> > +/*
> > + * Keep the TRANSCODER_A-C values fixed, we assume that pipe==transcoder for
> > + * these transcoders.
> > + */
>
> Same issue with the A-C part perhaps.
and here:
>
> > enum transcoder {
/*
* The following transcoders have a 1:1 transcoder->pipe mapping, keep
* their values fixed: the code assumes that TRANSCODER_A=0, the rest
* have consecutive values and match the enum values of the pipes they map
* to.
*/
> > TRANSCODER_A = 0,
> > TRANSCODER_B,
> > TRANSCODER_C,
/*
* The following transcoders can map to any pipe, their enum value
* doesn't need to stay fixed.
*/
> > TRANSCODER_EDP,
> > TRANSCODER_DSI_0,
> > TRANSCODER_DSI_1,
> > --
> > 2.13.2
>
> --
> 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] 20+ messages in thread
* Re: [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values
2018-11-19 15:29 ` [PATCH 1/3] " Ville Syrjälä
@ 2018-11-19 18:54 ` Imre Deak
2018-11-20 2:38 ` Zhenyu Wang
0 siblings, 1 reply; 20+ messages in thread
From: Imre Deak @ 2018-11-19 18:54 UTC (permalink / raw)
To: Ville Syrjälä, Zhi Wang, Zhenyu Wang; +Cc: intel-gfx
On Mon, Nov 19, 2018 at 05:29:26PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 19, 2018 at 04:41:07PM +0200, Imre Deak wrote:
> > Depending on the transcoder enum values to translate from transcoder
> > to pipe/transcoder register addresses can easily break if we add a new
> > transcoder. So remove the dependency by using named initializers.
> >
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_pci.c | 52 ++++++++++++++++++++++++++++++-----------
> > 1 file changed, 38 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > index 983ae7fd8217..1b81d7cb209e 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -33,16 +33,30 @@
> > #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
> >
> > #define GEN_DEFAULT_PIPEOFFSETS \
> > - .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> > - PIPE_C_OFFSET, PIPE_EDP_OFFSET }, \
> > - .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
> > - TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET }
> > + .pipe_offsets = { \
> > + [TRANSCODER_A] = PIPE_A_OFFSET, \
> > + [TRANSCODER_B] = PIPE_B_OFFSET, \
> > + [TRANSCODER_C] = PIPE_C_OFFSET, \
> > + [TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
>
> Hmm. We do define this as int pipe_offsets[I915_MAX_TRANSCODERS], and
> PIPECONF/TRANSCONF is per-transcoder so I suppose using TRANSCODER_foo
> here make sense.
>
> Couple of things that came to mind while thinking about this:
> - pipe_offsets[] & co. could probably be u32 since we don't
> need negative values
Ok, can follow up with that.
> - _PIPE_EDP could be removed, which would maybe shrink a few
> arrays based on I915_MAX_PIPES
Agreed. Looks like all the users are in gvt:
- PIPECONF(_PIPE_EDP) should use PIPECONF(TRANSCODER_EDP) instead. The
current code would also break if we added a new transcoder.
- AFAICS
PIPEDSL(_PIPE_EDP)
PIPESTAT(_PIPE_EDP)
PIPE_FRMCOUNT_G4X(_PIPE_EDP)
PIPE_FLIPCOUNT_G4X(_PIPE_EDP)
are bogus, since these registers don't exist.
Adding gvt folks for these.
>
> Patch is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> > + }, \
> > + .trans_offsets = { \
> > + [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> > + [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> > + [TRANSCODER_C] = TRANSCODER_C_OFFSET, \
> > + [TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
> > + }
> >
> > #define GEN_CHV_PIPEOFFSETS \
> > - .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> > - CHV_PIPE_C_OFFSET }, \
> > - .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
> > - CHV_TRANSCODER_C_OFFSET }
> > + .pipe_offsets = { \
> > + [TRANSCODER_A] = PIPE_A_OFFSET, \
> > + [TRANSCODER_B] = PIPE_B_OFFSET, \
> > + [TRANSCODER_C] = CHV_PIPE_C_OFFSET, \
> > + }, \
> > + .trans_offsets = { \
> > + [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> > + [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> > + [TRANSCODER_C] = CHV_TRANSCODER_C_OFFSET, \
> > + }
> >
> > #define CURSOR_OFFSETS \
> > .cursor_offsets = { CURSOR_A_OFFSET, CURSOR_B_OFFSET, CHV_CURSOR_C_OFFSET }
> > @@ -592,12 +606,22 @@ static const struct intel_device_info intel_cannonlake_info = {
> >
> > #define GEN11_FEATURES \
> > GEN10_FEATURES, \
> > - .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> > - PIPE_C_OFFSET, PIPE_EDP_OFFSET, \
> > - PIPE_DSI0_OFFSET, PIPE_DSI1_OFFSET }, \
> > - .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
> > - TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET, \
> > - TRANSCODER_DSI0_OFFSET, TRANSCODER_DSI1_OFFSET}, \
> > + .pipe_offsets = { \
> > + [TRANSCODER_A] = PIPE_A_OFFSET, \
> > + [TRANSCODER_B] = PIPE_B_OFFSET, \
> > + [TRANSCODER_C] = PIPE_C_OFFSET, \
> > + [TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
> > + [TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET, \
> > + [TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET, \
> > + }, \
> > + .trans_offsets = { \
> > + [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> > + [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> > + [TRANSCODER_C] = TRANSCODER_C_OFFSET, \
> > + [TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
> > + [TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
> > + [TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
> > + }, \
> > GEN(11), \
> > .ddb_size = 2048, \
> > .has_logical_ring_elsq = 1
> > --
> > 2.13.2
>
> --
> 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] 20+ messages in thread
* Re: [PATCH 3/3] drm/i915: Add code comment on assumption of pipe==transcoder
2018-11-19 18:43 ` Imre Deak
@ 2018-11-19 18:55 ` Ville Syrjälä
2018-11-19 22:06 ` Lucas De Marchi
1 sibling, 0 replies; 20+ messages in thread
From: Ville Syrjälä @ 2018-11-19 18:55 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx, Lucas De Marchi
On Mon, Nov 19, 2018 at 08:43:27PM +0200, Imre Deak wrote:
> On Mon, Nov 19, 2018 at 05:51:31PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 19, 2018 at 04:41:09PM +0200, Imre Deak wrote:
> > > Add a comment to the pipe and transcoder enum definitions about our
> > > assumption in the code that pipe==transcoder for PIPE_A-C /
> > > TRANSCODER_A-C. This means we have to keep the values for these
> > > pipe/transcoder enums fixed.
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > index 43eb4ebbcc35..cbb5d79d6a4c 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > @@ -43,6 +43,10 @@ enum i915_gpio {
> > > GPIOM,
> > > };
> > >
> > > +/*
> > > + * Keep the PIPE_A-C values fixed, we assume that pipe==transcoder for
> > > + * these pipes.
> > > + */
> >
> > I suspect the A-C part is going to bitrot. Also I'm sure we
> > make more assupmtions about these values (PIPE_A == 0,
> > PIPE_N+1 == n+1, etc.). We should probably try to spell it
> > all out here.
>
> Ok, can add instead:
>
> /*
> * Keep the pipe enum values fixed: the code assumes that PIPE_A=0, the
> * rest have consecutive values and match the enum values of transcoders
> * with a 1:1 transcoder->pipe mapping.
> */
>
> >
> > > enum pipe {
> > > INVALID_PIPE = -1,
> > >
> > > @@ -56,6 +60,10 @@ enum pipe {
> > >
> > > #define pipe_name(p) ((p) + 'A')
> > >
> > > +/*
> > > + * Keep the TRANSCODER_A-C values fixed, we assume that pipe==transcoder for
> > > + * these transcoders.
> > > + */
> >
> > Same issue with the A-C part perhaps.
>
> and here:
>
> >
> > > enum transcoder {
>
> /*
> * The following transcoders have a 1:1 transcoder->pipe mapping, keep
> * their values fixed: the code assumes that TRANSCODER_A=0, the rest
> * have consecutive values and match the enum values of the pipes they map
> * to.
> */
>
>
> > > TRANSCODER_A = 0,
> > > TRANSCODER_B,
> > > TRANSCODER_C,
>
> /*
> * The following transcoders can map to any pipe, their enum value
> * doesn't need to stay fixed.
> */
lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> > > TRANSCODER_EDP,
> > > TRANSCODER_DSI_0,
> > > TRANSCODER_DSI_1,
>
>
> > > --
> > > 2.13.2
> >
> > --
> > Ville Syrjälä
> > Intel
--
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] 20+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values
2018-11-19 14:41 [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Imre Deak
` (3 preceding siblings ...)
2018-11-19 15:29 ` [PATCH 1/3] " Ville Syrjälä
@ 2018-11-19 19:33 ` Patchwork
2018-11-19 21:09 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values (rev2) Patchwork
2018-11-19 22:32 ` ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values (rev4) Patchwork
6 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-11-19 19:33 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values
URL : https://patchwork.freedesktop.org/series/52693/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_5160_full -> Patchwork_10847_full =
== Summary - SUCCESS ==
No regressions found.
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_10847_full:
=== IGT changes ===
==== Possible regressions ====
igt@pm_rpm@pc8-residency:
{shard-iclb}: SKIP -> INCOMPLETE
==== Warnings ====
igt@tools_test@tools_test:
{shard-iclb}: PASS -> SKIP
== Known issues ==
Here are the changes found in Patchwork_10847_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_exec_schedule@pi-ringfull-bsd1:
shard-kbl: NOTRUN -> FAIL (fdo#103158)
igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
{shard-iclb}: PASS -> DMESG-WARN (fdo#107956)
igt@kms_busy@extended-pageflip-hang-newfb-render-b:
shard-glk: PASS -> DMESG-WARN (fdo#107956)
igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
shard-snb: NOTRUN -> DMESG-WARN (fdo#107956) +1
shard-apl: PASS -> DMESG-WARN (fdo#107956)
igt@kms_color@pipe-a-ctm-max:
shard-apl: PASS -> FAIL (fdo#108147)
igt@kms_cursor_crc@cursor-64x21-random:
{shard-iclb}: NOTRUN -> FAIL (fdo#103232)
igt@kms_cursor_crc@cursor-64x21-sliding:
shard-apl: PASS -> FAIL (fdo#103232)
igt@kms_cursor_crc@cursor-size-change:
shard-glk: PASS -> FAIL (fdo#103232)
igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-xtiled:
shard-skl: PASS -> FAIL (fdo#103184)
igt@kms_fbcon_fbt@psr:
{shard-iclb}: NOTRUN -> FAIL (fdo#107882)
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
shard-apl: PASS -> FAIL (fdo#103167) +1
igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-move:
{shard-iclb}: PASS -> FAIL (fdo#103167)
igt@kms_hdmi_inject@inject-audio:
{shard-iclb}: NOTRUN -> FAIL (fdo#102370)
igt@kms_pipe_crc_basic@read-crc-pipe-b:
shard-skl: NOTRUN -> FAIL (fdo#107362)
igt@kms_plane@plane-position-covered-pipe-c-planes:
shard-glk: PASS -> FAIL (fdo#103166) +1
igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
shard-glk: PASS -> FAIL (fdo#108145) +1
igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
shard-skl: PASS -> FAIL (fdo#107815)
igt@kms_plane_multiple@atomic-pipe-c-tiling-y:
shard-apl: PASS -> FAIL (fdo#103166)
igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
{shard-iclb}: PASS -> FAIL (fdo#103166) +1
igt@kms_plane_scaling@pipe-b-scaler-with-rotation:
{shard-iclb}: NOTRUN -> DMESG-WARN (fdo#107724)
igt@kms_setmode@basic:
shard-apl: PASS -> FAIL (fdo#99912)
igt@pm_rpm@debugfs-read:
shard-skl: PASS -> INCOMPLETE (fdo#107807) +1
igt@prime_vgem@busy-bsd:
shard-apl: PASS -> INCOMPLETE (fdo#103927)
==== Possible fixes ====
igt@gem_eio@in-flight-10ms:
shard-glk: FAIL (fdo#107799) -> PASS
igt@gem_exec_whisper@normal:
shard-skl: TIMEOUT (fdo#108592) -> PASS
igt@gem_render_copy_redux@normal:
shard-kbl: INCOMPLETE (fdo#106650, fdo#103665) -> PASS
igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
shard-skl: FAIL (fdo#108470, fdo#108228) -> PASS
igt@kms_available_modes_crc@available_mode_test_crc:
shard-apl: FAIL (fdo#106641) -> PASS
igt@kms_color@pipe-c-degamma:
shard-apl: FAIL (fdo#104782) -> PASS
igt@kms_color@pipe-c-gamma:
shard-skl: FAIL (fdo#104782, fdo#108228) -> PASS
igt@kms_cursor_crc@cursor-128x128-random:
shard-apl: FAIL (fdo#103232) -> PASS +5
igt@kms_cursor_crc@cursor-256x256-dpms:
shard-skl: FAIL (fdo#103232) -> PASS +1
igt@kms_cursor_crc@cursor-64x21-onscreen:
shard-glk: FAIL (fdo#103232) -> PASS +1
igt@kms_cursor_crc@cursor-64x64-suspend:
shard-apl: FAIL (fdo#103232, fdo#103191) -> PASS +1
igt@kms_draw_crc@draw-method-xrgb2101010-mmap-wc-xtiled:
shard-skl: FAIL (fdo#103184) -> PASS
igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-xtiled:
{shard-iclb}: WARN (fdo#108336) -> PASS +3
igt@kms_flip@flip-vs-expired-vblank:
shard-skl: FAIL (fdo#105363) -> PASS
shard-glk: FAIL (fdo#105363) -> PASS
igt@kms_flip_tiling@flip-changes-tiling:
{shard-iclb}: DMESG-WARN (fdo#107724, fdo#108336) -> PASS +12
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
shard-apl: FAIL (fdo#103167) -> PASS +1
igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-mmap-cpu:
shard-skl: FAIL (fdo#103167) -> PASS +6
igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt:
{shard-iclb}: DMESG-FAIL (fdo#107724) -> PASS +5
igt@kms_plane@plane-position-covered-pipe-a-planes:
shard-glk: FAIL (fdo#103166) -> PASS
igt@kms_plane@plane-position-covered-pipe-c-planes:
shard-apl: FAIL (fdo#103166) -> PASS +1
igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
shard-skl: FAIL (fdo#107815, fdo#108145) -> PASS
igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
{shard-iclb}: FAIL (fdo#103166) -> PASS
igt@kms_psr@sprite_mmap_gtt:
{shard-iclb}: DMESG-WARN (fdo#107724) -> PASS +22
igt@pm_rpm@universal-planes:
shard-skl: INCOMPLETE (fdo#107807) -> PASS
==== Warnings ====
igt@i915_suspend@shrink:
shard-skl: DMESG-WARN (fdo#108784) -> INCOMPLETE (fdo#106886)
igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-mmap-cpu:
{shard-iclb}: DMESG-FAIL (fdo#107724) -> FAIL (fdo#103167)
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
fdo#102370 https://bugs.freedesktop.org/show_bug.cgi?id=102370
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#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fdo#104782 https://bugs.freedesktop.org/show_bug.cgi?id=104782
fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
fdo#106650 https://bugs.freedesktop.org/show_bug.cgi?id=106650
fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
fdo#107799 https://bugs.freedesktop.org/show_bug.cgi?id=107799
fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
fdo#107815 https://bugs.freedesktop.org/show_bug.cgi?id=107815
fdo#107882 https://bugs.freedesktop.org/show_bug.cgi?id=107882
fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
fdo#108147 https://bugs.freedesktop.org/show_bug.cgi?id=108147
fdo#108228 https://bugs.freedesktop.org/show_bug.cgi?id=108228
fdo#108336 https://bugs.freedesktop.org/show_bug.cgi?id=108336
fdo#108470 https://bugs.freedesktop.org/show_bug.cgi?id=108470
fdo#108592 https://bugs.freedesktop.org/show_bug.cgi?id=108592
fdo#108784 https://bugs.freedesktop.org/show_bug.cgi?id=108784
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
== Participating hosts (7 -> 7) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_5160 -> Patchwork_10847
CI_DRM_5160: 80350f0233d79937675bdc76d7864f51843ccc63 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4720: c27aaca295d3ca2a38521e571c012449371e4bb5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10847: 799f685980adcac64afb2b90df4a0f38330553ff @ 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_10847/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 2/3] drm/i915: Make EDP PSR flags not depend on enum values
2018-11-19 14:41 ` [PATCH 2/3] drm/i915: Make EDP PSR flags " Imre Deak
@ 2018-11-19 20:46 ` Imre Deak
2018-11-19 20:58 ` Ville Syrjälä
2018-11-19 21:56 ` [PATCH v3 " Imre Deak
0 siblings, 2 replies; 20+ messages in thread
From: Imre Deak @ 2018-11-19 20:46 UTC (permalink / raw)
To: intel-gfx; +Cc: Lucas De Marchi
Depending on the transcoder enum values to translate from transcoder
to EDP PSR flags can easily break if we add a new transcoder. So remove
the dependency by using an explicit mapping.
While at it also add a WARN for unexpected trancoders.
v2:
- Simplify things by defining flag shift values instead of indices.
- s/trans/cpu_transcoder/ (Ville)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 10 +++++---
drivers/gpu/drm/i915/intel_psr.c | 55 +++++++++++++++++++++++++++-------------
2 files changed, 44 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index edb58af1e903..f80b16e3ff33 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4150,9 +4150,13 @@ enum {
/* Bspec claims those aren't shifted but stay at 0x64800 */
#define EDP_PSR_IMR _MMIO(0x64834)
#define EDP_PSR_IIR _MMIO(0x64838)
-#define EDP_PSR_ERROR(trans) (1 << (((trans) * 8 + 10) & 31))
-#define EDP_PSR_POST_EXIT(trans) (1 << (((trans) * 8 + 9) & 31))
-#define EDP_PSR_PRE_ENTRY(trans) (1 << (((trans) * 8 + 8) & 31))
+#define EDP_PSR_ERROR(shift) (4 << (shift))
+#define EDP_PSR_POST_EXIT(shift) (2 << (shift))
+#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
+#define EDP_PSR_TRANSCODER_C_SHIFT 24
+#define EDP_PSR_TRANSCODER_B_SHIFT 16
+#define EDP_PSR_TRANSCODER_A_SHIFT 8
+#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
#define EDP_PSR_AUX_CTL _MMIO(dev_priv->psr_mmio_base + 0x10)
#define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 48df16a02fac..26292961d693 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -83,25 +83,42 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
}
}
+static int edp_psr_shift(enum transcoder cpu_transcoder)
+{
+ switch (cpu_transcoder) {
+ case TRANSCODER_A:
+ return EDP_PSR_TRANSCODER_A_SHIFT;
+ case TRANSCODER_B:
+ return EDP_PSR_TRANSCODER_B_SHIFT;
+ case TRANSCODER_C:
+ return EDP_PSR_TRANSCODER_C_SHIFT;
+ default:
+ MISSING_CASE(cpu_transcoder);
+ /* fallthrough */
+ case TRANSCODER_EDP:
+ return EDP_PSR_TRANSCODER_EDP_SHIFT;
+ }
+}
+
void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
{
u32 debug_mask, mask;
+ enum transcoder cpu_transcoder;
+ u32 transcoders = BIT(TRANSCODER_EDP);
- mask = EDP_PSR_ERROR(TRANSCODER_EDP);
- debug_mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
-
- if (INTEL_GEN(dev_priv) >= 8) {
- mask |= EDP_PSR_ERROR(TRANSCODER_A) |
- EDP_PSR_ERROR(TRANSCODER_B) |
- EDP_PSR_ERROR(TRANSCODER_C);
-
- debug_mask |= EDP_PSR_POST_EXIT(TRANSCODER_A) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
- EDP_PSR_POST_EXIT(TRANSCODER_B) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
- EDP_PSR_POST_EXIT(TRANSCODER_C) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_C);
+ if (INTEL_GEN(dev_priv) >= 8)
+ transcoders |= BIT(TRANSCODER_A) |
+ BIT(TRANSCODER_B) |
+ BIT(TRANSCODER_C);
+
+ debug_mask = 0;
+ mask = 0;
+ for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
+ int shift = edp_psr_shift(cpu_transcoder);
+
+ mask |= EDP_PSR_ERROR(shift);
+ debug_mask |= EDP_PSR_POST_EXIT(shift) |
+ EDP_PSR_PRE_ENTRY(shift);
}
if (debug & I915_PSR_DEBUG_IRQ)
@@ -159,18 +176,20 @@ 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) {
+ int shift = edp_psr_shift(cpu_transcoder);
+
/* FIXME: Exit PSR and link train manually when this happens. */
- if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
+ if (psr_iir & EDP_PSR_ERROR(shift))
DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
transcoder_name(cpu_transcoder));
- if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
+ if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
dev_priv->psr.last_entry_attempt = time_ns;
DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
transcoder_name(cpu_transcoder));
}
- if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
+ if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
dev_priv->psr.last_exit = time_ns;
DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
transcoder_name(cpu_transcoder));
--
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/3] drm/i915: Make EDP PSR flags not depend on enum values
2018-11-19 20:46 ` [PATCH v2 " Imre Deak
@ 2018-11-19 20:58 ` Ville Syrjälä
2018-11-19 21:56 ` [PATCH v3 " Imre Deak
1 sibling, 0 replies; 20+ messages in thread
From: Ville Syrjälä @ 2018-11-19 20:58 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx, Lucas De Marchi
On Mon, Nov 19, 2018 at 10:46:37PM +0200, Imre Deak wrote:
> Depending on the transcoder enum values to translate from transcoder
> to EDP PSR flags can easily break if we add a new transcoder. So remove
> the dependency by using an explicit mapping.
>
> While at it also add a WARN for unexpected trancoders.
>
> v2:
> - Simplify things by defining flag shift values instead of indices.
> - s/trans/cpu_transcoder/ (Ville)
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 10 +++++---
> drivers/gpu/drm/i915/intel_psr.c | 55 +++++++++++++++++++++++++++-------------
> 2 files changed, 44 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index edb58af1e903..f80b16e3ff33 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4150,9 +4150,13 @@ enum {
> /* Bspec claims those aren't shifted but stay at 0x64800 */
> #define EDP_PSR_IMR _MMIO(0x64834)
> #define EDP_PSR_IIR _MMIO(0x64838)
> -#define EDP_PSR_ERROR(trans) (1 << (((trans) * 8 + 10) & 31))
> -#define EDP_PSR_POST_EXIT(trans) (1 << (((trans) * 8 + 9) & 31))
> -#define EDP_PSR_PRE_ENTRY(trans) (1 << (((trans) * 8 + 8) & 31))
> +#define EDP_PSR_ERROR(shift) (4 << (shift))
> +#define EDP_PSR_POST_EXIT(shift) (2 << (shift))
> +#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
The 1,2,4 suggest that these are values for the same bitfield, which
they are not. So I would prefer 1<<((shift)+n) etc. instead.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> +#define EDP_PSR_TRANSCODER_C_SHIFT 24
> +#define EDP_PSR_TRANSCODER_B_SHIFT 16
> +#define EDP_PSR_TRANSCODER_A_SHIFT 8
> +#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
>
> #define EDP_PSR_AUX_CTL _MMIO(dev_priv->psr_mmio_base + 0x10)
> #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 48df16a02fac..26292961d693 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -83,25 +83,42 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
> }
> }
>
> +static int edp_psr_shift(enum transcoder cpu_transcoder)
> +{
> + switch (cpu_transcoder) {
> + case TRANSCODER_A:
> + return EDP_PSR_TRANSCODER_A_SHIFT;
> + case TRANSCODER_B:
> + return EDP_PSR_TRANSCODER_B_SHIFT;
> + case TRANSCODER_C:
> + return EDP_PSR_TRANSCODER_C_SHIFT;
> + default:
> + MISSING_CASE(cpu_transcoder);
> + /* fallthrough */
> + case TRANSCODER_EDP:
> + return EDP_PSR_TRANSCODER_EDP_SHIFT;
> + }
> +}
> +
> void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
> {
> u32 debug_mask, mask;
> + enum transcoder cpu_transcoder;
> + u32 transcoders = BIT(TRANSCODER_EDP);
>
> - mask = EDP_PSR_ERROR(TRANSCODER_EDP);
> - debug_mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
> - EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
> -
> - if (INTEL_GEN(dev_priv) >= 8) {
> - mask |= EDP_PSR_ERROR(TRANSCODER_A) |
> - EDP_PSR_ERROR(TRANSCODER_B) |
> - EDP_PSR_ERROR(TRANSCODER_C);
> -
> - debug_mask |= EDP_PSR_POST_EXIT(TRANSCODER_A) |
> - EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
> - EDP_PSR_POST_EXIT(TRANSCODER_B) |
> - EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
> - EDP_PSR_POST_EXIT(TRANSCODER_C) |
> - EDP_PSR_PRE_ENTRY(TRANSCODER_C);
> + if (INTEL_GEN(dev_priv) >= 8)
> + transcoders |= BIT(TRANSCODER_A) |
> + BIT(TRANSCODER_B) |
> + BIT(TRANSCODER_C);
> +
> + debug_mask = 0;
> + mask = 0;
> + for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> + int shift = edp_psr_shift(cpu_transcoder);
> +
> + mask |= EDP_PSR_ERROR(shift);
> + debug_mask |= EDP_PSR_POST_EXIT(shift) |
> + EDP_PSR_PRE_ENTRY(shift);
> }
>
> if (debug & I915_PSR_DEBUG_IRQ)
> @@ -159,18 +176,20 @@ 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) {
> + int shift = edp_psr_shift(cpu_transcoder);
> +
> /* FIXME: Exit PSR and link train manually when this happens. */
> - if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> + if (psr_iir & EDP_PSR_ERROR(shift))
> DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> transcoder_name(cpu_transcoder));
>
> - if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> + if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
> dev_priv->psr.last_entry_attempt = time_ns;
> DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
> transcoder_name(cpu_transcoder));
> }
>
> - if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> + if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
> dev_priv->psr.last_exit = time_ns;
> DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> transcoder_name(cpu_transcoder));
> --
> 2.13.2
--
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] 20+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values (rev2)
2018-11-19 14:41 [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Imre Deak
` (4 preceding siblings ...)
2018-11-19 19:33 ` ✓ Fi.CI.IGT: success for series starting with [1/3] " Patchwork
@ 2018-11-19 21:09 ` Patchwork
2018-11-19 22:32 ` ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values (rev4) Patchwork
6 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-11-19 21:09 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values (rev2)
URL : https://patchwork.freedesktop.org/series/52693/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_5165 -> Patchwork_10853 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/52693/revisions/2/mbox/
== Known issues ==
Here are the changes found in Patchwork_10853 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_exec_suspend@basic-s3:
fi-icl-u: NOTRUN -> INCOMPLETE (fdo#107713)
igt@i915_selftest@live_coherency:
fi-gdg-551: PASS -> DMESG-FAIL (fdo#107164)
igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
fi-byt-clapper: PASS -> FAIL (fdo#107362, fdo#103191)
==== Possible fixes ====
igt@gem_ctx_create@basic-files:
fi-icl-u2: DMESG-WARN (fdo#107724) -> PASS
igt@kms_pipe_crc_basic@read-crc-pipe-a:
fi-byt-clapper: FAIL (fdo#107362) -> PASS
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
fi-byt-clapper: FAIL (fdo#107362, fdo#103191) -> PASS
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107713 https://bugs.freedesktop.org/show_bug.cgi?id=107713
fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
== Participating hosts (53 -> 47) ==
Additional (1): fi-icl-u
Missing (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-u3
== Build changes ==
* Linux: CI_DRM_5165 -> Patchwork_10853
CI_DRM_5165: 1715a40008b3d4d47ed0171925063b07ab0012f7 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4720: c27aaca295d3ca2a38521e571c012449371e4bb5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10853: 6df0f6ae34ea980b362f0595b578dc9e9e0e0b16 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
6df0f6ae34ea drm/i915: Add code comment on assumption of pipe==transcoder
3ad600726483 drm/i915: Make EDP PSR flags not depend on enum values
189055c6a3c6 drm/i915: Make pipe/transcoder offsets not depend on enum values
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10853/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 2/3] drm/i915: Make EDP PSR flags not depend on enum values
2018-11-19 20:46 ` [PATCH v2 " Imre Deak
2018-11-19 20:58 ` Ville Syrjälä
@ 2018-11-19 21:56 ` Imre Deak
2018-11-19 21:56 ` [PATCH v2 3/3] drm/i915: Add code comment on assumption of pipe==transcoder Imre Deak
1 sibling, 1 reply; 20+ messages in thread
From: Imre Deak @ 2018-11-19 21:56 UTC (permalink / raw)
To: intel-gfx; +Cc: Lucas De Marchi
Depending on the transcoder enum values to translate from transcoder
to EDP PSR flags can easily break if we add a new transcoder. So remove
the dependency by using an explicit mapping.
While at it also add a WARN for unexpected trancoders.
v2:
- Simplify things by defining flag shift values instead of indices.
- s/trans/cpu_transcoder/ (Ville)
v3:
- Define flags to look like seperate bits instead of the values of
the same bitfield. (Ville)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 10 +++++---
drivers/gpu/drm/i915/intel_psr.c | 55 +++++++++++++++++++++++++++-------------
2 files changed, 44 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index edb58af1e903..e6b371e986ee 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4150,9 +4150,13 @@ enum {
/* Bspec claims those aren't shifted but stay at 0x64800 */
#define EDP_PSR_IMR _MMIO(0x64834)
#define EDP_PSR_IIR _MMIO(0x64838)
-#define EDP_PSR_ERROR(trans) (1 << (((trans) * 8 + 10) & 31))
-#define EDP_PSR_POST_EXIT(trans) (1 << (((trans) * 8 + 9) & 31))
-#define EDP_PSR_PRE_ENTRY(trans) (1 << (((trans) * 8 + 8) & 31))
+#define EDP_PSR_ERROR(shift) (1 << ((shift) + 2))
+#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1))
+#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
+#define EDP_PSR_TRANSCODER_C_SHIFT 24
+#define EDP_PSR_TRANSCODER_B_SHIFT 16
+#define EDP_PSR_TRANSCODER_A_SHIFT 8
+#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
#define EDP_PSR_AUX_CTL _MMIO(dev_priv->psr_mmio_base + 0x10)
#define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 48df16a02fac..26292961d693 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -83,25 +83,42 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
}
}
+static int edp_psr_shift(enum transcoder cpu_transcoder)
+{
+ switch (cpu_transcoder) {
+ case TRANSCODER_A:
+ return EDP_PSR_TRANSCODER_A_SHIFT;
+ case TRANSCODER_B:
+ return EDP_PSR_TRANSCODER_B_SHIFT;
+ case TRANSCODER_C:
+ return EDP_PSR_TRANSCODER_C_SHIFT;
+ default:
+ MISSING_CASE(cpu_transcoder);
+ /* fallthrough */
+ case TRANSCODER_EDP:
+ return EDP_PSR_TRANSCODER_EDP_SHIFT;
+ }
+}
+
void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
{
u32 debug_mask, mask;
+ enum transcoder cpu_transcoder;
+ u32 transcoders = BIT(TRANSCODER_EDP);
- mask = EDP_PSR_ERROR(TRANSCODER_EDP);
- debug_mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
-
- if (INTEL_GEN(dev_priv) >= 8) {
- mask |= EDP_PSR_ERROR(TRANSCODER_A) |
- EDP_PSR_ERROR(TRANSCODER_B) |
- EDP_PSR_ERROR(TRANSCODER_C);
-
- debug_mask |= EDP_PSR_POST_EXIT(TRANSCODER_A) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
- EDP_PSR_POST_EXIT(TRANSCODER_B) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
- EDP_PSR_POST_EXIT(TRANSCODER_C) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_C);
+ if (INTEL_GEN(dev_priv) >= 8)
+ transcoders |= BIT(TRANSCODER_A) |
+ BIT(TRANSCODER_B) |
+ BIT(TRANSCODER_C);
+
+ debug_mask = 0;
+ mask = 0;
+ for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
+ int shift = edp_psr_shift(cpu_transcoder);
+
+ mask |= EDP_PSR_ERROR(shift);
+ debug_mask |= EDP_PSR_POST_EXIT(shift) |
+ EDP_PSR_PRE_ENTRY(shift);
}
if (debug & I915_PSR_DEBUG_IRQ)
@@ -159,18 +176,20 @@ 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) {
+ int shift = edp_psr_shift(cpu_transcoder);
+
/* FIXME: Exit PSR and link train manually when this happens. */
- if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
+ if (psr_iir & EDP_PSR_ERROR(shift))
DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
transcoder_name(cpu_transcoder));
- if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
+ if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
dev_priv->psr.last_entry_attempt = time_ns;
DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
transcoder_name(cpu_transcoder));
}
- if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
+ if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
dev_priv->psr.last_exit = time_ns;
DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
transcoder_name(cpu_transcoder));
--
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 3/3] drm/i915: Add code comment on assumption of pipe==transcoder
2018-11-19 21:56 ` [PATCH v3 " Imre Deak
@ 2018-11-19 21:56 ` Imre Deak
0 siblings, 0 replies; 20+ messages in thread
From: Imre Deak @ 2018-11-19 21:56 UTC (permalink / raw)
To: intel-gfx; +Cc: Lucas De Marchi
Add a comment to the pipe and transcoder enum definitions about our
assumption in the code about enum values for pipes and transcoders
with a 1:1 transcoder->pipe mapping.
v2:
- Clarify more what are the assumptions about the enum values. (Ville)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index 43eb4ebbcc35..846f63d056fb 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -43,6 +43,11 @@ enum i915_gpio {
GPIOM,
};
+/*
+ * Keep the pipe enum values fixed: the code assumes that PIPE_A=0, the
+ * rest have consecutive values and match the enum values of transcoders
+ * with a 1:1 transcoder->pipe mapping.
+ */
enum pipe {
INVALID_PIPE = -1,
@@ -57,9 +62,20 @@ enum pipe {
#define pipe_name(p) ((p) + 'A')
enum transcoder {
+ /*
+ * The following transcoders have a 1:1 transcoder->pipe mapping, keep
+ * their values fixed: the code assumes that TRANSCODER_A=0, the rest
+ * have consecutive values and match the enum values of the pipes they
+ * map to.
+ */
TRANSCODER_A = 0,
TRANSCODER_B,
TRANSCODER_C,
+
+ /*
+ * The following transcoders can map to any pipe, their enum value
+ * doesn't need to stay fixed.
+ */
TRANSCODER_EDP,
TRANSCODER_DSI_0,
TRANSCODER_DSI_1,
--
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] drm/i915: Add code comment on assumption of pipe==transcoder
2018-11-19 18:43 ` Imre Deak
2018-11-19 18:55 ` Ville Syrjälä
@ 2018-11-19 22:06 ` Lucas De Marchi
2018-11-19 22:34 ` Imre Deak
1 sibling, 1 reply; 20+ messages in thread
From: Lucas De Marchi @ 2018-11-19 22:06 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx, Lucas De Marchi
On Mon, Nov 19, 2018 at 08:43:27PM +0200, Imre Deak wrote:
> On Mon, Nov 19, 2018 at 05:51:31PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 19, 2018 at 04:41:09PM +0200, Imre Deak wrote:
> > > Add a comment to the pipe and transcoder enum definitions about our
> > > assumption in the code that pipe==transcoder for PIPE_A-C /
> > > TRANSCODER_A-C. This means we have to keep the values for these
> > > pipe/transcoder enums fixed.
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > index 43eb4ebbcc35..cbb5d79d6a4c 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > @@ -43,6 +43,10 @@ enum i915_gpio {
> > > GPIOM,
> > > };
> > >
> > > +/*
> > > + * Keep the PIPE_A-C values fixed, we assume that pipe==transcoder for
> > > + * these pipes.
> > > + */
> >
> > I suspect the A-C part is going to bitrot. Also I'm sure we
> > make more assupmtions about these values (PIPE_A == 0,
> > PIPE_N+1 == n+1, etc.). We should probably try to spell it
> > all out here.
>
> Ok, can add instead:
>
> /*
> * Keep the pipe enum values fixed: the code assumes that PIPE_A=0, the
> * rest have consecutive values and match the enum values of transcoders
> * with a 1:1 transcoder->pipe mapping.
1:1 transcoder <-> pipe mapping, so it doesn't look like it's a pointer :)
> */
>
> >
> > > enum pipe {
> > > INVALID_PIPE = -1,
> > >
> > > @@ -56,6 +60,10 @@ enum pipe {
> > >
> > > #define pipe_name(p) ((p) + 'A')
> > >
> > > +/*
> > > + * Keep the TRANSCODER_A-C values fixed, we assume that pipe==transcoder for
> > > + * these transcoders.
> > > + */
> >
> > Same issue with the A-C part perhaps.
>
> and here:
>
> >
> > > enum transcoder {
>
> /*
> * The following transcoders have a 1:1 transcoder->pipe mapping, keep
> * their values fixed: the code assumes that TRANSCODER_A=0, the rest
> * have consecutive values and match the enum values of the pipes they map
> * to.
> */
>
>
> > > TRANSCODER_A = 0,
> > > TRANSCODER_B,
> > > TRANSCODER_C,
could we additionally do like below?
TRANSCODER_A = PIPE_A,
TRANSCODER_B = PIPE_B,
TRANSCODER_C = PIPE_C,
Or at least a BUILD_BUG_ON(TRANSCODER_A != PIPE_A || TRANSCODER_B != PIPE_B || TRANSCODER_C != PIPE_C)
Lucas De Marchi
>
> /*
> * The following transcoders can map to any pipe, their enum value
> * doesn't need to stay fixed.
> */
>
> > > TRANSCODER_EDP,
> > > TRANSCODER_DSI_0,
> > > TRANSCODER_DSI_1,
>
>
> > > --
> > > 2.13.2
> >
> > --
> > Ville Syrjälä
> > Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values (rev4)
2018-11-19 14:41 [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Imre Deak
` (5 preceding siblings ...)
2018-11-19 21:09 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values (rev2) Patchwork
@ 2018-11-19 22:32 ` Patchwork
6 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-11-19 22:32 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values (rev4)
URL : https://patchwork.freedesktop.org/series/52693/
State : failure
== Summary ==
Applying: drm/i915: Make pipe/transcoder offsets not depend on enum values
Applying: drm/i915: Add code comment on assumption of pipe==transcoder
Applying: drm/i915: Add code comment on assumption of pipe==transcoder
Using index info to reconstruct a base tree...
M drivers/gpu/drm/i915/intel_display.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_display.h
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_display.h
error: Failed to merge in the changes.
Patch failed at 0003 drm/i915: Add code comment on assumption of pipe==transcoder
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] drm/i915: Add code comment on assumption of pipe==transcoder
2018-11-19 22:06 ` Lucas De Marchi
@ 2018-11-19 22:34 ` Imre Deak
0 siblings, 0 replies; 20+ messages in thread
From: Imre Deak @ 2018-11-19 22:34 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: intel-gfx, Lucas De Marchi
On Mon, Nov 19, 2018 at 02:06:31PM -0800, Lucas De Marchi wrote:
> On Mon, Nov 19, 2018 at 08:43:27PM +0200, Imre Deak wrote:
> > On Mon, Nov 19, 2018 at 05:51:31PM +0200, Ville Syrjälä wrote:
> > > On Mon, Nov 19, 2018 at 04:41:09PM +0200, Imre Deak wrote:
> > > > Add a comment to the pipe and transcoder enum definitions about our
> > > > assumption in the code that pipe==transcoder for PIPE_A-C /
> > > > TRANSCODER_A-C. This means we have to keep the values for these
> > > > pipe/transcoder enums fixed.
> > > >
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_display.h | 8 ++++++++
> > > > 1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > index 43eb4ebbcc35..cbb5d79d6a4c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > @@ -43,6 +43,10 @@ enum i915_gpio {
> > > > GPIOM,
> > > > };
> > > >
> > > > +/*
> > > > + * Keep the PIPE_A-C values fixed, we assume that pipe==transcoder for
> > > > + * these pipes.
> > > > + */
> > >
> > > I suspect the A-C part is going to bitrot. Also I'm sure we
> > > make more assupmtions about these values (PIPE_A == 0,
> > > PIPE_N+1 == n+1, etc.). We should probably try to spell it
> > > all out here.
> >
> > Ok, can add instead:
> >
> > /*
> > * Keep the pipe enum values fixed: the code assumes that PIPE_A=0, the
> > * rest have consecutive values and match the enum values of transcoders
> > * with a 1:1 transcoder->pipe mapping.
>
> 1:1 transcoder <-> pipe mapping, so it doesn't look like it's a pointer :)
You can only map from transcoder to pipe not the other way around (a pipe
can always be connected to any transcoder). But will add spaces around
'->'.
>
> > */
> >
> > >
> > > > enum pipe {
> > > > INVALID_PIPE = -1,
> > > >
> > > > @@ -56,6 +60,10 @@ enum pipe {
> > > >
> > > > #define pipe_name(p) ((p) + 'A')
> > > >
> > > > +/*
> > > > + * Keep the TRANSCODER_A-C values fixed, we assume that pipe==transcoder for
> > > > + * these transcoders.
> > > > + */
> > >
> > > Same issue with the A-C part perhaps.
> >
> > and here:
> >
> > >
> > > > enum transcoder {
> >
> > /*
> > * The following transcoders have a 1:1 transcoder->pipe mapping, keep
> > * their values fixed: the code assumes that TRANSCODER_A=0, the rest
> > * have consecutive values and match the enum values of the pipes they map
> > * to.
> > */
> >
> >
> > > > TRANSCODER_A = 0,
> > > > TRANSCODER_B,
> > > > TRANSCODER_C,
>
> could we additionally do like below?
>
> TRANSCODER_A = PIPE_A,
> TRANSCODER_B = PIPE_B,
> TRANSCODER_C = PIPE_C,
Good idea, will change it.
> Or at least a BUILD_BUG_ON(TRANSCODER_A != PIPE_A || TRANSCODER_B != PIPE_B || TRANSCODER_C != PIPE_C)
This could get stale.
>
> Lucas De Marchi
>
> >
> > /*
> > * The following transcoders can map to any pipe, their enum value
> > * doesn't need to stay fixed.
> > */
> >
> > > > TRANSCODER_EDP,
> > > > TRANSCODER_DSI_0,
> > > > TRANSCODER_DSI_1,
> >
> >
> > > > --
> > > > 2.13.2
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values
2018-11-19 18:54 ` Imre Deak
@ 2018-11-20 2:38 ` Zhenyu Wang
0 siblings, 0 replies; 20+ messages in thread
From: Zhenyu Wang @ 2018-11-20 2:38 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 5178 bytes --]
On 2018.11.19 20:54:30 +0200, Imre Deak wrote:
> On Mon, Nov 19, 2018 at 05:29:26PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 19, 2018 at 04:41:07PM +0200, Imre Deak wrote:
> > > Depending on the transcoder enum values to translate from transcoder
> > > to pipe/transcoder register addresses can easily break if we add a new
> > > transcoder. So remove the dependency by using named initializers.
> > >
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_pci.c | 52 ++++++++++++++++++++++++++++++-----------
> > > 1 file changed, 38 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > > index 983ae7fd8217..1b81d7cb209e 100644
> > > --- a/drivers/gpu/drm/i915/i915_pci.c
> > > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > > @@ -33,16 +33,30 @@
> > > #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
> > >
> > > #define GEN_DEFAULT_PIPEOFFSETS \
> > > - .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> > > - PIPE_C_OFFSET, PIPE_EDP_OFFSET }, \
> > > - .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
> > > - TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET }
> > > + .pipe_offsets = { \
> > > + [TRANSCODER_A] = PIPE_A_OFFSET, \
> > > + [TRANSCODER_B] = PIPE_B_OFFSET, \
> > > + [TRANSCODER_C] = PIPE_C_OFFSET, \
> > > + [TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
> >
> > Hmm. We do define this as int pipe_offsets[I915_MAX_TRANSCODERS], and
> > PIPECONF/TRANSCONF is per-transcoder so I suppose using TRANSCODER_foo
> > here make sense.
> >
> > Couple of things that came to mind while thinking about this:
> > - pipe_offsets[] & co. could probably be u32 since we don't
> > need negative values
>
> Ok, can follow up with that.
>
> > - _PIPE_EDP could be removed, which would maybe shrink a few
> > arrays based on I915_MAX_PIPES
>
> Agreed. Looks like all the users are in gvt:
>
> - PIPECONF(_PIPE_EDP) should use PIPECONF(TRANSCODER_EDP) instead. The
> current code would also break if we added a new transcoder.
yeah, agreed.
> - AFAICS
> PIPEDSL(_PIPE_EDP)
> PIPESTAT(_PIPE_EDP)
> PIPE_FRMCOUNT_G4X(_PIPE_EDP)
> PIPE_FLIPCOUNT_G4X(_PIPE_EDP)
> are bogus, since these registers don't exist.
>
Thanks for pointing this out, looks they were added in very beginning,
will remove those after double check.
thanks
> Adding gvt folks for these.
>
> >
> > Patch is
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > > + }, \
> > > + .trans_offsets = { \
> > > + [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> > > + [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> > > + [TRANSCODER_C] = TRANSCODER_C_OFFSET, \
> > > + [TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
> > > + }
> > >
> > > #define GEN_CHV_PIPEOFFSETS \
> > > - .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> > > - CHV_PIPE_C_OFFSET }, \
> > > - .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
> > > - CHV_TRANSCODER_C_OFFSET }
> > > + .pipe_offsets = { \
> > > + [TRANSCODER_A] = PIPE_A_OFFSET, \
> > > + [TRANSCODER_B] = PIPE_B_OFFSET, \
> > > + [TRANSCODER_C] = CHV_PIPE_C_OFFSET, \
> > > + }, \
> > > + .trans_offsets = { \
> > > + [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> > > + [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> > > + [TRANSCODER_C] = CHV_TRANSCODER_C_OFFSET, \
> > > + }
> > >
> > > #define CURSOR_OFFSETS \
> > > .cursor_offsets = { CURSOR_A_OFFSET, CURSOR_B_OFFSET, CHV_CURSOR_C_OFFSET }
> > > @@ -592,12 +606,22 @@ static const struct intel_device_info intel_cannonlake_info = {
> > >
> > > #define GEN11_FEATURES \
> > > GEN10_FEATURES, \
> > > - .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> > > - PIPE_C_OFFSET, PIPE_EDP_OFFSET, \
> > > - PIPE_DSI0_OFFSET, PIPE_DSI1_OFFSET }, \
> > > - .trans_offsets = { TRANSCODER_A_OFFSET, TRANSCODER_B_OFFSET, \
> > > - TRANSCODER_C_OFFSET, TRANSCODER_EDP_OFFSET, \
> > > - TRANSCODER_DSI0_OFFSET, TRANSCODER_DSI1_OFFSET}, \
> > > + .pipe_offsets = { \
> > > + [TRANSCODER_A] = PIPE_A_OFFSET, \
> > > + [TRANSCODER_B] = PIPE_B_OFFSET, \
> > > + [TRANSCODER_C] = PIPE_C_OFFSET, \
> > > + [TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
> > > + [TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET, \
> > > + [TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET, \
> > > + }, \
> > > + .trans_offsets = { \
> > > + [TRANSCODER_A] = TRANSCODER_A_OFFSET, \
> > > + [TRANSCODER_B] = TRANSCODER_B_OFFSET, \
> > > + [TRANSCODER_C] = TRANSCODER_C_OFFSET, \
> > > + [TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
> > > + [TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
> > > + [TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
> > > + }, \
> > > GEN(11), \
> > > .ddb_size = 2048, \
> > > .has_logical_ring_elsq = 1
> > > --
> > > 2.13.2
> >
> > --
> > Ville Syrjälä
> > Intel
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 2/3] drm/i915: Make EDP PSR flags not depend on enum values
2018-11-20 9:23 [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Imre Deak
@ 2018-11-20 9:23 ` Imre Deak
0 siblings, 0 replies; 20+ messages in thread
From: Imre Deak @ 2018-11-20 9:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Lucas De Marchi
Depending on the transcoder enum values to translate from transcoder
to EDP PSR flags can easily break if we add a new transcoder. So remove
the dependency by using an explicit mapping.
While at it also add a WARN for unexpected trancoders.
v2:
- Simplify things by defining flag shift values instead of indices.
- s/trans/cpu_transcoder/ (Ville)
v3:
- Define flags to look like seperate bits instead of the values of
the same bitfield. (Ville)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 10 +++++---
drivers/gpu/drm/i915/intel_psr.c | 55 +++++++++++++++++++++++++++-------------
2 files changed, 44 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index edb58af1e903..e6b371e986ee 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4150,9 +4150,13 @@ enum {
/* Bspec claims those aren't shifted but stay at 0x64800 */
#define EDP_PSR_IMR _MMIO(0x64834)
#define EDP_PSR_IIR _MMIO(0x64838)
-#define EDP_PSR_ERROR(trans) (1 << (((trans) * 8 + 10) & 31))
-#define EDP_PSR_POST_EXIT(trans) (1 << (((trans) * 8 + 9) & 31))
-#define EDP_PSR_PRE_ENTRY(trans) (1 << (((trans) * 8 + 8) & 31))
+#define EDP_PSR_ERROR(shift) (1 << ((shift) + 2))
+#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1))
+#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
+#define EDP_PSR_TRANSCODER_C_SHIFT 24
+#define EDP_PSR_TRANSCODER_B_SHIFT 16
+#define EDP_PSR_TRANSCODER_A_SHIFT 8
+#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
#define EDP_PSR_AUX_CTL _MMIO(dev_priv->psr_mmio_base + 0x10)
#define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 48df16a02fac..26292961d693 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -83,25 +83,42 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
}
}
+static int edp_psr_shift(enum transcoder cpu_transcoder)
+{
+ switch (cpu_transcoder) {
+ case TRANSCODER_A:
+ return EDP_PSR_TRANSCODER_A_SHIFT;
+ case TRANSCODER_B:
+ return EDP_PSR_TRANSCODER_B_SHIFT;
+ case TRANSCODER_C:
+ return EDP_PSR_TRANSCODER_C_SHIFT;
+ default:
+ MISSING_CASE(cpu_transcoder);
+ /* fallthrough */
+ case TRANSCODER_EDP:
+ return EDP_PSR_TRANSCODER_EDP_SHIFT;
+ }
+}
+
void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
{
u32 debug_mask, mask;
+ enum transcoder cpu_transcoder;
+ u32 transcoders = BIT(TRANSCODER_EDP);
- mask = EDP_PSR_ERROR(TRANSCODER_EDP);
- debug_mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
-
- if (INTEL_GEN(dev_priv) >= 8) {
- mask |= EDP_PSR_ERROR(TRANSCODER_A) |
- EDP_PSR_ERROR(TRANSCODER_B) |
- EDP_PSR_ERROR(TRANSCODER_C);
-
- debug_mask |= EDP_PSR_POST_EXIT(TRANSCODER_A) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
- EDP_PSR_POST_EXIT(TRANSCODER_B) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
- EDP_PSR_POST_EXIT(TRANSCODER_C) |
- EDP_PSR_PRE_ENTRY(TRANSCODER_C);
+ if (INTEL_GEN(dev_priv) >= 8)
+ transcoders |= BIT(TRANSCODER_A) |
+ BIT(TRANSCODER_B) |
+ BIT(TRANSCODER_C);
+
+ debug_mask = 0;
+ mask = 0;
+ for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
+ int shift = edp_psr_shift(cpu_transcoder);
+
+ mask |= EDP_PSR_ERROR(shift);
+ debug_mask |= EDP_PSR_POST_EXIT(shift) |
+ EDP_PSR_PRE_ENTRY(shift);
}
if (debug & I915_PSR_DEBUG_IRQ)
@@ -159,18 +176,20 @@ 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) {
+ int shift = edp_psr_shift(cpu_transcoder);
+
/* FIXME: Exit PSR and link train manually when this happens. */
- if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
+ if (psr_iir & EDP_PSR_ERROR(shift))
DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
transcoder_name(cpu_transcoder));
- if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
+ if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
dev_priv->psr.last_entry_attempt = time_ns;
DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
transcoder_name(cpu_transcoder));
}
- if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
+ if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
dev_priv->psr.last_exit = time_ns;
DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
transcoder_name(cpu_transcoder));
--
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-11-20 9:23 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-19 14:41 [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Imre Deak
2018-11-19 14:41 ` [PATCH 2/3] drm/i915: Make EDP PSR flags " Imre Deak
2018-11-19 20:46 ` [PATCH v2 " Imre Deak
2018-11-19 20:58 ` Ville Syrjälä
2018-11-19 21:56 ` [PATCH v3 " Imre Deak
2018-11-19 21:56 ` [PATCH v2 3/3] drm/i915: Add code comment on assumption of pipe==transcoder Imre Deak
2018-11-19 14:41 ` [PATCH " Imre Deak
2018-11-19 15:51 ` Ville Syrjälä
2018-11-19 18:43 ` Imre Deak
2018-11-19 18:55 ` Ville Syrjälä
2018-11-19 22:06 ` Lucas De Marchi
2018-11-19 22:34 ` Imre Deak
2018-11-19 15:11 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Patchwork
2018-11-19 15:29 ` [PATCH 1/3] " Ville Syrjälä
2018-11-19 18:54 ` Imre Deak
2018-11-20 2:38 ` Zhenyu Wang
2018-11-19 19:33 ` ✓ Fi.CI.IGT: success for series starting with [1/3] " Patchwork
2018-11-19 21:09 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values (rev2) Patchwork
2018-11-19 22:32 ` ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values (rev4) Patchwork
-- strict thread matches above, loose matches on Subject: below --
2018-11-20 9:23 [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Imre Deak
2018-11-20 9:23 ` [PATCH v3 2/3] drm/i915: Make EDP PSR flags " Imre Deak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox