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

[Marek Olšák]

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.

Marek