Linux Virtualization - May 2019 - [PATCH v4 01/19] drm: Add |struct drm_gem_vram_object| and helpers

> +/**
> + * drm_gem_vram_unpin() - Unpins a GEM VRAM object
> + * @gbo:	the GEM VRAM object
> + *
> + * Returns:
> + * 0 on success, or
> + * a negative error code otherwise.
> + */
> +int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
> +{
> +	int i, ret;
> +	struct ttm_operation_ctx ctx = { false, false };
> +
> +	if (!gbo->pin_count)
> +		return 0;
WARN_ON_ONCE() here?  That should not happen ...
> +/**
> + * drm_gem_vram_push_to_system() - \
> +	Unpins a GEM VRAM object and moves it to system memory
> + * @gbo:	the GEM VRAM object
> + *
> + * This operation only works if the caller holds the final pin on the
> + * buffer object.
> + *
> + * Returns:
> + * 0 on success, or
> + * a negative error code otherwise.
> + */
> +int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo)
> +{
> +	int i, ret;
> +	struct ttm_operation_ctx ctx = { false, false };
> +
> +	if (!gbo->pin_count)
> +		return 0;
Likewise.
> +	--gbo->pin_count;
> +	if (gbo->pin_count)
> +		return 0;
> +
> +	if (gbo->kmap.virtual)
> +		ttm_bo_kunmap(&gbo->kmap);
> +
> +	drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM);
> +	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)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_gem_vram_push_to_system);
Very simliar to drm_gem_vram_unpin, can't we just call that function?

Something like this:

	drm_gem_vram_push_to_system()
	{
		if (gbo->pin_count == 1 && gbo->kmap.virtual)
			ttm_bo_kunmap(&gbo->kmap);
		return drm_gem_vram_unpin();
	}
> +struct drm_gem_vram_object {
> +	/* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
> +	struct ttm_placement placement;
> +	struct ttm_place placements[3];
placements[2] should be enough I guess?

cheers,
  Gerd

Hi

thanks for reviewing the patches.

Am 06.05.19 um 14:31 schrieb Gerd Hoffmann:
>> +	--gbo->pin_count;
>> +	if (gbo->pin_count)
>> +		return 0;
>> +
>> +	if (gbo->kmap.virtual)
>> +		ttm_bo_kunmap(&gbo->kmap);
>> +
>> +	drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM);
>> +	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)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_gem_vram_push_to_system);
> 
> Very simliar to drm_gem_vram_unpin, can't we just call that function?
> 
> Something like this:
> 
> 	drm_gem_vram_push_to_system()
> 	{
> 		if (gbo->pin_count == 1 && gbo->kmap.virtual)
> 			ttm_bo_kunmap(&gbo->kmap);
> 		return drm_gem_vram_unpin();
> 	}
This misses the call to drm_gem_vram_placement(), where
drm_gem_vram_push_to_system() enforces placement in system memory. We
could build a common implementation out of both interfaces, but that
would obfuscate the code IMHO. I'd just leave it as it is.

The function is only used by ast and mgag200 framebuffer handling. It's
actually something that should be done by VRAM MM, but both drivers are
non-atomic and could require an overhaul anyway.
>> +struct drm_gem_vram_object {
>> +	/* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
>> +	struct ttm_placement placement;
>> +	struct ttm_place placements[3];
> 
> placements[2] should be enough I guess?
TTM_PL_VRAM has index 2 and TTM_PL_SYSTEM has index 0. There's TTM_PL_TT
at index 1. We don't use all three array entries here, but I'm not sure
if something in TTM does. I took the line from the drivers and didn't
change it for that reason.

Best regards
Thomas
> cheers,
>   Gerd
> 
-- 
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/20190506/0de200aa/attachment.sig>

Hi,
> This misses the call to drm_gem_vram_placement(), where
> drm_gem_vram_push_to_system() enforces placement in system memory.
Ah, missed that detail.
> We
> could build a common implementation out of both interfaces, but that
> would obfuscate the code IMHO. I'd just leave it as it is.
Ok.
> >> +struct drm_gem_vram_object {
> >> +	/* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
> >> +	struct ttm_placement placement;
> >> +	struct ttm_place placements[3];
> > 
> > placements[2] should be enough I guess?
> 
> TTM_PL_VRAM has index 2 and TTM_PL_SYSTEM has index 0. There's
TTM_PL_TT
> at index 1. We don't use all three array entries here, but I'm not
sure
> if something in TTM does. I took the line from the drivers and didn't
> change it for that reason.
TTM_PL_* isn't an index into that array.  See drm_gem_vram_placement()
which fills that array.  It'll use one or two entries of that array.

cheers,
  Gerd