Re: [Piglit] [Mesa-dev] [PATCH v2] amd_performance_monitor: Fix multi-statement macro 'report'.
On 05/07/2013 02:04 PM, Vinson Lee wrote:
Fixes "Nesting level does not match indentation" defect reported by
Coverity.
That has to be the stupidest defect I've ever heard of. But your patch
looks reasonable nonetheless - most people use do-while blocks like
that, so it's probably a good idea.
Reviewed-by: Kenneth Graunke
Signed-off-by: Vinson Lee
---
tests/spec/amd_performance_monitor/api.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/spec/amd_performance_monitor/api.c
b/tests/spec/amd_performance_monitor/api.c
index 7b321cf..3205fc0 100644
--- a/tests/spec/amd_performance_monitor/api.c
+++ b/tests/spec/amd_performance_monitor/api.c
@@ -113,8 +113,10 @@ find_invalid_counter(unsigned *counters, int num_counters)
}
#define report(pass) \
-piglit_report_subtest_result((pass) ? PIGLIT_PASS : PIGLIT_FAIL,
__FUNCTION__); \
-return
+ do { \
+ piglit_report_subtest_result((pass) ? PIGLIT_PASS :
PIGLIT_FAIL, __FUNCTION__); \
+ return; \
+ } while (0)
/**/
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH v2] amd_performance_monitor: Fix multi-statement macro 'report'.
On 05/07/2013 02:06 PM, Vinson Lee wrote:
Fixes "Nesting level does not match indentation" defect reported by
Coverity.
Signed-off-by: Vinson Lee
Reviewed-by: Ian Romanick
---
tests/spec/amd_performance_monitor/api.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/spec/amd_performance_monitor/api.c
b/tests/spec/amd_performance_monitor/api.c
index 7b321cf..3205fc0 100644
--- a/tests/spec/amd_performance_monitor/api.c
+++ b/tests/spec/amd_performance_monitor/api.c
@@ -113,8 +113,10 @@ find_invalid_counter(unsigned *counters, int num_counters)
}
#define report(pass) \
-piglit_report_subtest_result((pass) ? PIGLIT_PASS : PIGLIT_FAIL,
__FUNCTION__); \
-return
+ do { \
+ piglit_report_subtest_result((pass) ? PIGLIT_PASS :
PIGLIT_FAIL, __FUNCTION__); \
+ return; \
+ } while (0)
/**/
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [v5 04/12] tests: spec: EXT_image_dma_buf_import invalid hints
On 05/07/2013 04:04 PM, Eric Anholt wrote:
Chad Versace writes:
On 05/03/2013 02:23 PM, Eric Anholt wrote:
Topi Pohjolainen writes:
diff --git a/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
new file mode 100644
index 000..2499554
--- /dev/null
+++ b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
@@ -0,0 +1,15 @@
+#add_definitions(-DSOURCE_DIR="${piglit_SOURCE_DIR}/")
+
+include_directories(
+ ${OPENGL_INCLUDE_PATH}
+ )
+
+link_libraries(
+ ${OPENGL_gles1_LIBRARY}
+ ${OPENGL_egl_LIBRARY}
+ piglitutil_gles1
+ )
+
+piglit_add_executable(ext_image_dma_buf_import-invalid_hints invalid_hints.c)
I'd like to see conditional compile on HAVE_LIBDRM
I don't think it makes sense to condition the compile on HAVE_LIBDRM, because
Topi wrote these tests to be independent of drm drivers. If Linaro, for example,
implemented piglit_create/destroy_dma_buf for ARM, then the tests will run just
fine there. (I chose Linaro/ARM because together they drove the initial
implementation of this extension, and Linaro uses Piglit).
Well, conditional on whatever it is that gets you access to the DRM
interfaces that you're using to create dmabufs. Which in the
implementation posted here is HAVE_LIBDRM.
So, you're asserting that if someone adds a none-libdrm implemenation of
piglit_create_dma_buf, then the burden is on them to fix the CMakeLists
to work for them. That's fine with me.
> and no
ext_image_dma_buf_fourcc.h.
Why kill ext_image_dma_buf_fourcc.h? Several of the following tests resuse the
formats defined there. Are you suggesting that the formats be redefined in
each *.c file?
Because the correct place to get DRM fourcc defines from is
drm_fourcc.h. You're even told to use drm_fourcc.h in the spec!
Fair enough. Then the CMakeLists needs to condition this directory's
compile on the header's presence.
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [v5 04/12] tests: spec: EXT_image_dma_buf_import invalid hints
Chad Versace writes:
> On 05/03/2013 02:23 PM, Eric Anholt wrote:
>> Topi Pohjolainen writes:
>>
>>> diff --git a/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
>>> b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
>>> new file mode 100644
>>> index 000..2499554
>>> --- /dev/null
>>> +++ b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
>>> @@ -0,0 +1,15 @@
>>> +#add_definitions(-DSOURCE_DIR="${piglit_SOURCE_DIR}/")
>>> +
>>> +include_directories(
>>> + ${OPENGL_INCLUDE_PATH}
>>> + )
>>> +
>>> +link_libraries(
>>> + ${OPENGL_gles1_LIBRARY}
>>> + ${OPENGL_egl_LIBRARY}
>>> + piglitutil_gles1
>>> + )
>>> +
>>> +piglit_add_executable(ext_image_dma_buf_import-invalid_hints
>>> invalid_hints.c)
>>
>> I'd like to see conditional compile on HAVE_LIBDRM
>
> I don't think it makes sense to condition the compile on HAVE_LIBDRM, because
> Topi wrote these tests to be independent of drm drivers. If Linaro, for
> example,
> implemented piglit_create/destroy_dma_buf for ARM, then the tests will run
> just
> fine there. (I chose Linaro/ARM because together they drove the initial
> implementation of this extension, and Linaro uses Piglit).
Well, conditional on whatever it is that gets you access to the DRM
interfaces that you're using to create dmabufs. Which in the
implementation posted here is HAVE_LIBDRM.
> > and no
>> ext_image_dma_buf_fourcc.h.
>
> Why kill ext_image_dma_buf_fourcc.h? Several of the following tests resuse the
> formats defined there. Are you suggesting that the formats be redefined in
> each *.c file?
Because the correct place to get DRM fourcc defines from is
drm_fourcc.h. You're even told to use drm_fourcc.h in the spec!
pgptkKMuOasc7.pgp
Description: PGP signature
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH v2] amd_performance_monitor: Fix multi-statement macro 'report'.
On 05/07/2013 03:06 PM, Vinson Lee wrote:
Fixes "Nesting level does not match indentation" defect reported by
Coverity.
Signed-off-by: Vinson Lee
---
tests/spec/amd_performance_monitor/api.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/spec/amd_performance_monitor/api.c
b/tests/spec/amd_performance_monitor/api.c
index 7b321cf..3205fc0 100644
--- a/tests/spec/amd_performance_monitor/api.c
+++ b/tests/spec/amd_performance_monitor/api.c
@@ -113,8 +113,10 @@ find_invalid_counter(unsigned *counters, int num_counters)
}
#define report(pass) \
-piglit_report_subtest_result((pass) ? PIGLIT_PASS : PIGLIT_FAIL,
__FUNCTION__); \
-return
+ do { \
+ piglit_report_subtest_result((pass) ? PIGLIT_PASS :
PIGLIT_FAIL, __FUNCTION__); \
+ return; \
+ } while (0)
/**/
Reviewed-by: Brian Paul
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [v5 09/12] tests: spec: EXT_image_dma_buf_import planar with single fd
On 05/03/2013 04:26 AM, Topi Pohjolainen wrote:
Create a planar image where all the planes are located in the
same dma buffer, just in different offsets.
Signed-off-by: Topi Pohjolainen
---
.../ext_image_dma_buf_import/CMakeLists.gles1.txt | 1 +
.../create_yuv420_same_fd.c| 135 +
2 files changed, 136 insertions(+)
create mode 100644 tests/spec/ext_image_dma_buf_import/create_yuv420_same_fd.c
[snip]
+enum piglit_result
+piglit_display(void)
+{
+ /**
+* All three planes for 4x4 can be represented using six four-byte rows,
+* first four representing 4x4 and the two following 2x2 subsampled
+* chroma planes each.
+*/
+ const unsigned char pixels[4 * 4 * 1 + 2 * 2 * 1 + 2 * 2 * 1];
+ struct piglit_dma_buf *buf;
+ unsigned stride;
+ unsigned offset;
+ int fd;
+ enum piglit_result res;
+
+ res = piglit_create_dma_buf(4, 6, 1, pixels, 4 * 1, &buf, &fd,
+ &stride, &offset);
+ if (res != PIGLIT_PASS) {
+ fprintf(stderr, "buffer creation failed\n");
+ return res;
+ }
+
+ return test_create_and_destroy(4, 4, buf, fd, 4, 2, 2,
+ offset,
+ offset + 4 * 4 * 1,
+ offset + 4 * 4 * 1 + 2 * 2 * 1) ?
+ PIGLIT_PASS : PIGLIT_FAIL;
The 'return' statement is difficult to read. It looks like it returns the value
of test_create_and_destroy(), but on closer inspection I see it converts that
to a piglit_result. Please simplify that with an 'if' statement or a temporary
result variable.
Other than that, the test looks good.
[snip]
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [v5 08/12] tests: spec: EXT_image_dma_buf_import planar with multiple fds
On 05/03/2013 04:26 AM, Topi Pohjolainen wrote:
Signed-off-by: Topi Pohjolainen
---
.../ext_image_dma_buf_import/CMakeLists.gles1.txt | 1 +
.../spec/ext_image_dma_buf_import/create_yuv420.c | 154 +
.../ext_image_dma_buf_fourcc.h | 1 +
3 files changed, 156 insertions(+)
create mode 100644 tests/spec/ext_image_dma_buf_import/create_yuv420.c
[snip]
+static bool
+test_create_and_destroy(unsigned w, unsigned h,
+ void *buf0, void *buf1, void *buf2,
+ int fd0, int fd1, int fd2,
+ unsigned stride0, unsigned stride1, unsigned stride2,
+ unsigned offset0, unsigned offset1, unsigned offset2)
+{
+ EGLImageKHR img;
+ EGLint attr[] = {
+ EGL_WIDTH, w,
+ EGL_HEIGHT, h,
+ EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_YUV420,
+ EGL_DMA_BUF_PLANE0_FD_EXT, fd0,
+ EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset0,
+ EGL_DMA_BUF_PLANE0_PITCH_EXT, stride0,
+ EGL_DMA_BUF_PLANE1_FD_EXT, fd1,
+ EGL_DMA_BUF_PLANE1_OFFSET_EXT, offset1,
+ EGL_DMA_BUF_PLANE1_PITCH_EXT, stride1,
+ EGL_DMA_BUF_PLANE2_FD_EXT, fd2,
+ EGL_DMA_BUF_PLANE2_OFFSET_EXT, offset2,
+ EGL_DMA_BUF_PLANE2_PITCH_EXT, stride2,
+ EGL_NONE
+ };
+
+ /**
+* The spec says:
+*
+* "If is EGL_LINUX_DMA_BUF_EXT, must be a valid
+* display, must be EGL_NO_CONTEXT, and must be
+* NULL, cast into the type EGLClientBuffer."
+*/
+ img = eglCreateImageKHR(eglGetCurrentDisplay(), EGL_NO_CONTEXT,
+ EGL_LINUX_DMA_BUF_EXT, (EGLClientBuffer)0, attr);
+
+ /**
+* Release the creator side of the buffers, EGL should have the
+* ownership now.
+*/
+ piglit_destroy_dma_buf(buf0);
+ piglit_destroy_dma_buf(buf1);
+ piglit_destroy_dma_buf(buf2);
+
+ /**
+* Upon failure EGL does _not_ take the ownership, and needs to close
+* the file descriptors here.
+*/
+ if (!piglit_check_egl_error(EGL_SUCCESS)) {
+ close(fd0);
+ close(fd1);
+ close(fd2);
+ return false;
+ }
+
+ if (!img) {
+ fprintf(stderr, "image creation succeed but returned NULL\n");
+ return false;
+ }
+
+ eglDestroyImageKHR(eglGetCurrentDisplay(), img);
Here, I think we need to check again that close() returns EBADF for each
fd. We need to know that eglDestroyImageKHR closes things properly when
the image contains multiple dma_bufs.
Other than that, this test looks good.
[snip]
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [v5 07/12] tests: spec: EXT_image_dma_buf_import fd ownership transfer
On 05/03/2013 04:26 AM, Topi Pohjolainen wrote:
Simple test checking that EGL can close the export file handle
and the creator can in turn can its reference.
Without a verb, the second phrase above is a bit too elliptical. How about
"can in turn $VERB its reference"?
Signed-off-by: Topi Pohjolainen
---
.../ext_image_dma_buf_import/CMakeLists.gles1.txt | 1 +
tests/spec/ext_image_dma_buf_import/close_buffer.c | 117 +
2 files changed, 118 insertions(+)
create mode 100644 tests/spec/ext_image_dma_buf_import/close_buffer.c
diff --git a/tests/spec/ext_image_dma_buf_import/close_buffer.c
b/tests/spec/ext_image_dma_buf_import/close_buffer.c
new file mode 100644
index 000..fb03d1b
--- /dev/null
+++ b/tests/spec/ext_image_dma_buf_import/close_buffer.c
@@ -0,0 +1,117 @@
+/**
+ * @file close_buffer.c
+ *
+ * From the EXT_image_dma_buf_import spec:
+ *
+ * "3. Does ownership of the file descriptor pass to the EGL library?
+ *
+ * ANSWER: If eglCreateImageKHR is successful, EGL assumes ownership of the
+ * file descriptors and is responsible for closing them."
+ *
+ *
+ * Here on checks that the creator of the buffer can drop its reference once
^^^ one
+ * it has given the buffer to EGL, i.e., after calling 'eglCreateImageKHR()'.
I don't see how this test verifies the above spec quote. I think you need,
as Eric suggested previously, to call close(fd) after eglDestroyImageKHR
and check that it returns EBADF.
+ */
+
+PIGLIT_GL_TEST_CONFIG_BEGIN
+
+ config.supports_gl_es_version = 10;
+
+PIGLIT_GL_TEST_CONFIG_END
+
+static bool
+test_create_and_destroy(void *buf, int fd, unsigned w, unsigned h,
+ unsigned stride, unsigned offset)
+{
+ EGLImageKHR img;
+ EGLint attr[] = {
+ EGL_WIDTH, w,
+ EGL_HEIGHT, h,
+ EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB,
+ EGL_DMA_BUF_PLANE0_FD_EXT, fd,
+ EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset,
+ EGL_DMA_BUF_PLANE0_PITCH_EXT, stride,
+ EGL_NONE
+ };
+
+ /**
+* The spec says:
+*
+* "If is EGL_LINUX_DMA_BUF_EXT, must be a valid
+* display, must be EGL_NO_CONTEXT, and must be
+* NULL, cast into the type EGLClientBuffer."
+*/
+ img = eglCreateImageKHR(eglGetCurrentDisplay(), EGL_NO_CONTEXT,
+ EGL_LINUX_DMA_BUF_EXT, (EGLClientBuffer)0, attr);
+
+ /**
+* Release the creator side of the buffer, EGL should have the
+* ownership now.
+*/
+ piglit_destroy_dma_buf(buf);
+
+ if (!piglit_check_egl_error(EGL_SUCCESS))
+ return false;
+
+ if (!img) {
+ fprintf(stderr, "image creation succeed but returned NULL\n");
+ return false;
+ }
+
+ eglDestroyImageKHR(eglGetCurrentDisplay(), img);
+
+ return piglit_check_egl_error(EGL_SUCCESS);
+}
+
+enum piglit_result
+piglit_display(void)
+{
+ const unsigned char pixels[2 * 2 * 4];
+ struct piglit_dma_buf *buf;
+ unsigned stride;
+ unsigned offset;
+ int fd;
+ enum piglit_result res;
+
+ res = piglit_create_dma_buf(2, 2, 4, pixels, 2 * 4, &buf, &fd, &stride,
+ &offset);
+ if (res != PIGLIT_PASS)
+ return res;
+
+ return test_create_and_destroy(buf, fd, 2, 2, stride, offset) ?
+ PIGLIT_PASS : PIGLIT_FAIL;
+}
+
+void
+piglit_init(int argc, char **argv)
+{
+ piglit_require_egl_extension("EGL_EXT_image_dma_buf_import");
+}
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [v5 02/12] framework: hardware independent interface for dma buffers
On Tue, May 7, 2013 at 4:13 PM, Chad Versace wrote: > >> +void >> +piglit_destroy_dma_buf(struct piglit_dma_buf *buf); > > > Is it safe to pass NULL to this function, like free()? Please document that. > > With that documentation, this patch is > Reviewed-by: Chad Versace > I also agree that this should be documented. For ease of use, I would recommend that this function be documented that passing NULL results in no action ( Similar to how free acts ). However, as it is Implemented right now, I do not believe the function drm_intel_bo_unreference used in actual implementation may not be safe to having NULL pointers passed. ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [v5 02/12] framework: hardware independent interface for dma buffers
On 05/07/2013 02:13 PM, Chad Versace wrote:
On 05/03/2013 04:26 AM, Topi Pohjolainen wrote:
In order to test the EXT_image_dma_buf_import, one needs a way for
creating dma buffers that can be imported to EGL and filling them
with data for the GL-stack to sample.
While dma buffer themselves are only defined for linux, the actual
writing of the buffers using CPU differs from hardware to another,
and possibly from window system to another. The intention here is
to push these details into the framework leaving the actual tests
environment independent.
v2 (Chad):
- replace 'void*' buffer type by 'struct piglit_dma_buf *'
Signed-off-by: Topi Pohjolainen
---
tests/util/piglit-framework-gl.c | 24 ++
tests/util/piglit-framework-gl.h | 21 +++
.../util/piglit-framework-gl/piglit_gl_framework.h | 9
3 files changed, 54 insertions(+)
diff --git a/tests/util/piglit-framework-gl.c b/tests/util/piglit-framework-gl.c
index 441e271..6ccc38d 100644
--- a/tests/util/piglit-framework-gl.c
+++ b/tests/util/piglit-framework-gl.c
@@ -162,3 +162,27 @@ piglit_set_reshape_func(void (*func)(int w, int h))
if (!gl_fw->set_reshape_func)
gl_fw->set_reshape_func(gl_fw, func);
}
+
+enum piglit_result
+piglit_create_dma_buf(unsigned w, unsigned h, unsigned cpp,
+ const void *src_data, unsigned src_stride,
+ struct piglit_dma_buf **buf, int *fd,
+ unsigned *stride, unsigned *offset)
+{
+*fd = 0;
+*stride = 0;
+*offset = 0;
+
+if (!gl_fw->create_dma_buf)
+return PIGLIT_SKIP;
+
+return gl_fw->create_dma_buf(w, h, cpp, src_data, src_stride, buf, fd,
+stride, offset);
+}
+
+void
+piglit_destroy_dma_buf(struct piglit_dma_buf *buf)
+{
+if (gl_fw->destroy_dma_buf)
+gl_fw->destroy_dma_buf(buf);
+}
diff --git a/tests/util/piglit-framework-gl.h b/tests/util/piglit-framework-gl.h
index 4406c1b..26be68c 100644
--- a/tests/util/piglit-framework-gl.h
+++ b/tests/util/piglit-framework-gl.h
@@ -241,4 +241,25 @@ void piglit_post_redisplay(void);
void piglit_set_keyboard_func(void (*func)(unsigned char key, int x, int y));
void piglit_set_reshape_func(void (*func)(int w, int h));
+struct piglit_dma_buf;
+
+/**
+ * Create buffer suitable for dma_buf importing and set its contents to the
+ * given data (src_data). Different hardware may have different alignment
+ * constraints and hence one can specify one stride for the source and get
+ * another for the final buffer to be given further to EGL.
+ * An opaque handle, file descriptor, stride and offset for the buffer are only
+ * returned upon success indicated by the return value PIGLIT_PASS, otherwise
+ * no buffer is created. In case the framework simply does not support dma
+ * buffers, the return value is PIGLIT_SKIP instead of PIGLIT_FAIL.
+ */
+enum piglit_result
+piglit_create_dma_buf(unsigned w, unsigned h, unsigned cpp,
+ const void *src_data, unsigned src_stride,
+ struct piglit_dma_buf **buf, int *fd,
+ unsigned *stride, unsigned *offset);
+void
+piglit_destroy_dma_buf(struct piglit_dma_buf *buf);
Is it safe to pass NULL to this function, like free()? Please document that.
With that documentation, this patch is
Reviewed-by: Chad Versace
By the way, if in response to other review you need to make minor changes to
this patch
(for example, changing the function signatures a little in response to Anholt's
request),
then the custom among Mesa devs is to add a versioned Reviewed-by tag. This
patch is v2, so like this:
Reviewed-by: Chad Versace (v2)
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [v5 04/12] tests: spec: EXT_image_dma_buf_import invalid hints
On 05/03/2013 02:23 PM, Eric Anholt wrote:
Topi Pohjolainen writes:
diff --git a/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
new file mode 100644
index 000..2499554
--- /dev/null
+++ b/tests/spec/ext_image_dma_buf_import/CMakeLists.gles1.txt
@@ -0,0 +1,15 @@
+#add_definitions(-DSOURCE_DIR="${piglit_SOURCE_DIR}/")
+
+include_directories(
+ ${OPENGL_INCLUDE_PATH}
+ )
+
+link_libraries(
+ ${OPENGL_gles1_LIBRARY}
+ ${OPENGL_egl_LIBRARY}
+ piglitutil_gles1
+ )
+
+piglit_add_executable(ext_image_dma_buf_import-invalid_hints invalid_hints.c)
I'd like to see conditional compile on HAVE_LIBDRM
I don't think it makes sense to condition the compile on HAVE_LIBDRM, because
Topi wrote these tests to be independent of drm drivers. If Linaro, for example,
implemented piglit_create/destroy_dma_buf for ARM, then the tests will run just
fine there. (I chose Linaro/ARM because together they drove the initial
implementation of this extension, and Linaro uses Piglit).
> and no
ext_image_dma_buf_fourcc.h.
Why kill ext_image_dma_buf_fourcc.h? Several of the following tests resuse the
formats defined there. Are you suggesting that the formats be redefined in
each *.c file?
+/**
+ * One re-uses the buffer for all the tests. Each test is expected to fail
+ * meaning that the ownership is not transferred to the EGL in any point.
+ */
+enum piglit_result
+piglit_display(void)
+{
+ const unsigned char pixels[2 * 2 * 4];
+ struct piglit_dma_buf *buf;
+ unsigned stride;
+ unsigned offset;
+ int fd;
+ bool pass;
+ enum piglit_result res;
+
+ res = piglit_create_dma_buf(2, 2, 4, pixels, 2 * 4, &buf, &fd, &stride,
+ &offset);
+ if (res != PIGLIT_PASS)
+ return res;
+
+ pass = test_invalid_hint(fd, 2, 2, stride, offset,
+ EGL_YUV_COLOR_SPACE_HINT_EXT, 0);
+ pass = test_invalid_hint(fd, 2, 2, stride, offset,
+ EGL_SAMPLE_RANGE_HINT_EXT, 0) && pass;
+ pass = test_invalid_hint(fd, 2, 2, stride, offset,
+ EGL_YUV_CHROMA_HORIZONTAL_SITING_HINT_EXT, 0) && pass;
+ pass = test_invalid_hint(fd, 2, 2, stride, offset,
+ EGL_YUV_CHROMA_VERTICAL_SITING_HINT_EXT, 0) && pass;
+
+ piglit_destroy_dma_buf(buf);
+
+ /* Close the file descriptor also, EGL does not have ownership */
+ close(fd);
Check that you don't get EBADF here to make sure that EGL didn't
accidentally steal ownership in its error path?
Either way, if the header thing is fixed,
Reviewed-by: Eric Anholt
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [v5 03/12] framework: support for creating dma buffers through libdrm
On 05/03/2013 04:26 AM, Topi Pohjolainen wrote:
In order to test EXT_image_dma_buf_import one needs the capability
of creating driver specific buffers. By probing the environment for
drm libraries one can decide for which drivers the support is to
be built.
v2 (first five according to Chad's advice):
- replace manual search for drm with 'pkg_check_modules()'
- move BATCH_SZ into intel specific part
- use ARRAY_SIZE
- fix faulty check for mem-allocation (drm_buf vs. buf)
- define the opaque type piglit_dma_buf declared in platform
independent interface instead of introducing new local type
(piglit_drm_dma_buf)
- use 'drm_intel_bo_subdata()' instead of mapping the buffers
for CPU
- also set the support for GBM in addition to X11
v3:
- fix a type (does -> doesn't)
- exclude intel driver entry points when the driver is not
present
Signed-off-by: Topi Pohjolainen
---
tests/util/CMakeLists.txt | 29 +++
.../util/piglit-framework-gl/piglit_drm_dma_buf.c | 222 +
.../util/piglit-framework-gl/piglit_drm_dma_buf.h | 37
.../piglit-framework-gl/piglit_gbm_framework.c | 5 +
.../piglit-framework-gl/piglit_x11_framework.c | 5 +
5 files changed, 298 insertions(+)
create mode 100644 tests/util/piglit-framework-gl/piglit_drm_dma_buf.c
create mode 100644 tests/util/piglit-framework-gl/piglit_drm_dma_buf.h
[snip]
+static int
+piglit_intel_buf_create(unsigned w, unsigned h, unsigned cpp,
+ const unsigned char *src_data, unsigned src_stride,
+ struct piglit_dma_buf *buf)
+{
+ unsigned i;
+ drm_intel_bo *bo;
+ unsigned stride = ALIGN(w * cpp, 4);
+ drm_intel_bufmgr *mgr = piglit_intel_bufmgr_get();
+
+ if (!mgr || src_stride > stride || h % 2)
+ return -1;
+
+ bo = drm_intel_bo_alloc(mgr, "piglit_dma_buf", h * stride, 4096);
+ if (!bo)
+ return -1;
+
+ for (i = 0; i < h; ++i) {
+ if (drm_intel_bo_subdata(bo, i * stride, src_stride,
+ src_data + i * src_stride)) {
+ drm_intel_bo_unreference(bo);
+ return -1;
+ }
+ }
+
+ buf->w = w;
+ buf->h = h;
+ buf->stride = stride;
For safety, buf->fd = 0.
+ buf->priv = bo;
+
+ return 0;
+}
[snip]
+void
+piglit_drm_destroy_dma_buf(struct piglit_dma_buf *buf)
Just a reminder: whatever behavior you choose and document for
piglit_destroy_dma_buf() in patch 2 needs to be honored here.
+{
+ const struct piglit_drm_driver *drv = piglit_drm_get_driver();
+
+ if (!drv)
+ return;
+
+ drv->destroy(buf);
+ free(buf);
+}
diff --git a/tests/util/piglit-framework-gl/piglit_gbm_framework.c
b/tests/util/piglit-framework-gl/piglit_gbm_framework.c
index 4df3861..67ca21f 100644
--- a/tests/util/piglit-framework-gl/piglit_gbm_framework.c
+++ b/tests/util/piglit-framework-gl/piglit_gbm_framework.c
@@ -27,6 +27,7 @@
#include "piglit-util-gl-common.h"
#include "piglit_gbm_framework.h"
+#include "piglit_drm_dma_buf.h"
static void
enter_event_loop(struct piglit_winsys_framework *winsys_fw)
@@ -69,6 +70,10 @@ piglit_gbm_framework_create(const struct
piglit_gl_test_config *test_config)
winsys_fw->show_window = show_window;
winsys_fw->enter_event_loop = enter_event_loop;
gl_fw->destroy = destroy;
+#ifdef HAVE_LIBDRM
+ gl_fw->create_dma_buf = piglit_drm_create_dma_buf;
+ gl_fw->destroy_dma_buf = piglit_drm_destroy_dma_buf;
+#endif
return gl_fw;
It's not sufficient to enable the dma_buf functions for only X11
and GBM. If someone runs a Piglit test under fbo mode on any window
system [1], or under Wayland [2], then the function pointers won't get set.
The function pointer assignment should be moved up to
piglit_gl_framework.c:piglit_gl_framework_init() so that all Piglit tests
and platforms benefit.
[1] See piglit_fbo_framework.c:piglit_fbo_framework_create
[2] See piglit_wl_framework.c:piglit_wl_framework_create
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [v5 02/12] framework: hardware independent interface for dma buffers
On 05/03/2013 04:26 AM, Topi Pohjolainen wrote:
In order to test the EXT_image_dma_buf_import, one needs a way for
creating dma buffers that can be imported to EGL and filling them
with data for the GL-stack to sample.
While dma buffer themselves are only defined for linux, the actual
writing of the buffers using CPU differs from hardware to another,
and possibly from window system to another. The intention here is
to push these details into the framework leaving the actual tests
environment independent.
v2 (Chad):
- replace 'void*' buffer type by 'struct piglit_dma_buf *'
Signed-off-by: Topi Pohjolainen
---
tests/util/piglit-framework-gl.c | 24 ++
tests/util/piglit-framework-gl.h | 21 +++
.../util/piglit-framework-gl/piglit_gl_framework.h | 9
3 files changed, 54 insertions(+)
diff --git a/tests/util/piglit-framework-gl.c b/tests/util/piglit-framework-gl.c
index 441e271..6ccc38d 100644
--- a/tests/util/piglit-framework-gl.c
+++ b/tests/util/piglit-framework-gl.c
@@ -162,3 +162,27 @@ piglit_set_reshape_func(void (*func)(int w, int h))
if (!gl_fw->set_reshape_func)
gl_fw->set_reshape_func(gl_fw, func);
}
+
+enum piglit_result
+piglit_create_dma_buf(unsigned w, unsigned h, unsigned cpp,
+ const void *src_data, unsigned src_stride,
+ struct piglit_dma_buf **buf, int *fd,
+ unsigned *stride, unsigned *offset)
+{
+ *fd = 0;
+ *stride = 0;
+ *offset = 0;
+
+ if (!gl_fw->create_dma_buf)
+ return PIGLIT_SKIP;
+
+ return gl_fw->create_dma_buf(w, h, cpp, src_data, src_stride, buf, fd,
+ stride, offset);
+}
+
+void
+piglit_destroy_dma_buf(struct piglit_dma_buf *buf)
+{
+ if (gl_fw->destroy_dma_buf)
+ gl_fw->destroy_dma_buf(buf);
+}
diff --git a/tests/util/piglit-framework-gl.h b/tests/util/piglit-framework-gl.h
index 4406c1b..26be68c 100644
--- a/tests/util/piglit-framework-gl.h
+++ b/tests/util/piglit-framework-gl.h
@@ -241,4 +241,25 @@ void piglit_post_redisplay(void);
void piglit_set_keyboard_func(void (*func)(unsigned char key, int x, int y));
void piglit_set_reshape_func(void (*func)(int w, int h));
+struct piglit_dma_buf;
+
+/**
+ * Create buffer suitable for dma_buf importing and set its contents to the
+ * given data (src_data). Different hardware may have different alignment
+ * constraints and hence one can specify one stride for the source and get
+ * another for the final buffer to be given further to EGL.
+ * An opaque handle, file descriptor, stride and offset for the buffer are only
+ * returned upon success indicated by the return value PIGLIT_PASS, otherwise
+ * no buffer is created. In case the framework simply does not support dma
+ * buffers, the return value is PIGLIT_SKIP instead of PIGLIT_FAIL.
+ */
+enum piglit_result
+piglit_create_dma_buf(unsigned w, unsigned h, unsigned cpp,
+ const void *src_data, unsigned src_stride,
+ struct piglit_dma_buf **buf, int *fd,
+ unsigned *stride, unsigned *offset);
+void
+piglit_destroy_dma_buf(struct piglit_dma_buf *buf);
Is it safe to pass NULL to this function, like free()? Please document that.
With that documentation, this patch is
Reviewed-by: Chad Versace
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
[Piglit] [PATCH v2] amd_performance_monitor: Fix multi-statement macro 'report'.
Fixes "Nesting level does not match indentation" defect reported by
Coverity.
Signed-off-by: Vinson Lee
---
tests/spec/amd_performance_monitor/api.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/spec/amd_performance_monitor/api.c
b/tests/spec/amd_performance_monitor/api.c
index 7b321cf..3205fc0 100644
--- a/tests/spec/amd_performance_monitor/api.c
+++ b/tests/spec/amd_performance_monitor/api.c
@@ -113,8 +113,10 @@ find_invalid_counter(unsigned *counters, int num_counters)
}
#define report(pass) \
-piglit_report_subtest_result((pass) ? PIGLIT_PASS : PIGLIT_FAIL,
__FUNCTION__); \
-return
+ do { \
+ piglit_report_subtest_result((pass) ? PIGLIT_PASS :
PIGLIT_FAIL, __FUNCTION__); \
+ return; \
+ } while (0)
/**/
--
1.8.2.1
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [v5 01/12] util: egl: support for skipping unsupported extension tests
On 05/03/2013 04:26 AM, Topi Pohjolainen wrote:
Signed-off-by: Topi Pohjolainen
---
tests/util/piglit-util-egl.c | 9 +
tests/util/piglit-util-egl.h | 5 +
2 files changed, 14 insertions(+)
diff --git a/tests/util/piglit-util-egl.c b/tests/util/piglit-util-egl.c
index 1087429..5a9f0a9 100644
--- a/tests/util/piglit-util-egl.c
+++ b/tests/util/piglit-util-egl.c
@@ -84,3 +84,12 @@ piglit_is_egl_extension_supported(EGLDisplay egl_dpy, const
char *name)
return piglit_is_extension_in_string(egl_extension_list, name);
}
+
+void piglit_require_egl_extension(const char *name)
+{
+ if (!piglit_is_egl_extension_supported(eglGetCurrentDisplay(), name)) {
+ printf("Test requires %s\n", name);
+ piglit_report_result(PIGLIT_SKIP);
+ exit(1);
+ }
+}
piglit_report_result() already calls exit(). Remove the call to exit(1) here
and it's
Reviewed-by: Chad Versace
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
