On Tue 20 Jun 2017, Jason Ekstrand wrote:
> On Wed, Jun 7, 2017 at 4:45 PM, Chad Versace <[1]chadvers...@chromium.org>
> wrote:
>
> On Tue 06 Jun 2017, Daniel Stone wrote:
> > Hi Chad,
> >
> > On 6 June 2017 at 21:36, Chad Versace <[2]chadvers...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
