[PATCH 1/2] drm: Implement vm_operations_struct.access

[Christian König]

Am 13.07.2017 um 23:08 schrieb Felix Kuehling:
> Allows gdb to access contents of user mode mapped BOs. VRAM access
> requires the driver to implement a new callback in ttm_bo_driver.

One more comment additionally to what Michel already wrote below, apart 
from that it looks good to me.

>
> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo_vm.c | 66 ++++++++++++++++++++++++++++++++++++++++-
>   include/drm/ttm/ttm_bo_driver.h | 12 ++++++++
>   2 files changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index 9f53df9..0ef2eb9 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -294,10 +294,74 @@ static void ttm_bo_vm_close(struct vm_area_struct *vma)
>   	vma->vm_private_data = NULL;
>   }
>   
> +static int ttm_bo_vm_access_kmap(struct ttm_buffer_object *bo,
> +				 unsigned long offset,
> +				 void *buf, int len, int write)
> +{
> +	unsigned long first_page = offset >> PAGE_SHIFT;
> +	unsigned long last_page = (offset + len - 1) >> PAGE_SHIFT;
> +	unsigned long num_pages = last_page - first_page + 1;
> +	struct ttm_bo_kmap_obj map;
> +	void *ptr;
> +	bool is_iomem;
> +	int ret;
> +
> +	ret = ttm_bo_kmap(bo, first_page, num_pages, &map);
> +	if (ret)
> +		return ret;
> +
> +	offset -= first_page << PAGE_SHIFT;
> +	ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
> +	WARN_ON(is_iomem);
> +	if (write)
> +		memcpy(ptr, buf, len);
> +	else
> +		memcpy(buf, ptr, len);
> +	ttm_bo_kunmap(&map);
> +
> +	return len;
> +}
> +
> +static int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr,
> +			    void *buf, int len, int write)
> +{
> +	unsigned long offset = (addr) - vma->vm_start;
> +	struct ttm_buffer_object *bo = vma->vm_private_data;
> +	int ret;
> +
> +	if (len < 1 || (offset + len) >> PAGE_SHIFT > bo->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:

When the BO is in the system domain you need to add this as well I think:

         if (unlikely(bo->ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) {
                 ret = ttm_tt_swapin(bo->ttm);
                 if (unlikely(ret != 0))
                         return ret;
         }

Regards,
Christian.

> +	case TTM_PL_TT:
> +		ret = ttm_bo_vm_access_kmap(bo, offset, buf, len, write);
> +		break;
> +	case TTM_PL_VRAM:
> +		if (bo->bdev->driver->access_vram)
> +			ret = bo->bdev->driver->access_vram(
> +				bo, offset, buf, len, write);
> +		else
> +			ret = -EIO;
> +		break;
> +	default:
> +		ret = -EIO;
> +	}
> +
> +	ttm_bo_unreserve(bo);
> +
> +	return ret;
> +}
> +
>   static const struct vm_operations_struct ttm_bo_vm_ops = {
>   	.fault = ttm_bo_vm_fault,
>   	.open = ttm_bo_vm_open,
> -	.close = ttm_bo_vm_close
> +	.close = ttm_bo_vm_close,
> +	.access = ttm_bo_vm_access
>   };
>   
>   static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device *bdev,
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 6bbd34d..6ce5094 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -471,6 +471,18 @@ struct ttm_bo_driver {
>   	 */
>   	unsigned long (*io_mem_pfn)(struct ttm_buffer_object *bo,
>   				    unsigned long page_offset);
> +
> +	/**
> +	 * Read/write VRAM buffers for ptrace access
> +	 *
> +	 * @bo: the BO to access
> +	 * @offset: the offset from the start of the BO
> +	 * @buf: pointer to source/destination buffer
> +	 * @len: number of bytes to copy
> +	 * @write: whether to read (0) from or write (non-0) to BO
> +	 */
> +	int (*access_vram)(struct ttm_buffer_object *bo, unsigned long offset,
> +			   void *buf, int len, int write);
>   };
>   
>   /**