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

[Matt Coster <Matt.Coster@imgtec.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3656 bytes --]

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?

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)?

> Also an old incorrect comment was removed.

This detail probably doesn't need to be mentioned.

Cheers,
Matt

> 
> Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
> ---
>  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
> 


-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]