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

[Daniel Vetter]

On Tue, Apr 13, 2021 at 11:29 AM Matthew Auld
<matthew.william.auld at gmail.com> wrote:
>
> 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?

Nah, that was just an orthogonal observation. The confusion with magic
type-aware macros is preexisting and widespread, there's no point
holding up dg1 code with that. But it is maybe something we should put
on our cleanup list. Or at least have a better explanation for why
exactly it is needed. Also note I'm not worried about the px stuff
standing for pt/pd/whatever, it's the magic type casting property of
these macros added with the 2nd patch I've mentioned above that looks
rather questionable to me. Maybe as transition thing like we've done
with i915_priv pointers, but not something that we should build on top
for long term.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch