Re: [Intel-gfx] [PATCH 16/20] drm/i915/gem: Reintroduce multiple passes for reloc processing

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 06/07/2020 07:19, Chris Wilson wrote:
> The prospect of locking the entire submission sequence under a wide
> ww_mutex re-imposes some key restrictions, in particular that we must
> not call copy_(from|to)_user underneath the mutex (as the faulthandlers
> themselves may need to take the ww_mutex). To satisfy this requirement,
> we need to split the relocation handling into multiple phases again.
> After dropping the reservations, we need to allocate enough buffer space
> to both copy the relocations from userspace into, and serve as the
> relocation command buffer. Once we have finished copying the
> relocations, we can then re-aquire all the objects for the execbuf and
> rebind them, including our new relocations objects. After we have bound
> all the new and old objects into their final locations, we can then
> convert the relocation entries into the GPU commands to update the
> relocated vma. Finally, once it is all over and we have dropped the
> ww_mutex for the last time, we can then complete the update of the user
> relocation entries.

Good text. :)

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 842 ++++++++----------
>   .../i915/gem/selftests/i915_gem_execbuffer.c  | 195 ++--
>   2 files changed, 520 insertions(+), 517 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 629b736adf2c..fbf5c5cd51ca 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -35,6 +35,7 @@ struct eb_vma {
>   
>   	/** This vma's place in the execbuf reservation list */
>   	struct drm_i915_gem_exec_object2 *exec;
> +	u32 bias;
>   
>   	struct list_head bind_link;
>   	struct list_head unbound_link;
> @@ -60,15 +61,12 @@ struct eb_vma_array {
>   #define __EXEC_OBJECT_HAS_PIN		BIT(31)
>   #define __EXEC_OBJECT_HAS_FENCE		BIT(30)
>   #define __EXEC_OBJECT_NEEDS_MAP		BIT(29)
> -#define __EXEC_OBJECT_NEEDS_BIAS	BIT(28)
> -#define __EXEC_OBJECT_INTERNAL_FLAGS	(~0u << 28) /* all of the above */
> +#define __EXEC_OBJECT_INTERNAL_FLAGS	(~0u << 29) /* all of the above */
>   
>   #define __EXEC_HAS_RELOC	BIT(31)
>   #define __EXEC_INTERNAL_FLAGS	(~0u << 31)
>   #define UPDATE			PIN_OFFSET_FIXED
>   
> -#define BATCH_OFFSET_BIAS (256*1024)
> -
>   #define __I915_EXEC_ILLEGAL_FLAGS \
>   	(__I915_EXEC_UNKNOWN_FLAGS | \
>   	 I915_EXEC_CONSTANTS_MASK  | \
> @@ -266,20 +264,18 @@ struct i915_execbuffer {
>   	 * obj/page
>   	 */
>   	struct reloc_cache {
> -		struct drm_mm_node node; /** temporary GTT binding */
>   		unsigned int gen; /** Cached value of INTEL_GEN */
>   		bool use_64bit_reloc : 1;
> -		bool has_llc : 1;
>   		bool has_fence : 1;
>   		bool needs_unfenced : 1;
>   
>   		struct intel_context *ce;
>   
> -		struct i915_vma *target;
> -		struct i915_request *rq;
> -		struct i915_vma *rq_vma;
> -		u32 *rq_cmd;
> -		unsigned int rq_size;
> +		struct eb_relocs_link {
> +			struct i915_vma *vma;
> +		} head;
> +		struct drm_i915_gem_relocation_entry *map;
> +		unsigned int pos;

It's not trivial so please add some commentary around the new struct 
members. List handling (is there a single linked list in kernel which 
could be used for clarity?). Or maybe it is not a list.. Why is a list 
of vma links inside a mapped GEM bo? What are pos, bufsz, later max, 
etc. So a bit of high level operation and a bit of per field. I think 
it's needed because it is not straightforward.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx