| Message ID | 4e087e216d86e6be8109bb443d1ac6c9ced1f777.1688463863.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/uncore: unclaimed reg debug race fix and optimization | expand |
On 04/07/2023 10:48, Jani Nikula wrote: > While the default for the mmio_debug parameter depends on > CONFIG_DRM_I915_DEBUG_MMIO, we look it up and include all the code for > unclaimed reg debugging even when CONFIG_DRM_I915_DEBUG_MMIO=n. Fix it. > > Cc: Lee Shawn C <shawn.c.lee@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_uncore.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index dfefad5a5fec..da2edde4b6f6 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1929,7 +1929,8 @@ static inline bool __must_check > unclaimed_reg_debug_header(struct intel_uncore *uncore, > const i915_reg_t reg, const bool read) > { > - if (likely(!uncore->i915->params.mmio_debug) || !uncore->debug) > + if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO) || > + likely(!uncore->i915->params.mmio_debug) || !uncore->debug) > return false; But now it would not be possible to enable mmio_debug, if Kconfig _default_ is 'n'. What am I missing? Regards, Tvrtko > > /* interrupts are disabled and re-enabled around uncore->lock usage */
On Thu, 06 Jul 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 04/07/2023 10:48, Jani Nikula wrote: >> While the default for the mmio_debug parameter depends on >> CONFIG_DRM_I915_DEBUG_MMIO, we look it up and include all the code for >> unclaimed reg debugging even when CONFIG_DRM_I915_DEBUG_MMIO=n. Fix it. >> >> Cc: Lee Shawn C <shawn.c.lee@intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_uncore.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c >> index dfefad5a5fec..da2edde4b6f6 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -1929,7 +1929,8 @@ static inline bool __must_check >> unclaimed_reg_debug_header(struct intel_uncore *uncore, >> const i915_reg_t reg, const bool read) >> { >> - if (likely(!uncore->i915->params.mmio_debug) || !uncore->debug) >> + if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO) || >> + likely(!uncore->i915->params.mmio_debug) || !uncore->debug) >> return false; > > But now it would not be possible to enable mmio_debug, if Kconfig > _default_ is 'n'. What am I missing? You're not missing anything, I am. *facepalm* The question is, are the first two acceptable without the third? BR, Jani. > > Regards, > > Tvrtko > >> >> /* interrupts are disabled and re-enabled around uncore->lock usage */
On 06/07/2023 13:06, Jani Nikula wrote: > On Thu, 06 Jul 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> On 04/07/2023 10:48, Jani Nikula wrote: >>> While the default for the mmio_debug parameter depends on >>> CONFIG_DRM_I915_DEBUG_MMIO, we look it up and include all the code for >>> unclaimed reg debugging even when CONFIG_DRM_I915_DEBUG_MMIO=n. Fix it. >>> >>> Cc: Lee Shawn C <shawn.c.lee@intel.com> >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_uncore.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c >>> index dfefad5a5fec..da2edde4b6f6 100644 >>> --- a/drivers/gpu/drm/i915/intel_uncore.c >>> +++ b/drivers/gpu/drm/i915/intel_uncore.c >>> @@ -1929,7 +1929,8 @@ static inline bool __must_check >>> unclaimed_reg_debug_header(struct intel_uncore *uncore, >>> const i915_reg_t reg, const bool read) >>> { >>> - if (likely(!uncore->i915->params.mmio_debug) || !uncore->debug) >>> + if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO) || >>> + likely(!uncore->i915->params.mmio_debug) || !uncore->debug) >>> return false; >> >> But now it would not be possible to enable mmio_debug, if Kconfig >> _default_ is 'n'. What am I missing? > > You're not missing anything, I am. *facepalm* > > The question is, are the first two acceptable without the third? What are 1st, 2nd and 3rd in your counting? This area is confusing me a little bit. If I look at unclaimed_reg_debug it appears unclaimed register debug depends on mmio_debug. But if I look at the message output by intel_uncore_arm_unclaimed_mmio_detection it appears that on detecting an unclaimed register we suggest to enable mmio_debug. Isn't that a contradiction? Regards, Tvrtko
>On 06/07/2023 13:06, Jani Nikula wrote: >> On Thu, 06 Jul 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >>> On 04/07/2023 10:48, Jani Nikula wrote: >>>> While the default for the mmio_debug parameter depends on >>>> CONFIG_DRM_I915_DEBUG_MMIO, we look it up and include all the code >>>> for unclaimed reg debugging even when CONFIG_DRM_I915_DEBUG_MMIO=n. Fix it. >>>> >>>> Cc: Lee Shawn C <shawn.c.lee@intel.com> >>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_uncore.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c >>>> b/drivers/gpu/drm/i915/intel_uncore.c >>>> index dfefad5a5fec..da2edde4b6f6 100644 >>>> --- a/drivers/gpu/drm/i915/intel_uncore.c >>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c >>>> @@ -1929,7 +1929,8 @@ static inline bool __must_check >>>> unclaimed_reg_debug_header(struct intel_uncore *uncore, >>>> const i915_reg_t reg, const bool read) >>>> { >>>> - if (likely(!uncore->i915->params.mmio_debug) || !uncore->debug) >>>> + if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO) || >>>> + likely(!uncore->i915->params.mmio_debug) || !uncore->debug) >>>> return false; >>> >>> But now it would not be possible to enable mmio_debug, if Kconfig >>> _default_ is 'n'. What am I missing? >> >> You're not missing anything, I am. *facepalm* >> >> The question is, are the first two acceptable without the third? > >What are 1st, 2nd and 3rd in your counting? > >This area is confusing me a little bit. > >If I look at unclaimed_reg_debug it appears unclaimed register debug depends on mmio_debug. > >But if I look at the message output by >intel_uncore_arm_unclaimed_mmio_detection it appears that on detecting an unclaimed register we suggest to enable mmio_debug. > >Isn't that a contradiction? > >Regards, > >Tvrtko Hi Jani, Tvrtko, We are still waiting for these patches to fix issue. May I get your help to re-visit this series? Thanks! Best regards, Shawn
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index dfefad5a5fec..da2edde4b6f6 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1929,7 +1929,8 @@ static inline bool __must_check unclaimed_reg_debug_header(struct intel_uncore *uncore, const i915_reg_t reg, const bool read) { - if (likely(!uncore->i915->params.mmio_debug) || !uncore->debug) + if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO) || + likely(!uncore->i915->params.mmio_debug) || !uncore->debug) return false; /* interrupts are disabled and re-enabled around uncore->lock usage */
While the default for the mmio_debug parameter depends on CONFIG_DRM_I915_DEBUG_MMIO, we look it up and include all the code for unclaimed reg debugging even when CONFIG_DRM_I915_DEBUG_MMIO=n. Fix it. Cc: Lee Shawn C <shawn.c.lee@intel.com> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_uncore.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)