On 11 April 2016 at 15:31, Rob Clark <robdcl...@gmail.com> wrote:
> On Mon, Apr 11, 2016 at 10:09 AM, Emil Velikov <emil.l.veli...@gmail.com>
> wrote:
>> Hi Rob,
>>
>> On 31 March 2016 at 04:21, Rob Herring <r...@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 <r...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
