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

[Brian Paul]

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?
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.

-Brian