dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [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
* [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
* 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
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;
as well as URLs for NNTP newsgroup(s).