[Mesa-dev] [PATCH 4/8] intel: prepare for dri images having more than plane

[Eric Anholt]

Topi Pohjolainen <topi.pohjolainen at intel.com> writes:

> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>  src/mesa/drivers/dri/intel/intel_screen.c | 101 +++++++++++++++++++++---------
>  1 file changed, 71 insertions(+), 30 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/intel/intel_screen.c b/src/mesa/drivers/dri/intel/intel_screen.c
> index 3d0344e..8716688 100644
> --- a/src/mesa/drivers/dri/intel/intel_screen.c
> +++ b/src/mesa/drivers/dri/intel/intel_screen.c
> @@ -496,8 +496,14 @@ intel_create_image_from_texture(__DRIcontext *context, int target,
>  static void
>  intel_destroy_image(__DRIimage *image)
>  {
> -    intel_region_release(&image->regions[0]);
> -    free(image);
> +   int i;
> +
> +   for (i = 0; i < sizeof(image->regions) / sizeof(struct intel_region); ++i) {

All instances of this should be using ARRAY_SIZE.

> +      if (image->regions[i]->bo)
> +         intel_region_release(&image->regions[i]);

Why are you looking at the BO for deciding whether to free the region?
A region should always have a BO.  If you're trying to null-check the
region, intel_region_release already does that.

> +   for (i = 0; i < sizeof(image->regions) / sizeof(struct intel_region); ++i) {
> +      if (!orig_image->regions[i]->bo)
> +         break;

null-checking the wrong thing.

> +
> +      intel_region_reference(&image->regions[i], orig_image->regions[i]);
> +      if (image->regions[i] == NULL) {
> +         for (--i; i >= 0; --i)
> +            intel_region_release(&image->regions[i]);
> +
> +	 free(image);

mixed tab-and-space indentation, should always be spaces.

> +         return NULL;
> +      }
>     }
>  
>     image->internal_format = orig_image->internal_format;
> @@ -649,45 +664,71 @@ intel_create_image_from_names(__DRIscreen *screen,
>  }
>  
>  static __DRIimage *
> +intel_setup_image_from_fds(struct intel_screen *screen, int width, int height,
> +                           const struct intel_image_format *f,
> +                           const int *fds, int num_fds, const int *strides,
> +                           void *loaderPriv)
> +{
> +   int i;
> +   __DRIimage *img = intel_allocate_image(__DRI_IMAGE_FORMAT_NONE, loaderPriv);
> +
> +   if (img == NULL)
> +      return NULL;
> +
> +   for (i = 0; i < num_fds; i++) {
> +      img->regions[i] = intel_region_alloc_for_fd(screen, f->planes[i].cpp,
> +                           width >> f->planes[i].width_shift,
> +                           height >> f->planes[i].height_shift,
> +                           strides[i], fds[i], "image");
> +
> +      if (img->regions[i] == NULL) {
> +         for (--i; i >= 0; --i)
> +            intel_region_release(&img->regions[i]);
> +
> +         free(img);
> +         return NULL;
> +      }
> +   }

I've seen this image-freeing loop a few times, seems like it should be
in just one place.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130416/6687bfdf/attachment.pgp>