[Mesa-dev] Z24S8 textures on nv04/nv10

[Ilia Mirkin]

On Thu, Mar 13, 2014 at 7:36 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>
>> 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...
>>
>
> Why would they do something like that?  If ARB_depth_texture is not
> supported, I don't see how it would make sense to forbid "passing" Z24S8
> data to a texture, but still require support for it as internal format.

I think it's more like saying that if you _do_ have ARB_depth_texture,
then even though the ARB_depth_texture spec says nothing about letting
data passed to glTexImage2D being in Z24X8 format, you can still do
that for a texture with a DEPTH_COMPONENT internal format, instead of
it being in the unaligned Z24 format.

>
> I just checked what the nvidia blob does: It exposes
> EXT_packed_depth_stencil on nv1x and it doesn't support depth texturing,
> just like we do.

If this is a live system (rather than a saved-off glxinfo), mind
running the piglit test that I referenced originally and see what it
says for the GL_DEPTH_STENCIL bit? Perhaps they do something clever to
enable depth texturing...

  -ilia