[Piglit] [PATCH] fbo: add a few quirk-tests for format-emulation

Erik Faye-Lund erik.faye-lund at collabora.com
Fri Oct 26 11:35:31 UTC 2018


On Thu, 2018-10-25 at 14:39 +0100, Emil Velikov wrote:
> On Thu, 25 Oct 2018 at 11:07, Erik Faye-Lund
> <erik.faye-lund at collabora.com> wrote:
> > 
> > On Wed, 2018-10-24 at 16:32 +0100, Emil Velikov wrote:
> > > Hi Erik,
> > > 
> > > A bit of an open question, do we want this test on GLES? If yes,
> > > see
> > > the GLES
> > > notes below.
> > > 
> > 
> > Right. I think I'm fine with only OpenGL for now, as this is a
> > quirk-
> > test.
> > 
> > But yeah, it's totally valuable for GLES as well, and as you point
> > our, should be pretty easy to port there. I'll leave that for the
> > future for now, though.
> > 
> > > Alternatively, I've only spotted couple small nitpicks.
> > > 
> > > On Thu, 20 Sep 2018 at 19:19, Erik Faye-Lund
> > > <erik.faye-lund at collabora.com> wrote:
> > > > 
> > > > Drivers who implement GL_ALPHA textures by swizzling a GL_R
> > > > texture
> > > > needs to be prepared to offer multiple blend-colors, otherwise
> > > > they
> > > > will produce the wrong result, because the blend-color will
> > > > also
> > > > need per-format swizzling.
> > > > 
> > > > Similarly, when drivers implement GL_RGB with GL_RGBA and
> > > > adjust
> > > > the
> > > > blend-factors, they need to have separate blend-factors for all
> > > > framebuffer attachments or the results will be wrong.
> > > > 
> > > > This reveals a couple of bugs in virgl, where these currently
> > > > fail.
> > > > 
> > > > Signed-off-by: Erik Faye-Lund <erik.faye-lund at collabora.com>
> > > > ---
> > > >  tests/fbo/CMakeLists.gl.txt            |   1 +
> > > >  tests/fbo/fbo-blending-format-quirks.c | 175
> > > > +++++++++++++++++++++++++
> > > >  tests/opengl.py                        |   1 +
> > > >  3 files changed, 177 insertions(+)
> > > >  create mode 100644 tests/fbo/fbo-blending-format-quirks.c
> > > > 
> > > > diff --git a/tests/fbo/CMakeLists.gl.txt
> > > > b/tests/fbo/CMakeLists.gl.txt
> > > > index d594c24d3..1a1a60765 100644
> > > > --- a/tests/fbo/CMakeLists.gl.txt
> > > > +++ b/tests/fbo/CMakeLists.gl.txt
> > > > @@ -29,6 +29,7 @@ piglit_add_executable (fbo-blit fbo-blit.c)
> > > >  piglit_add_executable (fbo-blit-d24s8 fbo-blit-d24s8.c)
> > > >  piglit_add_executable (fbo-blit-stretch fbo-blit-stretch.cpp)
> > > >  piglit_add_executable (fbo-blending-formats fbo-blending-
> > > > formats.c)
> > > > +piglit_add_executable (fbo-blending-format-quirks fbo-
> > > > blending-
> > > > format-quirks.c)
> > > 
> > > GLES:
> > > Add a trivial .gles1.txt and/or gles2.txt file. You can use the
> > > ext_image_dma_buf_import ones for reference.
> > > 
> > > 
> > > >  piglit_add_executable (fbo-blending-snorm fbo-blending-
> > > > snorm.c)
> > > >  piglit_add_executable (fbo-colormask-formats fbo-colormask-
> > > > formats.c)
> > > >  piglit_add_executable (fbo-copypix fbo-copypix.c)
> > > > diff --git a/tests/fbo/fbo-blending-format-quirks.c
> > > > b/tests/fbo/fbo-blending-format-quirks.c
> > > > new file mode 100644
> > > > index 000000000..f6865d42b
> > > > --- /dev/null
> > > > +++ b/tests/fbo/fbo-blending-format-quirks.c
> > > > +#include "piglit-util-gl.h"
> > > > +
> > > > +PIGLIT_GL_TEST_CONFIG_BEGIN
> > > > +
> > > > +       config.supports_gl_compat_version = 10;
> > > > +
> > > 
> > > GLES:
> > > #ifdef PIGLIT_USE_OPENGL
> > >         config.supports_gl_compat_version = 10;
> > > #else
> > >         config.supports_gl_es_version = ...
> > > #endif
> > > 
> > > 
> > > > +enum piglit_result piglit_display(void)
> > > > +{
> > > > +       int i;
> > > > +        enum piglit_result result, end_result = PIGLIT_PASS;
> > > > +        bool all_skip = true;
> > > > +
> > > > +       struct {
> > > 
> > > static const
> > > 
> > 
> > Fixed, thanks.
> > 
> > > > +               const char *name;
> > > > +               GLenum formats[2];
> > > > +               GLenum factors[2];
> > > > +               float expect[2][4];
> > > > +       } cases[] = {
> > > 
> > > 
> > > > +
> > > > +void piglit_init(int argc, char **argv)
> > > > +{
> > > 
> > >         piglit_require_extension("GL_EXT_framebuffer_object");
> > > 
> 
> You need this ^^
> 

Right, thanks. Fixed.

However, I used tests/fbo/fbo-blending-formats.c as a reference, and
this doesn't seem to do this either. So perhaps a fixup there is in
order as well?

Also, I'm curious; EXT_framebuffer_object is an old, arguably broken
spec. Why are we requiring it rather than ARB_framebuffer_object? Is
the assumption that everyone who implements ARB_framebuffer_object will
also implement EXT_framebuffer_object?

If I were to create a new driver today, I think I would drop
EXT_framebuffer_object, because it creates an ambiguity if framebuffer
objects are shared among contexts or not... So I'm not sure it's a good
assumption.

Better, I think, would be to require *either*.

I'm leaving the test as-is with your suggested change, though. I think
this is a much bigger question, which can be dealt with independently.

> 
> > > GLES:
> > > #ifdef PIGLIT_USE_OPENGL
> > >         piglit_require_extension("GL_EXT_framebuffer_object");
> > > #else
> > >         piglit_require_extension("GL_OES_framebuffer_object");
> > >         // Or use GLES 2.0 above and drop the extension check
> > >         // Note; the EXT suffix will need need to be changed to
> > > OES
> > > or dropped.
> > > #endif
> > > 
> > > 
> > > With the nitpicks, ES is something that can be done when needed.
> > > Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
> > 
> > OK, cool. Since the only change is adding "static const", perhaps I
> > can
> > just push out the result as-is?
> > 
> 
> Add the EXT_fbo check and feel free to land w/o re-sending.
> 
> -Emil



More information about the Piglit mailing list