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

[Pohjolainen, Topi]

On Tue, Apr 16, 2013 at 04:57:49PM -0700, Eric Anholt wrote:
> 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.

As I switched to using a fixed sized array of regions (holding up-to-three
planes), the images having one or two planes would have the rest of the regions
initialized to zero. And I simply decided to use the 'bo'-pointer as telling if
the region was in use or not.

> 
> > +   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.