Re: [PATCH] drm/xe: Add more document to xe_vm::lock

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi, Oak.

On Mon, 2024-05-27 at 16:07 -0700, Oak Zeng wrote:
> More document is added to xe_vm::lock to describe what is protected
> by this lock.
> 
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Oak Zeng <oak.zeng@intel.com>

I don't think we should be documenting the locks in this detailed
manner, but rather take care when we document the structures / fields
that are protected by a lock.

If we double-document then there's a big chance that someone forgets to
update one of the documentation sites.

Also some members below have more complicated protection than just a
single lock, for example vm::rebind list.

Also (even if it's a copy-paste) it's not possible to protect a single
bit with a lock (thinking of vm::flags field), unless there are other
conditions as well, since:

A:			B:
read_flags		vm_lock
update_bit_0		read_flags
			update_bit_1
			write_flags
			vm_unlock
write_flags

Here process a resets bit 1 to its original value.

Meaning all mutable bits must be protected by the same lock.

/Thomas


> ---
>  drivers/gpu/drm/xe/xe_vm_types.h | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h
> b/drivers/gpu/drm/xe/xe_vm_types.h
> index ce1a63a5e3e7..5597d8072046 100644
> --- a/drivers/gpu/drm/xe/xe_vm_types.h
> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> @@ -161,7 +161,18 @@ struct xe_vm {
>  
>  	/**
>  	 * @lock: outer most lock, protects objects of anything
> attached to this
> -	 * VM
> +	 * VM, more specifically:
> +	 * 1) vm::rebind_list
> +	 * 2) vm::flags, only XE_VM_FLA_BANNED bit
> +	 * 3) vma::tile_present
> +	 * 4) userptr::repin_list
> +	 * 5) userptr::invalidated list
> +	 * 6) vm::preempt::exec_queue
> +	 * 7) drm_gpuvm::rb list and tree
> +	 * 8) vm::size
> +	 * 9) vm::q[]->last_fence, only if q->flags'
> EXEC_QUEUE_FLAG_VM is set,
> +	 *    see xe_exec_queue_last_fence_lockdep_assert
> +	 * 10) a contested list during vm close. see
> xe_vm_close_and_put
>  	 */
>  	struct rw_semaphore lock;
>  	/**