[Mesa-dev] [PATCH 3/4] i965/blorp: Implement logic for additional buffer formats.

[Paul Berry]

On 6 June 2012 23:42, Kenneth Graunke <kenneth at whitecape.org> wrote:

> On 06/06/2012 12:20 PM, Paul Berry wrote:
> > Previously the blorp engine only supported RGBA8 color buffers and
> > 24-bit depth buffers.  This patch adds support for any color buffer
> > format that is supported as a render target, and for 16-bit and 32-bit
> > depth buffers.
> >
> > This required threading the brw_context struct through into
> > brw_blorp_surface_info::set() so that it can consult the
> > brw->render_target_format array.
> > ---
> >  src/mesa/drivers/dri/i965/brw_blorp.cpp      |   37
> +++++++++++++++++++++++---
> >  src/mesa/drivers/dri/i965/brw_blorp.h        |    3 +-
> >  src/mesa/drivers/dri/i965/brw_blorp_blit.cpp |    4 +-
> >  3 files changed, 37 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.cpp
> b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> > index d6d00e7..32ea738 100644
> > --- a/src/mesa/drivers/dri/i965/brw_blorp.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> > @@ -53,23 +53,52 @@ brw_blorp_mip_info::set(struct intel_mipmap_tree *mt,
> >  }
> >
> >  void
> > -brw_blorp_surface_info::set(struct intel_mipmap_tree *mt,
> > +brw_blorp_surface_info::set(struct brw_context *brw,
> > +                            struct intel_mipmap_tree *mt,
> >                              unsigned int level, unsigned int layer)
> >  {
> >     brw_blorp_mip_info::set(mt, level, layer);
> >     this->num_samples = mt->num_samples;
> >     this->array_spacing_lod0 = mt->array_spacing_lod0;
> > +   this->map_stencil_as_y_tiled = false;
> >
> > -   if (mt->format == MESA_FORMAT_S8) {
> > +   switch (mt->format) {
> > +   case MESA_FORMAT_S8:
> >        /* The miptree is a W-tiled stencil buffer.  Surface states can't
> be set
> >         * up for W tiling, so we'll need to use Y tiling and have the WM
> >         * program swizzle the coordinates.
> >         */
> >        this->map_stencil_as_y_tiled = true;
> >        this->brw_surfaceformat = BRW_SURFACEFORMAT_R8_UNORM;
> > -   } else {
> > -      this->map_stencil_as_y_tiled = false;
> > +      break;
> > +   case MESA_FORMAT_X8_Z24:
> > +   case MESA_FORMAT_Z32_FLOAT:
> > +      /* The miptree consists of 32 bits per pixel, arranged either as
> 24-bit
> > +       * depth values interleaved with 8 "don't care" bits, or as 32-bit
> > +       * floating point depth values.  Since depth values don't require
> any
> > +       * blending, it doesn't matter how we interpret the bit pattern
> as long
> > +       * as we copy the right amount of data, so just map it as 8-bit
> RGBA.
>
> Nitpick...you're not mapping it as RGBA.  I would believe either
> "just map it as 8-bit BGRA" ... or "as 8-bit ARGB" since I can never
> keep the endianness straight.
>
> Of course, the point of the comment is precisely that it doesn't
> matter... :)
>

Yeah, I didn't want to trouble the reader with this subtlety, but you are
technically correct.  I'll update the comment.


>
> > +       */
> >        this->brw_surfaceformat = BRW_SURFACEFORMAT_B8G8R8A8_UNORM;
> > +      break;
> > +   case MESA_FORMAT_Z16:
> > +      /* The miptree consists of 16 bits per pixel of depth data.
>  Since depth
> > +       * values don't require any blending, it doesn't matter how we
> interpret
> > +       * the bit pattern as long as we copy the right amount of data,
> so just
> > +       * map is as 8-bit RG.
> > +       */
> > +      this->brw_surfaceformat = BRW_SURFACEFORMAT_R8G8_UNORM;
> > +      break;
> > +   default:
> > +      /* Blorp blits don't support any sort of format conversion, so we
> can
> > +       * safely assume that the same format is being used for the
> source and
> > +       * destination.  Therefore the format must be supported as a
> render
> > +       * target, even if this is the source image.  So we can convert
> to a
> > +       * surface format using brw->render_target_format.
> > +       */
> > +      assert(brw->format_supported_as_render_target[mt->format]);
> > +      this->brw_surfaceformat = brw->render_target_format[mt->format];
>
> Why not just do:
>
> default:
>   this->brw_surfaceformat = brw_format_for_mesa_format(mt->format);
>

The trouble is that brw->render_target_format[] and
brw_format_for_mesa_format() don't do the same thing.  In particular,
brw->render_target_format maps several formats that are otherwise
unsupported as render targets onto equivalent formats that are supported:

BRW_SURFACEFORMAT_B8G8R8X8_UNORM -> BRW_SURFACEFORMAT_B8G8R8A8_UNORM
BRW_SURFACEFORMAT_I32_FLOAT -> BRW_SURFACEFORMAT_R32_FLOAT
BRW_SURFACEFORMAT_L32_FLOAT -> BRW_SURFACEFORMAT_R32_FLOAT
BRW_SURFACEFORMAT_I16_FLOAT -> BRW_SURFACEFORMAT_R16_FLOAT
BRW_SURFACEFORMAT_L16_FLOAT -> BRW_SURFACEFORMAT_R16_FLOAT

Blorp needs to use brw->render_target_format[] so that these mappings will
occur, otherwise it will try to map the render target using an unsupported
format.


>
> Then you don't need to explain why it's safe to assume that source
> formats are render formats and access the array directly.  Obviously, it
> still needs to be renderable, but that's probably best captured
> elsewhere in an assert like
>
> assert(source_format == dest_format);
>
> which I assume you already have.
>
> With those minor changes, this is:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
> > +      break;
> >     }
> >  }
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.h
> b/src/mesa/drivers/dri/i965/brw_blorp.h
> > index 0de3d1e..4c74c91 100644
> > --- a/src/mesa/drivers/dri/i965/brw_blorp.h
> > +++ b/src/mesa/drivers/dri/i965/brw_blorp.h
> > @@ -66,7 +66,8 @@ class brw_blorp_surface_info : public
> brw_blorp_mip_info
> >  public:
> >     brw_blorp_surface_info();
> >
> > -   void set(struct intel_mipmap_tree *mt,
> > +void set(struct brw_context *brw,
> > +         struct intel_mipmap_tree *mt,
> >              unsigned int level, unsigned int layer);
> >
> >     /* Setting this flag indicates that the buffer's contents are W-tiled
> > diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > index 93c3f73..180468b 100644
> > --- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > @@ -1274,8 +1274,8 @@
> brw_blorp_blit_params::brw_blorp_blit_params(struct brw_context *brw,
> >                                               GLuint dst_x1, GLuint
> dst_y1,
> >                                               bool mirror_x, bool
> mirror_y)
> >  {
> > -   src.set(src_mt, 0, 0);
> > -   dst.set(dst_mt, 0, 0);
> > +   src.set(brw, src_mt, 0, 0);
> > +   dst.set(brw, dst_mt, 0, 0);
> >
> >     use_wm_prog = true;
> >     memset(&wm_prog_key, 0, sizeof(wm_prog_key));
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120607/fb7b9b74/attachment.html>