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

[Kenneth Graunke]

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... :)

> +       */
>        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);

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));