* [PATCH 1/2] drm/i915: Check if PSR is globally enabled before change PSR variables
@ 2018-11-30 2:31 José Roberto de Souza
2018-11-30 2:31 ` [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug José Roberto de Souza
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: José Roberto de Souza @ 2018-11-30 2:31 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi
There is no issues changing the PSR variables even if PSR will be not
enabled but it avoid having misleading values like have psr2_enabled
set but enabled unset.
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_psr.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2084784f320d..827b8c31783d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -716,14 +716,15 @@ void intel_psr_enable(struct intel_dp *intel_dp,
goto unlock;
}
+ if (!psr_global_enabled(dev_priv->psr.debug)) {
+ DRM_DEBUG_KMS("PSR disabled by flag\n");
+ goto unlock;
+ }
+
dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
dev_priv->psr.busy_frontbuffer_bits = 0;
dev_priv->psr.prepared = true;
-
- if (psr_global_enabled(dev_priv->psr.debug))
- intel_psr_enable_locked(dev_priv, crtc_state);
- else
- DRM_DEBUG_KMS("PSR disabled by flag\n");
+ intel_psr_enable_locked(dev_priv, crtc_state);
unlock:
mutex_unlock(&dev_priv->psr.lock);
--
2.19.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
2018-11-30 2:31 [PATCH 1/2] drm/i915: Check if PSR is globally enabled before change PSR variables José Roberto de Souza
@ 2018-11-30 2:31 ` José Roberto de Souza
2018-12-04 1:33 ` Dhinakaran Pandiyan
2018-11-30 2:58 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Check if PSR is globally enabled before change PSR variables Patchwork
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: José Roberto de Souza @ 2018-11-30 2:31 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi
Changing the i915_edp_psr_debug was enabling, disabling or switching
PSR version by directly calling intel_psr_disable_locked() and
intel_psr_enable_locked(), what is not the default PSR path that is
executed in a regular modesets.
So lets force a modeset in the PSR CRTC to trigger the requested PSR
state change and really stress the code path that matters for the
regular user.
Also by doing this way it fixes the issue below, were DRRS was left
enabled together with PSR when enabling PSR from debugfs.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 14 +---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/intel_drv.h | 4 +-
drivers/gpu/drm/i915/intel_psr.c | 119 ++++++++++++----------------
4 files changed, 55 insertions(+), 84 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 129b9a6f8309..bc32f683dc46 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2775,7 +2775,6 @@ static int
i915_edp_psr_debug_set(void *data, u64 val)
{
struct drm_i915_private *dev_priv = data;
- struct drm_modeset_acquire_ctx ctx;
int ret;
if (!CAN_PSR(dev_priv))
@@ -2785,18 +2784,7 @@ i915_edp_psr_debug_set(void *data, u64 val)
intel_runtime_pm_get(dev_priv);
- drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
-
-retry:
- ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
- if (ret == -EDEADLK) {
- ret = drm_modeset_backoff(&ctx);
- if (!ret)
- goto retry;
- }
-
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
+ ret = intel_psr_set_debugfs_mode(dev_priv, val);
intel_runtime_pm_put(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 43ac6873a2bb..93d8ca9c0375 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -493,7 +493,7 @@ struct i915_psr {
u32 debug;
bool sink_support;
- bool prepared, enabled;
+ bool enabled;
struct intel_dp *dp;
bool active;
struct work_struct work;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 86ff57738cd9..89d4eed0dd89 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2056,9 +2056,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
const struct intel_crtc_state *crtc_state);
void intel_psr_disable(struct intel_dp *intel_dp,
const struct intel_crtc_state *old_crtc_state);
-int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
- struct drm_modeset_acquire_ctx *ctx,
- u64 value);
+int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, u64 value);
void intel_psr_invalidate(struct drm_i915_private *dev_priv,
unsigned frontbuffer_bits,
enum fb_op_origin origin);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 827b8c31783d..305848cceda7 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -52,6 +52,7 @@
*/
#include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
#include "intel_drv.h"
#include "i915_drv.h"
@@ -711,10 +712,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
WARN_ON(dev_priv->drrs.dp);
mutex_lock(&dev_priv->psr.lock);
- if (dev_priv->psr.prepared) {
- DRM_DEBUG_KMS("PSR already in use\n");
- goto unlock;
- }
if (!psr_global_enabled(dev_priv->psr.debug)) {
DRM_DEBUG_KMS("PSR disabled by flag\n");
@@ -723,7 +720,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
dev_priv->psr.busy_frontbuffer_bits = 0;
- dev_priv->psr.prepared = true;
intel_psr_enable_locked(dev_priv, crtc_state);
unlock:
@@ -807,14 +803,9 @@ void intel_psr_disable(struct intel_dp *intel_dp,
return;
mutex_lock(&dev_priv->psr.lock);
- if (!dev_priv->psr.prepared) {
- mutex_unlock(&dev_priv->psr.lock);
- return;
- }
intel_psr_disable_locked(intel_dp);
- dev_priv->psr.prepared = false;
mutex_unlock(&dev_priv->psr.lock);
cancel_work_sync(&dev_priv->psr.work);
}
@@ -883,36 +874,61 @@ static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
return err == 0 && dev_priv->psr.enabled;
}
-static bool switching_psr(struct drm_i915_private *dev_priv,
- struct intel_crtc_state *crtc_state,
- u32 mode)
+static int intel_psr_modeset_force(struct drm_i915_private *dev_priv)
{
- /* Can't switch psr state anyway if PSR2 is not supported. */
- if (!crtc_state || !crtc_state->has_psr2)
- return false;
+ struct drm_device *dev = &dev_priv->drm;
+ struct drm_modeset_acquire_ctx ctx;
+ struct drm_atomic_state *state;
+ struct drm_crtc_state *crtc_state;
+ struct drm_crtc *crtc;
+ int err, i;
+
+ mutex_lock(&dev->mode_config.mutex);
+ drm_modeset_acquire_init(&ctx, 0);
+
+modeset_lock_retry:
+ err = drm_modeset_lock_all_ctx(dev, &ctx);
+ if (err) {
+ if (err == -EDEADLK) {
+ err = drm_modeset_backoff(&ctx);
+ if (!err)
+ goto modeset_lock_retry;
+ }
- if (dev_priv->psr.psr2_enabled && mode == I915_PSR_DEBUG_FORCE_PSR1)
- return true;
+ goto unlock;
+ }
- if (!dev_priv->psr.psr2_enabled && mode != I915_PSR_DEBUG_FORCE_PSR1)
- return true;
+ state = drm_atomic_helper_duplicate_state(dev, &ctx);
+ if (IS_ERR(state)) {
+ err = PTR_ERR(state);
+ goto unlock;
+ }
+
+ for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+ struct intel_crtc_state *pipe_state;
+
+ pipe_state = to_intel_crtc_state(crtc_state);
+ /* Mark mode as changed to force a modeset */
+ if (pipe_state->has_psr)
+ crtc_state->mode_changed = true;
+ }
+
+ err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
- return false;
+ drm_atomic_state_put(state);
+unlock:
+ drm_modeset_drop_locks(&ctx);
+ drm_modeset_acquire_fini(&ctx);
+ mutex_unlock(&dev->mode_config.mutex);
+
+ return err;
}
-int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
- struct drm_modeset_acquire_ctx *ctx,
- u64 val)
+int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, u64 val)
{
- struct drm_device *dev = &dev_priv->drm;
- struct drm_connector_state *conn_state;
- struct intel_crtc_state *crtc_state = NULL;
- struct drm_crtc_commit *commit;
- struct drm_crtc *crtc;
- struct intel_dp *dp;
+ const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
+ u32 old_mode;
int ret;
- bool enable;
- u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
mode > I915_PSR_DEBUG_FORCE_PSR1) {
@@ -920,49 +936,18 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
return -EINVAL;
}
- ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
- if (ret)
- return ret;
-
- /* dev_priv->psr.dp should be set once and then never touched again. */
- dp = READ_ONCE(dev_priv->psr.dp);
- conn_state = dp->attached_connector->base.state;
- crtc = conn_state->crtc;
- if (crtc) {
- ret = drm_modeset_lock(&crtc->mutex, ctx);
- if (ret)
- return ret;
-
- crtc_state = to_intel_crtc_state(crtc->state);
- commit = crtc_state->base.commit;
- } else {
- commit = conn_state->commit;
- }
- if (commit) {
- ret = wait_for_completion_interruptible(&commit->hw_done);
- if (ret)
- return ret;
- }
-
ret = mutex_lock_interruptible(&dev_priv->psr.lock);
if (ret)
return ret;
- enable = psr_global_enabled(val);
-
- if (!enable || switching_psr(dev_priv, crtc_state, mode))
- intel_psr_disable_locked(dev_priv->psr.dp);
-
+ old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
dev_priv->psr.debug = val;
- if (crtc)
- dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
- intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
+ mutex_unlock(&dev_priv->psr.lock);
- if (dev_priv->psr.prepared && enable)
- intel_psr_enable_locked(dev_priv, crtc_state);
+ if (old_mode != mode)
+ ret = intel_psr_modeset_force(dev_priv);
- mutex_unlock(&dev_priv->psr.lock);
return ret;
}
--
2.19.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Check if PSR is globally enabled before change PSR variables
2018-11-30 2:31 [PATCH 1/2] drm/i915: Check if PSR is globally enabled before change PSR variables José Roberto de Souza
2018-11-30 2:31 ` [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug José Roberto de Souza
@ 2018-11-30 2:58 ` Patchwork
2018-11-30 3:16 ` ✓ Fi.CI.BAT: success " Patchwork
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-11-30 2:58 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915: Check if PSR is globally enabled before change PSR variables
URL : https://patchwork.freedesktop.org/series/53293/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
3558d3c4ede6 drm/i915: Check if PSR is globally enabled before change PSR variables
e80f11002bee drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
-:69: CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384
#69: FILE: drivers/gpu/drm/i915/i915_drv.h:496:
+ bool enabled;
total: 0 errors, 0 warnings, 1 checks, 218 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Check if PSR is globally enabled before change PSR variables
2018-11-30 2:31 [PATCH 1/2] drm/i915: Check if PSR is globally enabled before change PSR variables José Roberto de Souza
2018-11-30 2:31 ` [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug José Roberto de Souza
2018-11-30 2:58 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Check if PSR is globally enabled before change PSR variables Patchwork
@ 2018-11-30 3:16 ` Patchwork
2018-11-30 5:56 ` [PATCH 1/2] " Rodrigo Vivi
2018-12-04 0:58 ` Dhinakaran Pandiyan
4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-11-30 3:16 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915: Check if PSR is globally enabled before change PSR variables
URL : https://patchwork.freedesktop.org/series/53293/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5227 -> Patchwork_10971
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/53293/revisions/1/mbox/
Known issues
------------
Here are the changes found in Patchwork_10971 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live_execlists:
- fi-apl-guc: PASS -> INCOMPLETE [fdo#103927]
#### Possible fixes ####
* igt@gem_ctx_create@basic-files:
- fi-bsw-kefka: FAIL [fdo#108656] -> PASS
* igt@kms_frontbuffer_tracking@basic:
- fi-byt-clapper: FAIL [fdo#103167] -> PASS
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#108656]: https://bugs.freedesktop.org/show_bug.cgi?id=108656
Participating hosts (50 -> 44)
------------------------------
Additional (1): fi-glk-j4005
Missing (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus
Build changes
-------------
* Linux: CI_DRM_5227 -> Patchwork_10971
CI_DRM_5227: 95052693524067ba66e1a6733355739fbcc8d5b6 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4736: 285ebfb3b7adc56586031afa5150c4e5ad40c229 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10971: e80f11002beea20c381b747016f887c59672bbd0 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
e80f11002bee drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
3558d3c4ede6 drm/i915: Check if PSR is globally enabled before change PSR variables
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10971/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] drm/i915: Check if PSR is globally enabled before change PSR variables
2018-11-30 2:31 [PATCH 1/2] drm/i915: Check if PSR is globally enabled before change PSR variables José Roberto de Souza
` (2 preceding siblings ...)
2018-11-30 3:16 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-11-30 5:56 ` Rodrigo Vivi
2018-12-04 0:58 ` Dhinakaran Pandiyan
4 siblings, 0 replies; 15+ messages in thread
From: Rodrigo Vivi @ 2018-11-30 5:56 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx, Dhinakaran Pandiyan
On Thu, Nov 29, 2018 at 06:31:32PM -0800, José Roberto de Souza wrote:
> There is no issues changing the PSR variables even if PSR will be not
> enabled but it avoid having misleading values like have psr2_enabled
> set but enabled unset.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/intel_psr.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2084784f320d..827b8c31783d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -716,14 +716,15 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> goto unlock;
> }
>
> + if (!psr_global_enabled(dev_priv->psr.debug)) {
> + DRM_DEBUG_KMS("PSR disabled by flag\n");
> + goto unlock;
> + }
> +
> dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
> dev_priv->psr.busy_frontbuffer_bits = 0;
> dev_priv->psr.prepared = true;
> -
> - if (psr_global_enabled(dev_priv->psr.debug))
> - intel_psr_enable_locked(dev_priv, crtc_state);
> - else
> - DRM_DEBUG_KMS("PSR disabled by flag\n");
> + intel_psr_enable_locked(dev_priv, crtc_state);
>
> unlock:
> mutex_unlock(&dev_priv->psr.lock);
> --
> 2.19.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] drm/i915: Check if PSR is globally enabled before change PSR variables
2018-11-30 2:31 [PATCH 1/2] drm/i915: Check if PSR is globally enabled before change PSR variables José Roberto de Souza
` (3 preceding siblings ...)
2018-11-30 5:56 ` [PATCH 1/2] " Rodrigo Vivi
@ 2018-12-04 0:58 ` Dhinakaran Pandiyan
2018-12-04 1:09 ` Souza, Jose
4 siblings, 1 reply; 15+ messages in thread
From: Dhinakaran Pandiyan @ 2018-12-04 0:58 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx; +Cc: Rodrigo Vivi
On Thu, 2018-11-29 at 18:31 -0800, José Roberto de Souza wrote:
> There is no issues changing the PSR variables even if PSR will be not
> enabled but it avoid having misleading values like have psr2_enabled
> set but enabled unset.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/intel_psr.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 2084784f320d..827b8c31783d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -716,14 +716,15 @@ void intel_psr_enable(struct intel_dp
> *intel_dp,
> goto unlock;
> }
>
> + if (!psr_global_enabled(dev_priv->psr.debug)) {
> + DRM_DEBUG_KMS("PSR disabled by flag\n");
> + goto unlock;
> + }
> +
> dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
> crtc_state);
> dev_priv->psr.busy_frontbuffer_bits = 0;
> dev_priv->psr.prepared = true;
.prepared needs to be set even when psr_global_enabled() returns false.
This is so that we can enable PSR via debugfs later.
> -
> - if (psr_global_enabled(dev_priv->psr.debug))
> - intel_psr_enable_locked(dev_priv, crtc_state);
> - else
> - DRM_DEBUG_KMS("PSR disabled by flag\n");
> + intel_psr_enable_locked(dev_priv, crtc_state);
>
> unlock:
> mutex_unlock(&dev_priv->psr.lock);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] drm/i915: Check if PSR is globally enabled before change PSR variables
2018-12-04 0:58 ` Dhinakaran Pandiyan
@ 2018-12-04 1:09 ` Souza, Jose
0 siblings, 0 replies; 15+ messages in thread
From: Souza, Jose @ 2018-12-04 1:09 UTC (permalink / raw)
To: intel-gfx, Pandiyan, Dhinakaran; +Cc: Vivi, Rodrigo
[-- Attachment #1.1: Type: text/plain, Size: 1846 bytes --]
On Mon, 2018-12-03 at 16:58 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-11-29 at 18:31 -0800, José Roberto de Souza wrote:
> > There is no issues changing the PSR variables even if PSR will be
> > not
> > enabled but it avoid having misleading values like have
> > psr2_enabled
> > set but enabled unset.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_psr.c | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 2084784f320d..827b8c31783d 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -716,14 +716,15 @@ void intel_psr_enable(struct intel_dp
> > *intel_dp,
> > goto unlock;
> > }
> >
> > + if (!psr_global_enabled(dev_priv->psr.debug)) {
> > + DRM_DEBUG_KMS("PSR disabled by flag\n");
> > + goto unlock;
> > + }
> > +
> > dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
> > crtc_state);
> > dev_priv->psr.busy_frontbuffer_bits = 0;
> > dev_priv->psr.prepared = true;
> .prepared needs to be set even when psr_global_enabled() returns
> false.
> This is so that we can enable PSR via debugfs later.
Oh that is right =/
Well so we can squash this patch with the next one.
>
> > -
> > - if (psr_global_enabled(dev_priv->psr.debug))
> > - intel_psr_enable_locked(dev_priv, crtc_state);
> > - else
> > - DRM_DEBUG_KMS("PSR disabled by flag\n");
> > + intel_psr_enable_locked(dev_priv, crtc_state);
> >
> > unlock:
> > mutex_unlock(&dev_priv->psr.lock);
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
2018-11-30 2:31 ` [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug José Roberto de Souza
@ 2018-12-04 1:33 ` Dhinakaran Pandiyan
2018-12-04 1:54 ` Souza, Jose
0 siblings, 1 reply; 15+ messages in thread
From: Dhinakaran Pandiyan @ 2018-12-04 1:33 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx; +Cc: Rodrigo Vivi
On Thu, 2018-11-29 at 18:31 -0800, José Roberto de Souza wrote:
> Changing the i915_edp_psr_debug was enabling, disabling or switching
> PSR version by directly calling intel_psr_disable_locked() and
> intel_psr_enable_locked(), what is not the default PSR path that is
> executed in a regular modesets.
>
> So lets force a modeset in the PSR CRTC to trigger the requested PSR
> state change and really stress the code path that matters for the
> regular user.
>
> Also by doing this way it fixes the issue below, were DRRS was left
> enabled together with PSR when enabling PSR from debugfs.
While this patch does fix the issue, psr_compute_config() not checking
crtc_state->has_drrs seems odd. We should change it to not set
crtc_state->has_psr if crtc_state->has_drrs happens to be set. Or do it
the other way around.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 14 +---
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 4 +-
> drivers/gpu/drm/i915/intel_psr.c | 119 ++++++++++++------------
> ----
> 4 files changed, 55 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 129b9a6f8309..bc32f683dc46 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2775,7 +2775,6 @@ static int
> i915_edp_psr_debug_set(void *data, u64 val)
> {
> struct drm_i915_private *dev_priv = data;
> - struct drm_modeset_acquire_ctx ctx;
> int ret;
>
> if (!CAN_PSR(dev_priv))
> @@ -2785,18 +2784,7 @@ i915_edp_psr_debug_set(void *data, u64 val)
>
> intel_runtime_pm_get(dev_priv);
>
> - drm_modeset_acquire_init(&ctx,
> DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> -
> -retry:
> - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
> - if (ret == -EDEADLK) {
> - ret = drm_modeset_backoff(&ctx);
> - if (!ret)
> - goto retry;
> - }
> -
> - drm_modeset_drop_locks(&ctx);
> - drm_modeset_acquire_fini(&ctx);
> + ret = intel_psr_set_debugfs_mode(dev_priv, val);
>
> intel_runtime_pm_put(dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 43ac6873a2bb..93d8ca9c0375 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -493,7 +493,7 @@ struct i915_psr {
>
> u32 debug;
> bool sink_support;
> - bool prepared, enabled;
> + bool enabled;
> struct intel_dp *dp;
> bool active;
> struct work_struct work;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 86ff57738cd9..89d4eed0dd89 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2056,9 +2056,7 @@ void intel_psr_enable(struct intel_dp
> *intel_dp,
> const struct intel_crtc_state *crtc_state);
> void intel_psr_disable(struct intel_dp *intel_dp,
> const struct intel_crtc_state *old_crtc_state);
> -int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> - struct drm_modeset_acquire_ctx *ctx,
> - u64 value);
> +int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> u64 value);
> void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> unsigned frontbuffer_bits,
> enum fb_op_origin origin);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 827b8c31783d..305848cceda7 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -52,6 +52,7 @@
> */
>
> #include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
>
> #include "intel_drv.h"
> #include "i915_drv.h"
> @@ -711,10 +712,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> WARN_ON(dev_priv->drrs.dp);
>
> mutex_lock(&dev_priv->psr.lock);
> - if (dev_priv->psr.prepared) {
> - DRM_DEBUG_KMS("PSR already in use\n");
> - goto unlock;
> - }
>
> if (!psr_global_enabled(dev_priv->psr.debug)) {
> DRM_DEBUG_KMS("PSR disabled by flag\n");
> @@ -723,7 +720,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>
> dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
> crtc_state);
> dev_priv->psr.busy_frontbuffer_bits = 0;
> - dev_priv->psr.prepared = true;
> intel_psr_enable_locked(dev_priv, crtc_state);
>
> unlock:
> @@ -807,14 +803,9 @@ void intel_psr_disable(struct intel_dp
> *intel_dp,
> return;
>
> mutex_lock(&dev_priv->psr.lock);
> - if (!dev_priv->psr.prepared) {
> - mutex_unlock(&dev_priv->psr.lock);
> - return;
> - }
>
> intel_psr_disable_locked(intel_dp);
>
> - dev_priv->psr.prepared = false;
> mutex_unlock(&dev_priv->psr.lock);
> cancel_work_sync(&dev_priv->psr.work);
> }
> @@ -883,36 +874,61 @@ static bool __psr_wait_for_idle_locked(struct
> drm_i915_private *dev_priv)
> return err == 0 && dev_priv->psr.enabled;
> }
>
> -static bool switching_psr(struct drm_i915_private *dev_priv,
> - struct intel_crtc_state *crtc_state,
> - u32 mode)
> +static int intel_psr_modeset_force(struct drm_i915_private
> *dev_priv)
> {
> - /* Can't switch psr state anyway if PSR2 is not supported. */
> - if (!crtc_state || !crtc_state->has_psr2)
> - return false;
> + struct drm_device *dev = &dev_priv->drm;
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_atomic_state *state;
> + struct drm_crtc_state *crtc_state;
> + struct drm_crtc *crtc;
> + int err, i;
> +
> + mutex_lock(&dev->mode_config.mutex);
> + drm_modeset_acquire_init(&ctx, 0);
> +
> +modeset_lock_retry:
> + err = drm_modeset_lock_all_ctx(dev, &ctx);
> + if (err) {
> + if (err == -EDEADLK) {
> + err = drm_modeset_backoff(&ctx);
> + if (!err)
> + goto modeset_lock_retry;
> + }
>
> - if (dev_priv->psr.psr2_enabled && mode ==
> I915_PSR_DEBUG_FORCE_PSR1)
> - return true;
> + goto unlock;
> + }
>
> - if (!dev_priv->psr.psr2_enabled && mode !=
> I915_PSR_DEBUG_FORCE_PSR1)
> - return true;
> + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> + if (IS_ERR(state)) {
> + err = PTR_ERR(state);
> + goto unlock;
> + }
> +
> + for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> + struct intel_crtc_state *pipe_state;
> +
> + pipe_state = to_intel_crtc_state(crtc_state);
> + /* Mark mode as changed to force a modeset */
> + if (pipe_state->has_psr)
> + crtc_state->mode_changed = true;
> + }
> +
> + err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
>
> - return false;
> + drm_atomic_state_put(state);
> +unlock:
> + drm_modeset_drop_locks(&ctx);
> + drm_modeset_acquire_fini(&ctx);
> + mutex_unlock(&dev->mode_config.mutex);
> +
> + return err;
> }
>
> -int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> - struct drm_modeset_acquire_ctx *ctx,
> - u64 val)
> +int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> u64 val)
> {
> - struct drm_device *dev = &dev_priv->drm;
> - struct drm_connector_state *conn_state;
> - struct intel_crtc_state *crtc_state = NULL;
> - struct drm_crtc_commit *commit;
> - struct drm_crtc *crtc;
> - struct intel_dp *dp;
> + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> + u32 old_mode;
> int ret;
> - bool enable;
> - u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
>
> if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
> mode > I915_PSR_DEBUG_FORCE_PSR1) {
> @@ -920,49 +936,18 @@ int intel_psr_set_debugfs_mode(struct
> drm_i915_private *dev_priv,
> return -EINVAL;
> }
>
> - ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> ctx);
> - if (ret)
> - return ret;
> -
> - /* dev_priv->psr.dp should be set once and then never touched
> again. */
> - dp = READ_ONCE(dev_priv->psr.dp);
> - conn_state = dp->attached_connector->base.state;
> - crtc = conn_state->crtc;
> - if (crtc) {
> - ret = drm_modeset_lock(&crtc->mutex, ctx);
> - if (ret)
> - return ret;
> -
> - crtc_state = to_intel_crtc_state(crtc->state);
> - commit = crtc_state->base.commit;
> - } else {
> - commit = conn_state->commit;
> - }
> - if (commit) {
> - ret = wait_for_completion_interruptible(&commit-
> >hw_done);
> - if (ret)
> - return ret;
> - }
> -
> ret = mutex_lock_interruptible(&dev_priv->psr.lock);
> if (ret)
> return ret;
>
> - enable = psr_global_enabled(val);
> -
> - if (!enable || switching_psr(dev_priv, crtc_state, mode))
> - intel_psr_disable_locked(dev_priv->psr.dp);
> -
> + old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
> dev_priv->psr.debug = val;
> - if (crtc)
> - dev_priv->psr.psr2_enabled =
> intel_psr2_enabled(dev_priv, crtc_state);
>
> - intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> + mutex_unlock(&dev_priv->psr.lock);
>
> - if (dev_priv->psr.prepared && enable)
> - intel_psr_enable_locked(dev_priv, crtc_state);
> + if (old_mode != mode)
> + ret = intel_psr_modeset_force(dev_priv);
>
> - mutex_unlock(&dev_priv->psr.lock);
> return ret;
> }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
2018-12-04 1:33 ` Dhinakaran Pandiyan
@ 2018-12-04 1:54 ` Souza, Jose
2018-12-04 2:58 ` Dhinakaran Pandiyan
0 siblings, 1 reply; 15+ messages in thread
From: Souza, Jose @ 2018-12-04 1:54 UTC (permalink / raw)
To: intel-gfx, Pandiyan, Dhinakaran; +Cc: Vivi, Rodrigo
[-- Attachment #1.1: Type: text/plain, Size: 10562 bytes --]
On Mon, 2018-12-03 at 17:33 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-11-29 at 18:31 -0800, José Roberto de Souza wrote:
> > Changing the i915_edp_psr_debug was enabling, disabling or
> > switching
> > PSR version by directly calling intel_psr_disable_locked() and
> > intel_psr_enable_locked(), what is not the default PSR path that is
> > executed in a regular modesets.
> >
> > So lets force a modeset in the PSR CRTC to trigger the requested
> > PSR
> > state change and really stress the code path that matters for the
> > regular user.
> >
> > Also by doing this way it fixes the issue below, were DRRS was left
> > enabled together with PSR when enabling PSR from debugfs.
>
> While this patch does fix the issue, psr_compute_config() not
> checking
> crtc_state->has_drrs seems odd. We should change it to not set
> crtc_state->has_psr if crtc_state->has_drrs happens to be set. Or do
> it
> the other way around.
psr_compute_config() is not called when enabling PSR from debugfs, this
issue was reported when PSR1 was not enabled by default so DRRS was
being enabled by default but not PSR and when PSR was enabled from
debugfs both were kept enabled.
In the first version of this patch I was just disabling DRRS when
enabling PSR but Maarten suggested to not do so and instead stress the
default code path.
>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 14 +---
> > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > drivers/gpu/drm/i915/intel_drv.h | 4 +-
> > drivers/gpu/drm/i915/intel_psr.c | 119 ++++++++++++------------
> > ----
> > 4 files changed, 55 insertions(+), 84 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 129b9a6f8309..bc32f683dc46 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2775,7 +2775,6 @@ static int
> > i915_edp_psr_debug_set(void *data, u64 val)
> > {
> > struct drm_i915_private *dev_priv = data;
> > - struct drm_modeset_acquire_ctx ctx;
> > int ret;
> >
> > if (!CAN_PSR(dev_priv))
> > @@ -2785,18 +2784,7 @@ i915_edp_psr_debug_set(void *data, u64 val)
> >
> > intel_runtime_pm_get(dev_priv);
> >
> > - drm_modeset_acquire_init(&ctx,
> > DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> > -
> > -retry:
> > - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
> > - if (ret == -EDEADLK) {
> > - ret = drm_modeset_backoff(&ctx);
> > - if (!ret)
> > - goto retry;
> > - }
> > -
> > - drm_modeset_drop_locks(&ctx);
> > - drm_modeset_acquire_fini(&ctx);
> > + ret = intel_psr_set_debugfs_mode(dev_priv, val);
> >
> > intel_runtime_pm_put(dev_priv);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 43ac6873a2bb..93d8ca9c0375 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -493,7 +493,7 @@ struct i915_psr {
> >
> > u32 debug;
> > bool sink_support;
> > - bool prepared, enabled;
> > + bool enabled;
> > struct intel_dp *dp;
> > bool active;
> > struct work_struct work;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 86ff57738cd9..89d4eed0dd89 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -2056,9 +2056,7 @@ void intel_psr_enable(struct intel_dp
> > *intel_dp,
> > const struct intel_crtc_state *crtc_state);
> > void intel_psr_disable(struct intel_dp *intel_dp,
> > const struct intel_crtc_state *old_crtc_state);
> > -int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> > - struct drm_modeset_acquire_ctx *ctx,
> > - u64 value);
> > +int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> > u64 value);
> > void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> > unsigned frontbuffer_bits,
> > enum fb_op_origin origin);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 827b8c31783d..305848cceda7 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -52,6 +52,7 @@
> > */
> >
> > #include <drm/drmP.h>
> > +#include <drm/drm_atomic_helper.h>
> >
> > #include "intel_drv.h"
> > #include "i915_drv.h"
> > @@ -711,10 +712,6 @@ void intel_psr_enable(struct intel_dp
> > *intel_dp,
> > WARN_ON(dev_priv->drrs.dp);
> >
> > mutex_lock(&dev_priv->psr.lock);
> > - if (dev_priv->psr.prepared) {
> > - DRM_DEBUG_KMS("PSR already in use\n");
> > - goto unlock;
> > - }
> >
> > if (!psr_global_enabled(dev_priv->psr.debug)) {
> > DRM_DEBUG_KMS("PSR disabled by flag\n");
> > @@ -723,7 +720,6 @@ void intel_psr_enable(struct intel_dp
> > *intel_dp,
> >
> > dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
> > crtc_state);
> > dev_priv->psr.busy_frontbuffer_bits = 0;
> > - dev_priv->psr.prepared = true;
> > intel_psr_enable_locked(dev_priv, crtc_state);
> >
> > unlock:
> > @@ -807,14 +803,9 @@ void intel_psr_disable(struct intel_dp
> > *intel_dp,
> > return;
> >
> > mutex_lock(&dev_priv->psr.lock);
> > - if (!dev_priv->psr.prepared) {
> > - mutex_unlock(&dev_priv->psr.lock);
> > - return;
> > - }
> >
> > intel_psr_disable_locked(intel_dp);
> >
> > - dev_priv->psr.prepared = false;
> > mutex_unlock(&dev_priv->psr.lock);
> > cancel_work_sync(&dev_priv->psr.work);
> > }
> > @@ -883,36 +874,61 @@ static bool __psr_wait_for_idle_locked(struct
> > drm_i915_private *dev_priv)
> > return err == 0 && dev_priv->psr.enabled;
> > }
> >
> > -static bool switching_psr(struct drm_i915_private *dev_priv,
> > - struct intel_crtc_state *crtc_state,
> > - u32 mode)
> > +static int intel_psr_modeset_force(struct drm_i915_private
> > *dev_priv)
> > {
> > - /* Can't switch psr state anyway if PSR2 is not supported. */
> > - if (!crtc_state || !crtc_state->has_psr2)
> > - return false;
> > + struct drm_device *dev = &dev_priv->drm;
> > + struct drm_modeset_acquire_ctx ctx;
> > + struct drm_atomic_state *state;
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_crtc *crtc;
> > + int err, i;
> > +
> > + mutex_lock(&dev->mode_config.mutex);
> > + drm_modeset_acquire_init(&ctx, 0);
> > +
> > +modeset_lock_retry:
> > + err = drm_modeset_lock_all_ctx(dev, &ctx);
> > + if (err) {
> > + if (err == -EDEADLK) {
> > + err = drm_modeset_backoff(&ctx);
> > + if (!err)
> > + goto modeset_lock_retry;
> > + }
> >
> > - if (dev_priv->psr.psr2_enabled && mode ==
> > I915_PSR_DEBUG_FORCE_PSR1)
> > - return true;
> > + goto unlock;
> > + }
> >
> > - if (!dev_priv->psr.psr2_enabled && mode !=
> > I915_PSR_DEBUG_FORCE_PSR1)
> > - return true;
> > + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > + if (IS_ERR(state)) {
> > + err = PTR_ERR(state);
> > + goto unlock;
> > + }
> > +
> > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > + struct intel_crtc_state *pipe_state;
> > +
> > + pipe_state = to_intel_crtc_state(crtc_state);
> > + /* Mark mode as changed to force a modeset */
> > + if (pipe_state->has_psr)
> > + crtc_state->mode_changed = true;
> > + }
> > +
> > + err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> >
> > - return false;
> > + drm_atomic_state_put(state);
> > +unlock:
> > + drm_modeset_drop_locks(&ctx);
> > + drm_modeset_acquire_fini(&ctx);
> > + mutex_unlock(&dev->mode_config.mutex);
> > +
> > + return err;
> > }
> >
> > -int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> > - struct drm_modeset_acquire_ctx *ctx,
> > - u64 val)
> > +int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> > u64 val)
> > {
> > - struct drm_device *dev = &dev_priv->drm;
> > - struct drm_connector_state *conn_state;
> > - struct intel_crtc_state *crtc_state = NULL;
> > - struct drm_crtc_commit *commit;
> > - struct drm_crtc *crtc;
> > - struct intel_dp *dp;
> > + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > + u32 old_mode;
> > int ret;
> > - bool enable;
> > - u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> >
> > if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
> > mode > I915_PSR_DEBUG_FORCE_PSR1) {
> > @@ -920,49 +936,18 @@ int intel_psr_set_debugfs_mode(struct
> > drm_i915_private *dev_priv,
> > return -EINVAL;
> > }
> >
> > - ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> > ctx);
> > - if (ret)
> > - return ret;
> > -
> > - /* dev_priv->psr.dp should be set once and then never touched
> > again. */
> > - dp = READ_ONCE(dev_priv->psr.dp);
> > - conn_state = dp->attached_connector->base.state;
> > - crtc = conn_state->crtc;
> > - if (crtc) {
> > - ret = drm_modeset_lock(&crtc->mutex, ctx);
> > - if (ret)
> > - return ret;
> > -
> > - crtc_state = to_intel_crtc_state(crtc->state);
> > - commit = crtc_state->base.commit;
> > - } else {
> > - commit = conn_state->commit;
> > - }
> > - if (commit) {
> > - ret = wait_for_completion_interruptible(&commit-
> > > hw_done);
> > - if (ret)
> > - return ret;
> > - }
> > -
> > ret = mutex_lock_interruptible(&dev_priv->psr.lock);
> > if (ret)
> > return ret;
> >
> > - enable = psr_global_enabled(val);
> > -
> > - if (!enable || switching_psr(dev_priv, crtc_state, mode))
> > - intel_psr_disable_locked(dev_priv->psr.dp);
> > -
> > + old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
> > dev_priv->psr.debug = val;
> > - if (crtc)
> > - dev_priv->psr.psr2_enabled =
> > intel_psr2_enabled(dev_priv, crtc_state);
> >
> > - intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > + mutex_unlock(&dev_priv->psr.lock);
> >
> > - if (dev_priv->psr.prepared && enable)
> > - intel_psr_enable_locked(dev_priv, crtc_state);
> > + if (old_mode != mode)
> > + ret = intel_psr_modeset_force(dev_priv);
> >
> > - mutex_unlock(&dev_priv->psr.lock);
> > return ret;
> > }
> >
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
2018-12-04 1:54 ` Souza, Jose
@ 2018-12-04 2:58 ` Dhinakaran Pandiyan
2018-12-04 18:52 ` Souza, Jose
0 siblings, 1 reply; 15+ messages in thread
From: Dhinakaran Pandiyan @ 2018-12-04 2:58 UTC (permalink / raw)
To: Souza, Jose, intel-gfx; +Cc: Vivi, Rodrigo
On Mon, 2018-12-03 at 17:54 -0800, Souza, Jose wrote:
> On Mon, 2018-12-03 at 17:33 -0800, Dhinakaran Pandiyan wrote:
> > On Thu, 2018-11-29 at 18:31 -0800, José Roberto de Souza wrote:
> > > Changing the i915_edp_psr_debug was enabling, disabling or
> > > switching
> > > PSR version by directly calling intel_psr_disable_locked() and
> > > intel_psr_enable_locked(), what is not the default PSR path that
> > > is
> > > executed in a regular modesets.
> > >
> > > So lets force a modeset in the PSR CRTC to trigger the requested
> > > PSR
> > > state change and really stress the code path that matters for the
> > > regular user.
> > >
> > > Also by doing this way it fixes the issue below, were DRRS was
> > > left
> > > enabled together with PSR when enabling PSR from debugfs.
> >
> > While this patch does fix the issue, psr_compute_config() not
> > checking
> > crtc_state->has_drrs seems odd. We should change it to not set
> > crtc_state->has_psr if crtc_state->has_drrs happens to be set. Or
> > do
> > it
> > the other way around.
>
> psr_compute_config() is not called when enabling PSR from debugfs,
> this
Right. My suggestion is to allow either ->has_drrs or ->has_psr being
set (not both) in the kernel and disable DRRS in the IGT before
starting the test.
> issue was reported when PSR1 was not enabled by default so DRRS was
> being enabled by default but not PSR and when PSR was enabled from
> debugfs both were kept enabled.
>
> In the first version of this patch I was just disabling DRRS when
> enabling PSR but Maarten suggested to not do so and instead stress
> the
> default code path.
This patch changes crtc_state->mode_changed to force a modeset, which
means it is not exactly the same as the 'default' code path.
>
> >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_debugfs.c | 14 +---
> > > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > > drivers/gpu/drm/i915/intel_drv.h | 4 +-
> > > drivers/gpu/drm/i915/intel_psr.c | 119 ++++++++++++--------
> > > ----
> > > ----
> > > 4 files changed, 55 insertions(+), 84 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 129b9a6f8309..bc32f683dc46 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2775,7 +2775,6 @@ static int
> > > i915_edp_psr_debug_set(void *data, u64 val)
> > > {
> > > struct drm_i915_private *dev_priv = data;
> > > - struct drm_modeset_acquire_ctx ctx;
> > > int ret;
> > >
> > > if (!CAN_PSR(dev_priv))
> > > @@ -2785,18 +2784,7 @@ i915_edp_psr_debug_set(void *data, u64
> > > val)
> > >
> > > intel_runtime_pm_get(dev_priv);
> > >
> > > - drm_modeset_acquire_init(&ctx,
> > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> > > -
> > > -retry:
> > > - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
> > > - if (ret == -EDEADLK) {
> > > - ret = drm_modeset_backoff(&ctx);
> > > - if (!ret)
> > > - goto retry;
> > > - }
> > > -
> > > - drm_modeset_drop_locks(&ctx);
> > > - drm_modeset_acquire_fini(&ctx);
> > > + ret = intel_psr_set_debugfs_mode(dev_priv, val);
> > >
> > > intel_runtime_pm_put(dev_priv);
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 43ac6873a2bb..93d8ca9c0375 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -493,7 +493,7 @@ struct i915_psr {
> > >
> > > u32 debug;
> > > bool sink_support;
> > > - bool prepared, enabled;
> > > + bool enabled;
> > > struct intel_dp *dp;
> > > bool active;
> > > struct work_struct work;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 86ff57738cd9..89d4eed0dd89 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -2056,9 +2056,7 @@ void intel_psr_enable(struct intel_dp
> > > *intel_dp,
> > > const struct intel_crtc_state *crtc_state);
> > > void intel_psr_disable(struct intel_dp *intel_dp,
> > > const struct intel_crtc_state *old_crtc_state);
> > > -int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > *dev_priv,
> > > - struct drm_modeset_acquire_ctx *ctx,
> > > - u64 value);
> > > +int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > *dev_priv,
> > > u64 value);
> > > void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> > > unsigned frontbuffer_bits,
> > > enum fb_op_origin origin);
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 827b8c31783d..305848cceda7 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -52,6 +52,7 @@
> > > */
> > >
> > > #include <drm/drmP.h>
> > > +#include <drm/drm_atomic_helper.h>
> > >
> > > #include "intel_drv.h"
> > > #include "i915_drv.h"
> > > @@ -711,10 +712,6 @@ void intel_psr_enable(struct intel_dp
> > > *intel_dp,
> > > WARN_ON(dev_priv->drrs.dp);
> > >
> > > mutex_lock(&dev_priv->psr.lock);
> > > - if (dev_priv->psr.prepared) {
> > > - DRM_DEBUG_KMS("PSR already in use\n");
> > > - goto unlock;
> > > - }
> > >
> > > if (!psr_global_enabled(dev_priv->psr.debug)) {
> > > DRM_DEBUG_KMS("PSR disabled by flag\n");
> > > @@ -723,7 +720,6 @@ void intel_psr_enable(struct intel_dp
> > > *intel_dp,
> > >
> > > dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
> > > crtc_state);
> > > dev_priv->psr.busy_frontbuffer_bits = 0;
> > > - dev_priv->psr.prepared = true;
> > > intel_psr_enable_locked(dev_priv, crtc_state);
> > >
> > > unlock:
> > > @@ -807,14 +803,9 @@ void intel_psr_disable(struct intel_dp
> > > *intel_dp,
> > > return;
> > >
> > > mutex_lock(&dev_priv->psr.lock);
> > > - if (!dev_priv->psr.prepared) {
> > > - mutex_unlock(&dev_priv->psr.lock);
> > > - return;
> > > - }
> > >
> > > intel_psr_disable_locked(intel_dp);
> > >
> > > - dev_priv->psr.prepared = false;
> > > mutex_unlock(&dev_priv->psr.lock);
> > > cancel_work_sync(&dev_priv->psr.work);
> > > }
> > > @@ -883,36 +874,61 @@ static bool
> > > __psr_wait_for_idle_locked(struct
> > > drm_i915_private *dev_priv)
> > > return err == 0 && dev_priv->psr.enabled;
> > > }
> > >
> > > -static bool switching_psr(struct drm_i915_private *dev_priv,
> > > - struct intel_crtc_state *crtc_state,
> > > - u32 mode)
> > > +static int intel_psr_modeset_force(struct drm_i915_private
> > > *dev_priv)
> > > {
> > > - /* Can't switch psr state anyway if PSR2 is not supported. */
> > > - if (!crtc_state || !crtc_state->has_psr2)
> > > - return false;
> > > + struct drm_device *dev = &dev_priv->drm;
> > > + struct drm_modeset_acquire_ctx ctx;
> > > + struct drm_atomic_state *state;
> > > + struct drm_crtc_state *crtc_state;
> > > + struct drm_crtc *crtc;
> > > + int err, i;
> > > +
> > > + mutex_lock(&dev->mode_config.mutex);
> > > + drm_modeset_acquire_init(&ctx, 0);
> > > +
> > > +modeset_lock_retry:
> > > + err = drm_modeset_lock_all_ctx(dev, &ctx);
> > > + if (err) {
> > > + if (err == -EDEADLK) {
> > > + err = drm_modeset_backoff(&ctx);
> > > + if (!err)
> > > + goto modeset_lock_retry;
> > > + }
> > >
> > > - if (dev_priv->psr.psr2_enabled && mode ==
> > > I915_PSR_DEBUG_FORCE_PSR1)
> > > - return true;
> > > + goto unlock;
> > > + }
> > >
> > > - if (!dev_priv->psr.psr2_enabled && mode !=
> > > I915_PSR_DEBUG_FORCE_PSR1)
> > > - return true;
> > > + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > + if (IS_ERR(state)) {
> > > + err = PTR_ERR(state);
> > > + goto unlock;
> > > + }
> > > +
> > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > > + struct intel_crtc_state *pipe_state;
> > > +
> > > + pipe_state = to_intel_crtc_state(crtc_state);
> > > + /* Mark mode as changed to force a modeset */
> > > + if (pipe_state->has_psr)
> > > + crtc_state->mode_changed = true;
> > > + }
> > > +
> > > + err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> > >
> > > - return false;
> > > + drm_atomic_state_put(state);
> > > +unlock:
> > > + drm_modeset_drop_locks(&ctx);
> > > + drm_modeset_acquire_fini(&ctx);
> > > + mutex_unlock(&dev->mode_config.mutex);
> > > +
> > > + return err;
> > > }
> > >
> > > -int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > *dev_priv,
> > > - struct drm_modeset_acquire_ctx *ctx,
> > > - u64 val)
> > > +int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > *dev_priv,
> > > u64 val)
> > > {
> > > - struct drm_device *dev = &dev_priv->drm;
> > > - struct drm_connector_state *conn_state;
> > > - struct intel_crtc_state *crtc_state = NULL;
> > > - struct drm_crtc_commit *commit;
> > > - struct drm_crtc *crtc;
> > > - struct intel_dp *dp;
> > > + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > > + u32 old_mode;
> > > int ret;
> > > - bool enable;
> > > - u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > >
> > > if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
> > > mode > I915_PSR_DEBUG_FORCE_PSR1) {
> > > @@ -920,49 +936,18 @@ int intel_psr_set_debugfs_mode(struct
> > > drm_i915_private *dev_priv,
> > > return -EINVAL;
> > > }
> > >
> > > - ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> > > ctx);
> > > - if (ret)
> > > - return ret;
> > > -
> > > - /* dev_priv->psr.dp should be set once and then never touched
> > > again. */
> > > - dp = READ_ONCE(dev_priv->psr.dp);
> > > - conn_state = dp->attached_connector->base.state;
> > > - crtc = conn_state->crtc;
> > > - if (crtc) {
> > > - ret = drm_modeset_lock(&crtc->mutex, ctx);
> > > - if (ret)
> > > - return ret;
> > > -
> > > - crtc_state = to_intel_crtc_state(crtc->state);
> > > - commit = crtc_state->base.commit;
> > > - } else {
> > > - commit = conn_state->commit;
> > > - }
> > > - if (commit) {
> > > - ret = wait_for_completion_interruptible(&commit-
> > > > hw_done);
> > >
> > > - if (ret)
> > > - return ret;
> > > - }
> > > -
> > > ret = mutex_lock_interruptible(&dev_priv->psr.lock);
> > > if (ret)
> > > return ret;
> > >
> > > - enable = psr_global_enabled(val);
> > > -
> > > - if (!enable || switching_psr(dev_priv, crtc_state, mode))
> > > - intel_psr_disable_locked(dev_priv->psr.dp);
> > > -
> > > + old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
> > > dev_priv->psr.debug = val;
> > > - if (crtc)
> > > - dev_priv->psr.psr2_enabled =
> > > intel_psr2_enabled(dev_priv, crtc_state);
> > >
> > > - intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > + mutex_unlock(&dev_priv->psr.lock);
> > >
> > > - if (dev_priv->psr.prepared && enable)
> > > - intel_psr_enable_locked(dev_priv, crtc_state);
> > > + if (old_mode != mode)
> > > + ret = intel_psr_modeset_force(dev_priv);
> > >
> > > - mutex_unlock(&dev_priv->psr.lock);
> > > return ret;
> > > }
> > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
2018-12-04 2:58 ` Dhinakaran Pandiyan
@ 2018-12-04 18:52 ` Souza, Jose
2018-12-04 21:23 ` Dhinakaran Pandiyan
0 siblings, 1 reply; 15+ messages in thread
From: Souza, Jose @ 2018-12-04 18:52 UTC (permalink / raw)
To: intel-gfx, Pandiyan, Dhinakaran; +Cc: Vivi, Rodrigo
[-- Attachment #1.1: Type: text/plain, Size: 12887 bytes --]
On Mon, 2018-12-03 at 18:58 -0800, Dhinakaran Pandiyan wrote:
> On Mon, 2018-12-03 at 17:54 -0800, Souza, Jose wrote:
> > On Mon, 2018-12-03 at 17:33 -0800, Dhinakaran Pandiyan wrote:
> > > On Thu, 2018-11-29 at 18:31 -0800, José Roberto de Souza wrote:
> > > > Changing the i915_edp_psr_debug was enabling, disabling or
> > > > switching
> > > > PSR version by directly calling intel_psr_disable_locked() and
> > > > intel_psr_enable_locked(), what is not the default PSR path
> > > > that
> > > > is
> > > > executed in a regular modesets.
> > > >
> > > > So lets force a modeset in the PSR CRTC to trigger the
> > > > requested
> > > > PSR
> > > > state change and really stress the code path that matters for
> > > > the
> > > > regular user.
> > > >
> > > > Also by doing this way it fixes the issue below, were DRRS was
> > > > left
> > > > enabled together with PSR when enabling PSR from debugfs.
> > >
> > > While this patch does fix the issue, psr_compute_config() not
> > > checking
> > > crtc_state->has_drrs seems odd. We should change it to not set
> > > crtc_state->has_psr if crtc_state->has_drrs happens to be set. Or
> > > do
> > > it
> > > the other way around.
> >
> > psr_compute_config() is not called when enabling PSR from debugfs,
> > this
> Right. My suggestion is to allow either ->has_drrs or ->has_psr being
> set (not both) in the kernel and disable DRRS in the IGT before
> starting the test.
So in case were PSR is disabled by parameter and DRRS is supported we
would not enable DRRS? Because has_psr is set even if PSR is disabled.
Disabling DRRS from IGT is duplicating the code that already do that
and also not validating the default code path.
>
>
> > issue was reported when PSR1 was not enabled by default so DRRS was
> > being enabled by default but not PSR and when PSR was enabled from
> > debugfs both were kept enabled.
> >
> > In the first version of this patch I was just disabling DRRS when
> > enabling PSR but Maarten suggested to not do so and instead stress
> > the
> > default code path.
>
> This patch changes crtc_state->mode_changed to force a modeset, which
> means it is not exactly the same as the 'default' code path.
Some drm and i915 code paths also set mode_changed and the result of
this is exactly the same as do a full modeset.
>
>
>
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_debugfs.c | 14 +---
> > > > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > > > drivers/gpu/drm/i915/intel_drv.h | 4 +-
> > > > drivers/gpu/drm/i915/intel_psr.c | 119 ++++++++++++--------
> > > > ----
> > > > ----
> > > > 4 files changed, 55 insertions(+), 84 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index 129b9a6f8309..bc32f683dc46 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -2775,7 +2775,6 @@ static int
> > > > i915_edp_psr_debug_set(void *data, u64 val)
> > > > {
> > > > struct drm_i915_private *dev_priv = data;
> > > > - struct drm_modeset_acquire_ctx ctx;
> > > > int ret;
> > > >
> > > > if (!CAN_PSR(dev_priv))
> > > > @@ -2785,18 +2784,7 @@ i915_edp_psr_debug_set(void *data, u64
> > > > val)
> > > >
> > > > intel_runtime_pm_get(dev_priv);
> > > >
> > > > - drm_modeset_acquire_init(&ctx,
> > > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> > > > -
> > > > -retry:
> > > > - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
> > > > - if (ret == -EDEADLK) {
> > > > - ret = drm_modeset_backoff(&ctx);
> > > > - if (!ret)
> > > > - goto retry;
> > > > - }
> > > > -
> > > > - drm_modeset_drop_locks(&ctx);
> > > > - drm_modeset_acquire_fini(&ctx);
> > > > + ret = intel_psr_set_debugfs_mode(dev_priv, val);
> > > >
> > > > intel_runtime_pm_put(dev_priv);
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 43ac6873a2bb..93d8ca9c0375 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -493,7 +493,7 @@ struct i915_psr {
> > > >
> > > > u32 debug;
> > > > bool sink_support;
> > > > - bool prepared, enabled;
> > > > + bool enabled;
> > > > struct intel_dp *dp;
> > > > bool active;
> > > > struct work_struct work;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 86ff57738cd9..89d4eed0dd89 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -2056,9 +2056,7 @@ void intel_psr_enable(struct intel_dp
> > > > *intel_dp,
> > > > const struct intel_crtc_state
> > > > *crtc_state);
> > > > void intel_psr_disable(struct intel_dp *intel_dp,
> > > > const struct intel_crtc_state
> > > > *old_crtc_state);
> > > > -int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > *dev_priv,
> > > > - struct drm_modeset_acquire_ctx
> > > > *ctx,
> > > > - u64 value);
> > > > +int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > *dev_priv,
> > > > u64 value);
> > > > void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> > > > unsigned frontbuffer_bits,
> > > > enum fb_op_origin origin);
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 827b8c31783d..305848cceda7 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -52,6 +52,7 @@
> > > > */
> > > >
> > > > #include <drm/drmP.h>
> > > > +#include <drm/drm_atomic_helper.h>
> > > >
> > > > #include "intel_drv.h"
> > > > #include "i915_drv.h"
> > > > @@ -711,10 +712,6 @@ void intel_psr_enable(struct intel_dp
> > > > *intel_dp,
> > > > WARN_ON(dev_priv->drrs.dp);
> > > >
> > > > mutex_lock(&dev_priv->psr.lock);
> > > > - if (dev_priv->psr.prepared) {
> > > > - DRM_DEBUG_KMS("PSR already in use\n");
> > > > - goto unlock;
> > > > - }
> > > >
> > > > if (!psr_global_enabled(dev_priv->psr.debug)) {
> > > > DRM_DEBUG_KMS("PSR disabled by flag\n");
> > > > @@ -723,7 +720,6 @@ void intel_psr_enable(struct intel_dp
> > > > *intel_dp,
> > > >
> > > > dev_priv->psr.psr2_enabled =
> > > > intel_psr2_enabled(dev_priv,
> > > > crtc_state);
> > > > dev_priv->psr.busy_frontbuffer_bits = 0;
> > > > - dev_priv->psr.prepared = true;
> > > > intel_psr_enable_locked(dev_priv, crtc_state);
> > > >
> > > > unlock:
> > > > @@ -807,14 +803,9 @@ void intel_psr_disable(struct intel_dp
> > > > *intel_dp,
> > > > return;
> > > >
> > > > mutex_lock(&dev_priv->psr.lock);
> > > > - if (!dev_priv->psr.prepared) {
> > > > - mutex_unlock(&dev_priv->psr.lock);
> > > > - return;
> > > > - }
> > > >
> > > > intel_psr_disable_locked(intel_dp);
> > > >
> > > > - dev_priv->psr.prepared = false;
> > > > mutex_unlock(&dev_priv->psr.lock);
> > > > cancel_work_sync(&dev_priv->psr.work);
> > > > }
> > > > @@ -883,36 +874,61 @@ static bool
> > > > __psr_wait_for_idle_locked(struct
> > > > drm_i915_private *dev_priv)
> > > > return err == 0 && dev_priv->psr.enabled;
> > > > }
> > > >
> > > > -static bool switching_psr(struct drm_i915_private *dev_priv,
> > > > - struct intel_crtc_state *crtc_state,
> > > > - u32 mode)
> > > > +static int intel_psr_modeset_force(struct drm_i915_private
> > > > *dev_priv)
> > > > {
> > > > - /* Can't switch psr state anyway if PSR2 is not
> > > > supported. */
> > > > - if (!crtc_state || !crtc_state->has_psr2)
> > > > - return false;
> > > > + struct drm_device *dev = &dev_priv->drm;
> > > > + struct drm_modeset_acquire_ctx ctx;
> > > > + struct drm_atomic_state *state;
> > > > + struct drm_crtc_state *crtc_state;
> > > > + struct drm_crtc *crtc;
> > > > + int err, i;
> > > > +
> > > > + mutex_lock(&dev->mode_config.mutex);
> > > > + drm_modeset_acquire_init(&ctx, 0);
> > > > +
> > > > +modeset_lock_retry:
> > > > + err = drm_modeset_lock_all_ctx(dev, &ctx);
> > > > + if (err) {
> > > > + if (err == -EDEADLK) {
> > > > + err = drm_modeset_backoff(&ctx);
> > > > + if (!err)
> > > > + goto modeset_lock_retry;
> > > > + }
> > > >
> > > > - if (dev_priv->psr.psr2_enabled && mode ==
> > > > I915_PSR_DEBUG_FORCE_PSR1)
> > > > - return true;
> > > > + goto unlock;
> > > > + }
> > > >
> > > > - if (!dev_priv->psr.psr2_enabled && mode !=
> > > > I915_PSR_DEBUG_FORCE_PSR1)
> > > > - return true;
> > > > + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > + if (IS_ERR(state)) {
> > > > + err = PTR_ERR(state);
> > > > + goto unlock;
> > > > + }
> > > > +
> > > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> > > > {
> > > > + struct intel_crtc_state *pipe_state;
> > > > +
> > > > + pipe_state = to_intel_crtc_state(crtc_state);
> > > > + /* Mark mode as changed to force a modeset */
> > > > + if (pipe_state->has_psr)
> > > > + crtc_state->mode_changed = true;
> > > > + }
> > > > +
> > > > + err = drm_atomic_helper_commit_duplicated_state(state,
> > > > &ctx);
> > > >
> > > > - return false;
> > > > + drm_atomic_state_put(state);
> > > > +unlock:
> > > > + drm_modeset_drop_locks(&ctx);
> > > > + drm_modeset_acquire_fini(&ctx);
> > > > + mutex_unlock(&dev->mode_config.mutex);
> > > > +
> > > > + return err;
> > > > }
> > > >
> > > > -int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > *dev_priv,
> > > > - struct drm_modeset_acquire_ctx
> > > > *ctx,
> > > > - u64 val)
> > > > +int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > *dev_priv,
> > > > u64 val)
> > > > {
> > > > - struct drm_device *dev = &dev_priv->drm;
> > > > - struct drm_connector_state *conn_state;
> > > > - struct intel_crtc_state *crtc_state = NULL;
> > > > - struct drm_crtc_commit *commit;
> > > > - struct drm_crtc *crtc;
> > > > - struct intel_dp *dp;
> > > > + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > > > + u32 old_mode;
> > > > int ret;
> > > > - bool enable;
> > > > - u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > > >
> > > > if (val & ~(I915_PSR_DEBUG_IRQ |
> > > > I915_PSR_DEBUG_MODE_MASK) ||
> > > > mode > I915_PSR_DEBUG_FORCE_PSR1) {
> > > > @@ -920,49 +936,18 @@ int intel_psr_set_debugfs_mode(struct
> > > > drm_i915_private *dev_priv,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > - ret = drm_modeset_lock(&dev-
> > > > >mode_config.connection_mutex,
> > > > ctx);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > - /* dev_priv->psr.dp should be set once and then never
> > > > touched
> > > > again. */
> > > > - dp = READ_ONCE(dev_priv->psr.dp);
> > > > - conn_state = dp->attached_connector->base.state;
> > > > - crtc = conn_state->crtc;
> > > > - if (crtc) {
> > > > - ret = drm_modeset_lock(&crtc->mutex, ctx);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > - crtc_state = to_intel_crtc_state(crtc->state);
> > > > - commit = crtc_state->base.commit;
> > > > - } else {
> > > > - commit = conn_state->commit;
> > > > - }
> > > > - if (commit) {
> > > > - ret =
> > > > wait_for_completion_interruptible(&commit-
> > > > > hw_done);
> > > >
> > > > - if (ret)
> > > > - return ret;
> > > > - }
> > > > -
> > > > ret = mutex_lock_interruptible(&dev_priv->psr.lock);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - enable = psr_global_enabled(val);
> > > > -
> > > > - if (!enable || switching_psr(dev_priv, crtc_state,
> > > > mode))
> > > > - intel_psr_disable_locked(dev_priv->psr.dp);
> > > > -
> > > > + old_mode = dev_priv->psr.debug &
> > > > I915_PSR_DEBUG_MODE_MASK;
> > > > dev_priv->psr.debug = val;
> > > > - if (crtc)
> > > > - dev_priv->psr.psr2_enabled =
> > > > intel_psr2_enabled(dev_priv, crtc_state);
> > > >
> > > > - intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > > + mutex_unlock(&dev_priv->psr.lock);
> > > >
> > > > - if (dev_priv->psr.prepared && enable)
> > > > - intel_psr_enable_locked(dev_priv, crtc_state);
> > > > + if (old_mode != mode)
> > > > + ret = intel_psr_modeset_force(dev_priv);
> > > >
> > > > - mutex_unlock(&dev_priv->psr.lock);
> > > > return ret;
> > > > }
> > > >
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
2018-12-04 18:52 ` Souza, Jose
@ 2018-12-04 21:23 ` Dhinakaran Pandiyan
2018-12-11 18:16 ` Souza, Jose
0 siblings, 1 reply; 15+ messages in thread
From: Dhinakaran Pandiyan @ 2018-12-04 21:23 UTC (permalink / raw)
To: Souza, Jose, intel-gfx; +Cc: Vivi, Rodrigo
On Tue, 2018-12-04 at 10:52 -0800, Souza, Jose wrote:
> On Mon, 2018-12-03 at 18:58 -0800, Dhinakaran Pandiyan wrote:
> > On Mon, 2018-12-03 at 17:54 -0800, Souza, Jose wrote:
> > > On Mon, 2018-12-03 at 17:33 -0800, Dhinakaran Pandiyan wrote:
> > > > On Thu, 2018-11-29 at 18:31 -0800, José Roberto de Souza wrote:
> > > > > Changing the i915_edp_psr_debug was enabling, disabling or
> > > > > switching
> > > > > PSR version by directly calling intel_psr_disable_locked()
> > > > > and
> > > > > intel_psr_enable_locked(), what is not the default PSR path
> > > > > that
> > > > > is
> > > > > executed in a regular modesets.
> > > > >
> > > > > So lets force a modeset in the PSR CRTC to trigger the
> > > > > requested
> > > > > PSR
> > > > > state change and really stress the code path that matters for
> > > > > the
> > > > > regular user.
> > > > >
> > > > > Also by doing this way it fixes the issue below, were DRRS
> > > > > was
> > > > > left
> > > > > enabled together with PSR when enabling PSR from debugfs.
> > > >
> > > > While this patch does fix the issue, psr_compute_config() not
> > > > checking
> > > > crtc_state->has_drrs seems odd. We should change it to not set
> > > > crtc_state->has_psr if crtc_state->has_drrs happens to be set.
> > > > Or
> > > > do
> > > > it
> > > > the other way around.
> > >
> > > psr_compute_config() is not called when enabling PSR from
> > > debugfs,
> > > this
> >
> > Right. My suggestion is to allow either ->has_drrs or ->has_psr
> > being
> > set (not both) in the kernel and disable DRRS in the IGT before
> > starting the test.
>
> So in case were PSR is disabled by parameter and DRRS is supported we
> would not enable DRRS? Because has_psr is set even if PSR is
> disabled.
Set ->has_psr = true in psr_compute_config() only if the module
parameter and debugfs mode allow it. That is how the code worked
earlier. Given that this patch duplicates the atomic state and runs
through all state checks, we can move back to the earlier way of
completing all checks in psr_compute_config().
>
> Disabling DRRS from IGT is duplicating the code that already do that
> and also not validating the default code path.
Call drrs_compute_config() after psr_compute_config(), don't set
has_drrs if has_psr is set.
>
> >
> >
> > > issue was reported when PSR1 was not enabled by default so DRRS
> > > was
> > > being enabled by default but not PSR and when PSR was enabled
> > > from
> > > debugfs both were kept enabled.
> > >
> > > In the first version of this patch I was just disabling DRRS when
> > > enabling PSR but Maarten suggested to not do so and instead
> > > stress
> > > the
> > > default code path.
> >
> > This patch changes crtc_state->mode_changed to force a modeset,
> > which
> > means it is not exactly the same as the 'default' code path.
>
> Some drm and i915 code paths also set mode_changed and the result of
> this is exactly the same as do a full modeset.
>
> >
> >
> >
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/i915_debugfs.c | 14 +---
> > > > > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > > > > drivers/gpu/drm/i915/intel_drv.h | 4 +-
> > > > > drivers/gpu/drm/i915/intel_psr.c | 119 ++++++++++++----
> > > > > ----
> > > > > ----
> > > > > ----
> > > > > 4 files changed, 55 insertions(+), 84 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > index 129b9a6f8309..bc32f683dc46 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > @@ -2775,7 +2775,6 @@ static int
> > > > > i915_edp_psr_debug_set(void *data, u64 val)
> > > > > {
> > > > > struct drm_i915_private *dev_priv = data;
> > > > > - struct drm_modeset_acquire_ctx ctx;
> > > > > int ret;
> > > > >
> > > > > if (!CAN_PSR(dev_priv))
> > > > > @@ -2785,18 +2784,7 @@ i915_edp_psr_debug_set(void *data, u64
> > > > > val)
> > > > >
> > > > > intel_runtime_pm_get(dev_priv);
> > > > >
> > > > > - drm_modeset_acquire_init(&ctx,
> > > > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> > > > > -
> > > > > -retry:
> > > > > - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
> > > > > - if (ret == -EDEADLK) {
> > > > > - ret = drm_modeset_backoff(&ctx);
> > > > > - if (!ret)
> > > > > - goto retry;
> > > > > - }
> > > > > -
> > > > > - drm_modeset_drop_locks(&ctx);
> > > > > - drm_modeset_acquire_fini(&ctx);
> > > > > + ret = intel_psr_set_debugfs_mode(dev_priv, val);
> > > > >
> > > > > intel_runtime_pm_put(dev_priv);
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 43ac6873a2bb..93d8ca9c0375 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -493,7 +493,7 @@ struct i915_psr {
> > > > >
> > > > > u32 debug;
> > > > > bool sink_support;
> > > > > - bool prepared, enabled;
> > > > > + bool enabled;
> > > > > struct intel_dp *dp;
> > > > > bool active;
> > > > > struct work_struct work;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 86ff57738cd9..89d4eed0dd89 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -2056,9 +2056,7 @@ void intel_psr_enable(struct intel_dp
> > > > > *intel_dp,
> > > > > const struct intel_crtc_state
> > > > > *crtc_state);
> > > > > void intel_psr_disable(struct intel_dp *intel_dp,
> > > > > const struct intel_crtc_state
> > > > > *old_crtc_state);
> > > > > -int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > > *dev_priv,
> > > > > - struct drm_modeset_acquire_ctx
> > > > > *ctx,
> > > > > - u64 value);
> > > > > +int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > > *dev_priv,
> > > > > u64 value);
> > > > > void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> > > > > unsigned frontbuffer_bits,
> > > > > enum fb_op_origin origin);
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index 827b8c31783d..305848cceda7 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -52,6 +52,7 @@
> > > > > */
> > > > >
> > > > > #include <drm/drmP.h>
> > > > > +#include <drm/drm_atomic_helper.h>
> > > > >
> > > > > #include "intel_drv.h"
> > > > > #include "i915_drv.h"
> > > > > @@ -711,10 +712,6 @@ void intel_psr_enable(struct intel_dp
> > > > > *intel_dp,
> > > > > WARN_ON(dev_priv->drrs.dp);
> > > > >
> > > > > mutex_lock(&dev_priv->psr.lock);
> > > > > - if (dev_priv->psr.prepared) {
> > > > > - DRM_DEBUG_KMS("PSR already in use\n");
> > > > > - goto unlock;
> > > > > - }
> > > > >
> > > > > if (!psr_global_enabled(dev_priv->psr.debug)) {
> > > > > DRM_DEBUG_KMS("PSR disabled by flag\n");
> > > > > @@ -723,7 +720,6 @@ void intel_psr_enable(struct intel_dp
> > > > > *intel_dp,
> > > > >
> > > > > dev_priv->psr.psr2_enabled =
> > > > > intel_psr2_enabled(dev_priv,
> > > > > crtc_state);
> > > > > dev_priv->psr.busy_frontbuffer_bits = 0;
> > > > > - dev_priv->psr.prepared = true;
> > > > > intel_psr_enable_locked(dev_priv, crtc_state);
> > > > >
> > > > > unlock:
> > > > > @@ -807,14 +803,9 @@ void intel_psr_disable(struct intel_dp
> > > > > *intel_dp,
> > > > > return;
> > > > >
> > > > > mutex_lock(&dev_priv->psr.lock);
> > > > > - if (!dev_priv->psr.prepared) {
> > > > > - mutex_unlock(&dev_priv->psr.lock);
> > > > > - return;
> > > > > - }
> > > > >
> > > > > intel_psr_disable_locked(intel_dp);
> > > > >
> > > > > - dev_priv->psr.prepared = false;
> > > > > mutex_unlock(&dev_priv->psr.lock);
> > > > > cancel_work_sync(&dev_priv->psr.work);
> > > > > }
> > > > > @@ -883,36 +874,61 @@ static bool
> > > > > __psr_wait_for_idle_locked(struct
> > > > > drm_i915_private *dev_priv)
> > > > > return err == 0 && dev_priv->psr.enabled;
> > > > > }
> > > > >
> > > > > -static bool switching_psr(struct drm_i915_private *dev_priv,
> > > > > - struct intel_crtc_state *crtc_state,
> > > > > - u32 mode)
> > > > > +static int intel_psr_modeset_force(struct drm_i915_private
> > > > > *dev_priv)
> > > > > {
> > > > > - /* Can't switch psr state anyway if PSR2 is not
> > > > > supported. */
> > > > > - if (!crtc_state || !crtc_state->has_psr2)
> > > > > - return false;
> > > > > + struct drm_device *dev = &dev_priv->drm;
> > > > > + struct drm_modeset_acquire_ctx ctx;
> > > > > + struct drm_atomic_state *state;
> > > > > + struct drm_crtc_state *crtc_state;
> > > > > + struct drm_crtc *crtc;
> > > > > + int err, i;
> > > > > +
> > > > > + mutex_lock(&dev->mode_config.mutex);
> > > > > + drm_modeset_acquire_init(&ctx, 0);
> > > > > +
> > > > > +modeset_lock_retry:
> > > > > + err = drm_modeset_lock_all_ctx(dev, &ctx);
> > > > > + if (err) {
> > > > > + if (err == -EDEADLK) {
> > > > > + err = drm_modeset_backoff(&ctx);
> > > > > + if (!err)
> > > > > + goto modeset_lock_retry;
> > > > > + }
> > > > >
> > > > > - if (dev_priv->psr.psr2_enabled && mode ==
> > > > > I915_PSR_DEBUG_FORCE_PSR1)
> > > > > - return true;
> > > > > + goto unlock;
> > > > > + }
> > > > >
> > > > > - if (!dev_priv->psr.psr2_enabled && mode !=
> > > > > I915_PSR_DEBUG_FORCE_PSR1)
> > > > > - return true;
> > > > > + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > > + if (IS_ERR(state)) {
> > > > > + err = PTR_ERR(state);
> > > > > + goto unlock;
> > > > > + }
> > > > > +
> > > > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> > > > > {
> > > > > + struct intel_crtc_state *pipe_state;
> > > > > +
> > > > > + pipe_state = to_intel_crtc_state(crtc_state);
> > > > > + /* Mark mode as changed to force a modeset */
> > > > > + if (pipe_state->has_psr)
> > > > > + crtc_state->mode_changed = true;
> > > > > + }
> > > > > +
> > > > > + err = drm_atomic_helper_commit_duplicated_state(state,
> > > > > &ctx);
> > > > >
> > > > > - return false;
> > > > > + drm_atomic_state_put(state);
> > > > > +unlock:
> > > > > + drm_modeset_drop_locks(&ctx);
> > > > > + drm_modeset_acquire_fini(&ctx);
> > > > > + mutex_unlock(&dev->mode_config.mutex);
> > > > > +
> > > > > + return err;
> > > > > }
> > > > >
> > > > > -int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > > *dev_priv,
> > > > > - struct drm_modeset_acquire_ctx
> > > > > *ctx,
> > > > > - u64 val)
> > > > > +int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > > *dev_priv,
> > > > > u64 val)
> > > > > {
> > > > > - struct drm_device *dev = &dev_priv->drm;
> > > > > - struct drm_connector_state *conn_state;
> > > > > - struct intel_crtc_state *crtc_state = NULL;
> > > > > - struct drm_crtc_commit *commit;
> > > > > - struct drm_crtc *crtc;
> > > > > - struct intel_dp *dp;
> > > > > + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > > > > + u32 old_mode;
> > > > > int ret;
> > > > > - bool enable;
> > > > > - u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > > > >
> > > > > if (val & ~(I915_PSR_DEBUG_IRQ |
> > > > > I915_PSR_DEBUG_MODE_MASK) ||
> > > > > mode > I915_PSR_DEBUG_FORCE_PSR1) {
> > > > > @@ -920,49 +936,18 @@ int intel_psr_set_debugfs_mode(struct
> > > > > drm_i915_private *dev_priv,
> > > > > return -EINVAL;
> > > > > }
> > > > >
> > > > > - ret = drm_modeset_lock(&dev-
> > > > > > mode_config.connection_mutex,
> > > > >
> > > > > ctx);
> > > > > - if (ret)
> > > > > - return ret;
> > > > > -
> > > > > - /* dev_priv->psr.dp should be set once and then never
> > > > > touched
> > > > > again. */
> > > > > - dp = READ_ONCE(dev_priv->psr.dp);
> > > > > - conn_state = dp->attached_connector->base.state;
> > > > > - crtc = conn_state->crtc;
> > > > > - if (crtc) {
> > > > > - ret = drm_modeset_lock(&crtc->mutex, ctx);
> > > > > - if (ret)
> > > > > - return ret;
> > > > > -
> > > > > - crtc_state = to_intel_crtc_state(crtc->state);
> > > > > - commit = crtc_state->base.commit;
> > > > > - } else {
> > > > > - commit = conn_state->commit;
> > > > > - }
> > > > > - if (commit) {
> > > > > - ret =
> > > > > wait_for_completion_interruptible(&commit-
> > > > > > hw_done);
> > > > >
> > > > > - if (ret)
> > > > > - return ret;
> > > > > - }
> > > > > -
> > > > > ret = mutex_lock_interruptible(&dev_priv->psr.lock);
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > - enable = psr_global_enabled(val);
> > > > > -
> > > > > - if (!enable || switching_psr(dev_priv, crtc_state,
> > > > > mode))
> > > > > - intel_psr_disable_locked(dev_priv->psr.dp);
> > > > > -
> > > > > + old_mode = dev_priv->psr.debug &
> > > > > I915_PSR_DEBUG_MODE_MASK;
> > > > > dev_priv->psr.debug = val;
> > > > > - if (crtc)
> > > > > - dev_priv->psr.psr2_enabled =
> > > > > intel_psr2_enabled(dev_priv, crtc_state);
> > > > >
> > > > > - intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > > > + mutex_unlock(&dev_priv->psr.lock);
> > > > >
> > > > > - if (dev_priv->psr.prepared && enable)
> > > > > - intel_psr_enable_locked(dev_priv, crtc_state);
> > > > > + if (old_mode != mode)
> > > > > + ret = intel_psr_modeset_force(dev_priv);
> > > > >
> > > > > - mutex_unlock(&dev_priv->psr.lock);
> > > > > return ret;
> > > > > }
> > > > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
2018-12-04 21:23 ` Dhinakaran Pandiyan
@ 2018-12-11 18:16 ` Souza, Jose
2018-12-17 11:16 ` Maarten Lankhorst
0 siblings, 1 reply; 15+ messages in thread
From: Souza, Jose @ 2018-12-11 18:16 UTC (permalink / raw)
To: intel-gfx, Pandiyan, Dhinakaran; +Cc: Vivi, Rodrigo
[-- Attachment #1.1: Type: text/plain, Size: 15485 bytes --]
On Tue, 2018-12-04 at 13:23 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2018-12-04 at 10:52 -0800, Souza, Jose wrote:
> > On Mon, 2018-12-03 at 18:58 -0800, Dhinakaran Pandiyan wrote:
> > > On Mon, 2018-12-03 at 17:54 -0800, Souza, Jose wrote:
> > > > On Mon, 2018-12-03 at 17:33 -0800, Dhinakaran Pandiyan wrote:
> > > > > On Thu, 2018-11-29 at 18:31 -0800, José Roberto de Souza
> > > > > wrote:
> > > > > > Changing the i915_edp_psr_debug was enabling, disabling or
> > > > > > switching
> > > > > > PSR version by directly calling intel_psr_disable_locked()
> > > > > > and
> > > > > > intel_psr_enable_locked(), what is not the default PSR path
> > > > > > that
> > > > > > is
> > > > > > executed in a regular modesets.
> > > > > >
> > > > > > So lets force a modeset in the PSR CRTC to trigger the
> > > > > > requested
> > > > > > PSR
> > > > > > state change and really stress the code path that matters
> > > > > > for
> > > > > > the
> > > > > > regular user.
> > > > > >
> > > > > > Also by doing this way it fixes the issue below, were DRRS
> > > > > > was
> > > > > > left
> > > > > > enabled together with PSR when enabling PSR from debugfs.
> > > > >
> > > > > While this patch does fix the issue, psr_compute_config() not
> > > > > checking
> > > > > crtc_state->has_drrs seems odd. We should change it to not
> > > > > set
> > > > > crtc_state->has_psr if crtc_state->has_drrs happens to be
> > > > > set.
> > > > > Or
> > > > > do
> > > > > it
> > > > > the other way around.
> > > >
> > > > psr_compute_config() is not called when enabling PSR from
> > > > debugfs,
> > > > this
> > >
> > > Right. My suggestion is to allow either ->has_drrs or ->has_psr
> > > being
> > > set (not both) in the kernel and disable DRRS in the IGT before
> > > starting the test.
> >
> > So in case were PSR is disabled by parameter and DRRS is supported
> > we
> > would not enable DRRS? Because has_psr is set even if PSR is
> > disabled.
> Set ->has_psr = true in psr_compute_config() only if the module
> parameter and debugfs mode allow it. That is how the code worked
> earlier. Given that this patch duplicates the atomic state and runs
> through all state checks, we can move back to the earlier way of
> completing all checks in psr_compute_config().
>
> > Disabling DRRS from IGT is duplicating the code that already do
> > that
> > and also not validating the default code path.
> Call drrs_compute_config() after psr_compute_config(), don't set
> has_drrs if has_psr is set.
What about add a flag to skip modeset so when running IGT tests we set
that flag and PSR mode will be changed in the next modeset, what is
already done after every write to i915_edp_psr_debug in IGT tests? This
way we remove the code duplication and only stress the default code
path.
Also plus the changes in has_drrs that you mentioned but in other
patch.
>
> > >
> > > > issue was reported when PSR1 was not enabled by default so DRRS
> > > > was
> > > > being enabled by default but not PSR and when PSR was enabled
> > > > from
> > > > debugfs both were kept enabled.
> > > >
> > > > In the first version of this patch I was just disabling DRRS
> > > > when
> > > > enabling PSR but Maarten suggested to not do so and instead
> > > > stress
> > > > the
> > > > default code path.
> > >
> > > This patch changes crtc_state->mode_changed to force a modeset,
> > > which
> > > means it is not exactly the same as the 'default' code path.
> >
> > Some drm and i915 code paths also set mode_changed and the result
> > of
> > this is exactly the same as do a full modeset.
> >
> > >
> > >
> > > > > > Bugzilla:
> > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_debugfs.c | 14 +---
> > > > > > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > > > > > drivers/gpu/drm/i915/intel_drv.h | 4 +-
> > > > > > drivers/gpu/drm/i915/intel_psr.c | 119 ++++++++++++----
> > > > > > ----
> > > > > > ----
> > > > > > ----
> > > > > > 4 files changed, 55 insertions(+), 84 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > index 129b9a6f8309..bc32f683dc46 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > @@ -2775,7 +2775,6 @@ static int
> > > > > > i915_edp_psr_debug_set(void *data, u64 val)
> > > > > > {
> > > > > > struct drm_i915_private *dev_priv = data;
> > > > > > - struct drm_modeset_acquire_ctx ctx;
> > > > > > int ret;
> > > > > >
> > > > > > if (!CAN_PSR(dev_priv))
> > > > > > @@ -2785,18 +2784,7 @@ i915_edp_psr_debug_set(void *data,
> > > > > > u64
> > > > > > val)
> > > > > >
> > > > > > intel_runtime_pm_get(dev_priv);
> > > > > >
> > > > > > - drm_modeset_acquire_init(&ctx,
> > > > > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> > > > > > -
> > > > > > -retry:
> > > > > > - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
> > > > > > - if (ret == -EDEADLK) {
> > > > > > - ret = drm_modeset_backoff(&ctx);
> > > > > > - if (!ret)
> > > > > > - goto retry;
> > > > > > - }
> > > > > > -
> > > > > > - drm_modeset_drop_locks(&ctx);
> > > > > > - drm_modeset_acquire_fini(&ctx);
> > > > > > + ret = intel_psr_set_debugfs_mode(dev_priv, val);
> > > > > >
> > > > > > intel_runtime_pm_put(dev_priv);
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > index 43ac6873a2bb..93d8ca9c0375 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > @@ -493,7 +493,7 @@ struct i915_psr {
> > > > > >
> > > > > > u32 debug;
> > > > > > bool sink_support;
> > > > > > - bool prepared, enabled;
> > > > > > + bool enabled;
> > > > > > struct intel_dp *dp;
> > > > > > bool active;
> > > > > > struct work_struct work;
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > index 86ff57738cd9..89d4eed0dd89 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > @@ -2056,9 +2056,7 @@ void intel_psr_enable(struct intel_dp
> > > > > > *intel_dp,
> > > > > > const struct intel_crtc_state
> > > > > > *crtc_state);
> > > > > > void intel_psr_disable(struct intel_dp *intel_dp,
> > > > > > const struct intel_crtc_state
> > > > > > *old_crtc_state);
> > > > > > -int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > > > *dev_priv,
> > > > > > - struct drm_modeset_acquire_ctx
> > > > > > *ctx,
> > > > > > - u64 value);
> > > > > > +int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > > > *dev_priv,
> > > > > > u64 value);
> > > > > > void intel_psr_invalidate(struct drm_i915_private
> > > > > > *dev_priv,
> > > > > > unsigned frontbuffer_bits,
> > > > > > enum fb_op_origin origin);
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > index 827b8c31783d..305848cceda7 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > @@ -52,6 +52,7 @@
> > > > > > */
> > > > > >
> > > > > > #include <drm/drmP.h>
> > > > > > +#include <drm/drm_atomic_helper.h>
> > > > > >
> > > > > > #include "intel_drv.h"
> > > > > > #include "i915_drv.h"
> > > > > > @@ -711,10 +712,6 @@ void intel_psr_enable(struct intel_dp
> > > > > > *intel_dp,
> > > > > > WARN_ON(dev_priv->drrs.dp);
> > > > > >
> > > > > > mutex_lock(&dev_priv->psr.lock);
> > > > > > - if (dev_priv->psr.prepared) {
> > > > > > - DRM_DEBUG_KMS("PSR already in use\n");
> > > > > > - goto unlock;
> > > > > > - }
> > > > > >
> > > > > > if (!psr_global_enabled(dev_priv->psr.debug)) {
> > > > > > DRM_DEBUG_KMS("PSR disabled by flag\n");
> > > > > > @@ -723,7 +720,6 @@ void intel_psr_enable(struct intel_dp
> > > > > > *intel_dp,
> > > > > >
> > > > > > dev_priv->psr.psr2_enabled =
> > > > > > intel_psr2_enabled(dev_priv,
> > > > > > crtc_state);
> > > > > > dev_priv->psr.busy_frontbuffer_bits = 0;
> > > > > > - dev_priv->psr.prepared = true;
> > > > > > intel_psr_enable_locked(dev_priv, crtc_state);
> > > > > >
> > > > > > unlock:
> > > > > > @@ -807,14 +803,9 @@ void intel_psr_disable(struct intel_dp
> > > > > > *intel_dp,
> > > > > > return;
> > > > > >
> > > > > > mutex_lock(&dev_priv->psr.lock);
> > > > > > - if (!dev_priv->psr.prepared) {
> > > > > > - mutex_unlock(&dev_priv->psr.lock);
> > > > > > - return;
> > > > > > - }
> > > > > >
> > > > > > intel_psr_disable_locked(intel_dp);
> > > > > >
> > > > > > - dev_priv->psr.prepared = false;
> > > > > > mutex_unlock(&dev_priv->psr.lock);
> > > > > > cancel_work_sync(&dev_priv->psr.work);
> > > > > > }
> > > > > > @@ -883,36 +874,61 @@ static bool
> > > > > > __psr_wait_for_idle_locked(struct
> > > > > > drm_i915_private *dev_priv)
> > > > > > return err == 0 && dev_priv->psr.enabled;
> > > > > > }
> > > > > >
> > > > > > -static bool switching_psr(struct drm_i915_private
> > > > > > *dev_priv,
> > > > > > - struct intel_crtc_state *crtc_state,
> > > > > > - u32 mode)
> > > > > > +static int intel_psr_modeset_force(struct drm_i915_private
> > > > > > *dev_priv)
> > > > > > {
> > > > > > - /* Can't switch psr state anyway if PSR2 is not
> > > > > > supported. */
> > > > > > - if (!crtc_state || !crtc_state->has_psr2)
> > > > > > - return false;
> > > > > > + struct drm_device *dev = &dev_priv->drm;
> > > > > > + struct drm_modeset_acquire_ctx ctx;
> > > > > > + struct drm_atomic_state *state;
> > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > + struct drm_crtc *crtc;
> > > > > > + int err, i;
> > > > > > +
> > > > > > + mutex_lock(&dev->mode_config.mutex);
> > > > > > + drm_modeset_acquire_init(&ctx, 0);
> > > > > > +
> > > > > > +modeset_lock_retry:
> > > > > > + err = drm_modeset_lock_all_ctx(dev, &ctx);
> > > > > > + if (err) {
> > > > > > + if (err == -EDEADLK) {
> > > > > > + err = drm_modeset_backoff(&ctx);
> > > > > > + if (!err)
> > > > > > + goto modeset_lock_retry;
> > > > > > + }
> > > > > >
> > > > > > - if (dev_priv->psr.psr2_enabled && mode ==
> > > > > > I915_PSR_DEBUG_FORCE_PSR1)
> > > > > > - return true;
> > > > > > + goto unlock;
> > > > > > + }
> > > > > >
> > > > > > - if (!dev_priv->psr.psr2_enabled && mode !=
> > > > > > I915_PSR_DEBUG_FORCE_PSR1)
> > > > > > - return true;
> > > > > > + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > > > + if (IS_ERR(state)) {
> > > > > > + err = PTR_ERR(state);
> > > > > > + goto unlock;
> > > > > > + }
> > > > > > +
> > > > > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> > > > > > {
> > > > > > + struct intel_crtc_state *pipe_state;
> > > > > > +
> > > > > > + pipe_state = to_intel_crtc_state(crtc_state);
> > > > > > + /* Mark mode as changed to force a modeset */
> > > > > > + if (pipe_state->has_psr)
> > > > > > + crtc_state->mode_changed = true;
> > > > > > + }
> > > > > > +
> > > > > > + err = drm_atomic_helper_commit_duplicated_state(state,
> > > > > > &ctx);
> > > > > >
> > > > > > - return false;
> > > > > > + drm_atomic_state_put(state);
> > > > > > +unlock:
> > > > > > + drm_modeset_drop_locks(&ctx);
> > > > > > + drm_modeset_acquire_fini(&ctx);
> > > > > > + mutex_unlock(&dev->mode_config.mutex);
> > > > > > +
> > > > > > + return err;
> > > > > > }
> > > > > >
> > > > > > -int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > > > *dev_priv,
> > > > > > - struct drm_modeset_acquire_ctx
> > > > > > *ctx,
> > > > > > - u64 val)
> > > > > > +int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > > > *dev_priv,
> > > > > > u64 val)
> > > > > > {
> > > > > > - struct drm_device *dev = &dev_priv->drm;
> > > > > > - struct drm_connector_state *conn_state;
> > > > > > - struct intel_crtc_state *crtc_state = NULL;
> > > > > > - struct drm_crtc_commit *commit;
> > > > > > - struct drm_crtc *crtc;
> > > > > > - struct intel_dp *dp;
> > > > > > + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > > > > > + u32 old_mode;
> > > > > > int ret;
> > > > > > - bool enable;
> > > > > > - u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > > > > >
> > > > > > if (val & ~(I915_PSR_DEBUG_IRQ |
> > > > > > I915_PSR_DEBUG_MODE_MASK) ||
> > > > > > mode > I915_PSR_DEBUG_FORCE_PSR1) {
> > > > > > @@ -920,49 +936,18 @@ int intel_psr_set_debugfs_mode(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > > return -EINVAL;
> > > > > > }
> > > > > >
> > > > > > - ret = drm_modeset_lock(&dev-
> > > > > > > mode_config.connection_mutex,
> > > > > >
> > > > > > ctx);
> > > > > > - if (ret)
> > > > > > - return ret;
> > > > > > -
> > > > > > - /* dev_priv->psr.dp should be set once and then never
> > > > > > touched
> > > > > > again. */
> > > > > > - dp = READ_ONCE(dev_priv->psr.dp);
> > > > > > - conn_state = dp->attached_connector->base.state;
> > > > > > - crtc = conn_state->crtc;
> > > > > > - if (crtc) {
> > > > > > - ret = drm_modeset_lock(&crtc->mutex, ctx);
> > > > > > - if (ret)
> > > > > > - return ret;
> > > > > > -
> > > > > > - crtc_state = to_intel_crtc_state(crtc->state);
> > > > > > - commit = crtc_state->base.commit;
> > > > > > - } else {
> > > > > > - commit = conn_state->commit;
> > > > > > - }
> > > > > > - if (commit) {
> > > > > > - ret =
> > > > > > wait_for_completion_interruptible(&commit-
> > > > > > > hw_done);
> > > > > >
> > > > > > - if (ret)
> > > > > > - return ret;
> > > > > > - }
> > > > > > -
> > > > > > ret = mutex_lock_interruptible(&dev_priv->psr.lock);
> > > > > > if (ret)
> > > > > > return ret;
> > > > > >
> > > > > > - enable = psr_global_enabled(val);
> > > > > > -
> > > > > > - if (!enable || switching_psr(dev_priv, crtc_state,
> > > > > > mode))
> > > > > > - intel_psr_disable_locked(dev_priv->psr.dp);
> > > > > > -
> > > > > > + old_mode = dev_priv->psr.debug &
> > > > > > I915_PSR_DEBUG_MODE_MASK;
> > > > > > dev_priv->psr.debug = val;
> > > > > > - if (crtc)
> > > > > > - dev_priv->psr.psr2_enabled =
> > > > > > intel_psr2_enabled(dev_priv, crtc_state);
> > > > > >
> > > > > > - intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > > > > + mutex_unlock(&dev_priv->psr.lock);
> > > > > >
> > > > > > - if (dev_priv->psr.prepared && enable)
> > > > > > - intel_psr_enable_locked(dev_priv, crtc_state);
> > > > > > + if (old_mode != mode)
> > > > > > + ret = intel_psr_modeset_force(dev_priv);
> > > > > >
> > > > > > - mutex_unlock(&dev_priv->psr.lock);
> > > > > > return ret;
> > > > > > }
> > > > > >
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
2018-12-11 18:16 ` Souza, Jose
@ 2018-12-17 11:16 ` Maarten Lankhorst
2018-12-21 13:54 ` Maarten Lankhorst
0 siblings, 1 reply; 15+ messages in thread
From: Maarten Lankhorst @ 2018-12-17 11:16 UTC (permalink / raw)
To: Souza, Jose, intel-gfx, Pandiyan, Dhinakaran; +Cc: Vivi, Rodrigo
Op 11-12-2018 om 19:16 schreef Souza, Jose:
> On Tue, 2018-12-04 at 13:23 -0800, Dhinakaran Pandiyan wrote:
>> On Tue, 2018-12-04 at 10:52 -0800, Souza, Jose wrote:
>>> On Mon, 2018-12-03 at 18:58 -0800, Dhinakaran Pandiyan wrote:
>>>> On Mon, 2018-12-03 at 17:54 -0800, Souza, Jose wrote:
>>>>> On Mon, 2018-12-03 at 17:33 -0800, Dhinakaran Pandiyan wrote:
>>>>>> On Thu, 2018-11-29 at 18:31 -0800, José Roberto de Souza
>>>>>> wrote:
>>>>>>> Changing the i915_edp_psr_debug was enabling, disabling or
>>>>>>> switching
>>>>>>> PSR version by directly calling intel_psr_disable_locked()
>>>>>>> and
>>>>>>> intel_psr_enable_locked(), what is not the default PSR path
>>>>>>> that
>>>>>>> is
>>>>>>> executed in a regular modesets.
>>>>>>>
>>>>>>> So lets force a modeset in the PSR CRTC to trigger the
>>>>>>> requested
>>>>>>> PSR
>>>>>>> state change and really stress the code path that matters
>>>>>>> for
>>>>>>> the
>>>>>>> regular user.
>>>>>>>
>>>>>>> Also by doing this way it fixes the issue below, were DRRS
>>>>>>> was
>>>>>>> left
>>>>>>> enabled together with PSR when enabling PSR from debugfs.
>>>>>> While this patch does fix the issue, psr_compute_config() not
>>>>>> checking
>>>>>> crtc_state->has_drrs seems odd. We should change it to not
>>>>>> set
>>>>>> crtc_state->has_psr if crtc_state->has_drrs happens to be
>>>>>> set.
>>>>>> Or
>>>>>> do
>>>>>> it
>>>>>> the other way around.
>>>>> psr_compute_config() is not called when enabling PSR from
>>>>> debugfs,
>>>>> this
>>>> Right. My suggestion is to allow either ->has_drrs or ->has_psr
>>>> being
>>>> set (not both) in the kernel and disable DRRS in the IGT before
>>>> starting the test.
>>> So in case were PSR is disabled by parameter and DRRS is supported
>>> we
>>> would not enable DRRS? Because has_psr is set even if PSR is
>>> disabled.
>> Set ->has_psr = true in psr_compute_config() only if the module
>> parameter and debugfs mode allow it. That is how the code worked
>> earlier. Given that this patch duplicates the atomic state and runs
>> through all state checks, we can move back to the earlier way of
>> completing all checks in psr_compute_config().
>>
>>> Disabling DRRS from IGT is duplicating the code that already do
>>> that
>>> and also not validating the default code path.
>> Call drrs_compute_config() after psr_compute_config(), don't set
>> has_drrs if has_psr is set.
> What about add a flag to skip modeset so when running IGT tests we set
> that flag and PSR mode will be changed in the next modeset, what is
> already done after every write to i915_edp_psr_debug in IGT tests? This
> way we remove the code duplication and only stress the default code
> path.
>
> Also plus the changes in has_drrs that you mentioned but in other
> patch.
Well the reason we set both is because kernel should decide which one to enable. The
only way we can have both active is if we mess with debugfs.
I see the fact you're trying to enable both as a failure from the user. Anything in
debugfs can be used for advanced debugging, and can possibly brick your system. If
we really care, then we should disable DRRS when enabling PSR, and the same when
enabling DRRS, that we disable PSR.
There's no need to restore it afterwards, because it's debugfs api.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
2018-12-17 11:16 ` Maarten Lankhorst
@ 2018-12-21 13:54 ` Maarten Lankhorst
0 siblings, 0 replies; 15+ messages in thread
From: Maarten Lankhorst @ 2018-12-21 13:54 UTC (permalink / raw)
To: Souza, Jose, intel-gfx, Pandiyan, Dhinakaran; +Cc: Vivi, Rodrigo
Op 17-12-2018 om 12:16 schreef Maarten Lankhorst:
> Op 11-12-2018 om 19:16 schreef Souza, Jose:
>> On Tue, 2018-12-04 at 13:23 -0800, Dhinakaran Pandiyan wrote:
>>> On Tue, 2018-12-04 at 10:52 -0800, Souza, Jose wrote:
>>>> On Mon, 2018-12-03 at 18:58 -0800, Dhinakaran Pandiyan wrote:
>>>>> On Mon, 2018-12-03 at 17:54 -0800, Souza, Jose wrote:
>>>>>> On Mon, 2018-12-03 at 17:33 -0800, Dhinakaran Pandiyan wrote:
>>>>>>> On Thu, 2018-11-29 at 18:31 -0800, José Roberto de Souza
>>>>>>> wrote:
>>>>>>>> Changing the i915_edp_psr_debug was enabling, disabling or
>>>>>>>> switching
>>>>>>>> PSR version by directly calling intel_psr_disable_locked()
>>>>>>>> and
>>>>>>>> intel_psr_enable_locked(), what is not the default PSR path
>>>>>>>> that
>>>>>>>> is
>>>>>>>> executed in a regular modesets.
>>>>>>>>
>>>>>>>> So lets force a modeset in the PSR CRTC to trigger the
>>>>>>>> requested
>>>>>>>> PSR
>>>>>>>> state change and really stress the code path that matters
>>>>>>>> for
>>>>>>>> the
>>>>>>>> regular user.
>>>>>>>>
>>>>>>>> Also by doing this way it fixes the issue below, were DRRS
>>>>>>>> was
>>>>>>>> left
>>>>>>>> enabled together with PSR when enabling PSR from debugfs.
>>>>>>> While this patch does fix the issue, psr_compute_config() not
>>>>>>> checking
>>>>>>> crtc_state->has_drrs seems odd. We should change it to not
>>>>>>> set
>>>>>>> crtc_state->has_psr if crtc_state->has_drrs happens to be
>>>>>>> set.
>>>>>>> Or
>>>>>>> do
>>>>>>> it
>>>>>>> the other way around.
>>>>>> psr_compute_config() is not called when enabling PSR from
>>>>>> debugfs,
>>>>>> this
>>>>> Right. My suggestion is to allow either ->has_drrs or ->has_psr
>>>>> being
>>>>> set (not both) in the kernel and disable DRRS in the IGT before
>>>>> starting the test.
>>>> So in case were PSR is disabled by parameter and DRRS is supported
>>>> we
>>>> would not enable DRRS? Because has_psr is set even if PSR is
>>>> disabled.
>>> Set ->has_psr = true in psr_compute_config() only if the module
>>> parameter and debugfs mode allow it. That is how the code worked
>>> earlier. Given that this patch duplicates the atomic state and runs
>>> through all state checks, we can move back to the earlier way of
>>> completing all checks in psr_compute_config().
>>>
>>>> Disabling DRRS from IGT is duplicating the code that already do
>>>> that
>>>> and also not validating the default code path.
>>> Call drrs_compute_config() after psr_compute_config(), don't set
>>> has_drrs if has_psr is set.
>> What about add a flag to skip modeset so when running IGT tests we set
>> that flag and PSR mode will be changed in the next modeset, what is
>> already done after every write to i915_edp_psr_debug in IGT tests? This
>> way we remove the code duplication and only stress the default code
>> path.
>>
>> Also plus the changes in has_drrs that you mentioned but in other
>> patch.
> Well the reason we set both is because kernel should decide which one to enable. The
> only way we can have both active is if we mess with debugfs.
>
> I see the fact you're trying to enable both as a failure from the user. Anything in
> debugfs can be used for advanced debugging, and can possibly brick your system. If
> we really care, then we should disable DRRS when enabling PSR, and the same when
> enabling DRRS, that we disable PSR.
>
> There's no need to restore it afterwards, because it's debugfs api.
>
> ~Maarten
>
.. or we could stop messing around, make a commit which sets crtc_state->mode_changed,
it will degrade the modeset to a fastset, and then we can use update_pipe() to make
the new settings active:
https://patchwork.freedesktop.org/series/54339/
And then only set enable_psr enable_drrs in crtc_state when we plan to use
it. This removes the need for separate settings..
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-12-21 13:54 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-30 2:31 [PATCH 1/2] drm/i915: Check if PSR is globally enabled before change PSR variables José Roberto de Souza
2018-11-30 2:31 ` [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug José Roberto de Souza
2018-12-04 1:33 ` Dhinakaran Pandiyan
2018-12-04 1:54 ` Souza, Jose
2018-12-04 2:58 ` Dhinakaran Pandiyan
2018-12-04 18:52 ` Souza, Jose
2018-12-04 21:23 ` Dhinakaran Pandiyan
2018-12-11 18:16 ` Souza, Jose
2018-12-17 11:16 ` Maarten Lankhorst
2018-12-21 13:54 ` Maarten Lankhorst
2018-11-30 2:58 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Check if PSR is globally enabled before change PSR variables Patchwork
2018-11-30 3:16 ` ✓ Fi.CI.BAT: success " Patchwork
2018-11-30 5:56 ` [PATCH 1/2] " Rodrigo Vivi
2018-12-04 0:58 ` Dhinakaran Pandiyan
2018-12-04 1:09 ` Souza, Jose
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.