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 <robdcl...@chromium.org>
> >
> > Make it work in terms of ctx so that it can be re-used for fdinfo.
> >
> > Signed-off-by: Rob Clark <robdcl...@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
