[Mesa-dev] [PATCH 2/3] egl_dri2: add support for Android

[Chad Versace]

On 08/24/2011 06:20 PM, Chia-I Wu wrote:
> On Thu, Aug 25, 2011 at 8:58 AM, Chad Versace <chad at chad-versace.us> wrote:
> Comments below.
> 
> On 08/23/2011 08:10 PM, Chia-I Wu wrote:
>>>> Add platform_android.c that supports _EGL_PLAFORM_ANDROID.  It works
>>>> with drm_gralloc, where back buffers of windows are backed by GEM
>>>> objects.
>>>>
>>>> In Android a native window has a queue of back buffers allocated by the
>>>> server, through drm_gralloc.  For each frame, EGL needs to
>>>>
>>>>   dequeue the next back buffer
>>>>   render to the buffer
>>>>   enqueue the buffer
>>>>
>>>> After enqueuing, the buffer is no longer valid to EGL.  A window has no
>>>> depth buffer or other aux buffers.  They need to be allocated locally by
>>>> EGL.
>>>> ---
>>>>  src/egl/drivers/dri2/egl_dri2.c         |   89 +++++
>>>>  src/egl/drivers/dri2/egl_dri2.h         |   18 +
>>>>  src/egl/drivers/dri2/platform_android.c |  588 +++++++++++++++++++++++++++++++
>>>>  3 files changed, 695 insertions(+), 0 deletions(-)
>>>>  create mode 100644 src/egl/drivers/dri2/platform_android.c
>>>>
>>>> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
>>>> index ba728a1..33c2330 100644
>>>> --- a/src/egl/drivers/dri2/egl_dri2.c
>>>> +++ b/src/egl/drivers/dri2/egl_dri2.c
> 
> [snip]
> 
>>>> +static _EGLImage *
>>>> +dri2_create_image_android_native_buffer(_EGLDisplay *disp,
>>>> +                                        EGLClientBuffer buffer)
> 
> To reiterate Benjamin, I think dri2_create_image_android_native_buffer should go
> in android_platform.c.
> 
> [snip]
> 
>>>> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
>>>> new file mode 100644
>>>> index 0000000..d0de94c
>>>> --- /dev/null
>>>> +++ b/src/egl/drivers/dri2/platform_android.c
> 
> [snip]
> 
>>>> +static _EGLSurface *
>>>> +droid_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
>>>> +                 _EGLConfig *conf, EGLNativeWindowType window,
>>>> +                 const EGLint *attrib_list)
>>>> +{
>>>> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>>>> +   struct dri2_egl_config *dri2_conf = dri2_egl_config(conf);
>>>> +   struct dri2_egl_surface *dri2_surf;
>>>> +   int format;
>>>> +
>>>> +   (void) drv;
>>>> +
>>>> +   if (!window || window->common.magic != ANDROID_NATIVE_WINDOW_MAGIC) {
>>>> +      _eglError(EGL_BAD_NATIVE_WINDOW, "droid_create_surface");
>>>> +      return NULL;
>>>> +   }
>>>> +   if (window->query(window, NATIVE_WINDOW_FORMAT, &format)) {
>>>> +      _eglError(EGL_BAD_NATIVE_WINDOW, "droid_create_surface");
>>>> +      return NULL;
>>>> +   }
>>>> +   if (format != dri2_conf->base.NativeVisualID) {
>>>> +      _eglLog(_EGL_WARNING, "Native format mismatch: 0x%x != 0x%x",
>>>> +            format, dri2_conf->base.NativeVisualID);
>>>> +   }
>>>> +
>>>> +   dri2_surf = calloc(1, sizeof *dri2_surf);
>>>> +   if (!dri2_surf) {
>>>> +      _eglError(EGL_BAD_ALLOC, "droid_create_surface");
>>>> +      return NULL;
>>>> +   }
>>>> +
>>>> +   if (!_eglInitSurface(&dri2_surf->base, disp, type, conf, attrib_list))
>>>> +      goto cleanup_surf;
> 
> It seems that droid_get_buffers_with_format() only supports single-buffered
> surfaces. So eglCreateWindowSurface should reject requests for which
> EGL_RENDER_BUFFER != EGL_SINGLE_BUFFER.
> 
>      if (dri2_surf->base.RenderBuffer != EGL_SINGLE_BUFFER)
>         goto cleanup_surf;
>> [CC Benjamin]
>> EGL_RENDER_BUFFER is a hint here.  It is fine if the native window
>> does not support EGL_SINGLE_BUFFER.


You're right, specifying EGL_RENDER_BUFFER in eglCreateWindowSurface is just
a hint. And since the spec says this about eglQuerySurface,
    Querying EGL_RENDER_BUFFER returns the buffer which client API rendering
    is requested to use. For a window surface, this is the same attribute value specified
    when the surface was created.
we shouldn't alter the requested value of dri2_surf->base.RenderBuffer.
 
>> On the other hand, ctx->WindowRenderBuffer should be set.  It is
>> queried by eglQueryContext.  I think it can be set in
>> dri2_create_context: to EGL_BACK_BUFFER when there is
>> dri_double_config and EGL_SINGLE_BUFFER when there is only
>> dri_single_config.  Idea?

I agree that it seems safe to set ctx->WindowRenderBuffer when there is only a
dri_single_config. So, to ensure that the EGLConfig passed to
eglCreateContext does not have a dri_double_config, I think you need
to add this snippet in droid_add_configs_for_visuals:
   for (i = 0; visuals[i].format; i++) {
      ...
      for (j = 0; dri2_dpy->driver_configs[j]; j++) {
         ...
         /* Maybe this? */
         if (dri2_dpy->driver_configs[j]->modes.doubleBufferMode)
            continue;
         /* end suggestion */

         dri2_conf = dri2_add_config(dpy, dri2_dpy->driver_configs[j],
               count + 1, visuals[i].size, EGL_WINDOW_BIT, NULL,
               visuals[i].rgba_masks);
Also, the call to dri2_dpy->dri2->createNewDrawable should be passed
dri2_conf->dri_single_config.

But I am hesitant to set it to EGL_BACK_BUFFER when there is a
dri2_double_config. If the driver supports rendering to a single buffered
surface under a context that supports double-buffering, then, if we had set
ctx->WindowRenderBuffer = EGL_BACK_BUFFER, that would force
eglQueryContext(EGL_RENDER_BUFFER) to always return EGL_BACK_BUFFER, even
when rendering to the single buffered surface.

> 
> I think the DRI2 surface type needs to be set here too.
> 
>      dri2_surf->type = DRI2_WINDOW_SURFACE;
>> This field seems to be used by wayland only.  For the other places,
>> dri2_surf->Base.Type is used.
>>>> +
>>>> +   dri2_surf->dri_drawable =
>>>> +      (*dri2_dpy->dri2->createNewDrawable)(dri2_dpy->dri_screen,
>>>> +                                        dri2_conf->dri_double_config,
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I think dri2_conf->dri_single_config should be passed here.

>>>> +                                           dri2_surf);
>>>> +   if (dri2_surf->dri_drawable == NULL) {
>>>> +      _eglError(EGL_BAD_ALLOC, "dri2->createNewDrawable");
>>>> +      goto cleanup_surf;
>>>> +   }
>>>> +
>>>> +   window->common.incRef(&window->common);
>>>> +   window->query(window, NATIVE_WINDOW_WIDTH, &dri2_surf->base.Width);
>>>> +   window->query(window, NATIVE_WINDOW_HEIGHT, &dri2_surf->base.Height);
>>>> +
>>>> +   dri2_surf->window = window;
>>>> +
>>>> +   return &dri2_surf->base;
>>>> +
>>>> +cleanup_surf:
>>>> +   free(dri2_surf);
>>>> +
>>>> +   return NULL;
>>>> +}
> 
> [snip]
> 
>>>> +#include <xf86drm.h>
>>>> +/* for i915 */
>>>> +#include <i915_drm.h>
>>>> +/* for radeon */
>>>> +#include <radeon_drm.h>
> 
> These includes should be moved to the top of the file.
>> Will do and mention that they are only used by droid_get_pci_id.
>>
> 
> 
> 

-- 
Chad Versace
chad at chad-versace.us