[Mesa-dev] [RFC 2/3] gallium: Move nv50 clear_texture impl down to util_surface

[Marek Olšák]

On Fri, Dec 4, 2015 at 1:37 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Thu, Dec 3, 2015 at 7:25 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 03.12.2015 um 23:48 schrieb Ilia Mirkin:
>>> On Thu, Dec 3, 2015 at 4:44 AM, Edward O'Callaghan
>>> <eocallaghan at alterapraxis.com> wrote:
>>>> ARB_clear_texture is reasonably generic enough that it should
>>>> be moved down to become part of the fallback mechanism of
>>>> pipe->clear_texture.
>>>>
>>>> Signed-off-by: Edward O'Callaghan <eocallaghan at alterapraxis.com>
>>>> ---
>>>>  src/gallium/auxiliary/util/u_surface.c          | 83 +++++++++++++++++++++++++
>>>>  src/gallium/auxiliary/util/u_surface.h          |  6 ++
>>>>  src/gallium/drivers/nouveau/nv50/nv50_surface.c | 67 +-------------------
>>>>  3 files changed, 90 insertions(+), 66 deletions(-)
>>>>
>>>> diff --git a/src/gallium/auxiliary/util/u_surface.c b/src/gallium/auxiliary/util/u_surface.c
>>>> index 6aa44f9..e7ab175 100644
>>>> --- a/src/gallium/auxiliary/util/u_surface.c
>>>> +++ b/src/gallium/auxiliary/util/u_surface.c
>>>> @@ -36,6 +36,7 @@
>>>>  #include "pipe/p_screen.h"
>>>>  #include "pipe/p_state.h"
>>>>
>>>> +#include "util/u_math.h"
>>>>  #include "util/u_format.h"
>>>>  #include "util/u_inlines.h"
>>>>  #include "util/u_rect.h"
>>>> @@ -547,6 +548,88 @@ util_clear_depth_stencil(struct pipe_context *pipe,
>>>>     }
>>>>  }
>>>>
>>>> +/**
>>>> + * Fallback for pipe->clear_texture() function.
>>>> + * clears a non-PIPE_BUFFER resource's specified level
>>>> + * and bounding box with a clear value provided in that
>>>> + * resource's native format.
>>>> + *
>>>> + * XXX sf->format = .. is problematic as hw need
>>>> + * not nessarily support the format.
>>>> + */
>>>> +void
>>>> +util_surface_clear_texture(struct pipe_context *pipe,
>>>> +                           struct pipe_resource *res,
>>>> +                           unsigned level,
>>>> +                           const struct pipe_box *box,
>>>> +                           const void *data)
>>>> +{
>>>> +   struct pipe_surface tmpl = {{0}}, *sf;
>>>> +
>>>> +   tmpl.format = res->format;
>>>> +   tmpl.u.tex.first_layer = box->z;
>>>> +   tmpl.u.tex.last_layer = box->z + box->depth - 1;
>>>> +   tmpl.u.tex.level = level;
>>>> +   sf = pipe->create_surface(pipe, res, &tmpl);
>>>> +   if (!sf)
>>>> +      return;
>>>> +
>>>> +   if (util_format_is_depth_or_stencil(res->format)) {
>>>> +      float depth = 0;
>>>> +      uint8_t stencil = 0;
>>>> +      unsigned clear = 0;
>>>> +      const struct util_format_description *desc =
>>>> +         util_format_description(res->format);
>>>> +
>>>> +      if (util_format_has_depth(desc)) {
>>>> +         clear |= PIPE_CLEAR_DEPTH;
>>>> +         desc->unpack_z_float(&depth, 0, data, 0, 1, 1);
>>>> +      }
>>>> +      if (util_format_has_stencil(desc)) {
>>>> +         clear |= PIPE_CLEAR_STENCIL;
>>>> +         desc->unpack_s_8uint(&stencil, 0, data, 0, 1, 1);
>>>> +      }
>>>> +      pipe->clear_depth_stencil(pipe, sf, clear, depth, stencil,
>>>> +                                box->x, box->y, box->width, box->height);
>>>> +   } else {
>>>> +      union pipe_color_union color;
>>>> +
>>>> +      switch (util_format_get_blocksizebits(res->format)) {
>>>> +      case 128:
>>>> +         sf->format = PIPE_FORMAT_R32G32B32A32_UINT;
>>>> +         memcpy(&color.ui, data, 128 / 8);
>>>> +         break;
>>>> +      case 64:
>>>> +         sf->format = PIPE_FORMAT_R32G32_UINT;
>>>> +         memcpy(&color.ui, data, 64 / 8);
>>>> +         memset(&color.ui[2], 0, 64 / 8);
>>>> +         break;
>>>> +      case 32:
>>>> +         sf->format = PIPE_FORMAT_R32_UINT;
>>>> +         memcpy(&color.ui, data, 32 / 8);
>>>> +         memset(&color.ui[1], 0, 96 / 8);
>>>> +         break;
>>>> +      case 16:
>>>> +         sf->format = PIPE_FORMAT_R16_UINT;
>>>> +         color.ui[0] = util_cpu_to_le32(
>>>> +            util_le16_to_cpu(*(unsigned short *)data));
>>>> +         memset(&color.ui[1], 0, 96 / 8);
>>>> +         break;
>>>> +      case 8:
>>>> +         sf->format = PIPE_FORMAT_R8_UINT;
>>>> +         color.ui[0] = util_cpu_to_le32(*(unsigned char *)data);
>>>> +         memset(&color.ui[1], 0, 96 / 8);
>>>> +         break;
>>>> +      default:
>>>> +         assert(!"Unknown texel element size");
>>>> +         return;
>>>> +      }
>>>> +
>>>> +      pipe->clear_render_target(pipe, sf, &color,
>>>> +                                box->x, box->y, box->width, box->height);
>>>
>>> I only recently realized this, but I'm fairly sure this is wrong --
>>> needs to be divided by util_format_blockwidth/height, otherwise we'll
>>> go way out of bounds for compressed formats. [And yes,
>>> nv50_clear_texture has the same problem.]
>> You can't clear compressed textures with ARB_clear_texture, so I guess
>> this shouldn't be a problem.
>
> Ah yes, indeed. That's why I didn't get any weird failurs :)
>
>> Even if you think it should work in gallium, with the format illegally
>> being changed after-the-fact (after creating the surface) it's not even
>> worth thinking about what the correct dimensions should be (but yes if
>> that would be fixed you'd be right)...
>
> Yeah... that's why that stuff sat as an nv50 helper, not in generic
> code. Figured it cheated too much for that... but if other drivers
> want the same cheats... perhaps worth sticking somewhere common?

It's not cheating. It's plainly simply incorrect usage of pipe_surface
and it can be fixed trivially.

Marek