On Mon, 12 Apr 2021 at 18:01, Daniel Vetter <daniel@xxxxxxxx> wrote: > > On Mon, Apr 12, 2021 at 6:08 PM Matthew Auld > <matthew.william.auld@xxxxxxxxx> wrote: > > > > On Mon, 12 Apr 2021 at 16:17, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxx> > > > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > > > > > > 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@xxxxxxxxxxxxxxx> > > > 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@xxxxxxxxxxxxxxxxxx> > > > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
