All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/psr: Fix race in intel_psr_work()
@ 2018-06-23  4:45 Dhinakaran Pandiyan
  2018-06-23  5:14 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Dhinakaran Pandiyan @ 2018-06-23  4:45 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi

commit 5422b37c907e ("drm/i915/psr: Kill delays when activating psr
back.") switched from delayed work to the plain variant and while doing so
remove the check for work_busy() before scheduling a PSR activation.
This appears to cause consecutive executions of psr_activate() in this
scenario - after a worker picks up the PSR work item for execution and
before the work function can acquire the PSR mutex, a psr_flush() can
get hold of the mutex and schedule another PSR work. Without a psr_exit()
between two psr_activate() calls, the warning messages get printed.
Further, since we drop the mutex in the midst of psr_work() to wait for
PSR to idle, another work item can also get scheduled. Fix this by
returning if PSR was already active.

Note:
I am not 100% sure this is what is going on as I could not reproduce
the bug (https://bugs.freedesktop.org/show_bug.cgi?id=106948)

This patch sort of defeats the point of the WARN_ON()s in psr_activate()
now, do we still need them?

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index aea81ace854b..7aa324f0d1f7 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -811,7 +811,7 @@ static void intel_psr_work(struct work_struct *work)
 	 * recheck. Since psr_flush first clears this and then reschedules we
 	 * won't ever miss a flush when bailing out here.
 	 */
-	if (dev_priv->psr.busy_frontbuffer_bits)
+	if (dev_priv->psr.busy_frontbuffer_bits || dev_priv->psr.active)
 		goto unlock;
 
 	intel_psr_activate(dev_priv->psr.enabled);
-- 
2.14.1

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

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

* ✓ Fi.CI.BAT: success for drm/i915/psr: Fix race in intel_psr_work()
  2018-06-23  4:45 [PATCH] drm/i915/psr: Fix race in intel_psr_work() Dhinakaran Pandiyan
@ 2018-06-23  5:14 ` Patchwork
  2018-06-23  6:05 ` ✓ Fi.CI.IGT: " Patchwork
  2018-06-24  8:54 ` [PATCH] " Chris Wilson
  2 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2018-06-23  5:14 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: Fix race in intel_psr_work()
URL   : https://patchwork.freedesktop.org/series/45291/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4370 -> Patchwork_9405 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45291/revisions/1/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_create@basic-files:
      fi-skl-6700k2:      PASS -> INCOMPLETE (fdo#106988)

    igt@gem_ringfill@basic-default-hang:
      fi-pnv-d510:        NOTRUN -> DMESG-WARN (fdo#101600)

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

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    
  fdo#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106988 https://bugs.freedesktop.org/show_bug.cgi?id=106988


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

  Additional (2): fi-byt-j1900 fi-pnv-d510 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-glk-dsi fi-bsw-cyan fi-ctg-p8600 fi-kbl-x1275 


== Build changes ==

    * Linux: CI_DRM_4370 -> Patchwork_9405

  CI_DRM_4370: e43f851caf5a4b7a30c0ccbdf10fb1df81fa1c75 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4528: 6be300d405de5974b262e8b93a445be4ac618e6a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9405: 934e52033eb267b58490c8255dd17fb4d994fdd4 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

934e52033eb2 drm/i915/psr: Fix race in intel_psr_work()

== Logs ==

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

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

* ✓ Fi.CI.IGT: success for drm/i915/psr: Fix race in intel_psr_work()
  2018-06-23  4:45 [PATCH] drm/i915/psr: Fix race in intel_psr_work() Dhinakaran Pandiyan
  2018-06-23  5:14 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-06-23  6:05 ` Patchwork
  2018-06-24  8:54 ` [PATCH] " Chris Wilson
  2 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2018-06-23  6:05 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: Fix race in intel_psr_work()
URL   : https://patchwork.freedesktop.org/series/45291/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4370_full -> Patchwork_9405_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9405_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9405_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_9405_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-blt:
      shard-kbl:          SKIP -> PASS +1

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_workarounds@suspend-resume-context:
      shard-apl:          PASS -> FAIL (fdo#103375)

    igt@kms_flip@blocking-wf_vblank:
      shard-glk:          PASS -> FAIL (fdo#103928)

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-pgflip-blt:
      shard-glk:          PASS -> FAIL (fdo#104724, fdo#103167)

    igt@kms_setmode@basic:
      shard-hsw:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-apl:          FAIL (fdo#105347) -> PASS

    igt@drv_selftest@live_hangcheck:
      shard-glk:          DMESG-FAIL (fdo#106560, fdo#106947) -> PASS

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


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

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4370 -> Patchwork_9405

  CI_DRM_4370: e43f851caf5a4b7a30c0ccbdf10fb1df81fa1c75 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4528: 6be300d405de5974b262e8b93a445be4ac618e6a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9405: 934e52033eb267b58490c8255dd17fb4d994fdd4 @ 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_9405/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/psr: Fix race in intel_psr_work()
  2018-06-23  4:45 [PATCH] drm/i915/psr: Fix race in intel_psr_work() Dhinakaran Pandiyan
  2018-06-23  5:14 ` ✓ Fi.CI.BAT: success for " Patchwork
  2018-06-23  6:05 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-06-24  8:54 ` Chris Wilson
  2018-06-25 18:04   ` Dhinakaran Pandiyan
  2 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2018-06-24  8:54 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi

Quoting Dhinakaran Pandiyan (2018-06-23 05:45:06)
> commit 5422b37c907e ("drm/i915/psr: Kill delays when activating psr
> back.") switched from delayed work to the plain variant and while doing so
> remove the check for work_busy() before scheduling a PSR activation.
> This appears to cause consecutive executions of psr_activate() in this
> scenario - after a worker picks up the PSR work item for execution and
> before the work function can acquire the PSR mutex, a psr_flush() can
> get hold of the mutex and schedule another PSR work. Without a psr_exit()
> between two psr_activate() calls, the warning messages get printed.

Ok, that would explain the second work.

> Further, since we drop the mutex in the midst of psr_work() to wait for
> PSR to idle, another work item can also get scheduled. Fix this by
> returning if PSR was already active.
> 
> Note:
> I am not 100% sure this is what is going on as I could not reproduce
> the bug (https://bugs.freedesktop.org/show_bug.cgi?id=106948)
> 
> This patch sort of defeats the point of the WARN_ON()s in psr_activate()
> now, do we still need them?

WARN_ON(active), yup. Seems reasonable to keep for the moment.
WARN_ON(hw_state) would be worth keeping for some time. Hmm, shouldn't
it be checking both PSR_CTL irrespective of intended mode. After
switching psr1 to psr2 we should check that psr1 is disabled as well,
and vice versa.

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/psr: Fix race in intel_psr_work()
  2018-06-24  8:54 ` [PATCH] " Chris Wilson
@ 2018-06-25 18:04   ` Dhinakaran Pandiyan
  0 siblings, 0 replies; 5+ messages in thread
From: Dhinakaran Pandiyan @ 2018-06-25 18:04 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: Rodrigo Vivi

On Sun, 2018-06-24 at 09:54 +0100, Chris Wilson wrote:
> Quoting Dhinakaran Pandiyan (2018-06-23 05:45:06)
> > 
> > commit 5422b37c907e ("drm/i915/psr: Kill delays when activating psr
> > back.") switched from delayed work to the plain variant and while
> > doing so
> > remove the check for work_busy() before scheduling a PSR
> > activation.
> > This appears to cause consecutive executions of psr_activate() in
> > this
> > scenario - after a worker picks up the PSR work item for execution
> > and
> > before the work function can acquire the PSR mutex, a psr_flush()
> > can
> > get hold of the mutex and schedule another PSR work. Without a
> > psr_exit()
> > between two psr_activate() calls, the warning messages get printed.
> Ok, that would explain the second work.
> 
> > 
> > Further, since we drop the mutex in the midst of psr_work() to wait
> > for
> > PSR to idle, another work item can also get scheduled. Fix this by
> > returning if PSR was already active.
> > 
> > Note:
> > I am not 100% sure this is what is going on as I could not
> > reproduce
> > the bug (https://bugs.freedesktop.org/show_bug.cgi?id=106948)
> > 
> > This patch sort of defeats the point of the WARN_ON()s in
> > psr_activate()
> > now, do we still need them?
> WARN_ON(active), yup. Seems reasonable to keep for the moment.
> WARN_ON(hw_state) would be worth keeping for some time. Hmm,
> shouldn't
> it be checking both PSR_CTL irrespective of intended mode. After
> switching psr1 to psr2 we should check that psr1 is disabled as well,
> and vice versa.
> 
Yeah, I've sent a patch to check both PSR_CTL and PSR2_CTL. Thanks for
the review.

-DK

> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> _______________________________________________
> 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] 5+ messages in thread

end of thread, other threads:[~2018-06-25 17:38 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-23  4:45 [PATCH] drm/i915/psr: Fix race in intel_psr_work() Dhinakaran Pandiyan
2018-06-23  5:14 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-06-23  6:05 ` ✓ Fi.CI.IGT: " Patchwork
2018-06-24  8:54 ` [PATCH] " Chris Wilson
2018-06-25 18:04   ` 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.