<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jun 7, 2017 at 4:45 PM, Chad Versace <<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue 06 Jun 2017, Daniel Stone wrote: > Hi Chad, > > On 6 June 2017 at 21:36, Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> 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. </blockquote><div> </div><div>Does this mean we're dropping patch 1? If not, I sent out a new version which I find much easier to comprehend. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > > > /* 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. </blockquote><div> </div><div>I don't think that argument is all that weak </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > 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. <div class="HOEnZb"><div class="h5">_______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>