Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Christian König]

Am 18.05.21 um 16:55 schrieb Thomas Hellström:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> This allows other drivers that may not setup the vma in the same way
> to use the ttm bo helpers.

Uff can you please explain why exactly you need that?

Providing the BO is not much of a problem, but having the BO at 
different VMA offsets is really a no-go with TTM.

Christian.

>
> Also clarify the documentation a bit, especially related to VM_FAULT_RETRY.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    |  4 +-
>   drivers/gpu/drm/nouveau/nouveau_ttm.c      |  4 +-
>   drivers/gpu/drm/radeon/radeon_ttm.c        |  4 +-
>   drivers/gpu/drm/ttm/ttm_bo_vm.c            | 84 +++++++++++++---------
>   drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c |  8 ++-
>   include/drm/ttm/ttm_bo_api.h               |  9 ++-
>   6 files changed, 75 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index d5a9d7a88315..89dafe14f828 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1919,7 +1919,9 @@ static vm_fault_t amdgpu_ttm_fault(struct vm_fault *vmf)
>   	if (ret)
>   		goto unlock;
>   
> -	ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,
> +	ret = ttm_bo_vm_fault_reserved(bo, vmf,
> +				       drm_vma_node_start(&bo->base.vma_node),
> +				       vmf->vma->vm_page_prot,
>   				       TTM_BO_VM_NUM_PREFAULT, 1);
>   	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
>   		return ret;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c
> index b81ae90b8449..555fb6d8be8b 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
> @@ -144,7 +144,9 @@ static vm_fault_t nouveau_ttm_fault(struct vm_fault *vmf)
>   
>   	nouveau_bo_del_io_reserve_lru(bo);
>   	prot = vm_get_page_prot(vma->vm_flags);
> -	ret = ttm_bo_vm_fault_reserved(vmf, prot, TTM_BO_VM_NUM_PREFAULT, 1);
> +	ret = ttm_bo_vm_fault_reserved(bo, vmf,
> +				       drm_vma_node_start(&bo->base.vma_node),
> +				       prot, TTM_BO_VM_NUM_PREFAULT, 1);
>   	nouveau_bo_add_io_reserve_lru(bo);
>   	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
>   		return ret;
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 3361d11769a2..ba48a2acdef0 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -816,7 +816,9 @@ static vm_fault_t radeon_ttm_fault(struct vm_fault *vmf)
>   	if (ret)
>   		goto unlock_resv;
>   
> -	ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,
> +	ret = ttm_bo_vm_fault_reserved(bo, vmf,
> +				       drm_vma_node_start(&bo->base.vma_node),
> +				       vmf->vma->vm_page_prot,
>   				       TTM_BO_VM_NUM_PREFAULT, 1);
>   	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
>   		goto unlock_mclk;
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index b31b18058965..ed00ccf1376e 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -42,7 +42,7 @@
>   #include <linux/mem_encrypt.h>
>   
>   static vm_fault_t ttm_bo_vm_fault_idle(struct ttm_buffer_object *bo,
> -				struct vm_fault *vmf)
> +				       struct vm_fault *vmf)
>   {
>   	vm_fault_t ret = 0;
>   	int err = 0;
> @@ -122,7 +122,8 @@ static unsigned long ttm_bo_io_mem_pfn(struct ttm_buffer_object *bo,
>    * Return:
>    *    0 on success and the bo was reserved.
>    *    VM_FAULT_RETRY if blocking wait.
> - *    VM_FAULT_NOPAGE if blocking wait and retrying was not allowed.
> + *    VM_FAULT_NOPAGE if blocking wait and retrying was not allowed, or wait interrupted.
> + *    VM_FAULT_SIGBUS if wait on bo->moving failed for reason other than a signal.
>    */
>   vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo,
>   			     struct vm_fault *vmf)
> @@ -254,7 +255,9 @@ static vm_fault_t ttm_bo_vm_insert_huge(struct vm_fault *vmf,
>   
>   /**
>    * ttm_bo_vm_fault_reserved - TTM fault helper
> + * @bo: The buffer object
>    * @vmf: The struct vm_fault given as argument to the fault callback
> + * @mmap_base: The base of the mmap, to which the @vmf fault is relative to.
>    * @prot: The page protection to be used for this memory area.
>    * @num_prefault: Maximum number of prefault pages. The caller may want to
>    * specify this based on madvice settings and the size of the GPU object
> @@ -265,19 +268,28 @@ static vm_fault_t ttm_bo_vm_insert_huge(struct vm_fault *vmf,
>    * memory backing the buffer object, and then returns a return code
>    * instructing the caller to retry the page access.
>    *
> + * This function ensures any pipelined wait is finished.
> + *
> + * WARNING:
> + * On VM_FAULT_RETRY, the bo will be unlocked by this function when
> + * #FAULT_FLAG_RETRY_NOWAIT is not set inside @vmf->flags. In this
> + * case, the caller should not unlock the @bo.
> + *
>    * Return:
> - *   VM_FAULT_NOPAGE on success or pending signal
> + *   0 on success.
> + *   VM_FAULT_NOPAGE on pending signal
>    *   VM_FAULT_SIGBUS on unspecified error
>    *   VM_FAULT_OOM on out-of-memory
> - *   VM_FAULT_RETRY if retryable wait
> + *   VM_FAULT_RETRY if retryable wait, see WARNING above.
>    */
> -vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
> +vm_fault_t ttm_bo_vm_fault_reserved(struct ttm_buffer_object *bo,
> +				    struct vm_fault *vmf,
> +				    unsigned long mmap_base,
>   				    pgprot_t prot,
>   				    pgoff_t num_prefault,
>   				    pgoff_t fault_page_size)
>   {
>   	struct vm_area_struct *vma = vmf->vma;
> -	struct ttm_buffer_object *bo = vma->vm_private_data;
>   	struct ttm_device *bdev = bo->bdev;
>   	unsigned long page_offset;
>   	unsigned long page_last;
> @@ -286,15 +298,11 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
>   	struct page *page;
>   	int err;
>   	pgoff_t i;
> -	vm_fault_t ret = VM_FAULT_NOPAGE;
> +	vm_fault_t ret;
>   	unsigned long address = vmf->address;
>   
> -	/*
> -	 * Wait for buffer data in transit, due to a pipelined
> -	 * move.
> -	 */
>   	ret = ttm_bo_vm_fault_idle(bo, vmf);
> -	if (unlikely(ret != 0))
> +	if (ret)
>   		return ret;
>   
>   	err = ttm_mem_io_reserve(bdev, &bo->mem);
> @@ -302,9 +310,8 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
>   		return VM_FAULT_SIGBUS;
>   
>   	page_offset = ((address - vma->vm_start) >> PAGE_SHIFT) +
> -		vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node);
> -	page_last = vma_pages(vma) + vma->vm_pgoff -
> -		drm_vma_node_start(&bo->base.vma_node);
> +		vma->vm_pgoff - mmap_base;
> +	page_last = vma_pages(vma) + vma->vm_pgoff - mmap_base;
>   
>   	if (unlikely(page_offset >= bo->mem.num_pages))
>   		return VM_FAULT_SIGBUS;
> @@ -344,8 +351,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
>   			} else if (unlikely(!page)) {
>   				break;
>   			}
> -			page->index = drm_vma_node_start(&bo->base.vma_node) +
> -				page_offset;
> +			page->index = mmap_base + page_offset;
>   			pfn = page_to_pfn(page);
>   		}
>   
> @@ -392,7 +398,10 @@ vm_fault_t ttm_bo_vm_fault(struct vm_fault *vmf)
>   		return ret;
>   
>   	prot = vma->vm_page_prot;
> -	ret = ttm_bo_vm_fault_reserved(vmf, prot, TTM_BO_VM_NUM_PREFAULT, 1);
> +	ret = ttm_bo_vm_fault_reserved(bo, vmf,
> +				       drm_vma_node_start(&bo->base.vma_node),
> +				       prot, TTM_BO_VM_NUM_PREFAULT, 1);
> +
>   	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
>   		return ret;
>   
> @@ -460,22 +469,16 @@ static int ttm_bo_vm_access_kmap(struct ttm_buffer_object *bo,
>   	return len;
>   }
>   
> -int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr,
> -		     void *buf, int len, int write)
> +int ttm_bo_vm_access_reserved(struct ttm_buffer_object *bo,
> +			      struct vm_area_struct *vma,
> +			      unsigned long offset,
> +			      void *buf, int len, int write)
>   {
> -	struct ttm_buffer_object *bo = vma->vm_private_data;
> -	unsigned long offset = (addr) - vma->vm_start +
> -		((vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node))
> -		 << PAGE_SHIFT);
>   	int ret;
>   
>   	if (len < 1 || (offset + len) >> PAGE_SHIFT > bo->mem.num_pages)
>   		return -EIO;
>   
> -	ret = ttm_bo_reserve(bo, true, false, NULL);
> -	if (ret)
> -		return ret;
> -
>   	switch (bo->mem.mem_type) {
>   	case TTM_PL_SYSTEM:
>   		if (unlikely(bo->ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) {
> @@ -485,16 +488,33 @@ int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr,
>   		}
>   		fallthrough;
>   	case TTM_PL_TT:
> -		ret = ttm_bo_vm_access_kmap(bo, offset, buf, len, write);
> -		break;
> +		return ttm_bo_vm_access_kmap(bo, offset, buf, len, write);
>   	default:
>   		if (bo->bdev->funcs->access_memory)
> -			ret = bo->bdev->funcs->access_memory(
> +			return bo->bdev->funcs->access_memory(
>   				bo, offset, buf, len, write);
>   		else
> -			ret = -EIO;
> +			return -EIO;
>   	}
>   
> +	return ret;
> +}
> +EXPORT_SYMBOL(ttm_bo_vm_access_reserved);
> +
> +int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr,
> +		     void *buf, int len, int write)
> +{
> +	struct ttm_buffer_object *bo = vma->vm_private_data;
> +	unsigned long offset = (addr) - vma->vm_start +
> +		((vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node))
> +		 << PAGE_SHIFT);
> +	int ret;
> +
> +	ret = ttm_bo_reserve(bo, true, false, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = ttm_bo_vm_access_reserved(bo, vma, offset, buf, len, write);
>   	ttm_bo_unreserve(bo);
>   
>   	return ret;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> index 45c9c6a7f1d6..56ecace0cf5c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> @@ -477,7 +477,9 @@ vm_fault_t vmw_bo_vm_fault(struct vm_fault *vmf)
>   	else
>   		prot = vm_get_page_prot(vma->vm_flags);
>   
> -	ret = ttm_bo_vm_fault_reserved(vmf, prot, num_prefault, 1);
> +	ret = ttm_bo_vm_fault_reserved(bo, vmf,
> +				       drm_vma_node_start(&bo->base.vma_node),
> +				       prot, num_prefault, 1);
>   	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
>   		return ret;
>   
> @@ -546,7 +548,9 @@ vm_fault_t vmw_bo_vm_huge_fault(struct vm_fault *vmf,
>   		prot = vm_get_page_prot(vma->vm_flags);
>   	}
>   
> -	ret = ttm_bo_vm_fault_reserved(vmf, prot, 1, fault_page_size);
> +	ret = ttm_bo_vm_fault_reserved(bo, vmf,
> +				       drm_vma_node_start(&bo->base.vma_node),
> +				       prot, 1, fault_page_size);
>   	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
>   		return ret;
>   
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 639521880c29..434f91f1fdbf 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -605,7 +605,9 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
>   vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo,
>   			     struct vm_fault *vmf);
>   
> -vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
> +vm_fault_t ttm_bo_vm_fault_reserved(struct ttm_buffer_object *bo,
> +				    struct vm_fault *vmf,
> +				    unsigned long mmap_base,
>   				    pgprot_t prot,
>   				    pgoff_t num_prefault,
>   				    pgoff_t fault_page_size);
> @@ -616,6 +618,11 @@ void ttm_bo_vm_open(struct vm_area_struct *vma);
>   
>   void ttm_bo_vm_close(struct vm_area_struct *vma);
>   
> +int ttm_bo_vm_access_reserved(struct ttm_buffer_object *bo,
> +			      struct vm_area_struct *vma,
> +			      unsigned long offset,
> +			      void *buf, int len, int write);
> +
>   int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr,
>   		     void *buf, int len, int write);
>   bool ttm_bo_delayed_delete(struct ttm_device *bdev, bool remove_all);

Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Thomas Hellström]

On 5/18/21 5:07 PM, Christian König wrote:
> Am 18.05.21 um 16:55 schrieb Thomas Hellström:
>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>
>> This allows other drivers that may not setup the vma in the same way
>> to use the ttm bo helpers.
>
> Uff can you please explain why exactly you need that?
>
> Providing the BO is not much of a problem, but having the BO at 
> different VMA offsets is really a no-go with TTM.
>
> Christian.

The current i915 uapi is using different offsets for different caching 
:/. We're currently working around that by using ttm_bo_type_kernel (no 
TTM vma offset at all) and i915's offset.

/Thomas

Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Christian König]

Am 18.05.21 um 17:11 schrieb Thomas Hellström:
>
> On 5/18/21 5:07 PM, Christian König wrote:
>> Am 18.05.21 um 16:55 schrieb Thomas Hellström:
>>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>
>>> This allows other drivers that may not setup the vma in the same way
>>> to use the ttm bo helpers.
>>
>> Uff can you please explain why exactly you need that?
>>
>> Providing the BO is not much of a problem, but having the BO at 
>> different VMA offsets is really a no-go with TTM.
>>
>> Christian.
>
> The current i915 uapi is using different offsets for different caching 
> :/. We're currently working around that by using ttm_bo_type_kernel 
> (no TTM vma offset at all) and i915's offset.

Can you instead adjust the offset in the mmap callback like we do for 
dma-buf?

That's really a no-go what you describe here because it will mess up 
reverse mapping lockup for buffer movement.

Christian.

>
> /Thomas
>

Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Thomas Hellström]

On 5/18/21 5:17 PM, Christian König wrote:
>
>
> Am 18.05.21 um 17:11 schrieb Thomas Hellström:
>>
>> On 5/18/21 5:07 PM, Christian König wrote:
>>> Am 18.05.21 um 16:55 schrieb Thomas Hellström:
>>>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>
>>>> This allows other drivers that may not setup the vma in the same way
>>>> to use the ttm bo helpers.
>>>
>>> Uff can you please explain why exactly you need that?
>>>
>>> Providing the BO is not much of a problem, but having the BO at 
>>> different VMA offsets is really a no-go with TTM.
>>>
>>> Christian.
>>
>> The current i915 uapi is using different offsets for different 
>> caching :/. We're currently working around that by using 
>> ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset.
>
> Can you instead adjust the offset in the mmap callback like we do for 
> dma-buf?
Will have to take a look.
>
> That's really a no-go what you describe here because it will mess up 
> reverse mapping lockup for buffer movement.

You mean the unmap_mapping_range() stuff? That's not a problem since 
it's a NOP for kernel ttm buffers, and the i915 move() / swap_notify() 
takes care of killing the ptes.

While we're in the process of killing that offset flexibility for 
discrete, we can't do so for older hardware unfortunately.

/Thomas


>
> Christian.



>
>>
>> /Thomas
>>
>

Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Christian König]

Am 18.05.21 um 17:25 schrieb Thomas Hellström:
>
> On 5/18/21 5:17 PM, Christian König wrote:
>>
>>
>> Am 18.05.21 um 17:11 schrieb Thomas Hellström:
>>>
>>> On 5/18/21 5:07 PM, Christian König wrote:
>>>> Am 18.05.21 um 16:55 schrieb Thomas Hellström:
>>>>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>
>>>>> This allows other drivers that may not setup the vma in the same way
>>>>> to use the ttm bo helpers.
>>>>
>>>> Uff can you please explain why exactly you need that?
>>>>
>>>> Providing the BO is not much of a problem, but having the BO at 
>>>> different VMA offsets is really a no-go with TTM.
>>>>
>>>> Christian.
>>>
>>> The current i915 uapi is using different offsets for different 
>>> caching :/. We're currently working around that by using 
>>> ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset.
>>
>> Can you instead adjust the offset in the mmap callback like we do for 
>> dma-buf?
> Will have to take a look.
>>
>> That's really a no-go what you describe here because it will mess up 
>> reverse mapping lockup for buffer movement.
>
> You mean the unmap_mapping_range() stuff? That's not a problem since 
> it's a NOP for kernel ttm buffers, and the i915 move() / swap_notify() 
> takes care of killing the ptes.

That design is a certain NAK from my side for upstreaming this.

PTE handling is the domain of TTM, drivers should never mess with that 
directly.

Christian.

>
> While we're in the process of killing that offset flexibility for 
> discrete, we can't do so for older hardware unfortunately.
>
> /Thomas
>
>
>>
>> Christian.
>
>
>
>>
>>>
>>> /Thomas
>>>
>>

Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Thomas Hellström]

On 5/18/21 5:29 PM, Christian König wrote:
>
>
> Am 18.05.21 um 17:25 schrieb Thomas Hellström:
>>
>> On 5/18/21 5:17 PM, Christian König wrote:
>>>
>>>
>>> Am 18.05.21 um 17:11 schrieb Thomas Hellström:
>>>>
>>>> On 5/18/21 5:07 PM, Christian König wrote:
>>>>> Am 18.05.21 um 16:55 schrieb Thomas Hellström:
>>>>>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>
>>>>>> This allows other drivers that may not setup the vma in the same way
>>>>>> to use the ttm bo helpers.
>>>>>
>>>>> Uff can you please explain why exactly you need that?
>>>>>
>>>>> Providing the BO is not much of a problem, but having the BO at 
>>>>> different VMA offsets is really a no-go with TTM.
>>>>>
>>>>> Christian.
>>>>
>>>> The current i915 uapi is using different offsets for different 
>>>> caching :/. We're currently working around that by using 
>>>> ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset.
>>>
>>> Can you instead adjust the offset in the mmap callback like we do 
>>> for dma-buf?
>> Will have to take a look.
>>>
>>> That's really a no-go what you describe here because it will mess up 
>>> reverse mapping lockup for buffer movement.
>>
>> You mean the unmap_mapping_range() stuff? That's not a problem since 
>> it's a NOP for kernel ttm buffers, and the i915 move() / 
>> swap_notify() takes care of killing the ptes.
>
> That design is a certain NAK from my side for upstreaming this.
>
> PTE handling is the domain of TTM, drivers should never mess with that 
> directly.

Hmm. May I humbly suggest a different view on this:

I agree fully for ttm_bo_type_device bos but for ttm_bo_type_kernel, TTM 
has no business whatsoever with user-space PTEs. That's really why that 
bo type exists in the first place. But otoh one can of course argue that 
then i915 has no business calling the TTM fault helper for these bos.

So for discrete we can probably do the right thing with 
ttm_bo_type_device. What worries me a bit is when we get to older 
hardware support because whatever we do is by definition going to be 
ugly. At best we might be able to split the address space between i915's 
mmos, and hand the rest to TTM, modifying offsets as you suggest. That 
way a TTM call to unmap_mapping_range() would do the right thing, I think.

/Thomas

>
> Christian.
>
>>
>> While we're in the process of killing that offset flexibility for 
>> discrete, we can't do so for older hardware unfortunately.
>>
>> /Thomas
>>
>>
>>>
>>> Christian.
>>
>>
>>
>>>
>>>>
>>>> /Thomas
>>>>
>>>
>

Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Christian König]

Am 18.05.21 um 19:10 schrieb Thomas Hellström:
>
> On 5/18/21 5:29 PM, Christian König wrote:
>>
>>
>> Am 18.05.21 um 17:25 schrieb Thomas Hellström:
>>>
>>> On 5/18/21 5:17 PM, Christian König wrote:
>>>>
>>>>
>>>> Am 18.05.21 um 17:11 schrieb Thomas Hellström:
>>>>>
>>>>> On 5/18/21 5:07 PM, Christian König wrote:
>>>>>> Am 18.05.21 um 16:55 schrieb Thomas Hellström:
>>>>>>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>
>>>>>>> This allows other drivers that may not setup the vma in the same 
>>>>>>> way
>>>>>>> to use the ttm bo helpers.
>>>>>>
>>>>>> Uff can you please explain why exactly you need that?
>>>>>>
>>>>>> Providing the BO is not much of a problem, but having the BO at 
>>>>>> different VMA offsets is really a no-go with TTM.
>>>>>>
>>>>>> Christian.
>>>>>
>>>>> The current i915 uapi is using different offsets for different 
>>>>> caching :/. We're currently working around that by using 
>>>>> ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset.
>>>>
>>>> Can you instead adjust the offset in the mmap callback like we do 
>>>> for dma-buf?
>>> Will have to take a look.
>>>>
>>>> That's really a no-go what you describe here because it will mess 
>>>> up reverse mapping lockup for buffer movement.
>>>
>>> You mean the unmap_mapping_range() stuff? That's not a problem since 
>>> it's a NOP for kernel ttm buffers, and the i915 move() / 
>>> swap_notify() takes care of killing the ptes.
>>
>> That design is a certain NAK from my side for upstreaming this.
>>
>> PTE handling is the domain of TTM, drivers should never mess with 
>> that directly.
>
> Hmm. May I humbly suggest a different view on this:
>
> I agree fully for ttm_bo_type_device bos but for ttm_bo_type_kernel, 
> TTM has no business whatsoever with user-space PTEs. That's really why 
> that bo type exists in the first place. But otoh one can of course 
> argue that then i915 has no business calling the TTM fault helper for 
> these bos.

Well the real question is why does i915 wants to expose kernel BOs to 
userspace? As the name says only the kernel should be able to access them.

>
> So for discrete we can probably do the right thing with 
> ttm_bo_type_device. What worries me a bit is when we get to older 
> hardware support because whatever we do is by definition going to be 
> ugly. At best we might be able to split the address space between 
> i915's mmos, and hand the rest to TTM, modifying offsets as you 
> suggest. That way a TTM call to unmap_mapping_range() would do the 
> right thing, I think.

Well we do all kind of nasty stuff with the offset in DMA-buf, KFD, 
overlayfs etc. So adjusting the offset inside the kernel is already well 
supported and used.

What I don't fully understand is your use case here? Can you briefly 
describe that?

Do you use different bits of the offset to signal what caching behavior 
you have? And then based on this adjust the pgprot_t in the vma?

Thanks,
Christian.

>
> /Thomas
>
>>
>> Christian.
>>
>>>
>>> While we're in the process of killing that offset flexibility for 
>>> discrete, we can't do so for older hardware unfortunately.
>>>
>>> /Thomas
>>>
>>>
>>>>
>>>> Christian.
>>>
>>>
>>>
>>>>
>>>>>
>>>>> /Thomas
>>>>>
>>>>
>>

Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Thomas Hellström]

On 5/18/21 7:17 PM, Christian König wrote:
> Am 18.05.21 um 19:10 schrieb Thomas Hellström:
>>
>> On 5/18/21 5:29 PM, Christian König wrote:
>>>
>>>
>>> Am 18.05.21 um 17:25 schrieb Thomas Hellström:
>>>>
>>>> On 5/18/21 5:17 PM, Christian König wrote:
>>>>>
>>>>>
>>>>> Am 18.05.21 um 17:11 schrieb Thomas Hellström:
>>>>>>
>>>>>> On 5/18/21 5:07 PM, Christian König wrote:
>>>>>>> Am 18.05.21 um 16:55 schrieb Thomas Hellström:
>>>>>>>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>>
>>>>>>>> This allows other drivers that may not setup the vma in the 
>>>>>>>> same way
>>>>>>>> to use the ttm bo helpers.
>>>>>>>
>>>>>>> Uff can you please explain why exactly you need that?
>>>>>>>
>>>>>>> Providing the BO is not much of a problem, but having the BO at 
>>>>>>> different VMA offsets is really a no-go with TTM.
>>>>>>>
>>>>>>> Christian.
>>>>>>
>>>>>> The current i915 uapi is using different offsets for different 
>>>>>> caching :/. We're currently working around that by using 
>>>>>> ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset.
>>>>>
>>>>> Can you instead adjust the offset in the mmap callback like we do 
>>>>> for dma-buf?
>>>> Will have to take a look.
>>>>>
>>>>> That's really a no-go what you describe here because it will mess 
>>>>> up reverse mapping lockup for buffer movement.
>>>>
>>>> You mean the unmap_mapping_range() stuff? That's not a problem 
>>>> since it's a NOP for kernel ttm buffers, and the i915 move() / 
>>>> swap_notify() takes care of killing the ptes.
>>>
>>> That design is a certain NAK from my side for upstreaming this.
>>>
>>> PTE handling is the domain of TTM, drivers should never mess with 
>>> that directly.
>>
>> Hmm. May I humbly suggest a different view on this:
>>
>> I agree fully for ttm_bo_type_device bos but for ttm_bo_type_kernel, 
>> TTM has no business whatsoever with user-space PTEs. That's really 
>> why that bo type exists in the first place. But otoh one can of 
>> course argue that then i915 has no business calling the TTM fault 
>> helper for these bos.
>
> Well the real question is why does i915 wants to expose kernel BOs to 
> userspace? As the name says only the kernel should be able to access 
> them.

I'd say "kernel" of course refers to how TTM handles them.

>
>>
>> So for discrete we can probably do the right thing with 
>> ttm_bo_type_device. What worries me a bit is when we get to older 
>> hardware support because whatever we do is by definition going to be 
>> ugly. At best we might be able to split the address space between 
>> i915's mmos, and hand the rest to TTM, modifying offsets as you 
>> suggest. That way a TTM call to unmap_mapping_range() would do the 
>> right thing, I think.
>
> Well we do all kind of nasty stuff with the offset in DMA-buf, KFD, 
> overlayfs etc. So adjusting the offset inside the kernel is already 
> well supported and used.
>
> What I don't fully understand is your use case here? Can you briefly 
> describe that?
>
> Do you use different bits of the offset to signal what caching 
> behavior you have? And then based on this adjust the pgprot_t in the vma?
>
> Thanks,
> Christian.

TBH I'm not completely sure about the history of this. I think the basic 
idea is that you want to support different vmas with different caching 
modes, so you never change the vma pgprot after mmap time, like TTM 
does. Needless to say this only works with Intel processors on 
integrated and the more I write about this the more I'm convinced that 
we should perhaps not concern TTM with that at all.

/Thomas


>
>>
>> /Thomas
>>
>>>
>>> Christian.
>>>
>>>>
>>>> While we're in the process of killing that offset flexibility for 
>>>> discrete, we can't do so for older hardware unfortunately.
>>>>
>>>> /Thomas
>>>>
>>>>
>>>>>
>>>>> Christian.
>>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> /Thomas
>>>>>>
>>>>>
>>>
>

Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Christian König]

Am 18.05.21 um 19:32 schrieb Thomas Hellström:
> [SNIP]
>>>> PTE handling is the domain of TTM, drivers should never mess with 
>>>> that directly.
>>>
>>> Hmm. May I humbly suggest a different view on this:
>>>
>>> I agree fully for ttm_bo_type_device bos but for ttm_bo_type_kernel, 
>>> TTM has no business whatsoever with user-space PTEs. That's really 
>>> why that bo type exists in the first place. But otoh one can of 
>>> course argue that then i915 has no business calling the TTM fault 
>>> helper for these bos.
>>
>> Well the real question is why does i915 wants to expose kernel BOs to 
>> userspace? As the name says only the kernel should be able to access 
>> them.
>
> I'd say "kernel" of course refers to how TTM handles them.

Fair enough.

>>
>>>
>>> So for discrete we can probably do the right thing with 
>>> ttm_bo_type_device. What worries me a bit is when we get to older 
>>> hardware support because whatever we do is by definition going to be 
>>> ugly. At best we might be able to split the address space between 
>>> i915's mmos, and hand the rest to TTM, modifying offsets as you 
>>> suggest. That way a TTM call to unmap_mapping_range() would do the 
>>> right thing, I think.
>>
>> Well we do all kind of nasty stuff with the offset in DMA-buf, KFD, 
>> overlayfs etc. So adjusting the offset inside the kernel is already 
>> well supported and used.
>>
>> What I don't fully understand is your use case here? Can you briefly 
>> describe that?
>>
>> Do you use different bits of the offset to signal what caching 
>> behavior you have? And then based on this adjust the pgprot_t in the 
>> vma?
>>
>> Thanks,
>> Christian.
>
> TBH I'm not completely sure about the history of this. I think the 
> basic idea is that you want to support different vmas with different 
> caching modes, so you never change the vma pgprot after mmap time, 
> like TTM does. Needless to say this only works with Intel processors 
> on integrated and the more I write about this the more I'm convinced 
> that we should perhaps not concern TTM with that at all.

Yeah, that might be a possibility.

But KFD does something very similar IIRC. They stuff the routing 
information into the upper bits of the offset and adjust it then to 
match what TTM wants in the mmap() callback.

Adjusting the caching behavior on the fly is really tricky and I'm 
pretty sure you should not do that outside of the integrated Intel 
processor anyway :)

Cheers,
Christian.

>
> /Thomas
>
>
>>
>>>
>>> /Thomas
>>>
>>>>
>>>> Christian.
>>>>
>>>>>
>>>>> While we're in the process of killing that offset flexibility for 
>>>>> discrete, we can't do so for older hardware unfortunately.
>>>>>
>>>>> /Thomas
>>>>>
>>>>>
>>>>>>
>>>>>> Christian.
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> /Thomas
>>>>>>>
>>>>>>
>>>>
>>

Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Maarten Lankhorst]

Op 18-05-2021 om 17:07 schreef Christian König:
> Am 18.05.21 um 16:55 schrieb Thomas Hellström:
>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>
>> This allows other drivers that may not setup the vma in the same way
>> to use the ttm bo helpers.
>
> Uff can you please explain why exactly you need that?
>
> Providing the BO is not much of a problem, but having the BO at different VMA offsets is really a no-go with TTM. 

i915 has its own mmap allocation handling to handle caching modes, and wouldn't use TTM's vma handling. But we do want to use the ttm fault handlers, which only need mmap_base + bo to work correctly.

~Maarten

Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Christian König]

Am 18.05.21 um 18:49 schrieb Maarten Lankhorst:
> Op 18-05-2021 om 17:07 schreef Christian König:
>> Am 18.05.21 um 16:55 schrieb Thomas Hellström:
>>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>
>>> This allows other drivers that may not setup the vma in the same way
>>> to use the ttm bo helpers.
>> Uff can you please explain why exactly you need that?
>>
>> Providing the BO is not much of a problem, but having the BO at different VMA offsets is really a no-go with TTM.
> i915 has its own mmap allocation handling to handle caching modes, and wouldn't use TTM's vma handling. But we do want to use the ttm fault handlers, which only need mmap_base + bo to work correctly.

Yeah, but that is really a no-go. The VMA handling is a very essential 
part of TTM.

If you want to use TTM we have to find a cleaner way to implement this.

Christian.

>
> ~Maarten
>

[PATCH v2 00/15] drm/i915: Move LMEM (VRAM) management over to TTM

[Thomas Hellström]

This is an initial patch series to move discrete memory management over to
TTM. It will be followed up shortly with adding more functionality.

The buddy allocator is temporarily removed along with its selftests and
It is replaced with the TTM range manager and some selftests are adjusted
to account for introduced fragmentation. Work is ongoing to reintroduce the
buddy allocator as a TTM resource manager.

A new memcpy ttm move is introduced that uses kmap_local() functionality
rather than vmap(). Among other things stated in the patch commit message
it helps us deal with page-pased LMEM memory. It is generic enough to replace
the ttm memcpy move with some additional work if so desired. On x86 it also
enables prefetching reads from write-combined memory.

Finally the old i915 gem object LMEM backend is replaced with a
i915 gem object TTM backend and some additional i915 gem object ops are
introduced to support the added functionality.
Currently it is used only to support management and eviction of the LMEM
region, but work is underway to extend the support to system memory. In this
way we use TTM the way it was originally intended, having the GPU binding
taken care of by driver code.

Intention is to follow up with
- System memory support
- Pipelined accelerated moves / migration
- Re-added buddy allocator in the TTM framework

v2:
- Add patches to move pagefaulting over to TTM
- Break out TTM changes to separate patches
- Address various review comments as detailed in the affected patches

Cc: Christian König <christian.koenig@amd.com>

Maarten Lankhorst (4):
  drm/i915: Disable mmap ioctl for gen12+
  drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm
    handlers.
  drm/i915: Use ttm mmap handling for ttm bo's.
  drm/i915/ttm: Add io sgt caching to i915_ttm_io_mem_pfn

Thomas Hellström (11):
  drm/i915: Untangle the vma pages_mutex
  drm/i915: Don't free shared locks while shared
  drm/i915: Fix i915_sg_page_sizes to record dma segments rather than
    physical pages
  drm/ttm: Export functions to initialize and finalize the ttm range
    manager standalone
  drm/i915/ttm Initialize the ttm device and memory managers
  drm/i915/ttm: Embed a ttm buffer object in the i915 gem object
  drm/ttm: Export ttm_bo_tt_destroy()
  drm/i915/ttm Add a generic TTM memcpy move for page-based iomem
  drm/ttm, drm/amdgpu: Allow the driver some control over swapping
  drm/i915/ttm: Introduce a TTM i915 gem object backend
  drm/i915/lmem: Verify checks for lmem residency

 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |   8 +-
 drivers/gpu/drm/i915/Kconfig                  |   1 +
 drivers/gpu/drm/i915/Makefile                 |   4 +-
 drivers/gpu/drm/i915/display/intel_display.c  |   2 +-
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |   2 +-
 drivers/gpu/drm/i915/gem/i915_gem_lmem.c      |  71 +-
 drivers/gpu/drm/i915/gem/i915_gem_lmem.h      |   5 -
 drivers/gpu/drm/i915/gem/i915_gem_mman.c      |  24 +-
 drivers/gpu/drm/i915/gem/i915_gem_mman.h      |   2 +
 drivers/gpu/drm/i915/gem/i915_gem_object.c    | 149 +++-
 drivers/gpu/drm/i915/gem/i915_gem_object.h    |  19 +-
 .../gpu/drm/i915/gem/i915_gem_object_types.h  |  39 +-
 drivers/gpu/drm/i915/gem/i915_gem_pages.c     |   6 +-
 drivers/gpu/drm/i915/gem/i915_gem_phys.c      |   2 +-
 drivers/gpu/drm/i915/gem/i915_gem_region.c    | 126 +--
 drivers/gpu/drm/i915/gem/i915_gem_region.h    |   4 -
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c     |   4 +-
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c    |  10 +-
 drivers/gpu/drm/i915/gem/i915_gem_stolen.h    |   9 +-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c       | 626 ++++++++++++++
 drivers/gpu/drm/i915/gem/i915_gem_ttm.h       |  48 ++
 .../gpu/drm/i915/gem/i915_gem_ttm_bo_util.c   | 194 +++++
 .../gpu/drm/i915/gem/i915_gem_ttm_bo_util.h   | 107 +++
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c   |   2 +-
 drivers/gpu/drm/i915/gt/intel_ggtt.c          |  19 +-
 drivers/gpu/drm/i915/gt/intel_gt.c            |   2 -
 drivers/gpu/drm/i915/gt/intel_gtt.c           |  45 +-
 drivers/gpu/drm/i915/gt/intel_gtt.h           |  30 +-
 drivers/gpu/drm/i915/gt/intel_ppgtt.c         |   2 +-
 drivers/gpu/drm/i915/gt/intel_region_lmem.c   |  30 +-
 drivers/gpu/drm/i915/i915_buddy.c             | 435 ----------
 drivers/gpu/drm/i915/i915_buddy.h             | 131 ---
 drivers/gpu/drm/i915/i915_drv.c               |  13 +
 drivers/gpu/drm/i915/i915_drv.h               |   7 +-
 drivers/gpu/drm/i915/i915_gem.c               |   6 +-
 drivers/gpu/drm/i915/i915_globals.c           |   1 -
 drivers/gpu/drm/i915/i915_globals.h           |   1 -
 drivers/gpu/drm/i915/i915_scatterlist.c       |  70 ++
 drivers/gpu/drm/i915/i915_scatterlist.h       |  20 +-
 drivers/gpu/drm/i915/i915_vma.c               |  33 +-
 drivers/gpu/drm/i915/intel_memory_region.c    | 181 ++--
 drivers/gpu/drm/i915/intel_memory_region.h    |  45 +-
 drivers/gpu/drm/i915/intel_region_ttm.c       | 246 ++++++
 drivers/gpu/drm/i915/intel_region_ttm.h       |  32 +
 drivers/gpu/drm/i915/selftests/i915_buddy.c   | 789 ------------------
 .../drm/i915/selftests/i915_mock_selftests.h  |   1 -
 .../drm/i915/selftests/intel_memory_region.c  | 133 +--
 drivers/gpu/drm/i915/selftests/mock_region.c  |  50 +-
 drivers/gpu/drm/nouveau/nouveau_ttm.c         |   4 +-
 drivers/gpu/drm/radeon/radeon_ttm.c           |   4 +-
 drivers/gpu/drm/ttm/ttm_bo.c                  |  42 +-
 drivers/gpu/drm/ttm/ttm_bo_vm.c               |  84 +-
 drivers/gpu/drm/ttm/ttm_range_manager.c       |  55 +-
 drivers/gpu/drm/ttm/ttm_tt.c                  |   4 +
 drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c    |   8 +-
 include/drm/ttm/ttm_bo_api.h                  |   9 +-
 include/drm/ttm/ttm_bo_driver.h               |  23 +
 57 files changed, 2068 insertions(+), 1951 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm.c
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm.h
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_bo_util.c
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_bo_util.h
 delete mode 100644 drivers/gpu/drm/i915/i915_buddy.c
 delete mode 100644 drivers/gpu/drm/i915/i915_buddy.h
 create mode 100644 drivers/gpu/drm/i915/intel_region_ttm.c
 create mode 100644 drivers/gpu/drm/i915/intel_region_ttm.h
 delete mode 100644 drivers/gpu/drm/i915/selftests/i915_buddy.c

-- 
2.31.1

[PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.

[Thomas Hellström]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This allows other drivers that may not setup the vma in the same way
to use the ttm bo helpers.

Also clarify the documentation a bit, especially related to VM_FAULT_RETRY.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    |  4 +-
 drivers/gpu/drm/nouveau/nouveau_ttm.c      |  4 +-
 drivers/gpu/drm/radeon/radeon_ttm.c        |  4 +-
 drivers/gpu/drm/ttm/ttm_bo_vm.c            | 84 +++++++++++++---------
 drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c |  8 ++-
 include/drm/ttm/ttm_bo_api.h               |  9 ++-
 6 files changed, 75 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index d5a9d7a88315..89dafe14f828 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1919,7 +1919,9 @@ static vm_fault_t amdgpu_ttm_fault(struct vm_fault *vmf)
 	if (ret)
 		goto unlock;
 
-	ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,
+	ret = ttm_bo_vm_fault_reserved(bo, vmf,
+				       drm_vma_node_start(&bo->base.vma_node),
+				       vmf->vma->vm_page_prot,
 				       TTM_BO_VM_NUM_PREFAULT, 1);
 	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
 		return ret;
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index b81ae90b8449..555fb6d8be8b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -144,7 +144,9 @@ static vm_fault_t nouveau_ttm_fault(struct vm_fault *vmf)
 
 	nouveau_bo_del_io_reserve_lru(bo);
 	prot = vm_get_page_prot(vma->vm_flags);
-	ret = ttm_bo_vm_fault_reserved(vmf, prot, TTM_BO_VM_NUM_PREFAULT, 1);
+	ret = ttm_bo_vm_fault_reserved(bo, vmf,
+				       drm_vma_node_start(&bo->base.vma_node),
+				       prot, TTM_BO_VM_NUM_PREFAULT, 1);
 	nouveau_bo_add_io_reserve_lru(bo);
 	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
 		return ret;
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 3361d11769a2..ba48a2acdef0 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -816,7 +816,9 @@ static vm_fault_t radeon_ttm_fault(struct vm_fault *vmf)
 	if (ret)
 		goto unlock_resv;
 
-	ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,
+	ret = ttm_bo_vm_fault_reserved(bo, vmf,
+				       drm_vma_node_start(&bo->base.vma_node),
+				       vmf->vma->vm_page_prot,
 				       TTM_BO_VM_NUM_PREFAULT, 1);
 	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
 		goto unlock_mclk;
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index b31b18058965..ed00ccf1376e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -42,7 +42,7 @@
 #include <linux/mem_encrypt.h>
 
 static vm_fault_t ttm_bo_vm_fault_idle(struct ttm_buffer_object *bo,
-				struct vm_fault *vmf)
+				       struct vm_fault *vmf)
 {
 	vm_fault_t ret = 0;
 	int err = 0;
@@ -122,7 +122,8 @@ static unsigned long ttm_bo_io_mem_pfn(struct ttm_buffer_object *bo,
  * Return:
  *    0 on success and the bo was reserved.
  *    VM_FAULT_RETRY if blocking wait.
- *    VM_FAULT_NOPAGE if blocking wait and retrying was not allowed.
+ *    VM_FAULT_NOPAGE if blocking wait and retrying was not allowed, or wait interrupted.
+ *    VM_FAULT_SIGBUS if wait on bo->moving failed for reason other than a signal.
  */
 vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo,
 			     struct vm_fault *vmf)
@@ -254,7 +255,9 @@ static vm_fault_t ttm_bo_vm_insert_huge(struct vm_fault *vmf,
 
 /**
  * ttm_bo_vm_fault_reserved - TTM fault helper
+ * @bo: The buffer object
  * @vmf: The struct vm_fault given as argument to the fault callback
+ * @mmap_base: The base of the mmap, to which the @vmf fault is relative to.
  * @prot: The page protection to be used for this memory area.
  * @num_prefault: Maximum number of prefault pages. The caller may want to
  * specify this based on madvice settings and the size of the GPU object
@@ -265,19 +268,28 @@ static vm_fault_t ttm_bo_vm_insert_huge(struct vm_fault *vmf,
  * memory backing the buffer object, and then returns a return code
  * instructing the caller to retry the page access.
  *
+ * This function ensures any pipelined wait is finished.
+ *
+ * WARNING:
+ * On VM_FAULT_RETRY, the bo will be unlocked by this function when
+ * #FAULT_FLAG_RETRY_NOWAIT is not set inside @vmf->flags. In this
+ * case, the caller should not unlock the @bo.
+ *
  * Return:
- *   VM_FAULT_NOPAGE on success or pending signal
+ *   0 on success.
+ *   VM_FAULT_NOPAGE on pending signal
  *   VM_FAULT_SIGBUS on unspecified error
  *   VM_FAULT_OOM on out-of-memory
- *   VM_FAULT_RETRY if retryable wait
+ *   VM_FAULT_RETRY if retryable wait, see WARNING above.
  */
-vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
+vm_fault_t ttm_bo_vm_fault_reserved(struct ttm_buffer_object *bo,
+				    struct vm_fault *vmf,
+				    unsigned long mmap_base,
 				    pgprot_t prot,
 				    pgoff_t num_prefault,
 				    pgoff_t fault_page_size)
 {
 	struct vm_area_struct *vma = vmf->vma;
-	struct ttm_buffer_object *bo = vma->vm_private_data;
 	struct ttm_device *bdev = bo->bdev;
 	unsigned long page_offset;
 	unsigned long page_last;
@@ -286,15 +298,11 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
 	struct page *page;
 	int err;
 	pgoff_t i;
-	vm_fault_t ret = VM_FAULT_NOPAGE;
+	vm_fault_t ret;
 	unsigned long address = vmf->address;
 
-	/*
-	 * Wait for buffer data in transit, due to a pipelined
-	 * move.
-	 */
 	ret = ttm_bo_vm_fault_idle(bo, vmf);
-	if (unlikely(ret != 0))
+	if (ret)
 		return ret;
 
 	err = ttm_mem_io_reserve(bdev, &bo->mem);
@@ -302,9 +310,8 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
 		return VM_FAULT_SIGBUS;
 
 	page_offset = ((address - vma->vm_start) >> PAGE_SHIFT) +
-		vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node);
-	page_last = vma_pages(vma) + vma->vm_pgoff -
-		drm_vma_node_start(&bo->base.vma_node);
+		vma->vm_pgoff - mmap_base;
+	page_last = vma_pages(vma) + vma->vm_pgoff - mmap_base;
 
 	if (unlikely(page_offset >= bo->mem.num_pages))
 		return VM_FAULT_SIGBUS;
@@ -344,8 +351,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
 			} else if (unlikely(!page)) {
 				break;
 			}
-			page->index = drm_vma_node_start(&bo->base.vma_node) +
-				page_offset;
+			page->index = mmap_base + page_offset;
 			pfn = page_to_pfn(page);
 		}
 
@@ -392,7 +398,10 @@ vm_fault_t ttm_bo_vm_fault(struct vm_fault *vmf)
 		return ret;
 
 	prot = vma->vm_page_prot;
-	ret = ttm_bo_vm_fault_reserved(vmf, prot, TTM_BO_VM_NUM_PREFAULT, 1);
+	ret = ttm_bo_vm_fault_reserved(bo, vmf,
+				       drm_vma_node_start(&bo->base.vma_node),
+				       prot, TTM_BO_VM_NUM_PREFAULT, 1);
+
 	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
 		return ret;
 
@@ -460,22 +469,16 @@ static int ttm_bo_vm_access_kmap(struct ttm_buffer_object *bo,
 	return len;
 }
 
-int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr,
-		     void *buf, int len, int write)
+int ttm_bo_vm_access_reserved(struct ttm_buffer_object *bo,
+			      struct vm_area_struct *vma,
+			      unsigned long offset,
+			      void *buf, int len, int write)
 {
-	struct ttm_buffer_object *bo = vma->vm_private_data;
-	unsigned long offset = (addr) - vma->vm_start +
-		((vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node))
-		 << PAGE_SHIFT);
 	int ret;
 
 	if (len < 1 || (offset + len) >> PAGE_SHIFT > bo->mem.num_pages)
 		return -EIO;
 
-	ret = ttm_bo_reserve(bo, true, false, NULL);
-	if (ret)
-		return ret;
-
 	switch (bo->mem.mem_type) {
 	case TTM_PL_SYSTEM:
 		if (unlikely(bo->ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) {
@@ -485,16 +488,33 @@ int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr,
 		}
 		fallthrough;
 	case TTM_PL_TT:
-		ret = ttm_bo_vm_access_kmap(bo, offset, buf, len, write);
-		break;
+		return ttm_bo_vm_access_kmap(bo, offset, buf, len, write);
 	default:
 		if (bo->bdev->funcs->access_memory)
-			ret = bo->bdev->funcs->access_memory(
+			return bo->bdev->funcs->access_memory(
 				bo, offset, buf, len, write);
 		else
-			ret = -EIO;
+			return -EIO;
 	}
 
+	return ret;
+}
+EXPORT_SYMBOL(ttm_bo_vm_access_reserved);
+
+int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr,
+		     void *buf, int len, int write)
+{
+	struct ttm_buffer_object *bo = vma->vm_private_data;
+	unsigned long offset = (addr) - vma->vm_start +
+		((vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node))
+		 << PAGE_SHIFT);
+	int ret;
+
+	ret = ttm_bo_reserve(bo, true, false, NULL);
+	if (ret)
+		return ret;
+
+	ret = ttm_bo_vm_access_reserved(bo, vma, offset, buf, len, write);
 	ttm_bo_unreserve(bo);
 
 	return ret;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
index 45c9c6a7f1d6..56ecace0cf5c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
@@ -477,7 +477,9 @@ vm_fault_t vmw_bo_vm_fault(struct vm_fault *vmf)
 	else
 		prot = vm_get_page_prot(vma->vm_flags);
 
-	ret = ttm_bo_vm_fault_reserved(vmf, prot, num_prefault, 1);
+	ret = ttm_bo_vm_fault_reserved(bo, vmf,
+				       drm_vma_node_start(&bo->base.vma_node),
+				       prot, num_prefault, 1);
 	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
 		return ret;
 
@@ -546,7 +548,9 @@ vm_fault_t vmw_bo_vm_huge_fault(struct vm_fault *vmf,
 		prot = vm_get_page_prot(vma->vm_flags);
 	}
 
-	ret = ttm_bo_vm_fault_reserved(vmf, prot, 1, fault_page_size);
+	ret = ttm_bo_vm_fault_reserved(bo, vmf,
+				       drm_vma_node_start(&bo->base.vma_node),
+				       prot, 1, fault_page_size);
 	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
 		return ret;
 
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 639521880c29..434f91f1fdbf 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -605,7 +605,9 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
 vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo,
 			     struct vm_fault *vmf);
 
-vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
+vm_fault_t ttm_bo_vm_fault_reserved(struct ttm_buffer_object *bo,
+				    struct vm_fault *vmf,
+				    unsigned long mmap_base,
 				    pgprot_t prot,
 				    pgoff_t num_prefault,
 				    pgoff_t fault_page_size);
@@ -616,6 +618,11 @@ void ttm_bo_vm_open(struct vm_area_struct *vma);
 
 void ttm_bo_vm_close(struct vm_area_struct *vma);
 
+int ttm_bo_vm_access_reserved(struct ttm_buffer_object *bo,
+			      struct vm_area_struct *vma,
+			      unsigned long offset,
+			      void *buf, int len, int write);
+
 int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr,
 		     void *buf, int len, int write);
 bool ttm_bo_delayed_delete(struct ttm_device *bdev, bool remove_all);
-- 
2.31.1