> 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@xxxxxxx>
> >>>> ---
> >>>> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
