On 12/06/2012 01:46 PM, Daniel Vetter wrote: > On Thu, Dec 06, 2012 at 10:07:48AM -0800, Aaron Plattner wrote: >> Instead of reimplementing all of the dma_buf functionality in every driver, >> create helpers drm_prime_import and drm_prime_export that implement them in >> terms of new, lower-level hook functions: >> >> gem_prime_pin: callback when a buffer is created, used to pin buffers >> into GTT >> gem_prime_get_pages: convert a drm_gem_object to an sg_table for export >> gem_prime_import_sg: convert an sg_table into a drm_gem_object >> gem_prime_vmap, gem_prime_vunmap: map and unmap an object >> >> These hooks are optional; drivers can opt in by using drm_gem_prime_import >> and >> drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of >> struct drm_driver. >> >> Signed-off-by: Aaron Plattner <aplattner at nvidia.com> > > A few comments: > > - Can you please add kerneldoc entries for these new helpers? Laurent > Pinchart started a nice drm kerneldoc as an overview. And since then > we've at least integrated the kerneldoc api reference at a nice place > there and brushed it up a bit when touching drm core or helpers in a > bigger patch series. See e.g. my recent dp helper series for what I'm > looking for. I think we should have kerneldoc at least for the exported > functions.
Good call, I'll look into that. > - Just an idea for all the essential noop cpu helpers: Maybe just call > them dma_buf*noop and shovel them as convenience helpers into the > dma-buf.c code in the core, for drivers that don't want/can't/won't > bother to implement the cpu access support. Related: Exporting the > functions in general so that drivers could pick&choose Seems like a good idea as a follow-on change. Do you think it would make more sense to have noop helpers, or allow them to be NULL in dma-buf.c? > - s/gem_prime_get_pages/gem_prime_get_sg/ drm/i915 now uses sg_tables > everywhere to manage backing storage, and we're starting to use the > stolen memory, too. Stolen memory is not struct page backed, so better > make this clear in the function calls. I know that the current > prime/dma_buf code from Dave doesn't really work too well (*cough*) if > the backing storage isn't struct page backed, at least on the ttm import > side. This also links in with my 2nd comment above - dma_buf requires > that exporter map the sg into the importers device space to support fake > subdevices which share the exporters iommu/gart, hence such incetious > exporter might want to overwrite some of the functions. Will do in a v2 set. > - To make it a first-class helper and remove any and all midlayer stints > from this (I truly loath midlayers ...) maybe shovel this into a > drm_prime_helper.c file. Also, adding functions to the core vtable for > pure helpers is usually not too neat, since it makes it way too damn > easy to mix things up and transform the helper into a midlayer by > accident. E.g. the crtc helper functions all just use a generic void * > pointer for their vtables, other helpers either subclass an existing > struct or use their completely independent structs (which the driver can > both embed into it's own object struct and then do the right upclassing > in the callbacks). tbh haven't looked to closely at the series to asses > what would make sense, but we have way too much gunk in the drm vtable > already ... I'm not sure I fully understand what you mean by this. Are you suggesting creating a separate struct for these functions and having a void* pointer to that in struct drm_driver? > Dunno whether this aligns with what you want to achieve here. But on a > quick hunch this code feels rather unflexible and just refactors the > identical copypasta code back into one copy. Anyway, just figured I'll > drop my 2 cents here, feel free to ignore (maybe safe for the last one). I think uncopypasta-ing the current code is beneficial on its own. Do you think doing this now would make it harder to do the further refactoring you suggest later? -- Aaron
