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

[Daniel Vetter]

Hi Thomas,

On Sat, Jun 22, 2019 at 12:52 AM Thomas Hellstrom <thomas at shipmail.org> wrote:
> On 6/21/19 5:57 PM, Daniel Vetter wrote:
> > On Fri, Jun 21, 2019 at 05:12:19PM +0200, Thomas Hellström (VMware) wrote:
> >>
> >> On 6/21/19 1:57 PM, Gerd Hoffmann wrote:
> >>
> >> Aargh. Please don't do this. Multiple reasons:
> >>
> >> 1) I think It's bad to dump all buffer object functionality we can possibly
> >> think of in a single struct and force that on all (well at least most)
> >> users. It's better to isolate functionality in structs, have utility
> >> functions for those and let the drivers derive their buffer objects from
> >> whatever functionality they actually need.
> >> 2) vmwgfx is not using gem and we don't want to carry that extra payload in
> >> the buffer object.
> >> 3) TTM historically hasn't been using the various drm layers except for
> >> later when common helpers have been used, (like the vma manager and the
> >> cache utilities). It's desirable to keep that layer distinction. (which is
> >> really what I'm saying in 1.)
> >>
> >> Now if more and more functionality that originated in TTM is moving into GEM
> >> we need to find a better way to do that without duplicating functionality. I
> >> suggest adding pointers in the TTM structs and defaulting those pointers to
> >> the member in the TTM struct. Optionally to to the member in the GEM struct.
> >> If we need to migrate those members out of the TTM struct, vmwgfx would have
> >> to provide them in its own buffer class.
> >>
> >> NAK from the vmwgfx side.
> > It's 59 DRIVER_GEM vs 1 which is not. I think the verdict is clear what
> > the reasonable thing to do is here, and this will allow us to
> > substantially improve code and concept sharing across drm drivers.
> >
> > 10 years ago it was indeed not clear whether everyone doing the same is a
> > bright idea, but that's no more. If you want I guess you can keep a
> > private copy of ttm in vmwgfx, but not sure that's really worth it
> > long-term.
> > -Daniel
>
> It's not a question about whether GEM or TTM, or even the number of
> drivers using one or the other. (GEM would actually be a good choice for
> the latter vmwgfx device versions). But this is going against all recent
> effort to make different parts of drm functionality recently self-contained.
>
> Just stop and think for a while what would happen if someone would
> suggest doing the opposite: making a gem object a derived class of a TTM
> object, arguing that a lot of GEM drivers are using TTM as a backend.
> There would probably be a lot of people claiming "we don't want to
> unnecesarily carry that stuff". That's because that would also be a poor
> design.

That case is a bit a different case. We have
- 5 ttm+gem drivers, recently refactored into vram helpers (but still
ttm underneath)
- 5 ttm+gem drivers, using ttm directly
- 1 ttm driver, no gem
- 48 other gem drivers with no vram support
- 1 gem driver which will gain vram support shortly, with or without
ttm still not clear

11:48 is not even close to 59:1 imo. And I think even if Thomas
Zimmermann and others get really busy porting old discrete fbdev
drivers to kms, that ratio won't change much since we're also gaining
new soc drm drivers at a steady rate.

Also I wouldn't mind if we e.g. stuff a struct list_head lru; into
drm_gem_buffer_object, that's probably useful for many cases (not the
pure display drivers, but they tend to have so few bo it really wont
matter even if we add a few kb of cruft).

> What I'm suggesting is, build that improved code and concept sharing around
>
> struct gem_ttm_object {
>     struct gem_object;
>     struct ttm_object;
> };

I guess technically this would work too. Bit more churn (maybe
substantially more, I haven't looked tbh) to roll this out for all the
ttm drivers using gem.

> And lets work toghether to eliminate what's duplicated.

How would you share the bo.resv pointer with the above design? With
embedding ttm can use the gem one, and we drop a bunch of code (and
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. And that's just the things Gerd did
in this initial patch series. I think Christian König has some ideas
around ttm_bo_(un)reserve, and I just sent out a patch series to
streamline how we handle the reservation_object pointer for prime
import/export.

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

I'd still go with pragmatic choice of "this here is typed & reviewed,
and a bunch of people are enthusastistic about using this to drive
further refactoring in this area". Whatever the reasons, but since a
few months the ttm refactor train seems to have gained massive
momentum.

So if you want to change the direction of this train, I think you need
to get typing and show that your solution is at least as effective at
faciliting all the things people want to do with code sharing across
drm drivers.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch