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

[Zeng, Oak]

Hi Thomas,

Thanks for take a look. I will drop this patch then. When I have time, I will try to document locking in each structure fields as you said. Probably a very low priority task.

Oak

> -----Original Message-----
> From: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Sent: Tuesday, May 28, 2024 3:22 AM
> To: Zeng, Oak <oak.zeng at intel.com>; intel-xe at lists.freedesktop.org
> Subject: Re: [PATCH] drm/xe: Add more document to xe_vm::lock
> 
> 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 at linux.intel.com>
> > Signed-off-by: Oak Zeng <oak.zeng at 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;
> >  	/**