[Mesa-dev] [PATCH 4/4] gbm: Add map/unmap functions

[Emil Velikov]

Hi Rob,

On 22 April 2016 at 16:50, Rob Herring <robh at kernel.org> wrote:
> This adds map and unmap functions to GBM utilizing the DRIimage extension
> mapImage/unmapImage functions or existing internal mapping for dumb
> buffers.
Ftr that this is quite sensitive and apart from the obvious breakage
(coming in a second) it will need some testing on a gnome-continuous
setup (iirc some used to hand out in #xorg-devel)

> Unlike prior attempts, this version provides a region to map and
> usage flags for the mapping. The operation follows the same semantics as
> the gallium transfer_map() function.
>
> This was tested with GBM based gralloc on Android.
>
> This still creates a context, but I've moved it into gbm_create_device
> rather than in the map function. This should remove any need for reference
> counting and problems with memory leaks.
>
> Signed-off-by: Rob Herring <robh at kernel.org>
> ---
>  src/gbm/backends/dri/gbm_dri.c    | 62 +++++++++++++++++++++++++++++++++++++--
>  src/gbm/backends/dri/gbm_driint.h |  5 ++--
>  src/gbm/gbm-symbols-check         |  2 ++
>  src/gbm/main/gbm.c                | 51 ++++++++++++++++++++++++++++++++
>  src/gbm/main/gbm.h                | 25 ++++++++++++++++
>  src/gbm/main/gbmint.h             |  6 ++++
>  6 files changed, 147 insertions(+), 4 deletions(-)
>
> diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c
> index 0625422..c93dc9c 100644
> --- a/src/gbm/backends/dri/gbm_dri.c
> +++ b/src/gbm/backends/dri/gbm_dri.c
> @@ -32,6 +32,7 @@
>  #include <string.h>
>  #include <errno.h>
>  #include <limits.h>
> +#include <assert.h>
>
>  #include <sys/types.h>
>  #include <unistd.h>
> @@ -603,7 +604,7 @@ gbm_dri_bo_destroy(struct gbm_bo *_bo)
>     if (bo->image != NULL) {
>        dri->image->destroyImage(bo->image);
>     } else {
> -      gbm_dri_bo_unmap(bo);
> +      gbm_dri_bo_unmap_dumb(bo);
Trivial things first - please split the renames to a separate patch.
As is the egl+gbm on swrast is broken - it should be using the *_dumb
functions.

>        memset(&arg, 0, sizeof(arg));
>        arg.handle = bo->handle;
>        drmIoctl(dri->base.base.fd, DRM_IOCTL_MODE_DESTROY_DUMB, &arg);
> @@ -828,7 +829,7 @@ create_dumb(struct gbm_device *gbm,
>     bo->handle = create_arg.handle;
>     bo->size = create_arg.size;
>
> -   if (gbm_dri_bo_map(bo) == NULL)
> +   if (gbm_dri_bo_map_dumb(bo) == NULL)
>        goto destroy_dumb;
>
>     return &bo->base.base;
> @@ -924,6 +925,54 @@ 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 flags, uint32_t *stride, void **map_data)
> +{
> +   struct gbm_dri_device *dri = gbm_dri_device(_bo->gbm);
> +   struct gbm_dri_bo *bo = gbm_dri_bo(_bo);
> +
> +   /* If it's a dumb buffer, we already have a mapping */
> +   if (bo->map) {
> +      *map_data = (char *)bo->map + (bo->base.base.stride * y) + (x * 4);
> +      *stride = bo->base.base.stride;
> +      return *map_data;
How did you test this ? I'm not sure if we'll even hit it (or we
should hit it actually).

> +   }
> +
> +   if (!dri->image || dri->image->base.version < 12) {
> +      errno = ENOSYS;
> +      return NULL;
> +   }
> +
> +   if (!dri->context)
> +      return NULL;
> +
> +   /* GBM flags and DRI flags are the same, so just pass them on */
> +   return dri->image->mapImage(dri->context, bo->image, x, y,
> +                               width, height, flags, stride, map_data);
> +}
> +
> +static void
> +gbm_dri_bo_unmap(struct gbm_bo *_bo, void *map_data)
> +{
> +   struct gbm_dri_device *dri = gbm_dri_device(_bo->gbm);
> +   struct gbm_dri_bo *bo = gbm_dri_bo(_bo);
> +
> +   /* Check if it's a dumb buffer */
> +   if (bo->map) {
> +      assert((map_data >= bo->map) && (map_data < (bo->map + bo->size)));
Nit: try to avoid multiple expressions within single assert. Then
again the whole if statement should go imho (similar to the map above)

> +      return;
> +   }
> +
> +   if (!dri->image || !dri->context || dri->image->base.version < 12)
Bikeshed: flip the order - context, image, image.foo

> +      return;
> +
> +   dri->image->unmapImage(dri->context, bo->image, map_data);
> +}
> +
> +
>  static struct gbm_surface *
>  gbm_dri_surface_create(struct gbm_device *gbm,
>                         uint32_t width, uint32_t height,
> @@ -958,6 +1007,9 @@ dri_destroy(struct gbm_device *gbm)
>     struct gbm_dri_device *dri = gbm_dri_device(gbm);
>     unsigned i;
>
> +   if (dri->context)
> +      dri->core->destroyContext(dri->context);
> +
>     dri->core->destroyScreen(dri->screen);
>     for (i = 0; dri->driver_configs[i]; i++)
>        free((__DRIconfig *) dri->driver_configs[i]);
> @@ -981,6 +1033,8 @@ dri_device_create(int fd)
>     dri->base.base.fd = fd;
>     dri->base.base.bo_create = gbm_dri_bo_create;
>     dri->base.base.bo_import = gbm_dri_bo_import;
> +   dri->base.base.bo_map = gbm_dri_bo_map;
> +   dri->base.base.bo_unmap = gbm_dri_bo_unmap;
>     dri->base.base.is_format_supported = gbm_dri_is_format_supported;
>     dri->base.base.bo_write = gbm_dri_bo_write;
>     dri->base.base.bo_get_fd = gbm_dri_bo_get_fd;
> @@ -1004,6 +1058,10 @@ dri_device_create(int fd)
>     if (ret)
>        goto err_dri;
>
> +   if (dri->image->base.version >= 12)
> +      dri->context = dri->dri2->createNewContext(dri->screen, NULL,
> +                                                 NULL, NULL);
> +
Have you measured how much this costs us (cpu time and/or memory) ?

> --- 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
Nice one. Thanks !


> --- a/src/gbm/main/gbm.c
> +++ b/src/gbm/main/gbm.c
> @@ -386,6 +386,57 @@ gbm_bo_import(struct gbm_device *gbm,
>  }
>
>  /**
> + * Map a region of a gbm buffer object for cpu access
> + *
> + * This function maps a region of a gbm bo for cpu read and/or write
> + * access.
> + *
> + * \param bo The buffer object
> + * \param x The X starting position of the mapped region for the buffer
> + * \param y The Y starting position of the mapped region for the buffer
Since this is the first time we mention X and Y, we should clearly
define the point of origin.

> + * \param width The width of the mapped region for the buffer
> + * \param height The height of the mapped region for the buffer
> + * \param flags The union of the usage flags for this buffer
> + * \param stride Returned stride in bytes of the mapped region.
> + * \param map_data Returned opaque ptr for the mapped region
> + *
> + * \return Address of the mapped buffer
> + * gbm_bo_unmap() when no longer needed. On error, %NULL is returned
Something went wrong here - this does not look right.

> + * and errno is set.
> + *
> + * \sa enum gbm_bo_transfer_flags for the list of flags
> + */
Imho the documentation would be better in the header - for users to
see. Actually same goes for the rest of the file.

Can we take a look at the GBM gralloc as well. One thing that worries
me is that (most likely) you are requesting/creating a bo without
GBM_BO_USE_WRITE whist using MAP + CPU write UNMAP. If you do set the
USE_WRITE flag, you're getting a dumb buffer, which I'm not sure how
well is going to work.

Whichever way one goes, we want to clearly define/describe the
expected behaviour with the different GBM_BO_USE and GBM_BO_TRANSFER
flags.

Emil