[Mesa-dev] [PATCH] r600g: fix z/stencil texture creation

[Marek Olšák]

On Fri, Jun 15, 2012 at 8:40 PM, Brian Paul <brian.e.paul at gmail.com> wrote:
> On Fri, Jun 15, 2012 at 12:29 PM, Marek Olšák <maraeo at gmail.com> wrote:
>> Hi Jerome,
>>
>> Your patch somehow fixes all regressions, but it's not correct. Please
>> see below.
>>
>> On Fri, Jun 15, 2012 at 5:25 PM,  <j.glisse at gmail.com> wrote:
>>> From: Jerome Glisse <jglisse at redhat.com>
>>>
>>> z or stencil texture should not be created with the z/stencil
>>> flags for surface creation as they are intended to be bound
>>> as texture.
>>>
>>> Signed-off-by: Jerome Glisse <jglisse at redhat.com>
>>> ---
>>>  src/gallium/drivers/r600/r600_texture.c |   34 +++++++++++++++++-------------
>>>  1 files changed, 19 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/r600/r600_texture.c b/src/gallium/drivers/r600/r600_texture.c
>>> index 5b15990..517f273 100644
>>> --- a/src/gallium/drivers/r600/r600_texture.c
>>> +++ b/src/gallium/drivers/r600/r600_texture.c
>>> @@ -237,7 +237,8 @@ static void r600_texture_set_array_mode(struct pipe_screen *screen,
>>>
>>>  static int r600_init_surface(struct radeon_surface *surface,
>>>                             const struct pipe_resource *ptex,
>>> -                            unsigned array_mode)
>>> +                            unsigned array_mode, unsigned is_transfer,
>>> +                            unsigned is_texture)
>>
>> We use the "boolean" type in gallium for bools. You also seem to need
>> only one flag, not two, because you don't make any distinction between
>> is_transfer and is_texture further in the code. The "is_texture" flag
>> is not very useful anyway, see below.
>>
>>>  {
>>>        surface->npix_x = ptex->width0;
>>>        surface->npix_y = ptex->height0;
>>> @@ -298,7 +299,7 @@ static int r600_init_surface(struct radeon_surface *surface,
>>>        if (ptex->bind & PIPE_BIND_SCANOUT) {
>>>                surface->flags |= RADEON_SURF_SCANOUT;
>>>        }
>>> -       if (util_format_is_depth_and_stencil(ptex->format)) {
>>> +       if (util_format_is_depth_and_stencil(ptex->format) && !is_transfer && !is_texture) {
>>>                surface->flags |= RADEON_SURF_ZBUFFER;
>>>                surface->flags |= RADEON_SURF_SBUFFER;
>>>        }
>>> @@ -316,11 +317,6 @@ static int r600_setup_surface(struct pipe_screen *screen,
>>>        unsigned i;
>>>        int r;
>>>
>>> -       if (util_format_is_depth_or_stencil(rtex->real_format)) {
>>> -               rtex->surface.flags |= RADEON_SURF_ZBUFFER;
>>> -               rtex->surface.flags |= RADEON_SURF_SBUFFER;
>>> -       }
>>> -
>>>        r = rscreen->ws->surface_init(rscreen->ws, &rtex->surface);
>>>        if (r) {
>>>                return r;
>>> @@ -572,7 +568,8 @@ r600_texture_create_object(struct pipe_screen *screen,
>>>        r600_setup_miptree(screen, rtex, array_mode);
>>>        if (rscreen->use_surface_alloc) {
>>>                rtex->surface = *surface;
>>> -               r = r600_setup_surface(screen, rtex, array_mode, pitch_in_bytes_override);
>>> +               r = r600_setup_surface(screen, rtex, array_mode,
>>> +                                      pitch_in_bytes_override);
>>>                if (r) {
>>>                        FREE(rtex);
>>>                        return NULL;
>>> @@ -642,7 +639,9 @@ struct pipe_resource *r600_texture_create(struct pipe_screen *screen,
>>>                }
>>>        }
>>>
>>> -       r = r600_init_surface(&surface, templ, array_mode);
>>> +       r = r600_init_surface(&surface, templ, array_mode,
>>> +                             templ->flags & R600_RESOURCE_FLAG_TRANSFER,
>>> +                             templ->usage & PIPE_BIND_SAMPLER_VIEW);
>>
>> There are two reasons why this is wrong.
>>
>> 1) templ->usage may only contain one PIPE_USAGE_* flag.
>>
>> 2) Even if you had used (templ->bind & PIPE_BIND_SAMPLER_VIEW), it
>> wouldn't be any useful for OpenGL, because all textures and renderbuffers in
>> OpenGL can be used as samplers. Yeah, that's right, everything. The
>> typical use cases are glBlitFramebuffer and glCopyTex(Sub)Image.
>>
>> Also it would be safer not to trust the state tracker that it set the
>> bind flags correctly, and behave as if PIPE_BIND_SAMPLER_VIEW were set
>> always.
>>
>>>        if (r) {
>>>                return NULL;
>>>        }
>>> @@ -723,7 +722,7 @@ struct pipe_resource *r600_texture_from_handle(struct pipe_screen *screen,
>>>        else
>>>                array_mode = 0;
>>>
>>> -       r = r600_init_surface(&surface, templ, array_mode);
>>> +       r = r600_init_surface(&surface, templ, array_mode, 0, 0);
>>
>> This is wrong for the same reason as above. The window framebuffer
>> allocated by the DDX can be used as a sampler in OpenGL.
>
> Just to be clear, you don't mean that at the GL API level, do you?

That's correct.

> OpenGL doesn't support using the window's color buffer (nor plain
> renderbuffers) to be used as a texture.  A compositor might use the
> window image as a texture though.

I mean it from the perspective of an OpenGL driver which implements
glCopyTexImage by drawing a full-screen textured quad into the target texture.

Marek