On 6 June 2012 23:42, Kenneth Graunke <kenn...@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 <kenn...@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));
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
