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

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

Op 06-12-12 22:50, Daniel Vetter schreef:
> On Thu, Dec 06, 2012 at 10:46:30PM +0100, 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.
>>
>> - 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
> One more: For the cpu access noop helpers I'd vote for -ENOTTY as the more
> canonical "not implemented error, you're talking to the wrong thing" error
> instead of -EINVAL, which an exporter could throw back to the importer if
> e.g. the range is outside of the size of the dma_buf. With a quick dma_buf
> doc update we could then bless this as the official way to denounce cpu
> access support.
>
Yeah lets reinvent a new error to return, and make it not a typewriter to confuse users,
instead of using -ENODEV which would actually be valid and most descriptive here if you
read the mmap page:

-ENODEV The underlying file system of the specified file does not support memory mapping.

~Maarten