Re: [bisected] Re: drm, qxl: post 5.11 merge warning+explosion
Please test it a bit. I don't know the qxl code at all, but it looks like the dma address array is just superfluous. Christian. Am 17.12.2020 17:55 schrieb Mike Galbraith : On Thu, 2020-12-17 at 17:38 +0100, Christian König wrote: > > Mike can you test the attached patch? Yup, one-liner made it all better. That was quick like bunny. -Mike ___ 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
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
---
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,
+
Re: [Spice-devel] [PATCH 1/2] drm/qxl: stop abusing TTM to call driver internal functions
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
>> ---
>> 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 v3 2/3] drm: plumb attaching dev thru to prime_pin/unpin
Am 16.07.19 um 23:37 schrieb Rob Clark:
> From: Rob Clark
>
> Needed in the following patch for cache operations.
Well have you seen that those callbacks are deprecated?
>* Deprecated hook in favour of &drm_gem_object_funcs.pin.
>* Deprecated hook in favour of &drm_gem_object_funcs.unpin.
>
I would rather say if you want to extend something it would be better to
switch over to the per GEM object functions first.
Regards,
Christian.
>
> Signed-off-by: Rob Clark
> ---
> v3: rebased on drm-tip
>
> drivers/gpu/drm/drm_gem.c | 8
> drivers/gpu/drm/drm_internal.h | 4 ++--
> drivers/gpu/drm/drm_prime.c | 4 ++--
> drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 4 ++--
> drivers/gpu/drm/msm/msm_drv.h | 4 ++--
> drivers/gpu/drm/msm/msm_gem_prime.c | 4 ++--
> drivers/gpu/drm/nouveau/nouveau_gem.h | 4 ++--
> drivers/gpu/drm/nouveau/nouveau_prime.c | 4 ++--
> drivers/gpu/drm/qxl/qxl_prime.c | 4 ++--
> drivers/gpu/drm/radeon/radeon_prime.c | 4 ++--
> drivers/gpu/drm/vgem/vgem_drv.c | 4 ++--
> include/drm/drm_drv.h | 5 ++---
> 12 files changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 84689ccae885..af2549c45027 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1215,22 +1215,22 @@ void drm_gem_print_info(struct drm_printer *p,
> unsigned int indent,
> obj->dev->driver->gem_print_info(p, indent, obj);
> }
>
> -int drm_gem_pin(struct drm_gem_object *obj)
> +int drm_gem_pin(struct drm_gem_object *obj, struct device *dev)
> {
> if (obj->funcs && obj->funcs->pin)
> return obj->funcs->pin(obj);
> else if (obj->dev->driver->gem_prime_pin)
> - return obj->dev->driver->gem_prime_pin(obj);
> + return obj->dev->driver->gem_prime_pin(obj, dev);
> else
> return 0;
> }
>
> -void drm_gem_unpin(struct drm_gem_object *obj)
> +void drm_gem_unpin(struct drm_gem_object *obj, struct device *dev)
> {
> if (obj->funcs && obj->funcs->unpin)
> obj->funcs->unpin(obj);
> else if (obj->dev->driver->gem_prime_unpin)
> - obj->dev->driver->gem_prime_unpin(obj);
> + obj->dev->driver->gem_prime_unpin(obj, dev);
> }
>
> void *drm_gem_vmap(struct drm_gem_object *obj)
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 51a2055c8f18..e64090373e3a 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -133,8 +133,8 @@ void drm_gem_release(struct drm_device *dev, struct
> drm_file *file_private);
> void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
> const struct drm_gem_object *obj);
>
> -int drm_gem_pin(struct drm_gem_object *obj);
> -void drm_gem_unpin(struct drm_gem_object *obj);
> +int drm_gem_pin(struct drm_gem_object *obj, struct device *dev);
> +void drm_gem_unpin(struct drm_gem_object *obj, struct device *dev);
> void *drm_gem_vmap(struct drm_gem_object *obj);
> void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr);
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 189d980402ad..126860432ff9 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -575,7 +575,7 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
> {
> struct drm_gem_object *obj = dma_buf->priv;
>
> - return drm_gem_pin(obj);
> + return drm_gem_pin(obj, attach->dev);
> }
> EXPORT_SYMBOL(drm_gem_map_attach);
>
> @@ -593,7 +593,7 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
> {
> struct drm_gem_object *obj = dma_buf->priv;
>
> - drm_gem_unpin(obj);
> + drm_gem_unpin(obj, attach->dev);
> }
> EXPORT_SYMBOL(drm_gem_map_detach);
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> index a05292e8ed6f..67e69a5f00f2 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> @@ -43,7 +43,7 @@ int etnaviv_gem_prime_mmap(struct drm_gem_object *obj,
> return etnaviv_obj->ops->mmap(etnaviv_obj, vma);
> }
>
> -int etnaviv_gem_prime_pin(struct drm_gem_object *obj)
> +int etnaviv_gem_prime_pin(struct drm_gem_object *obj, struct device *dev)
> {
> if (!obj->import_attach) {
> struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
> @@ -55,7 +55,7 @@ int etnaviv_gem_prime_pin(struct drm_gem_object *obj)
> return 0;
> }
>
> -void etnaviv_gem_prime_unpin(struct drm_gem_object *obj)
> +void etnaviv_gem_prime_unpin(struct drm_gem_object *obj, struct device *dev)
> {
> if (!obj->import_attach) {
> struct etnaviv_gem_object *etnaviv_obj = to_etnaviv
Re: [PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200
Am 20.05.19 um 18:26 schrieb Daniel Vetter:
> [CAUTION: External Email]
>
> On Mon, May 20, 2019 at 06:19:00PM +0200, Daniel Vetter wrote:
>> On Thu, May 16, 2019 at 06:27:45PM +0200, Thomas Zimmermann wrote:
>>> The new interfaces drm_gem_vram_{pin/unpin}_reserved() are variants of the
>>> GEM VRAM pin/unpin functions that do not reserve the BO during validation.
>>> The mgag200 driver requires this behavior for its cursor handling. The
>>> patch also converts the driver to use the new interfaces.
>>>
>>> Signed-off-by: Thomas Zimmermann
>>> ---
>>> drivers/gpu/drm/drm_gem_vram_helper.c| 75
>>> drivers/gpu/drm/mgag200/mgag200_cursor.c | 18 +++---
>>> include/drm/drm_gem_vram_helper.h| 3 +
>>> 3 files changed, 88 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>> index 8f142b810eb4..a002c03eaf4c 100644
>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>> @@ -254,6 +254,47 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo,
>>> unsigned long pl_flag)
>>> }
>>> EXPORT_SYMBOL(drm_gem_vram_pin);
>>>
>>> +/**
>>> + * drm_gem_vram_pin_reserved() - Pins a GEM VRAM object in a region.
>>> + * @gbo: the GEM VRAM object
>>> + * @pl_flag: a bitmask of possible memory regions
>>> + *
>>> + * Pinning a buffer object ensures that it is not evicted from
>>> + * a memory region. A pinned buffer object has to be unpinned before
>>> + * it can be pinned to another region.
>>> + *
>>> + * This function pins a GEM VRAM object that has already been
>>> + * reserved. Use drm_gem_vram_pin() if possible.
>>> + *
>>> + * Returns:
>>> + * 0 on success, or
>>> + * a negative error code otherwise.
>>> + */
>>> +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
>>> + unsigned long pl_flag)
>>> +{
>>> + int i, ret;
>>> + struct ttm_operation_ctx ctx = { false, false };
>> I think would be good to have a lockdep_assert_held here for the ww_mutex.
>>
>> Also general thing: _reserved is kinda ttm lingo, for dma-buf reservations
>> we call the structure tracking the fences+lock the "reservation", but the
>> naming scheme used is _lock/_unlock.
>>
>> I think would be good to be consistent with that, and use _locked here.
>> Especially for a very simplified vram helper like this one I expect that's
>> going to lead to less wtf moments by driver writers :-)
>>
>> Maybe we should also do a large-scale s/reserve/lock/ within ttm, to align
>> more with what we now have in dma-buf.
> More aside:
>
> Could be a good move to demidlayer this an essentially remove
> ttm_bo_reserve as a wrapper around the linux/reservation.h functions. Not
> sure whether that aligns with Christian's plans. TODO.rst patch might be a
> good step to get that discussion started.
Well what ttm_bo_reserve()/_unreserve() does is a) lock/unlock the
reservation object and b) remove the BO from the LRU.
Since I'm desperately trying to get rid of the LRU removal right now we
sooner or later should be able to remove ttmo_bo_reserve()/_unreserve()
as well (or at least replace them with tiny ttm_bo_lock() wrappers.
Christian.
> -Daniel
>
>> Cheers, Daniel
>>
>>> +
>>> + if (gbo->pin_count) {
>>> + ++gbo->pin_count;
>>> + return 0;
>>> + }
>>> +
>>> + drm_gem_vram_placement(gbo, pl_flag);
>>> + for (i = 0; i < gbo->placement.num_placement; ++i)
>>> + gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>>> +
>>> + ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + gbo->pin_count = 1;
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_gem_vram_pin_reserved);
>>> +
>>> /**
>>>* drm_gem_vram_unpin() - Unpins a GEM VRAM object
>>>* @gbo: the GEM VRAM object
>>> @@ -285,6 +326,40 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
>>> }
>>> EXPORT_SYMBOL(drm_gem_vram_unpin);
>>>
>>> +/**
>>> + * drm_gem_vram_unpin_reserved() - Unpins a GEM VRAM object
>>> + * @gbo: the GEM VRAM object
>>> + *
>>> + * This function unpins a GEM VRAM object that has already been
>>> + * reserved. Use drm_gem_vram_unpin() if possible.
>>> + *
>>> + * Returns:
>>> + * 0 on success, or
>>> + * a negative error code otherwise.
>>> + */
>>> +int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo)
>>> +{
>>> + int i, ret;
>>> + struct ttm_operation_ctx ctx = { false, false };
>>> +
>>> + if (WARN_ON_ONCE(!gbo->pin_count))
>>> + return 0;
>>> +
>>> + --gbo->pin_count;
>>> + if (gbo->pin_count)
>>> + return 0;
>>> +
>>> + for (i = 0; i < gbo->placement.num_placement ; ++i)
>>> + gbo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
>>> +
>>> + ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYM
Re: [PATCH v3 01/19] drm: Add |struct drm_gem_vram_object| and helpers
Am 03.05.19 um 14:01 schrieb Daniel Vetter:
> [CAUTION: External Email]
>
> On Fri, May 3, 2019 at 12:15 PM Thomas Zimmermann wrote:
>> Hi Christian,
>>
>> would you review the whole patch set? Daniel mentioned that he'd prefer
>> to leave the review to memory-mgmt developers.
> I think Noralf Tronnes or Gerd Hoffmann would also make good reviewers
> for this, fairly close to what they've been working on in the past.
I will try to take another look next week. Busy as usual here.
Christian.
> -Daniel
>
>> Best regards
>> Thomas
>>
>> Am 30.04.19 um 11:35 schrieb Koenig, Christian:
>>> Am 30.04.19 um 11:23 schrieb Sam Ravnborg:
>>>> [CAUTION: External Email]
>>>>
>>>> Hi Thomas.
>>>>
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * Returns the container of type &struct drm_gem_vram_object
>>>>>>> + * for field bo.
>>>>>>> + * @bo: the VRAM buffer object
>>>>>>> + * Returns: The containing GEM VRAM object
>>>>>>> + */
>>>>>>> +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
>>>>>>> + struct ttm_buffer_object *bo)
>>>>>>> +{
>>>>>>> + return container_of(bo, struct drm_gem_vram_object, bo);
>>>>>>> +}
>>>>>> Indent funny. USe same indent as used in other parts of file for
>>>>>> function arguments.
>>>>> If I put the argument next to the function's name, it will exceed the
>>>>> 80-character limit. From the coding-style document, I could not see what
>>>>> to do in this case. One solution would move the return type to a
>>>>> separate line before the function name. I've not seen that anywhere in
>>>>> the source code, so moving the argument onto a separate line and
>>>>> indenting by one tab appears to be the next best solution. Please let me
>>>>> know if there's if there's a preferred style for cases like this one.
>>>> Readability has IMO higher priority than some limit of 80 chars.
>>>> And it hurts readability (at least my OCD) when style changes
>>>> as you do with indent here. So my personal preference is to fix
>>>> indent and accect longer lines.
>>> In this case the an often used convention (which is also kind of
>>> readable) is to add a newline after the return values, but before the
>>> function name. E.g. something like this:
>>>
>>> static inline struct drm_gem_vram_object*
>>> drm_gem_vram_of_bo(struct ttm_buffer_object *bo)
>>>
>>> Regards,
>>> Christian.
>>>
>>>> But you ask for a preferred style - which I do not think we have in this
>>>> case. So it boils down to what you prefer.
>>>>
>>>> Enough bikeshedding, thanks for the quick response.
>>>>
>>>> Sam
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
>> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
>> HRB 21284 (AG Nürnberg)
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 01/19] drm: Add |struct drm_gem_vram_object| and helpers
Am 30.04.19 um 11:23 schrieb Sam Ravnborg:
> [CAUTION: External Email]
>
> Hi Thomas.
>
+
+/**
+ * Returns the container of type &struct drm_gem_vram_object
+ * for field bo.
+ * @bo: the VRAM buffer object
+ * Returns: The containing GEM VRAM object
+ */
+static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
+ struct ttm_buffer_object *bo)
+{
+ return container_of(bo, struct drm_gem_vram_object, bo);
+}
>>> Indent funny. USe same indent as used in other parts of file for
>>> function arguments.
>> If I put the argument next to the function's name, it will exceed the
>> 80-character limit. From the coding-style document, I could not see what
>> to do in this case. One solution would move the return type to a
>> separate line before the function name. I've not seen that anywhere in
>> the source code, so moving the argument onto a separate line and
>> indenting by one tab appears to be the next best solution. Please let me
>> know if there's if there's a preferred style for cases like this one.
> Readability has IMO higher priority than some limit of 80 chars.
> And it hurts readability (at least my OCD) when style changes
> as you do with indent here. So my personal preference is to fix
> indent and accect longer lines.
In this case the an often used convention (which is also kind of
readable) is to add a newline after the return values, but before the
function name. E.g. something like this:
static inline struct drm_gem_vram_object*
drm_gem_vram_of_bo(struct ttm_buffer_object *bo)
Regards,
Christian.
>
> But you ask for a preferred style - which I do not think we have in this
> case. So it boils down to what you prefer.
>
> Enough bikeshedding, thanks for the quick response.
>
> Sam
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 02/17] drm: Add |struct drm_gem_vram_object| callbacks for |struct ttm_bo_driver|
Am 24.04.19 um 14:05 schrieb Thomas Zimmermann:
> Hi Christian,
>
> Am 24.04.19 um 13:48 schrieb Thomas Zimmermann:
>> +
>> +/*
>> + * Helpers for struct ttm_bo_driver
>> + */
>> +
>> +static bool drm_is_gem_vram(struct ttm_buffer_object *bo)
>> +{
>> +return (bo->destroy == ttm_buffer_object_destroy);
>> +}
>> +
>> +/**
>> + * drm_gem_vram_bo_driver_evict_flags() - \
>> +Implements &struct ttm_bo_driver.evict_flags
>> + * @bo: TTM buffer object. Refers to &struct drm_gem_vram_object.bo
>> + * @pl: TTM placement information.
>> + */
>> +void drm_gem_vram_bo_driver_evict_flags(struct ttm_buffer_object *bo,
>> +struct ttm_placement *pl)
>> +{
>> +struct drm_gem_vram_object *gbo;
>> +
>> +/* TTM may pass BOs that are not GEM VRAM BOs. */
>> +if (!drm_is_gem_vram(bo))
>> +return;
>> +
>> +gbo = drm_gem_vram_of_bo(bo);
>> +drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM);
>> +*pl = gbo->placement;
>> +}
> For drm_is_gem_vram(), I'm not quite sure what else to test for. So
> there are still a few things I'd like to discuss.
>
> 1) If this test is about the placement flags, then it's unrelated to
> the actual DRM driver. All buffers of type |struct drm_gem_vram_object|
> share the same placement flags.
>
> 2) I tested the code to work with ast and mgag200.
>
> 3) If this test is really about individual instances of each DRM
> driver, then the current implementations are already broken. In this
> scenario TTM should sort out BOs with a BO device different from the one
> that triggered the eviction process; or pass the original BO device to
> the evict_flags callback, so we can sort out buffers here.
Just go ahead with the current approach for now. It could be that we see
fallout, but that is really unlikely.
Christian.
>
> Best regards
> Thomas
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 00/15] Share TTM code among framebuffer drivers
Am 16.04.19 um 13:03 schrieb Daniel Vetter: > On Tue, Apr 16, 2019 at 12:05 PM Koenig, Christian > wrote: >> Am 15.04.19 um 21:17 schrieb Daniel Vetter: >>> On Mon, Apr 15, 2019 at 6:21 PM Thomas Zimmermann >>> wrote: >>>> Hi >>>> >>>> Am 15.04.19 um 17:54 schrieb Daniel Vetter: >>>>> On Tue, Apr 09, 2019 at 09:50:40AM +0200, Thomas Zimmermann wrote: >>>>>> Hi >>>>>> >>>>>> Am 09.04.19 um 09:12 schrieb kra...@redhat.com: >>>>>> [SNIP] >>>>>>> I'd expect the same applies to the vbox driver. >>>>>>> >>>>>>> Dunno about the other drm drivers and the fbdev drivers you plan to >>>>>>> migrate over. >>>>>> The AST HW can support up to 512 MiB, but 32-64 MiB seems more realistic >>>>>> for a server. It's similar with mgag200 HW. The old fbdev-supported >>>>>> device are all somewhere in the range between cirrus and bochs. Some >>>>>> drivers would probably benefit from the cirrus approach, some could use >>>>>> VRAM directly. >>>>> I think for dumb scanout with vram all we need is: >>>>> - pin framebuffers, which potentially moves the underlying bo into vram >>>>> - unpin framebuffers (which is just accounting, we don't want to move the >>>>> bo on every flip!) >>>>> - if a pin doesn't find enough space, move one of the unpinned bo still >>>>> resident in vram out >>>> For dumb buffers, I'd expect userspace to have a working set of only a >>>> front and back buffer (plus maybe a third one). This working set has to >>>> reside in VRAM for performance reasons; non-WS BOs from other userspace >>>> programs don't have to be. >>>> >>>> So we could simplify even more: if there's not enough free space in >>>> vram, remove all unpinned BO's. This would avoid the need to implement >>>> an LRU algorithm or another eviction strategy. Userspace with a WS >>>> larger than the absolute VRAM would see degraded performance but >>>> otherwise still work. >>> You still need a list of unpinned bo, and the lru scan algorithm is >>> just a few lines of code more than unpinning everything. Plus it'd be >>> a neat example of the drm_mm scan logic. Given that some folks might >>> think that not having lru evict si a problem and get them to type >>> their own, I'd just add it. But up to you. Plus with ttm you get it no >>> matter what. >> Well how about making an drm_lru component which just does the following >> (and nothing else, please :): >> >> 1. Keep a list of objects and a spinlock protecting the list. >> >> 2. Offer helpers for adding/deleting/moving stuff from the list. >> >> 3. Offer a functionality to do the necessary dance of picking the first >> entry where we can trylock it's reservation object. >> >> 4. Offer bulk move functionality similar to what TTM does at the moment >> (can be implemented later on). > At a basic level, this is list_head of drm_gem_object. Not sure that's > all that useful (outside of the fairly simplistic vram helpers we're > discussing here). Reasons for that is that there's a lot of trickery > in selecting which is the best object to pick in any given case (e.g. > do you want to use drm_mm scanning, or is there a slab of objects you > prefer to throw out because that avoids. Given that I'm not sure > implementing the entire scanning/drm_lru logic is beneficial. > > The magic trylock+kref_get_unless_zero otoh could be worth > implementing as a helper, together with a note about how to build your > own custom lru algorithm. Same for some bulk/nonblocking movement > helpers maybe. Both not really needed for the dumb scanout vram > helpers we're discussing here. Yeah, exactly that's what I wanted to get towards as well. This magic trylock+kref_get is what needs to be handled correctly by all drivers which implement an LRU. LRU bulk move is something which is tricky to get right as well, but so far only amdgpu uses it so it only make sense to share when somebody else wants the same approach. Christian. > -Daniel > > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 00/15] Share TTM code among framebuffer drivers
Am 15.04.19 um 21:17 schrieb Daniel Vetter: > On Mon, Apr 15, 2019 at 6:21 PM Thomas Zimmermann wrote: >> Hi >> >> Am 15.04.19 um 17:54 schrieb Daniel Vetter: >>> On Tue, Apr 09, 2019 at 09:50:40AM +0200, Thomas Zimmermann wrote: Hi Am 09.04.19 um 09:12 schrieb kra...@redhat.com: [SNIP] > I'd expect the same applies to the vbox driver. > > Dunno about the other drm drivers and the fbdev drivers you plan to > migrate over. The AST HW can support up to 512 MiB, but 32-64 MiB seems more realistic for a server. It's similar with mgag200 HW. The old fbdev-supported device are all somewhere in the range between cirrus and bochs. Some drivers would probably benefit from the cirrus approach, some could use VRAM directly. >>> I think for dumb scanout with vram all we need is: >>> - pin framebuffers, which potentially moves the underlying bo into vram >>> - unpin framebuffers (which is just accounting, we don't want to move the >>>bo on every flip!) >>> - if a pin doesn't find enough space, move one of the unpinned bo still >>>resident in vram out >> For dumb buffers, I'd expect userspace to have a working set of only a >> front and back buffer (plus maybe a third one). This working set has to >> reside in VRAM for performance reasons; non-WS BOs from other userspace >> programs don't have to be. >> >> So we could simplify even more: if there's not enough free space in >> vram, remove all unpinned BO's. This would avoid the need to implement >> an LRU algorithm or another eviction strategy. Userspace with a WS >> larger than the absolute VRAM would see degraded performance but >> otherwise still work. > You still need a list of unpinned bo, and the lru scan algorithm is > just a few lines of code more than unpinning everything. Plus it'd be > a neat example of the drm_mm scan logic. Given that some folks might > think that not having lru evict si a problem and get them to type > their own, I'd just add it. But up to you. Plus with ttm you get it no > matter what. Well how about making an drm_lru component which just does the following (and nothing else, please :): 1. Keep a list of objects and a spinlock protecting the list. 2. Offer helpers for adding/deleting/moving stuff from the list. 3. Offer a functionality to do the necessary dance of picking the first entry where we can trylock it's reservation object. 4. Offer bulk move functionality similar to what TTM does at the moment (can be implemented later on). Regards, Christian. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 00/15] Share TTM code among framebuffer drivers
Am 08.04.19 um 13:59 schrieb Thomas Zimmermann: [SNIP] > If not for TTM, what would be the alternative? One VMA manager per > memory region per device? Since everybody vital seems to be on this mail thread anyway, let's use it a bit for brain storming what a possible replacement for TTM should look like. Well for simple drivers like qemu/bochs and cirrus the answer is to not use it at all. E.g. VRAM is only used for scanout and unprivileged userspace should not mess with it at all. In this case we don't need dynamic eviction and so also don't need TTM. That leaves us with the more complex drivers, like radeon, amdgpu, nouveu and maybe some of the ARM based stuff, with vmwgfx being a bit special here. Now I can summarize the requirements for at least the amdgpu in the following way: 1. We need to be able to allocate memory objects in different locations. 2. We need to be able to move around memory objects between different locations. 3. We need some LRU component which tells us what to evict when memory in a location becomes to tight. Now for lessons learned we should at least avoid the following design pitfalls: A) DON'T make it a layered design! Layers are for making cake, not software. B) DON'T make it a "Eierlegende Wollmilchsau" (German saying). E.g. don't try to solve every single corner cases in one piece of software. Let's make components which solve one specific problem. C) Pipeline everything! E.g. the hardware we deal with is asynchronous by design. Blocking for the hardware to finish in the common components itself is an absolutely no-go. If a driver wants to do something synchronous it should wait itself. Those comments where not really intended for you Thomas, but I had to write them down somewhere :) Regards, Christian. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 00/15] Share TTM code among framebuffer drivers
Well first problem is I'm not sure if that is a good idea. Essentially
we want to get rid of TTM in the long run.
On the other hand this work might aid with that goal, so it might be
worth a try.
Second is that this might actually not work of hand. The problem is here:
> + /* TODO: This test used to be performed by drivers, but can
> + * this actually happen? If so, should we put the check into
> + * drm_gem_ttm_of_gem()? */
> + if (!drm_is_gem_ttm(bo))
> + return;
Yeah, this test is mandatory because TTM on itself can present buffer
object which doesn't belong to the driver called.
E.g. we sometimes have BOs which don't belong to the current drivers on
a driver specific LRU. A totally brain dead design if you ask me, but
that's how it is.
Not 100% sure, but by converting all drivers to use a common GEM_TTM
backend you might actually break that test.
I'm not sure if that is actually a problem in the real world, it most
likely isn't. But I still won't bet on it without being able to test this.
Regards,
Christian.
Am 08.04.19 um 11:21 schrieb Thomas Zimmermann:
> Several simple framebuffer drivers copy most of the TTM code from each
> other. The implementation is always the same; except for the name of
> some data structures.
>
> As recently discussed, this patch set provides generic TTM memory-
> management code for framebuffers with dedicated video memory. It further
> converts the respective drivers to the generic code. The shared code
> is basically the same implementation as the one copied among individual
> drivers.
>
> The patch set contains two major changes: first, it introduces
> |struct drm_gem_ttm_object| and helpers. It's a GEM object that is
> backed by TTM-managed memory. The type's purpose is somewhat similar
> to |struct drm_gem_{cma, shmem}_object|. Second, the patch set
> introduces |struct drm_simple_ttm| (for the lack of a better name) and
> helpers. It's an implementation of a basic TTM-based memory manager.
>
> Both, GEM TTM and Simple TTM, support VRAM and SYSTEM placements. Support
> for TT could probably be added if necessary. Both can be used independedly
> from each other if desired by the DRM driver.
>
> Currently ast, bochs, mgag200, vboxvideo and hisilicon/hibmc can use
> these helpers. Cirrus would also be a candidate, but as it's being
> rewrtten from scratch, I didn't bother doing the conversion.
>
> Future directions: with these changes, the respective drivers can also
> share some of their mode-setting or fbdev code. GEM TTM could implement
> PRIME helpers, which would allow for using the generic fbcon.
>
> The patch set is against a recent drm-tip.
>
> Thomas Zimmermann (15):
>drm: Add |struct drm_gem_ttm_object| and helpers
>drm: Add |struct drm_gem_ttm_object| callbacks for |struct
> ttm_bo_driver|
>drm: Add |struct drm_gem_ttm_object| callbacks for |struct drm_driver|
>drm: Add drm_gem_ttm_fill_create_dumb() to create dumb buffers
>drm: Add Simple TTM, a memory manager for dedicated VRAM
>drm/ast: Convert AST driver to |struct drm_gem_ttm_object|
>drm/ast: Convert AST driver to Simple TTM
>drm/bochs: Convert Bochs driver to |struct drm_gem_ttm_object|
>drm/bochs: Convert Bochs driver to Simple TTM
>drm/mgag200: Convert mgag200 driver to |struct drm_gem_ttm_object|
>drm/mgag200: Convert mgag200 driver to Simple TTM
>drm/vboxvideo: Convert vboxvideo driver to |struct drm_gem_ttm_object|
>drm/vboxvideo: Convert vboxvideo driver to Simple TTM
>drm/hisilicon: Convert hibmc-drm driver to |struct drm_gem_ttm_object|
>drm/hisilicon: Convert hibmc-drm driver to Simple TTM
>
> Documentation/gpu/drm-mm.rst | 23 +
> drivers/gpu/drm/Kconfig | 20 +
> drivers/gpu/drm/Makefile | 5 +
> drivers/gpu/drm/ast/Kconfig | 3 +-
> drivers/gpu/drm/ast/ast_drv.c | 4 +-
> drivers/gpu/drm/ast/ast_drv.h | 58 +-
> drivers/gpu/drm/ast/ast_fb.c | 18 +-
> drivers/gpu/drm/ast/ast_main.c| 74 +--
> drivers/gpu/drm/ast/ast_mode.c| 78 +--
> drivers/gpu/drm/ast/ast_ttm.c | 290 +-
> drivers/gpu/drm/bochs/Kconfig | 2 +
> drivers/gpu/drm/bochs/bochs.h | 42 +-
> drivers/gpu/drm/bochs/bochs_drv.c | 4 +-
> drivers/gpu/drm/bochs/bochs_kms.c | 18 +-
> drivers/gpu/drm/bochs/bochs_mm.c | 392 +-
> drivers/gpu/drm/drm_gem_ttm_helper.c | 507 ++
> drivers/gpu/drm/drm_simple_ttm_helper.c | 191 +++
> drivers/gpu/drm/drm_ttm_helper_common.c | 6 +
> drivers/gpu/drm/hisilicon/hibmc/Kconfig | 2 +
> .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c| 19 +-
> .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 4 +-
> .../gpu/drm/hisilicon/hibmc/hibmc_drm_dr
Re: [PATCH 0/5] Clean up TTM mmap offsets
Am 07.02.19 um 09:59 schrieb Thomas Zimmermann: > Almost all TTM-based drivers use the same values for the mmap-able > range of BO addresses. Each driver therefore duplicates the > DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not > configurable by drivers. > > This patch set replaces driver-specific configuration with a single > setup. All code is located within TTM. TTM and GEM share the same > range for mmap-able BOs. > > Thomas Zimmermann (5): >staging/vboxvideo: Use same BO mmap offset as other drivers >drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant >drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init() >drm/ttm: Quick-test mmap offset in ttm_bo_mmap() >drm: Use the same mmap-range offset and size for GEM and TTM Reviewed-by: Christian König for the whole series. Nice cleanup! Thanks, Christian. > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 ++-- > drivers/gpu/drm/ast/ast_drv.h | 2 -- > drivers/gpu/drm/ast/ast_ttm.c | 10 ++ > drivers/gpu/drm/bochs/bochs.h | 2 -- > drivers/gpu/drm/bochs/bochs_mm.c| 10 ++ > drivers/gpu/drm/cirrus/cirrus_drv.h | 1 - > drivers/gpu/drm/cirrus/cirrus_ttm.c | 10 ++ > drivers/gpu/drm/drm_gem.c | 17 - > drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 12 ++-- > drivers/gpu/drm/mgag200/mgag200_drv.h | 1 - > drivers/gpu/drm/mgag200/mgag200_ttm.c | 10 ++ > drivers/gpu/drm/nouveau/nouveau_drv.h | 2 -- > drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 > drivers/gpu/drm/qxl/qxl_drv.h | 3 --- > drivers/gpu/drm/qxl/qxl_ttm.c | 11 +++ > drivers/gpu/drm/radeon/radeon_ttm.c | 13 ++--- > drivers/gpu/drm/ttm/ttm_bo.c| 6 +++--- > drivers/gpu/drm/ttm/ttm_bo_vm.c | 3 +++ > drivers/gpu/drm/virtio/virtgpu_ttm.c| 4 +--- > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 - > drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 1 - > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c| 11 ++- > drivers/staging/vboxvideo/vbox_drv.h| 2 -- > drivers/staging/vboxvideo/vbox_ttm.c| 12 +++- > include/drm/drm_vma_manager.h | 12 > include/drm/ttm/ttm_bo_driver.h | 2 +- > 26 files changed, 42 insertions(+), 132 deletions(-) > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: virtio-gpu without ARCH_HAS_SG_CHAIN
Am 30.10.18 um 08:23 schrieb Gerd Hoffmann: > On Mon, Oct 29, 2018 at 12:46:34PM -0700, Michael Forney wrote: >> Hi, >> >> I was looking at adding virtio-gpu support to tinyemu >> (https://bellard.org/tinyemu/). I got it to work on x86, but just for >> fun I tried it under riscv and ran into an issue with buffer >> allocations (though, this should affect any arch without >> ARCH_HAS_SG_CHAIN). >> >> virtio-gpu uses ttm to allocate buffers, which swaps pages to ensure >> that they aren't consecutive[0][1]. > Interesting. > > While hacking the virtio-gpu ttm code I've already noticed that I get > non-contignous memory even for small allocations (cursor, which is only > 4 pages), but havn't found the time yet to look at this. > > Christian, care to explain the background? The commit message sounds a > bit like it papers over a bug somewhere else. The problem is that the TTM pool handler thinks that we need to free pages to the huge page pool when it sees that they are consecutive. The root problem of that in turn is that we can't use compound pages for device allocated memory, but a real fix for that would require quite some work in the MM. What we can do rather easily is to paper over the problem by only swapping page 511 and 510 to avoid that the pool things that this is a huge page. Regards, Christian. > >> However, this causes sg_alloc_table_from_pages to use a sg entry for >> every single page, limiting buffers to only 170 pages (the number of >> sg entries that can fit into a page). This is only 417x417 with 32bpp. >> I believe the page swapping also makes TRANSFER_TO_HOST_2D inefficient >> by forcing the host to do many memcpys instead of just a few. > Probably not *that* bad, the amount of data copyed doesn't change after > all. But, yes, I'd prefer to have shorter sh lists too. > > cheers, >Gerd > >> [0] >> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=fdb1a2236b07948e83e0a777e1795d4f07e52c33 >> [1] >> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=ae937fe19636067ec5e20d7f1fa10c6cc6000b52 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] drm/ttm: Rename ttm_bo_global_{init,release}() to ttm_bo_global_ref_{,}()
Am 16.10.2018 um 10:04 schrieb Thomas Zimmermann:
> The functions ttm_bo_global_init() and ttm_bo_global_release() do not
> receive an argument of type struct ttm_bo_global. Both take a struct
> drm_global_reference that contains points to a struct ttm_bo_global_ref.
> Renaming them reflects this.
>
> Signed-off-by: Thomas Zimmermann
Reviewed and pushed upstream.
Christian.
> ---
> Documentation/gpu/drm-mm.rst| 4 ++--
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 ++--
> drivers/gpu/drm/ast/ast_ttm.c | 4 ++--
> drivers/gpu/drm/bochs/bochs_mm.c| 4 ++--
> drivers/gpu/drm/cirrus/cirrus_ttm.c | 4 ++--
> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 4 ++--
> drivers/gpu/drm/mgag200/mgag200_ttm.c | 4 ++--
> drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 ++--
> drivers/gpu/drm/qxl/qxl_ttm.c | 4 ++--
> drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++--
> drivers/gpu/drm/ttm/ttm_bo.c| 8
> drivers/gpu/drm/virtio/virtgpu_ttm.c| 4 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c| 4 ++--
> drivers/staging/vboxvideo/vbox_ttm.c| 4 ++--
> include/drm/ttm/ttm_bo_driver.h | 4 ++--
> 15 files changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
> index e725e8449e72..d0f3c6b03200 100644
> --- a/Documentation/gpu/drm-mm.rst
> +++ b/Documentation/gpu/drm-mm.rst
> @@ -72,8 +72,8 @@ object TTM to provide a pool for buffer object allocation
> by clients and
> the kernel itself. The type of this object should be
> TTM_GLOBAL_TTM_BO, and its size should be sizeof(struct
> ttm_bo_global). Again, driver-specific init and release functions may
> -be provided, likely eventually calling ttm_bo_global_init() and
> -ttm_bo_global_release(), respectively. Also, like the previous
> +be provided, likely eventually calling ttm_bo_global_ref_init() and
> +ttm_bo_global_ref_release(), respectively. Also, like the previous
> object, ttm_global_item_ref() is used to create an initial reference
> count for the TTM, which will call your initialization function.
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index a44fc12ae1f9..3a6802846698 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -125,8 +125,8 @@ static int amdgpu_ttm_global_init(struct amdgpu_device
> *adev)
> global_ref = &adev->mman.bo_global_ref.ref;
> global_ref->global_type = DRM_GLOBAL_TTM_BO;
> global_ref->size = sizeof(struct ttm_bo_global);
> - global_ref->init = &ttm_bo_global_init;
> - global_ref->release = &ttm_bo_global_release;
> + global_ref->init = &ttm_bo_global_ref_init;
> + global_ref->release = &ttm_bo_global_ref_release;
> r = drm_global_item_ref(global_ref);
> if (r) {
> DRM_ERROR("Failed setting up TTM BO subsystem.\n");
> diff --git a/drivers/gpu/drm/ast/ast_ttm.c b/drivers/gpu/drm/ast/ast_ttm.c
> index fe354ebf374d..d21fbd26785a 100644
> --- a/drivers/gpu/drm/ast/ast_ttm.c
> +++ b/drivers/gpu/drm/ast/ast_ttm.c
> @@ -70,8 +70,8 @@ static int ast_ttm_global_init(struct ast_private *ast)
> global_ref = &ast->ttm.bo_global_ref.ref;
> global_ref->global_type = DRM_GLOBAL_TTM_BO;
> global_ref->size = sizeof(struct ttm_bo_global);
> - global_ref->init = &ttm_bo_global_init;
> - global_ref->release = &ttm_bo_global_release;
> + global_ref->init = &ttm_bo_global_ref_init;
> + global_ref->release = &ttm_bo_global_ref_release;
> r = drm_global_item_ref(global_ref);
> if (r != 0) {
> DRM_ERROR("Failed setting up TTM BO subsystem.\n");
> diff --git a/drivers/gpu/drm/bochs/bochs_mm.c
> b/drivers/gpu/drm/bochs/bochs_mm.c
> index e6ccf7fa92d4..ff4f41dec228 100644
> --- a/drivers/gpu/drm/bochs/bochs_mm.c
> +++ b/drivers/gpu/drm/bochs/bochs_mm.c
> @@ -48,8 +48,8 @@ static int bochs_ttm_global_init(struct bochs_device *bochs)
> global_ref = &bochs->ttm.bo_global_ref.ref;
> global_ref->global_type = DRM_GLOBAL_TTM_BO;
> global_ref->size = sizeof(struct ttm_bo_global);
> - global_ref->init = &ttm_bo_global_init;
> - global_ref->release = &ttm_bo_global_release;
> + global_ref->init = &ttm_bo_global_ref_init;
> + global_ref->release = &ttm_bo_global_ref_release;
> r = drm_global_item_ref(global_ref);
> if (r != 0) {
> DRM_ERROR("Failed setting up TTM BO subsystem.\n");
> diff --git a/drivers/gpu/drm/cirrus/cirrus_ttm.c
> b/drivers/gpu/drm/cirrus/cirrus_ttm.c
> index f21953243790..2e2141f26c5b 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_ttm.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_ttm.c
> @@ -70,8 +70,8 @@ static int cirrus_ttm_global_init(struct cirrus_device
> *cirrus)
> global_ref = &cirrus->ttm.bo_global_ref.ref;
> global_ref->global_
