[Mesa-dev] [v4 06/10] intel: prepare for dri images having more than one plane
Pohjolainen, Topi
topi.pohjolainen at intel.com
Tue May 28 01:48:10 PDT 2013
On Fri, May 24, 2013 at 11:55:00AM +0300, Pohjolainen, Topi wrote:
> 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.
>
I didn't add any tests for this yet as I could only check that the image
creation succeeded. Once there is the image external extension available one
could have more meaningful test with sampling. That would check that the
offsets are not just accepted but also used correctly by the driver.
More information about the mesa-dev
mailing list