* [PATCH v3 1/6] drm/i915/psr: Remove PSR2 FIXME
@ 2019-02-28 1:32 José Roberto de Souza
2019-02-28 1:32 ` [PATCH v3 2/6] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset José Roberto de Souza
` (6 more replies)
0 siblings, 7 replies; 27+ messages in thread
From: José Roberto de Souza @ 2019-02-28 1:32 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan
Now we are checking sink capabilities when probing PSR DPCD register
and then dynamically checking in if new state is compatible with PSR
in, so this FIXME can be dropped.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_psr.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 75c1a5deebf5..8bed73914876 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -532,11 +532,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
int crtc_vdisplay = crtc_state->base.adjusted_mode.crtc_vdisplay;
int psr_max_h = 0, psr_max_v = 0;
- /*
- * FIXME psr2_support is messed up. It's both computed
- * dynamically during PSR enable, and extracted from sink
- * caps during eDP detection.
- */
if (!dev_priv->psr.sink_psr2_support)
return false;
--
2.21.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 2/6] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
2019-02-28 1:32 [PATCH v3 1/6] drm/i915/psr: Remove PSR2 FIXME José Roberto de Souza
@ 2019-02-28 1:32 ` José Roberto de Souza
2019-02-28 16:39 ` Ville Syrjälä
2019-02-28 1:32 ` [PATCH v3 3/6] drm/i915: Compute and commit color features in fastsets José Roberto de Souza
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: José Roberto de Souza @ 2019-02-28 1:32 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan
Forcing a specific CRTC to the eDP connector was causing the
intel_psr_fastset_force() to mark mode_chaged in the wrong and
disabled CRTC causing no update in the PSR state.
Looks like our internal state track do not clear output_types and
has_psr in the disabled CRTCs, not sure if this is the expected
behavior or not but in the mean time this fix the issue.
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_psr.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 8bed73914876..6175b1d2e0c8 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -981,7 +981,8 @@ static int intel_psr_fastset_force(struct drm_i915_private *dev_priv)
intel_crtc_state = to_intel_crtc_state(crtc_state);
- if (intel_crtc_has_type(intel_crtc_state, INTEL_OUTPUT_EDP) &&
+ if (crtc_state->active &&
+ intel_crtc_has_type(intel_crtc_state, INTEL_OUTPUT_EDP) &&
intel_crtc_state->has_psr) {
/* Mark mode as changed to trigger a pipe->update() */
crtc_state->mode_changed = true;
--
2.21.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 3/6] drm/i915: Compute and commit color features in fastsets
2019-02-28 1:32 [PATCH v3 1/6] drm/i915/psr: Remove PSR2 FIXME José Roberto de Souza
2019-02-28 1:32 ` [PATCH v3 2/6] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset José Roberto de Souza
@ 2019-02-28 1:32 ` José Roberto de Souza
2019-02-28 16:41 ` Ville Syrjälä
2019-02-28 1:32 ` [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic José Roberto de Souza
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: José Roberto de Souza @ 2019-02-28 1:32 UTC (permalink / raw)
To: intel-gfx
In any commit, intel_modeset_pipe_config() will initialilly clear
and then recalculate most of the pipe states but it leave intel
specific color features states in reset state.
If after intel_pipe_config_compare() is detected that a fastset is
possible it will mark update_pipe as true and unsed mode_changed,
causing the color features state to be kept in reset state and then
latter being committed to hardware disabling the color features.
This issue can be reproduced by any code patch that duplicates the
actual(with color features already enabled) state and only mark
mode_changed as true.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7c5e84ef5171..816e8f124b3b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11232,7 +11232,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
return ret;
}
- if (mode_changed || crtc_state->color_mgmt_changed) {
+ if (mode_changed || pipe_config->update_pipe ||
+ crtc_state->color_mgmt_changed) {
ret = intel_color_check(pipe_config);
if (ret)
return ret;
--
2.21.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic
2019-02-28 1:32 [PATCH v3 1/6] drm/i915/psr: Remove PSR2 FIXME José Roberto de Souza
2019-02-28 1:32 ` [PATCH v3 2/6] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset José Roberto de Souza
2019-02-28 1:32 ` [PATCH v3 3/6] drm/i915: Compute and commit color features in fastsets José Roberto de Souza
@ 2019-02-28 1:32 ` José Roberto de Souza
2019-02-28 16:56 ` Ville Syrjälä
2019-02-28 1:32 ` [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC José Roberto de Souza
` (3 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: José Roberto de Souza @ 2019-02-28 1:32 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan
Other features like PSR2 also needs to be disabled while getting CRC
so lets rename ips_force_disable to crc_enabled, drop all this checks
for pipe A and HSW and BDW and make it generic and
hsw_compute_ips_config() will take care of all the checks removed
from here.
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 10 +++++--
drivers/gpu/drm/i915/intel_drv.h | 3 +-
drivers/gpu/drm/i915/intel_pipe_crc.c | 42 +++++++++------------------
3 files changed, 24 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 816e8f124b3b..328967c642b3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *crtc_state)
if (!hsw_crtc_state_ips_capable(crtc_state))
return false;
- if (crtc_state->ips_force_disable)
+ /*
+ * When IPS gets enabled, the pipe CRC changes. Since IPS gets
+ * enabled and disabled dynamically based on package C states,
+ * user space can't make reliable use of the CRCs, so let's just
+ * completely disable it.
+ */
+ if (crtc_state->crc_enabled)
return false;
/* IPS should be fine as long as at least one plane is enabled. */
@@ -11684,7 +11690,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
saved_state->shared_dpll = crtc_state->shared_dpll;
saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
saved_state->pch_pfit.force_thru = crtc_state->pch_pfit.force_thru;
- saved_state->ips_force_disable = crtc_state->ips_force_disable;
+ saved_state->crc_enabled = crtc_state->crc_enabled;
if (IS_G4X(dev_priv) ||
IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
saved_state->wm = crtc_state->wm;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5412373e2f98..2be64529e4a2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -999,7 +999,8 @@ struct intel_crtc_state {
struct intel_link_m_n fdi_m_n;
bool ips_enabled;
- bool ips_force_disable;
+
+ bool crc_enabled;
bool enable_fbc;
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 53d4ec68d3c4..f6d0b2aaffe2 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -280,11 +280,12 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
return 0;
}
-static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
- bool enable)
+static void
+intel_crtc_crc_prepare(struct drm_i915_private *dev_priv, struct drm_crtc *crtc,
+ bool enable)
{
struct drm_device *dev = &dev_priv->drm;
- struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct intel_crtc_state *pipe_config;
struct drm_atomic_state *state;
struct drm_modeset_acquire_ctx ctx;
@@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
state->acquire_ctx = &ctx;
retry:
- pipe_config = intel_atomic_get_crtc_state(state, crtc);
+ pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
if (IS_ERR(pipe_config)) {
ret = PTR_ERR(pipe_config);
goto put_state;
}
- if (HAS_IPS(dev_priv)) {
- /*
- * When IPS gets enabled, the pipe CRC changes. Since IPS gets
- * enabled and disabled dynamically based on package C states,
- * user space can't make reliable use of the CRCs, so let's just
- * completely disable it.
- */
- pipe_config->ips_force_disable = enable;
- }
+ pipe_config->crc_enabled = enable;
- if (IS_HASWELL(dev_priv)) {
+ if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
pipe_config->pch_pfit.force_thru = enable;
if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
pipe_config->pch_pfit.enabled != enable)
@@ -343,8 +336,7 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
enum pipe pipe,
enum intel_pipe_crc_source *source,
- u32 *val,
- bool set_wa)
+ u32 *val)
{
if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
*source = INTEL_PIPE_CRC_SOURCE_PIPE;
@@ -357,10 +349,6 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
break;
case INTEL_PIPE_CRC_SOURCE_PIPE:
- if (set_wa && (IS_HASWELL(dev_priv) ||
- IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
- hsw_pipe_A_crc_wa(dev_priv, true);
-
*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
break;
case INTEL_PIPE_CRC_SOURCE_NONE:
@@ -418,8 +406,7 @@ static int skl_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
enum pipe pipe,
- enum intel_pipe_crc_source *source, u32 *val,
- bool set_wa)
+ enum intel_pipe_crc_source *source, u32 *val)
{
if (IS_GEN(dev_priv, 2))
return i8xx_pipe_crc_ctl_reg(source, val);
@@ -430,7 +417,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
else if (IS_GEN_RANGE(dev_priv, 5, 6))
return ilk_pipe_crc_ctl_reg(source, val);
else if (INTEL_GEN(dev_priv) < 9)
- return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
+ return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
else
return skl_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
}
@@ -618,7 +605,9 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
return -EIO;
}
- ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
+ intel_crtc_crc_prepare(dev_priv, crtc, source != INTEL_PIPE_CRC_SOURCE_NONE);
+
+ ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
if (ret != 0)
goto out;
@@ -629,9 +618,6 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
if (!source) {
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
vlv_undo_pipe_scramble_reset(dev_priv, crtc->index);
- else if ((IS_HASWELL(dev_priv) ||
- IS_BROADWELL(dev_priv)) && crtc->index == PIPE_A)
- hsw_pipe_A_crc_wa(dev_priv, false);
}
pipe_crc->skipped = 0;
@@ -652,7 +638,7 @@ void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
if (!crtc->crc.opened)
return;
- if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0)
+ if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val) < 0)
return;
/* Don't need pipe_crc->lock here, IRQs are not generated. */
--
2.21.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC
2019-02-28 1:32 [PATCH v3 1/6] drm/i915/psr: Remove PSR2 FIXME José Roberto de Souza
` (2 preceding siblings ...)
2019-02-28 1:32 ` [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic José Roberto de Souza
@ 2019-02-28 1:32 ` José Roberto de Souza
2019-02-28 16:58 ` Ville Syrjälä
` (2 more replies)
2019-02-28 1:32 ` [PATCH v3 6/6] drm/i915: Enable PSR2 by default José Roberto de Souza
` (2 subsequent siblings)
6 siblings, 3 replies; 27+ messages in thread
From: José Roberto de Souza @ 2019-02-28 1:32 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan
When PSR2 is active aka after the number of frames programmed in
PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC
interruptions causing IGT tests to fail due timeout.
Oddly that don't happen when PSR1 active, so here it switches from
PSR2 to PSR1 while user is requesting pipe CRC.
Force setting mode_changed as true is necessary to atomic checks
functions compute new PSR state, that is why it was added to
intel_crtc_crc_prepare().
v3: Reusing intel_crtc_crc_prepare() and crc_enabled
v2: Changed commit description to describe that PSR2 inhibit CRC
calculations.
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
drivers/gpu/drm/i915/intel_psr.c | 3 +++
2 files changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index f6d0b2aaffe2..e7ac24c33650 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -308,6 +308,7 @@ intel_crtc_crc_prepare(struct drm_i915_private *dev_priv, struct drm_crtc *crtc,
goto put_state;
}
+ pipe_config->base.mode_changed = pipe_config->crc_enabled != enable;
pipe_config->crc_enabled = enable;
if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 6175b1d2e0c8..f7730b8b2ec0 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
return false;
}
+ if (crtc_state->crc_enabled)
+ return false;
+
return true;
}
--
2.21.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 6/6] drm/i915: Enable PSR2 by default
2019-02-28 1:32 [PATCH v3 1/6] drm/i915/psr: Remove PSR2 FIXME José Roberto de Souza
` (3 preceding siblings ...)
2019-02-28 1:32 ` [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC José Roberto de Souza
@ 2019-02-28 1:32 ` José Roberto de Souza
2019-02-28 2:35 ` ✓ Fi.CI.BAT: success for series starting with [v3,1/6] drm/i915/psr: Remove PSR2 FIXME Patchwork
2019-02-28 5:34 ` ✗ Fi.CI.IGT: failure " Patchwork
6 siblings, 0 replies; 27+ messages in thread
From: José Roberto de Souza @ 2019-02-28 1:32 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan
The support for PSR2 was polished, IGT tests for PSR2 was added and
it was tested performing regular user workloads like browsing,
editing documents and compiling Linux, so it is time to enable it by
default and enjoy even more power-savings.
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_psr.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index f7730b8b2ec0..f168f92912eb 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -80,9 +80,6 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
case I915_PSR_DEBUG_DISABLE:
case I915_PSR_DEBUG_FORCE_PSR1:
return false;
- case I915_PSR_DEBUG_DEFAULT:
- if (i915_modparams.enable_psr <= 0)
- return false;
default:
return crtc_state->has_psr2;
}
--
2.21.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v3,1/6] drm/i915/psr: Remove PSR2 FIXME
2019-02-28 1:32 [PATCH v3 1/6] drm/i915/psr: Remove PSR2 FIXME José Roberto de Souza
` (4 preceding siblings ...)
2019-02-28 1:32 ` [PATCH v3 6/6] drm/i915: Enable PSR2 by default José Roberto de Souza
@ 2019-02-28 2:35 ` Patchwork
2019-02-28 5:34 ` ✗ Fi.CI.IGT: failure " Patchwork
6 siblings, 0 replies; 27+ messages in thread
From: Patchwork @ 2019-02-28 2:35 UTC (permalink / raw)
To: intel-gfx
== Series Details ==
Series: series starting with [v3,1/6] drm/i915/psr: Remove PSR2 FIXME
URL : https://patchwork.freedesktop.org/series/57324/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5668 -> Patchwork_12324
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/57324/revisions/1/
Known issues
------------
Here are the changes found in Patchwork_12324 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_basic@basic-bsd2:
- fi-kbl-7500u: NOTRUN -> SKIP [fdo#109271] +9
* igt@gem_exec_suspend@basic-s3:
- fi-hsw-peppy: PASS -> DMESG-WARN [fdo#102614] +1
* igt@gem_exec_suspend@basic-s4-devices:
- fi-kbl-7560u: PASS -> INCOMPLETE [fdo#107139]
* igt@i915_selftest@live_execlists:
- fi-apl-guc: NOTRUN -> INCOMPLETE [fdo#103927] / [fdo#109720]
* igt@kms_chamelium@dp-crc-fast:
- fi-kbl-7500u: NOTRUN -> DMESG-WARN [fdo#103841]
* igt@kms_psr@cursor_plane_move:
- fi-whl-u: PASS -> FAIL [fdo#107383] +3
* igt@kms_psr@primary_page_flip:
- fi-apl-guc: NOTRUN -> SKIP [fdo#109271] +40
* igt@prime_vgem@basic-fence-flip:
- fi-ilk-650: PASS -> FAIL [fdo#104008]
* igt@runner@aborted:
- fi-kbl-7500u: NOTRUN -> FAIL [fdo#103841]
#### Possible fixes ####
* igt@gem_exec_suspend@basic-s3:
- fi-apl-guc: DMESG-WARN -> PASS
[fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
[fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#104008]: https://bugs.freedesktop.org/show_bug.cgi?id=104008
[fdo#107139]: https://bugs.freedesktop.org/show_bug.cgi?id=107139
[fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
Participating hosts (42 -> 35)
------------------------------
Additional (1): fi-kbl-7500u
Missing (8): fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-byt-squawks fi-bsw-cyan fi-kbl-guc fi-icl-u3 fi-icl-y
Build changes
-------------
* Linux: CI_DRM_5668 -> Patchwork_12324
CI_DRM_5668: f31df5b814fb600861b577e2bc2cdb75c8c3e323 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4861: 017d8461ae509b2d8ef5f585de8ad908081d28a0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12324: c77872de172eace9bb835d4a42c3484188affbca @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
c77872de172e drm/i915: Enable PSR2 by default
eb31e5391b9a drm/i915: Disable PSR2 while getting pipe CRC
3ef971bb424a drm/i915/crc: Make IPS workaround generic
8d51e309df10 drm/i915: Compute and commit color features in fastsets
afd41f2adbcf drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
3131977023c5 drm/i915/psr: Remove PSR2 FIXME
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12324/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* ✗ Fi.CI.IGT: failure for series starting with [v3,1/6] drm/i915/psr: Remove PSR2 FIXME
2019-02-28 1:32 [PATCH v3 1/6] drm/i915/psr: Remove PSR2 FIXME José Roberto de Souza
` (5 preceding siblings ...)
2019-02-28 2:35 ` ✓ Fi.CI.BAT: success for series starting with [v3,1/6] drm/i915/psr: Remove PSR2 FIXME Patchwork
@ 2019-02-28 5:34 ` Patchwork
6 siblings, 0 replies; 27+ messages in thread
From: Patchwork @ 2019-02-28 5:34 UTC (permalink / raw)
To: intel-gfx
== Series Details ==
Series: series starting with [v3,1/6] drm/i915/psr: Remove PSR2 FIXME
URL : https://patchwork.freedesktop.org/series/57324/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_5668_full -> Patchwork_12324_full
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_12324_full absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_12324_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_12324_full:
### IGT changes ###
#### Possible regressions ####
* igt@kms_color@pipe-b-ctm-0-75:
- shard-apl: PASS -> FAIL +9
* igt@kms_cursor_legacy@flip-vs-cursor-busy-crc-legacy:
- shard-apl: PASS -> DMESG-FAIL +17
* igt@kms_flip@2x-flip-vs-suspend-interruptible:
- shard-hsw: PASS -> DMESG-WARN
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-pgflip-blt:
- shard-kbl: PASS -> DMESG-FAIL +12
* igt@kms_pipe_crc_basic@read-crc-pipe-c:
- shard-kbl: PASS -> FAIL +7
#### Warnings ####
* igt@kms_cursor_crc@cursor-64x64-sliding:
- shard-apl: FAIL [fdo#103232] -> DMESG-FAIL
* igt@kms_plane_alpha_blend@pipe-a-alpha-basic:
- shard-apl: FAIL [fdo#108145] -> DMESG-FAIL
- shard-kbl: FAIL [fdo#108145] / [fdo#108590] -> DMESG-FAIL
Known issues
------------
Here are the changes found in Patchwork_12324_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_ctx_isolation@vcs0-clean:
- shard-iclb: NOTRUN -> SKIP [fdo#109281]
* igt@gem_exec_big:
- shard-apl: NOTRUN -> SKIP [fdo#109271] +37
* igt@gem_exec_parallel@bsd1:
- shard-skl: NOTRUN -> SKIP [fdo#109271] +94
* igt@gem_exec_params@no-blt:
- shard-snb: NOTRUN -> SKIP [fdo#109271] +30
* igt@gem_mocs_settings@mocs-rc6-vebox:
- shard-iclb: NOTRUN -> SKIP [fdo#109287]
* igt@gem_mocs_settings@mocs-settings-bsd2:
- shard-iclb: NOTRUN -> SKIP [fdo#109276] / [fdo#109287]
* igt@gem_pwrite@huge-cpu-forwards:
- shard-iclb: NOTRUN -> SKIP [fdo#109290] +1
* igt@gen3_render_mixed_blits:
- shard-iclb: NOTRUN -> SKIP [fdo#109289] +1
* igt@i915_hangman@error-state-capture-bsd2:
- shard-iclb: NOTRUN -> SKIP [fdo#109276] +6
* igt@i915_pm_lpsp@non-edp:
- shard-iclb: NOTRUN -> SKIP [fdo#109301]
* igt@i915_pm_rpm@dpms-lpsp:
- shard-iclb: PASS -> DMESG-WARN [fdo#107724] +4
* igt@i915_pm_rpm@system-suspend-execbuf:
- shard-skl: PASS -> INCOMPLETE [fdo#104108] / [fdo#107807]
* igt@kms_available_modes_crc@available_mode_test_crc:
- shard-skl: NOTRUN -> FAIL [fdo#106641]
* igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
- shard-apl: NOTRUN -> DMESG-WARN [fdo#107956]
* igt@kms_busy@extended-modeset-hang-oldfb-render-f:
- shard-skl: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +6
* igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
- shard-kbl: PASS -> DMESG-WARN [fdo#107956]
* igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-f:
- shard-hsw: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1
* igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
- shard-apl: PASS -> FAIL [fdo#106510]
* igt@kms_chamelium@vga-frame-dump:
- shard-iclb: NOTRUN -> SKIP [fdo#109284] +1
* igt@kms_cursor_crc@cursor-256x256-sliding:
- shard-iclb: NOTRUN -> FAIL [fdo#103232] +1
* igt@kms_cursor_crc@cursor-512x170-random:
- shard-iclb: NOTRUN -> SKIP [fdo#109279] +1
* igt@kms_cursor_crc@cursor-64x64-suspend:
- shard-skl: PASS -> INCOMPLETE [fdo#104108] / [fdo#107773]
* igt@kms_flip@2x-flip-vs-panning-vs-hang-interruptible:
- shard-iclb: NOTRUN -> SKIP [fdo#109274] +1
* igt@kms_flip@flip-vs-expired-vblank:
- shard-skl: PASS -> FAIL [fdo#105363]
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
- shard-iclb: NOTRUN -> FAIL [fdo#103167] +1
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-render:
- shard-kbl: PASS -> DMESG-WARN [fdo#103558] / [fdo#105602] +1
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
- shard-apl: PASS -> FAIL [fdo#103167]
* igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-wc:
- shard-glk: PASS -> FAIL [fdo#103167] +6
* igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
- shard-iclb: PASS -> FAIL [fdo#103167] +20
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-blt:
- shard-hsw: NOTRUN -> SKIP [fdo#109271] +27
* igt@kms_frontbuffer_tracking@psr-2p-scndscrn-spr-indfb-draw-mmap-gtt:
- shard-iclb: NOTRUN -> SKIP [fdo#109280] +10
* igt@kms_plane@pixel-format-pipe-a-planes:
- shard-skl: NOTRUN -> DMESG-WARN [fdo#106885]
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
- shard-skl: PASS -> INCOMPLETE [fdo#104108]
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
- shard-iclb: PASS -> INCOMPLETE [fdo#107713]
* igt@kms_plane@plane-position-covered-pipe-c-planes:
- shard-apl: PASS -> FAIL [fdo#103166] +2
- shard-glk: PASS -> FAIL [fdo#103166]
* igt@kms_plane_alpha_blend@pipe-b-alpha-7efc:
- shard-skl: NOTRUN -> FAIL [fdo#107815] / [fdo#108145]
* igt@kms_plane_alpha_blend@pipe-b-alpha-basic:
- shard-apl: NOTRUN -> FAIL [fdo#108145] +1
* igt@kms_plane_multiple@atomic-pipe-a-tiling-yf:
- shard-iclb: PASS -> FAIL [fdo#103166]
* igt@kms_psr@psr2_cursor_plane_onoff:
- shard-iclb: NOTRUN -> SKIP [fdo#109441]
* igt@kms_rotation_crc@multiplane-rotation:
- shard-kbl: PASS -> FAIL [fdo#109016]
* igt@kms_sysfs_edid_timing:
- shard-iclb: PASS -> FAIL [fdo#100047]
* igt@kms_universal_plane@cursor-fb-leak-pipe-f:
- shard-apl: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1
* igt@kms_universal_plane@disable-primary-vs-flip-pipe-d:
- shard-snb: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +4
* igt@kms_universal_plane@universal-plane-gen9-features-pipe-d:
- shard-iclb: NOTRUN -> SKIP [fdo#109278] +2
* igt@kms_vblank@pipe-b-ts-continuation-suspend:
- shard-hsw: PASS -> FAIL [fdo#104894]
#### Possible fixes ####
* igt@gem_exec_big:
- shard-hsw: TIMEOUT [fdo#107937] -> PASS
* igt@gem_exec_suspend@basic-s3:
- shard-hsw: DMESG-WARN -> PASS
* igt@gem_softpin@noreloc-s3:
- shard-iclb: INCOMPLETE [fdo#107713] -> PASS
* igt@i915_pm_rpm@basic-rte:
- shard-iclb: DMESG-WARN [fdo#107724] -> PASS +2
* igt@i915_pm_rpm@dpms-mode-unset-lpsp:
- shard-skl: INCOMPLETE [fdo#107807] -> PASS
* igt@kms_atomic_transition@plane-all-modeset-transition-fencing:
- shard-apl: INCOMPLETE [fdo#103927] -> PASS
* igt@kms_busy@extended-pageflip-hang-newfb-render-b:
- shard-apl: DMESG-WARN [fdo#107956] -> PASS +1
* igt@kms_cursor_crc@cursor-256x256-suspend:
- shard-apl: FAIL [fdo#103191] / [fdo#103232] -> PASS
* igt@kms_cursor_crc@cursor-64x21-random:
- shard-apl: FAIL [fdo#103232] -> PASS +1
* igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
- shard-glk: FAIL [fdo#105363] -> PASS
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
- shard-apl: FAIL [fdo#103167] -> PASS
* igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-mmap-gtt:
- shard-skl: FAIL [fdo#103167] -> PASS
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-mmap-gtt:
- shard-iclb: FAIL [fdo#103167] -> PASS +2
* igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
- shard-skl: FAIL [fdo#103191] / [fdo#107362] -> PASS
* igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
- shard-skl: FAIL [fdo#108145] -> PASS +1
* igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
- shard-skl: FAIL [fdo#107815] -> PASS
* igt@kms_plane_multiple@atomic-pipe-a-tiling-none:
- shard-iclb: FAIL [fdo#103166] -> PASS
* igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
- shard-apl: FAIL [fdo#103166] -> PASS +2
* igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
- shard-glk: FAIL [fdo#103166] -> PASS
#### Warnings ####
* igt@i915_pm_backlight@fade_with_suspend:
- shard-iclb: FAIL [fdo#107847] -> DMESG-FAIL [fdo#107724] / [fdo#107847]
* igt@i915_pm_rpm@modeset-non-lpsp:
- shard-skl: SKIP [fdo#109271] -> INCOMPLETE [fdo#107807]
* igt@i915_pm_rpm@modeset-non-lpsp-stress-no-wait:
- shard-iclb: INCOMPLETE [fdo#108840] -> SKIP [fdo#109308]
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
- shard-skl: INCOMPLETE [fdo#104108] -> FAIL [fdo#103166]
* igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
- shard-kbl: FAIL [fdo#108145] -> DMESG-FAIL [fdo#103558] / [fdo#105602] / [fdo#108145]
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
[fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
[fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
[fdo#106641]: https://bugs.freedesktop.org/show_bug.cgi?id=106641
[fdo#106885]: https://bugs.freedesktop.org/show_bug.cgi?id=106885
[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
[fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
[fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
[fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
[fdo#107847]: https://bugs.freedesktop.org/show_bug.cgi?id=107847
[fdo#107937]: https://bugs.freedesktop.org/show_bug.cgi?id=107937
[fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
[fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
[fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
[fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
[fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
[fdo#109281]: https://bugs.freedesktop.org/show_bug.cgi?id=109281
[fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
[fdo#109287]: https://bugs.freedesktop.org/show_bug.cgi?id=109287
[fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
[fdo#109290]: https://bugs.freedesktop.org/show_bug.cgi?id=109290
[fdo#109301]: https://bugs.freedesktop.org/show_bug.cgi?id=109301
[fdo#109308]: https://bugs.freedesktop.org/show_bug.cgi?id=109308
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
Participating hosts (7 -> 7)
------------------------------
No changes in participating hosts
Build changes
-------------
* Linux: CI_DRM_5668 -> Patchwork_12324
CI_DRM_5668: f31df5b814fb600861b577e2bc2cdb75c8c3e323 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4861: 017d8461ae509b2d8ef5f585de8ad908081d28a0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12324: c77872de172eace9bb835d4a42c3484188affbca @ 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_12324/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/6] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
2019-02-28 1:32 ` [PATCH v3 2/6] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset José Roberto de Souza
@ 2019-02-28 16:39 ` Ville Syrjälä
2019-02-28 22:39 ` Souza, Jose
0 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2019-02-28 16:39 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx, Dhinakaran Pandiyan
On Wed, Feb 27, 2019 at 05:32:55PM -0800, José Roberto de Souza wrote:
> Forcing a specific CRTC to the eDP connector was causing the
> intel_psr_fastset_force() to mark mode_chaged in the wrong and
> disabled CRTC causing no update in the PSR state.
>
> Looks like our internal state track do not clear output_types and
> has_psr in the disabled CRTCs, not sure if this is the expected
> behavior or not but in the mean time this fix the issue.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 8bed73914876..6175b1d2e0c8 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -981,7 +981,8 @@ static int intel_psr_fastset_force(struct drm_i915_private *dev_priv)
>
> intel_crtc_state = to_intel_crtc_state(crtc_state);
>
> - if (intel_crtc_has_type(intel_crtc_state, INTEL_OUTPUT_EDP) &&
> + if (crtc_state->active &&
> + intel_crtc_has_type(intel_crtc_state, INTEL_OUTPUT_EDP) &&
What's the point of that eDP check anyway?
> intel_crtc_state->has_psr) {
> /* Mark mode as changed to trigger a pipe->update() */
> crtc_state->mode_changed = true;
> --
> 2.21.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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] 27+ messages in thread
* Re: [PATCH v3 3/6] drm/i915: Compute and commit color features in fastsets
2019-02-28 1:32 ` [PATCH v3 3/6] drm/i915: Compute and commit color features in fastsets José Roberto de Souza
@ 2019-02-28 16:41 ` Ville Syrjälä
0 siblings, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2019-02-28 16:41 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
On Wed, Feb 27, 2019 at 05:32:56PM -0800, José Roberto de Souza wrote:
> In any commit, intel_modeset_pipe_config() will initialilly clear
> and then recalculate most of the pipe states but it leave intel
> specific color features states in reset state.
>
> If after intel_pipe_config_compare() is detected that a fastset is
> possible it will mark update_pipe as true and unsed mode_changed,
> causing the color features state to be kept in reset state and then
> latter being committed to hardware disabling the color features.
>
> This issue can be reproduced by any code patch that duplicates the
> actual(with color features already enabled) state and only mark
> mode_changed as true.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7c5e84ef5171..816e8f124b3b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11232,7 +11232,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> return ret;
> }
>
> - if (mode_changed || crtc_state->color_mgmt_changed) {
> + if (mode_changed || pipe_config->update_pipe ||
> + crtc_state->color_mgmt_changed) {
> ret = intel_color_check(pipe_config);
> if (ret)
> return ret;
> --
> 2.21.0
--
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] 27+ messages in thread
* Re: [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic
2019-02-28 1:32 ` [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic José Roberto de Souza
@ 2019-02-28 16:56 ` Ville Syrjälä
2019-02-28 17:04 ` Ville Syrjälä
2019-02-28 23:26 ` Souza, Jose
0 siblings, 2 replies; 27+ messages in thread
From: Ville Syrjälä @ 2019-02-28 16:56 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx, Dhinakaran Pandiyan
On Wed, Feb 27, 2019 at 05:32:57PM -0800, José Roberto de Souza wrote:
> Other features like PSR2 also needs to be disabled while getting CRC
> so lets rename ips_force_disable to crc_enabled, drop all this checks
> for pipe A and HSW and BDW and make it generic and
> hsw_compute_ips_config() will take care of all the checks removed
> from here.
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 10 +++++--
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_pipe_crc.c | 42 +++++++++------------------
> 3 files changed, 24 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 816e8f124b3b..328967c642b3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *crtc_state)
> if (!hsw_crtc_state_ips_capable(crtc_state))
> return false;
>
> - if (crtc_state->ips_force_disable)
> + /*
> + * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> + * enabled and disabled dynamically based on package C states,
> + * user space can't make reliable use of the CRCs, so let's just
> + * completely disable it.
> + */
> + if (crtc_state->crc_enabled)
> return false;
Hmm. I was wondering how we even manage to pass the state checker with
the current code. But apparently we don't have state checking for IPS.
I would suggest moving this into hsw_compute_ips_config() and then
adding the state checker (for HSW only though since BDW can't do the
readout).
>
> /* IPS should be fine as long as at least one plane is enabled. */
> @@ -11684,7 +11690,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> saved_state->shared_dpll = crtc_state->shared_dpll;
> saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> saved_state->pch_pfit.force_thru = crtc_state->pch_pfit.force_thru;
> - saved_state->ips_force_disable = crtc_state->ips_force_disable;
> + saved_state->crc_enabled = crtc_state->crc_enabled;
> if (IS_G4X(dev_priv) ||
> IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> saved_state->wm = crtc_state->wm;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5412373e2f98..2be64529e4a2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -999,7 +999,8 @@ struct intel_crtc_state {
> struct intel_link_m_n fdi_m_n;
>
> bool ips_enabled;
> - bool ips_force_disable;
> +
> + bool crc_enabled;
>
> bool enable_fbc;
>
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 53d4ec68d3c4..f6d0b2aaffe2 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -280,11 +280,12 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
> return 0;
> }
>
> -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> - bool enable)
> +static void
> +intel_crtc_crc_prepare(struct drm_i915_private *dev_priv, struct drm_crtc *crtc,
Just pass in the intel_crtc
> + bool enable)
> {
> struct drm_device *dev = &dev_priv->drm;
> - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
and then we don't have to have an ugly name for this.
Also pasing in dev_priv is redundant when you're already passing in the
crtc.
The function name isn't super descriptive. It makes me think we're
preparing for CRC capture, when in fact it just adds/removes the w/as.
> struct intel_crtc_state *pipe_config;
> struct drm_atomic_state *state;
> struct drm_modeset_acquire_ctx ctx;
> @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> state->acquire_ctx = &ctx;
>
> retry:
> - pipe_config = intel_atomic_get_crtc_state(state, crtc);
> + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> if (IS_ERR(pipe_config)) {
> ret = PTR_ERR(pipe_config);
> goto put_state;
> }
>
> - if (HAS_IPS(dev_priv)) {
> - /*
> - * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> - * enabled and disabled dynamically based on package C states,
> - * user space can't make reliable use of the CRCs, so let's just
> - * completely disable it.
> - */
> - pipe_config->ips_force_disable = enable;
> - }
> + pipe_config->crc_enabled = enable;
>
> - if (IS_HASWELL(dev_priv)) {
> + if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
> pipe_config->pch_pfit.force_thru = enable;
> if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> pipe_config->pch_pfit.enabled != enable)
> @@ -343,8 +336,7 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> enum pipe pipe,
> enum intel_pipe_crc_source *source,
> - u32 *val,
> - bool set_wa)
> + u32 *val)
> {
> if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> *source = INTEL_PIPE_CRC_SOURCE_PIPE;
> @@ -357,10 +349,6 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
> break;
> case INTEL_PIPE_CRC_SOURCE_PIPE:
> - if (set_wa && (IS_HASWELL(dev_priv) ||
> - IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> - hsw_pipe_A_crc_wa(dev_priv, true);
> -
> *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
> break;
> case INTEL_PIPE_CRC_SOURCE_NONE:
> @@ -418,8 +406,7 @@ static int skl_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>
> static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> enum pipe pipe,
> - enum intel_pipe_crc_source *source, u32 *val,
> - bool set_wa)
> + enum intel_pipe_crc_source *source, u32 *val)
> {
> if (IS_GEN(dev_priv, 2))
> return i8xx_pipe_crc_ctl_reg(source, val);
> @@ -430,7 +417,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> else if (IS_GEN_RANGE(dev_priv, 5, 6))
> return ilk_pipe_crc_ctl_reg(source, val);
> else if (INTEL_GEN(dev_priv) < 9)
> - return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
> + return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> else
> return skl_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> }
> @@ -618,7 +605,9 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
> return -EIO;
> }
>
> - ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
> + intel_crtc_crc_prepare(dev_priv, crtc, source != INTEL_PIPE_CRC_SOURCE_NONE);
> +
Aren't we potentially corrupting the last CRC(s) by turning off the w/as
before disbling CRC capture?
> + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
> if (ret != 0)
> goto out;
>
> @@ -629,9 +618,6 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
> if (!source) {
> if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> vlv_undo_pipe_scramble_reset(dev_priv, crtc->index);
> - else if ((IS_HASWELL(dev_priv) ||
> - IS_BROADWELL(dev_priv)) && crtc->index == PIPE_A)
> - hsw_pipe_A_crc_wa(dev_priv, false);
> }
>
> pipe_crc->skipped = 0;
> @@ -652,7 +638,7 @@ void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
> if (!crtc->crc.opened)
> return;
>
> - if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0)
> + if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val) < 0)
> return;
>
> /* Don't need pipe_crc->lock here, IRQs are not generated. */
> --
> 2.21.0
--
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] 27+ messages in thread
* Re: [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC
2019-02-28 1:32 ` [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC José Roberto de Souza
@ 2019-02-28 16:58 ` Ville Syrjälä
2019-02-28 23:07 ` Souza, Jose
2019-03-01 20:12 ` Dhinakaran Pandiyan
2019-03-01 20:45 ` Ville Syrjälä
2 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2019-02-28 16:58 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx, Dhinakaran Pandiyan
On Wed, Feb 27, 2019 at 05:32:58PM -0800, José Roberto de Souza wrote:
> When PSR2 is active aka after the number of frames programmed in
> PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC
> interruptions causing IGT tests to fail due timeout.
>
> Oddly that don't happen when PSR1 active, so here it switches from
> PSR2 to PSR1 while user is requesting pipe CRC.
>
> Force setting mode_changed as true is necessary to atomic checks
> functions compute new PSR state, that is why it was added to
> intel_crtc_crc_prepare().
>
> v3: Reusing intel_crtc_crc_prepare() and crc_enabled
>
> v2: Changed commit description to describe that PSR2 inhibit CRC
> calculations.
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 3 +++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index f6d0b2aaffe2..e7ac24c33650 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -308,6 +308,7 @@ intel_crtc_crc_prepare(struct drm_i915_private *dev_priv, struct drm_crtc *crtc,
> goto put_state;
> }
>
> + pipe_config->base.mode_changed = pipe_config->crc_enabled != enable;
Do we really want to set that unconditionally?
> pipe_config->crc_enabled = enable;
>
> if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 6175b1d2e0c8..f7730b8b2ec0 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> return false;
> }
>
> + if (crtc_state->crc_enabled)
> + return false;
> +
> return true;
> }
>
> --
> 2.21.0
--
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] 27+ messages in thread
* Re: [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic
2019-02-28 16:56 ` Ville Syrjälä
@ 2019-02-28 17:04 ` Ville Syrjälä
2019-02-28 23:26 ` Souza, Jose
1 sibling, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2019-02-28 17:04 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx, Dhinakaran Pandiyan
On Thu, Feb 28, 2019 at 06:56:48PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 27, 2019 at 05:32:57PM -0800, José Roberto de Souza wrote:
> > Other features like PSR2 also needs to be disabled while getting CRC
> > so lets rename ips_force_disable to crc_enabled, drop all this checks
> > for pipe A and HSW and BDW and make it generic and
> > hsw_compute_ips_config() will take care of all the checks removed
> > from here.
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 10 +++++--
> > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > drivers/gpu/drm/i915/intel_pipe_crc.c | 42 +++++++++------------------
> > 3 files changed, 24 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 816e8f124b3b..328967c642b3 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *crtc_state)
> > if (!hsw_crtc_state_ips_capable(crtc_state))
> > return false;
> >
> > - if (crtc_state->ips_force_disable)
> > + /*
> > + * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> > + * enabled and disabled dynamically based on package C states,
> > + * user space can't make reliable use of the CRCs, so let's just
> > + * completely disable it.
> > + */
> > + if (crtc_state->crc_enabled)
> > return false;
>
> Hmm. I was wondering how we even manage to pass the state checker with
> the current code. But apparently we don't have state checking for IPS.
> I would suggest moving this into hsw_compute_ips_config() and then
> adding the state checker (for HSW only though since BDW can't do the
> readout).
And of course this _is_ hsw_compute_ips_config(). Not sure what code
I was reading.
--
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] 27+ messages in thread
* Re: [PATCH v3 2/6] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
2019-02-28 16:39 ` Ville Syrjälä
@ 2019-02-28 22:39 ` Souza, Jose
0 siblings, 0 replies; 27+ messages in thread
From: Souza, Jose @ 2019-02-28 22:39 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx, Pandiyan, Dhinakaran
[-- Attachment #1.1: Type: text/plain, Size: 1952 bytes --]
On Thu, 2019-02-28 at 18:39 +0200, Ville Syrjälä wrote:
> On Wed, Feb 27, 2019 at 05:32:55PM -0800, José Roberto de Souza
> wrote:
> > Forcing a specific CRTC to the eDP connector was causing the
> > intel_psr_fastset_force() to mark mode_chaged in the wrong and
> > disabled CRTC causing no update in the PSR state.
> >
> > Looks like our internal state track do not clear output_types and
> > has_psr in the disabled CRTCs, not sure if this is the expected
> > behavior or not but in the mean time this fix the issue.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 8bed73914876..6175b1d2e0c8 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -981,7 +981,8 @@ static int intel_psr_fastset_force(struct
> > drm_i915_private *dev_priv)
> >
> > intel_crtc_state = to_intel_crtc_state(crtc_state);
> >
> > - if (intel_crtc_has_type(intel_crtc_state,
> > INTEL_OUTPUT_EDP) &&
> > + if (crtc_state->active &&
> > + intel_crtc_has_type(intel_crtc_state,
> > INTEL_OUTPUT_EDP) &&
>
> What's the point of that eDP check anyway?
I gonna drop it in another patch.
>
> > intel_crtc_state->has_psr) {
> > /* Mark mode as changed to trigger a pipe-
> > >update() */
> > crtc_state->mode_changed = true;
> > --
> > 2.21.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC
2019-02-28 16:58 ` Ville Syrjälä
@ 2019-02-28 23:07 ` Souza, Jose
2019-03-01 1:57 ` Dhinakaran Pandiyan
0 siblings, 1 reply; 27+ messages in thread
From: Souza, Jose @ 2019-02-28 23:07 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx, Pandiyan, Dhinakaran
[-- Attachment #1.1: Type: text/plain, Size: 2597 bytes --]
On Thu, 2019-02-28 at 18:58 +0200, Ville Syrjälä wrote:
> On Wed, Feb 27, 2019 at 05:32:58PM -0800, José Roberto de Souza
> wrote:
> > When PSR2 is active aka after the number of frames programmed in
> > PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC
> > interruptions causing IGT tests to fail due timeout.
> >
> > Oddly that don't happen when PSR1 active, so here it switches from
> > PSR2 to PSR1 while user is requesting pipe CRC.
> >
> > Force setting mode_changed as true is necessary to atomic checks
> > functions compute new PSR state, that is why it was added to
> > intel_crtc_crc_prepare().
> >
> > v3: Reusing intel_crtc_crc_prepare() and crc_enabled
> >
> > v2: Changed commit description to describe that PSR2 inhibit CRC
> > calculations.
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
> > drivers/gpu/drm/i915/intel_psr.c | 3 +++
> > 2 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index f6d0b2aaffe2..e7ac24c33650 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -308,6 +308,7 @@ intel_crtc_crc_prepare(struct drm_i915_private
> > *dev_priv, struct drm_crtc *crtc,
> > goto put_state;
> > }
> >
> > + pipe_config->base.mode_changed = pipe_config->crc_enabled !=
> > enable;
>
> Do we really want to set that unconditionally?
Without it atomic helpers will return ealier as there is no state
changes, I was wondering if the IPS was being applied in the bellow
connectors_changed is not set.
Anyways to triggers the code paths to disable PSR2 it is needed, and
with fastboot enable by default in gen9+ it will do a fastset so not
much drawbacks.
>
> > pipe_config->crc_enabled = enable;
> >
> > if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 6175b1d2e0c8..f7730b8b2ec0 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> > return false;
> > }
> >
> > + if (crtc_state->crc_enabled)
> > + return false;
> > +
> > return true;
> > }
> >
> > --
> > 2.21.0
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic
2019-02-28 16:56 ` Ville Syrjälä
2019-02-28 17:04 ` Ville Syrjälä
@ 2019-02-28 23:26 ` Souza, Jose
2019-03-01 1:06 ` Dhinakaran Pandiyan
2019-03-01 13:35 ` Ville Syrjälä
1 sibling, 2 replies; 27+ messages in thread
From: Souza, Jose @ 2019-02-28 23:26 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx, Pandiyan, Dhinakaran
[-- Attachment #1.1: Type: text/plain, Size: 8973 bytes --]
On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote:
> On Wed, Feb 27, 2019 at 05:32:57PM -0800, José Roberto de Souza
> wrote:
> > Other features like PSR2 also needs to be disabled while getting
> > CRC
> > so lets rename ips_force_disable to crc_enabled, drop all this
> > checks
> > for pipe A and HSW and BDW and make it generic and
> > hsw_compute_ips_config() will take care of all the checks removed
> > from here.
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 10 +++++--
> > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > drivers/gpu/drm/i915/intel_pipe_crc.c | 42 +++++++++------------
> > ------
> > 3 files changed, 24 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 816e8f124b3b..328967c642b3 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct
> > intel_crtc_state *crtc_state)
> > if (!hsw_crtc_state_ips_capable(crtc_state))
> > return false;
> >
> > - if (crtc_state->ips_force_disable)
> > + /*
> > + * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> > + * enabled and disabled dynamically based on package C states,
> > + * user space can't make reliable use of the CRCs, so let's
> > just
> > + * completely disable it.
> > + */
> > + if (crtc_state->crc_enabled)
> > return false;
>
> Hmm. I was wondering how we even manage to pass the state checker
> with
> the current code. But apparently we don't have state checking for
> IPS.
> I would suggest moving this into hsw_compute_ips_config() and then
> adding the state checker (for HSW only though since BDW can't do the
> readout).
>
> >
> > /* IPS should be fine as long as at least one plane is enabled.
> > */
> > @@ -11684,7 +11690,7 @@ clear_intel_crtc_state(struct
> > intel_crtc_state *crtc_state)
> > saved_state->shared_dpll = crtc_state->shared_dpll;
> > saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> > saved_state->pch_pfit.force_thru = crtc_state-
> > >pch_pfit.force_thru;
> > - saved_state->ips_force_disable = crtc_state->ips_force_disable;
> > + saved_state->crc_enabled = crtc_state->crc_enabled;
> > if (IS_G4X(dev_priv) ||
> > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > saved_state->wm = crtc_state->wm;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 5412373e2f98..2be64529e4a2 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -999,7 +999,8 @@ struct intel_crtc_state {
> > struct intel_link_m_n fdi_m_n;
> >
> > bool ips_enabled;
> > - bool ips_force_disable;
> > +
> > + bool crc_enabled;
> >
> > bool enable_fbc;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index 53d4ec68d3c4..f6d0b2aaffe2 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -280,11 +280,12 @@ static int ilk_pipe_crc_ctl_reg(enum
> > intel_pipe_crc_source *source,
> > return 0;
> > }
> >
> > -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > - bool enable)
> > +static void
> > +intel_crtc_crc_prepare(struct drm_i915_private *dev_priv, struct
> > drm_crtc *crtc,
>
> Just pass in the intel_crtc
Okay
>
> > + bool enable)
> > {
> > struct drm_device *dev = &dev_priv->drm;
> > - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> > PIPE_A);
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
> and then we don't have to have an ugly name for this.
>
> Also pasing in dev_priv is redundant when you're already passing in
> the
> crtc.
>
okay
> The function name isn't super descriptive. It makes me think we're
> preparing for CRC capture, when in fact it just adds/removes the
> w/as.
What about: intel_crtc_crc_workarounds_setup()?
>
> > struct intel_crtc_state *pipe_config;
> > struct drm_atomic_state *state;
> > struct drm_modeset_acquire_ctx ctx;
> > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct
> > drm_i915_private *dev_priv,
> > state->acquire_ctx = &ctx;
> >
> > retry:
> > - pipe_config = intel_atomic_get_crtc_state(state, crtc);
> > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> > if (IS_ERR(pipe_config)) {
> > ret = PTR_ERR(pipe_config);
> > goto put_state;
> > }
> >
> > - if (HAS_IPS(dev_priv)) {
> > - /*
> > - * When IPS gets enabled, the pipe CRC changes. Since
> > IPS gets
> > - * enabled and disabled dynamically based on package C
> > states,
> > - * user space can't make reliable use of the CRCs, so
> > let's just
> > - * completely disable it.
> > - */
> > - pipe_config->ips_force_disable = enable;
> > - }
> > + pipe_config->crc_enabled = enable;
> >
> > - if (IS_HASWELL(dev_priv)) {
> > + if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
> > pipe_config->pch_pfit.force_thru = enable;
> > if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > pipe_config->pch_pfit.enabled != enable)
> > @@ -343,8 +336,7 @@ static void hsw_pipe_A_crc_wa(struct
> > drm_i915_private *dev_priv,
> > static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > enum pipe pipe,
> > enum intel_pipe_crc_source *source,
> > - u32 *val,
> > - bool set_wa)
> > + u32 *val)
> > {
> > if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> > *source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > @@ -357,10 +349,6 @@ static int ivb_pipe_crc_ctl_reg(struct
> > drm_i915_private *dev_priv,
> > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
> > break;
> > case INTEL_PIPE_CRC_SOURCE_PIPE:
> > - if (set_wa && (IS_HASWELL(dev_priv) ||
> > - IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > - hsw_pipe_A_crc_wa(dev_priv, true);
> > -
> > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
> > break;
> > case INTEL_PIPE_CRC_SOURCE_NONE:
> > @@ -418,8 +406,7 @@ static int skl_pipe_crc_ctl_reg(struct
> > drm_i915_private *dev_priv,
> >
> > static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > enum pipe pipe,
> > - enum intel_pipe_crc_source *source, u32
> > *val,
> > - bool set_wa)
> > + enum intel_pipe_crc_source *source, u32
> > *val)
> > {
> > if (IS_GEN(dev_priv, 2))
> > return i8xx_pipe_crc_ctl_reg(source, val);
> > @@ -430,7 +417,7 @@ static int get_new_crc_ctl_reg(struct
> > drm_i915_private *dev_priv,
> > else if (IS_GEN_RANGE(dev_priv, 5, 6))
> > return ilk_pipe_crc_ctl_reg(source, val);
> > else if (INTEL_GEN(dev_priv) < 9)
> > - return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > val, set_wa);
> > + return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > val);
> > else
> > return skl_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > val);
> > }
> > @@ -618,7 +605,9 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > *crtc, const char *source_name)
> > return -EIO;
> > }
> >
> > - ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val,
> > true);
> > + intel_crtc_crc_prepare(dev_priv, crtc, source !=
> > INTEL_PIPE_CRC_SOURCE_NONE);
> > +
>
> Aren't we potentially corrupting the last CRC(s) by turning off the
> w/as
> before disbling CRC capture?
It will only be NULL when uses closes the file descriptor to CRC data,
so even if its corrupted user will never get those CRCs.
>
> > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source,
> > &val);
> > if (ret != 0)
> > goto out;
> >
> > @@ -629,9 +618,6 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > *crtc, const char *source_name)
> > if (!source) {
> > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > vlv_undo_pipe_scramble_reset(dev_priv, crtc-
> > >index);
> > - else if ((IS_HASWELL(dev_priv) ||
> > - IS_BROADWELL(dev_priv)) && crtc->index ==
> > PIPE_A)
> > - hsw_pipe_A_crc_wa(dev_priv, false);
> > }
> >
> > pipe_crc->skipped = 0;
> > @@ -652,7 +638,7 @@ void intel_crtc_enable_pipe_crc(struct
> > intel_crtc *intel_crtc)
> > if (!crtc->crc.opened)
> > return;
> >
> > - if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc-
> > >source, &val, false) < 0)
> > + if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc-
> > >source, &val) < 0)
> > return;
> >
> > /* Don't need pipe_crc->lock here, IRQs are not generated. */
> > --
> > 2.21.0
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic
2019-02-28 23:26 ` Souza, Jose
@ 2019-03-01 1:06 ` Dhinakaran Pandiyan
2019-03-01 1:14 ` Souza, Jose
2019-03-01 13:35 ` Ville Syrjälä
1 sibling, 1 reply; 27+ messages in thread
From: Dhinakaran Pandiyan @ 2019-03-01 1:06 UTC (permalink / raw)
To: Souza, Jose, ville.syrjala; +Cc: intel-gfx
On Thu, 2019-02-28 at 15:26 -0800, Souza, Jose wrote:
> On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote:
> > On Wed, Feb 27, 2019 at 05:32:57PM -0800, José Roberto de Souza
> > wrote:
> > > Other features like PSR2 also needs to be disabled while getting
> > > CRC
> > > so lets rename ips_force_disable to crc_enabled, drop all this
> > > checks
> > > for pipe A and HSW and BDW and make it generic and
> > > hsw_compute_ips_config() will take care of all the checks removed
> > > from here.
> > >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 10 +++++--
> > > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > > drivers/gpu/drm/i915/intel_pipe_crc.c | 42 +++++++++------------
> > > ------
> > > 3 files changed, 24 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 816e8f124b3b..328967c642b3 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct
> > > intel_crtc_state *crtc_state)
> > > if (!hsw_crtc_state_ips_capable(crtc_state))
> > > return false;
> > >
> > > - if (crtc_state->ips_force_disable)
> > > + /*
> > > + * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> > > + * enabled and disabled dynamically based on package C states,
> > > + * user space can't make reliable use of the CRCs, so let's
> > > just
> > > + * completely disable it.
> > > + */
> > > + if (crtc_state->crc_enabled)
> > > return false;
> >
> > Hmm. I was wondering how we even manage to pass the state checker
> > with
> > the current code. But apparently we don't have state checking for
> > IPS.
> > I would suggest moving this into hsw_compute_ips_config() and then
> > adding the state checker (for HSW only though since BDW can't do
> > the
> > readout).
> >
> > >
> > > /* IPS should be fine as long as at least one plane is enabled.
> > > */
> > > @@ -11684,7 +11690,7 @@ clear_intel_crtc_state(struct
> > > intel_crtc_state *crtc_state)
> > > saved_state->shared_dpll = crtc_state->shared_dpll;
> > > saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> > > saved_state->pch_pfit.force_thru = crtc_state-
> > > > pch_pfit.force_thru;
> > >
> > > - saved_state->ips_force_disable = crtc_state->ips_force_disable;
> > > + saved_state->crc_enabled = crtc_state->crc_enabled;
> > > if (IS_G4X(dev_priv) ||
> > > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > saved_state->wm = crtc_state->wm;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 5412373e2f98..2be64529e4a2 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -999,7 +999,8 @@ struct intel_crtc_state {
> > > struct intel_link_m_n fdi_m_n;
> > >
> > > bool ips_enabled;
> > > - bool ips_force_disable;
> > > +
> > > + bool crc_enabled;
> > >
> > > bool enable_fbc;
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > index 53d4ec68d3c4..f6d0b2aaffe2 100644
> > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > @@ -280,11 +280,12 @@ static int ilk_pipe_crc_ctl_reg(enum
> > > intel_pipe_crc_source *source,
> > > return 0;
> > > }
> > >
> > > -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > > - bool enable)
> > > +static void
> > > +intel_crtc_crc_prepare(struct drm_i915_private *dev_priv, struct
> > > drm_crtc *crtc,
> >
> > Just pass in the intel_crtc
>
> Okay
>
> >
> > > + bool enable)
> > > {
> > > struct drm_device *dev = &dev_priv->drm;
> > > - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> > > PIPE_A);
> > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >
> > and then we don't have to have an ugly name for this.
> >
> > Also pasing in dev_priv is redundant when you're already passing in
> > the
> > crtc.
> >
>
> okay
>
> > The function name isn't super descriptive. It makes me think we're
> > preparing for CRC capture, when in fact it just adds/removes the
> > w/as.
>
> What about: intel_crtc_crc_workarounds_setup()?
>
> >
> > > struct intel_crtc_state *pipe_config;
> > > struct drm_atomic_state *state;
> > > struct drm_modeset_acquire_ctx ctx;
> > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct
> > > drm_i915_private *dev_priv,
> > > state->acquire_ctx = &ctx;
> > >
> > > retry:
> > > - pipe_config = intel_atomic_get_crtc_state(state, crtc);
> > > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> > > if (IS_ERR(pipe_config)) {
> > > ret = PTR_ERR(pipe_config);
> > > goto put_state;
> > > }
> > >
> > > - if (HAS_IPS(dev_priv)) {
> > > - /*
> > > - * When IPS gets enabled, the pipe CRC changes. Since
> > > IPS gets
> > > - * enabled and disabled dynamically based on package C
> > > states,
> > > - * user space can't make reliable use of the CRCs, so
> > > let's just
> > > - * completely disable it.
> > > - */
> > > - pipe_config->ips_force_disable = enable;
> > > - }
> > > + pipe_config->crc_enabled = enable;
> > >
> > > - if (IS_HASWELL(dev_priv)) {
> > > + if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
> > > pipe_config->pch_pfit.force_thru = enable;
> > > if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > > pipe_config->pch_pfit.enabled != enable)
> > > @@ -343,8 +336,7 @@ static void hsw_pipe_A_crc_wa(struct
> > > drm_i915_private *dev_priv,
> > > static int ivb_pipe_crc_ctl_reg(struct drm_i915_private
> > > *dev_priv,
> > > enum pipe pipe,
> > > enum intel_pipe_crc_source *source,
> > > - u32 *val,
> > > - bool set_wa)
> > > + u32 *val)
> > > {
> > > if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> > > *source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > > @@ -357,10 +349,6 @@ static int ivb_pipe_crc_ctl_reg(struct
> > > drm_i915_private *dev_priv,
> > > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
> > > break;
> > > case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > - if (set_wa && (IS_HASWELL(dev_priv) ||
> > > - IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > > - hsw_pipe_A_crc_wa(dev_priv, true);
> > > -
> > > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
> > > break;
> > > case INTEL_PIPE_CRC_SOURCE_NONE:
> > > @@ -418,8 +406,7 @@ static int skl_pipe_crc_ctl_reg(struct
> > > drm_i915_private *dev_priv,
> > >
> > > static int get_new_crc_ctl_reg(struct drm_i915_private
> > > *dev_priv,
> > > enum pipe pipe,
> > > - enum intel_pipe_crc_source *source, u32
> > > *val,
> > > - bool set_wa)
> > > + enum intel_pipe_crc_source *source, u32
> > > *val)
> > > {
> > > if (IS_GEN(dev_priv, 2))
> > > return i8xx_pipe_crc_ctl_reg(source, val);
> > > @@ -430,7 +417,7 @@ static int get_new_crc_ctl_reg(struct
> > > drm_i915_private *dev_priv,
> > > else if (IS_GEN_RANGE(dev_priv, 5, 6))
> > > return ilk_pipe_crc_ctl_reg(source, val);
> > > else if (INTEL_GEN(dev_priv) < 9)
> > > - return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > > val, set_wa);
> > > + return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > > val);
> > > else
> > > return skl_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > > val);
> > > }
> > > @@ -618,7 +605,9 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > > *crtc, const char *source_name)
> > > return -EIO;
> > > }
> > >
> > > - ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val,
> > > true);
> > > + intel_crtc_crc_prepare(dev_priv, crtc, source !=
> > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > +
> >
> > Aren't we potentially corrupting the last CRC(s) by turning off the
> > w/as
> > before disbling CRC capture?
>
> It will only be NULL when uses closes the file descriptor to CRC
> data,
> so even if its corrupted user will never get those CRCs.
Even if the userspace reads from another thread? Probably not a common
use case though.
>
> >
> > > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source,
> > > &val);
> > > if (ret != 0)
> > > goto out;
> > >
> > > @@ -629,9 +618,6 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > > *crtc, const char *source_name)
> > > if (!source) {
> > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > vlv_undo_pipe_scramble_reset(dev_priv, crtc-
> > > > index);
> > >
> > > - else if ((IS_HASWELL(dev_priv) ||
> > > - IS_BROADWELL(dev_priv)) && crtc->index ==
> > > PIPE_A)
> > > - hsw_pipe_A_crc_wa(dev_priv, false);
> > > }
> > >
> > > pipe_crc->skipped = 0;
> > > @@ -652,7 +638,7 @@ void intel_crtc_enable_pipe_crc(struct
> > > intel_crtc *intel_crtc)
> > > if (!crtc->crc.opened)
> > > return;
> > >
> > > - if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc-
> > > > source, &val, false) < 0)
> > >
> > > + if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc-
> > > > source, &val) < 0)
> > >
> > > return;
> > >
> > > /* Don't need pipe_crc->lock here, IRQs are not generated. */
> > > --
> > > 2.21.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic
2019-03-01 1:06 ` Dhinakaran Pandiyan
@ 2019-03-01 1:14 ` Souza, Jose
2019-03-01 20:07 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 27+ messages in thread
From: Souza, Jose @ 2019-03-01 1:14 UTC (permalink / raw)
To: ville.syrjala, Pandiyan, Dhinakaran; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 10922 bytes --]
On Thu, 2019-02-28 at 17:06 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-02-28 at 15:26 -0800, Souza, Jose wrote:
> > On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 27, 2019 at 05:32:57PM -0800, José Roberto de Souza
> > > wrote:
> > > > Other features like PSR2 also needs to be disabled while
> > > > getting
> > > > CRC
> > > > so lets rename ips_force_disable to crc_enabled, drop all this
> > > > checks
> > > > for pipe A and HSW and BDW and make it generic and
> > > > hsw_compute_ips_config() will take care of all the checks
> > > > removed
> > > > from here.
> > > >
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_display.c | 10 +++++--
> > > > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > > > drivers/gpu/drm/i915/intel_pipe_crc.c | 42 +++++++++--------
> > > > ----
> > > > ------
> > > > 3 files changed, 24 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 816e8f124b3b..328967c642b3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -6751,7 +6751,13 @@ static bool
> > > > hsw_compute_ips_config(struct
> > > > intel_crtc_state *crtc_state)
> > > > if (!hsw_crtc_state_ips_capable(crtc_state))
> > > > return false;
> > > >
> > > > - if (crtc_state->ips_force_disable)
> > > > + /*
> > > > + * When IPS gets enabled, the pipe CRC changes. Since
> > > > IPS gets
> > > > + * enabled and disabled dynamically based on package C
> > > > states,
> > > > + * user space can't make reliable use of the CRCs, so
> > > > let's
> > > > just
> > > > + * completely disable it.
> > > > + */
> > > > + if (crtc_state->crc_enabled)
> > > > return false;
> > >
> > > Hmm. I was wondering how we even manage to pass the state checker
> > > with
> > > the current code. But apparently we don't have state checking for
> > > IPS.
> > > I would suggest moving this into hsw_compute_ips_config() and
> > > then
> > > adding the state checker (for HSW only though since BDW can't do
> > > the
> > > readout).
> > >
> > > >
> > > > /* IPS should be fine as long as at least one plane is
> > > > enabled.
> > > > */
> > > > @@ -11684,7 +11690,7 @@ clear_intel_crtc_state(struct
> > > > intel_crtc_state *crtc_state)
> > > > saved_state->shared_dpll = crtc_state->shared_dpll;
> > > > saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> > > > saved_state->pch_pfit.force_thru = crtc_state-
> > > > > pch_pfit.force_thru;
> > > >
> > > > - saved_state->ips_force_disable = crtc_state-
> > > > >ips_force_disable;
> > > > + saved_state->crc_enabled = crtc_state->crc_enabled;
> > > > if (IS_G4X(dev_priv) ||
> > > > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > saved_state->wm = crtc_state->wm;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 5412373e2f98..2be64529e4a2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -999,7 +999,8 @@ struct intel_crtc_state {
> > > > struct intel_link_m_n fdi_m_n;
> > > >
> > > > bool ips_enabled;
> > > > - bool ips_force_disable;
> > > > +
> > > > + bool crc_enabled;
> > > >
> > > > bool enable_fbc;
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > index 53d4ec68d3c4..f6d0b2aaffe2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > @@ -280,11 +280,12 @@ static int ilk_pipe_crc_ctl_reg(enum
> > > > intel_pipe_crc_source *source,
> > > > return 0;
> > > > }
> > > >
> > > > -static void hsw_pipe_A_crc_wa(struct drm_i915_private
> > > > *dev_priv,
> > > > - bool enable)
> > > > +static void
> > > > +intel_crtc_crc_prepare(struct drm_i915_private *dev_priv,
> > > > struct
> > > > drm_crtc *crtc,
> > >
> > > Just pass in the intel_crtc
> >
> > Okay
> >
> > > > + bool enable)
> > > > {
> > > > struct drm_device *dev = &dev_priv->drm;
> > > > - struct intel_crtc *crtc =
> > > > intel_get_crtc_for_pipe(dev_priv,
> > > > PIPE_A);
> > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >
> > > and then we don't have to have an ugly name for this.
> > >
> > > Also pasing in dev_priv is redundant when you're already passing
> > > in
> > > the
> > > crtc.
> > >
> >
> > okay
> >
> > > The function name isn't super descriptive. It makes me think
> > > we're
> > > preparing for CRC capture, when in fact it just adds/removes the
> > > w/as.
> >
> > What about: intel_crtc_crc_workarounds_setup()?
> >
> > > > struct intel_crtc_state *pipe_config;
> > > > struct drm_atomic_state *state;
> > > > struct drm_modeset_acquire_ctx ctx;
> > > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct
> > > > drm_i915_private *dev_priv,
> > > > state->acquire_ctx = &ctx;
> > > >
> > > > retry:
> > > > - pipe_config = intel_atomic_get_crtc_state(state, crtc);
> > > > + pipe_config = intel_atomic_get_crtc_state(state,
> > > > intel_crtc);
> > > > if (IS_ERR(pipe_config)) {
> > > > ret = PTR_ERR(pipe_config);
> > > > goto put_state;
> > > > }
> > > >
> > > > - if (HAS_IPS(dev_priv)) {
> > > > - /*
> > > > - * When IPS gets enabled, the pipe CRC changes.
> > > > Since
> > > > IPS gets
> > > > - * enabled and disabled dynamically based on
> > > > package C
> > > > states,
> > > > - * user space can't make reliable use of the
> > > > CRCs, so
> > > > let's just
> > > > - * completely disable it.
> > > > - */
> > > > - pipe_config->ips_force_disable = enable;
> > > > - }
> > > > + pipe_config->crc_enabled = enable;
> > > >
> > > > - if (IS_HASWELL(dev_priv)) {
> > > > + if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A)
> > > > {
> > > > pipe_config->pch_pfit.force_thru = enable;
> > > > if (pipe_config->cpu_transcoder ==
> > > > TRANSCODER_EDP &&
> > > > pipe_config->pch_pfit.enabled != enable)
> > > > @@ -343,8 +336,7 @@ static void hsw_pipe_A_crc_wa(struct
> > > > drm_i915_private *dev_priv,
> > > > static int ivb_pipe_crc_ctl_reg(struct drm_i915_private
> > > > *dev_priv,
> > > > enum pipe pipe,
> > > > enum intel_pipe_crc_source
> > > > *source,
> > > > - u32 *val,
> > > > - bool set_wa)
> > > > + u32 *val)
> > > > {
> > > > if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> > > > *source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > > > @@ -357,10 +349,6 @@ static int ivb_pipe_crc_ctl_reg(struct
> > > > drm_i915_private *dev_priv,
> > > > *val = PIPE_CRC_ENABLE |
> > > > PIPE_CRC_SOURCE_SPRITE_IVB;
> > > > break;
> > > > case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > > - if (set_wa && (IS_HASWELL(dev_priv) ||
> > > > - IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > > > - hsw_pipe_A_crc_wa(dev_priv, true);
> > > > -
> > > > *val = PIPE_CRC_ENABLE |
> > > > PIPE_CRC_SOURCE_PF_IVB;
> > > > break;
> > > > case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > @@ -418,8 +406,7 @@ static int skl_pipe_crc_ctl_reg(struct
> > > > drm_i915_private *dev_priv,
> > > >
> > > > static int get_new_crc_ctl_reg(struct drm_i915_private
> > > > *dev_priv,
> > > > enum pipe pipe,
> > > > - enum intel_pipe_crc_source
> > > > *source, u32
> > > > *val,
> > > > - bool set_wa)
> > > > + enum intel_pipe_crc_source
> > > > *source, u32
> > > > *val)
> > > > {
> > > > if (IS_GEN(dev_priv, 2))
> > > > return i8xx_pipe_crc_ctl_reg(source, val);
> > > > @@ -430,7 +417,7 @@ static int get_new_crc_ctl_reg(struct
> > > > drm_i915_private *dev_priv,
> > > > else if (IS_GEN_RANGE(dev_priv, 5, 6))
> > > > return ilk_pipe_crc_ctl_reg(source, val);
> > > > else if (INTEL_GEN(dev_priv) < 9)
> > > > - return ivb_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > source,
> > > > val, set_wa);
> > > > + return ivb_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > source,
> > > > val);
> > > > else
> > > > return skl_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > source,
> > > > val);
> > > > }
> > > > @@ -618,7 +605,9 @@ int intel_crtc_set_crc_source(struct
> > > > drm_crtc
> > > > *crtc, const char *source_name)
> > > > return -EIO;
> > > > }
> > > >
> > > > - ret = get_new_crc_ctl_reg(dev_priv, crtc->index,
> > > > &source, &val,
> > > > true);
> > > > + intel_crtc_crc_prepare(dev_priv, crtc, source !=
> > > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > > +
> > >
> > > Aren't we potentially corrupting the last CRC(s) by turning off
> > > the
> > > w/as
> > > before disbling CRC capture?
> >
> > It will only be NULL when uses closes the file descriptor to CRC
> > data,
> > so even if its corrupted user will never get those CRCs.
> Even if the userspace reads from another thread? Probably not a
> common
> use case though.
It is called with NULL from the release() hook, so when all
filedescriptors to this file is closed.
But anyways the drm crc code only allows one open() of this file at
time, so if other thread tries to read it after the main thread have
closed it it will fail before get into any drm code.
>
> > > > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index,
> > > > &source,
> > > > &val);
> > > > if (ret != 0)
> > > > goto out;
> > > >
> > > > @@ -629,9 +618,6 @@ int intel_crtc_set_crc_source(struct
> > > > drm_crtc
> > > > *crtc, const char *source_name)
> > > > if (!source) {
> > > > if (IS_VALLEYVIEW(dev_priv) ||
> > > > IS_CHERRYVIEW(dev_priv))
> > > > vlv_undo_pipe_scramble_reset(dev_priv,
> > > > crtc-
> > > > > index);
> > > >
> > > > - else if ((IS_HASWELL(dev_priv) ||
> > > > - IS_BROADWELL(dev_priv)) && crtc-
> > > > >index ==
> > > > PIPE_A)
> > > > - hsw_pipe_A_crc_wa(dev_priv, false);
> > > > }
> > > >
> > > > pipe_crc->skipped = 0;
> > > > @@ -652,7 +638,7 @@ void intel_crtc_enable_pipe_crc(struct
> > > > intel_crtc *intel_crtc)
> > > > if (!crtc->crc.opened)
> > > > return;
> > > >
> > > > - if (get_new_crc_ctl_reg(dev_priv, crtc->index,
> > > > &pipe_crc-
> > > > > source, &val, false) < 0)
> > > >
> > > > + if (get_new_crc_ctl_reg(dev_priv, crtc->index,
> > > > &pipe_crc-
> > > > > source, &val) < 0)
> > > >
> > > > return;
> > > >
> > > > /* Don't need pipe_crc->lock here, IRQs are not
> > > > generated. */
> > > > --
> > > > 2.21.0
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC
2019-02-28 23:07 ` Souza, Jose
@ 2019-03-01 1:57 ` Dhinakaran Pandiyan
2019-03-01 2:11 ` Souza, Jose
0 siblings, 1 reply; 27+ messages in thread
From: Dhinakaran Pandiyan @ 2019-03-01 1:57 UTC (permalink / raw)
To: Souza, Jose, ville.syrjala; +Cc: intel-gfx
On Thu, 2019-02-28 at 15:07 -0800, Souza, Jose wrote:
> On Thu, 2019-02-28 at 18:58 +0200, Ville Syrjälä wrote:
> > On Wed, Feb 27, 2019 at 05:32:58PM -0800, José Roberto de Souza
> > wrote:
> > > When PSR2 is active aka after the number of frames programmed in
> > > PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC
> > > interruptions causing IGT tests to fail due timeout.
> > >
> > > Oddly that don't happen when PSR1 active, so here it switches
> > > from
> > > PSR2 to PSR1 while user is requesting pipe CRC.
> > >
> > > Force setting mode_changed as true is necessary to atomic checks
> > > functions compute new PSR state, that is why it was added to
> > > intel_crtc_crc_prepare().
> > >
> > > v3: Reusing intel_crtc_crc_prepare() and crc_enabled
> > >
> > > v2: Changed commit description to describe that PSR2 inhibit CRC
> > > calculations.
> > >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
> > > drivers/gpu/drm/i915/intel_psr.c | 3 +++
> > > 2 files changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > index f6d0b2aaffe2..e7ac24c33650 100644
> > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > @@ -308,6 +308,7 @@ intel_crtc_crc_prepare(struct
> > > drm_i915_private
> > > *dev_priv, struct drm_crtc *crtc,
> > > goto put_state;
> > > }
> > >
> > > + pipe_config->base.mode_changed = pipe_config->crc_enabled !=
> > > enable;
> >
> > Do we really want to set that unconditionally?
>
> Without it atomic helpers will return ealier as there is no state
> changes, I was wondering if the IPS was being applied in the bellow
> connectors_changed is not set.
> Anyways to triggers the code paths to disable PSR2 it is needed, and
> with fastboot enable by default in gen9+ it will do a fastset so not
> much drawbacks.
What about pre gen9 platforms that do not have PSR? Running through
state checks and acquiring several locks on platforms that will never
need it is wasteful.
Why not make it conditional upon?
if (HAS_PSR()) or even CAN_PSR() for that matter.
>
> >
> > > pipe_config->crc_enabled = enable;
> > >
> > > if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 6175b1d2e0c8..f7730b8b2ec0 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct
> > > intel_dp *intel_dp,
> > > return false;
> > > }
> > >
> > > + if (crtc_state->crc_enabled)
> > > + return false;
> > > +
> > > return true;
> > > }
> > >
> > > --
> > > 2.21.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC
2019-03-01 1:57 ` Dhinakaran Pandiyan
@ 2019-03-01 2:11 ` Souza, Jose
0 siblings, 0 replies; 27+ messages in thread
From: Souza, Jose @ 2019-03-01 2:11 UTC (permalink / raw)
To: ville.syrjala, Pandiyan, Dhinakaran; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 3363 bytes --]
On Thu, 2019-02-28 at 17:57 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-02-28 at 15:07 -0800, Souza, Jose wrote:
> > On Thu, 2019-02-28 at 18:58 +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 27, 2019 at 05:32:58PM -0800, José Roberto de Souza
> > > wrote:
> > > > When PSR2 is active aka after the number of frames programmed
> > > > in
> > > > PSR2_CTL 'Frames Before SU Entry' hardware stops to generate
> > > > CRC
> > > > interruptions causing IGT tests to fail due timeout.
> > > >
> > > > Oddly that don't happen when PSR1 active, so here it switches
> > > > from
> > > > PSR2 to PSR1 while user is requesting pipe CRC.
> > > >
> > > > Force setting mode_changed as true is necessary to atomic
> > > > checks
> > > > functions compute new PSR state, that is why it was added to
> > > > intel_crtc_crc_prepare().
> > > >
> > > > v3: Reusing intel_crtc_crc_prepare() and crc_enabled
> > > >
> > > > v2: Changed commit description to describe that PSR2 inhibit
> > > > CRC
> > > > calculations.
> > > >
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
> > > > drivers/gpu/drm/i915/intel_psr.c | 3 +++
> > > > 2 files changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > index f6d0b2aaffe2..e7ac24c33650 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > @@ -308,6 +308,7 @@ intel_crtc_crc_prepare(struct
> > > > drm_i915_private
> > > > *dev_priv, struct drm_crtc *crtc,
> > > > goto put_state;
> > > > }
> > > >
> > > > + pipe_config->base.mode_changed = pipe_config-
> > > > >crc_enabled !=
> > > > enable;
> > >
> > > Do we really want to set that unconditionally?
> >
> > Without it atomic helpers will return ealier as there is no state
> > changes, I was wondering if the IPS was being applied in the bellow
> > connectors_changed is not set.
> > Anyways to triggers the code paths to disable PSR2 it is needed,
> > and
> > with fastboot enable by default in gen9+ it will do a fastset so
> > not
> > much drawbacks.
> What about pre gen9 platforms that do not have PSR? Running through
> state checks and acquiring several locks on platforms that will never
> need it is wasteful.
>
> Why not make it conditional upon?
> if (HAS_PSR()) or even CAN_PSR() for that matter.
Okay, done.
>
>
>
> > > > pipe_config->crc_enabled = enable;
> > > >
> > > > if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A)
> > > > {
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 6175b1d2e0c8..f7730b8b2ec0 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct
> > > > intel_dp *intel_dp,
> > > > return false;
> > > > }
> > > >
> > > > + if (crtc_state->crc_enabled)
> > > > + return false;
> > > > +
> > > > return true;
> > > > }
> > > >
> > > > --
> > > > 2.21.0
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic
2019-02-28 23:26 ` Souza, Jose
2019-03-01 1:06 ` Dhinakaran Pandiyan
@ 2019-03-01 13:35 ` Ville Syrjälä
2019-03-01 18:29 ` Souza, Jose
1 sibling, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2019-03-01 13:35 UTC (permalink / raw)
To: Souza, Jose; +Cc: intel-gfx, Pandiyan, Dhinakaran
On Thu, Feb 28, 2019 at 11:26:57PM +0000, Souza, Jose wrote:
> On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote:
> > On Wed, Feb 27, 2019 at 05:32:57PM -0800, José Roberto de Souza
> > wrote:
> > > Other features like PSR2 also needs to be disabled while getting
> > > CRC
> > > so lets rename ips_force_disable to crc_enabled, drop all this
> > > checks
> > > for pipe A and HSW and BDW and make it generic and
> > > hsw_compute_ips_config() will take care of all the checks removed
> > > from here.
> > >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 10 +++++--
> > > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > > drivers/gpu/drm/i915/intel_pipe_crc.c | 42 +++++++++------------
> > > ------
> > > 3 files changed, 24 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 816e8f124b3b..328967c642b3 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct
> > > intel_crtc_state *crtc_state)
> > > if (!hsw_crtc_state_ips_capable(crtc_state))
> > > return false;
> > >
> > > - if (crtc_state->ips_force_disable)
> > > + /*
> > > + * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> > > + * enabled and disabled dynamically based on package C states,
> > > + * user space can't make reliable use of the CRCs, so let's
> > > just
> > > + * completely disable it.
> > > + */
> > > + if (crtc_state->crc_enabled)
> > > return false;
> >
> > Hmm. I was wondering how we even manage to pass the state checker
> > with
> > the current code. But apparently we don't have state checking for
> > IPS.
> > I would suggest moving this into hsw_compute_ips_config() and then
> > adding the state checker (for HSW only though since BDW can't do the
> > readout).
> >
> > >
> > > /* IPS should be fine as long as at least one plane is enabled.
> > > */
> > > @@ -11684,7 +11690,7 @@ clear_intel_crtc_state(struct
> > > intel_crtc_state *crtc_state)
> > > saved_state->shared_dpll = crtc_state->shared_dpll;
> > > saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> > > saved_state->pch_pfit.force_thru = crtc_state-
> > > >pch_pfit.force_thru;
> > > - saved_state->ips_force_disable = crtc_state->ips_force_disable;
> > > + saved_state->crc_enabled = crtc_state->crc_enabled;
> > > if (IS_G4X(dev_priv) ||
> > > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > saved_state->wm = crtc_state->wm;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 5412373e2f98..2be64529e4a2 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -999,7 +999,8 @@ struct intel_crtc_state {
> > > struct intel_link_m_n fdi_m_n;
> > >
> > > bool ips_enabled;
> > > - bool ips_force_disable;
> > > +
> > > + bool crc_enabled;
> > >
> > > bool enable_fbc;
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > index 53d4ec68d3c4..f6d0b2aaffe2 100644
> > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > @@ -280,11 +280,12 @@ static int ilk_pipe_crc_ctl_reg(enum
> > > intel_pipe_crc_source *source,
> > > return 0;
> > > }
> > >
> > > -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > > - bool enable)
> > > +static void
> > > +intel_crtc_crc_prepare(struct drm_i915_private *dev_priv, struct
> > > drm_crtc *crtc,
> >
> > Just pass in the intel_crtc
>
> Okay
>
> >
> > > + bool enable)
> > > {
> > > struct drm_device *dev = &dev_priv->drm;
> > > - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> > > PIPE_A);
> > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >
> > and then we don't have to have an ugly name for this.
> >
> > Also pasing in dev_priv is redundant when you're already passing in
> > the
> > crtc.
> >
>
> okay
>
> > The function name isn't super descriptive. It makes me think we're
> > preparing for CRC capture, when in fact it just adds/removes the
> > w/as.
>
> What about: intel_crtc_crc_workarounds_setup()?
Or _setup_workarounds() so that it reads more naturally?
>
> >
> > > struct intel_crtc_state *pipe_config;
> > > struct drm_atomic_state *state;
> > > struct drm_modeset_acquire_ctx ctx;
> > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct
> > > drm_i915_private *dev_priv,
> > > state->acquire_ctx = &ctx;
> > >
> > > retry:
> > > - pipe_config = intel_atomic_get_crtc_state(state, crtc);
> > > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> > > if (IS_ERR(pipe_config)) {
> > > ret = PTR_ERR(pipe_config);
> > > goto put_state;
> > > }
> > >
> > > - if (HAS_IPS(dev_priv)) {
> > > - /*
> > > - * When IPS gets enabled, the pipe CRC changes. Since
> > > IPS gets
> > > - * enabled and disabled dynamically based on package C
> > > states,
> > > - * user space can't make reliable use of the CRCs, so
> > > let's just
> > > - * completely disable it.
> > > - */
> > > - pipe_config->ips_force_disable = enable;
> > > - }
> > > + pipe_config->crc_enabled = enable;
> > >
> > > - if (IS_HASWELL(dev_priv)) {
> > > + if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
> > > pipe_config->pch_pfit.force_thru = enable;
> > > if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > > pipe_config->pch_pfit.enabled != enable)
> > > @@ -343,8 +336,7 @@ static void hsw_pipe_A_crc_wa(struct
> > > drm_i915_private *dev_priv,
> > > static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > > enum pipe pipe,
> > > enum intel_pipe_crc_source *source,
> > > - u32 *val,
> > > - bool set_wa)
> > > + u32 *val)
> > > {
> > > if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> > > *source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > > @@ -357,10 +349,6 @@ static int ivb_pipe_crc_ctl_reg(struct
> > > drm_i915_private *dev_priv,
> > > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
> > > break;
> > > case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > - if (set_wa && (IS_HASWELL(dev_priv) ||
> > > - IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > > - hsw_pipe_A_crc_wa(dev_priv, true);
> > > -
> > > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
> > > break;
> > > case INTEL_PIPE_CRC_SOURCE_NONE:
> > > @@ -418,8 +406,7 @@ static int skl_pipe_crc_ctl_reg(struct
> > > drm_i915_private *dev_priv,
> > >
> > > static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > > enum pipe pipe,
> > > - enum intel_pipe_crc_source *source, u32
> > > *val,
> > > - bool set_wa)
> > > + enum intel_pipe_crc_source *source, u32
> > > *val)
> > > {
> > > if (IS_GEN(dev_priv, 2))
> > > return i8xx_pipe_crc_ctl_reg(source, val);
> > > @@ -430,7 +417,7 @@ static int get_new_crc_ctl_reg(struct
> > > drm_i915_private *dev_priv,
> > > else if (IS_GEN_RANGE(dev_priv, 5, 6))
> > > return ilk_pipe_crc_ctl_reg(source, val);
> > > else if (INTEL_GEN(dev_priv) < 9)
> > > - return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > > val, set_wa);
> > > + return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > > val);
> > > else
> > > return skl_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > > val);
> > > }
> > > @@ -618,7 +605,9 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > > *crtc, const char *source_name)
> > > return -EIO;
> > > }
> > >
> > > - ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val,
> > > true);
> > > + intel_crtc_crc_prepare(dev_priv, crtc, source !=
> > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > +
> >
> > Aren't we potentially corrupting the last CRC(s) by turning off the
> > w/as
> > before disbling CRC capture?
>
> It will only be NULL when uses closes the file descriptor to CRC data,
> so even if its corrupted user will never get those CRCs.
Userspace may not get it but the tracepoint will still see it. Which may
cause confusion.
>
> >
> > > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source,
> > > &val);
> > > if (ret != 0)
> > > goto out;
> > >
> > > @@ -629,9 +618,6 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > > *crtc, const char *source_name)
> > > if (!source) {
> > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > vlv_undo_pipe_scramble_reset(dev_priv, crtc-
> > > >index);
> > > - else if ((IS_HASWELL(dev_priv) ||
> > > - IS_BROADWELL(dev_priv)) && crtc->index ==
> > > PIPE_A)
> > > - hsw_pipe_A_crc_wa(dev_priv, false);
> > > }
> > >
> > > pipe_crc->skipped = 0;
> > > @@ -652,7 +638,7 @@ void intel_crtc_enable_pipe_crc(struct
> > > intel_crtc *intel_crtc)
> > > if (!crtc->crc.opened)
> > > return;
> > >
> > > - if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc-
> > > >source, &val, false) < 0)
> > > + if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc-
> > > >source, &val) < 0)
> > > return;
> > >
> > > /* Don't need pipe_crc->lock here, IRQs are not generated. */
> > > --
> > > 2.21.0
--
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] 27+ messages in thread
* Re: [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic
2019-03-01 13:35 ` Ville Syrjälä
@ 2019-03-01 18:29 ` Souza, Jose
0 siblings, 0 replies; 27+ messages in thread
From: Souza, Jose @ 2019-03-01 18:29 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx, Pandiyan, Dhinakaran
[-- Attachment #1.1: Type: text/plain, Size: 10887 bytes --]
On Fri, 2019-03-01 at 15:35 +0200, Ville Syrjälä wrote:
> On Thu, Feb 28, 2019 at 11:26:57PM +0000, Souza, Jose wrote:
> > On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 27, 2019 at 05:32:57PM -0800, José Roberto de Souza
> > > wrote:
> > > > Other features like PSR2 also needs to be disabled while
> > > > getting
> > > > CRC
> > > > so lets rename ips_force_disable to crc_enabled, drop all this
> > > > checks
> > > > for pipe A and HSW and BDW and make it generic and
> > > > hsw_compute_ips_config() will take care of all the checks
> > > > removed
> > > > from here.
> > > >
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_display.c | 10 +++++--
> > > > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > > > drivers/gpu/drm/i915/intel_pipe_crc.c | 42 +++++++++--------
> > > > ----
> > > > ------
> > > > 3 files changed, 24 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 816e8f124b3b..328967c642b3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -6751,7 +6751,13 @@ static bool
> > > > hsw_compute_ips_config(struct
> > > > intel_crtc_state *crtc_state)
> > > > if (!hsw_crtc_state_ips_capable(crtc_state))
> > > > return false;
> > > >
> > > > - if (crtc_state->ips_force_disable)
> > > > + /*
> > > > + * When IPS gets enabled, the pipe CRC changes. Since
> > > > IPS gets
> > > > + * enabled and disabled dynamically based on package C
> > > > states,
> > > > + * user space can't make reliable use of the CRCs, so
> > > > let's
> > > > just
> > > > + * completely disable it.
> > > > + */
> > > > + if (crtc_state->crc_enabled)
> > > > return false;
> > >
> > > Hmm. I was wondering how we even manage to pass the state checker
> > > with
> > > the current code. But apparently we don't have state checking for
> > > IPS.
> > > I would suggest moving this into hsw_compute_ips_config() and
> > > then
> > > adding the state checker (for HSW only though since BDW can't do
> > > the
> > > readout).
> > >
> > > >
> > > > /* IPS should be fine as long as at least one plane is
> > > > enabled.
> > > > */
> > > > @@ -11684,7 +11690,7 @@ clear_intel_crtc_state(struct
> > > > intel_crtc_state *crtc_state)
> > > > saved_state->shared_dpll = crtc_state->shared_dpll;
> > > > saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> > > > saved_state->pch_pfit.force_thru = crtc_state-
> > > > > pch_pfit.force_thru;
> > > > - saved_state->ips_force_disable = crtc_state-
> > > > >ips_force_disable;
> > > > + saved_state->crc_enabled = crtc_state->crc_enabled;
> > > > if (IS_G4X(dev_priv) ||
> > > > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > saved_state->wm = crtc_state->wm;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 5412373e2f98..2be64529e4a2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -999,7 +999,8 @@ struct intel_crtc_state {
> > > > struct intel_link_m_n fdi_m_n;
> > > >
> > > > bool ips_enabled;
> > > > - bool ips_force_disable;
> > > > +
> > > > + bool crc_enabled;
> > > >
> > > > bool enable_fbc;
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > index 53d4ec68d3c4..f6d0b2aaffe2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > @@ -280,11 +280,12 @@ static int ilk_pipe_crc_ctl_reg(enum
> > > > intel_pipe_crc_source *source,
> > > > return 0;
> > > > }
> > > >
> > > > -static void hsw_pipe_A_crc_wa(struct drm_i915_private
> > > > *dev_priv,
> > > > - bool enable)
> > > > +static void
> > > > +intel_crtc_crc_prepare(struct drm_i915_private *dev_priv,
> > > > struct
> > > > drm_crtc *crtc,
> > >
> > > Just pass in the intel_crtc
> >
> > Okay
> >
> > > > + bool enable)
> > > > {
> > > > struct drm_device *dev = &dev_priv->drm;
> > > > - struct intel_crtc *crtc =
> > > > intel_get_crtc_for_pipe(dev_priv,
> > > > PIPE_A);
> > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >
> > > and then we don't have to have an ugly name for this.
> > >
> > > Also pasing in dev_priv is redundant when you're already passing
> > > in
> > > the
> > > crtc.
> > >
> >
> > okay
> >
> > > The function name isn't super descriptive. It makes me think
> > > we're
> > > preparing for CRC capture, when in fact it just adds/removes the
> > > w/as.
> >
> > What about: intel_crtc_crc_workarounds_setup()?
>
> Or _setup_workarounds() so that it reads more naturally?
>
> > > > struct intel_crtc_state *pipe_config;
> > > > struct drm_atomic_state *state;
> > > > struct drm_modeset_acquire_ctx ctx;
> > > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct
> > > > drm_i915_private *dev_priv,
> > > > state->acquire_ctx = &ctx;
> > > >
> > > > retry:
> > > > - pipe_config = intel_atomic_get_crtc_state(state, crtc);
> > > > + pipe_config = intel_atomic_get_crtc_state(state,
> > > > intel_crtc);
> > > > if (IS_ERR(pipe_config)) {
> > > > ret = PTR_ERR(pipe_config);
> > > > goto put_state;
> > > > }
> > > >
> > > > - if (HAS_IPS(dev_priv)) {
> > > > - /*
> > > > - * When IPS gets enabled, the pipe CRC changes.
> > > > Since
> > > > IPS gets
> > > > - * enabled and disabled dynamically based on
> > > > package C
> > > > states,
> > > > - * user space can't make reliable use of the
> > > > CRCs, so
> > > > let's just
> > > > - * completely disable it.
> > > > - */
> > > > - pipe_config->ips_force_disable = enable;
> > > > - }
> > > > + pipe_config->crc_enabled = enable;
> > > >
> > > > - if (IS_HASWELL(dev_priv)) {
> > > > + if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A)
> > > > {
> > > > pipe_config->pch_pfit.force_thru = enable;
> > > > if (pipe_config->cpu_transcoder ==
> > > > TRANSCODER_EDP &&
> > > > pipe_config->pch_pfit.enabled != enable)
> > > > @@ -343,8 +336,7 @@ static void hsw_pipe_A_crc_wa(struct
> > > > drm_i915_private *dev_priv,
> > > > static int ivb_pipe_crc_ctl_reg(struct drm_i915_private
> > > > *dev_priv,
> > > > enum pipe pipe,
> > > > enum intel_pipe_crc_source
> > > > *source,
> > > > - u32 *val,
> > > > - bool set_wa)
> > > > + u32 *val)
> > > > {
> > > > if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> > > > *source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > > > @@ -357,10 +349,6 @@ static int ivb_pipe_crc_ctl_reg(struct
> > > > drm_i915_private *dev_priv,
> > > > *val = PIPE_CRC_ENABLE |
> > > > PIPE_CRC_SOURCE_SPRITE_IVB;
> > > > break;
> > > > case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > > - if (set_wa && (IS_HASWELL(dev_priv) ||
> > > > - IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > > > - hsw_pipe_A_crc_wa(dev_priv, true);
> > > > -
> > > > *val = PIPE_CRC_ENABLE |
> > > > PIPE_CRC_SOURCE_PF_IVB;
> > > > break;
> > > > case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > @@ -418,8 +406,7 @@ static int skl_pipe_crc_ctl_reg(struct
> > > > drm_i915_private *dev_priv,
> > > >
> > > > static int get_new_crc_ctl_reg(struct drm_i915_private
> > > > *dev_priv,
> > > > enum pipe pipe,
> > > > - enum intel_pipe_crc_source
> > > > *source, u32
> > > > *val,
> > > > - bool set_wa)
> > > > + enum intel_pipe_crc_source
> > > > *source, u32
> > > > *val)
> > > > {
> > > > if (IS_GEN(dev_priv, 2))
> > > > return i8xx_pipe_crc_ctl_reg(source, val);
> > > > @@ -430,7 +417,7 @@ static int get_new_crc_ctl_reg(struct
> > > > drm_i915_private *dev_priv,
> > > > else if (IS_GEN_RANGE(dev_priv, 5, 6))
> > > > return ilk_pipe_crc_ctl_reg(source, val);
> > > > else if (INTEL_GEN(dev_priv) < 9)
> > > > - return ivb_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > source,
> > > > val, set_wa);
> > > > + return ivb_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > source,
> > > > val);
> > > > else
> > > > return skl_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > source,
> > > > val);
> > > > }
> > > > @@ -618,7 +605,9 @@ int intel_crtc_set_crc_source(struct
> > > > drm_crtc
> > > > *crtc, const char *source_name)
> > > > return -EIO;
> > > > }
> > > >
> > > > - ret = get_new_crc_ctl_reg(dev_priv, crtc->index,
> > > > &source, &val,
> > > > true);
> > > > + intel_crtc_crc_prepare(dev_priv, crtc, source !=
> > > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > > +
> > >
> > > Aren't we potentially corrupting the last CRC(s) by turning off
> > > the
> > > w/as
> > > before disbling CRC capture?
> >
> > It will only be NULL when uses closes the file descriptor to CRC
> > data,
> > so even if its corrupted user will never get those CRCs.
>
> Userspace may not get it but the tracepoint will still see it. Which
> may
> cause confusion.
Okay changing to:
if (source != INTEL_PIPE_CRC_SOURCE_NONE)
intel_crtc_crc_setup_workarounds()
ret = get_new_crc_ctl_reg()
...
if (source == INTEL_PIPE_CRC_SOURCE_NONE)
intel_crtc_crc_setup_workarounds()
>
> > > > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index,
> > > > &source,
> > > > &val);
> > > > if (ret != 0)
> > > > goto out;
> > > >
> > > > @@ -629,9 +618,6 @@ int intel_crtc_set_crc_source(struct
> > > > drm_crtc
> > > > *crtc, const char *source_name)
> > > > if (!source) {
> > > > if (IS_VALLEYVIEW(dev_priv) ||
> > > > IS_CHERRYVIEW(dev_priv))
> > > > vlv_undo_pipe_scramble_reset(dev_priv,
> > > > crtc-
> > > > > index);
> > > > - else if ((IS_HASWELL(dev_priv) ||
> > > > - IS_BROADWELL(dev_priv)) && crtc-
> > > > >index ==
> > > > PIPE_A)
> > > > - hsw_pipe_A_crc_wa(dev_priv, false);
> > > > }
> > > >
> > > > pipe_crc->skipped = 0;
> > > > @@ -652,7 +638,7 @@ void intel_crtc_enable_pipe_crc(struct
> > > > intel_crtc *intel_crtc)
> > > > if (!crtc->crc.opened)
> > > > return;
> > > >
> > > > - if (get_new_crc_ctl_reg(dev_priv, crtc->index,
> > > > &pipe_crc-
> > > > > source, &val, false) < 0)
> > > > + if (get_new_crc_ctl_reg(dev_priv, crtc->index,
> > > > &pipe_crc-
> > > > > source, &val) < 0)
> > > > return;
> > > >
> > > > /* Don't need pipe_crc->lock here, IRQs are not
> > > > generated. */
> > > > --
> > > > 2.21.0
>
>
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic
2019-03-01 1:14 ` Souza, Jose
@ 2019-03-01 20:07 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 27+ messages in thread
From: Pandiyan, Dhinakaran @ 2019-03-01 20:07 UTC (permalink / raw)
To: ville.syrjala, Souza, Jose; +Cc: intel-gfx
On Thu, 2019-02-28 at 17:14 -0800, Souza, Jose wrote:
> On Thu, 2019-02-28 at 17:06 -0800, Dhinakaran Pandiyan wrote:
> > On Thu, 2019-02-28 at 15:26 -0800, Souza, Jose wrote:
> > > On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote:
> > > > On Wed, Feb 27, 2019 at 05:32:57PM -0800, José Roberto de Souza
> > > > wrote:
> > > > > Other features like PSR2 also needs to be disabled while
> > > > > getting
> > > > > CRC
> > > > > so lets rename ips_force_disable to crc_enabled, drop all
> > > > > this
> > > > > checks
> > > > > for pipe A and HSW and BDW and make it generic and
> > > > > hsw_compute_ips_config() will take care of all the checks
> > > > > removed
> > > > > from here.
> > > > >
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_display.c | 10 +++++--
> > > > > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > > > > drivers/gpu/drm/i915/intel_pipe_crc.c | 42 +++++++++--------
> > > > > ----
> > > > > ------
> > > > > 3 files changed, 24 insertions(+), 31 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 816e8f124b3b..328967c642b3 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -6751,7 +6751,13 @@ static bool
> > > > > hsw_compute_ips_config(struct
> > > > > intel_crtc_state *crtc_state)
> > > > > if (!hsw_crtc_state_ips_capable(crtc_state))
> > > > > return false;
> > > > >
> > > > > - if (crtc_state->ips_force_disable)
> > > > > + /*
> > > > > + * When IPS gets enabled, the pipe CRC changes. Since
> > > > > IPS gets
> > > > > + * enabled and disabled dynamically based on package C
> > > > > states,
> > > > > + * user space can't make reliable use of the CRCs, so
> > > > > let's
> > > > > just
> > > > > + * completely disable it.
> > > > > + */
> > > > > + if (crtc_state->crc_enabled)
> > > > > return false;
> > > >
> > > > Hmm. I was wondering how we even manage to pass the state
> > > > checker
> > > > with
> > > > the current code. But apparently we don't have state checking
> > > > for
> > > > IPS.
> > > > I would suggest moving this into hsw_compute_ips_config() and
> > > > then
> > > > adding the state checker (for HSW only though since BDW can't
> > > > do
> > > > the
> > > > readout).
> > > >
> > > > >
> > > > > /* IPS should be fine as long as at least one plane is
> > > > > enabled.
> > > > > */
> > > > > @@ -11684,7 +11690,7 @@ clear_intel_crtc_state(struct
> > > > > intel_crtc_state *crtc_state)
> > > > > saved_state->shared_dpll = crtc_state->shared_dpll;
> > > > > saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> > > > > saved_state->pch_pfit.force_thru = crtc_state-
> > > > > > pch_pfit.force_thru;
> > > > >
> > > > > - saved_state->ips_force_disable = crtc_state-
> > > > > > ips_force_disable;
> > > > >
> > > > > + saved_state->crc_enabled = crtc_state->crc_enabled;
> > > > > if (IS_G4X(dev_priv) ||
> > > > > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > saved_state->wm = crtc_state->wm;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 5412373e2f98..2be64529e4a2 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -999,7 +999,8 @@ struct intel_crtc_state {
> > > > > struct intel_link_m_n fdi_m_n;
> > > > >
> > > > > bool ips_enabled;
> > > > > - bool ips_force_disable;
> > > > > +
> > > > > + bool crc_enabled;
> > > > >
> > > > > bool enable_fbc;
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > index 53d4ec68d3c4..f6d0b2aaffe2 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > @@ -280,11 +280,12 @@ static int ilk_pipe_crc_ctl_reg(enum
> > > > > intel_pipe_crc_source *source,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > -static void hsw_pipe_A_crc_wa(struct drm_i915_private
> > > > > *dev_priv,
> > > > > - bool enable)
> > > > > +static void
> > > > > +intel_crtc_crc_prepare(struct drm_i915_private *dev_priv,
> > > > > struct
> > > > > drm_crtc *crtc,
> > > >
> > > > Just pass in the intel_crtc
> > >
> > > Okay
> > >
> > > > > + bool enable)
> > > > > {
> > > > > struct drm_device *dev = &dev_priv->drm;
> > > > > - struct intel_crtc *crtc =
> > > > > intel_get_crtc_for_pipe(dev_priv,
> > > > > PIPE_A);
> > > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > >
> > > > and then we don't have to have an ugly name for this.
> > > >
> > > > Also pasing in dev_priv is redundant when you're already
> > > > passing
> > > > in
> > > > the
> > > > crtc.
> > > >
> > >
> > > okay
> > >
> > > > The function name isn't super descriptive. It makes me think
> > > > we're
> > > > preparing for CRC capture, when in fact it just adds/removes
> > > > the
> > > > w/as.
> > >
> > > What about: intel_crtc_crc_workarounds_setup()?
> > >
> > > > > struct intel_crtc_state *pipe_config;
> > > > > struct drm_atomic_state *state;
> > > > > struct drm_modeset_acquire_ctx ctx;
> > > > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct
> > > > > drm_i915_private *dev_priv,
> > > > > state->acquire_ctx = &ctx;
> > > > >
> > > > > retry:
> > > > > - pipe_config = intel_atomic_get_crtc_state(state, crtc);
> > > > > + pipe_config = intel_atomic_get_crtc_state(state,
> > > > > intel_crtc);
> > > > > if (IS_ERR(pipe_config)) {
> > > > > ret = PTR_ERR(pipe_config);
> > > > > goto put_state;
> > > > > }
> > > > >
> > > > > - if (HAS_IPS(dev_priv)) {
> > > > > - /*
> > > > > - * When IPS gets enabled, the pipe CRC changes.
> > > > > Since
> > > > > IPS gets
> > > > > - * enabled and disabled dynamically based on
> > > > > package C
> > > > > states,
> > > > > - * user space can't make reliable use of the
> > > > > CRCs, so
> > > > > let's just
> > > > > - * completely disable it.
> > > > > - */
> > > > > - pipe_config->ips_force_disable = enable;
> > > > > - }
> > > > > + pipe_config->crc_enabled = enable;
> > > > >
> > > > > - if (IS_HASWELL(dev_priv)) {
> > > > > + if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A)
> > > > > {
> > > > > pipe_config->pch_pfit.force_thru = enable;
> > > > > if (pipe_config->cpu_transcoder ==
> > > > > TRANSCODER_EDP &&
> > > > > pipe_config->pch_pfit.enabled != enable)
> > > > > @@ -343,8 +336,7 @@ static void hsw_pipe_A_crc_wa(struct
> > > > > drm_i915_private *dev_priv,
> > > > > static int ivb_pipe_crc_ctl_reg(struct drm_i915_private
> > > > > *dev_priv,
> > > > > enum pipe pipe,
> > > > > enum intel_pipe_crc_source
> > > > > *source,
> > > > > - u32 *val,
> > > > > - bool set_wa)
> > > > > + u32 *val)
> > > > > {
> > > > > if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> > > > > *source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > > > > @@ -357,10 +349,6 @@ static int ivb_pipe_crc_ctl_reg(struct
> > > > > drm_i915_private *dev_priv,
> > > > > *val = PIPE_CRC_ENABLE |
> > > > > PIPE_CRC_SOURCE_SPRITE_IVB;
> > > > > break;
> > > > > case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > > > - if (set_wa && (IS_HASWELL(dev_priv) ||
> > > > > - IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > > > > - hsw_pipe_A_crc_wa(dev_priv, true);
> > > > > -
> > > > > *val = PIPE_CRC_ENABLE |
> > > > > PIPE_CRC_SOURCE_PF_IVB;
> > > > > break;
> > > > > case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > > @@ -418,8 +406,7 @@ static int skl_pipe_crc_ctl_reg(struct
> > > > > drm_i915_private *dev_priv,
> > > > >
> > > > > static int get_new_crc_ctl_reg(struct drm_i915_private
> > > > > *dev_priv,
> > > > > enum pipe pipe,
> > > > > - enum intel_pipe_crc_source
> > > > > *source, u32
> > > > > *val,
> > > > > - bool set_wa)
> > > > > + enum intel_pipe_crc_source
> > > > > *source, u32
> > > > > *val)
> > > > > {
> > > > > if (IS_GEN(dev_priv, 2))
> > > > > return i8xx_pipe_crc_ctl_reg(source, val);
> > > > > @@ -430,7 +417,7 @@ static int get_new_crc_ctl_reg(struct
> > > > > drm_i915_private *dev_priv,
> > > > > else if (IS_GEN_RANGE(dev_priv, 5, 6))
> > > > > return ilk_pipe_crc_ctl_reg(source, val);
> > > > > else if (INTEL_GEN(dev_priv) < 9)
> > > > > - return ivb_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > > source,
> > > > > val, set_wa);
> > > > > + return ivb_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > > source,
> > > > > val);
> > > > > else
> > > > > return skl_pipe_crc_ctl_reg(dev_priv, pipe,
> > > > > source,
> > > > > val);
> > > > > }
> > > > > @@ -618,7 +605,9 @@ int intel_crtc_set_crc_source(struct
> > > > > drm_crtc
> > > > > *crtc, const char *source_name)
> > > > > return -EIO;
> > > > > }
> > > > >
> > > > > - ret = get_new_crc_ctl_reg(dev_priv, crtc->index,
> > > > > &source, &val,
> > > > > true);
> > > > > + intel_crtc_crc_prepare(dev_priv, crtc, source !=
> > > > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > > > +
> > > >
> > > > Aren't we potentially corrupting the last CRC(s) by turning off
> > > > the
> > > > w/as
> > > > before disbling CRC capture?
> > >
> > > It will only be NULL when uses closes the file descriptor to CRC
> > > data,
> > > so even if its corrupted user will never get those CRCs.
> >
> > Even if the userspace reads from another thread? Probably not a
> > common
> > use case though.
>
> It is called with NULL from the release() hook, so when all
> filedescriptors to this file is closed.
>
> But anyways the drm crc code only allows one open() of this file at
> time, so if other thread tries to read it after the main thread have
> closed it it will fail before get into any drm code.
>
Makes sense, thanks.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC
2019-02-28 1:32 ` [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC José Roberto de Souza
2019-02-28 16:58 ` Ville Syrjälä
@ 2019-03-01 20:12 ` Dhinakaran Pandiyan
2019-03-01 20:18 ` Souza, Jose
2019-03-01 20:45 ` Ville Syrjälä
2 siblings, 1 reply; 27+ messages in thread
From: Dhinakaran Pandiyan @ 2019-03-01 20:12 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx
On Wed, 2019-02-27 at 17:32 -0800, José Roberto de Souza wrote:
> When PSR2 is active aka after the number of frames programmed in
> PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC
> interruptions causing IGT tests to fail due timeout.
Just to make sure we are documenting the issue correctly in the commit
message - is it the SU state when CRC generation stops or deep sleep?
-DK
>
> Oddly that don't happen when PSR1 active, so here it switches from
> PSR2 to PSR1 while user is requesting pipe CRC.
>
> Force setting mode_changed as true is necessary to atomic checks
> functions compute new PSR state, that is why it was added to
> intel_crtc_crc_prepare().
>
> v3: Reusing intel_crtc_crc_prepare() and crc_enabled
>
> v2: Changed commit description to describe that PSR2 inhibit CRC
> calculations.
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 3 +++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index f6d0b2aaffe2..e7ac24c33650 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -308,6 +308,7 @@ intel_crtc_crc_prepare(struct drm_i915_private
> *dev_priv, struct drm_crtc *crtc,
> goto put_state;
> }
>
> + pipe_config->base.mode_changed = pipe_config->crc_enabled !=
> enable;
> pipe_config->crc_enabled = enable;
>
> if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 6175b1d2e0c8..f7730b8b2ec0 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct
> intel_dp *intel_dp,
> return false;
> }
>
> + if (crtc_state->crc_enabled)
> + return false;
> +
> return true;
> }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC
2019-03-01 20:12 ` Dhinakaran Pandiyan
@ 2019-03-01 20:18 ` Souza, Jose
0 siblings, 0 replies; 27+ messages in thread
From: Souza, Jose @ 2019-03-01 20:18 UTC (permalink / raw)
To: intel-gfx, Pandiyan, Dhinakaran
[-- Attachment #1.1: Type: text/plain, Size: 2455 bytes --]
On Fri, 2019-03-01 at 12:12 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2019-02-27 at 17:32 -0800, José Roberto de Souza wrote:
> > When PSR2 is active aka after the number of frames programmed in
> > PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC
> > interruptions causing IGT tests to fail due timeout.
>
> Just to make sure we are documenting the issue correctly in the
> commit
> message - is it the SU state when CRC generation stops or deep sleep?
'Frames Before SU Entry' is the number of frames to activate PSR2, so
it stops as soons as PSR2 is active.
>
> -DK
> > Oddly that don't happen when PSR1 active, so here it switches from
> > PSR2 to PSR1 while user is requesting pipe CRC.
> >
> > Force setting mode_changed as true is necessary to atomic checks
> > functions compute new PSR state, that is why it was added to
> > intel_crtc_crc_prepare().
> >
> > v3: Reusing intel_crtc_crc_prepare() and crc_enabled
> >
> > v2: Changed commit description to describe that PSR2 inhibit CRC
> > calculations.
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
> > drivers/gpu/drm/i915/intel_psr.c | 3 +++
> > 2 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index f6d0b2aaffe2..e7ac24c33650 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -308,6 +308,7 @@ intel_crtc_crc_prepare(struct drm_i915_private
> > *dev_priv, struct drm_crtc *crtc,
> > goto put_state;
> > }
> >
> > + pipe_config->base.mode_changed = pipe_config->crc_enabled !=
> > enable;
> > pipe_config->crc_enabled = enable;
> >
> > if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 6175b1d2e0c8..f7730b8b2ec0 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> > return false;
> > }
> >
> > + if (crtc_state->crc_enabled)
> > + return false;
> > +
> > return true;
> > }
> >
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC
2019-02-28 1:32 ` [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC José Roberto de Souza
2019-02-28 16:58 ` Ville Syrjälä
2019-03-01 20:12 ` Dhinakaran Pandiyan
@ 2019-03-01 20:45 ` Ville Syrjälä
2019-03-01 22:18 ` Souza, Jose
2 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2019-03-01 20:45 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx, Dhinakaran Pandiyan
On Wed, Feb 27, 2019 at 05:32:58PM -0800, José Roberto de Souza wrote:
> When PSR2 is active aka after the number of frames programmed in
> PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC
> interruptions causing IGT tests to fail due timeout.
I'm more concerned about the all ones (or was it all zeroes?) crc we
get when coming back from PSR. But I don't remmber right now if that
was limited PSR2 or if it happens with PSR1 as well. Have you looked
at that issue as well?
>
> Oddly that don't happen when PSR1 active, so here it switches from
> PSR2 to PSR1 while user is requesting pipe CRC.
>
> Force setting mode_changed as true is necessary to atomic checks
> functions compute new PSR state, that is why it was added to
> intel_crtc_crc_prepare().
>
> v3: Reusing intel_crtc_crc_prepare() and crc_enabled
>
> v2: Changed commit description to describe that PSR2 inhibit CRC
> calculations.
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 3 +++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index f6d0b2aaffe2..e7ac24c33650 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -308,6 +308,7 @@ intel_crtc_crc_prepare(struct drm_i915_private *dev_priv, struct drm_crtc *crtc,
> goto put_state;
> }
>
> + pipe_config->base.mode_changed = pipe_config->crc_enabled != enable;
> pipe_config->crc_enabled = enable;
>
> if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 6175b1d2e0c8..f7730b8b2ec0 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> return false;
> }
>
> + if (crtc_state->crc_enabled)
> + return false;
> +
> return true;
> }
>
> --
> 2.21.0
--
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] 27+ messages in thread
* Re: [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC
2019-03-01 20:45 ` Ville Syrjälä
@ 2019-03-01 22:18 ` Souza, Jose
0 siblings, 0 replies; 27+ messages in thread
From: Souza, Jose @ 2019-03-01 22:18 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx, Pandiyan, Dhinakaran
[-- Attachment #1.1: Type: text/plain, Size: 2771 bytes --]
On Fri, 2019-03-01 at 22:45 +0200, Ville Syrjälä wrote:
> On Wed, Feb 27, 2019 at 05:32:58PM -0800, José Roberto de Souza
> wrote:
> > When PSR2 is active aka after the number of frames programmed in
> > PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC
> > interruptions causing IGT tests to fail due timeout.
>
> I'm more concerned about the all ones (or was it all zeroes?) crc we
> get when coming back from PSR. But I don't remmber right now if that
> was limited PSR2 or if it happens with PSR1 as well. Have you looked
> at that issue as well?
Just wrote a test that gets 500 CRCs and the real difference between
PSR1 and PSR2 is that PSR1 activation is blocked after the pipe CRC is
enabled while on PSR2 that don't happen.
After exit PSR2 I got more 4 CRC interruptions, 1 invalid value and 3
valid ones.
Got the results above in a WHL and ICL.
>
> > Oddly that don't happen when PSR1 active, so here it switches from
> > PSR2 to PSR1 while user is requesting pipe CRC.
> >
> > Force setting mode_changed as true is necessary to atomic checks
> > functions compute new PSR state, that is why it was added to
> > intel_crtc_crc_prepare().
> >
> > v3: Reusing intel_crtc_crc_prepare() and crc_enabled
> >
> > v2: Changed commit description to describe that PSR2 inhibit CRC
> > calculations.
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
> > drivers/gpu/drm/i915/intel_psr.c | 3 +++
> > 2 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index f6d0b2aaffe2..e7ac24c33650 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -308,6 +308,7 @@ intel_crtc_crc_prepare(struct drm_i915_private
> > *dev_priv, struct drm_crtc *crtc,
> > goto put_state;
> > }
> >
> > + pipe_config->base.mode_changed = pipe_config->crc_enabled !=
> > enable;
> > pipe_config->crc_enabled = enable;
> >
> > if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A) {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 6175b1d2e0c8..f7730b8b2ec0 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> > return false;
> > }
> >
> > + if (crtc_state->crc_enabled)
> > + return false;
> > +
> > return true;
> > }
> >
> > --
> > 2.21.0
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2019-03-01 22:19 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-28 1:32 [PATCH v3 1/6] drm/i915/psr: Remove PSR2 FIXME José Roberto de Souza
2019-02-28 1:32 ` [PATCH v3 2/6] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset José Roberto de Souza
2019-02-28 16:39 ` Ville Syrjälä
2019-02-28 22:39 ` Souza, Jose
2019-02-28 1:32 ` [PATCH v3 3/6] drm/i915: Compute and commit color features in fastsets José Roberto de Souza
2019-02-28 16:41 ` Ville Syrjälä
2019-02-28 1:32 ` [PATCH v3 4/6] drm/i915/crc: Make IPS workaround generic José Roberto de Souza
2019-02-28 16:56 ` Ville Syrjälä
2019-02-28 17:04 ` Ville Syrjälä
2019-02-28 23:26 ` Souza, Jose
2019-03-01 1:06 ` Dhinakaran Pandiyan
2019-03-01 1:14 ` Souza, Jose
2019-03-01 20:07 ` Pandiyan, Dhinakaran
2019-03-01 13:35 ` Ville Syrjälä
2019-03-01 18:29 ` Souza, Jose
2019-02-28 1:32 ` [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC José Roberto de Souza
2019-02-28 16:58 ` Ville Syrjälä
2019-02-28 23:07 ` Souza, Jose
2019-03-01 1:57 ` Dhinakaran Pandiyan
2019-03-01 2:11 ` Souza, Jose
2019-03-01 20:12 ` Dhinakaran Pandiyan
2019-03-01 20:18 ` Souza, Jose
2019-03-01 20:45 ` Ville Syrjälä
2019-03-01 22:18 ` Souza, Jose
2019-02-28 1:32 ` [PATCH v3 6/6] drm/i915: Enable PSR2 by default José Roberto de Souza
2019-02-28 2:35 ` ✓ Fi.CI.BAT: success for series starting with [v3,1/6] drm/i915/psr: Remove PSR2 FIXME Patchwork
2019-02-28 5:34 ` ✗ Fi.CI.IGT: failure " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.