Re: [PATCH 0/5] drm/vmwgfx: Support module unload and hotunplug

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 12, 2021 at 05:34:50PM +0000, Zack Rusin wrote:
> On Tue, 2021-10-12 at 11:10 +0200, Thomas Hellström wrote:
> > On Tue, 2021-10-12 at 10:27 +0200, Christian König wrote:
> > > Am 11.10.21 um 14:04 schrieb Thomas Hellström:
> > > 
> > > > > 
> > 
> > > > So now if this is going to be changed, I think we need to
> > > > understand
> > > > why and think this through really thoroughly:
> > > > 
> > > > * What is not working and why (the teardown seems to be a trivial
> > > > fix).
> > > > * How did we end up here,
> > > > * What's the cost of fixing that up compared to refactoring the
> > > > drivers
> > > > that rely on bindable system memory,
> > > > * What's the justification of a system type at all if it's not
> > > > GPU-
> > > > bindable, meaning it's basically equivalent to swapped-out shmem
> > > > with
> > > > the exception that it's mappable?
> > > 
> > > Well, once more that isn't correct. This is nothing new and as far
> > > as
> > > I 
> > > know that behavior existing as long as TTM existed.
> > 
> > I'm not sure whats incorrect? I'm trying to explain what the initial
> > design was, and it may of course have been bad and the one you
> > propose
> > a better one and if required we certainly need to fix i915 to align
> > with a new one.
> > 
> > What worries me though, that if you perceive the design differently
> > and
> > change things in TTM according to that perception that breaks drivers
> > that rely on the initial design and then force drivers to change
> > claiming they are incorrect without a thorough discussion on dri-
> > devel,
> > that's IMHO not good.
> 
> We should probably do that in a seperate thread so that this,
> fundametally important, discussion is easier to find and reference in
> the future. It looks like we're settling on a decision here so I'd
> appreciate an Acked-by for the patch 4/5 just so it doesn't look like I
> was making things up to someone looking at git history in the future.

Jumping in sidesways and late, and also without real context on the
decision itself:

The way to properly formalize this is
- type a kerneldoc patch which writes down the rules we agree on, whether
  that's uapi, or internal helper api like for ttm, or on types or
  whatever
- get acks from everyone who participated + everyone who might care
- merge it

> It seems that in general TTM was designed to be able to handle an
> amazing number of special/corner cases at a cost of complexity which
> meant that over the years very few people understood it and the code
> handling those cases sometimes broke. It sounds like Christian is now
> trying to reign it in and make the code a lot more focused.
> 
> Working on other OS'es for the last few years, certainly made me
> appreciate simple frameworks that move complexity towards drivers that
> actually need them, e.g. it's of course anecdotal but I found wddm gpu
> virtual addressing models (iommu/gpummu) a lot easier to grok.
> 
> On the flip side that does mean that vmwgfx and i915 need to redo some
> code. For vmwgfx it's probably a net positive anyway as we've been
> using TTM for, what is really nowadays, an integrated GPU so maybe it's
> time for us to think about transition to gem.

Aside, but we're looking at adopting ttm for integrated gpu too. The
execbuf utils and dynamic memory management helpers for pure gem just
aren't quite there yet, and improving ttm a bit in this area looks
reasonable (like adding a unified memory aware shrinker like we have in
i915-gem).

Also I thought vmwgfx is using ttm to also manage some id spaces, you'd
have to hand-roll that.

Anyway entirely orthogonal.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch