Daniel Vetter
2019-May-20 16:19 UTC
[PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200
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 at suse.de> > --- > 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. 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
2019-May-20 16:26 UTC
[PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200
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 at suse.de> > > --- > > 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
Koenig, Christian
2019-May-20 19:24 UTC
[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 <tzimmermann at suse.de> >>> --- >>> 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
Gerd Hoffmann
2019-May-21 10:35 UTC
[PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200
Hi,> 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.Given that mgag200 is the only user I think the best way forward is to improve the mgag200 cursor handling so we can just drop the _reserved variants ... When looking at mga_crtc_cursor_set() I suspect the easierst way to do that would be to simply pin the cursor bo's at driver_load time, then we don't have to bother with pinning in mga_crtc_cursor_set() at all. Thomas, as you have test hardware, can you look into this? thanks, Gerd
Thomas Zimmermann
2019-May-21 10:57 UTC
[PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200
Hi Am 21.05.19 um 12:35 schrieb Gerd Hoffmann:> Hi, > >> 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. > > Given that mgag200 is the only user I think the best way forward is to > improve the mgag200 cursor handling so we can just drop the _reserved > variants ... > > When looking at mga_crtc_cursor_set() I suspect the easierst way to do > that would be to simply pin the cursor bo's at driver_load time, then we > don't have to bother with pinning in mga_crtc_cursor_set() at all. > > Thomas, as you have test hardware, can you look into this?Sure, that's the plan. May take a few days, though. I'd like to see all of the locking/reservation gone from the helper API. By using bochs' PRIME helpers, the generic console could probably be used in mgag200 and ast. That would remove the remaining calls from mgag200 and ast. Best regards Thomas> thanks, > Gerd > > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >-- 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) -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: OpenPGP digital signature URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20190521/495679f2/attachment-0001.sig>
Thomas Zimmermann
2019-May-21 11:35 UTC
[PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200
Hi Am 21.05.19 um 12:35 schrieb Gerd Hoffmann:> Hi, > >> 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. > > Given that mgag200 is the only user I think the best way forward is to > improve the mgag200 cursor handling so we can just drop the _reserved > variants ... > > When looking at mga_crtc_cursor_set() I suspect the easierst way to do > that would be to simply pin the cursor bo's at driver_load time, then we > don't have to bother with pinning in mga_crtc_cursor_set() at all.I've been thinking about converting ast and mgag200 to atomic mode setting. For that, universal planes would be required. But this seems incompatible with HW cursors. With universal planes, cursor planes would probably have to be exported as RGBA8 framebuffer, but the HW uses MMIO region and 16-color palette. So there's got to be a translation step and a way of notifying userspace if the palette overflows. In the current driver code, drm_crtc_funcs.set_cursor() does this. How is this done with universal planes? I know that there's drm_framebuffer_funcs.dirty(), but it doesn't seem to be used much. Best regards Thomas> Thomas, as you have test hardware, can you look into this? > > thanks, > Gerd > > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >-- 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) -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: OpenPGP digital signature URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20190521/8ffb0c3e/attachment.sig>