Re: [PATCH 1/4] drm/imagination: Populate FW common context ID before passing to the FW

[Brajesh Gupta]

On Mon, 2026-05-18 at 15:01 +0000, Matt Coster wrote:
> Hi Brajesh,
> 
> On 12/05/2026 07:47, Brajesh Gupta wrote:
> > Fix context ID value for the FW common context by moving the context
> > allocation earlier.
> 
> "Fix" is pretty vague here – can you describe the current behaviour, and
> why that's wrong?

Updated

> 
> I also assume this fixes a previous change, so can you please add a
> Fixes: tag referencing the commit which introduced the faulty behaviour
> (and possibly a Cc: stable tag, if needed)?
> 
Didn't include fix tag as Context ID is not used currently in driver so nothing
is broken.
> > Also an old incorrect comment was removed.
> 
> This detail probably doesn't need to be mentioned.
> 
Dropped

> Cheers,
> Matt
> 
> > 
> > Signed-off-by: Brajesh Gupta <brajesh.gupta@xxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/imagination/pvr_context.c | 30 ++++++++++++++++--------------
> >  1 file changed, 16 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imagination/pvr_context.c b/drivers/gpu/drm/imagination/pvr_context.c
> > index 8de70c30b9de..eba4694400b5 100644
> > --- a/drivers/gpu/drm/imagination/pvr_context.c
> > +++ b/drivers/gpu/drm/imagination/pvr_context.c
> > @@ -318,10 +318,14 @@ int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_co
> >                 goto err_put_vm;
> >         }
> > 
> > -       err = pvr_context_create_queues(ctx, args, ctx->data);
> > +       err = xa_alloc(&pvr_dev->ctx_ids, &ctx->ctx_id, ctx, xa_limit_32b, GFP_KERNEL);
> >         if (err)
> >                 goto err_free_ctx_data;
> > 
> > +       err = pvr_context_create_queues(ctx, args, ctx->data);
> > +       if (err)
> > +               goto err_free_ctx_id;
> > +
> >         err = init_fw_objs(ctx, args, ctx->data);
> >         if (err)
> >                 goto err_destroy_queues;
> > @@ -329,23 +333,12 @@ int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_co
> >         err = pvr_fw_object_create(pvr_dev, ctx_size, PVR_BO_FW_FLAGS_DEVICE_UNCACHED,
> >                                    ctx_fw_data_init, ctx, &ctx->fw_obj);
> >         if (err)
> > -               goto err_free_ctx_data;
> > +               goto err_destroy_queues;
> > 
> > -       err = xa_alloc(&pvr_dev->ctx_ids, &ctx->ctx_id, ctx, xa_limit_32b, GFP_KERNEL);
> > +       err = xa_alloc(&pvr_file->ctx_handles, &args->handle, ctx, xa_limit_32b, GFP_KERNEL);
> >         if (err)
> >                 goto err_destroy_fw_obj;
> > 
> > -       err = xa_alloc(&pvr_file->ctx_handles, &args->handle, ctx, xa_limit_32b, GFP_KERNEL);
> > -       if (err) {
> > -               /*
> > -                * It's possible that another thread could have taken a reference on the context at
> > -                * this point as it is in the ctx_ids xarray. Therefore instead of directly
> > -                * destroying the context, drop a reference instead.
> > -                */
> > -               pvr_context_put(ctx);
> > -               return err;
> > -       }
> > -
> >         spin_lock(&pvr_dev->ctx_list_lock);
> >         list_add_tail(&ctx->file_link, &pvr_file->contexts);
> >         spin_unlock(&pvr_dev->ctx_list_lock);
> > @@ -358,6 +351,15 @@ int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_co
> >  err_destroy_queues:
> >         pvr_context_destroy_queues(ctx);
> > 
> > +err_free_ctx_id:
> > +       /*
> > +        * Ctx_id is not exposed to userspace and not visible yet within
> > +        * the kernel/FW, plus a matching context handle (exposed to userspace)
> > +        * hasn't been allocated yet, so it is safe to remove ctx_id
> > +        * from the ctx_ids xarray.
> > +        */
> > +       xa_erase(&pvr_dev->ctx_ids, ctx->ctx_id);
> > +
> >  err_free_ctx_data:
> >         kfree(ctx->data);
> > 
> > 
> > --
> > 2.43.0
> > 
> 
> 
Thanks,
Brajesh