[Mesa-dev] EGL/android: pbuffer implementation.

[Emil Velikov]

On 21 November 2016 at 14:36, Tomasz Figa <tfiga at chromium.org> wrote:
> On Mon, Nov 21, 2016 at 11:15 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 21 November 2016 at 07:23, Tomasz Figa <tfiga at chromium.org> wrote:
>>> Hi,
>>>
>>> On Wed, Nov 16, 2016 at 11:11 AM, Liu Zhiquan <zhiquan.liu at intel.com> wrote:
>>>> mesa android path didn't support pbuffer, so add pbuffer support to
>>>> fix most deqp and cts pbuffer test cases fail;
>>>> add single buffer config to support pbuffer, and create image for
>>>> pbuffer when pbuffer type is front surface.
>>>> The EGL 1.5 spec states that pbuffers have a back buffer but no front
>>>> buffer, single-buffered surfaces with no front buffer confuse Mesa;
>>>> so we deviate from the spec, following the precedent of Mesa's
>>>> EGL X11 platform.
>>>>
>>>> Test status: android CTS EGL pbuffer test can run without native crash.
>>>> test:[DEQP,EGL]all deqp pbuffer case passed.
>>>>
>>>> V3: update commit message and code review changes.
>>>>
>>>> Signed-off-by: Liu Zhiquan <zhiquan.liu at intel.com>
>>>> Signed-off-by: Kalyan Kondapally <kalyan.kondapally at intel.com>
>>>> ---
>>>>  src/egl/drivers/dri2/egl_dri2.h         |  3 +-
>>>>  src/egl/drivers/dri2/platform_android.c | 98 +++++++++++++++++++++++++--------
>>>>  2 files changed, 78 insertions(+), 23 deletions(-)
>>>>
>>>
>>> Looks like this patch has already landed, but please let me try to
>>> confirm some things here anyway. Would you mind keeping me on CC for
>>> any future patches for the EGL/Android module? (
>>
>>
>>> I believe
>>> scripts/get_reviewer.pl should already include me on the list of
>>> suggested reviewers, similarly for Rob Herring, who did a great job
>>> before helping us with testing on platforms other than i915.)
>>>
>> I'll add you and update the documentation to reference the script.
>>
>> Rob H let me know if you'd like to be in there as well which files
>> (platform_egl.c, Android build and/or other) and which email you'd
>> prefer.
>
> Thanks!
>
>>>>
>>>>     if (update_buffers(dri2_surf) < 0)
>>>>        return 0;
>>>>
>>>>     if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) {
>>>> -      /*
>>>> -       * We don't support front buffers and GLES doesn't require them for
>>>> -       * window surfaces, but some DRI drivers will request them anyway.
>>>> -       * We just ignore such request as other platforms backends do.
>>>> +      if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
>>>> +         /* According current EGL spec,
>>>> +          * front buffer rendering for window surface is not supported now */
>>>> +         _eglLog(_EGL_WARNING,
>>>> +               "%s:%d front buffer rendering for window surface is not supported!\n",
>>>> +               __func__, __LINE__);
>>>> +         return 0;
>>>
>>> This is a semantic change and according to the old comment it might
>>> break some drivers ("We don't support front buffers and GLES doesn't
>>> require them for window surfaces, but some DRI drivers will request
>>> them anyway.").
>>>
>> Fwiw -
>
> Hmm, not sure I'm getting your point here correctly (it's quite late
> so my parser might be half asleep already), but comment below is about
> pbuffers only. The comment above apparently says that EGL doesn't
> support front buffer rendering, but my comment here is more about the
> DRI extension. The semantics of the image loader extension seemed to
> allow this kind of "aggresive" probing, when the driver sets bits for
> any kinds of buffers it's interested in and it's not fatal if some of
> them are not available. However this was inferred from looking at
> driver code, as I couldn't really find any precise spec.
>
Brain went on, fingers did not follow. I meant to say
"Fwiw I've fine with temporary reverting the patch, if things are
seriously broken on your end."

On the spec side - don't think we have anything but the implicit one
in dri_interface.h

-Emil