[Intel-gfx] [RFC 1/3] drm/i915: split out uncore_mmio_debug
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Thu Aug 8 16:43:00 UTC 2019
On 8/8/19 6:08 AM, Mika Kuoppala wrote:
> Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com> writes:
>
>> Multiple uncore structures will share the debug infrastructure, so
>> move it to a common place and add extra locking around it.
>> Also, since we now have a separate object, it is cleaner to have
>> dedicated functions working on the object to stop and restart the
>> mmio debug. Apart from the cosmetic changes, this patch introduces
>> 2 functional updates:
>>
>> - All calls to check_for_unclaimed_mmio will now return false when
>> the debug is suspended, not just the ones that are active only when
>> i915_modparams.mmio_debug is set. If we don't trust the result of the
>> check while a user is doing mmio access then we shouldn't attempt the
>> check anywhere.
>>
>> - i915_modparams.mmio_debug is not save/restored anymore around user
>> access. The value is now never touched by the kernel while debug is
>> disabled so no need for save/restore.
>>
>> v2: squash mmio_debug patches, restrict mmio_debug lock usage (Chris)
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> ---
>> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
>> drivers/gpu/drm/i915/i915_drv.c | 3 +-
>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>> drivers/gpu/drm/i915/intel_uncore.c | 91 ++++++++++++++++++++---------
>> drivers/gpu/drm/i915/intel_uncore.h | 18 +++---
>> 5 files changed, 79 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 3b15266c54fd..2310512111ab 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1129,7 +1129,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data)
>> unsigned int tmp;
>>
>> seq_printf(m, "user.bypass_count = %u\n",
>> - uncore->user_forcewake.count);
>> + uncore->user_forcewake_count);
>>
>> for_each_fw_domain(fw_domain, uncore, tmp)
>> seq_printf(m, "%s.wake_count = %u\n",
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 3480db36b63f..fbbff4a133ba 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -744,6 +744,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>>
>> intel_device_info_subplatform_init(dev_priv);
>>
>> + intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug);
>> intel_uncore_init_early(&dev_priv->uncore, dev_priv);
>>
>> spin_lock_init(&dev_priv->irq_lock);
>> @@ -2044,7 +2045,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>>
>> out:
>> enable_rpm_wakeref_asserts(rpm);
>> - if (!dev_priv->uncore.user_forcewake.count)
>> + if (!dev_priv->uncore.user_forcewake_count)
>> intel_runtime_pm_driver_release(rpm);
>>
>> return ret;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index c9476f24f5c1..13c27a75dca8 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1346,6 +1346,7 @@ struct drm_i915_private {
>> resource_size_t stolen_usable_size; /* Total size minus reserved ranges */
>>
>> struct intel_uncore uncore;
>> + struct intel_uncore_mmio_debug mmio_debug;
>>
>> struct i915_virtual_gpu vgpu;
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 39e8710afdd9..9e583f13a9e4 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -34,6 +34,32 @@
>>
>> #define __raw_posting_read(...) ((void)__raw_uncore_read32(__VA_ARGS__))
>>
>> +void
>> +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug)
>> +{
>> + spin_lock_init(&mmio_debug->lock);
>> + mmio_debug->unclaimed_mmio_check = 1;
>> +}
>> +
>> +static void mmio_debug_suspend(struct intel_uncore_mmio_debug *mmio_debug)
>> +{
>> + lockdep_assert_held(&mmio_debug->lock);
>> +
>> + /* Save and disable mmio debugging for the user bypass */
>> + if (!mmio_debug->suspend_count++) {
>> + mmio_debug->saved_mmio_check = mmio_debug->unclaimed_mmio_check;
>> + mmio_debug->unclaimed_mmio_check = 0;
>> + }
>> +}
>> +
>> +static void mmio_debug_resume(struct intel_uncore_mmio_debug *mmio_debug)
>> +{
>> + lockdep_assert_held(&mmio_debug->lock);
>> +
>> + if (!--mmio_debug->suspend_count)
>> + mmio_debug->unclaimed_mmio_check = mmio_debug->saved_mmio_check;
>> +}
>> +
>> static const char * const forcewake_domain_names[] = {
>> "render",
>> "blitter",
>> @@ -476,6 +502,11 @@ check_for_unclaimed_mmio(struct intel_uncore *uncore)
>> {
>> bool ret = false;
>>
>> + lockdep_assert_held(&uncore->debug->lock);
>> +
>> + if (uncore->debug->suspend_count)
>> + return false;
>> +
>> if (intel_uncore_has_fpga_dbg_unclaimed(uncore))
>> ret |= fpga_check_for_unclaimed_mmio(uncore);
>>
>> @@ -608,17 +639,11 @@ void intel_uncore_forcewake_get(struct intel_uncore *uncore,
>> void intel_uncore_forcewake_user_get(struct intel_uncore *uncore)
>> {
>> spin_lock_irq(&uncore->lock);
>> - if (!uncore->user_forcewake.count++) {
>> + if (!uncore->user_forcewake_count++) {
>> intel_uncore_forcewake_get__locked(uncore, FORCEWAKE_ALL);
>> -
>> - /* Save and disable mmio debugging for the user bypass */
>> - uncore->user_forcewake.saved_mmio_check =
>> - uncore->unclaimed_mmio_check;
>> - uncore->user_forcewake.saved_mmio_debug =
>> - i915_modparams.mmio_debug;
>> -
>> - uncore->unclaimed_mmio_check = 0;
>> - i915_modparams.mmio_debug = 0;
>> + spin_lock(&uncore->debug->lock);
>
> For me it looks like you need spin_lock_irq here also
> like with the parent lock above.
The parent lock should ensure that the irqs are already off at this
point, so no need to disable them twice. Also, spin_unlock_irq can't be
called twice because the first of the 2 calls would re-enable interrupts
while the second lock is still taken. Irqflags would solve that, but
still wouldn't give any benefit compared to a straight lock IMO.
Daniele
> -Mika
>
>> + mmio_debug_suspend(uncore->debug);
>> + spin_unlock(&uncore->debug->lock);
>> }
>> spin_unlock_irq(&uncore->lock);
>> }
>> @@ -633,15 +658,14 @@ void intel_uncore_forcewake_user_get(struct intel_uncore *uncore)
>> void intel_uncore_forcewake_user_put(struct intel_uncore *uncore)
>> {
>> spin_lock_irq(&uncore->lock);
>> - if (!--uncore->user_forcewake.count) {
>> - if (intel_uncore_unclaimed_mmio(uncore))
>> + if (!--uncore->user_forcewake_count) {
>> + spin_lock(&uncore->debug->lock);
>> + mmio_debug_resume(uncore->debug);
>> +
>> + if (check_for_unclaimed_mmio(uncore))
>> dev_info(uncore->i915->drm.dev,
>> "Invalid mmio detected during user access\n");
>> -
>> - uncore->unclaimed_mmio_check =
>> - uncore->user_forcewake.saved_mmio_check;
>> - i915_modparams.mmio_debug =
>> - uncore->user_forcewake.saved_mmio_debug;
>> + spin_unlock(&uncore->debug->lock);
>>
>> intel_uncore_forcewake_put__locked(uncore, FORCEWAKE_ALL);
>> }
>> @@ -1088,7 +1112,16 @@ unclaimed_reg_debug(struct intel_uncore *uncore,
>> if (likely(!i915_modparams.mmio_debug))
>> return;
>>
>> + /* interrupts are disabled and re-enabled around uncore->lock usage */
>> + lockdep_assert_held(&uncore->lock);
>> +
>> + if (before)
>> + spin_lock(&uncore->debug->lock);
>> +
>> __unclaimed_reg_debug(uncore, reg, read, before);
>> +
>> + if (!before)
>> + spin_unlock(&uncore->debug->lock);
>> }
>>
>> #define GEN2_READ_HEADER(x) \
>> @@ -1607,6 +1640,7 @@ void intel_uncore_init_early(struct intel_uncore *uncore,
>> spin_lock_init(&uncore->lock);
>> uncore->i915 = i915;
>> uncore->rpm = &i915->runtime_pm;
>> + uncore->debug = &i915->mmio_debug;
>> }
>>
>> static void uncore_raw_init(struct intel_uncore *uncore)
>> @@ -1632,7 +1666,6 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
>> ret = intel_uncore_fw_domains_init(uncore);
>> if (ret)
>> return ret;
>> -
>> forcewake_early_sanitize(uncore, 0);
>>
>> if (IS_GEN_RANGE(i915, 6, 7)) {
>> @@ -1681,8 +1714,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>> if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
>> uncore->flags |= UNCORE_HAS_FORCEWAKE;
>>
>> - uncore->unclaimed_mmio_check = 1;
>> -
>> if (!intel_uncore_has_forcewake(uncore)) {
>> uncore_raw_init(uncore);
>> } else {
>> @@ -1707,7 +1738,7 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>> uncore->flags |= UNCORE_HAS_FIFO;
>>
>> /* clear out unclaimed reg detection bit */
>> - if (check_for_unclaimed_mmio(uncore))
>> + if (intel_uncore_unclaimed_mmio(uncore))
>> DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
>>
>> return 0;
>> @@ -1952,7 +1983,13 @@ int __intel_wait_for_register(struct intel_uncore *uncore,
>>
>> bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore)
>> {
>> - return check_for_unclaimed_mmio(uncore);
>> + bool ret;
>> +
>> + spin_lock_irq(&uncore->debug->lock);
>> + ret = check_for_unclaimed_mmio(uncore);
>> + spin_unlock_irq(&uncore->debug->lock);
>> +
>> + return ret;
>> }
>>
>> bool
>> @@ -1960,24 +1997,24 @@ intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore)
>> {
>> bool ret = false;
>>
>> - spin_lock_irq(&uncore->lock);
>> + spin_lock_irq(&uncore->debug->lock);
>>
>> - if (unlikely(uncore->unclaimed_mmio_check <= 0))
>> + if (unlikely(uncore->debug->unclaimed_mmio_check <= 0))
>> goto out;
>>
>> - if (unlikely(intel_uncore_unclaimed_mmio(uncore))) {
>> + if (unlikely(check_for_unclaimed_mmio(uncore))) {
>> 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++;
>> }
>> - uncore->unclaimed_mmio_check--;
>> + uncore->debug->unclaimed_mmio_check--;
>> ret = true;
>> }
>>
>> out:
>> - spin_unlock_irq(&uncore->lock);
>> + spin_unlock_irq(&uncore->debug->lock);
>>
>> return ret;
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
>> index e603d210a34d..414fc2cb0459 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.h
>> +++ b/drivers/gpu/drm/i915/intel_uncore.h
>> @@ -36,6 +36,13 @@ struct drm_i915_private;
>> struct intel_runtime_pm;
>> struct intel_uncore;
>>
>> +struct intel_uncore_mmio_debug {
>> + spinlock_t lock; /** lock is also taken in irq contexts. */
>> + int unclaimed_mmio_check;
>> + int saved_mmio_check;
>> + u32 suspend_count;
>> +};
>> +
>> enum forcewake_domain_id {
>> FW_DOMAIN_ID_RENDER = 0,
>> FW_DOMAIN_ID_BLITTER,
>> @@ -137,14 +144,9 @@ struct intel_uncore {
>> u32 __iomem *reg_ack;
>> } *fw_domain[FW_DOMAIN_ID_COUNT];
>>
>> - struct {
>> - unsigned int count;
>> -
>> - int saved_mmio_check;
>> - int saved_mmio_debug;
>> - } user_forcewake;
>> + unsigned int user_forcewake_count;
>>
>> - int unclaimed_mmio_check;
>> + struct intel_uncore_mmio_debug *debug;
>> };
>>
>> /* Iterate over initialised fw domains */
>> @@ -179,6 +181,8 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
>> return uncore->flags & UNCORE_HAS_FIFO;
>> }
>>
>> +void
>> +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug);
>> void intel_uncore_init_early(struct intel_uncore *uncore,
>> struct drm_i915_private *i915);
>> int intel_uncore_init_mmio(struct intel_uncore *uncore);
>> --
>> 2.22.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list