Re: [PATCH 1/4] drm: add prime helpers

[Aaron Plattner <aplattner@nvidia.com>]

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