[Mesa-dev] [PATCH v2 06/23] mesa: Fix incorrect assertion in init_teximage_fields_ms

[Iago Toral]

On Mon, 2014-12-01 at 13:25 -0500, Ilia Mirkin wrote:
> On Mon, Dec 1, 2014 at 6:04 AM, Iago Toral Quiroga <itoral at igalia.com> wrote:
> > _BaseFormat is a GLenum (unsigned int) so testing if its value is
> > greater than 0 to detect the cases where _mesa_base_tex_format
> > returns -1 doesn't work.
> >
> > Fixing the assertion breaks the arb_texture_view-lifetime-format
> > piglit test on nouveau, since that test calls
> > _mesa_base_tex_format with GL_R16F with a context that does not
> > have ARB_texture_float, so it returns -1 for the BaseFormat, which
> > was not being catched properly by the ASSERT in init_teximage_fields_ms
> > until now.
> 
> Sorry to hijack the thread, but... can you elaborate why this would
> fail on nouveau but not on i965? Does st/mesa do something differently
> wrt exposing ARB_texture_float vs i965? Does nouveau do something
> wrong?

Hi Illia, I didn't investigate the source of the problem in Nouveau or
why this is different than the other drivers, however I can give more
details:

We are running piglit against i965, llvmpipe, classic swrast, nouveu and
radeon, but we only hit the problem with nouveau. The code that triggers
the assertion is _mesa_base_tex_format (teximage.c), which is called
with internalFormat = GL_R16F, and then, in that function there is this
code:

if (ctx->Extensions.ARB_texture_rg) {
   switch (internalFormat) {
   case GL_R16F:
   case GL_R32F:
      if (!ctx->Extensions.ARB_texture_float)
         break;
      return GL_RED; 
...

On i965, the code returns GL_RED, but in Nouveau it hits the break
because ctx->Extensions.ARB_texture_float is false in this case (and
later will return -1 after being unable to find a proper base format).

In the case of Intel, ARB_texture_float is always enabled. In the case
of Gallium I see this in st_extensions.c:

static const struct st_extension_format_mapping rendertarget_mapping[] =
{
      { { o(ARB_texture_float) },
        { PIPE_FORMAT_R32G32B32A32_FLOAT,
          PIPE_FORMAT_R16G16B16A16_FLOAT } },
...

so if I understand that right, for ARB_texture_float to be activated I
need PIPE_FORMAT_R32G32B32A32_FLOAT and PIPE_FORMAT_R16G16B16A16_FLOAT
to be supported, so I guess that at least one of these is not supported
in the nVidia hardware I am using (NVIDIA GT218 [ION]). If that is not
the problem, then I guess it would need further investigation, but I
don't see any other places where Gallium or nouveau set that extension.

Iago

> > ---
> >  src/mesa/main/teximage.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
> > index e238863..c9658c1 100644
> > --- a/src/mesa/main/teximage.c
> > +++ b/src/mesa/main/teximage.c
> > @@ -1313,7 +1313,7 @@ init_teximage_fields_ms(struct gl_context *ctx,
> >
> >     target = img->TexObject->Target;
> >     img->_BaseFormat = _mesa_base_tex_format( ctx, internalFormat );
> > -   ASSERT(img->_BaseFormat > 0);
> > +   ASSERT(img->_BaseFormat != -1);
> >     img->InternalFormat = internalFormat;
> >     img->Border = border;
> >     img->Width = width;
> > --
> > 1.9.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>