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

[Koenig, Christian]

Am 30.09.19 um 11:51 schrieb Frediano Ziglio:
>> Am 27.09.19 um 18:31 schrieb Frediano Ziglio:
>>>> 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.
>> See the history of exporting those, that was done specifically so that
>> QXL can call them (commit afe6804c045fbd69a1b75c681107b5d6df9190de).
>>
>> But those are the internal functions which TTM uses to call into the
>> driver. That a driver uses them to call into itself doesn't seem to make
>> sense.
>>
> The commit was merged and release in Linux 3.10 6 years ago, since
> then these functions have been exported. Not saying that the QXL change
> was wrong and should not have been acked and merged but after 6 years
> saying that these functions are internal it's not correct.

Why? If a function is internal or not is defined by the design and not 
the actual implementation.

> Something like
>
> "The ttm_mem_io_* functions were intended to be internal to TTM and
> shouldn't have been used in a driver. They were exported in commit
> afe6804c045fbd69a1b75c681107b5d6df9190de just for QXL."

Good point mentioning the commit adding that, going to use this for the 
commit message.

Christian.

>
>>>> 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.
>> Good point, going to add that.
>>
>> Thanks,
>> Christian.
>>
> Frediano
>
>>>> Signed-off-by: Christian König <christian.koenig at 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
>>