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

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

[Christian König]

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

Instead call the qxl_ttm_io_mem_reserve() function directly.

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

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

[PATCH 2/2] drm/ttm: stop exporting ttm_mem_io_* functions

[Christian König]

Those are not supposed to be used by drivers.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 086ba2c2f60b..2eca752c39e9 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -102,7 +102,6 @@ int ttm_mem_io_lock(struct ttm_mem_type_manager *man, bool interruptible)
 	mutex_lock(&man->io_reserve_mutex);
 	return 0;
 }
-EXPORT_SYMBOL(ttm_mem_io_lock);
 
 void ttm_mem_io_unlock(struct ttm_mem_type_manager *man)
 {
@@ -111,7 +110,6 @@ void ttm_mem_io_unlock(struct ttm_mem_type_manager *man)
 
 	mutex_unlock(&man->io_reserve_mutex);
 }
-EXPORT_SYMBOL(ttm_mem_io_unlock);
 
 static int ttm_mem_io_evict(struct ttm_mem_type_manager *man)
 {
@@ -153,7 +151,6 @@ int ttm_mem_io_reserve(struct ttm_bo_device *bdev,
 	}
 	return ret;
 }
-EXPORT_SYMBOL(ttm_mem_io_reserve);
 
 void ttm_mem_io_free(struct ttm_bo_device *bdev,
 		     struct ttm_mem_reg *mem)
@@ -169,7 +166,6 @@ void ttm_mem_io_free(struct ttm_bo_device *bdev,
 		bdev->driver->io_mem_free(bdev, mem);
 
 }
-EXPORT_SYMBOL(ttm_mem_io_free);
 
 int ttm_mem_io_reserve_vm(struct ttm_buffer_object *bo)
 {
-- 
2.14.1

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

Re: [PATCH 2/2] drm/ttm: stop exporting ttm_mem_io_* functions

[Gerd Hoffmann]

On Fri, Sep 27, 2019 at 03:24:58PM +0200, Christian König wrote:
> Those are not supposed to be used by drivers.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

Re: [PATCH 2/2] drm/ttm: stop exporting ttm_mem_io_* functions

[Gerd Hoffmann]

On Fri, Sep 27, 2019 at 03:24:58PM +0200, Christian König wrote:
> Those are not supposed to be used by drivers.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

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

[PATCH 2/2] drm/ttm: stop exporting ttm_mem_io_* functions

[Christian König]

Those are not supposed to be used by drivers.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 086ba2c2f60b..2eca752c39e9 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -102,7 +102,6 @@ int ttm_mem_io_lock(struct ttm_mem_type_manager *man, bool interruptible)
 	mutex_lock(&man->io_reserve_mutex);
 	return 0;
 }
-EXPORT_SYMBOL(ttm_mem_io_lock);
 
 void ttm_mem_io_unlock(struct ttm_mem_type_manager *man)
 {
@@ -111,7 +110,6 @@ void ttm_mem_io_unlock(struct ttm_mem_type_manager *man)
 
 	mutex_unlock(&man->io_reserve_mutex);
 }
-EXPORT_SYMBOL(ttm_mem_io_unlock);
 
 static int ttm_mem_io_evict(struct ttm_mem_type_manager *man)
 {
@@ -153,7 +151,6 @@ int ttm_mem_io_reserve(struct ttm_bo_device *bdev,
 	}
 	return ret;
 }
-EXPORT_SYMBOL(ttm_mem_io_reserve);
 
 void ttm_mem_io_free(struct ttm_bo_device *bdev,
 		     struct ttm_mem_reg *mem)
@@ -169,7 +166,6 @@ void ttm_mem_io_free(struct ttm_bo_device *bdev,
 		bdev->driver->io_mem_free(bdev, mem);
 
 }
-EXPORT_SYMBOL(ttm_mem_io_free);
 
 int ttm_mem_io_reserve_vm(struct ttm_buffer_object *bo)
 {
-- 
2.14.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[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.

> 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

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

[Koenig, Christian]

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.

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

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Koenig, Christian]

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.

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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.

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."

> >> 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@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
> 
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [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@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
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Frediano Ziglio]

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

Where's documented? I cannot find it. Probably my kernel devel is a bit
rusty.

> > 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@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.
> >>>
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

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

Where's documented? I cannot find it. Probably my kernel devel is a bit
rusty.

> > 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@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.
> >>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [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@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
>>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[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.

> 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Gerd Hoffmann]

On Fri, Sep 27, 2019 at 03:24:57PM +0200, Christian König wrote:
> The ttm_mem_io_* functions are actually internal to TTM and shouldn't be
> used in a driver.
> 
> Instead call the qxl_ttm_io_mem_reserve() function directly.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

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

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

[Gerd Hoffmann]

On Fri, Sep 27, 2019 at 03:24:57PM +0200, Christian König wrote:
> The ttm_mem_io_* functions are actually internal to TTM and shouldn't be
> used in a driver.
> 
> Instead call the qxl_ttm_io_mem_reserve() function directly.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

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

[Christian König]

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

Instead call the qxl_ttm_io_mem_reserve() function directly.

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization