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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
