Re: [Intel-gfx] [PATCH v4] drm/i915/uncore: Use GT message helpers in uncore
On Thu, Jan 26, 2023 at 10:46:01AM +, Tvrtko Ursulin wrote:
>
> On 25/01/2023 19:04, Matt Roper wrote:
> > On Wed, Jan 25, 2023 at 10:51:53AM +, Tvrtko Ursulin wrote:
> > >
> > > On 24/01/2023 20:54, john.c.harri...@intel.com wrote:
> > > > From: John Harrison
> > > >
> > > > Uncore is really part of the GT. So use the GT specific debug/error
> >
> > That's not really true; uncore should be outside the GT since it's used
> > for all kinds of non-GT stuff as well (sgunit, display, etc.). I
> > believe "uncore" is just an old-fashioned name for what modern docs
> > refer to as "system agent" these days.
> >
> > Granted, our i915 design does stretch the truth quite a bit today by
> > rolling some of the GT-specific concepts into the uncore code (GT
> > forcewake/shadowing, GSI offset, etc.). Having two intel_uncore
> > structures on a platform like MTL doesn't really match the hardware
> > reality at the lowest levels, but allows us to update the software for
> > these new platforms without major, intrusive changes for all platforms.
> >
> > I feel like including 'gt' information in log messages unrelated to GT
> > might be confusing. For display stuff it's probably obvious that the GT
> > information is bogus, but when stuff is related to the sgunit it won't
> > always be so obvious.
>
> Level of confusing vs absence of debugability and a suggested way forward?
> Just do nothing and accept any forcewake related errors will not include the
> originating GT?
I guess it's probably fine to keep it on all messages; people will just
learn to ignore the bogus GT stuff on things that aren't actually
related to the GT.
It would still be good to change the commit message though so that
people doing git archaeology in the future don't get an incorrect
understanding of the relationship.
Matt
>
> Regards,
>
> Tvrtko
>
> >
> >
> > Matt
> >
> > > > message helpers so as to get the GT index in the prints.
> > >
> > > Conversion looks good to me and on balance it's better to include the
> > > origin
> > > in logs even for messages which strictly are not GT related, than not to
> > > have the origin at all (intel_de_... helpers, I *think*).
> > >
> > > Reviewed-by: Tvrtko Ursulin
> > >
> > > I'll just add Jani and Matt in case they have a different opinion.
> > >
> > > Regards,
> > >
> > > Tvrtko
> > >
> > > > Signed-off-by: John Harrison
> > > > ---
> > > >drivers/gpu/drm/i915/intel_uncore.c | 133
> > > > +---
> > > >1 file changed, 63 insertions(+), 70 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> > > > b/drivers/gpu/drm/i915/intel_uncore.c
> > > > index 8dee9e62a73ee..4e357477c6592 100644
> > > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > > @@ -25,6 +25,7 @@
> > > >#include
> > > >#include "gt/intel_engine_regs.h"
> > > > +#include "gt/intel_gt_print.h"
> > > >#include "gt/intel_gt_regs.h"
> > > >#include "i915_drv.h"
> > > > @@ -83,8 +84,7 @@ static void mmio_debug_resume(struct intel_uncore
> > > > *uncore)
> > > > uncore->debug->unclaimed_mmio_check =
> > > > uncore->debug->saved_mmio_check;
> > > > if (check_for_unclaimed_mmio(uncore))
> > > > - drm_info(&uncore->i915->drm,
> > > > -"Invalid mmio detected during user access\n");
> > > > + gt_info(uncore->gt, "Invalid mmio detected during user
> > > > access\n");
> > > > spin_unlock(&uncore->debug->lock);
> > > >}
> > > > @@ -179,9 +179,9 @@ static inline void
> > > >fw_domain_wait_ack_clear(const struct intel_uncore_forcewake_domain
> > > > *d)
> > > >{
> > > > if (wait_ack_clear(d, FORCEWAKE_KERNEL)) {
> > > > - drm_err(&d->uncore->i915->drm,
> > > > - "%s: timed out waiting for forcewake ack to
> > > > clear.\n",
> > > > - intel_uncore_forcewake_domain_to_str(d->id));
> > > > + gt_err(d->uncore->gt,
> > > > + "%s: timed out waiting for forcewake ack to
> > > > clear.\n",
> > > > + intel_uncore_forcewake_domain_to_str(d->id));
> > > > add_taint_for_CI(d->uncore->i915, TAINT_WARN); /* CI
> > > > now unreliable */
> > > > }
> > > >}
> > > > @@ -228,12 +228,11 @@ fw_domain_wait_ack_with_fallback(const struct
> > > > intel_uncore_forcewake_domain *d,
> > > > fw_clear(d, FORCEWAKE_KERNEL_FALLBACK);
> > > > } while (!ack_detected && pass++ < 10);
> > > > - drm_dbg(&d->uncore->i915->drm,
> > > > - "%s had to use fallback to %s ack, 0x%x (passes %u)\n",
> > > > - intel_uncore_forcewake_domain_to_str(d->id),
> > > > - type == ACK_SET ? "set" : "clear",
> > > > - fw_ack(d),
> > > > - pass);
> > > > + gt_dbg(d->uncore->gt, "%s had to use fallback to
Re: [Intel-gfx] [PATCH v4] drm/i915/uncore: Use GT message helpers in uncore
On 25/01/2023 19:04, Matt Roper wrote:
On Wed, Jan 25, 2023 at 10:51:53AM +, Tvrtko Ursulin wrote:
On 24/01/2023 20:54, john.c.harri...@intel.com wrote:
From: John Harrison
Uncore is really part of the GT. So use the GT specific debug/error
That's not really true; uncore should be outside the GT since it's used
for all kinds of non-GT stuff as well (sgunit, display, etc.). I
believe "uncore" is just an old-fashioned name for what modern docs
refer to as "system agent" these days.
Granted, our i915 design does stretch the truth quite a bit today by
rolling some of the GT-specific concepts into the uncore code (GT
forcewake/shadowing, GSI offset, etc.). Having two intel_uncore
structures on a platform like MTL doesn't really match the hardware
reality at the lowest levels, but allows us to update the software for
these new platforms without major, intrusive changes for all platforms.
I feel like including 'gt' information in log messages unrelated to GT
might be confusing. For display stuff it's probably obvious that the GT
information is bogus, but when stuff is related to the sgunit it won't
always be so obvious.
Level of confusing vs absence of debugability and a suggested way
forward? Just do nothing and accept any forcewake related errors will
not include the originating GT?
Regards,
Tvrtko
Matt
message helpers so as to get the GT index in the prints.
Conversion looks good to me and on balance it's better to include the origin
in logs even for messages which strictly are not GT related, than not to
have the origin at all (intel_de_... helpers, I *think*).
Reviewed-by: Tvrtko Ursulin
I'll just add Jani and Matt in case they have a different opinion.
Regards,
Tvrtko
Signed-off-by: John Harrison
---
drivers/gpu/drm/i915/intel_uncore.c | 133 +---
1 file changed, 63 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c
b/drivers/gpu/drm/i915/intel_uncore.c
index 8dee9e62a73ee..4e357477c6592 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -25,6 +25,7 @@
#include
#include "gt/intel_engine_regs.h"
+#include "gt/intel_gt_print.h"
#include "gt/intel_gt_regs.h"
#include "i915_drv.h"
@@ -83,8 +84,7 @@ static void mmio_debug_resume(struct intel_uncore *uncore)
uncore->debug->unclaimed_mmio_check =
uncore->debug->saved_mmio_check;
if (check_for_unclaimed_mmio(uncore))
- drm_info(&uncore->i915->drm,
-"Invalid mmio detected during user access\n");
+ gt_info(uncore->gt, "Invalid mmio detected during user
access\n");
spin_unlock(&uncore->debug->lock);
}
@@ -179,9 +179,9 @@ static inline void
fw_domain_wait_ack_clear(const struct intel_uncore_forcewake_domain *d)
{
if (wait_ack_clear(d, FORCEWAKE_KERNEL)) {
- drm_err(&d->uncore->i915->drm,
- "%s: timed out waiting for forcewake ack to clear.\n",
- intel_uncore_forcewake_domain_to_str(d->id));
+ gt_err(d->uncore->gt,
+ "%s: timed out waiting for forcewake ack to clear.\n",
+ intel_uncore_forcewake_domain_to_str(d->id));
add_taint_for_CI(d->uncore->i915, TAINT_WARN); /* CI now
unreliable */
}
}
@@ -228,12 +228,11 @@ fw_domain_wait_ack_with_fallback(const struct
intel_uncore_forcewake_domain *d,
fw_clear(d, FORCEWAKE_KERNEL_FALLBACK);
} while (!ack_detected && pass++ < 10);
- drm_dbg(&d->uncore->i915->drm,
- "%s had to use fallback to %s ack, 0x%x (passes %u)\n",
- intel_uncore_forcewake_domain_to_str(d->id),
- type == ACK_SET ? "set" : "clear",
- fw_ack(d),
- pass);
+ gt_dbg(d->uncore->gt, "%s had to use fallback to %s ack, 0x%x (passes
%u)\n",
+ intel_uncore_forcewake_domain_to_str(d->id),
+ type == ACK_SET ? "set" : "clear",
+ fw_ack(d),
+ pass);
return ack_detected ? 0 : -ETIMEDOUT;
}
@@ -258,9 +257,8 @@ static inline void
fw_domain_wait_ack_set(const struct intel_uncore_forcewake_domain *d)
{
if (wait_ack_set(d, FORCEWAKE_KERNEL)) {
- drm_err(&d->uncore->i915->drm,
- "%s: timed out waiting for forcewake ack request.\n",
- intel_uncore_forcewake_domain_to_str(d->id));
+ gt_err(d->uncore->gt, "%s: timed out waiting for forcewake ack
request.\n",
+ intel_uncore_forcewake_domain_to_str(d->id));
add_taint_for_CI(d->uncore->i915, TAINT_WARN); /* CI now
unreliable */
}
}
@@ -366,9 +364,9 @@ static void __gen6_gt_wait_for_thread_c0(struct
intel_uncore *uncore)
* w/a for a sporadic read returning 0 by waiting for the GT
* thread to wake up.
*/
- d
Re: [Intel-gfx] [PATCH v4] drm/i915/uncore: Use GT message helpers in uncore
On Wed, Jan 25, 2023 at 10:51:53AM +, Tvrtko Ursulin wrote:
>
> On 24/01/2023 20:54, john.c.harri...@intel.com wrote:
> > From: John Harrison
> >
> > Uncore is really part of the GT. So use the GT specific debug/error
That's not really true; uncore should be outside the GT since it's used
for all kinds of non-GT stuff as well (sgunit, display, etc.). I
believe "uncore" is just an old-fashioned name for what modern docs
refer to as "system agent" these days.
Granted, our i915 design does stretch the truth quite a bit today by
rolling some of the GT-specific concepts into the uncore code (GT
forcewake/shadowing, GSI offset, etc.). Having two intel_uncore
structures on a platform like MTL doesn't really match the hardware
reality at the lowest levels, but allows us to update the software for
these new platforms without major, intrusive changes for all platforms.
I feel like including 'gt' information in log messages unrelated to GT
might be confusing. For display stuff it's probably obvious that the GT
information is bogus, but when stuff is related to the sgunit it won't
always be so obvious.
Matt
> > message helpers so as to get the GT index in the prints.
>
> Conversion looks good to me and on balance it's better to include the origin
> in logs even for messages which strictly are not GT related, than not to
> have the origin at all (intel_de_... helpers, I *think*).
>
> Reviewed-by: Tvrtko Ursulin
>
> I'll just add Jani and Matt in case they have a different opinion.
>
> Regards,
>
> Tvrtko
>
> > Signed-off-by: John Harrison
> > ---
> > drivers/gpu/drm/i915/intel_uncore.c | 133 +---
> > 1 file changed, 63 insertions(+), 70 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> > b/drivers/gpu/drm/i915/intel_uncore.c
> > index 8dee9e62a73ee..4e357477c6592 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -25,6 +25,7 @@
> > #include
> > #include "gt/intel_engine_regs.h"
> > +#include "gt/intel_gt_print.h"
> > #include "gt/intel_gt_regs.h"
> > #include "i915_drv.h"
> > @@ -83,8 +84,7 @@ static void mmio_debug_resume(struct intel_uncore *uncore)
> > uncore->debug->unclaimed_mmio_check =
> > uncore->debug->saved_mmio_check;
> > if (check_for_unclaimed_mmio(uncore))
> > - drm_info(&uncore->i915->drm,
> > -"Invalid mmio detected during user access\n");
> > + gt_info(uncore->gt, "Invalid mmio detected during user
> > access\n");
> > spin_unlock(&uncore->debug->lock);
> > }
> > @@ -179,9 +179,9 @@ static inline void
> > fw_domain_wait_ack_clear(const struct intel_uncore_forcewake_domain *d)
> > {
> > if (wait_ack_clear(d, FORCEWAKE_KERNEL)) {
> > - drm_err(&d->uncore->i915->drm,
> > - "%s: timed out waiting for forcewake ack to clear.\n",
> > - intel_uncore_forcewake_domain_to_str(d->id));
> > + gt_err(d->uncore->gt,
> > + "%s: timed out waiting for forcewake ack to clear.\n",
> > + intel_uncore_forcewake_domain_to_str(d->id));
> > add_taint_for_CI(d->uncore->i915, TAINT_WARN); /* CI now
> > unreliable */
> > }
> > }
> > @@ -228,12 +228,11 @@ fw_domain_wait_ack_with_fallback(const struct
> > intel_uncore_forcewake_domain *d,
> > fw_clear(d, FORCEWAKE_KERNEL_FALLBACK);
> > } while (!ack_detected && pass++ < 10);
> > - drm_dbg(&d->uncore->i915->drm,
> > - "%s had to use fallback to %s ack, 0x%x (passes %u)\n",
> > - intel_uncore_forcewake_domain_to_str(d->id),
> > - type == ACK_SET ? "set" : "clear",
> > - fw_ack(d),
> > - pass);
> > + gt_dbg(d->uncore->gt, "%s had to use fallback to %s ack, 0x%x (passes
> > %u)\n",
> > + intel_uncore_forcewake_domain_to_str(d->id),
> > + type == ACK_SET ? "set" : "clear",
> > + fw_ack(d),
> > + pass);
> > return ack_detected ? 0 : -ETIMEDOUT;
> > }
> > @@ -258,9 +257,8 @@ static inline void
> > fw_domain_wait_ack_set(const struct intel_uncore_forcewake_domain *d)
> > {
> > if (wait_ack_set(d, FORCEWAKE_KERNEL)) {
> > - drm_err(&d->uncore->i915->drm,
> > - "%s: timed out waiting for forcewake ack request.\n",
> > - intel_uncore_forcewake_domain_to_str(d->id));
> > + gt_err(d->uncore->gt, "%s: timed out waiting for forcewake ack
> > request.\n",
> > + intel_uncore_forcewake_domain_to_str(d->id));
> > add_taint_for_CI(d->uncore->i915, TAINT_WARN); /* CI now
> > unreliable */
> > }
> > }
> > @@ -366,9 +364,9 @@ static void __gen6_gt_wait_for_thread_c0(struct
> > intel_uncore *uncore)
> > * w/a for a sporadic read returning 0 by waiting for the GT
> > * thread to wake up.
> > */
> > - drm_WARN_ONCE(&uncore->i915->drm,
> > - wait_fo
Re: [Intel-gfx] [PATCH v4] drm/i915/uncore: Use GT message helpers in uncore
On 24/01/2023 20:54, john.c.harri...@intel.com wrote:
From: John Harrison
Uncore is really part of the GT. So use the GT specific debug/error
message helpers so as to get the GT index in the prints.
Conversion looks good to me and on balance it's better to include the
origin in logs even for messages which strictly are not GT related, than
not to have the origin at all (intel_de_... helpers, I *think*).
Reviewed-by: Tvrtko Ursulin
I'll just add Jani and Matt in case they have a different opinion.
Regards,
Tvrtko
Signed-off-by: John Harrison
---
drivers/gpu/drm/i915/intel_uncore.c | 133 +---
1 file changed, 63 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c
b/drivers/gpu/drm/i915/intel_uncore.c
index 8dee9e62a73ee..4e357477c6592 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -25,6 +25,7 @@
#include
#include "gt/intel_engine_regs.h"
+#include "gt/intel_gt_print.h"
#include "gt/intel_gt_regs.h"
#include "i915_drv.h"
@@ -83,8 +84,7 @@ static void mmio_debug_resume(struct intel_uncore *uncore)
uncore->debug->unclaimed_mmio_check =
uncore->debug->saved_mmio_check;
if (check_for_unclaimed_mmio(uncore))
- drm_info(&uncore->i915->drm,
-"Invalid mmio detected during user access\n");
+ gt_info(uncore->gt, "Invalid mmio detected during user
access\n");
spin_unlock(&uncore->debug->lock);
}
@@ -179,9 +179,9 @@ static inline void
fw_domain_wait_ack_clear(const struct intel_uncore_forcewake_domain *d)
{
if (wait_ack_clear(d, FORCEWAKE_KERNEL)) {
- drm_err(&d->uncore->i915->drm,
- "%s: timed out waiting for forcewake ack to clear.\n",
- intel_uncore_forcewake_domain_to_str(d->id));
+ gt_err(d->uncore->gt,
+ "%s: timed out waiting for forcewake ack to clear.\n",
+ intel_uncore_forcewake_domain_to_str(d->id));
add_taint_for_CI(d->uncore->i915, TAINT_WARN); /* CI now
unreliable */
}
}
@@ -228,12 +228,11 @@ fw_domain_wait_ack_with_fallback(const struct
intel_uncore_forcewake_domain *d,
fw_clear(d, FORCEWAKE_KERNEL_FALLBACK);
} while (!ack_detected && pass++ < 10);
- drm_dbg(&d->uncore->i915->drm,
- "%s had to use fallback to %s ack, 0x%x (passes %u)\n",
- intel_uncore_forcewake_domain_to_str(d->id),
- type == ACK_SET ? "set" : "clear",
- fw_ack(d),
- pass);
+ gt_dbg(d->uncore->gt, "%s had to use fallback to %s ack, 0x%x (passes
%u)\n",
+ intel_uncore_forcewake_domain_to_str(d->id),
+ type == ACK_SET ? "set" : "clear",
+ fw_ack(d),
+ pass);
return ack_detected ? 0 : -ETIMEDOUT;
}
@@ -258,9 +257,8 @@ static inline void
fw_domain_wait_ack_set(const struct intel_uncore_forcewake_domain *d)
{
if (wait_ack_set(d, FORCEWAKE_KERNEL)) {
- drm_err(&d->uncore->i915->drm,
- "%s: timed out waiting for forcewake ack request.\n",
- intel_uncore_forcewake_domain_to_str(d->id));
+ gt_err(d->uncore->gt, "%s: timed out waiting for forcewake ack
request.\n",
+ intel_uncore_forcewake_domain_to_str(d->id));
add_taint_for_CI(d->uncore->i915, TAINT_WARN); /* CI now
unreliable */
}
}
@@ -366,9 +364,9 @@ static void __gen6_gt_wait_for_thread_c0(struct
intel_uncore *uncore)
* w/a for a sporadic read returning 0 by waiting for the GT
* thread to wake up.
*/
- drm_WARN_ONCE(&uncore->i915->drm,
- wait_for_atomic_us(gt_thread_status(uncore) == 0, 5000),
- "GT thread status wait timed out\n");
+ gt_WARN_ONCE(uncore->gt,
+wait_for_atomic_us(gt_thread_status(uncore) == 0, 5000),
+"GT thread status wait timed out\n");
}
static void fw_domains_get_with_thread_status(struct intel_uncore *uncore,
@@ -402,8 +400,7 @@ static void __gen6_gt_wait_for_fifo(struct intel_uncore
*uncore)
if (wait_for_atomic((n = fifo_free_entries(uncore)) >
GT_FIFO_NUM_RESERVED_ENTRIES,
GT_FIFO_TIMEOUT_MS)) {
- drm_dbg(&uncore->i915->drm,
- "GT_FIFO timeout, entries: %u\n", n);
+ gt_dbg(uncore->gt, "GT_FIFO timeout, entries: %u\n", n);
return;
}
}
@@ -476,7 +473,7 @@ intel_uncore_forcewake_reset(struct intel_uncore *uncore)
break;
if (--retry_count == 0) {
- drm_err(&uncore->i915->drm, "Timed out waiting for forcewake
timers to fin
