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

[Emil Velikov]

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.

> 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
;-)


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