Re: [Piglit] [PATCH piglit] egl: add test for EGL_MESA_query_driver
On Tue, 22 Jan 2019 at 15:29, Eric Engestrom wrote: > > I'd add a "TODO: add basic xml config validation/printing based on the > > example in the spec." > > That would be good in theory, but: > - we'd need to write a proper DTD, which we don't have right now > - xml parsing is not trivial (we'd need a library, it's not something > one should do by hand), and I'm not sure we want to add that kind of > beast to piglit. > > If we decide to do this, I'm happy to write the DTD. I think we could have left this for a later stage - hence the TODO. If you prefer to do it now (and have time of course) sure. Thanks Emil ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH piglit] egl: add test for EGL_MESA_query_driver
On Tuesday, 2019-01-22 15:29:44 +, Eric Engestrom wrote:
> On Tuesday, 2019-01-22 13:43:07 +, Emil Velikov wrote:
> > Hi Eric,
> >
> > Thanks for writing this up.
> >
> > On Tue, 22 Jan 2019 at 12:43, Eric Engestrom
> > wrote:
> > >
> > > Cc: Veluri Mithun
> > > Cc: Emil Velikov
> > > Cc: Rob Clark
> > > Cc: Nicolai Hähnle
> > > Signed-off-by: Eric Engestrom
> > > ---
> > > The extension is currently in development in this MR:
> > > https://gitlab.freedesktop.org/mesa/mesa/merge_requests/47
> > >
> > > Veluri will send updated versions of this test if the spec changes.
> >
> > > +
> > > + piglit_require_egl_extension(EGL_NO_DISPLAY,
> > > "EGL_MESA_query_driver");
> > > +
> > AFAICT we need an valid/initialized display here. EGL_NO_DISPLAY is
> > for _client_ EGL extensions and EGL_MESA_query_driver is not one.
>
> Good point, indeed.
>
> >
> >
> > > + egl_error = eglGetError();
> > > + if (driver_name || egl_error != EGL_BAD_DISPLAY) {
> > > + printf("eglGetDisplayDriverName() should have failed with
> > > EGL_BAD_DISPLAY\n");
> > > + printf("Instead, it returned %s and with error %s\n",
> > > + driver_name, piglit_get_egl_error_name(egl_error));
> > > + piglit_report_result(PIGLIT_FAIL);
> > > + }
> > > +
> > This hunk seems to be an open-coded piglit_check_egl_error(), use the
> > helper instead?
> > Suggestion applies for the whole file.
>
> Yup, didn't know about the helper :)
>
> >
> > > + printf("Driver name: %s\n", driver_name);
> > > + printf("Driver config: %s\n", driver_config);
> > > + free(driver_config);
> > > +
> > I'd add a "TODO: add basic xml config validation/printing based on the
> > example in the spec."
>
> That would be good in theory, but:
> - we'd need to write a proper DTD, which we don't have right now
> - xml parsing is not trivial (we'd need a library, it's not something
> one should do by hand), and I'm not sure we want to add that kind of
> beast to piglit.
>
> If we decide to do this, I'm happy to write the DTD.
Actually, after thinking about this a bit more, I think we really should
have a DTD, and it should be part of the extension spec.
I've started writing it, but I don't have much time left today. I'll
send it once it's done.
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH piglit] egl: add test for EGL_MESA_query_driver
On Tuesday, 2019-01-22 13:43:07 +, Emil Velikov wrote:
> Hi Eric,
>
> Thanks for writing this up.
>
> On Tue, 22 Jan 2019 at 12:43, Eric Engestrom wrote:
> >
> > Cc: Veluri Mithun
> > Cc: Emil Velikov
> > Cc: Rob Clark
> > Cc: Nicolai Hähnle
> > Signed-off-by: Eric Engestrom
> > ---
> > The extension is currently in development in this MR:
> > https://gitlab.freedesktop.org/mesa/mesa/merge_requests/47
> >
> > Veluri will send updated versions of this test if the spec changes.
>
> > +
> > + piglit_require_egl_extension(EGL_NO_DISPLAY,
> > "EGL_MESA_query_driver");
> > +
> AFAICT we need an valid/initialized display here. EGL_NO_DISPLAY is
> for _client_ EGL extensions and EGL_MESA_query_driver is not one.
Good point, indeed.
>
>
> > + egl_error = eglGetError();
> > + if (driver_name || egl_error != EGL_BAD_DISPLAY) {
> > + printf("eglGetDisplayDriverName() should have failed with
> > EGL_BAD_DISPLAY\n");
> > + printf("Instead, it returned %s and with error %s\n",
> > + driver_name, piglit_get_egl_error_name(egl_error));
> > + piglit_report_result(PIGLIT_FAIL);
> > + }
> > +
> This hunk seems to be an open-coded piglit_check_egl_error(), use the
> helper instead?
> Suggestion applies for the whole file.
Yup, didn't know about the helper :)
>
> > + printf("Driver name: %s\n", driver_name);
> > + printf("Driver config: %s\n", driver_config);
> > + free(driver_config);
> > +
> I'd add a "TODO: add basic xml config validation/printing based on the
> example in the spec."
That would be good in theory, but:
- we'd need to write a proper DTD, which we don't have right now
- xml parsing is not trivial (we'd need a library, it's not something
one should do by hand), and I'm not sure we want to add that kind of
beast to piglit.
If we decide to do this, I'm happy to write the DTD.
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH piglit] egl: add test for EGL_MESA_query_driver
Hi Eric,
Thanks for writing this up.
On Tue, 22 Jan 2019 at 12:43, Eric Engestrom wrote:
>
> Cc: Veluri Mithun
> Cc: Emil Velikov
> Cc: Rob Clark
> Cc: Nicolai Hähnle
> Signed-off-by: Eric Engestrom
> ---
> The extension is currently in development in this MR:
> https://gitlab.freedesktop.org/mesa/mesa/merge_requests/47
>
> Veluri will send updated versions of this test if the spec changes.
> +
> + piglit_require_egl_extension(EGL_NO_DISPLAY, "EGL_MESA_query_driver");
> +
AFAICT we need an valid/initialized display here. EGL_NO_DISPLAY is
for _client_ EGL extensions and EGL_MESA_query_driver is not one.
> + egl_error = eglGetError();
> + if (driver_name || egl_error != EGL_BAD_DISPLAY) {
> + printf("eglGetDisplayDriverName() should have failed with
> EGL_BAD_DISPLAY\n");
> + printf("Instead, it returned %s and with error %s\n",
> + driver_name, piglit_get_egl_error_name(egl_error));
> + piglit_report_result(PIGLIT_FAIL);
> + }
> +
This hunk seems to be an open-coded piglit_check_egl_error(), use the
helper instead?
Suggestion applies for the whole file.
> + printf("Driver name: %s\n", driver_name);
> + printf("Driver config: %s\n", driver_config);
> + free(driver_config);
> +
I'd add a "TODO: add basic xml config validation/printing based on the
example in the spec."
Thanks
-Emil
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
