[Mesa-dev] [PATCH 2/6] i965: Add a RGBX->RGBA fallback for glEGLImageTextureTarget2D()

[Chad Versace]

On Tue 06 Jun 2017, Daniel Stone wrote:
> Hi Chad,
> 
> On 6 June 2017 at 21:36, Chad Versace <chadversary at chromium.org> wrote:
> > @@ -254,8 +255,22 @@ create_mt_for_dri_image(struct brw_context *brw,
> >     struct gl_context *ctx = &brw->ctx;
> >     struct intel_mipmap_tree *mt;
> >     uint32_t draw_x, draw_y;
> > +   mesa_format format = image->format;
> > +
> > +   if (!ctx->TextureFormatSupported[format]) {
> > +      /* The texture storage paths in core Mesa detect if the driver does not
> > +       * support the user-requested format, and then searches for a
> > +       * fallback format. The DRIimage code bypasses core Mesa, though. So we
> > +       * do the fallbacks here for important formats.
> > +       *
> > +       * We must support DRM_FOURCC_XBGR8888 textures because the Android
> > +       * framework produces HAL_PIXEL_FORMAT_RGBX8888 winsys surfaces, which
> > +       * the Chrome OS compositor consumes as dma_buf EGLImages.
> > +       */
> > +      format = _mesa_format_fallback_rgbx_to_rgba(format);
> > +   }
> >
> > -   if (!ctx->TextureFormatSupported[image->format])
> > +   if (!ctx->TextureFormatSupported[format])
> >        return NULL;

I dislike what I wrote above. There's a much better way to do the
fallback, a way that handles more types of fallback than rgbx->rgba and
that's the same as the fallback used by glTexStorage2D(). The better way
is to re-use the core Mesa code that the comment refers to, like this:

    mesa_format format = ctx->Driver.ChooseTextureFormat(ctx, GL_TEXTURE_2D,
                                                         internalFormat, GL_NONE, GL_NONE);

As precedent, that's exactly what intel_renderbuffer_format() does.

> >
> >     /* Disable creation of the texture's aux buffers because the driver exposes
> > @@ -263,7 +278,7 @@ create_mt_for_dri_image(struct brw_context *brw,
> >      * buffer's content to the main buffer nor for invalidating the aux buffer's
> >      * content.
> >      */
> > -   mt = intel_miptree_create_for_bo(brw, image->bo, image->format,
> > +   mt = intel_miptree_create_for_bo(brw, image->bo, format,
> >                                      0, image->width, image->height, 1,
> >                                      image->pitch,
> >                                      MIPTREE_LAYOUT_DISABLE_AUX);
> 
> I wonder if it wouldn't be better to do this in
> intel_create_image_from_name. That way it would be more obvious
> up-front what's happening,

I agree that the intent would become more obvious if the format fallback
were done at time of import instead of gl*Storage. But I see two
arguments against it:

    1. First, the weaker argument.

       The chosen fallback format,
       and even the choice to do a fallback at all, is a property of the
       image's usage and not a property of the image itself. A single
       image can have multiple uses during its lifetime, and the driver
       may need a different fallback or no fallback for each. I'm
       defining "image usage" here in terms of
       glEGLImageTargetTexture2DOES, glEGLImageTargetRenderbufferStorageOES, and
       GL_TEXURE_EXTERNAL_OES vs GL_TEXTURE_2D.

       Which reminds me... I should have submitted an analgous patch for
       glEGLImageTargetRenderbufferStorageOES().

       Since the driver may support a given format for texturing but not
       rendering, or for rendering but not texturing, we would need to do at
       least two format fallbacks during image import, and cache the fallback
       results in the image struct. This approach is possible, but...
       onto the next bullet.

    2. A more practical argument.

       If possible, it's better to do the fallback for
       glEGLImageTextureTarget2DOES() in the same way as for
       glTexStorage2D(), as I explained above. But that requires access
       to a GL context; eglCreateImage may be called without
       a context. [EGL_EXT_image_dma_buf_import explicitly requires that
       eglCreateImage(context=EGL_CONTEXT_NONE)]; and therefore
       intel_create_image_name() and friends also have no context.

> and also it'd be easy to add the analogue
> to intel_create_image_from_fds for the explicit dmabuf (as opposed to
> ANativeBuffer; your description of 'from dmabufs' threw me because the
> dmabuf fd import path would fail immediately on FourCC lookup) import
> path.

I did mean dma_buf, not AHardwareBuffer or ANativeBuffer, in this patch.

The patch series is secretly crazy. It's implicitly partitioned into
3 sets of patches: patches that touch code that will run only on
Android; that will run only on Chrome OS; and that will run on both.

  - The patch "i965/dri: Support R8G8B8A8 and R8G8B8X8 configs" falls
    into the "runs only on Android category". It deals with creating an
    EGLSurface from a AHardwareBuffer with HAL_PIXEL_FORMAT_RGBX_8888.

  - This patch falls into the "runs only on Chrome OS" category. It
    helps the Chrome OS compositor import the above-mentioned client's
    R8G8B8X8 EGLSurface through eglCreateImage(EGL_LINUX_DMA_BUF) and
    glEGLImageTextureTarget2DOES. Outside the Android container, there's
    no AHardwareBuffer; it's all dma_bufs here.

Anyway, why would it fail during fourcc lookup?  The table
intel_screen.c:intel_image_formats[] contains an entry for
__DRI_IMAGE_FOURCC_XBGR8888. And egl_dri2.c:dri2_check_dma_buf_format()
knows about DRM_FORMAT_XBGR8888 too.

> As an aside, it's safe to enable in Wayland (IMO anyway), because we
> only use the DRM format there; there's no concept of a 'surface
> format' or visuals inside the Wayland client EGL platform, just direct
> sampling from whichever buffer was last attached. EGL_NATIVE_VISUAL_ID
> is empty, because we don't have anything to expose to the client.
> Probably famous last words tho.

Good to know. That's one less thing my patches may break.