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 <tzimmermann@xxxxxxx>
>>> ---
>>> 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_SYMBOL(drm_gem_vram_unpin_reserved);
>>> +
>>> /**
>>> * drm_gem_vram_push_to_system() - \
>>> Unpins a GEM VRAM object and moves it to system memory
>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
>>> index 6c1a9d724d85..1c4fc85315a0 100644
>>> --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
>>> +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
>>> @@ -23,9 +23,9 @@ static void mga_hide_cursor(struct mga_device *mdev)
>>> WREG8(MGA_CURPOSXL, 0);
>>> WREG8(MGA_CURPOSXH, 0);
>>> if (mdev->cursor.pixels_1->pin_count)
>>> - drm_gem_vram_unpin(mdev->cursor.pixels_1);
>>> + drm_gem_vram_unpin_reserved(mdev->cursor.pixels_1);
>>> if (mdev->cursor.pixels_2->pin_count)
>>> - drm_gem_vram_unpin(mdev->cursor.pixels_2);
>>> + drm_gem_vram_unpin_reserved(mdev->cursor.pixels_2);
>>> }
>>>
>>> int mga_crtc_cursor_set(struct drm_crtc *crtc,
>>> @@ -96,26 +96,28 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
>>>
>>> /* Move cursor buffers into VRAM if they aren't already */
>>> if (!pixels_1->pin_count) {
>>> - ret = drm_gem_vram_pin(pixels_1, DRM_GEM_VRAM_PL_FLAG_VRAM);
>>> + ret = drm_gem_vram_pin_reserved(pixels_1,
>>> + DRM_GEM_VRAM_PL_FLAG_VRAM);
>>> if (ret)
>>> goto out1;
>>> gpu_addr = drm_gem_vram_offset(pixels_1);
>>> if (gpu_addr < 0) {
>>> - drm_gem_vram_unpin(pixels_1);
>>> + drm_gem_vram_unpin_reserved(pixels_1);
>>> goto out1;
>>> }
>>> mdev->cursor.pixels_1_gpu_addr = gpu_addr;
>>> }
>>> if (!pixels_2->pin_count) {
>>> - ret = drm_gem_vram_pin(pixels_2, DRM_GEM_VRAM_PL_FLAG_VRAM);
>>> + ret = drm_gem_vram_pin_reserved(pixels_2,
>>> + DRM_GEM_VRAM_PL_FLAG_VRAM);
>>> if (ret) {
>>> - drm_gem_vram_unpin(pixels_1);
>>> + drm_gem_vram_unpin_reserved(pixels_1);
>>> goto out1;
>>> }
>>> gpu_addr = drm_gem_vram_offset(pixels_2);
>>> if (gpu_addr < 0) {
>>> - drm_gem_vram_unpin(pixels_1);
>>> - drm_gem_vram_unpin(pixels_2);
>>> + drm_gem_vram_unpin_reserved(pixels_1);
>>> + drm_gem_vram_unpin_reserved(pixels_2);
>>> goto out1;
>>> }
>>> mdev->cursor.pixels_2_gpu_addr = gpu_addr;
>>> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
>>> index b056f189ba62..ff1a81761543 100644
>>> --- a/include/drm/drm_gem_vram_helper.h
>>> +++ b/include/drm/drm_gem_vram_helper.h
>>> @@ -82,7 +82,10 @@ void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo);
>>> u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo);
>>> s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo);
>>> int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag);
>>> +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo,
>>> + unsigned long pl_flag);
>>> int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo);
>>> +int drm_gem_vram_unpin_reserved(struct drm_gem_vram_object *gbo);
>>> int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo);
>>> void *drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
>>> bool *is_iomem, struct ttm_bo_kmap_obj *kmap);
>>> --
>>> 2.21.0
>>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
