All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update
@ 2018-07-10  0:39 Tarun Vyas
  2018-07-10  1:12 ` ✓ Fi.CI.BAT: success for drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update (rev4) Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Tarun Vyas @ 2018-07-10  0:39 UTC (permalink / raw)
  To: intel-gfx; +Cc: dhinakaran.pandiyan, rodrigo.vivi

In commit "drm/i915: Wait for PSR exit before checking for vblank
evasion", the idea was to limit the PSR IDLE checks when PSR is
actually supported. While CAN_PSR does do that check, it doesn't
applies on a per-crtc basis. crtc_state->has_psr is a more granular
check that only applies to pipe(s) that have PSR enabled.

Currently, the driver supports PSR on port A + transcoder eDP, so
only pipe A will wait for PSR to go IDLE, as it should, and other
pipes should return immediately.

Without the has_psr check, non-PSR pipe_updates (pipe B/C in this
case), end up waiting on PSR pipe (pipe A in this case) to exit PSR,
which may incur substantial delays for non-PSR pipe updates alongwith
the fact the it doesn't makes any sense.

Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for
vblank evasion")

v2: Remove unnecessary parantheses, make checkpatch happy.

v3: Move the has_psr check to intel_psr_wait_for_idle and commit
    message changes (DK).

v4: Derive dev_priv from intel_crtc_state (DK)

Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h    | 2 +-
 drivers/gpu/drm/i915/intel_psr.c    | 7 ++++++-
 drivers/gpu/drm/i915/intel_sprite.c | 2 +-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 61e715ddd0d5..699073fbecb1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1923,7 +1923,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
 void intel_psr_short_pulse(struct intel_dp *intel_dp);
-int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
+int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 23acc9ac8d4d..e97db5dd75b1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -717,11 +717,16 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 	cancel_work_sync(&dev_priv->psr.work);
 }
 
-int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
+int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state)
 {
+	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	i915_reg_t reg;
 	u32 mask;
 
+	if (!new_crtc_state->has_psr)
+		return 0;
+
 	/*
 	 * The sole user right now is intel_pipe_update_start(),
 	 * which won't race with psr_enable/disable, which is
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 4990d6e84ddf..9d6d1ac149da 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 	 * VBL interrupts will start the PSR exit and prevent a PSR
 	 * re-entry as well.
 	 */
-	if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
+	if (intel_psr_wait_for_idle(new_crtc_state))
 		DRM_ERROR("PSR idle timed out, atomic update may fail\n");
 
 	local_irq_disable();
-- 
2.13.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update (rev4)
  2018-07-10  0:39 [PATCH v4] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update Tarun Vyas
@ 2018-07-10  1:12 ` Patchwork
  2018-07-10 10:22 ` ✓ Fi.CI.IGT: " Patchwork
  2018-07-12  5:44 ` [PATCH v4] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update Dhinakaran Pandiyan
  2 siblings, 0 replies; 4+ messages in thread
From: Patchwork @ 2018-07-10  1:12 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update (rev4)
URL   : https://patchwork.freedesktop.org/series/46104/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4458 -> Patchwork_9600 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46104/revisions/4/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9600 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#105128, fdo#107139) -> PASS

    igt@kms_flip@basic-flip-vs-dpms:
      fi-skl-6700hq:      DMESG-WARN (fdo#105998) -> PASS

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139


== Participating hosts (45 -> 40) ==

  Additional (1): fi-skl-guc 
  Missing    (6): fi-ilk-m540 fi-bxt-dsi fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_4458 -> Patchwork_9600

  CI_DRM_4458: dbed4c29cc0cd0225aae60c57ac2f4a738bb5a17 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4544: 764160f214cd916ddb79408b9f28ac0ad2df40e0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9600: 1e1602d46abafb8f5722c411d653376f6b2e3c83 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

1e1602d46aba drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9600/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 4+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update (rev4)
  2018-07-10  0:39 [PATCH v4] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update Tarun Vyas
  2018-07-10  1:12 ` ✓ Fi.CI.BAT: success for drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update (rev4) Patchwork
@ 2018-07-10 10:22 ` Patchwork
  2018-07-12  5:44 ` [PATCH v4] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update Dhinakaran Pandiyan
  2 siblings, 0 replies; 4+ messages in thread
From: Patchwork @ 2018-07-10 10:22 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update (rev4)
URL   : https://patchwork.freedesktop.org/series/46104/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4458_full -> Patchwork_9600_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9600_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9600_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9600_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-blt:
      shard-kbl:          PASS -> SKIP

    igt@gem_exec_schedule@deep-vebox:
      shard-kbl:          SKIP -> PASS +2

    
== Known issues ==

  Here are the changes found in Patchwork_9600_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106023)

    igt@kms_flip@dpms-vs-vblank-race-interruptible:
      shard-glk:          PASS -> FAIL (fdo#103060)

    igt@kms_rotation_crc@sprite-rotation-180:
      shard-hsw:          PASS -> FAIL (fdo#103925)

    igt@pm_rpm@system-suspend:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    
    ==== Possible fixes ====

    igt@gem_exec_big:
      shard-hsw:          INCOMPLETE (fdo#103540) -> PASS

    igt@kms_flip@2x-dpms-vs-vblank-race-interruptible:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@2x-flip-vs-absolute-wf_vblank:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          FAIL (fdo#103822, fdo#107161) -> PASS +1

    igt@kms_flip_tiling@flip-y-tiled:
      shard-glk:          FAIL (fdo#107161) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#107161 https://bugs.freedesktop.org/show_bug.cgi?id=107161


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4458 -> Patchwork_9600

  CI_DRM_4458: dbed4c29cc0cd0225aae60c57ac2f4a738bb5a17 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4544: 764160f214cd916ddb79408b9f28ac0ad2df40e0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9600: 1e1602d46abafb8f5722c411d653376f6b2e3c83 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9600/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update
  2018-07-10  0:39 [PATCH v4] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update Tarun Vyas
  2018-07-10  1:12 ` ✓ Fi.CI.BAT: success for drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update (rev4) Patchwork
  2018-07-10 10:22 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-07-12  5:44 ` Dhinakaran Pandiyan
  2 siblings, 0 replies; 4+ messages in thread
From: Dhinakaran Pandiyan @ 2018-07-12  5:44 UTC (permalink / raw)
  To: Tarun Vyas, intel-gfx; +Cc: rodrigo.vivi

On Mon, 2018-07-09 at 17:39 -0700, Tarun Vyas wrote:
> In commit "drm/i915: Wait for PSR exit before checking for vblank
> evasion", the idea was to limit the PSR IDLE checks when PSR is
> actually supported. While CAN_PSR does do that check, it doesn't
> applies on a per-crtc basis. crtc_state->has_psr is a more granular
> check that only applies to pipe(s) that have PSR enabled.
> 
> Currently, the driver supports PSR on port A + transcoder eDP, so
> only pipe A will wait for PSR to go IDLE, as it should, and other
> pipes should return immediately.
This still doesn't read right to me. Sorry for being pedantic,
documenting the hardware behaviour, especially when it comes to PSR is
important.



> Without the has_psr check, non-PSR pipe_updates (pipe B/C in this
> case), end up waiting on PSR pipe (pipe A in this case) to exit PSR,
> which may incur substantial delays for non-PSR pipe updates alongwith
> the fact the it doesn't makes any sense.

How about just saying "Without the crtc_state->has_psr check, we end up
waiting on the eDP transcoder's PSR_STATUS register irrespective of
whether the pipe being updated is driving it or not".

With the commit message altered, feel free to add
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> 
> Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for
> vblank evasion")
> 
> v2: Remove unnecessary parantheses, make checkpatch happy.
> 
> v3: Move the has_psr check to intel_psr_wait_for_idle and commit
>     message changes (DK).
> 
> v4: Derive dev_priv from intel_crtc_state (DK)
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h    | 2 +-
>  drivers/gpu/drm/i915/intel_psr.c    | 7 ++++++-
>  drivers/gpu/drm/i915/intel_sprite.c | 2 +-
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 61e715ddd0d5..699073fbecb1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1923,7 +1923,7 @@ void intel_psr_compute_config(struct intel_dp
> *intel_dp,
>  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
> debug);
>  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32
> psr_iir);
>  void intel_psr_short_pulse(struct intel_dp *intel_dp);
> -int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
> +int intel_psr_wait_for_idle(const struct intel_crtc_state
> *new_crtc_state);
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 23acc9ac8d4d..e97db5dd75b1 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -717,11 +717,16 @@ void intel_psr_disable(struct intel_dp
> *intel_dp,
>  	cancel_work_sync(&dev_priv->psr.work);
>  }
>  
> -int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
> +int intel_psr_wait_for_idle(const struct intel_crtc_state
> *new_crtc_state)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state-
> >base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	i915_reg_t reg;
>  	u32 mask;
>  
> +	if (!new_crtc_state->has_psr)
> +		return 0;
> +
>  	/*
>  	 * The sole user right now is intel_pipe_update_start(),
>  	 * which won't race with psr_enable/disable, which is
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 4990d6e84ddf..9d6d1ac149da 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct
> intel_crtc_state *new_crtc_state)
>  	 * VBL interrupts will start the PSR exit and prevent a PSR
>  	 * re-entry as well.
>  	 */
> -	if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
> +	if (intel_psr_wait_for_idle(new_crtc_state))
>  		DRM_ERROR("PSR idle timed out, atomic update may
> fail\n");
>  
>  	local_irq_disable();
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-07-12  5:19 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-10  0:39 [PATCH v4] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update Tarun Vyas
2018-07-10  1:12 ` ✓ Fi.CI.BAT: success for drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update (rev4) Patchwork
2018-07-10 10:22 ` ✓ Fi.CI.IGT: " Patchwork
2018-07-12  5:44 ` [PATCH v4] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update Dhinakaran Pandiyan

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.