Hi Rob,
On 22 April 2016 at 16:50, Rob Herring <r...@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 <r...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
