* [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 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values
@ 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
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>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.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
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
2018-11-20 9:23 [PATCH 1/3] drm/i915: Make pipe/transcoder offsets not depend on enum values Imre Deak
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.