Re: [PATCH v5 5/9] drm/ttm: TTM fault handler helpers

[Thomas Hellstrom]

Hi!

On Thu, 2019-06-13 at 12:25 +0800, Hillf Danton wrote:
> Hello Thomas
> 
> On Wed, 12 Jun 2019 08:42:39 +0200 Thomas Hellstrom wrote:
> > From: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
> > 
> > With the vmwgfx dirty tracking, the default TTM fault handler is
> > not
> > completely sufficient (vmwgfx need to modify the vma->vm_flags
> > member,
> > and also needs to restrict the number of prefaults).
> > 
> > We also want to replicate the new ttm_bo_vm_reserve() functionality
> > 
> > So start turning the TTM vm code into helpers:
> > ttm_bo_vm_fault_reserved()
> > and ttm_bo_vm_reserve(), and provide a default TTM fault handler
> > for other
> > drivers to use.
> > 
> > Cc: "Christian KÃnig" <christian.koenig@xxxxxxx>
> > 
> > Signed-off-by: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
> > Reviewed-by: "Christian KÃnig" <christian.koenig@xxxxxxx> #v1
> > ---
> >  drivers/gpu/drm/ttm/ttm_bo_vm.c | 175 +++++++++++++++++++---------
> > ----
> >  include/drm/ttm/ttm_bo_api.h    |  10 ++
> >  2 files changed, 113 insertions(+), 72 deletions(-)
> > 
> > 

...


> > -	/*
> > -	 * Work around locking order reversal in fault / nopfn
> > -	 * between mmap_sem and bo_reserve: Perform a trylock operation
> > -	 * for reserve, and if it fails, retry the fault after waiting
> > -	 * for the buffer to become unreserved.
> > -	 */
> Is it likely to not cut the comment as the trylock is still there?

Yes, I'll re-add that. It was removed in an early version of the patch
when I actually removed the trylock as well, but I changed my mind on
that.

> 
> >  	if (unlikely(!reservation_object_trylock(bo->resv))) {
> >  		if (vmf->flags & FAULT_FLAG_ALLOW_RETRY) {
> >  			if (!(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) {
> > @@ -151,14 +148,55 @@ static vm_fault_t ttm_bo_vm_fault(struct
> > vm_fault *vmf)
> >  		return VM_FAULT_NOPAGE;
> >  	}
> >  
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ttm_bo_vm_reserve);

...


> > 
> > -	if (unlikely(err != 0)) {
> > -		ret = VM_FAULT_SIGBUS;
> > -		goto out_io_unlock;
> > -	}
> > +	if (unlikely(err != 0))
> > +		return VM_FAULT_SIGBUS;
> >  
> Is it likely a typo to skip the io_unlock?
> 
> --
> Hillf


Yes. Good catch. That io_unlock should definitely remain.

I'll respin and resend to dri-devel and lkml only.

Thanks,
Thomas