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

[Pohjolainen, Topi]

On Thu, May 23, 2013 at 09:39:57PM -0700, Chad Versace wrote:
> 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.

Good catch!

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

I maintained the old logic skipping the copy in case the source does not have
any regions. I cannot see how that would be possible but I'm not confident
enough to assert.

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

Agreed, it does complicate things.

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

Indeed, I'll take a look at tests also.

Thanks for you keen yeys,
Topi