[Mesa-dev] [PATCH 2/2] intel: set dri_format field for all images

[Kenneth Graunke]

On 04/26/2012 06:21 AM, Ander Conselvan de Oliveira wrote:
> Only images created with intel_create_image() had the field properly
> set. Set it also on intel_dup_image(), intel_create_image_from_name()
> and intel_create_image_from_renderbuffer().
> ---
>   src/mesa/drivers/dri/intel/intel_screen.c |   15 +++++++++++++++
>   1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/intel/intel_screen.c b/src/mesa/drivers/dri/intel/intel_screen.c
> index ac21e96..26be76e 100644
> --- a/src/mesa/drivers/dri/intel/intel_screen.c
> +++ b/src/mesa/drivers/dri/intel/intel_screen.c
> @@ -188,6 +188,8 @@ intel_create_image_from_name(__DRIscreen *screen,
>       if (image == NULL)
>   	return NULL;
>
> +    image->dri_format = format;
> +
>       switch (format) {
>       case __DRI_IMAGE_FORMAT_RGB565:
>          image->format = MESA_FORMAT_RGB565;
> @@ -255,6 +257,18 @@ intel_create_image_from_renderbuffer(__DRIcontext *context,
>      image->data = loaderPrivate;
>      intel_region_reference(&image->region, irb->mt->region);
>
> +   switch (image->format) {
> +   case MESA_FORMAT_XRGB8888:
> +      image->dri_format = __DRI_IMAGE_FORMAT_XRGB8888;
> +      break;
> +   case MESA_FORMAT_ARGB8888:
> +      image->dri_format = __DRI_IMAGE_FORMAT_ARGB8888;
> +      break;
> +   case MESA_FORMAT_RGBA8888_REV:
> +      image->dri_format = __DRI_IMAGE_FORMAT_ABGR8888;
> +      break;
> +   }

Should we handle 565 here too?  It looks like some of the other switch 
statements that map from DRI image format -> Mesa format handle that.

I also wonder whether we want to create functions to do this mapping:
1. DRI_IMAGE_FORMAT -> MESA_FORMAT
2. MESA_FORMAT -> DRI_IMAGE_FORMAT

That way, you could just call them wherever you need them...

>      return image;
>   }
>
> @@ -365,6 +379,7 @@ intel_dup_image(__DRIimage *orig_image, void *loaderPrivate)
>      }
>
>      image->internal_format = orig_image->internal_format;
> +   image->dri_format      = orig_image->dri_format;
>      image->format          = orig_image->format;
>      image->data_type       = orig_image->data_type;
>      image->data            = loaderPrivate;

These look good to me.  I'd like someone else to take a look at patch 2 
before pushing, though, as I'm not terribly familiar with this code. 
Patch 1 is clearly right.

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>