* [PATCH 0/2] A few CT updates @ 2022-01-20 18:36 Matthew Brost 2022-01-20 18:36 ` [PATCH 1/2] drm/i915/guc: Don't check CT descriptor status before CT write / read Matthew Brost 2022-01-20 18:36 ` [PATCH 2/2] drm/i915/guc: Print CT descriptor status in CT debug function Matthew Brost 0 siblings, 2 replies; 6+ messages in thread From: Matthew Brost @ 2022-01-20 18:36 UTC (permalink / raw) To: intel-gfx, dri-devel Cc: daniele.ceraolospurio, john.c.harrison, michal.wajdeczko A couple of minor CT updates. 1 for performance, 1 for extra debug. Signed-off-by: Matthew Brost <matthew.brost@intel.com> Matthew Brost (2): drm/i915/guc: Don't check CT descriptor status before CT write / read drm/i915/guc: Print CT descriptor status in CT debug function drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 10 ++++++++++ 1 file changed, 10 insertions(+) -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] drm/i915/guc: Don't check CT descriptor status before CT write / read 2022-01-20 18:36 [PATCH 0/2] A few CT updates Matthew Brost @ 2022-01-20 18:36 ` Matthew Brost 2022-01-21 7:28 ` Jani Nikula 2022-01-20 18:36 ` [PATCH 2/2] drm/i915/guc: Print CT descriptor status in CT debug function Matthew Brost 1 sibling, 1 reply; 6+ messages in thread From: Matthew Brost @ 2022-01-20 18:36 UTC (permalink / raw) To: intel-gfx, dri-devel Cc: daniele.ceraolospurio, john.c.harrison, michal.wajdeczko Don't check CT descriptor status, unless CONFIG_DRM_I915_DEBUG_GUC is set, before CT write / read as this could result in a read across the PCIe bus thus adding latency to every CT write / read. On well behavied systems this vaue should always read as zero. For some reason it doesn't the CT channel is broken and will eventually recover from a GT reset, albeit the GT reset will not be triggered immediately by seeing that descriptor status is non-zero. v2: (CI) - Fix build error (hide corrupted label in write function behind CONFIG_DRM_I915_DEBUG_GUC) Signed-off-by: Matthew Brost <matthew.brost@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index de89d40abd38d..948cf31429412 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -379,8 +379,10 @@ static int ct_write(struct intel_guc_ct *ct, u32 *cmds = ctb->cmds; unsigned int i; +#ifdef CONFIG_DRM_I915_DEBUG_GUC if (unlikely(desc->status)) goto corrupted; +#endif GEM_BUG_ON(tail > size); @@ -445,11 +447,13 @@ static int ct_write(struct intel_guc_ct *ct, return 0; +#ifdef CONFIG_DRM_I915_DEBUG_GUC corrupted: CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u status=%#x\n", desc->head, desc->tail, desc->status); ctb->broken = true; return -EPIPE; +#endif } /** @@ -815,8 +819,10 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) if (unlikely(ctb->broken)) return -EPIPE; +#ifdef CONFIG_DRM_I915_DEBUG_GUC if (unlikely(desc->status)) goto corrupted; +#endif GEM_BUG_ON(head > size); -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915/guc: Don't check CT descriptor status before CT write / read 2022-01-20 18:36 ` [PATCH 1/2] drm/i915/guc: Don't check CT descriptor status before CT write / read Matthew Brost @ 2022-01-21 7:28 ` Jani Nikula 2022-01-21 21:03 ` Matthew Brost 0 siblings, 1 reply; 6+ messages in thread From: Jani Nikula @ 2022-01-21 7:28 UTC (permalink / raw) To: Matthew Brost, intel-gfx, dri-devel Cc: daniele.ceraolospurio, john.c.harrison, michal.wajdeczko On Thu, 20 Jan 2022, Matthew Brost <matthew.brost@intel.com> wrote: > Don't check CT descriptor status, unless CONFIG_DRM_I915_DEBUG_GUC is > set, before CT write / read as this could result in a read across the > PCIe bus thus adding latency to every CT write / read. On well behavied > systems this vaue should always read as zero. For some reason it doesn't > the CT channel is broken and will eventually recover from a GT reset, > albeit the GT reset will not be triggered immediately by seeing that > descriptor status is non-zero. > > v2: > (CI) > - Fix build error (hide corrupted label in write function behind > CONFIG_DRM_I915_DEBUG_GUC) > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > index de89d40abd38d..948cf31429412 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > @@ -379,8 +379,10 @@ static int ct_write(struct intel_guc_ct *ct, > u32 *cmds = ctb->cmds; > unsigned int i; > > +#ifdef CONFIG_DRM_I915_DEBUG_GUC > if (unlikely(desc->status)) > goto corrupted; > +#endif Please don't add #ifdefs inline. You can use IS_ENABLED(CONFIG_DRM_I915_DEBUG_GUC) in if statements, but otherwise the code needs to be split out to a separate function. BR, Jani. > > GEM_BUG_ON(tail > size); > > @@ -445,11 +447,13 @@ static int ct_write(struct intel_guc_ct *ct, > > return 0; > > +#ifdef CONFIG_DRM_I915_DEBUG_GUC > corrupted: > CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u status=%#x\n", > desc->head, desc->tail, desc->status); > ctb->broken = true; > return -EPIPE; > +#endif > } > > /** > @@ -815,8 +819,10 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) > if (unlikely(ctb->broken)) > return -EPIPE; > > +#ifdef CONFIG_DRM_I915_DEBUG_GUC > if (unlikely(desc->status)) > goto corrupted; > +#endif > > GEM_BUG_ON(head > size); -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915/guc: Don't check CT descriptor status before CT write / read 2022-01-21 7:28 ` Jani Nikula @ 2022-01-21 21:03 ` Matthew Brost 2022-01-24 13:07 ` Jani Nikula 0 siblings, 1 reply; 6+ messages in thread From: Matthew Brost @ 2022-01-21 21:03 UTC (permalink / raw) To: Jani Nikula Cc: intel-gfx, daniele.ceraolospurio, john.c.harrison, dri-devel, michal.wajdeczko On Fri, Jan 21, 2022 at 09:28:46AM +0200, Jani Nikula wrote: > On Thu, 20 Jan 2022, Matthew Brost <matthew.brost@intel.com> wrote: > > Don't check CT descriptor status, unless CONFIG_DRM_I915_DEBUG_GUC is > > set, before CT write / read as this could result in a read across the > > PCIe bus thus adding latency to every CT write / read. On well behavied > > systems this vaue should always read as zero. For some reason it doesn't > > the CT channel is broken and will eventually recover from a GT reset, > > albeit the GT reset will not be triggered immediately by seeing that > > descriptor status is non-zero. > > > > v2: > > (CI) > > - Fix build error (hide corrupted label in write function behind > > CONFIG_DRM_I915_DEBUG_GUC) > > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > --- > > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > > index de89d40abd38d..948cf31429412 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > > @@ -379,8 +379,10 @@ static int ct_write(struct intel_guc_ct *ct, > > u32 *cmds = ctb->cmds; > > unsigned int i; > > > > +#ifdef CONFIG_DRM_I915_DEBUG_GUC > > if (unlikely(desc->status)) > > goto corrupted; > > +#endif > > Please don't add #ifdefs inline. You can use > IS_ENABLED(CONFIG_DRM_I915_DEBUG_GUC) in if statements, but otherwise > the code needs to be split out to a separate function. > Sure, but I feel like I've actually been by someone else to not use the IS_ENABLED macro and use ifdefs inlines... Matt > BR, > Jani. > > > > > GEM_BUG_ON(tail > size); > > > > @@ -445,11 +447,13 @@ static int ct_write(struct intel_guc_ct *ct, > > > > return 0; > > > > +#ifdef CONFIG_DRM_I915_DEBUG_GUC > > corrupted: > > CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u status=%#x\n", > > desc->head, desc->tail, desc->status); > > ctb->broken = true; > > return -EPIPE; > > +#endif > > } > > > > /** > > @@ -815,8 +819,10 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) > > if (unlikely(ctb->broken)) > > return -EPIPE; > > > > +#ifdef CONFIG_DRM_I915_DEBUG_GUC > > if (unlikely(desc->status)) > > goto corrupted; > > +#endif > > > > GEM_BUG_ON(head > size); > > -- > Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915/guc: Don't check CT descriptor status before CT write / read 2022-01-21 21:03 ` Matthew Brost @ 2022-01-24 13:07 ` Jani Nikula 0 siblings, 0 replies; 6+ messages in thread From: Jani Nikula @ 2022-01-24 13:07 UTC (permalink / raw) To: Matthew Brost Cc: intel-gfx, daniele.ceraolospurio, john.c.harrison, dri-devel, michal.wajdeczko On Fri, 21 Jan 2022, Matthew Brost <matthew.brost@intel.com> wrote: > On Fri, Jan 21, 2022 at 09:28:46AM +0200, Jani Nikula wrote: >> On Thu, 20 Jan 2022, Matthew Brost <matthew.brost@intel.com> wrote: >> > Don't check CT descriptor status, unless CONFIG_DRM_I915_DEBUG_GUC is >> > set, before CT write / read as this could result in a read across the >> > PCIe bus thus adding latency to every CT write / read. On well behavied >> > systems this vaue should always read as zero. For some reason it doesn't >> > the CT channel is broken and will eventually recover from a GT reset, >> > albeit the GT reset will not be triggered immediately by seeing that >> > descriptor status is non-zero. >> > >> > v2: >> > (CI) >> > - Fix build error (hide corrupted label in write function behind >> > CONFIG_DRM_I915_DEBUG_GUC) >> > >> > Signed-off-by: Matthew Brost <matthew.brost@intel.com> >> > --- >> > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 6 ++++++ >> > 1 file changed, 6 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> > index de89d40abd38d..948cf31429412 100644 >> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c >> > @@ -379,8 +379,10 @@ static int ct_write(struct intel_guc_ct *ct, >> > u32 *cmds = ctb->cmds; >> > unsigned int i; >> > >> > +#ifdef CONFIG_DRM_I915_DEBUG_GUC >> > if (unlikely(desc->status)) >> > goto corrupted; >> > +#endif >> >> Please don't add #ifdefs inline. You can use >> IS_ENABLED(CONFIG_DRM_I915_DEBUG_GUC) in if statements, but otherwise >> the code needs to be split out to a separate function. >> > > Sure, but I feel like I've actually been by someone else to not use the > IS_ENABLED macro and use ifdefs inlines... Citation needed. Basically never use #if/#ifdef inline. Only use them at the top level like this: #if IS_ENABLED(CONFIG_FOO) static int bar(void) { /* implementation with foo */ } #else static int bar(void) { /* implementation without foo */ } #endif Sometimes you can avoid the above boilerplate with IS_ENABLED() inline: if (IS_ENABLED(CONFIG_FOO)) ... Basically if you think #if/#ifdef inline is the easiest, you need to refactor the code to do it cleanly without them. BR, Jani. > > Matt > >> BR, >> Jani. >> >> > >> > GEM_BUG_ON(tail > size); >> > >> > @@ -445,11 +447,13 @@ static int ct_write(struct intel_guc_ct *ct, >> > >> > return 0; >> > >> > +#ifdef CONFIG_DRM_I915_DEBUG_GUC >> > corrupted: >> > CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u status=%#x\n", >> > desc->head, desc->tail, desc->status); >> > ctb->broken = true; >> > return -EPIPE; >> > +#endif >> > } >> > >> > /** >> > @@ -815,8 +819,10 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) >> > if (unlikely(ctb->broken)) >> > return -EPIPE; >> > >> > +#ifdef CONFIG_DRM_I915_DEBUG_GUC >> > if (unlikely(desc->status)) >> > goto corrupted; >> > +#endif >> > >> > GEM_BUG_ON(head > size); >> >> -- >> Jani Nikula, Intel Open Source Graphics Center -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/i915/guc: Print CT descriptor status in CT debug function 2022-01-20 18:36 [PATCH 0/2] A few CT updates Matthew Brost 2022-01-20 18:36 ` [PATCH 1/2] drm/i915/guc: Don't check CT descriptor status before CT write / read Matthew Brost @ 2022-01-20 18:36 ` Matthew Brost 1 sibling, 0 replies; 6+ messages in thread From: Matthew Brost @ 2022-01-20 18:36 UTC (permalink / raw) To: intel-gfx, dri-devel Cc: daniele.ceraolospurio, john.c.harrison, michal.wajdeczko Noticed that the CT descriptor status was not printed in the CT debug function, add that in. Signed-off-by: Matthew Brost <matthew.brost@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 948cf31429412..5df2e3413796e 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -1219,10 +1219,14 @@ void intel_guc_ct_print_info(struct intel_guc_ct *ct, ct->ctbs.send.desc->head); drm_printf(p, "Tail: %u\n", ct->ctbs.send.desc->tail); + drm_printf(p, "Status: %u\n", + ct->ctbs.send.desc->status); drm_printf(p, "G2H Space: %u\n", atomic_read(&ct->ctbs.recv.space) * 4); drm_printf(p, "Head: %u\n", ct->ctbs.recv.desc->head); drm_printf(p, "Tail: %u\n", ct->ctbs.recv.desc->tail); + drm_printf(p, "Status: %u\n", + ct->ctbs.recv.desc->status); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-24 13:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-01-20 18:36 [PATCH 0/2] A few CT updates Matthew Brost 2022-01-20 18:36 ` [PATCH 1/2] drm/i915/guc: Don't check CT descriptor status before CT write / read Matthew Brost 2022-01-21 7:28 ` Jani Nikula 2022-01-21 21:03 ` Matthew Brost 2022-01-24 13:07 ` Jani Nikula 2022-01-20 18:36 ` [PATCH 2/2] drm/i915/guc: Print CT descriptor status in CT debug function Matthew Brost
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox