[Mesa-dev] [RFC PATCH] GBM: Add map/unmap functions

[Rob Clark]

On Mon, Apr 11, 2016 at 10:09 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> Hi Rob,
>
> On 31 March 2016 at 04:21, Rob Herring <robh at kernel.org> wrote:
>> This adds GBM map and unmap functions. Unlike prior attempts, this
>> version provides a region and usage flags for the mapping. The operation
>> follows gallium transfer_map() function. This is complete enough to work
>> on Android with gralloc using the GBM API.
>>
>> TODO:
>> - split up patches
>> - define and propagate GBM usage flags
>> - DRIImage extension version checking
>> - Need to flush context on unmap?
>> - Need fences for gralloc lockAsync
>> - integrate dumb buffer mapping
>> - bikeshed the name (map or lock)
>>
> With the risk of starting a war - wasn't there objections about having
> such interfaces in GBM a while back ?
> Personally I would CC the person so that he can clarify if current
> version is in better shape. Hopefully he'll bring forward what needs
> fixing (if any) to get this kind of interface in acceptable shape.
>
> Note: not sure who was against the previous patches so I haven't CC'd him.

iirc, it was Thomas (who I've added in CC)..  and the issue was more
about how the previous implementation worked (ie. returning mmap
pointer to entire buffer).  Rob's implementation fits on top of
pipe_transfer and should address Thomas's concerns.


>> Signed-off-by: Rob Herring <robh at kernel.org>
>> ---
>> This reflects the discussion on irc today. I got further than I expected,
>> and it is basically working with Android. The goal here is to use GBM
>> for Android gralloc instead of per driver and pipe implementations. I'm
>> looking for some early feedback on the GBM interface and the extending
>> of the DRIimage extension.
>>
>> Rob
>>
>>
>>  include/GL/internal/dri_interface.h         | 30 +++++++++++++++++++++++
>>  src/gallium/state_trackers/dri/dri2.c       | 38 +++++++++++++++++++++++++++++
>>  src/gallium/state_trackers/dri/dri_screen.h |  1 +
>>  src/gbm/backends/dri/gbm_dri.c              | 30 +++++++++++++++++++++++
>>  src/gbm/backends/dri/gbm_driint.h           |  1 +
>>  src/gbm/gbm-symbols-check                   |  2 ++
>>  src/gbm/main/gbm.c                          | 20 +++++++++++++++
>>  src/gbm/main/gbm.h                          |  9 +++++++
>>  src/gbm/main/gbmint.h                       |  5 ++++
>>  9 files changed, 136 insertions(+)
>>
>> diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
>> index 6bbd3fa..b059112 100644
>> --- a/include/GL/internal/dri_interface.h
>> +++ b/include/GL/internal/dri_interface.h
>
>> @@ -1350,6 +1353,33 @@ struct __DRIimageExtensionRec {
>
>> +   /**
>> +    * Unlock a __DRIimage for specified usage
>> +    *
>> +    * flush_flag:
>> +    *    0:                  no flush
>> +    *    __BLIT_FLAG_FLUSH:  flush after the blit operation
>> +    *    __BLIT_FLAG_FINISH: flush and wait the blit finished
>> +    *
> There are no flags here.
>
>> +    * \since 11
>> +    */
>> +   int (*unlockImage)(__DRIcontext *context, __DRIimage *image);
>> +
>>  };
>>
>>
>
>> --- a/src/gbm/backends/dri/gbm_dri.c
>> +++ b/src/gbm/backends/dri/gbm_dri.c
>> @@ -918,6 +918,34 @@ failed:
>>     return NULL;
>>  }
>>
>> +static void *
>> +_gbm_dri_bo_map(struct gbm_bo *_bo,
>> +              uint32_t x, uint32_t y,
>> +              uint32_t width, uint32_t height,
>> +              uint32_t usage)
>> +{
>> +   struct gbm_dri_device *dri = gbm_dri_device(_bo->gbm);
>> +   struct gbm_dri_bo *bo = gbm_dri_bo(_bo);
>> +
>> +   if (!dri->context)
>> +      dri->context = dri->dri2->createNewContext(dri->screen, NULL, NULL, NULL);
>> +   if (!dri->context)
>> +      return NULL;
>> +
> As mentioned by Dan, bringing up a whole context may or may not have
> some side so I'm wondering if we cannot reuse the one already
> created/bound by the user (the user does create a GL/GLES context
> right ?) via libglapi (GET_CURRENT_CONTEXT macro) ? Obviously
> documenting the semantics of the API (in gbm.h) is greatly encouraged
> ;-)

I don't think you can assume a context is bound, and not sure what
assumptions you can make about threading.  So trying to re-use an
existing context seems like a bad idea.

A better approach would be to have a TRANSFER_ONLY context flag, so if
drivers want they could skip any setup/allocations not required for
transfer_map/unmap.

(That said, the existing drm_gralloc_pipe solution, or alternate XA
based approach, would also be creating a full-blown context, so what
this patch does is not worse than the alternatives.)

BR,
-R

>
>> +   usage = __DRI_IMAGE_USE_WRITE;
>> +   return dri->image->lockImage(dri->context, bo->image, x, y, width, height, usage);
>> +}
>> +
>> +static void
>> +_gbm_dri_bo_unmap(struct gbm_bo *_bo)
>> +{
>> +   struct gbm_dri_device *dri = gbm_dri_device(_bo->gbm);
>> +   struct gbm_dri_bo *bo = gbm_dri_bo(_bo);
>> +
>> +   dri->image->unlockImage(dri->context, bo->image);
> If we keep the context management as is (i.e. don't reuse the user
> context), I'd throw in a null check on the context. Additionally we
> might want to make keep track (refcount?) thus we can get concurrency
> and  no memory leaks.
>
>
>> --- a/src/gbm/gbm-symbols-check
>> +++ b/src/gbm/gbm-symbols-check
>> @@ -9,6 +9,8 @@ gbm_device_destroy
>>  gbm_create_device
>>  gbm_bo_create
>>  gbm_bo_import
>> +gbm_bo_map
>> +gbm_bo_unmap
> Thanks for these. Did not imagine you'll find them while running an
> android build ;-)
>
>
>> --- a/src/gbm/main/gbm.h
>> +++ b/src/gbm/main/gbm.h
>> @@ -237,6 +237,15 @@ gbm_bo_create(struct gbm_device *gbm,
>>                uint32_t width, uint32_t height,
>>                uint32_t format, uint32_t flags);
>>
>> +void *
>> +gbm_bo_map(struct gbm_bo *bo,
>> +              uint32_t x, uint32_t y,
>> +              uint32_t width, uint32_t height,
>> +              uint32_t flags);
>> +
>> +void
>> +gbm_bo_unmap(struct gbm_bo *bo);
>> +
> Nit: Please move these after gbm_bo_import - just how you've sorted
> them it the gbmint.h vtbl and the symbols-check.
>
> -Emil