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