[PATCH] drm/i915: Double check we didn't miss an unclaimed register access

[PATCH] drm/i915: Double check we didn't miss an unclaimed register access

[Chris Wilson]

Currently, if the user has enabled mmio-debug around each register
access, we presume that we have then checked them all. However, it is
still possible through omission (raw register access) or external
interaction that the unclaimed access was not highlighted.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 20f2f5ad9c3f..05f0cda18501 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -2283,15 +2283,16 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
 bool
 intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
 {
-	if (unlikely(i915_modparams.mmio_debug ||
-		     dev_priv->uncore.unclaimed_mmio_check <= 0))
+	if (unlikely(dev_priv->uncore.unclaimed_mmio_check <= 0))
 		return false;
 
 	if (unlikely(intel_uncore_unclaimed_mmio(dev_priv))) {
-		DRM_DEBUG("Unclaimed register detected, "
-			  "enabling oneshot unclaimed register reporting. "
-			  "Please use i915.mmio_debug=N for more information.\n");
-		i915_modparams.mmio_debug++;
+		if (!i915_modparams.mmio_debug) {
+			DRM_DEBUG("Unclaimed register detected, "
+				  "enabling oneshot unclaimed register reporting. "
+				  "Please use i915.mmio_debug=N for more information.\n");
+			i915_modparams.mmio_debug++;
+		}
 		dev_priv->uncore.unclaimed_mmio_check--;
 		return true;
 	}
-- 
2.19.0.rc1

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

✓ Fi.CI.BAT: success for drm/i915: Double check we didn't miss an unclaimed register access

[Patchwork]

== Series Details ==

Series: drm/i915: Double check we didn't miss an unclaimed register access
URL   : https://patchwork.freedesktop.org/series/49121/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4763 -> Patchwork_10079 =

== Summary - WARNING ==

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

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rpm@module-reload:
      fi-hsw-4770r:       PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload-inject:
      fi-hsw-4770r:       PASS -> DMESG-WARN (fdo#107425)

    igt@gem_exec_suspend@basic-s4-devices:
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    igt@kms_psr@primary_page_flip:
      fi-cnl-psr:         PASS -> FAIL (fdo#107336)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
      fi-skl-guc:         FAIL (fdo#103191) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      fi-byt-clapper:     FAIL (fdo#103191, fdo#107362) -> PASS

    igt@pm_rpm@module-reload:
      fi-cnl-psr:         WARN (fdo#107602, fdo#107708) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107425 https://bugs.freedesktop.org/show_bug.cgi?id=107425
  fdo#107602 https://bugs.freedesktop.org/show_bug.cgi?id=107602
  fdo#107708 https://bugs.freedesktop.org/show_bug.cgi?id=107708
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (52 -> 47) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4763 -> Patchwork_10079

  CI_DRM_4763: 1f8c06844acac7a349fb80471afcc09f33c6cfc0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4622: 022be555443eaa3317da6a9a451cf2c9dfcd6ab8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10079: 905bbb627b514b8a80e0788139f6ba46c8baed0e @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

905bbb627b51 drm/i915: Double check we didn't miss an unclaimed register access

== Logs ==

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

Re: [PATCH] drm/i915: Double check we didn't miss an unclaimed register access

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Currently, if the user has enabled mmio-debug around each register
> access, we presume that we have then checked them all. However, it is
> still possible through omission (raw register access) or external
> interaction that the unclaimed access was not highlighted.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 20f2f5ad9c3f..05f0cda18501 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -2283,15 +2283,16 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
>  bool
>  intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
>  {
> -	if (unlikely(i915_modparams.mmio_debug ||
> -		     dev_priv->uncore.unclaimed_mmio_check <= 0))
> +	if (unlikely(dev_priv->uncore.unclaimed_mmio_check <= 0))
>  		return false;
>

We could catch the readers attention by marking this as READ_ONCE.


And then take spinlock here before checking for unclaimed.

We poke here from hangcheck at unknown intervals and I am
concerned both the trampling on the check values and also
the register access of the unclaimed debug regs.

Which also raises the question that should we just move
the arming check to park/unpark?

-Mika

>  	if (unlikely(intel_uncore_unclaimed_mmio(dev_priv))) {
> -		DRM_DEBUG("Unclaimed register detected, "
> -			  "enabling oneshot unclaimed register reporting. "
> -			  "Please use i915.mmio_debug=N for more information.\n");
> -		i915_modparams.mmio_debug++;
> +		if (!i915_modparams.mmio_debug) {
> +			DRM_DEBUG("Unclaimed register detected, "
> +				  "enabling oneshot unclaimed register reporting. "
> +				  "Please use i915.mmio_debug=N for more information.\n");
> +			i915_modparams.mmio_debug++;
> +		}
>  		dev_priv->uncore.unclaimed_mmio_check--;
>  		return true;
>  	}
> -- 
> 2.19.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Double check we didn't miss an unclaimed register access

[Chris Wilson]

Quoting Mika Kuoppala (2018-09-04 13:34:12)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Currently, if the user has enabled mmio-debug around each register
> > access, we presume that we have then checked them all. However, it is
> > still possible through omission (raw register access) or external
> > interaction that the unclaimed access was not highlighted.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 20f2f5ad9c3f..05f0cda18501 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -2283,15 +2283,16 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
> >  bool
> >  intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
> >  {
> > -     if (unlikely(i915_modparams.mmio_debug ||
> > -                  dev_priv->uncore.unclaimed_mmio_check <= 0))
> > +     if (unlikely(dev_priv->uncore.unclaimed_mmio_check <= 0))
> >               return false;
> >
> 
> We could catch the readers attention by marking this as READ_ONCE.
> 
> 
> And then take spinlock here before checking for unclaimed.

Could do, feels like overkill, but not contentious.
> 
> We poke here from hangcheck at unknown intervals and I am
> concerned both the trampling on the check values and also
> the register access of the unclaimed debug regs.
> 
> Which also raises the question that should we just move
> the arming check to park/unpark?

We still want around modeset, suspend/resume. I'd rather have the
periodic poking off the main thread (i.e. hangcheck) tbh.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Double check we didn't miss an unclaimed register access

[Chris Wilson]

Quoting Chris Wilson (2018-09-04 13:38:27)
> Quoting Mika Kuoppala (2018-09-04 13:34:12)
> > Chris Wilson <chris@chris-wilson.co.uk> writes:
> > 
> > > Currently, if the user has enabled mmio-debug around each register
> > > access, we presume that we have then checked them all. However, it is
> > > still possible through omission (raw register access) or external
> > > interaction that the unclaimed access was not highlighted.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_uncore.c | 13 +++++++------
> > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index 20f2f5ad9c3f..05f0cda18501 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -2283,15 +2283,16 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
> > >  bool
> > >  intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
> > >  {
> > > -     if (unlikely(i915_modparams.mmio_debug ||
> > > -                  dev_priv->uncore.unclaimed_mmio_check <= 0))
> > > +     if (unlikely(dev_priv->uncore.unclaimed_mmio_check <= 0))
> > >               return false;
> > >
> > 
> > We could catch the readers attention by marking this as READ_ONCE.
> > 
> > 
> > And then take spinlock here before checking for unclaimed.
> 
> Could do, feels like overkill, but not contentious.

Implied here, is improving unclaimed_mmio_check a fundamental
requirement for this patch or additional work? I think the latter.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Double check we didn't miss an unclaimed register access

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Chris Wilson (2018-09-04 13:38:27)
>> Quoting Mika Kuoppala (2018-09-04 13:34:12)
>> > Chris Wilson <chris@chris-wilson.co.uk> writes:
>> > 
>> > > Currently, if the user has enabled mmio-debug around each register
>> > > access, we presume that we have then checked them all. However, it is
>> > > still possible through omission (raw register access) or external
>> > > interaction that the unclaimed access was not highlighted.
>> > >
>> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/intel_uncore.c | 13 +++++++------
>> > >  1 file changed, 7 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> > > index 20f2f5ad9c3f..05f0cda18501 100644
>> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
>> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> > > @@ -2283,15 +2283,16 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
>> > >  bool
>> > >  intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
>> > >  {
>> > > -     if (unlikely(i915_modparams.mmio_debug ||
>> > > -                  dev_priv->uncore.unclaimed_mmio_check <= 0))
>> > > +     if (unlikely(dev_priv->uncore.unclaimed_mmio_check <= 0))
>> > >               return false;
>> > >
>> > 
>> > We could catch the readers attention by marking this as READ_ONCE.
>> > 
>> > 
>> > And then take spinlock here before checking for unclaimed.
>> 
>> Could do, feels like overkill, but not contentious.
>
> Implied here, is improving unclaimed_mmio_check a fundamental
> requirement for this patch or additional work? I think the latter.

Additional work. Was just thinking aloud about the
possible races in this area.

Patch does what it says.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Double check we didn't miss an unclaimed register access

[Chris Wilson]

Quoting Mika Kuoppala (2018-09-04 13:56:07)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Chris Wilson (2018-09-04 13:38:27)
> >> Quoting Mika Kuoppala (2018-09-04 13:34:12)
> >> > Chris Wilson <chris@chris-wilson.co.uk> writes:
> >> > 
> >> > > Currently, if the user has enabled mmio-debug around each register
> >> > > access, we presume that we have then checked them all. However, it is
> >> > > still possible through omission (raw register access) or external
> >> > > interaction that the unclaimed access was not highlighted.
> >> > >
> >> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >> > > ---
> >> > >  drivers/gpu/drm/i915/intel_uncore.c | 13 +++++++------
> >> > >  1 file changed, 7 insertions(+), 6 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> >> > > index 20f2f5ad9c3f..05f0cda18501 100644
> >> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> >> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> >> > > @@ -2283,15 +2283,16 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
> >> > >  bool
> >> > >  intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
> >> > >  {
> >> > > -     if (unlikely(i915_modparams.mmio_debug ||
> >> > > -                  dev_priv->uncore.unclaimed_mmio_check <= 0))
> >> > > +     if (unlikely(dev_priv->uncore.unclaimed_mmio_check <= 0))
> >> > >               return false;
> >> > >
> >> > 
> >> > We could catch the readers attention by marking this as READ_ONCE.
> >> > 
> >> > 
> >> > And then take spinlock here before checking for unclaimed.
> >> 
> >> Could do, feels like overkill, but not contentious.
> >
> > Implied here, is improving unclaimed_mmio_check a fundamental
> > requirement for this patch or additional work? I think the latter.
> 
> Additional work. Was just thinking aloud about the
> possible races in this area.
> 
> Patch does what it says.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Plonked itin . When I remember, I'll rerun the mmio-debugging patch for
pm_rpm/module-reload and see what happens now.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Double check we didn't miss an unclaimed register access

[Patchwork]

== Series Details ==

Series: drm/i915: Double check we didn't miss an unclaimed register access
URL   : https://patchwork.freedesktop.org/series/49121/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4763_full -> Patchwork_10079_full =

== Summary - WARNING ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@kms_frontbuffer_tracking@fbc-1p-indfb-fliptrack:
      shard-snb:          SKIP -> PASS

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

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_schedule@pi-ringfull-bsd2:
      shard-kbl:          NOTRUN -> FAIL (fdo#103158)

    igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-shrfb-plflip-blt:
      shard-glk:          PASS -> FAIL (fdo#103167)

    igt@perf@polling:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    
    ==== Possible fixes ====

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

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

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  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_4763 -> Patchwork_10079

  CI_DRM_4763: 1f8c06844acac7a349fb80471afcc09f33c6cfc0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4622: 022be555443eaa3317da6a9a451cf2c9dfcd6ab8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10079: 905bbb627b514b8a80e0788139f6ba46c8baed0e @ 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_10079/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx