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

[Kenneth Graunke]

On 06/07/2012 10:17 AM, Paul Berry wrote:
> On 6 June 2012 23:42, Kenneth Graunke <kenneth at whitecape.org
> <mailto: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.

*grumble-grumble* luminance formats *grumble*

Right then :)

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>