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.pohjolai...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
