Re: [PATCH v2 1/2] drm/imagination: Add a per-file PVR context list

[Frank Binns <Frank.Binns@imgtec.com>]

On Mon, 2024-10-14 at 14:22 +0000, Matt Coster wrote:
> From: Brendan King <brendan.king@imgtec.com>
> 
> This adds a linked list of VM contexts which is needed for the next patch
> to be able to correctly track VM contexts for destruction on file close.
> 
> It is only safe for VM contexts to be removed from the list and destroyed
> when not in interrupt context.
> 

Reviewed-by: Frank Binns <frank.binns@imgtec.com>

> Signed-off-by: Brendan King <brendan.king@imgtec.com>
> Signed-off-by: Matt Coster <matt.coster@imgtec.com>
> ---
> Changes in v1 -> v2:
>  - Add justification to the commit message so it stands on its own (e.g.
>    during bisect)
> ---
>  drivers/gpu/drm/imagination/pvr_context.c | 14 ++++++++++++++
>  drivers/gpu/drm/imagination/pvr_context.h |  3 +++
>  drivers/gpu/drm/imagination/pvr_device.h  | 10 ++++++++++
>  drivers/gpu/drm/imagination/pvr_drv.c     |  3 +++
>  4 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_context.c b/drivers/gpu/drm/imagination/pvr_context.c
> index eded5e955cc0..255c93582734 100644
> --- a/drivers/gpu/drm/imagination/pvr_context.c
> +++ b/drivers/gpu/drm/imagination/pvr_context.c
> @@ -17,10 +17,14 @@
>  
>  #include <drm/drm_auth.h>
>  #include <drm/drm_managed.h>
> +
> +#include <linux/bug.h>
>  #include <linux/errno.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
>  #include <linux/xarray.h>
> @@ -354,6 +358,10 @@ int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_co
>  		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);
> +
>  	return 0;
>  
>  err_destroy_fw_obj:
> @@ -380,6 +388,11 @@ pvr_context_release(struct kref *ref_count)
>  		container_of(ref_count, struct pvr_context, ref_count);
>  	struct pvr_device *pvr_dev = ctx->pvr_dev;
>  
> +	WARN_ON(in_interrupt());
> +	spin_lock(&pvr_dev->ctx_list_lock);
> +	list_del(&ctx->file_link);
> +	spin_unlock(&pvr_dev->ctx_list_lock);
> +
>  	xa_erase(&pvr_dev->ctx_ids, ctx->ctx_id);
>  	pvr_context_destroy_queues(ctx);
>  	pvr_fw_object_destroy(ctx->fw_obj);
> @@ -451,6 +464,7 @@ void pvr_destroy_contexts_for_file(struct pvr_file *pvr_file)
>  void pvr_context_device_init(struct pvr_device *pvr_dev)
>  {
>  	xa_init_flags(&pvr_dev->ctx_ids, XA_FLAGS_ALLOC1);
> +	spin_lock_init(&pvr_dev->ctx_list_lock);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/imagination/pvr_context.h b/drivers/gpu/drm/imagination/pvr_context.h
> index 0c7b97dfa6ba..a5b0a82a54a1 100644
> --- a/drivers/gpu/drm/imagination/pvr_context.h
> +++ b/drivers/gpu/drm/imagination/pvr_context.h
> @@ -85,6 +85,9 @@ struct pvr_context {
>  		/** @compute: Transfer queue. */
>  		struct pvr_queue *transfer;
>  	} queues;
> +
> +	/** @file_link: pvr_file PVR context list link. */
> +	struct list_head file_link;
>  };
>  
>  static __always_inline struct pvr_queue *
> diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
> index b574e23d484b..6d0dfacb677b 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.h
> +++ b/drivers/gpu/drm/imagination/pvr_device.h
> @@ -23,6 +23,7 @@
>  #include <linux/kernel.h>
>  #include <linux/math.h>
>  #include <linux/mutex.h>
> +#include <linux/spinlock_types.h>
>  #include <linux/timer.h>
>  #include <linux/types.h>
>  #include <linux/wait.h>
> @@ -293,6 +294,12 @@ struct pvr_device {
>  
>  	/** @sched_wq: Workqueue for schedulers. */
>  	struct workqueue_struct *sched_wq;
> +
> +	/**
> +	 * @ctx_list_lock: Lock to be held when accessing the context list in
> +	 *  struct pvr_file.
> +	 */
> +	spinlock_t ctx_list_lock;
>  };
>  
>  /**
> @@ -344,6 +351,9 @@ struct pvr_file {
>  	 * This array is used to allocate handles returned to userspace.
>  	 */
>  	struct xarray vm_ctx_handles;
> +
> +	/** @contexts: PVR context list. */
> +	struct list_head contexts;
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index 1a0cb7aa9cea..fb17196e05f4 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -28,6 +28,7 @@
>  #include <linux/export.h>
>  #include <linux/fs.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> @@ -1326,6 +1327,8 @@ pvr_drm_driver_open(struct drm_device *drm_dev, struct drm_file *file)
>  	 */
>  	pvr_file->pvr_dev = pvr_dev;
>  
> +	INIT_LIST_HEAD(&pvr_file->contexts);
> +
>  	xa_init_flags(&pvr_file->ctx_handles, XA_FLAGS_ALLOC1);
>  	xa_init_flags(&pvr_file->free_list_handles, XA_FLAGS_ALLOC1);
>  	xa_init_flags(&pvr_file->hwrt_handles, XA_FLAGS_ALLOC1);