thr3ads.net - Linux Virtualization - [PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200 [May 2019]

If this information is useful, please help other people find it:
Share via:

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

head link

[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

head link

[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

head link

[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

head link

[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

head link

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

Maybe Matching Threads

Search for more maybe matching threads

Linux Virtualization - May 2019 - [PATCH 1/2] drm: Add drm_gem_vram_{pin/unpin}_reserved() and convert mgag200

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

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

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

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

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

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

Maybe Matching Threads