[PATCH v2 2/6] drm/i915/gem: Implement a function to process all gem objects of a region

[Matthew Auld]

On 06/09/2021 17:55, Thomas Hellström wrote:
> An upcoming common pattern is to traverse the region object list and
> perform certain actions on all objects in a region. It's a little tricky
> to get the list locking right, in particular since a gem object may
> change region unless it's pinned or the object lock is held.
> 
> Define a function that does this for us and that takes an argument that
> defines the action to be performed on each object.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_region.c | 70 ++++++++++++++++++++++
>   drivers/gpu/drm/i915/gem/i915_gem_region.h | 33 ++++++++++
>   2 files changed, 103 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> index 1f557b2178ed..a016ccec36f3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> @@ -80,3 +80,73 @@ i915_gem_object_create_region(struct intel_memory_region *mem,
>   	i915_gem_object_free(obj);
>   	return ERR_PTR(err);
>   }
> +
> +/**
> + * i915_gem_process_region - Iterate over all objects of a region using ops
> + * to process and optionally skip objects
> + * @mr: The memory region
> + * @apply: ops and private data
> + *
> + * This function can be used to iterate over the regions object list,
> + * checking whether to skip objects, and, if not, lock the objects and
> + * process them using the supplied ops. Note that this function temporarily
> + * removes objects from the region list while iterating, so that if run
> + * concurrently with itself may not iterate over all objects.
> + *
> + * Return: 0 if successful, negative error code on failure.
> + */
> +int i915_gem_process_region(struct intel_memory_region *mr,
> +			    struct i915_gem_apply_to_region *apply)
> +{
> +	const struct i915_gem_apply_to_region_ops *ops = apply->ops;
> +	struct drm_i915_gem_object *obj;
> +	struct list_head still_in_list;
> +	int ret = 0;
> +
> +	/*
> +	 * In the future, a non-NULL apply->ww could mean the caller is
> +	 * already in a locking transaction and provides its own context.
> +	 */
> +	GEM_WARN_ON(apply->ww);
> +
> +	INIT_LIST_HEAD(&still_in_list);
> +	mutex_lock(&mr->objects.lock);
> +	for (;;) {
> +		struct i915_gem_ww_ctx ww;
> +
> +		obj = list_first_entry_or_null(&mr->objects.list, typeof(*obj),
> +					       mm.region_link);
> +		if (!obj)
> +			break;
> +
> +		list_move_tail(&obj->mm.region_link, &still_in_list);
> +		if (!kref_get_unless_zero(&obj->base.refcount))
> +			continue;
> +
> +		/*
> +		 * Note: Someone else might be migrating the object at this
> +		 * point. The object's region is not stable until we lock
> +		 * the object.
> +		 */
> +		mutex_unlock(&mr->objects.lock);
> +		apply->ww = &ww;
> +		for_i915_gem_ww(&ww, ret, apply->interruptible) {
> +			ret = i915_gem_object_lock(obj, apply->ww);
> +			if (ret)
> +				continue;
> +
> +			if (obj->mm.region == mr)
> +				ret = ops->process_obj(apply, obj);
> +			/* Implicit object unlock */
> +		}
> +
> +		i915_gem_object_put(obj);
> +		mutex_lock(&mr->objects.lock);
> +		if (ret)
> +			break;
> +	}
> +	list_splice_tail(&still_in_list, &mr->objects.list);
> +	mutex_unlock(&mr->objects.lock);
> +
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.h b/drivers/gpu/drm/i915/gem/i915_gem_region.h
> index 1008e580a89a..f62195847056 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_region.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.h
> @@ -12,6 +12,37 @@ struct intel_memory_region;
>   struct drm_i915_gem_object;
>   struct sg_table;
>   
> +struct i915_gem_apply_to_region;
> +
> +/**
> + * struct i915_gem_apply_to_region_ops - ops to use when iterating over all
> + * region objects.
> + */
> +struct i915_gem_apply_to_region_ops {
> +	/**
> +	 * process_obj - Process the current object

Should we mention that returning -EDEADLK might result in re-entering 
the process_obj?

> +	 * @apply: Embed this for provate data

s/provate/private

> +	 * @obj: The current object.
> +	 */
> +	int (*process_obj)(struct i915_gem_apply_to_region *apply,
> +			   struct drm_i915_gem_object *obj);
> +};
> +
> +/**
> + * struct i915_gem_apply_to_region - Argument to the struct
> + * i915_gem_apply_to_region_ops functions.
> + * @ops: The ops for the operation.
> + * @ww: Locking context used for the transaction.
> + * @interruptible: Whether to perform object locking interruptible.
> + *
> + * This structure is intended to be embedded in a private struct if needed
> + */
> +struct i915_gem_apply_to_region {
> +	const struct i915_gem_apply_to_region_ops *ops;
> +	struct i915_gem_ww_ctx *ww;
> +	u32 interruptible:1;
> +};
> +
>   void i915_gem_object_init_memory_region(struct drm_i915_gem_object *obj,
>   					struct intel_memory_region *mem);
>   void i915_gem_object_release_memory_region(struct drm_i915_gem_object *obj);
> @@ -22,4 +53,6 @@ i915_gem_object_create_region(struct intel_memory_region *mem,
>   			      resource_size_t page_size,
>   			      unsigned int flags);
>   
> +int i915_gem_process_region(struct intel_memory_region *mr,
> +			    struct i915_gem_apply_to_region *apply);
>   #endif
>