[Mesa-dev] [PATCH 13/30] i965/miptree: Add an explicit format parameter to create_for_dri_image

[Chad Versace]

On Fri 16 Jun 2017, Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/intel_fbo.c         | 3 ++-
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ++++---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 3 ++-
>  src/mesa/drivers/dri/i965/intel_tex_image.c   | 3 ++-
>  4 files changed, 10 insertions(+), 6 deletions(-)

I dislike this patch. A lot.

The __DRIimage already has a 'format' member. Why is it necessary to
override that format? More importantly, *when* is it necessary?

In patch "i965: Use create_for_dri_image in intel_update_image_buffer",
I see that you pass intel_rb_format(rb) down as the 'format' parameter.
Is that the only place the override is needed? In that function, why do
the image's format and the renderbuffer's format differ? When do they
differ? When they do differ, is it illegal then to update the
image's format to match? If we don't update the image's format in
intel_update_image_buffer(), then does the invalidity of
__DRIimage::format cause potential issues elsewhere?

[...]

> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 08c13fc..7b4d431 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -900,12 +900,13 @@ miptree_create_for_planar_image(struct brw_context *brw,
>  
>  struct intel_mipmap_tree *
>  intel_miptree_create_for_dri_image(struct brw_context *brw,
> -                                   __DRIimage *image, GLenum target)
> +                                   __DRIimage *image, GLenum target,
> +                                   mesa_format format)
>  {
>     if (image->planar_format && image->planar_format->nplanes > 0)
>        return miptree_create_for_planar_image(brw, image, target);
>  
> -   if (!brw->ctx.TextureFormatSupported[image->format])
> +   if (!brw->ctx.TextureFormatSupported[format])
>        return NULL;

The 'format' parameter is ignored if the image has a planar format. That
makes me suspicious. At a minimum, this needs

  assert(!format == !image->planar_format)

or an explanation of why the assertion is invalid.