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