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