[Mesa-dev] [v4 06/10] intel: prepare for dri images having more than one plane

[Chad Versace]

On 05/02/2013 12:08 AM, Topi Pohjolainen wrote:
> v2 (as advised by Eric):
>     - use ARRAY_SIZE
>     - re-use 'image_destroy' for cleaning up after failure
>     - check directly the region pointer instead of the buffer object
>       when determining if a region exists
>
> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>   src/mesa/drivers/dri/intel/intel_screen.c | 103 +++++++++++++++++++++---------
>   1 file changed, 72 insertions(+), 31 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/intel/intel_screen.c b/src/mesa/drivers/dri/intel/intel_screen.c
> index 4973441..d822b1c 100644
> --- a/src/mesa/drivers/dri/intel/intel_screen.c
> +++ b/src/mesa/drivers/dri/intel/intel_screen.c
> @@ -490,8 +490,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 < ARRAY_SIZE(image->regions); ++i) {
> +      if (image->regions[i])
> +         intel_region_release(&image->regions[i]);
> +   }
> +
> +   free(image);
>   }
>
>   static __DRIimage *
> @@ -568,16 +574,22 @@ intel_query_image(__DRIimage *image, int attrib, int *value)
>   static __DRIimage *
>   intel_dup_image(__DRIimage *orig_image, void *loaderPrivate)
>   {
> +   int i;
>      __DRIimage *image;
>
>      image = calloc(1, sizeof *image);
>      if (image == NULL)
>         return NULL;
>
> -   intel_region_reference(&image->regions[0], orig_image->regions[0]);
> -   if (image->regions[0] == NULL) {
> -      free(image);
> -      return NULL;

Pre-patch, this hunk returned NULL if orig_image->region[0] was somehow NULL.

> +   for (i = 0; i < ARRAY_SIZE(image->regions); ++i) {
> +      if (!orig_image->regions[i])
> +         break;

Post-patch, if orig_image->region[0] was NULL, then this function no longer returns
NULL because of the above break. To ensure that this patch doesn't regress anything,
it needs to reproduce that behavior with `if (orig_image->regions[0] != NULL) return NULL`..
Or, if your confident (... I'm not, but maybe you are) that orig_image->region[0] is never NULL
then assert that.

> +
> +      intel_region_reference(&image->regions[i], orig_image->regions[i]);
> +      if (image->regions[i] == NULL) {
> +         intel_destroy_image(image);
> +         return NULL;
> +      }
>      }
>
>      image->internal_format = orig_image->internal_format;
> @@ -646,47 +658,76 @@ 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)
> +{

I don't see the utility in extracting this code out of intel_create_image_from_fds()
into its own, similarly named function. In fact, it makes the code harder to read.
If no following patch reuses this function, then its body should remain in its original location,
intel_create_image_from_fds.

> +   int i;
> +   __DRIimage *img;
> +
> +   if (f->nplanes == 1)
> +      img = intel_allocate_image(f->planes[0].dri_format, loaderPriv);
> +   else
> +      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) {
> +         intel_destroy_image(img);
> +         return NULL;
> +      }
> +   }
> +
> +   intel_setup_image_from_dimensions(img);
> +
> +   return img;
> +}
> +
> +static __DRIimage *
>   intel_create_image_from_fds(__DRIscreen *screen,
>                               int width, int height, int fourcc,
>                               int *fds, int num_fds, int *strides, int *offsets,
>                               void *loaderPrivate)
>   {
>      struct intel_screen *intelScreen = screen->driverPrivate;
> -   struct intel_image_format *f;
> +   struct intel_image_format *f = intel_image_format_lookup(fourcc);
>      __DRIimage *image;
>      int i, index;
>
> -   if (fds == NULL || num_fds != 1)
> -      return NULL;
> -
> -   f = intel_image_format_lookup(fourcc);
> -   if (f == NULL)
> +   /**
> +    * In case the image is to consist of multiple regions, there must be exactly
> +    * one region per plane.
> +    */
> +   if (fds == NULL || f == NULL || (num_fds > 1 && f->nplanes != num_fds))
>         return NULL;
>
> -   if (f->nplanes == 1)
> -      image = intel_allocate_image(f->planes[0].dri_format, loaderPriv);
> -   else
> -      image = intel_allocate_image(__DRI_IMAGE_FORMAT_NONE, loaderPriv);
> -
> +   image = intel_setup_image_from_fds(intelScreen, width, height, f,
> +                                      fds, num_fds, strides, loaderPrivate);
>      if (image == NULL)
>         return NULL;
>
> -   image->regions[0] = intel_region_alloc_for_fd(intelScreen,
> -                                             1, width, height,
> -                                             strides[0], fds[0], "image");
> -   if (image->regions[0] == NULL) {
> -      free(image);
> -      return NULL;
> -   }
> -
>      image->planar_format = f;
> -   for (i = 0; i < f->nplanes; i++) {
> -      index = f->planes[i].buffer_index;
> -      image->offsets[index] = offsets[index];
> -      image->strides[index] = strides[index];
> -   }
>
> -   intel_setup_image_from_dimensions(image);
> +   /**
> +    * In case the image is to consist of multiple planes all in the same region,
> +    * one needs to record not only the invidual strides, but also the locations
> +    * of the planes within the region.
> +    */
> +   if (num_fds == 1 && f->nplanes > 1) {
> +      for (i = 0; i < f->nplanes; i++) {
> +         index = f->planes[i].buffer_index;
> +         image->offsets[index] = offsets[index];
> +         image->strides[index] = strides[index];
> +      }
> +   }

 From my reading of the EGL_EXT_image_dma_buf_import spec, the user may specify
EGL_DMA_BUF_PLANE${N}_OFFSET_EXT and EGL_DMA_BUF_PLANE${N}_PITCH_EXT (N=1,2) when
there are multiple fds. So, I believe the 'if' guard here should be removed.

On that topic, we should probably add Piglit tests for that.