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

[Rob Herring]

On Fri, Apr 22, 2016 at 6:32 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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>

[...]

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

I haven't. It will get hit if you use GBM_BO_USE_WRITE. That (or no
image ext) causes a dumb buffer to be used and mmapped on allocate.

[...]

>> @@ -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) ?

No, will do.

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

I just followed what was done for the rest of the file. Are you
suggesting I do that first?

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

I'm not using GBM_BO_USE_WRITE and that is not a condition for mapping
given that flag is tied to cursors (according to comments) and gives
dumb buffers. Also of note, if gralloc flags are set for r/w often,
then I request a linear buffer. Here's the gralloc side:

https://github.com/robherring/gbm_gralloc

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

Ultimately, it is probably going to be dependent on the gallium
drivers. At the GBM level there is no restriction. If the drivers have
cases where mappings can fail, then that is going to propagate up. I
was under the impression that is not the case, but that the mapping
process could be slow. I'm not sure how we would determine either it
could fail or that it would be slow.

Rob