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

[Jason Ekstrand]

On Wed, Jun 7, 2017 at 4:45 PM, Chad Versace <chadversary at chromium.org>
wrote:

> 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.
>

Does this mean we're dropping patch 1?  If not, I sent out a new version
which I find much easier to comprehend.


> > >
> > >     /* 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.
>

I don't think that argument is all that weak


>     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.
>

But this one really sets it.  Let's have the image have the "correct"
format and fix it in miptree_create_for_dri_image.  Speaking of such, I
just rewrote that function and I'm not sure how this fits in.


> > 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.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170620/9d1b5570/attachment-0001.html>