[03/25] drm/i915/gem: Update context name on closing
diff mbox series

Message ID 20191110185806.17413-3-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/25] drm/i915: Protect context while grabbing its name for the request
Related show

Commit Message

Chris Wilson Nov. 10, 2019, 6:57 p.m. UTC
Update the context.name on closing so that the persistent requests are
clear in debug prints.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Mika Kuoppala Nov. 11, 2019, 10:47 a.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Update the context.name on closing so that the persistent requests are
> clear in debug prints.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 982770e8163d..72d389afa28a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -461,11 +461,29 @@ static void kill_context(struct i915_gem_context *ctx)
>  	}
>  }
>  
> +static void set_closed_name(struct i915_gem_context *ctx)
> +{
> +	char *s;
> +
> +	/* Replace '[]' with '<>' to indicate closed in debug prints */
> +
> +	s = strrchr(ctx->name, '[');
> +	if (!s)
> +		return;
> +
> +	*s = '<';
> +
> +	s = strchr(s + 1, ']');

I can't think of a way for s+1 to be NULL as the TASKCOM_LEN + 8
makes the [pid] appear at the end.

With extending the buffer, one could have gone with 
+= "(closed)". To be more readable.

But would bloat the buffer more.

Which leads to thinking that perhaps we should grab only
the taskname/pid and then construct the name on the fly.

That needs buffer for callers, which might be nontrivial
due to usage on error situations.

So after running a circle,

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>


> +	if (s)
> +		*s = '>';
> +}
> +
>  static void context_close(struct i915_gem_context *ctx)
>  {
>  	struct i915_address_space *vm;
>  
>  	i915_gem_context_set_closed(ctx);
> +	set_closed_name(ctx);
>  
>  	mutex_lock(&ctx->mutex);
>  
> -- 
> 2.24.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Nov. 11, 2019, 10:58 a.m. UTC | #2
Quoting Mika Kuoppala (2019-11-11 10:47:00)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Update the context.name on closing so that the persistent requests are
> > clear in debug prints.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 982770e8163d..72d389afa28a 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -461,11 +461,29 @@ static void kill_context(struct i915_gem_context *ctx)
> >       }
> >  }
> >  
> > +static void set_closed_name(struct i915_gem_context *ctx)
> > +{
> > +     char *s;
> > +
> > +     /* Replace '[]' with '<>' to indicate closed in debug prints */
> > +
> > +     s = strrchr(ctx->name, '[');
> > +     if (!s)
> > +             return;
> > +
> > +     *s = '<';
> > +
> > +     s = strchr(s + 1, ']');
> 
> I can't think of a way for s+1 to be NULL as the TASKCOM_LEN + 8
> makes the [pid] appear at the end.
> 
> With extending the buffer, one could have gone with 
> += "(closed)". To be more readable.

Yeah, [] vs <> is subtle. It's debug so if we want to make it more
obvious later, or encode more information in there, I do not foresee
much problem in doing so.

> But would bloat the buffer more.

What's a few bytes more when we waste pages per context ;)
 
> Which leads to thinking that perhaps we should grab only
> the taskname/pid and then construct the name on the fly.
> 
> That needs buffer for callers, which might be nontrivial
> due to usage on error situations.

Exactly, and we can't allocate there either.
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 982770e8163d..72d389afa28a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -461,11 +461,29 @@  static void kill_context(struct i915_gem_context *ctx)
 	}
 }
 
+static void set_closed_name(struct i915_gem_context *ctx)
+{
+	char *s;
+
+	/* Replace '[]' with '<>' to indicate closed in debug prints */
+
+	s = strrchr(ctx->name, '[');
+	if (!s)
+		return;
+
+	*s = '<';
+
+	s = strchr(s + 1, ']');
+	if (s)
+		*s = '>';
+}
+
 static void context_close(struct i915_gem_context *ctx)
 {
 	struct i915_address_space *vm;
 
 	i915_gem_context_set_closed(ctx);
+	set_closed_name(ctx);
 
 	mutex_lock(&ctx->mutex);