[RFC 0/3] drm: Add comm/cmdline fdinfo fields

[RFC 0/3] drm: Add comm/cmdline fdinfo fields

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

When many of the things using the GPU are processes in a VM guest, the
actual client process is just a proxy.  The msm driver has a way to let
the proxy tell the kernel the actual VM client process's executable name
and command-line, which has until now been used simply for GPU crash
devcore dumps.  Lets also expose this via fdinfo so that tools can
expose who the actual user of the GPU is.

Rob Clark (3):
  drm/doc: Relax fdinfo string constraints
  drm/msm: Rework get_comm_cmdline() helper
  drm/msm: Add comm/cmdline fields

 Documentation/gpu/drm-usage-stats.rst   | 37 +++++++++++++++----------
 drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 +--
 drivers/gpu/drm/msm/msm_drv.c           |  2 ++
 drivers/gpu/drm/msm/msm_gpu.c           | 27 +++++++++++++-----
 drivers/gpu/drm/msm/msm_gpu.h           | 12 ++++++--
 drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
 6 files changed, 58 insertions(+), 25 deletions(-)

-- 
2.39.2

[RFC 2/3] drm/msm: Rework get_comm_cmdline() helper

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Make it work in terms of ctx so that it can be re-used for fdinfo.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 ++--
 drivers/gpu/drm/msm/msm_drv.c           |  2 ++
 drivers/gpu/drm/msm/msm_gpu.c           | 13 ++++++-------
 drivers/gpu/drm/msm/msm_gpu.h           | 12 ++++++++++--
 drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
 5 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index bb38e728864d..43c4e1fea83f 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -412,7 +412,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
 		/* Ensure string is null terminated: */
 		str[len] = '\0';
 
-		mutex_lock(&gpu->lock);
+		mutex_lock(&ctx->lock);
 
 		if (param == MSM_PARAM_COMM) {
 			paramp = &ctx->comm;
@@ -423,7 +423,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
 		kfree(*paramp);
 		*paramp = str;
 
-		mutex_unlock(&gpu->lock);
+		mutex_unlock(&ctx->lock);
 
 		return 0;
 	}
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 3d73b98d6a9c..ca0e89e46e13 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -581,6 +581,8 @@ static int context_init(struct drm_device *dev, struct drm_file *file)
 	rwlock_init(&ctx->queuelock);
 
 	kref_init(&ctx->ref);
+	ctx->pid = get_pid(task_pid(current));
+	mutex_init(&ctx->lock);
 	msm_submitqueue_init(dev, ctx);
 
 	ctx->aspace = msm_gpu_create_private_address_space(priv->gpu, current);
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index c403912d13ab..f0f4f845c32d 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -327,18 +327,17 @@ find_submit(struct msm_ringbuffer *ring, uint32_t fence)
 
 static void retire_submits(struct msm_gpu *gpu);
 
-static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd)
+static void get_comm_cmdline(struct msm_file_private *ctx, char **comm, char **cmd)
 {
-	struct msm_file_private *ctx = submit->queue->ctx;
 	struct task_struct *task;
 
-	WARN_ON(!mutex_is_locked(&submit->gpu->lock));
-
 	/* Note that kstrdup will return NULL if argument is NULL: */
+	mutex_lock(&ctx->lock);
 	*comm = kstrdup(ctx->comm, GFP_KERNEL);
 	*cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
+	mutex_unlock(&ctx->lock);
 
-	task = get_pid_task(submit->pid, PIDTYPE_PID);
+	task = get_pid_task(ctx->pid, PIDTYPE_PID);
 	if (!task)
 		return;
 
@@ -372,7 +371,7 @@ static void recover_worker(struct kthread_work *work)
 		if (submit->aspace)
 			submit->aspace->faults++;
 
-		get_comm_cmdline(submit, &comm, &cmd);
+		get_comm_cmdline(submit->queue->ctx, &comm, &cmd);
 
 		if (comm && cmd) {
 			DRM_DEV_ERROR(dev->dev, "%s: offending task: %s (%s)\n",
@@ -460,7 +459,7 @@ static void fault_worker(struct kthread_work *work)
 		goto resume_smmu;
 
 	if (submit) {
-		get_comm_cmdline(submit, &comm, &cmd);
+		get_comm_cmdline(submit->queue->ctx, &comm, &cmd);
 
 		/*
 		 * When we get GPU iova faults, we can get 1000s of them,
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 7a4fa1b8655b..b2023a42116b 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -377,17 +377,25 @@ struct msm_file_private {
 	 */
 	int sysprof;
 
+	/** @pid: Process that opened this file. */
+	struct pid *pid;
+
+	/**
+	 * lock: Protects comm and cmdline
+	 */
+	struct mutex lock;
+
 	/**
 	 * comm: Overridden task comm, see MSM_PARAM_COMM
 	 *
-	 * Accessed under msm_gpu::lock
+	 * Accessed under msm_file_private::lock
 	 */
 	char *comm;
 
 	/**
 	 * cmdline: Overridden task cmdline, see MSM_PARAM_CMDLINE
 	 *
-	 * Accessed under msm_gpu::lock
+	 * Accessed under msm_file_private::lock
 	 */
 	char *cmdline;
 
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index 0e803125a325..0444ba04fa06 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -61,6 +61,7 @@ void __msm_file_private_destroy(struct kref *kref)
 	}
 
 	msm_gem_address_space_put(ctx->aspace);
+	put_pid(ctx->pid);
 	kfree(ctx->comm);
 	kfree(ctx->cmdline);
 	kfree(ctx);
-- 
2.39.2

Re: [RFC 2/3] drm/msm: Rework get_comm_cmdline() helper

[Tvrtko Ursulin]

On 17/04/2023 21:12, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Make it work in terms of ctx so that it can be re-used for fdinfo.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>   drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 ++--
>   drivers/gpu/drm/msm/msm_drv.c           |  2 ++
>   drivers/gpu/drm/msm/msm_gpu.c           | 13 ++++++-------
>   drivers/gpu/drm/msm/msm_gpu.h           | 12 ++++++++++--
>   drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
>   5 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index bb38e728864d..43c4e1fea83f 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -412,7 +412,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
>   		/* Ensure string is null terminated: */
>   		str[len] = '\0';
>   
> -		mutex_lock(&gpu->lock);
> +		mutex_lock(&ctx->lock);
>   
>   		if (param == MSM_PARAM_COMM) {
>   			paramp = &ctx->comm;
> @@ -423,7 +423,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
>   		kfree(*paramp);
>   		*paramp = str;
>   
> -		mutex_unlock(&gpu->lock);
> +		mutex_unlock(&ctx->lock);
>   
>   		return 0;
>   	}
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 3d73b98d6a9c..ca0e89e46e13 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -581,6 +581,8 @@ static int context_init(struct drm_device *dev, struct drm_file *file)
>   	rwlock_init(&ctx->queuelock);
>   
>   	kref_init(&ctx->ref);
> +	ctx->pid = get_pid(task_pid(current));

Would it simplify things for msm if DRM core had an up to date file->pid 
as proposed in 
https://patchwork.freedesktop.org/patch/526752/?series=109902&rev=4 ? It 
gets updated if ioctl issuer is different than fd opener and this being 
context_init here reminded me of it. Maybe you wouldn't have to track 
the pid in msm?

Regards,

Tvrtko

> +	mutex_init(&ctx->lock);
>   	msm_submitqueue_init(dev, ctx);
>   
>   	ctx->aspace = msm_gpu_create_private_address_space(priv->gpu, current);
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index c403912d13ab..f0f4f845c32d 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -327,18 +327,17 @@ find_submit(struct msm_ringbuffer *ring, uint32_t fence)
>   
>   static void retire_submits(struct msm_gpu *gpu);
>   
> -static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd)
> +static void get_comm_cmdline(struct msm_file_private *ctx, char **comm, char **cmd)
>   {
> -	struct msm_file_private *ctx = submit->queue->ctx;
>   	struct task_struct *task;
>   
> -	WARN_ON(!mutex_is_locked(&submit->gpu->lock));
> -
>   	/* Note that kstrdup will return NULL if argument is NULL: */
> +	mutex_lock(&ctx->lock);
>   	*comm = kstrdup(ctx->comm, GFP_KERNEL);
>   	*cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
> +	mutex_unlock(&ctx->lock);
>   
> -	task = get_pid_task(submit->pid, PIDTYPE_PID);
> +	task = get_pid_task(ctx->pid, PIDTYPE_PID);
>   	if (!task)
>   		return;
>   
> @@ -372,7 +371,7 @@ static void recover_worker(struct kthread_work *work)
>   		if (submit->aspace)
>   			submit->aspace->faults++;
>   
> -		get_comm_cmdline(submit, &comm, &cmd);
> +		get_comm_cmdline(submit->queue->ctx, &comm, &cmd);
>   
>   		if (comm && cmd) {
>   			DRM_DEV_ERROR(dev->dev, "%s: offending task: %s (%s)\n",
> @@ -460,7 +459,7 @@ static void fault_worker(struct kthread_work *work)
>   		goto resume_smmu;
>   
>   	if (submit) {
> -		get_comm_cmdline(submit, &comm, &cmd);
> +		get_comm_cmdline(submit->queue->ctx, &comm, &cmd);
>   
>   		/*
>   		 * When we get GPU iova faults, we can get 1000s of them,
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 7a4fa1b8655b..b2023a42116b 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -377,17 +377,25 @@ struct msm_file_private {
>   	 */
>   	int sysprof;
>   
> +	/** @pid: Process that opened this file. */
> +	struct pid *pid;
> +
> +	/**
> +	 * lock: Protects comm and cmdline
> +	 */
> +	struct mutex lock;
> +
>   	/**
>   	 * comm: Overridden task comm, see MSM_PARAM_COMM
>   	 *
> -	 * Accessed under msm_gpu::lock
> +	 * Accessed under msm_file_private::lock
>   	 */
>   	char *comm;
>   
>   	/**
>   	 * cmdline: Overridden task cmdline, see MSM_PARAM_CMDLINE
>   	 *
> -	 * Accessed under msm_gpu::lock
> +	 * Accessed under msm_file_private::lock
>   	 */
>   	char *cmdline;
>   
> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index 0e803125a325..0444ba04fa06 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> @@ -61,6 +61,7 @@ void __msm_file_private_destroy(struct kref *kref)
>   	}
>   
>   	msm_gem_address_space_put(ctx->aspace);
> +	put_pid(ctx->pid);
>   	kfree(ctx->comm);
>   	kfree(ctx->cmdline);
>   	kfree(ctx);

Re: [RFC 2/3] drm/msm: Rework get_comm_cmdline() helper

[Daniel Vetter]

On Tue, Apr 18, 2023 at 09:27:49AM +0100, Tvrtko Ursulin wrote:
> 
> On 17/04/2023 21:12, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> > 
> > Make it work in terms of ctx so that it can be re-used for fdinfo.
> > 
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >   drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 ++--
> >   drivers/gpu/drm/msm/msm_drv.c           |  2 ++
> >   drivers/gpu/drm/msm/msm_gpu.c           | 13 ++++++-------
> >   drivers/gpu/drm/msm/msm_gpu.h           | 12 ++++++++++--
> >   drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
> >   5 files changed, 21 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > index bb38e728864d..43c4e1fea83f 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -412,7 +412,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> >   		/* Ensure string is null terminated: */
> >   		str[len] = '\0';
> > -		mutex_lock(&gpu->lock);
> > +		mutex_lock(&ctx->lock);
> >   		if (param == MSM_PARAM_COMM) {
> >   			paramp = &ctx->comm;
> > @@ -423,7 +423,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> >   		kfree(*paramp);
> >   		*paramp = str;
> > -		mutex_unlock(&gpu->lock);
> > +		mutex_unlock(&ctx->lock);
> >   		return 0;
> >   	}
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 3d73b98d6a9c..ca0e89e46e13 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -581,6 +581,8 @@ static int context_init(struct drm_device *dev, struct drm_file *file)
> >   	rwlock_init(&ctx->queuelock);
> >   	kref_init(&ctx->ref);
> > +	ctx->pid = get_pid(task_pid(current));
> 
> Would it simplify things for msm if DRM core had an up to date file->pid as
> proposed in
> https://patchwork.freedesktop.org/patch/526752/?series=109902&rev=4 ? It
> gets updated if ioctl issuer is different than fd opener and this being
> context_init here reminded me of it. Maybe you wouldn't have to track the
> pid in msm?

Can we go one step further and let the drm fdinfo stuff print these new
additions? Consistency across drivers and all that.

Also for a generic trigger I think any driver ioctl is good enough (we
only really need to avoid the auth dance when you're not on a render
node).
-Daniel

> 
> Regards,
> 
> Tvrtko
> 
> > +	mutex_init(&ctx->lock);
> >   	msm_submitqueue_init(dev, ctx);
> >   	ctx->aspace = msm_gpu_create_private_address_space(priv->gpu, current);
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > index c403912d13ab..f0f4f845c32d 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > @@ -327,18 +327,17 @@ find_submit(struct msm_ringbuffer *ring, uint32_t fence)
> >   static void retire_submits(struct msm_gpu *gpu);
> > -static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd)
> > +static void get_comm_cmdline(struct msm_file_private *ctx, char **comm, char **cmd)
> >   {
> > -	struct msm_file_private *ctx = submit->queue->ctx;
> >   	struct task_struct *task;
> > -	WARN_ON(!mutex_is_locked(&submit->gpu->lock));
> > -
> >   	/* Note that kstrdup will return NULL if argument is NULL: */
> > +	mutex_lock(&ctx->lock);
> >   	*comm = kstrdup(ctx->comm, GFP_KERNEL);
> >   	*cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
> > +	mutex_unlock(&ctx->lock);
> > -	task = get_pid_task(submit->pid, PIDTYPE_PID);
> > +	task = get_pid_task(ctx->pid, PIDTYPE_PID);
> >   	if (!task)
> >   		return;
> > @@ -372,7 +371,7 @@ static void recover_worker(struct kthread_work *work)
> >   		if (submit->aspace)
> >   			submit->aspace->faults++;
> > -		get_comm_cmdline(submit, &comm, &cmd);
> > +		get_comm_cmdline(submit->queue->ctx, &comm, &cmd);
> >   		if (comm && cmd) {
> >   			DRM_DEV_ERROR(dev->dev, "%s: offending task: %s (%s)\n",
> > @@ -460,7 +459,7 @@ static void fault_worker(struct kthread_work *work)
> >   		goto resume_smmu;
> >   	if (submit) {
> > -		get_comm_cmdline(submit, &comm, &cmd);
> > +		get_comm_cmdline(submit->queue->ctx, &comm, &cmd);
> >   		/*
> >   		 * When we get GPU iova faults, we can get 1000s of them,
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> > index 7a4fa1b8655b..b2023a42116b 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.h
> > +++ b/drivers/gpu/drm/msm/msm_gpu.h
> > @@ -377,17 +377,25 @@ struct msm_file_private {
> >   	 */
> >   	int sysprof;
> > +	/** @pid: Process that opened this file. */
> > +	struct pid *pid;
> > +
> > +	/**
> > +	 * lock: Protects comm and cmdline
> > +	 */
> > +	struct mutex lock;
> > +
> >   	/**
> >   	 * comm: Overridden task comm, see MSM_PARAM_COMM
> >   	 *
> > -	 * Accessed under msm_gpu::lock
> > +	 * Accessed under msm_file_private::lock
> >   	 */
> >   	char *comm;
> >   	/**
> >   	 * cmdline: Overridden task cmdline, see MSM_PARAM_CMDLINE
> >   	 *
> > -	 * Accessed under msm_gpu::lock
> > +	 * Accessed under msm_file_private::lock
> >   	 */
> >   	char *cmdline;
> > diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> > index 0e803125a325..0444ba04fa06 100644
> > --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> > +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> > @@ -61,6 +61,7 @@ void __msm_file_private_destroy(struct kref *kref)
> >   	}
> >   	msm_gem_address_space_put(ctx->aspace);
> > +	put_pid(ctx->pid);
> >   	kfree(ctx->comm);
> >   	kfree(ctx->cmdline);
> >   	kfree(ctx);

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC 2/3] drm/msm: Rework get_comm_cmdline() helper

[Rob Clark]

On Tue, Apr 18, 2023 at 1:34 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Apr 18, 2023 at 09:27:49AM +0100, Tvrtko Ursulin wrote:
> >
> > On 17/04/2023 21:12, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > Make it work in terms of ctx so that it can be re-used for fdinfo.
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >   drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 ++--
> > >   drivers/gpu/drm/msm/msm_drv.c           |  2 ++
> > >   drivers/gpu/drm/msm/msm_gpu.c           | 13 ++++++-------
> > >   drivers/gpu/drm/msm/msm_gpu.h           | 12 ++++++++++--
> > >   drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
> > >   5 files changed, 21 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > index bb38e728864d..43c4e1fea83f 100644
> > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > @@ -412,7 +412,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > >             /* Ensure string is null terminated: */
> > >             str[len] = '\0';
> > > -           mutex_lock(&gpu->lock);
> > > +           mutex_lock(&ctx->lock);
> > >             if (param == MSM_PARAM_COMM) {
> > >                     paramp = &ctx->comm;
> > > @@ -423,7 +423,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > >             kfree(*paramp);
> > >             *paramp = str;
> > > -           mutex_unlock(&gpu->lock);
> > > +           mutex_unlock(&ctx->lock);
> > >             return 0;
> > >     }
> > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > index 3d73b98d6a9c..ca0e89e46e13 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > @@ -581,6 +581,8 @@ static int context_init(struct drm_device *dev, struct drm_file *file)
> > >     rwlock_init(&ctx->queuelock);
> > >     kref_init(&ctx->ref);
> > > +   ctx->pid = get_pid(task_pid(current));
> >
> > Would it simplify things for msm if DRM core had an up to date file->pid as
> > proposed in
> > https://patchwork.freedesktop.org/patch/526752/?series=109902&rev=4 ? It
> > gets updated if ioctl issuer is different than fd opener and this being
> > context_init here reminded me of it. Maybe you wouldn't have to track the
> > pid in msm?

The problem is that we also need this for gpu devcore dumps, which
could happen after the drm_file is closed.  The ctx can outlive the
file.

But the ctx->pid has the same problem as the existing file->pid when
it comes to Xorg.. hopefully over time that problem just goes away.  I
guess I could do a similar dance to your patch to update the pid
whenever (for ex) a submitqueue is created.

> Can we go one step further and let the drm fdinfo stuff print these new
> additions? Consistency across drivers and all that.

Hmm, I guess I could _also_ store the overridden comm/cmdline in
drm_file.  I still need to track it in ctx (msm_file_private) because
I could need it after the file is closed.

Maybe it could be useful to have a gl extension to let the app set a
name on the context so that this is useful beyond native-ctx (ie.
maybe it would be nice to see that "chrome: lwn.net" is using less gpu
memory than "chrome: phoronix.com", etc)

BR,
-R

> Also for a generic trigger I think any driver ioctl is good enough (we
> only really need to avoid the auth dance when you're not on a render
> node).
> -Daniel
>
> >
> > Regards,
> >
> > Tvrtko
> >
> > > +   mutex_init(&ctx->lock);
> > >     msm_submitqueue_init(dev, ctx);
> > >     ctx->aspace = msm_gpu_create_private_address_space(priv->gpu, current);
> > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > > index c403912d13ab..f0f4f845c32d 100644
> > > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > > @@ -327,18 +327,17 @@ find_submit(struct msm_ringbuffer *ring, uint32_t fence)
> > >   static void retire_submits(struct msm_gpu *gpu);
> > > -static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char **cmd)
> > > +static void get_comm_cmdline(struct msm_file_private *ctx, char **comm, char **cmd)
> > >   {
> > > -   struct msm_file_private *ctx = submit->queue->ctx;
> > >     struct task_struct *task;
> > > -   WARN_ON(!mutex_is_locked(&submit->gpu->lock));
> > > -
> > >     /* Note that kstrdup will return NULL if argument is NULL: */
> > > +   mutex_lock(&ctx->lock);
> > >     *comm = kstrdup(ctx->comm, GFP_KERNEL);
> > >     *cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
> > > +   mutex_unlock(&ctx->lock);
> > > -   task = get_pid_task(submit->pid, PIDTYPE_PID);
> > > +   task = get_pid_task(ctx->pid, PIDTYPE_PID);
> > >     if (!task)
> > >             return;
> > > @@ -372,7 +371,7 @@ static void recover_worker(struct kthread_work *work)
> > >             if (submit->aspace)
> > >                     submit->aspace->faults++;
> > > -           get_comm_cmdline(submit, &comm, &cmd);
> > > +           get_comm_cmdline(submit->queue->ctx, &comm, &cmd);
> > >             if (comm && cmd) {
> > >                     DRM_DEV_ERROR(dev->dev, "%s: offending task: %s (%s)\n",
> > > @@ -460,7 +459,7 @@ static void fault_worker(struct kthread_work *work)
> > >             goto resume_smmu;
> > >     if (submit) {
> > > -           get_comm_cmdline(submit, &comm, &cmd);
> > > +           get_comm_cmdline(submit->queue->ctx, &comm, &cmd);
> > >             /*
> > >              * When we get GPU iova faults, we can get 1000s of them,
> > > diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> > > index 7a4fa1b8655b..b2023a42116b 100644
> > > --- a/drivers/gpu/drm/msm/msm_gpu.h
> > > +++ b/drivers/gpu/drm/msm/msm_gpu.h
> > > @@ -377,17 +377,25 @@ struct msm_file_private {
> > >      */
> > >     int sysprof;
> > > +   /** @pid: Process that opened this file. */
> > > +   struct pid *pid;
> > > +
> > > +   /**
> > > +    * lock: Protects comm and cmdline
> > > +    */
> > > +   struct mutex lock;
> > > +
> > >     /**
> > >      * comm: Overridden task comm, see MSM_PARAM_COMM
> > >      *
> > > -    * Accessed under msm_gpu::lock
> > > +    * Accessed under msm_file_private::lock
> > >      */
> > >     char *comm;
> > >     /**
> > >      * cmdline: Overridden task cmdline, see MSM_PARAM_CMDLINE
> > >      *
> > > -    * Accessed under msm_gpu::lock
> > > +    * Accessed under msm_file_private::lock
> > >      */
> > >     char *cmdline;
> > > diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> > > index 0e803125a325..0444ba04fa06 100644
> > > --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> > > +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> > > @@ -61,6 +61,7 @@ void __msm_file_private_destroy(struct kref *kref)
> > >     }
> > >     msm_gem_address_space_put(ctx->aspace);
> > > +   put_pid(ctx->pid);
> > >     kfree(ctx->comm);
> > >     kfree(ctx->cmdline);
> > >     kfree(ctx);
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [RFC 2/3] drm/msm: Rework get_comm_cmdline() helper

[Emil Velikov]

Greeting all,

Sorry for the delay - Easter Holidays, food coma and all that :-)

On Tue, 18 Apr 2023 at 15:31, Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Apr 18, 2023 at 1:34 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Tue, Apr 18, 2023 at 09:27:49AM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 17/04/2023 21:12, Rob Clark wrote:
> > > > From: Rob Clark <robdclark@chromium.org>
> > > >
> > > > Make it work in terms of ctx so that it can be re-used for fdinfo.
> > > >
> > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > ---
> > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 ++--
> > > >   drivers/gpu/drm/msm/msm_drv.c           |  2 ++
> > > >   drivers/gpu/drm/msm/msm_gpu.c           | 13 ++++++-------
> > > >   drivers/gpu/drm/msm/msm_gpu.h           | 12 ++++++++++--
> > > >   drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
> > > >   5 files changed, 21 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > index bb38e728864d..43c4e1fea83f 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > @@ -412,7 +412,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > > >             /* Ensure string is null terminated: */
> > > >             str[len] = '\0';
> > > > -           mutex_lock(&gpu->lock);
> > > > +           mutex_lock(&ctx->lock);
> > > >             if (param == MSM_PARAM_COMM) {
> > > >                     paramp = &ctx->comm;
> > > > @@ -423,7 +423,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > > >             kfree(*paramp);
> > > >             *paramp = str;
> > > > -           mutex_unlock(&gpu->lock);
> > > > +           mutex_unlock(&ctx->lock);
> > > >             return 0;
> > > >     }
> > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > > index 3d73b98d6a9c..ca0e89e46e13 100644
> > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > @@ -581,6 +581,8 @@ static int context_init(struct drm_device *dev, struct drm_file *file)
> > > >     rwlock_init(&ctx->queuelock);
> > > >     kref_init(&ctx->ref);
> > > > +   ctx->pid = get_pid(task_pid(current));
> > >
> > > Would it simplify things for msm if DRM core had an up to date file->pid as
> > > proposed in
> > > https://patchwork.freedesktop.org/patch/526752/?series=109902&rev=4 ? It
> > > gets updated if ioctl issuer is different than fd opener and this being
> > > context_init here reminded me of it. Maybe you wouldn't have to track the
> > > pid in msm?
>
> The problem is that we also need this for gpu devcore dumps, which
> could happen after the drm_file is closed.  The ctx can outlive the
> file.
>
I think we all kept forgetting about that. MSM had support for ages,
while AMDGPU is the second driver to land support - just a release
ago.

> But the ctx->pid has the same problem as the existing file->pid when
> it comes to Xorg.. hopefully over time that problem just goes away.

Out of curiosity: what do you mean with "when it comes to Xorg" - the
"was_master" handling or something else?

> guess I could do a similar dance to your patch to update the pid
> whenever (for ex) a submitqueue is created.
>
> > Can we go one step further and let the drm fdinfo stuff print these new
> > additions? Consistency across drivers and all that.
>
> Hmm, I guess I could _also_ store the overridden comm/cmdline in
> drm_file.  I still need to track it in ctx (msm_file_private) because
> I could need it after the file is closed.
>
> Maybe it could be useful to have a gl extension to let the app set a
> name on the context so that this is useful beyond native-ctx (ie.
> maybe it would be nice to see that "chrome: lwn.net" is using less gpu
> memory than "chrome: phoronix.com", etc)
>

/me awaits for the series to hit the respective websites ;-)

But seriously - the series from Tvrtko (thanks for the link, will
check in a moment) makes sense. Although given the livespan issue
mentioned above, I don't think it's applicable here.

So if it were me, I would consider the two orthogonal for the
short/mid term. Fwiw this and patch 1/3 are:
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

HTH
-Emil

Re: [RFC 2/3] drm/msm: Rework get_comm_cmdline() helper

[Rob Clark]

On Fri, Apr 21, 2023 at 2:33 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> Greeting all,
>
> Sorry for the delay - Easter Holidays, food coma and all that :-)
>
> On Tue, 18 Apr 2023 at 15:31, Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Tue, Apr 18, 2023 at 1:34 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Tue, Apr 18, 2023 at 09:27:49AM +0100, Tvrtko Ursulin wrote:
> > > >
> > > > On 17/04/2023 21:12, Rob Clark wrote:
> > > > > From: Rob Clark <robdclark@chromium.org>
> > > > >
> > > > > Make it work in terms of ctx so that it can be re-used for fdinfo.
> > > > >
> > > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > > ---
> > > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 ++--
> > > > >   drivers/gpu/drm/msm/msm_drv.c           |  2 ++
> > > > >   drivers/gpu/drm/msm/msm_gpu.c           | 13 ++++++-------
> > > > >   drivers/gpu/drm/msm/msm_gpu.h           | 12 ++++++++++--
> > > > >   drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
> > > > >   5 files changed, 21 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > > index bb38e728864d..43c4e1fea83f 100644
> > > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > > @@ -412,7 +412,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > > > >             /* Ensure string is null terminated: */
> > > > >             str[len] = '\0';
> > > > > -           mutex_lock(&gpu->lock);
> > > > > +           mutex_lock(&ctx->lock);
> > > > >             if (param == MSM_PARAM_COMM) {
> > > > >                     paramp = &ctx->comm;
> > > > > @@ -423,7 +423,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > > > >             kfree(*paramp);
> > > > >             *paramp = str;
> > > > > -           mutex_unlock(&gpu->lock);
> > > > > +           mutex_unlock(&ctx->lock);
> > > > >             return 0;
> > > > >     }
> > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > > > index 3d73b98d6a9c..ca0e89e46e13 100644
> > > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > > @@ -581,6 +581,8 @@ static int context_init(struct drm_device *dev, struct drm_file *file)
> > > > >     rwlock_init(&ctx->queuelock);
> > > > >     kref_init(&ctx->ref);
> > > > > +   ctx->pid = get_pid(task_pid(current));
> > > >
> > > > Would it simplify things for msm if DRM core had an up to date file->pid as
> > > > proposed in
> > > > https://patchwork.freedesktop.org/patch/526752/?series=109902&rev=4 ? It
> > > > gets updated if ioctl issuer is different than fd opener and this being
> > > > context_init here reminded me of it. Maybe you wouldn't have to track the
> > > > pid in msm?
> >
> > The problem is that we also need this for gpu devcore dumps, which
> > could happen after the drm_file is closed.  The ctx can outlive the
> > file.
> >
> I think we all kept forgetting about that. MSM had support for ages,
> while AMDGPU is the second driver to land support - just a release
> ago.
>
> > But the ctx->pid has the same problem as the existing file->pid when
> > it comes to Xorg.. hopefully over time that problem just goes away.
>
> Out of curiosity: what do you mean with "when it comes to Xorg" - the
> "was_master" handling or something else?

The problem is that Xorg is the one to open the drm fd, and then
passes the fd to the client.. so the pid of drm_file is the Xorg pid,
not the client.  Making it not terribly informative.

Tvrtko's patch he linked above would address that for drm_file, but
not for other driver internal usages.  Maybe it could be wired up as a
helper so that drivers don't have to re-invent that dance.  Idk, I
have to think about it.

Btw, with my WIP drm sched fence signalling patch lockdep is unhappy
when gpu devcore dumps are triggered.  I'm still pondering how to
decouple the locking so that anything coming from fs (ie.
show_fdinfo()) is decoupled from anything that happens in the fence
signaling path.  But will repost this series once I get that sorted
out.

BR,
-R

>
> > guess I could do a similar dance to your patch to update the pid
> > whenever (for ex) a submitqueue is created.
> >
> > > Can we go one step further and let the drm fdinfo stuff print these new
> > > additions? Consistency across drivers and all that.
> >
> > Hmm, I guess I could _also_ store the overridden comm/cmdline in
> > drm_file.  I still need to track it in ctx (msm_file_private) because
> > I could need it after the file is closed.
> >
> > Maybe it could be useful to have a gl extension to let the app set a
> > name on the context so that this is useful beyond native-ctx (ie.
> > maybe it would be nice to see that "chrome: lwn.net" is using less gpu
> > memory than "chrome: phoronix.com", etc)
> >
>
> /me awaits for the series to hit the respective websites ;-)
>
> But seriously - the series from Tvrtko (thanks for the link, will
> check in a moment) makes sense. Although given the livespan issue
> mentioned above, I don't think it's applicable here.
>
> So if it were me, I would consider the two orthogonal for the
> short/mid term. Fwiw this and patch 1/3 are:
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>
> HTH
> -Emil

Re: [RFC 2/3] drm/msm: Rework get_comm_cmdline() helper

[Daniel Vetter]

On Fri, Apr 21, 2023 at 07:47:26AM -0700, Rob Clark wrote:
> On Fri, Apr 21, 2023 at 2:33 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >
> > Greeting all,
> >
> > Sorry for the delay - Easter Holidays, food coma and all that :-)
> >
> > On Tue, 18 Apr 2023 at 15:31, Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Tue, Apr 18, 2023 at 1:34 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Tue, Apr 18, 2023 at 09:27:49AM +0100, Tvrtko Ursulin wrote:
> > > > >
> > > > > On 17/04/2023 21:12, Rob Clark wrote:
> > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > >
> > > > > > Make it work in terms of ctx so that it can be re-used for fdinfo.
> > > > > >
> > > > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > > > ---
> > > > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 ++--
> > > > > >   drivers/gpu/drm/msm/msm_drv.c           |  2 ++
> > > > > >   drivers/gpu/drm/msm/msm_gpu.c           | 13 ++++++-------
> > > > > >   drivers/gpu/drm/msm/msm_gpu.h           | 12 ++++++++++--
> > > > > >   drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
> > > > > >   5 files changed, 21 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > > > index bb38e728864d..43c4e1fea83f 100644
> > > > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > > > @@ -412,7 +412,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > > > > >             /* Ensure string is null terminated: */
> > > > > >             str[len] = '\0';
> > > > > > -           mutex_lock(&gpu->lock);
> > > > > > +           mutex_lock(&ctx->lock);
> > > > > >             if (param == MSM_PARAM_COMM) {
> > > > > >                     paramp = &ctx->comm;
> > > > > > @@ -423,7 +423,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > > > > >             kfree(*paramp);
> > > > > >             *paramp = str;
> > > > > > -           mutex_unlock(&gpu->lock);
> > > > > > +           mutex_unlock(&ctx->lock);
> > > > > >             return 0;
> > > > > >     }
> > > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > > > > index 3d73b98d6a9c..ca0e89e46e13 100644
> > > > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > > > @@ -581,6 +581,8 @@ static int context_init(struct drm_device *dev, struct drm_file *file)
> > > > > >     rwlock_init(&ctx->queuelock);
> > > > > >     kref_init(&ctx->ref);
> > > > > > +   ctx->pid = get_pid(task_pid(current));
> > > > >
> > > > > Would it simplify things for msm if DRM core had an up to date file->pid as
> > > > > proposed in
> > > > > https://patchwork.freedesktop.org/patch/526752/?series=109902&rev=4 ? It
> > > > > gets updated if ioctl issuer is different than fd opener and this being
> > > > > context_init here reminded me of it. Maybe you wouldn't have to track the
> > > > > pid in msm?
> > >
> > > The problem is that we also need this for gpu devcore dumps, which
> > > could happen after the drm_file is closed.  The ctx can outlive the
> > > file.
> > >
> > I think we all kept forgetting about that. MSM had support for ages,
> > while AMDGPU is the second driver to land support - just a release
> > ago.
> >
> > > But the ctx->pid has the same problem as the existing file->pid when
> > > it comes to Xorg.. hopefully over time that problem just goes away.
> >
> > Out of curiosity: what do you mean with "when it comes to Xorg" - the
> > "was_master" handling or something else?
> 
> The problem is that Xorg is the one to open the drm fd, and then
> passes the fd to the client.. so the pid of drm_file is the Xorg pid,
> not the client.  Making it not terribly informative.
> 
> Tvrtko's patch he linked above would address that for drm_file, but
> not for other driver internal usages.  Maybe it could be wired up as a
> helper so that drivers don't have to re-invent that dance.  Idk, I
> have to think about it.
> 
> Btw, with my WIP drm sched fence signalling patch lockdep is unhappy
> when gpu devcore dumps are triggered.  I'm still pondering how to
> decouple the locking so that anything coming from fs (ie.
> show_fdinfo()) is decoupled from anything that happens in the fence
> signaling path.  But will repost this series once I get that sorted
> out.

So the cleanest imo is that you push most of the capturing into a worker
that's entirely decoupled. If you have terminal context (i.e. on first
hang they stop all further cmd submission, which is anyway what
vk/arb_robustness want), then you don't have to capture at tdr time,
because there's no subsequent batch that will wreck the state.

But it only works if your gpu ctx don't have recoverable semantics.

If you can't do that it's a _lot_ of GFP_ATOMIC and trylock and bailing
out if any fails :-/
-Daniel

> 
> BR,
> -R
> 
> >
> > > guess I could do a similar dance to your patch to update the pid
> > > whenever (for ex) a submitqueue is created.
> > >
> > > > Can we go one step further and let the drm fdinfo stuff print these new
> > > > additions? Consistency across drivers and all that.
> > >
> > > Hmm, I guess I could _also_ store the overridden comm/cmdline in
> > > drm_file.  I still need to track it in ctx (msm_file_private) because
> > > I could need it after the file is closed.
> > >
> > > Maybe it could be useful to have a gl extension to let the app set a
> > > name on the context so that this is useful beyond native-ctx (ie.
> > > maybe it would be nice to see that "chrome: lwn.net" is using less gpu
> > > memory than "chrome: phoronix.com", etc)
> > >
> >
> > /me awaits for the series to hit the respective websites ;-)
> >
> > But seriously - the series from Tvrtko (thanks for the link, will
> > check in a moment) makes sense. Although given the livespan issue
> > mentioned above, I don't think it's applicable here.
> >
> > So if it were me, I would consider the two orthogonal for the
> > short/mid term. Fwiw this and patch 1/3 are:
> > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> >
> > HTH
> > -Emil

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC 2/3] drm/msm: Rework get_comm_cmdline() helper

[Rob Clark]

On Thu, Apr 27, 2023 at 2:39 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Apr 21, 2023 at 07:47:26AM -0700, Rob Clark wrote:
> > On Fri, Apr 21, 2023 at 2:33 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > >
> > > Greeting all,
> > >
> > > Sorry for the delay - Easter Holidays, food coma and all that :-)
> > >
> > > On Tue, 18 Apr 2023 at 15:31, Rob Clark <robdclark@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 18, 2023 at 1:34 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Tue, Apr 18, 2023 at 09:27:49AM +0100, Tvrtko Ursulin wrote:
> > > > > >
> > > > > > On 17/04/2023 21:12, Rob Clark wrote:
> > > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > > >
> > > > > > > Make it work in terms of ctx so that it can be re-used for fdinfo.
> > > > > > >
> > > > > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > > > > ---
> > > > > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 ++--
> > > > > > >   drivers/gpu/drm/msm/msm_drv.c           |  2 ++
> > > > > > >   drivers/gpu/drm/msm/msm_gpu.c           | 13 ++++++-------
> > > > > > >   drivers/gpu/drm/msm/msm_gpu.h           | 12 ++++++++++--
> > > > > > >   drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
> > > > > > >   5 files changed, 21 insertions(+), 11 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > > > > index bb38e728864d..43c4e1fea83f 100644
> > > > > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > > > > @@ -412,7 +412,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > > > > > >             /* Ensure string is null terminated: */
> > > > > > >             str[len] = '\0';
> > > > > > > -           mutex_lock(&gpu->lock);
> > > > > > > +           mutex_lock(&ctx->lock);
> > > > > > >             if (param == MSM_PARAM_COMM) {
> > > > > > >                     paramp = &ctx->comm;
> > > > > > > @@ -423,7 +423,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > > > > > >             kfree(*paramp);
> > > > > > >             *paramp = str;
> > > > > > > -           mutex_unlock(&gpu->lock);
> > > > > > > +           mutex_unlock(&ctx->lock);
> > > > > > >             return 0;
> > > > > > >     }
> > > > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > > > > > index 3d73b98d6a9c..ca0e89e46e13 100644
> > > > > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > > > > @@ -581,6 +581,8 @@ static int context_init(struct drm_device *dev, struct drm_file *file)
> > > > > > >     rwlock_init(&ctx->queuelock);
> > > > > > >     kref_init(&ctx->ref);
> > > > > > > +   ctx->pid = get_pid(task_pid(current));
> > > > > >
> > > > > > Would it simplify things for msm if DRM core had an up to date file->pid as
> > > > > > proposed in
> > > > > > https://patchwork.freedesktop.org/patch/526752/?series=109902&rev=4 ? It
> > > > > > gets updated if ioctl issuer is different than fd opener and this being
> > > > > > context_init here reminded me of it. Maybe you wouldn't have to track the
> > > > > > pid in msm?
> > > >
> > > > The problem is that we also need this for gpu devcore dumps, which
> > > > could happen after the drm_file is closed.  The ctx can outlive the
> > > > file.
> > > >
> > > I think we all kept forgetting about that. MSM had support for ages,
> > > while AMDGPU is the second driver to land support - just a release
> > > ago.
> > >
> > > > But the ctx->pid has the same problem as the existing file->pid when
> > > > it comes to Xorg.. hopefully over time that problem just goes away.
> > >
> > > Out of curiosity: what do you mean with "when it comes to Xorg" - the
> > > "was_master" handling or something else?
> >
> > The problem is that Xorg is the one to open the drm fd, and then
> > passes the fd to the client.. so the pid of drm_file is the Xorg pid,
> > not the client.  Making it not terribly informative.
> >
> > Tvrtko's patch he linked above would address that for drm_file, but
> > not for other driver internal usages.  Maybe it could be wired up as a
> > helper so that drivers don't have to re-invent that dance.  Idk, I
> > have to think about it.
> >
> > Btw, with my WIP drm sched fence signalling patch lockdep is unhappy
> > when gpu devcore dumps are triggered.  I'm still pondering how to
> > decouple the locking so that anything coming from fs (ie.
> > show_fdinfo()) is decoupled from anything that happens in the fence
> > signaling path.  But will repost this series once I get that sorted
> > out.
>
> So the cleanest imo is that you push most of the capturing into a worker
> that's entirely decoupled. If you have terminal context (i.e. on first
> hang they stop all further cmd submission, which is anyway what
> vk/arb_robustness want), then you don't have to capture at tdr time,
> because there's no subsequent batch that will wreck the state.

It is already in a worker, but we (a) need to block other contexts
from submitting while at the same time (b) using the GPU itself to
capture its state.. (yes, the way the hw works is overly complicated
in this regard)

> But it only works if your gpu ctx don't have recoverable semantics.

We do have recoverable semantics.. but that is pretty orthogonal.  We
just need a different lock.. I have a plan to move (a copy) of the
override strings to drm_file with it's own locking decoupled from what
we need in the recovery path.. and hopefully will finally have time to
type it up today and post it (just before disappearing off into the
woods to go backpacking ;-))

BR,
-R

> If you can't do that it's a _lot_ of GFP_ATOMIC and trylock and bailing
> out if any fails :-/
> -Daniel
>
> >
> > BR,
> > -R
> >
> > >
> > > > guess I could do a similar dance to your patch to update the pid
> > > > whenever (for ex) a submitqueue is created.
> > > >
> > > > > Can we go one step further and let the drm fdinfo stuff print these new
> > > > > additions? Consistency across drivers and all that.
> > > >
> > > > Hmm, I guess I could _also_ store the overridden comm/cmdline in
> > > > drm_file.  I still need to track it in ctx (msm_file_private) because
> > > > I could need it after the file is closed.
> > > >
> > > > Maybe it could be useful to have a gl extension to let the app set a
> > > > name on the context so that this is useful beyond native-ctx (ie.
> > > > maybe it would be nice to see that "chrome: lwn.net" is using less gpu
> > > > memory than "chrome: phoronix.com", etc)
> > > >
> > >
> > > /me awaits for the series to hit the respective websites ;-)
> > >
> > > But seriously - the series from Tvrtko (thanks for the link, will
> > > check in a moment) makes sense. Although given the livespan issue
> > > mentioned above, I don't think it's applicable here.
> > >
> > > So if it were me, I would consider the two orthogonal for the
> > > short/mid term. Fwiw this and patch 1/3 are:
> > > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> > >
> > > HTH
> > > -Emil
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

[RFC 3/3] drm/msm: Add comm/cmdline fields

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Normally this would be the same information that can be obtained in
other ways.  But in some cases the process opening the drm fd is merely
a sort of proxy for the actual process using the GPU.  This is the case
for guest VM processes using the GPU via virglrenderer, in which case
the msm native-context renderer in virglrenderer overrides the comm/
cmdline to be the guest process's values.

Exposing this via fdinfo allows tools like gputop to show something more
meaningful than just a bunch of "pcivirtio-gpu" users.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 Documentation/gpu/drm-usage-stats.rst |  8 ++++++++
 drivers/gpu/drm/msm/msm_gpu.c         | 14 ++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
index 8e00d53231e0..bc90bed455e3 100644
--- a/Documentation/gpu/drm-usage-stats.rst
+++ b/Documentation/gpu/drm-usage-stats.rst
@@ -148,6 +148,14 @@ percentage utilization of the engine, whereas drm-engine-<keystr> only reflects
 time active without considering what frequency the engine is operating as a
 percentage of it's maximum frequency.
 
+- drm-comm: <valstr>
+
+Returns the clients executable path.
+
+- drm-cmdline: <valstr>
+
+Returns the clients cmdline.
+
 Implementation Details
 ======================
 
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index f0f4f845c32d..1150dcbf28aa 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -148,12 +148,26 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
 	return 0;
 }
 
+static void get_comm_cmdline(struct msm_file_private *ctx, char **comm, char **cmd);
+
 void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
 			 struct drm_printer *p)
 {
+	char *comm, *cmdline;
+
+	get_comm_cmdline(ctx, &comm, &cmdline);
+
 	drm_printf(p, "drm-engine-gpu:\t%llu ns\n", ctx->elapsed_ns);
 	drm_printf(p, "drm-cycles-gpu:\t%llu\n", ctx->cycles);
 	drm_printf(p, "drm-maxfreq-gpu:\t%u Hz\n", gpu->fast_rate);
+
+	if (comm)
+		drm_printf(p, "drm-comm:\t%s\n", comm);
+	if (cmdline)
+		drm_printf(p, "drm-cmdline:\t%s\n", cmdline);
+
+	kfree(comm);
+	kfree(cmdline);
 }
 
 int msm_gpu_hw_init(struct msm_gpu *gpu)
-- 
2.39.2

Re: [RFC 3/3] drm/msm: Add comm/cmdline fields

[Tvrtko Ursulin]

On 17/04/2023 21:12, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Normally this would be the same information that can be obtained in
> other ways.  But in some cases the process opening the drm fd is merely
> a sort of proxy for the actual process using the GPU.  This is the case
> for guest VM processes using the GPU via virglrenderer, in which case
> the msm native-context renderer in virglrenderer overrides the comm/
> cmdline to be the guest process's values.
> 
> Exposing this via fdinfo allows tools like gputop to show something more
> meaningful than just a bunch of "pcivirtio-gpu" users.

You also later expanded with:

"""
I should have also mentioned, in the VM/proxy scenario we have a
single process with separate drm_file's for each guest VM process.  So
it isn't an option to just change the proxy process's name to match
the client.
"""

So how does that work - this single process temporarily changes it's 
name for each drm fd it opens and creates a context or it is actually in 
the native context protocol?

> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>   Documentation/gpu/drm-usage-stats.rst |  8 ++++++++
>   drivers/gpu/drm/msm/msm_gpu.c         | 14 ++++++++++++++
>   2 files changed, 22 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> index 8e00d53231e0..bc90bed455e3 100644
> --- a/Documentation/gpu/drm-usage-stats.rst
> +++ b/Documentation/gpu/drm-usage-stats.rst
> @@ -148,6 +148,14 @@ percentage utilization of the engine, whereas drm-engine-<keystr> only reflects
>   time active without considering what frequency the engine is operating as a
>   percentage of it's maximum frequency.
>   
> +- drm-comm: <valstr>
> +
> +Returns the clients executable path.

Full path and not just current->comm? In this case probably give it a 
more descriptive name here.

drm-client-executable
drm-client-command-line

So we stay in the drm-client- namespace?

Or if the former is absolute path could one key be enough for both?

drm-client-command-line: /path/to/executable --arguments

> +
> +- drm-cmdline: <valstr>
> +
> +Returns the clients cmdline.

I think drm-usage-stats.rst text should provide some more text with 
these two. To precisely define their content and outline the use case 
under which driver authors may want to add them, and fdinfo consumer 
therefore expect to see them. Just so everything is completely clear and 
people do not start adding them for drivers which do not support native 
context (or like).

But on the overall it sounds reasonable to me - it would be really cool 
to not just see pcivirtio-gpu as you say. Even if the standard virtiogpu 
use case (not native context) could show real users.

Regards,

Tvrtko

> +
>   Implementation Details
>   ======================
>   
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index f0f4f845c32d..1150dcbf28aa 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -148,12 +148,26 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
>   	return 0;
>   }
>   
> +static void get_comm_cmdline(struct msm_file_private *ctx, char **comm, char **cmd);
> +
>   void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
>   			 struct drm_printer *p)
>   {
> +	char *comm, *cmdline;
> +
> +	get_comm_cmdline(ctx, &comm, &cmdline);
> +
>   	drm_printf(p, "drm-engine-gpu:\t%llu ns\n", ctx->elapsed_ns);
>   	drm_printf(p, "drm-cycles-gpu:\t%llu\n", ctx->cycles);
>   	drm_printf(p, "drm-maxfreq-gpu:\t%u Hz\n", gpu->fast_rate);
> +
> +	if (comm)
> +		drm_printf(p, "drm-comm:\t%s\n", comm);
> +	if (cmdline)
> +		drm_printf(p, "drm-cmdline:\t%s\n", cmdline);
> +
> +	kfree(comm);
> +	kfree(cmdline);
>   }
>   
>   int msm_gpu_hw_init(struct msm_gpu *gpu)

Re: [RFC 3/3] drm/msm: Add comm/cmdline fields

[Rob Clark]

On Tue, Apr 18, 2023 at 1:53 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 17/04/2023 21:12, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Normally this would be the same information that can be obtained in
> > other ways.  But in some cases the process opening the drm fd is merely
> > a sort of proxy for the actual process using the GPU.  This is the case
> > for guest VM processes using the GPU via virglrenderer, in which case
> > the msm native-context renderer in virglrenderer overrides the comm/
> > cmdline to be the guest process's values.
> >
> > Exposing this via fdinfo allows tools like gputop to show something more
> > meaningful than just a bunch of "pcivirtio-gpu" users.
>
> You also later expanded with:
>
> """
> I should have also mentioned, in the VM/proxy scenario we have a
> single process with separate drm_file's for each guest VM process.  So
> it isn't an option to just change the proxy process's name to match
> the client.
> """
>
> So how does that work - this single process temporarily changes it's
> name for each drm fd it opens and creates a context or it is actually in
> the native context protocol?

It is part of the protocol, the mesa driver in the VM sends[1] this
info to the native-context "shim" in host userspace which uses the
SET_PARAM ioctl to pass this to the kernel.  In the host userspace
there is just a single process (you see the host PID below) but it
does a separate open() of the drm dev for each guest process (so that
they each have their own GPU address space for isolation):

DRM minor 128
    PID    MEM ACTIV              NAME                    gpu
    5297  200M   82M com.mojang.minecr |██████████████▏                        |
    1859  199M    0B            chrome |█▉                                     |
    5297   64M    9M    surfaceflinger |                                       |
    5297   12M    0B org.chromium.arc. |                                       |
    5297   12M    0B com.android.syste |                                       |
    5297   12M    0B org.chromium.arc. |                                       |
    5297   26M    0B com.google.androi |                                       |
    5297   65M    0B     system_server |                                       |


[1] https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/drm/msm/msm_proto.h#L326
[2] https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/drm/msm/msm_renderer.c#L1050

> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >   Documentation/gpu/drm-usage-stats.rst |  8 ++++++++
> >   drivers/gpu/drm/msm/msm_gpu.c         | 14 ++++++++++++++
> >   2 files changed, 22 insertions(+)
> >
> > diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> > index 8e00d53231e0..bc90bed455e3 100644
> > --- a/Documentation/gpu/drm-usage-stats.rst
> > +++ b/Documentation/gpu/drm-usage-stats.rst
> > @@ -148,6 +148,14 @@ percentage utilization of the engine, whereas drm-engine-<keystr> only reflects
> >   time active without considering what frequency the engine is operating as a
> >   percentage of it's maximum frequency.
> >
> > +- drm-comm: <valstr>
> > +
> > +Returns the clients executable path.
>
> Full path and not just current->comm? In this case probably give it a
> more descriptive name here.
>
> drm-client-executable
> drm-client-command-line
>
> So we stay in the drm-client- namespace?
>
> Or if the former is absolute path could one key be enough for both?
>
> drm-client-command-line: /path/to/executable --arguments

comm and cmdline can be different. Android seems to change the comm to
the apk name, for example (and w/ the zygote stuff cmdline isn't
really a thing)

I guess it could be drm-client-comm and drm-client-cmdline?  Although
comm/cmdline aren't the best names, they are just following what the
kernel calls them elsewhere.

> > +
> > +- drm-cmdline: <valstr>
> > +
> > +Returns the clients cmdline.
>
> I think drm-usage-stats.rst text should provide some more text with
> these two. To precisely define their content and outline the use case
> under which driver authors may want to add them, and fdinfo consumer
> therefore expect to see them. Just so everything is completely clear and
> people do not start adding them for drivers which do not support native
> context (or like).

I really was just piggy-backing on existing comm/cmdline.. but I'll
try to write up something better.

I think it maybe should not be limited just to native context.. for
ex. if the browser did somehow manage to create different displays
associated with different drm_file instances (I guess it would have to
use gbm to do this?) it would be nice to see browser tab names.

> But on the overall it sounds reasonable to me - it would be really cool
> to not just see pcivirtio-gpu as you say. Even if the standard virtiogpu
> use case (not native context) could show real users.

For vrend/virgl, we'd first need to solve the issue that there is just
a single drm_file for all guest processes.  But really, just don't use
virgl.  (I mean, like seriously, would you put a gl driver in the
kernel?  Vrend has access to all guest memory, so this is essentially
what you have with virgl.  This is just not a sane thing to do.) The
only "valid" reason for not doing native-context is if you don't have
the src code for your UMD to be able to modify it to talk
native-context to virtgpu in the guest. ;-)

BR,
-R

> Regards,
>
> Tvrtko
>
> > +
> >   Implementation Details
> >   ======================
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > index f0f4f845c32d..1150dcbf28aa 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > @@ -148,12 +148,26 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
> >       return 0;
> >   }
> >
> > +static void get_comm_cmdline(struct msm_file_private *ctx, char **comm, char **cmd);
> > +
> >   void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
> >                        struct drm_printer *p)
> >   {
> > +     char *comm, *cmdline;
> > +
> > +     get_comm_cmdline(ctx, &comm, &cmdline);
> > +
> >       drm_printf(p, "drm-engine-gpu:\t%llu ns\n", ctx->elapsed_ns);
> >       drm_printf(p, "drm-cycles-gpu:\t%llu\n", ctx->cycles);
> >       drm_printf(p, "drm-maxfreq-gpu:\t%u Hz\n", gpu->fast_rate);
> > +
> > +     if (comm)
> > +             drm_printf(p, "drm-comm:\t%s\n", comm);
> > +     if (cmdline)
> > +             drm_printf(p, "drm-cmdline:\t%s\n", cmdline);
> > +
> > +     kfree(comm);
> > +     kfree(cmdline);
> >   }
> >
> >   int msm_gpu_hw_init(struct msm_gpu *gpu)

Re: [RFC 3/3] drm/msm: Add comm/cmdline fields

[Tvrtko Ursulin]

On 18/04/2023 15:56, Rob Clark wrote:
> On Tue, Apr 18, 2023 at 1:53 AM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 17/04/2023 21:12, Rob Clark wrote:
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> Normally this would be the same information that can be obtained in
>>> other ways.  But in some cases the process opening the drm fd is merely
>>> a sort of proxy for the actual process using the GPU.  This is the case
>>> for guest VM processes using the GPU via virglrenderer, in which case
>>> the msm native-context renderer in virglrenderer overrides the comm/
>>> cmdline to be the guest process's values.
>>>
>>> Exposing this via fdinfo allows tools like gputop to show something more
>>> meaningful than just a bunch of "pcivirtio-gpu" users.
>>
>> You also later expanded with:
>>
>> """
>> I should have also mentioned, in the VM/proxy scenario we have a
>> single process with separate drm_file's for each guest VM process.  So
>> it isn't an option to just change the proxy process's name to match
>> the client.
>> """
>>
>> So how does that work - this single process temporarily changes it's
>> name for each drm fd it opens and creates a context or it is actually in
>> the native context protocol?
> 
> It is part of the protocol, the mesa driver in the VM sends[1] this
> info to the native-context "shim" in host userspace which uses the
> SET_PARAM ioctl to pass this to the kernel.  In the host userspace
> there is just a single process (you see the host PID below) but it
> does a separate open() of the drm dev for each guest process (so that
> they each have their own GPU address space for isolation):
> 
> DRM minor 128
>      PID    MEM ACTIV              NAME                    gpu
>      5297  200M   82M com.mojang.minecr |██████████████▏                        |
>      1859  199M    0B            chrome |█▉                                     |
>      5297   64M    9M    surfaceflinger |                                       |
>      5297   12M    0B org.chromium.arc. |                                       |
>      5297   12M    0B com.android.syste |                                       |
>      5297   12M    0B org.chromium.arc. |                                       |
>      5297   26M    0B com.google.androi |                                       |
>      5297   65M    0B     system_server |                                       |
> 
> 
> [1] https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/drm/msm/msm_proto.h#L326
> [2] https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/drm/msm/msm_renderer.c#L1050
> 
>>>
>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>> ---
>>>    Documentation/gpu/drm-usage-stats.rst |  8 ++++++++
>>>    drivers/gpu/drm/msm/msm_gpu.c         | 14 ++++++++++++++
>>>    2 files changed, 22 insertions(+)
>>>
>>> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
>>> index 8e00d53231e0..bc90bed455e3 100644
>>> --- a/Documentation/gpu/drm-usage-stats.rst
>>> +++ b/Documentation/gpu/drm-usage-stats.rst
>>> @@ -148,6 +148,14 @@ percentage utilization of the engine, whereas drm-engine-<keystr> only reflects
>>>    time active without considering what frequency the engine is operating as a
>>>    percentage of it's maximum frequency.
>>>
>>> +- drm-comm: <valstr>
>>> +
>>> +Returns the clients executable path.
>>
>> Full path and not just current->comm? In this case probably give it a
>> more descriptive name here.
>>
>> drm-client-executable
>> drm-client-command-line
>>
>> So we stay in the drm-client- namespace?
>>
>> Or if the former is absolute path could one key be enough for both?
>>
>> drm-client-command-line: /path/to/executable --arguments
> 
> comm and cmdline can be different. Android seems to change the comm to
> the apk name, for example (and w/ the zygote stuff cmdline isn't
> really a thing)
> 
> I guess it could be drm-client-comm and drm-client-cmdline?  Although
> comm/cmdline aren't the best names, they are just following what the
> kernel calls them elsewhere.

I wasn't sure what do you plan to do given mention of a path under the 
drm-comm description. If it is a path then comm would be misleading, 
since comm as defined in procfs is not a path, I don't think so at 
least. Which is why I was suggesting executable. But if you remove the 
mention of a path from rst and rather refer to processes' comm value I 
think that is then okay.

>>> +
>>> +- drm-cmdline: <valstr>
>>> +
>>> +Returns the clients cmdline.
>>
>> I think drm-usage-stats.rst text should provide some more text with
>> these two. To precisely define their content and outline the use case
>> under which driver authors may want to add them, and fdinfo consumer
>> therefore expect to see them. Just so everything is completely clear and
>> people do not start adding them for drivers which do not support native
>> context (or like).
> 
> I really was just piggy-backing on existing comm/cmdline.. but I'll
> try to write up something better.
> 
> I think it maybe should not be limited just to native context.. for
> ex. if the browser did somehow manage to create different displays
> associated with different drm_file instances (I guess it would have to
> use gbm to do this?) it would be nice to see browser tab names.

Would be cool yes.

My thinking behind why we maybe do not want to blanket add them is 
because for common case is it the same information which can be obtained 
from procfs. Like in igt_drm_clients.c I get the pid and comm from 
/proc/$pid/stat. So I was thinking it is only interesting to add to 
fdinfo for drivers where it could differ by the explicit override like 
you have with native context.

It can be added once there is a GL/whatever extension which would allow 
it? (I am not familiar with how browsers manage rendering contexts so 
maybe I am missing something.)

>> But on the overall it sounds reasonable to me - it would be really cool
>> to not just see pcivirtio-gpu as you say. Even if the standard virtiogpu
>> use case (not native context) could show real users.
> 
> For vrend/virgl, we'd first need to solve the issue that there is just
> a single drm_file for all guest processes.  But really, just don't use
> virgl.  (I mean, like seriously, would you put a gl driver in the
> kernel?  Vrend has access to all guest memory, so this is essentially
> what you have with virgl.  This is just not a sane thing to do.) The
> only "valid" reason for not doing native-context is if you don't have
> the src code for your UMD to be able to modify it to talk
> native-context to virtgpu in the guest. ;-)

I am just observing the current state of things on an Intel based 
Chromebook. :) Presumably the custom name for a context would be 
passable via the virtio-gpu protocol or something?

Regards,

Tvrtko

> 
> BR,
> -R
> 
>> Regards,
>>
>> Tvrtko
>>
>>> +
>>>    Implementation Details
>>>    ======================
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
>>> index f0f4f845c32d..1150dcbf28aa 100644
>>> --- a/drivers/gpu/drm/msm/msm_gpu.c
>>> +++ b/drivers/gpu/drm/msm/msm_gpu.c
>>> @@ -148,12 +148,26 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
>>>        return 0;
>>>    }
>>>
>>> +static void get_comm_cmdline(struct msm_file_private *ctx, char **comm, char **cmd);
>>> +
>>>    void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
>>>                         struct drm_printer *p)
>>>    {
>>> +     char *comm, *cmdline;
>>> +
>>> +     get_comm_cmdline(ctx, &comm, &cmdline);
>>> +
>>>        drm_printf(p, "drm-engine-gpu:\t%llu ns\n", ctx->elapsed_ns);
>>>        drm_printf(p, "drm-cycles-gpu:\t%llu\n", ctx->cycles);
>>>        drm_printf(p, "drm-maxfreq-gpu:\t%u Hz\n", gpu->fast_rate);
>>> +
>>> +     if (comm)
>>> +             drm_printf(p, "drm-comm:\t%s\n", comm);
>>> +     if (cmdline)
>>> +             drm_printf(p, "drm-cmdline:\t%s\n", cmdline);
>>> +
>>> +     kfree(comm);
>>> +     kfree(cmdline);
>>>    }
>>>
>>>    int msm_gpu_hw_init(struct msm_gpu *gpu)

Re: [RFC 3/3] drm/msm: Add comm/cmdline fields

[Rob Clark]

On Wed, Apr 19, 2023 at 6:36 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 18/04/2023 15:56, Rob Clark wrote:
> > On Tue, Apr 18, 2023 at 1:53 AM Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> >>
> >>
> >> On 17/04/2023 21:12, Rob Clark wrote:
> >>> From: Rob Clark <robdclark@chromium.org>
> >>>
> >>> Normally this would be the same information that can be obtained in
> >>> other ways.  But in some cases the process opening the drm fd is merely
> >>> a sort of proxy for the actual process using the GPU.  This is the case
> >>> for guest VM processes using the GPU via virglrenderer, in which case
> >>> the msm native-context renderer in virglrenderer overrides the comm/
> >>> cmdline to be the guest process's values.
> >>>
> >>> Exposing this via fdinfo allows tools like gputop to show something more
> >>> meaningful than just a bunch of "pcivirtio-gpu" users.
> >>
> >> You also later expanded with:
> >>
> >> """
> >> I should have also mentioned, in the VM/proxy scenario we have a
> >> single process with separate drm_file's for each guest VM process.  So
> >> it isn't an option to just change the proxy process's name to match
> >> the client.
> >> """
> >>
> >> So how does that work - this single process temporarily changes it's
> >> name for each drm fd it opens and creates a context or it is actually in
> >> the native context protocol?
> >
> > It is part of the protocol, the mesa driver in the VM sends[1] this
> > info to the native-context "shim" in host userspace which uses the
> > SET_PARAM ioctl to pass this to the kernel.  In the host userspace
> > there is just a single process (you see the host PID below) but it
> > does a separate open() of the drm dev for each guest process (so that
> > they each have their own GPU address space for isolation):
> >
> > DRM minor 128
> >      PID    MEM ACTIV              NAME                    gpu
> >      5297  200M   82M com.mojang.minecr |██████████████▏                        |
> >      1859  199M    0B            chrome |█▉                                     |
> >      5297   64M    9M    surfaceflinger |                                       |
> >      5297   12M    0B org.chromium.arc. |                                       |
> >      5297   12M    0B com.android.syste |                                       |
> >      5297   12M    0B org.chromium.arc. |                                       |
> >      5297   26M    0B com.google.androi |                                       |
> >      5297   65M    0B     system_server |                                       |
> >
> >
> > [1] https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/drm/msm/msm_proto.h#L326
> > [2] https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/drm/msm/msm_renderer.c#L1050
> >
> >>>
> >>> Signed-off-by: Rob Clark <robdclark@chromium.org>
> >>> ---
> >>>    Documentation/gpu/drm-usage-stats.rst |  8 ++++++++
> >>>    drivers/gpu/drm/msm/msm_gpu.c         | 14 ++++++++++++++
> >>>    2 files changed, 22 insertions(+)
> >>>
> >>> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> >>> index 8e00d53231e0..bc90bed455e3 100644
> >>> --- a/Documentation/gpu/drm-usage-stats.rst
> >>> +++ b/Documentation/gpu/drm-usage-stats.rst
> >>> @@ -148,6 +148,14 @@ percentage utilization of the engine, whereas drm-engine-<keystr> only reflects
> >>>    time active without considering what frequency the engine is operating as a
> >>>    percentage of it's maximum frequency.
> >>>
> >>> +- drm-comm: <valstr>
> >>> +
> >>> +Returns the clients executable path.
> >>
> >> Full path and not just current->comm? In this case probably give it a
> >> more descriptive name here.
> >>
> >> drm-client-executable
> >> drm-client-command-line
> >>
> >> So we stay in the drm-client- namespace?
> >>
> >> Or if the former is absolute path could one key be enough for both?
> >>
> >> drm-client-command-line: /path/to/executable --arguments
> >
> > comm and cmdline can be different. Android seems to change the comm to
> > the apk name, for example (and w/ the zygote stuff cmdline isn't
> > really a thing)
> >
> > I guess it could be drm-client-comm and drm-client-cmdline?  Although
> > comm/cmdline aren't the best names, they are just following what the
> > kernel calls them elsewhere.
>
> I wasn't sure what do you plan to do given mention of a path under the
> drm-comm description. If it is a path then comm would be misleading,
> since comm as defined in procfs is not a path, I don't think so at
> least. Which is why I was suggesting executable. But if you remove the
> mention of a path from rst and rather refer to processes' comm value I
> think that is then okay.

Oh, whoops the mention of "path" for comm was a mistake.  task->comm
is described as executable name without path, and that is what the
fdinfo field was intending to follow.

> >>> +
> >>> +- drm-cmdline: <valstr>
> >>> +
> >>> +Returns the clients cmdline.
> >>
> >> I think drm-usage-stats.rst text should provide some more text with
> >> these two. To precisely define their content and outline the use case
> >> under which driver authors may want to add them, and fdinfo consumer
> >> therefore expect to see them. Just so everything is completely clear and
> >> people do not start adding them for drivers which do not support native
> >> context (or like).
> >
> > I really was just piggy-backing on existing comm/cmdline.. but I'll
> > try to write up something better.
> >
> > I think it maybe should not be limited just to native context.. for
> > ex. if the browser did somehow manage to create different displays
> > associated with different drm_file instances (I guess it would have to
> > use gbm to do this?) it would be nice to see browser tab names.
>
> Would be cool yes.
>
> My thinking behind why we maybe do not want to blanket add them is
> because for common case is it the same information which can be obtained
> from procfs. Like in igt_drm_clients.c I get the pid and comm from
> /proc/$pid/stat. So I was thinking it is only interesting to add to
> fdinfo for drivers where it could differ by the explicit override like
> you have with native context.

Yeah, I suppose I could define them as drm-client-comm-override and
drm-client-cmdline-override

> It can be added once there is a GL/whatever extension which would allow
> it? (I am not familiar with how browsers manage rendering contexts so
> maybe I am missing something.)
>
> >> But on the overall it sounds reasonable to me - it would be really cool
> >> to not just see pcivirtio-gpu as you say. Even if the standard virtiogpu
> >> use case (not native context) could show real users.
> >
> > For vrend/virgl, we'd first need to solve the issue that there is just
> > a single drm_file for all guest processes.  But really, just don't use
> > virgl.  (I mean, like seriously, would you put a gl driver in the
> > kernel?  Vrend has access to all guest memory, so this is essentially
> > what you have with virgl.  This is just not a sane thing to do.) The
> > only "valid" reason for not doing native-context is if you don't have
> > the src code for your UMD to be able to modify it to talk
> > native-context to virtgpu in the guest. ;-)
>
> I am just observing the current state of things on an Intel based
> Chromebook. :) Presumably the custom name for a context would be
> passable via the virtio-gpu protocol or something?

It is part of the context-type specific protocol.  Ie. some parts of
the protocol are "core" and dealt with in virtgpu guest kernel driver.
But on top of that there are various context-types with their own
protocol (ie. virgl, venus, cross-domain, msm native ctx, and some WIP
native ctx types floating around)

BR,
-R

> Regards,
>
> Tvrtko
>
> >
> > BR,
> > -R
> >
> >> Regards,
> >>
> >> Tvrtko
> >>
> >>> +
> >>>    Implementation Details
> >>>    ======================
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> >>> index f0f4f845c32d..1150dcbf28aa 100644
> >>> --- a/drivers/gpu/drm/msm/msm_gpu.c
> >>> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> >>> @@ -148,12 +148,26 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
> >>>        return 0;
> >>>    }
> >>>
> >>> +static void get_comm_cmdline(struct msm_file_private *ctx, char **comm, char **cmd);
> >>> +
> >>>    void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
> >>>                         struct drm_printer *p)
> >>>    {
> >>> +     char *comm, *cmdline;
> >>> +
> >>> +     get_comm_cmdline(ctx, &comm, &cmdline);
> >>> +
> >>>        drm_printf(p, "drm-engine-gpu:\t%llu ns\n", ctx->elapsed_ns);
> >>>        drm_printf(p, "drm-cycles-gpu:\t%llu\n", ctx->cycles);
> >>>        drm_printf(p, "drm-maxfreq-gpu:\t%u Hz\n", gpu->fast_rate);
> >>> +
> >>> +     if (comm)
> >>> +             drm_printf(p, "drm-comm:\t%s\n", comm);
> >>> +     if (cmdline)
> >>> +             drm_printf(p, "drm-cmdline:\t%s\n", cmdline);
> >>> +
> >>> +     kfree(comm);
> >>> +     kfree(cmdline);
> >>>    }
> >>>
> >>>    int msm_gpu_hw_init(struct msm_gpu *gpu)

Re: [RFC 0/3] drm: Add comm/cmdline fdinfo fields

[Rob Clark]

On Mon, Apr 17, 2023 at 1:12 PM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> When many of the things using the GPU are processes in a VM guest, the
> actual client process is just a proxy.  The msm driver has a way to let
> the proxy tell the kernel the actual VM client process's executable name
> and command-line, which has until now been used simply for GPU crash
> devcore dumps.  Lets also expose this via fdinfo so that tools can
> expose who the actual user of the GPU is.

I should have also mentioned, in the VM/proxy scenario we have a
single process with separate drm_file's for each guest VM process.  So
it isn't an option to just change the proxy process's name to match
the client.

> Rob Clark (3):
>   drm/doc: Relax fdinfo string constraints
>   drm/msm: Rework get_comm_cmdline() helper
>   drm/msm: Add comm/cmdline fields
>
>  Documentation/gpu/drm-usage-stats.rst   | 37 +++++++++++++++----------
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 +--
>  drivers/gpu/drm/msm/msm_drv.c           |  2 ++
>  drivers/gpu/drm/msm/msm_gpu.c           | 27 +++++++++++++-----
>  drivers/gpu/drm/msm/msm_gpu.h           | 12 ++++++--
>  drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
>  6 files changed, 58 insertions(+), 25 deletions(-)
>
> --
> 2.39.2
>

Re: [RFC 0/3] drm: Add comm/cmdline fdinfo fields

[Konrad Dybcio]

Looks like the 'PATCH' part of your subject was cut off!

Konrad

On 17.04.2023 22:12, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> When many of the things using the GPU are processes in a VM guest, the
> actual client process is just a proxy.  The msm driver has a way to let
> the proxy tell the kernel the actual VM client process's executable name
> and command-line, which has until now been used simply for GPU crash
> devcore dumps.  Lets also expose this via fdinfo so that tools can
> expose who the actual user of the GPU is.
> 
> Rob Clark (3):
>   drm/doc: Relax fdinfo string constraints
>   drm/msm: Rework get_comm_cmdline() helper
>   drm/msm: Add comm/cmdline fields
> 
>  Documentation/gpu/drm-usage-stats.rst   | 37 +++++++++++++++----------
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 +--
>  drivers/gpu/drm/msm/msm_drv.c           |  2 ++
>  drivers/gpu/drm/msm/msm_gpu.c           | 27 +++++++++++++-----
>  drivers/gpu/drm/msm/msm_gpu.h           | 12 ++++++--
>  drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
>  6 files changed, 58 insertions(+), 25 deletions(-)
>