Re: [PATCH v6 2/8] drm/ttm: Add ttm_bo_access

["Christian König" <christian.koenig@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 8719 bytes --]

Am 04.11.24 um 22:49 schrieb Matthew Brost:
> On Mon, Nov 04, 2024 at 08:28:34PM +0100, Christian König wrote:
>> Am 04.11.24 um 18:34 schrieb Rodrigo Vivi:
>>> On Thu, Oct 31, 2024 at 04:43:19PM -0700, Matthew Brost wrote:
>>>> On Thu, Oct 31, 2024 at 11:10:42AM -0700, Matthew Brost wrote:
>>>>> Non-contiguous VRAM cannot easily be mapped in TTM nor can non-visible
>>>>> VRAM easily be accessed. Add ttm_bo_access, which is similar to
>>>>> ttm_bo_vm_access, to access such memory.
>>>>>
>>>>> v4:
>>>>>    - Fix checkpatch warnings (CI)
>>>>> v5:
>>>>>    - Fix checkpatch warnings (CI)
>>>>> v6:
>>>>>    - Fix kernel doc (Auld)
>>>>>
>>>> Christian - Do you mind if I merge patch along with the rest of the
>>>> series to drm-xe-next?
>>> Ray, Christian,
>>>
>>> ack on getting this patch to drm-xe-next?
>> No, we actually spend quite some time removing the single page mapping
>> functionality for BOs.
>>
> I don't understand this statement. This patch just adds a TTM BO helper
> for access - it doesn't change anything wrt to single page mapping.

Well we spend quite some time removing single page mappings from device 
drivers.

The only remaining use case of ttm_bo_kmap() with just one page is the 
ttm_bo_vm_access_kmap() function and I was really hoping to make that 
one TTM internal at some point.

>> You need a really good justification to bring that back.
>>
> The use case is EuDebugger requires essentially the same functionality
> as ptrace -> vm_access.

Then why don't you use ptrace in the first place?

> TTM mapping non-contiguous VRAM doesn't work unless I'm blind. User BOs
> which the EuDebugger accesses can be non-contiguous, hence the new
> helper.

Then why don't you handle that inside the driver in the first place 
instead of going through a TTM midlayer?

Regards,
Christian.

>
> Matt
>
>> Regards,
>> Christian.
>>
>>>> Matt
>>>>
>>>>> Reported-by: Christoph Manszewski<christoph.manszewski@intel.com>
>>>>> Suggested-by: Thomas Hellström<thomas.hellstrom@linux.intel.com>
>>>>> Signed-off-by: Matthew Brost<matthew.brost@intel.com>
>>>>> Tested-by: Mika Kuoppala<mika.kuoppala@linux.intel.com>
>>>>> Reviewed-by: Matthew Auld<matthew.auld@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/ttm/ttm_bo_util.c | 86 +++++++++++++++++++++++++++++++
>>>>>    drivers/gpu/drm/ttm/ttm_bo_vm.c   | 65 +----------------------
>>>>>    include/drm/ttm/ttm_bo.h          |  2 +
>>>>>    3 files changed, 89 insertions(+), 64 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>>> index d939925efa81..77e760ea7193 100644
>>>>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>>> @@ -919,3 +919,89 @@ s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct ttm_device *bdev,
>>>>>    	return progress;
>>>>>    }
>>>>> +
>>>>> +static int ttm_bo_access_kmap(struct ttm_buffer_object *bo,
>>>>> +			      unsigned long offset,
>>>>> +			      void *buf, int len, int write)
>>>>> +{
>>>>> +	unsigned long page = offset >> PAGE_SHIFT;
>>>>> +	unsigned long bytes_left = len;
>>>>> +	int ret;
>>>>> +
>>>>> +	/* Copy a page at a time, that way no extra virtual address
>>>>> +	 * mapping is needed
>>>>> +	 */
>>>>> +	offset -= page << PAGE_SHIFT;
>>>>> +	do {
>>>>> +		unsigned long bytes = min(bytes_left, PAGE_SIZE - offset);
>>>>> +		struct ttm_bo_kmap_obj map;
>>>>> +		void *ptr;
>>>>> +		bool is_iomem;
>>>>> +
>>>>> +		ret = ttm_bo_kmap(bo, page, 1, &map);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +
>>>>> +		ptr = (void *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
>>>>> +		WARN_ON_ONCE(is_iomem);
>>>>> +		if (write)
>>>>> +			memcpy(ptr, buf, bytes);
>>>>> +		else
>>>>> +			memcpy(buf, ptr, bytes);
>>>>> +		ttm_bo_kunmap(&map);
>>>>> +
>>>>> +		page++;
>>>>> +		buf += bytes;
>>>>> +		bytes_left -= bytes;
>>>>> +		offset = 0;
>>>>> +	} while (bytes_left);
>>>>> +
>>>>> +	return len;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * ttm_bo_access - Helper to access a buffer object
>>>>> + *
>>>>> + * @bo: ttm buffer object
>>>>> + * @offset: access offset into buffer object
>>>>> + * @buf: pointer to caller memory to read into or write from
>>>>> + * @len: length of access
>>>>> + * @write: write access
>>>>> + *
>>>>> + * Utility function to access a buffer object. Useful when buffer object cannot
>>>>> + * be easily mapped (non-contiguous, non-visible, etc...).
>>>>> + *
>>>>> + * Returns:
>>>>> + * @len if successful, negative error code on failure.
>>>>> + */
>>>>> +int ttm_bo_access(struct ttm_buffer_object *bo, unsigned long offset,
>>>>> +		  void *buf, int len, int write)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	if (len < 1 || (offset + len) > bo->base.size)
>>>>> +		return -EIO;
>>>>> +
>>>>> +	ret = ttm_bo_reserve(bo, true, false, NULL);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	switch (bo->resource->mem_type) {
>>>>> +	case TTM_PL_SYSTEM:
>>>>> +		fallthrough;
>>>>> +	case TTM_PL_TT:
>>>>> +		ret = ttm_bo_access_kmap(bo, offset, buf, len, write);
>>>>> +		break;
>>>>> +	default:
>>>>> +		if (bo->bdev->funcs->access_memory)
>>>>> +			ret = bo->bdev->funcs->access_memory
>>>>> +				(bo, offset, buf, len, write);
>>>>> +		else
>>>>> +			ret = -EIO;
>>>>> +	}
>>>>> +
>>>>> +	ttm_bo_unreserve(bo);
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL(ttm_bo_access);
>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>>> index 2c699ed1963a..20b1e5f78684 100644
>>>>> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>>> @@ -366,45 +366,6 @@ void ttm_bo_vm_close(struct vm_area_struct *vma)
>>>>>    }
>>>>>    EXPORT_SYMBOL(ttm_bo_vm_close);
>>>>> -static int ttm_bo_vm_access_kmap(struct ttm_buffer_object *bo,
>>>>> -				 unsigned long offset,
>>>>> -				 uint8_t *buf, int len, int write)
>>>>> -{
>>>>> -	unsigned long page = offset >> PAGE_SHIFT;
>>>>> -	unsigned long bytes_left = len;
>>>>> -	int ret;
>>>>> -
>>>>> -	/* Copy a page at a time, that way no extra virtual address
>>>>> -	 * mapping is needed
>>>>> -	 */
>>>>> -	offset -= page << PAGE_SHIFT;
>>>>> -	do {
>>>>> -		unsigned long bytes = min(bytes_left, PAGE_SIZE - offset);
>>>>> -		struct ttm_bo_kmap_obj map;
>>>>> -		void *ptr;
>>>>> -		bool is_iomem;
>>>>> -
>>>>> -		ret = ttm_bo_kmap(bo, page, 1, &map);
>>>>> -		if (ret)
>>>>> -			return ret;
>>>>> -
>>>>> -		ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
>>>>> -		WARN_ON_ONCE(is_iomem);
>>>>> -		if (write)
>>>>> -			memcpy(ptr, buf, bytes);
>>>>> -		else
>>>>> -			memcpy(buf, ptr, bytes);
>>>>> -		ttm_bo_kunmap(&map);
>>>>> -
>>>>> -		page++;
>>>>> -		buf += bytes;
>>>>> -		bytes_left -= bytes;
>>>>> -		offset = 0;
>>>>> -	} while (bytes_left);
>>>>> -
>>>>> -	return len;
>>>>> -}
>>>>> -
>>>>>    int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr,
>>>>>    		     void *buf, int len, int write)
>>>>>    {
>>>>> @@ -412,32 +373,8 @@ int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr,
>>>>>    	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) > bo->base.size)
>>>>> -		return -EIO;
>>>>> -	ret = ttm_bo_reserve(bo, true, false, NULL);
>>>>> -	if (ret)
>>>>> -		return ret;
>>>>> -
>>>>> -	switch (bo->resource->mem_type) {
>>>>> -	case TTM_PL_SYSTEM:
>>>>> -		fallthrough;
>>>>> -	case TTM_PL_TT:
>>>>> -		ret = ttm_bo_vm_access_kmap(bo, offset, buf, len, write);
>>>>> -		break;
>>>>> -	default:
>>>>> -		if (bo->bdev->funcs->access_memory)
>>>>> -			ret = bo->bdev->funcs->access_memory(
>>>>> -				bo, offset, buf, len, write);
>>>>> -		else
>>>>> -			ret = -EIO;
>>>>> -	}
>>>>> -
>>>>> -	ttm_bo_unreserve(bo);
>>>>> -
>>>>> -	return ret;
>>>>> +	return ttm_bo_access(bo, offset, buf, len, write);
>>>>>    }
>>>>>    EXPORT_SYMBOL(ttm_bo_vm_access);
>>>>> diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
>>>>> index 5804408815be..8ea11cd8df39 100644
>>>>> --- a/include/drm/ttm/ttm_bo.h
>>>>> +++ b/include/drm/ttm/ttm_bo.h
>>>>> @@ -421,6 +421,8 @@ void ttm_bo_unpin(struct ttm_buffer_object *bo);
>>>>>    int ttm_bo_evict_first(struct ttm_device *bdev,
>>>>>    		       struct ttm_resource_manager *man,
>>>>>    		       struct ttm_operation_ctx *ctx);
>>>>> +int ttm_bo_access(struct ttm_buffer_object *bo, unsigned long offset,
>>>>> +		  void *buf, int len, int write);
>>>>>    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,
>>>>> -- 
>>>>> 2.34.1
>>>>>

[-- Attachment #2: Type: text/html, Size: 10076 bytes --]