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

[Chad Versace]

On Tue 20 Jun 2017, Jason Ekstrand wrote:
> On Wed, Jun 7, 2017 at 4:45 PM, Chad Versace <[1]chadversary at chromium.org>
> wrote:
> 
>     On Tue 06 Jun 2017, Daniel Stone wrote:
>     > Hi Chad,
>     >
>     > On 6 June 2017 at 21:36, Chad Versace <[2]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.

We still need patch 1 for the intelCreateBuffer paths, which have no
current context, and therefore no ctx->Driver.ChooseTextureFormat.

>     > >
>     > >     /* 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, glEGLImageTargetRenderbufferSt
>     orageOES, 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.

Good to hear we agree.