[Mesa-stable] [Mesa-dev] [PATCH 2/4] i965/tex_image: Reference the renderbuffer miptree in setTexBuffer2

[Chad Versace]

On Wed 08 Nov 2017, Jason Ekstrand wrote:
> The old code made a new miptree that referenced the same BO as the
> renderbuffer and just trusted in the memory aliasing to work.  There are
> only two ways in which the new miptree is liable to differ from the one
> in the renderbuffer and neither of them matter:

Ouch. I didn't realize intelSetTexBuffer2 created a new miptree, but I'm
not surprised.

>
>  1) It may have a different target.  The only targets that we can ever
>     see in intelSetTexBuffer2 are GL_TEXTURE_2D and GL_TEXTURE_RECTANGLE
>     and the difference between the two doesn't matter as far as the
>     miptree is concerned; genX(update_sampler_state) only looks at the
>     gl_texture_object and not the miptree when determining whether or
>     not to use normalized coordinates.
>
>  2) It may have a very slightly different format.  Again, this doesn't
>     matter because we've supported texture views for quite some time so
>     we always look at the gl_texture_object format instead of the
>     miptree format for hardware setup anyway.
>
> On the other hand, because we were recreating the miptree, we were using
> intel_miptree_create_for_bo which doesn't understand modifiers.  We
> really want this function to work without doing a resolve so long as you
> have modifiers so we need to fix that.
>
> Cc: "17.3" <mesa-stable at lists.freedesktop.org>
> ---
>  src/mesa/drivers/dri/i965/intel_tex_image.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c b/src/mesa/drivers/dri/i965/intel_tex_image.c
> index 37c8e24..c52992a 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
> @@ -405,6 +405,7 @@ static void
>  intel_set_texture_image_mt(struct brw_context *brw,
>                             struct gl_texture_image *image,
>                             GLenum internal_format,
> +                           mesa_format format,
>                             struct intel_mipmap_tree *mt)
>
>  {
> @@ -415,7 +416,7 @@ intel_set_texture_image_mt(struct brw_context *brw,
>     _mesa_init_teximage_fields(&brw->ctx, image,
>                                mt->surf.logical_level0_px.width,
>                                mt->surf.logical_level0_px.height, 1,
> -                              0, internal_format, mt->format);
> +                              0, internal_format, format);
>
>     brw->ctx.Driver.FreeTextureImageBuffer(&brw->ctx, image);
>
> @@ -442,7 +443,6 @@ intelSetTexBuffer2(__DRIcontext *pDRICtx, GLint target,
>     struct gl_texture_object *texObj;
>     struct gl_texture_image *texImage;
>     mesa_format texFormat = MESA_FORMAT_NONE;
> -   struct intel_mipmap_tree *mt;
>     GLenum internal_format = 0;
>
>     texObj = _mesa_get_current_tex_object(ctx, target);
> @@ -464,31 +464,24 @@ intelSetTexBuffer2(__DRIcontext *pDRICtx, GLint target,
>     if (rb->mt->cpp == 4) {
>        if (texture_format == __DRI_TEXTURE_FORMAT_RGB) {
>           internal_format = GL_RGB;
> -         texFormat = MESA_FORMAT_B8G8R8X8_UNORM;
> +         texFormat = MESA_FORMAT_B8G8R8A8_UNORM;

Why replace rgbx with rgba? I suspect the replace is due to the same
reasons explained in intel_miptree_create_for_dri_image(). Whatever the
reasons are, they're subtle and deserve a comment.

For reference, here is the rgbx->rgba replacement in
intel_miptree_create_for_dri_image:

   if (!brw->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);
   }


>        }
>        else {
>           internal_format = GL_RGBA;
>           texFormat = MESA_FORMAT_B8G8R8A8_UNORM;
>        }
>     } else if (rb->mt->cpp == 2) {
> +      /* This is 565 */
>        internal_format = GL_RGB;
>        texFormat = MESA_FORMAT_B5G6R5_UNORM;
>     }
>
>     intel_miptree_make_shareable(brw, rb->mt);
> -   mt = intel_miptree_create_for_bo(brw, rb->mt->bo, texFormat, 0,
> -                                    rb->Base.Base.Width,
> -                                    rb->Base.Base.Height,
> -                                    1, rb->mt->surf.row_pitch,
> -                                    MIPTREE_CREATE_DEFAULT);
> -   if (mt == NULL)
> -       return;
> -   mt->target = target;
>
>     _mesa_lock_texture(&brw->ctx, texObj);
>     texImage = _mesa_get_tex_image(ctx, texObj, target, 0);
> -   intel_set_texture_image_mt(brw, texImage, internal_format, mt);
> -   intel_miptree_release(&mt);
> +   intel_set_texture_image_mt(brw, texImage, internal_format,
> +                              texFormat, rb->mt);
>     _mesa_unlock_texture(&brw->ctx, texObj);
>  }
>
> @@ -581,7 +574,7 @@ intel_image_target_texture_2d(struct gl_context *ctx, GLenum target,
>     const GLenum internal_format =
>        image->internal_format != 0 ?
>        image->internal_format : _mesa_get_format_base_format(mt->format);
> -   intel_set_texture_image_mt(brw, texImage, internal_format, mt);
> +   intel_set_texture_image_mt(brw, texImage, internal_format, mt->format, mt);

This hunk looks correct. mt->format is the only suitable format
available in this function.

>     intel_miptree_release(&mt);
>  }
>
> --
> 2.5.0.400.gff86faf
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev