On 6 June 2012 23:42, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5">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. </div></div>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... :) </blockquote><div> Yeah, I didn't want to trouble the reader with this subtlety, but you are technically correct. I'll update the comment. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > + */ > 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]; </div>Why not just do: default: this->brw_surfaceformat = brw_format_for_mesa_format(mt->format); </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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 <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> <div class="HOEnZb"><div class="h5"> > + 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)); </div></div></blockquote></div>