egl_ext_device_drm: don't fail the test if open("/dev/dri/cardX") errors

Submitted by Emil Velikov on Oct. 3, 2018, 12:21 p.m.

Details

Message ID 20181003122156.8253-1-emil.l.velikov@gmail.com
State New
Headers show
Series "egl_ext_device_drm: don't fail the test if open("/dev/dri/cardX") errors" ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Emil Velikov Oct. 3, 2018, 12:21 p.m.
From: Emil Velikov <emil.velikov@collabora.com>

As pointed out by Mathias opening the card node can fail in some cases.
In boils down to a) the node is owned by root:video b) by default, the
user is not part of the video group c) logind dynamically allows any
user to open the node, once a user has logged-in.

Thus if we use a remote machine accessible only over ssh, the test will
fail. A fairly common setup that one could use for their CI.

Demote the failure to a warning.

Cc: Mathias Fröhlich <mathias.froehlich@web.de>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c b/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c
index 537c8b60f..2236783c8 100644
--- a/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c
+++ b/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c
@@ -162,7 +162,9 @@  main(void)
 		if (fd < 0) {
 			printf("Failed to open drm device file %s: %s\n",
 				devstring, strerror(errno));
-			piglit_report_result(PIGLIT_FAIL);
+			printf("Make sure you have permissions to open %s\n");
+			result = PIGLIT_WARN;
+			continue;
 		}
 #ifndef EGL_DRM_MASTER_FD_EXT
 #define EGL_DRM_MASTER_FD_EXT                   0x333C

Comments

On Fri, 5 Oct 2018 at 19:13, Dylan Baker <dylan@pnwbakers.com> wrote:
>
> In other cases like this we return skip instead of warn.
Can you be more specific about those "other cases like these"? It's
the core part of your argument so w/o a quote/reference it's
impossible to make a decision.

> I think that it should
> be skip because:
>
> 1) warn doesn't have any real meaning
Not a usual black-and-white one, but it does.
WARN is something that should be addressed (looked at), but isn't
considered an blocking issue.

> 2) we couldn't actually run the test so there isn't a result
>
There is no clear "one test" here, but a collection of small subtests
- more or less one for each piglit_report_result.
You don't want to skip the whole thing, because a section is no applicable.

HTH
Emil
On Tue, 9 Oct 2018 at 16:50, Dylan Baker <dylan@pnwbakers.com> wrote:
>
> Quoting Emil Velikov (2018-10-09 05:35:39)
> > On Fri, 5 Oct 2018 at 19:13, Dylan Baker <dylan@pnwbakers.com> wrote:
> > >
> > > In other cases like this we return skip instead of warn.
> > Can you be more specific about those "other cases like these"? It's
> > the core part of your argument so w/o a quote/reference it's
> > impossible to make a decision.
>
> For example, if a test binary doesn't exist or isn't executable.
>
I fear you're confusing things. There's extension with it's requirements.
There are parts which are optional and the extension interacts with,
when available.

> > > I think that it should
> > > be skip because:
> > >
> > > 1) warn doesn't have any real meaning
> > Not a usual black-and-white one, but it does.
> > WARN is something that should be addressed (looked at), but isn't
> > considered an blocking issue.
>
> Piglit's warn is a bad particularly because it doesn't have a concise meaning,
> it means "something might have happened". Tests results should not be ambiguous
> like that. pass, fail, crash, and skip all have clear concise meanings.
>
I'm not sure that this is the best place to discuss WARN.
Please start a separate thread, instead.

> > > 2) we couldn't actually run the test so there isn't a result
> > >
> > There is no clear "one test" here, but a collection of small subtests
> > - more or less one for each piglit_report_result.
> > You don't want to skip the whole thing, because a section is no applicable.
>
> In that case, I think a more correct solution would be to use
> piglit_report_subtest_result in the test so that each subtest returns an
> individual result.
>
$git grep piglit_report_subtest_result -- tests/egl/ | wc -l
0

So I'm consistent ... despite how good or bad that is.
If you want to go ahead and fix that up, please be my guest.

Personally, I've wanted to fix multiple things in piglit for ages,
sadly I haven't had time to do much apart from the odd test.

Thanks
Emil
On Wed, 3 Oct 2018 at 13:27, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> From: Emil Velikov <emil.velikov@collabora.com>
>
> As pointed out by Mathias opening the card node can fail in some cases.
> In boils down to a) the node is owned by root:video b) by default, the
> user is not part of the video group c) logind dynamically allows any
> user to open the node, once a user has logged-in.
>
> Thus if we use a remote machine accessible only over ssh, the test will
> fail. A fairly common setup that one could use for their CI.
>
> Demote the failure to a warning.
>
> Cc: Mathias Fröhlich <mathias.froehlich@web.de>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c b/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c
> index 537c8b60f..2236783c8 100644
> --- a/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c
> +++ b/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c
> @@ -162,7 +162,9 @@ main(void)
>                 if (fd < 0) {
>                         printf("Failed to open drm device file %s: %s\n",
>                                 devstring, strerror(errno));
> -                       piglit_report_result(PIGLIT_FAIL);
> +                       printf("Make sure you have permissions to open %s\n");
> +                       result = PIGLIT_WARN;
> +                       continue;
>                 }
>  #ifndef EGL_DRM_MASTER_FD_EXT
>  #define EGL_DRM_MASTER_FD_EXT                   0x333C
> --

Humble ping?

-Emil
Good morning,

On Wednesday, 3 October 2018 14:21:56 CEST Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> As pointed out by Mathias opening the card node can fail in some cases.
> In boils down to a) the node is owned by root:video b) by default, the
> user is not part of the video group c) logind dynamically allows any
> user to open the node, once a user has logged-in.
> 
> Thus if we use a remote machine accessible only over ssh, the test will
> fail. A fairly common setup that one could use for their CI.
> 
> Demote the failure to a warning.
> 
> Cc: Mathias Fröhlich <mathias.froehlich@web.de>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c b/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c
> index 537c8b60f..2236783c8 100644
> --- a/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c
> +++ b/tests/egl/spec/egl_ext_device_drm/egl_ext_device_drm.c
> @@ -162,7 +162,9 @@ main(void)
>  		if (fd < 0) {
>  			printf("Failed to open drm device file %s: %s\n",
>  				devstring, strerror(errno));
> -			piglit_report_result(PIGLIT_FAIL);
> +			printf("Make sure you have permissions to open %s\n");
> +			result = PIGLIT_WARN;
> +			continue;

Sorry for being picky, but you are missing the eglTerminate on dpy1 with that continue.

With that eglTerminate:
Reviewed-by: Mathias Fröhlich <Mathias.Froehlich@web.de>


The discussion about the WARN/SKIP. I don't insist on one or the other, but my feeling tells
me also that this is more like a skip here. For the basic functionality of the extension
it is the standard mode of operation that you are NOT logged into the console where
you would have access to the master node. So, there is nothing to warn about that.
Ok, it's bad that the master node is returned instead of the one used
which would have actually worked. But that is a spec/design problem that should not be
'warned' about in a any piglit test IMO.

plenty thanks and best

Mathias

>  		}
>  #ifndef EGL_DRM_MASTER_FD_EXT
>  #define EGL_DRM_MASTER_FD_EXT                   0x333C
>