[Mesa-dev] Z24S8 textures on nv04/nv10

[Ilia Mirkin]

On Thu, Mar 13, 2014 at 6:55 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> 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.

Hmmmm... OTOH, that's perhaps more correctly interpreted as "You can
pass GL_DEPTH_STENCIL data to a GL_DEPTH_COMPONENT texture". Which
would in turn mean that EXT_packed_depth_stencil does require
GL_DEPTH_STENCIL texture support...

Anyone else have opinions on this?

  -ilia