Re: [PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200

[Daniel Vetter <daniel@ffwll.ch>]

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