Re: [PATCH 1/2] drm/qxl: stop abusing TTM to call driver internal functions

[Frediano Ziglio <fziglio@redhat.com>]

> 
> The ttm_mem_io_* functions are actually internal to TTM and shouldn't be
> used in a driver.
> 

As far as I can see by your second patch QXL is just using exported
(that is not internal) functions.
Not that the idea of making them internal is bad but this comment is
a wrong statement.

> Instead call the qxl_ttm_io_mem_reserve() function directly.
> 

I would add that qxl_ttm_io_mem_free is empty so the removal of
ttm_mem_io_free is fine.

> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/qxl/qxl_drv.h    |  2 ++
>  drivers/gpu/drm/qxl/qxl_object.c | 11 +----------
>  drivers/gpu/drm/qxl/qxl_ttm.c    |  4 ++--
>  3 files changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> index 9e034c5fa87d..8a24f8e101da 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -354,6 +354,8 @@ int qxl_mode_dumb_mmap(struct drm_file *filp,
>  /* qxl ttm */
>  int qxl_ttm_init(struct qxl_device *qdev);
>  void qxl_ttm_fini(struct qxl_device *qdev);
> +int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
> +			   struct ttm_mem_reg *mem);
>  int qxl_mmap(struct file *filp, struct vm_area_struct *vma);
>  
>  /* qxl image */
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c
> b/drivers/gpu/drm/qxl/qxl_object.c
> index 548dfe6f3b26..299e63a951c5 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -148,7 +148,6 @@ int qxl_bo_kmap(struct qxl_bo *bo, void **ptr)
>  void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
>  			      struct qxl_bo *bo, int page_offset)
>  {
> -	struct ttm_mem_type_manager *man =
> &bo->tbo.bdev->man[bo->tbo.mem.mem_type];
>  	void *rptr;
>  	int ret;
>  	struct io_mapping *map;
> @@ -160,9 +159,7 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
>  	else
>  		goto fallback;
>  
> -	(void) ttm_mem_io_lock(man, false);
> -	ret = ttm_mem_io_reserve(bo->tbo.bdev, &bo->tbo.mem);
> -	ttm_mem_io_unlock(man);
> +	ret = qxl_ttm_io_mem_reserve(bo->tbo.bdev, &bo->tbo.mem);
>  
>  	return io_mapping_map_atomic_wc(map, bo->tbo.mem.bus.offset + page_offset);
>  fallback:
> @@ -193,17 +190,11 @@ void qxl_bo_kunmap(struct qxl_bo *bo)
>  void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
>  			       struct qxl_bo *bo, void *pmap)
>  {
> -	struct ttm_mem_type_manager *man =
> &bo->tbo.bdev->man[bo->tbo.mem.mem_type];
> -
>  	if ((bo->tbo.mem.mem_type != TTM_PL_VRAM) &&
>  	    (bo->tbo.mem.mem_type != TTM_PL_PRIV))
>  		goto fallback;
>  
>  	io_mapping_unmap_atomic(pmap);
> -
> -	(void) ttm_mem_io_lock(man, false);
> -	ttm_mem_io_free(bo->tbo.bdev, &bo->tbo.mem);
> -	ttm_mem_io_unlock(man);
>  	return;
>   fallback:
>  	qxl_bo_kunmap(bo);
> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
> index 9b24514c75aa..cb80e512dd46 100644
> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> @@ -159,8 +159,8 @@ static int qxl_verify_access(struct ttm_buffer_object
> *bo, struct file *filp)
>  					  filp->private_data);
>  }
>  
> -static int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
> -				  struct ttm_mem_reg *mem)
> +int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
> +			   struct ttm_mem_reg *mem)
>  {
>  	struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
>  	struct qxl_device *qdev = qxl_get_qdev(bdev);

Otherwise fine for me.

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel