[PATCH v4 01/19] drm: Add |struct drm_gem_vram_object| and helpers

[Thomas Zimmermann]

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: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190506/0de200aa/attachment.sig>