On Tue, Apr 16, 2013 at 04:57:49PM -0700, Eric Anholt wrote:
> Topi Pohjolainen <topi.pohjolai...@intel.com> writes:
>
> > Signed-off-by: Topi Pohjolainen <topi.pohjolai...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
