[Mesa-dev] [PATCH 2/6] i965: Add a RGBX->RGBA fallback for glEGLImageTextureTarget2D()
This enables support for importing RGBX EGLImage textures on
Skylake.
Chrome OS needs support for RGBX EGLImage textures because because
the Android framework produces HAL_PIXEL_FORMAT_RGBX winsys
surfaces, which the Chrome OS compositor consumes as dma_bufs. On
hardware for which RGBX is unsupported or disabled, normally core Mesa
provides the RGBX->RGBA fallback during glTexStorage. But the DRIimage
code bypasses core Mesa, so we must do the fallback in i965.
---
src/mesa/drivers/dri/i965/intel_tex_image.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c
b/src/mesa/drivers/dri/i965/intel_tex_image.c
index 649b3907d1..92c6c15c72 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_image.c
+++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
@@ -5,6 +5,7 @@
#include "main/bufferobj.h"
#include "main/context.h"
#include "main/formats.h"
+#include "main/format_fallback.h"
#include "main/glformats.h"
#include "main/image.h"
#include "main/pbo.h"
@@ -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_XBGR textures because the Android
+ * framework produces HAL_PIXEL_FORMAT_RGBX 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;
/* 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);
--
2.13.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/6] i965: Add a RGBX->RGBA fallback for glEGLImageTextureTarget2D()
Hi Chad,
On 6 June 2017 at 21:36, Chad Versace 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_XBGR textures because the Android
> + * framework produces HAL_PIXEL_FORMAT_RGBX 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;
>
> /* 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, 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.
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.
Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/6] i965: Add a RGBX->RGBA fallback for glEGLImageTextureTarget2D()
On Tue 06 Jun 2017, Daniel Stone wrote:
> Hi Chad,
>
> On 6 June 2017 at 21:36, Chad Versace 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_XBGR textures because the Android
> > + * framework produces HAL_PIXEL_FORMAT_RGBX 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_.
- This patch falls into the "runs only on Chrome OS" category. It
helps the Chrome OS compositor import the above-mentioned cli
Re: [Mesa-dev] [PATCH 2/6] i965: Add a RGBX->RGBA fallback for glEGLImageTextureTarget2D()
Hi Chad, On 8 June 2017 at 00:45, Chad Versace wrote: > On Tue 06 Jun 2017, Daniel Stone wrote: >> 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: Thanks a lot for the detailed explanation! Makes sense. > Anyway, why would it fail during fourcc lookup? The table > intel_screen.c:intel_image_formats[] contains an entry for > __DRI_IMAGE_FOURCC_XBGR. And egl_dri2.c:dri2_check_dma_buf_format() > knows about DRM_FORMAT_XBGR too. Apparently I can't read, or type, so missed it in the table. Cheers, Daniel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/6] i965: Add a RGBX->RGBA fallback for glEGLImageTextureTarget2D()
On Wed, Jun 7, 2017 at 4:45 PM, Chad Versace
wrote:
> On Tue 06 Jun 2017, Daniel Stone wrote:
> > Hi Chad,
> >
> > On 6 June 2017 at 21:36, Chad Versace 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_XBGR textures because the
> Android
> > > + * framework produces HAL_PIXEL_FORMAT_RGBX 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 AHardwa
Re: [Mesa-dev] [PATCH 2/6] i965: Add a RGBX->RGBA fallback for glEGLImageTextureTarget2D()
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_XBGR textures because the
> Android
> > > + * framework produces HAL_PIXEL_FORMAT_RGBX 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
