[PATCH v2 00/18] drm/ttm: make ttm bo a gem bo subclass

[Christian König]

Am 24.06.19 um 08:32 schrieb Gerd Hoffmann:
>    Hi,
>
>> Yeah, my point was not really suggesting that we do this, but rather that
>> people would rightfully get upset because the struct contains unused stuff.
> Well, struct drm_gem_object isn't that big, lets have a look:
>
> 320 bytes in total, of which are:
>    184 bytes the embedded vma_mode
>     64 bytes the embedded _resv struct
>      8 bytes the resv pointer.
>     64 bytes everything else.
>
> So, after applying this series it's 64 bytes more per bo for vmwgfx,
> due to some unused fields being added.
>
> And it's 256 bytes less per bo for all ttm+gem drivers, because they
> don't have vma_mode + resv struct + resv pointer twice any more.
>
> And that is just the low-hanging fruit, there is room for more
> ttm_buffer_object elements being removed by using the drm_gem_object
> struct elements instead.  num_pages, kref, maybe more.

Yeah, and that is exactly the reason why I'm strongly in favor of this 
approach.

GEM is our de-facto standard for buffer UAPI in DRM. AFAIK vmgfx is one 
of the few drivers not using it and as you wrote as well it might 
actually be a good idea to change that.

The only thing I can of hand see which is misplaced in the 
drm_gem_object structure is "struct file *filp;", cause that is specific 
to a backend implementation.

Regards,
Christian.

>
>> Also a trap we might end up with in the future is that with the design
>> suggested in this patch series is that people start assuming that the
>> embedded gem object is actually initialized and working, which could lead to
>> pretty severe problems for vmwgfx...
> I guess I should reword patch #1 then, making clear that the
> ttm_bo_uses_embedded_gem_object() helper function is going to stay.
>
> What is the reason for vmwgfx to not use gem btw?
>
>>> for all the ttm+gem drivers, one pointer they don't need twice). With
>>> the side-by-side, which is the design all gem+ttm drivers used the
>>> past few years, we still need that duplication. Same for the vma node
>>> thing, which is also duplicated.
>> To bemore precise I'd probably define a
>>
>> struct drm_bo_common {
>>      struct reservation_object r;
>>      struct drm_vma_node v;
>> };
>>
>> Embed it in a struct drm_gem_object (and in a struct vmwgfx_buffer_object)
>> and then have a pointer to a struct drm_bo_common in the struct
>> ttm_buffer_object.
> A pointer doesn't cut it.
>
> Beside removing the duplication the other thing I want is to have a
> standard way of finding the ttm_buffer_object for a given drm_gem_object
> for all the ttm+gem drivers.  With struct drm_gem_object being embedded
> the usual containter_of() will work.
>
> That'll allow to create drm_gem_ttm_helper.c with helper functions for
> struct drm_gem_object_funcs.  For starters some of the current vram
> helpers can become generic ttm helpers because they loose the dependency
> on struct drm_gem_vram_object for finding ttm_buffer_object.
>
>>>> The vmwgfx driver is doing what it does mostly because all buffer
>>>> objects do not need to be user-space visible, and do not need to be
>>>> mapped by user-space. And there are other types of objects that DO need
>>>> to be user-space visible, and that do need to be shared by processes.
>>>> Hence user-space visibility is something that should be abstracted and
>>>> made available to those objects. Not lumped together with all other
>>>> potential buffer object functionality.
> Well, ttm_buffer_object has a vma_node embedded, so it already is
> "lumped together".  This patch series only moves it around ...
>
> cheers,
>    Gerd
>