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

[Michel Dänzer]

On 14/07/17 06:08 AM, Felix Kuehling wrote:
> Allows gdb to access contents of user mode mapped BOs. VRAM access
> requires the driver to implement a new callback in ttm_bo_driver.

Thanks for doing this. Looks mostly good, but I still have some comments.

The shortlog prefix should be "drm/ttm:".


> 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;

Could there be cases (e.g. on 32-bit) where mapping all pages at once
fails, but mapping one page at a time would work?


> +	offset -= first_page << PAGE_SHIFT;
> +	ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
> +	WARN_ON(is_iomem);

I suggest making this WARN_ON_ONCE, to avoid flooding dmesg if it ever
triggers in practice.


>  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

Is there any reason for making this specific to VRAM? ttm_bo_vm_access
could just call this for anything except TTM_PL_SYSTEM / TTM_PL_TT, and
let the driver return an error if it can't handle the memory type.


> +	 * @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
> +	 */

The meaning of the return value should also be documented here.


> +	int (*access_vram)(struct ttm_buffer_object *bo, unsigned long offset,
> +			   void *buf, int len, int write);
>  };

I suggest making the write parameter a bool.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer