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

[Emil Velikov]

On 11 April 2016 at 15:31, Rob Clark <robdclark at gmail.com> wrote:
> 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.
>
Nice. Would be great to confirm if there are points that he did not
explicitly mention and/or thought of at the time.

>
>>> 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.
>
I'm not familiar how exactly the API is meant/should be used, although
I do recall that nouveau has/had problems with multiple contexts. So
you can see why I brought the idea forward ;-)

> 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.
>
Seems that currently almost no drivers bother with the flags. And from
a quick look some drivers might be a bit awkward with although the
idea sounds good. Just wondering if (how much) st/dri will give us the
finger (read bugs), as opposed to using pure gallium.

> (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.)
>
Never said (or meant to imply) it's worse.

-Emil