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@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