[Mesa-dev] Z24S8 textures on nv04/nv10

[Ilia Mirkin]

On Thu, Mar 13, 2014 at 6:43 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Thu, Mar 13, 2014 at 6:42 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>>
>>> Hi Francisco,
>>>
>>> I'm looking at some piglit failures on nv10... full run at
>>>
>>> http://people.freedesktop.org/~imirkin/nv10-comparison/problems.html
>>>
>>> The specific one is:
>>>
>>> http://people.freedesktop.org/~imirkin/nv10-comparison/nv18-imirkin/spec/!OpenGL%201.1/copyteximage%201D.html
>>>
>>> Which fails in nouveau_choose_tex_format for GL_DEPTH24_STENCIL8. And
>>> indeed, that case isn't handled. Looking through the #defines, I see a
>>> NV20_3D_TEX_FORMAT_FORMAT_Z24 but not for the earlier versions. And
>>> even in the nv20 case, it's not actually used in the driver.
>>>
>>> Does that mean that EXT_packed_depth_stencil should never have been
>>> enabled on nouveau? I do see that it's a valid renderbuffer format
>>> though. [It is now always enabled, since commit a92b9e60, but at the
>>> time of the commit, it was also enabled for everything.
>>>
>>> Any advice?
>>>
>>
>> AFAICT EXT_packed_depth_stencil only requires the implementation to
>> support GL_DEPTH_STENCIL for texturing if ARB_depth_texture is supported
>> in addition -- which, as you guessed correctly, could be supported on
>> nv20 or higher.
>>
>> Shouldn't the piglit test be checking if the implementation supports
>> depth texturing before doing that?
>
> Mmmaybe. But that's not such a big deal -- worst case, the piglit
> should get a gl error of some sort. Sounds like the real issue is that
> mesa core is letting the values through to the driver. I'll re-read
> EXT_packed_depth_stencil more carefully, I didn't see the
> ARB_depth_texture dependency, but I only scanned it very quickly.

Aha, and in _mesa_base_tex_format, we have

   if (ctx->Extensions.ARB_depth_texture) {
      switch (internalFormat) {
         case GL_DEPTH_COMPONENT:
         case GL_DEPTH_COMPONENT16:
         case GL_DEPTH_COMPONENT24:
         case GL_DEPTH_COMPONENT32:
            return GL_DEPTH_COMPONENT;
         default:
            ; /* fallthrough */
      }
   }

followed by

   switch (internalFormat) {
   case GL_DEPTH_STENCIL:
   case GL_DEPTH24_STENCIL8:
      return GL_DEPTH_STENCIL;
   default:
      ; /* fallthrough */
   }

And I think you're right about these only being available for ARB_depth_texture:

    If ARB_depth_texture or SGIX_depth_texture is
    supported, GL_DEPTH_STENCIL_EXT/GL_UNSIGNED_INT_24_8_EXT data can
    also be used for textures; this provides a more efficient way to
    supply data for a 24-bit depth texture.

This doesn't reference GL_DEPTH24_STENCIL8 explicitly, but I can only
assume that's a spec bug.

I'll send a patch.

  -ilia