[Intel-gfx] [PATCH 18/19] drm/i915/gtt: map the PD up front

[Matthew Auld]

On Mon, 12 Apr 2021 at 18:01, Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Mon, Apr 12, 2021 at 6:08 PM Matthew Auld
> <matthew.william.auld at gmail.com> wrote:
> >
> > On Mon, 12 Apr 2021 at 16:17, Daniel Vetter <daniel at ffwll.ch> wrote:
> > >
> > > On Mon, Apr 12, 2021 at 10:05:25AM +0100, Matthew Auld wrote:
> > > > We need to general our accessor for the page directories and tables from
> > > > using the simple kmap_atomic to support local memory, and this setup
> > > > must be done on acquisition of the backing storage prior to entering
> > > > fence execution contexts. Here we replace the kmap with the object
> > > > maping code that for simple single page shmemfs object will return a
> > > > plain kmap, that is then kept for the lifetime of the page directory.
> > > >
> > > > v2: (Thomas) Rebase on dma_resv and obj->mm.lock removal.
> > > >
> > > > Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > >
> > > So I wanted to understand what px stands for as an abbreviation, and dug
> > > all the way down to this:
> > >
> > > commit 567047be2a7ede082d29f45524c287b87bd75e53
> > > Author: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > > Date:   Thu Jun 25 18:35:12 2015 +0300
> > >
> > >     drm/i915/gtt: Use macros to access dma mapped pages
> > >
> > > I still have no idea what it means, I guess px = page. But I also
> > > committed this, so I guess can blame myself :-)
> > >
> > > But while digging I've stumbled over this here
> > >
> > > commit 6eebfe8a10a62139d681e2f1af1386252742278b
> > > Author: Chris Wilson <chris at chris-wilson.co.uk>
> > > Date:   Fri Jul 12 08:58:18 2019 +0100
> > >
> > >     drm/i915/gtt: Use shallow dma pages for scratch
> > >
> > >
> > > And that's some serious wtf. Yes we've done some compile-time type
> > > casting automagic between i915_priv and dev in the past, and I think even
> > > that was bad taste. But it was justified with that we have these
> > > everywhere (especially in the mmio macros), and it would be a terrible
> > > flag day.
> > >
> > > But I'm not seeing any need for auto-casting for these pages here, and I'm
> > > not aware that we're doing this anywhere else in kernel code. There is
> > > some macro-trickery in lockdep annotations, but that relies on the lockdep
> > > map having the same struct member name in all lock types, and is not
> > > exposed to drivers at all.
> > >
> > > Am I missing something, or why do we have this compile-time type casting
> > > stuff going on in i915 page accessors?
> >
> > I think 'x' in the px family of macros/functions is meant in the
> > variable/polymorphic sense, so it can potentially be a pt, pd, etc
> > underneath. If you look at px_base() for example all it does is fish
> > out the base GEM object from the structure, using the
> > known-at-compile-time-type, which then lets us get at the dma address,
> > vaddr etc.
>
> Yeah, but that's not how things landed. px predates the magic
> polymorphism. I think the px just stands for page, or at least
> originally only stood for page. I'm not sure honestly. It seems to be
> just used for page directory type of things, but I haven't found that
> written down anywhere.
>
> > It does seem pretty magical, but seems ok to me, if it means less typing?
>
> That's the worst justification. Code is generally write once, read
> many times. Optimizing for writing at the cost of magic indirection is
> generally not the right tradeoff in the kernel, where any indirection
> could hide a major gotcha. In huge userspace applications fancy
> abstraction and polymorphism is often the right thing to do, but there
> you also have a real compiler with a real typesystem (generally at
> least) helping you out. Or it's yolo duct-taping with lots of tests,
> where the speed at which you can hack up something matters more than
> being able to read it quickly.
>
> We're typing C here. It is generally rather verbose, with type casting
> all done explicitly.

Ok. So should we change this around for this patch? The px_ stuff is
already quite prevalent it seems, and the px_vaddr() is just one part
of it? Maybe just add pt_vaddr(), pd_vaddr() etc instead?

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch