[Mesa-dev] [PATCH 3/3] intel: Clean up confusion between logical and physical surface dimensions.
Ian Romanick
idr at freedesktop.org
Thu Jan 10 13:23:19 PST 2013
On 01/10/2013 01:03 PM, Paul Berry wrote:
> On 10 January 2013 12:01, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
> On 01/08/2013 02:27 PM, Paul Berry wrote:
>
> In most cases, the width, height, and depth of the physical surface
> used by the driver to implement a texture or renderbuffer is
> equal to
> the logical width, height, and depth exposed to the client through
> functions such as glTexImage3D(). However, there are two
> exceptions:
> cube maps (which have a physical depth of 6 but a logical depth
> of 1)
> and multisampled renderbuffers (which have larger physical
> dimensions
> than logical dimensions to allow multiple samples per pixel).
>
> Previous to this patch, we accounted for the difference between
> physical and logical surface dimensions at inconsistent places
> in the
> call graph (multisampling was accounted for in
> intel_miptree_create_for___renderbuffer(), and cubemaps were
> accounted
> for in intel_miptree_create_internal(__)). As a result, it wasn't
> always clear, when calling a miptree creation function, whether
> physical or logical dimensions were needed. Also, we weren't
> consistent about storing logical dimensions in the intel_mipmap_tree
> structure (we only did so in the
> intel_miptree_create_for___renderbuffer() code path, and we did not
> store depth).
>
> This patch refactors things so that
> intel_miptree_create_internal(__) is
> responsible for converting logical to physical dimensions and for
> storing both the physical and logical dimensions in the
> intel_mipmap_tree structure. As a result, all miptree creation
> functions interpret their arguments as logical dimensions, and both
> physical and logical dimensions are always available to
> functions that
> work with intel_mipmap_trees.
>
> In addition, it renames the fields in intel_mipmap_tree used to
> store
> the dimensions, so that it is clear from the name whether
> physical or
> logical dimensions are being referred to.
>
> This should fix the following bugs:
>
> - When creating a separate stencil surface for a depthstencil
> cubemap,
> we would erroneously try to convert the depth from 1 to 6 twice,
> resulting in an assertion failure.
>
> - When creating an MCS buffer for compressed multisampling, we used
> physical dimensions instead of logical dimensions, resulting in
> wasted memory.
>
> In addition, this should considerably simplify the implementation of
> ARB_texture_multisample, because it moves the code to compute the
> physical size of multisampled surfaces out of renderbuffer-only
> code.
> ---
> src/mesa/drivers/dri/i915/__i915_tex_layout.c | 36 ++---
> src/mesa/drivers/dri/i965/brw___tex_layout.c | 20 +--
> src/mesa/drivers/dri/intel/__intel_fbo.c | 1 -
> src/mesa/drivers/dri/intel/__intel_mipmap_tree.c | 191
> +++++++++++-------------
> src/mesa/drivers/dri/intel/__intel_mipmap_tree.h | 28 ++--
> src/mesa/drivers/dri/intel/__intel_tex_image.c | 1 -
> src/mesa/drivers/dri/intel/__intel_tex_layout.c | 18 +--
> src/mesa/drivers/dri/intel/__intel_tex_validate.c | 1 -
> 8 files changed, 143 insertions(+), 153 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i915/__i915_tex_layout.c
> b/src/mesa/drivers/dri/i915/__i915_tex_layout.c
> index 1e3cfad..90911a6 100644
> --- a/src/mesa/drivers/dri/i915/__i915_tex_layout.c
> +++ b/src/mesa/drivers/dri/i915/__i915_tex_layout.c
> @@ -114,9 +114,9 @@ static GLint bottom_offsets[6] = {
> static void
> i915_miptree_layout_cube(__struct intel_mipmap_tree * mt)
> {
> - const GLuint dim = mt->width0;
> + const GLuint dim = mt->physical_width0;
> GLuint face;
> - GLuint lvlWidth = mt->width0, lvlHeight = mt->height0;
> + GLuint lvlWidth = mt->physical_width0, lvlHeight =
> mt->physical_height0;
> GLint level;
>
> assert(lvlWidth == lvlHeight); /* cubemap images are square */
> @@ -156,14 +156,14 @@ i915_miptree_layout_cube(__struct
> intel_mipmap_tree * mt)
> static void
> i915_miptree_layout_3d(struct intel_mipmap_tree * mt)
> {
> - GLuint width = mt->width0;
> - GLuint height = mt->height0;
> - GLuint depth = mt->depth0;
> + GLuint width = mt->physical_width0;
> + GLuint height = mt->physical_height0;
> + GLuint depth = mt->physical_depth0;
> GLuint stack_height = 0;
> GLint level;
>
> /* Calculate the size of a single slice. */
> - mt->total_width = mt->width0;
> + mt->total_width = mt->physical_width0;
>
> /* XXX: hardware expects/requires 9 levels at minimum. */
> for (level = mt->first_level; level <= MAX2(8,
> mt->last_level); level++) {
> @@ -178,7 +178,7 @@ i915_miptree_layout_3d(struct
> intel_mipmap_tree * mt)
> }
>
> /* Fixup depth image_offsets: */
> - depth = mt->depth0;
> + depth = mt->physical_depth0;
> for (level = mt->first_level; level <= mt->last_level;
> level++) {
> GLuint i;
> for (i = 0; i < depth; i++) {
> @@ -193,18 +193,18 @@ i915_miptree_layout_3d(struct
> intel_mipmap_tree * mt)
> * remarkable how wasteful of memory the i915 texture layouts
> * are. They are largely fixed in the i945.
> */
> - mt->total_height = stack_height * mt->depth0;
> + mt->total_height = stack_height * mt->physical_depth0;
> }
>
> static void
> i915_miptree_layout_2d(struct intel_mipmap_tree * mt)
> {
> - GLuint width = mt->width0;
> - GLuint height = mt->height0;
> + GLuint width = mt->physical_width0;
> + GLuint height = mt->physical_height0;
> GLuint img_height;
> GLint level;
>
> - mt->total_width = mt->width0;
> + mt->total_width = mt->physical_width0;
> mt->total_height = 0;
>
> for (level = mt->first_level; level <= mt->last_level;
> level++) {
> @@ -312,9 +312,9 @@ i915_miptree_layout(struct intel_mipmap_tree
> * mt)
> static void
> i945_miptree_layout_cube(__struct intel_mipmap_tree * mt)
> {
> - const GLuint dim = mt->width0;
> + const GLuint dim = mt->physical_width0;
> GLuint face;
> - GLuint lvlWidth = mt->width0, lvlHeight = mt->height0;
> + GLuint lvlWidth = mt->physical_width0, lvlHeight =
> mt->physical_height0;
> GLint level;
>
> assert(lvlWidth == lvlHeight); /* cubemap images are square */
> @@ -402,17 +402,17 @@ i945_miptree_layout_cube(__struct
> intel_mipmap_tree * mt)
> static void
> i945_miptree_layout_3d(struct intel_mipmap_tree * mt)
> {
> - GLuint width = mt->width0;
> - GLuint height = mt->height0;
> - GLuint depth = mt->depth0;
> + GLuint width = mt->physical_width0;
> + GLuint height = mt->physical_height0;
> + GLuint depth = mt->physical_depth0;
> GLuint pack_x_pitch, pack_x_nr;
> GLuint pack_y_pitch;
> GLuint level;
>
> - mt->total_width = mt->width0;
> + mt->total_width = mt->physical_width0;
> mt->total_height = 0;
>
> - pack_y_pitch = MAX2(mt->height0, 2);
> + pack_y_pitch = MAX2(mt->physical_height0, 2);
> pack_x_pitch = mt->total_width;
> pack_x_nr = 1;
>
> diff --git a/src/mesa/drivers/dri/i965/__brw_tex_layout.c
> b/src/mesa/drivers/dri/i965/__brw_tex_layout.c
> index b661570..1428396 100644
> --- a/src/mesa/drivers/dri/i965/__brw_tex_layout.c
> +++ b/src/mesa/drivers/dri/i965/__brw_tex_layout.c
> @@ -47,8 +47,8 @@ brw_miptree_layout_texture___array(struct
> intel_context *intel,
> GLuint qpitch = 0;
> int h0, h1, q;
>
> - h0 = ALIGN(mt->height0, mt->align_h);
> - h1 = ALIGN(minify(mt->height0), mt->align_h);
> + h0 = ALIGN(mt->physical_height0, mt->align_h);
> + h1 = ALIGN(minify(mt->physical___height0), mt->align_h);
> if (mt->array_spacing_lod0)
> qpitch = h0;
> else
> @@ -59,11 +59,11 @@ brw_miptree_layout_texture___array(struct
> intel_context *intel,
> i945_miptree_layout_2d(mt);
>
> for (level = mt->first_level; level <= mt->last_level;
> level++) {
> - for (q = 0; q < mt->depth0; q++) {
> + for (q = 0; q < mt->physical_depth0; q++) {
> intel_miptree_set_image___offset(mt, level, q, 0, q *
> qpitch);
> }
> }
> - mt->total_height = qpitch * mt->depth0;
> + mt->total_height = qpitch * mt->physical_depth0;
> }
>
> void
> @@ -84,13 +84,13 @@ brw_miptree_layout(struct intel_context
> *intel, struct intel_mipmap_tree *mt)
> brw_miptree_layout_texture___array(intel, mt);
> break;
> }
> - assert(mt->depth0 == 6);
> + assert(mt->physical_depth0 == 6);
> /* FALLTHROUGH */
>
> case GL_TEXTURE_3D: {
> - GLuint width = mt->width0;
> - GLuint height = mt->height0;
> - GLuint depth = mt->depth0;
> + GLuint width = mt->physical_width0;
> + GLuint height = mt->physical_height0;
> + GLuint depth = mt->physical_depth0;
> GLuint pack_x_pitch, pack_x_nr;
> GLuint pack_y_pitch;
> GLuint level;
> @@ -101,8 +101,8 @@ brw_miptree_layout(struct intel_context
> *intel, struct intel_mipmap_tree *mt)
> mt->total_width = ALIGN(width, mt->align_w);
> pack_y_pitch = (height + 3) / 4;
> } else {
> - mt->total_width = mt->width0;
> - pack_y_pitch = ALIGN(mt->height0, mt->align_h);
> + mt->total_width = mt->physical_width0;
> + pack_y_pitch = ALIGN(mt->physical_height0, mt->align_h);
> }
>
> pack_x_pitch = width;
> diff --git a/src/mesa/drivers/dri/intel/__intel_fbo.c
> b/src/mesa/drivers/dri/intel/__intel_fbo.c
> index 0597015..034fa8a 100644
> --- a/src/mesa/drivers/dri/intel/__intel_fbo.c
> +++ b/src/mesa/drivers/dri/intel/__intel_fbo.c
> @@ -939,7 +939,6 @@ intel_renderbuffer_move_to___temp(struct
> intel_context *intel,
> width, height, depth,
> true,
> irb->mt->num_samples,
> - irb->mt->msaa_layout,
> false /* force_y_tiling */);
>
> intel_miptree_copy_teximage(__intel, intel_image, new_mt);
> diff --git a/src/mesa/drivers/dri/intel/__intel_mipmap_tree.c
> b/src/mesa/drivers/dri/intel/__intel_mipmap_tree.c
> index 12b77b6..7542219 100644
> --- a/src/mesa/drivers/dri/intel/__intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/intel/__intel_mipmap_tree.c
> @@ -122,8 +122,7 @@ intel_miptree_create_internal(__struct
> intel_context *intel,
> GLuint height0,
> GLuint depth0,
> bool for_region,
> - GLuint num_samples,
> - enum intel_msaa_layout msaa_layout)
> + GLuint num_samples)
> {
> struct intel_mipmap_tree *mt = calloc(sizeof(*mt), 1);
> int compress_byte = 0;
> @@ -140,18 +139,78 @@ intel_miptree_create_internal(__struct
> intel_context *intel,
> mt->format = format;
> mt->first_level = first_level;
> mt->last_level = last_level;
> - mt->width0 = width0;
> - mt->height0 = height0;
> + mt->logical_width0 = width0;
> + mt->logical_height0 = height0;
> + mt->logical_depth0 = depth0;
> mt->cpp = compress_byte ? compress_byte :
> _mesa_get_format_bytes(mt->__format);
> mt->num_samples = num_samples;
> mt->compressed = compress_byte ? 1 : 0;
> - mt->msaa_layout = msaa_layout;
> + mt->msaa_layout = INTEL_MSAA_LAYOUT_NONE;
> mt->refcount = 1;
>
> + if (num_samples > 1) {
> + /* Adjust width/height/depth for MSAA */
> + mt->msaa_layout = compute_msaa_layout(intel, format);
> + if (mt->msaa_layout == INTEL_MSAA_LAYOUT_IMS) {
> + /* In the Sandy Bridge PRM, volume 4, part 1, page 31,
> it says:
> + *
> + * "Any of the other messages (sample*, LOD,
> load4) used with a
> + * (4x) multisampled surface will in-effect
> sample a surface with
> + * double the height and width as that indicated
> in the surface
> + * state. Each pixel position on the
> original-sized surface is
> + * replaced with a 2x2 of samples with the
> following arrangement:
> + *
> + * sample 0 sample 2
> + * sample 1 sample 3"
> + *
> + * Thus, when sampling from a multisampled texture, it
> behaves as
> + * though the layout in memory for (x,y,sample) is:
> + *
> + * (0,0,0) (0,0,2) (1,0,0) (1,0,2)
> + * (0,0,1) (0,0,3) (1,0,1) (1,0,3)
> + *
> + * (0,1,0) (0,1,2) (1,1,0) (1,1,2)
> + * (0,1,1) (0,1,3) (1,1,1) (1,1,3)
> + *
> + * However, the actual layout of multisampled data in
> memory is:
> + *
> + * (0,0,0) (1,0,0) (0,0,1) (1,0,1)
> + * (0,1,0) (1,1,0) (0,1,1) (1,1,1)
> + *
> + * (0,0,2) (1,0,2) (0,0,3) (1,0,3)
> + * (0,1,2) (1,1,2) (0,1,3) (1,1,3)
> + *
> + * This pattern repeats for each 2x2 pixel block.
> + *
> + * As a result, when calculating the size of our
> 4-sample buffer for
> + * an odd width or height, we have to align before
> scaling up because
> + * sample 3 is in that bottom right 2x2 block.
> + */
> + switch (num_samples) {
> + case 4:
> + width0 = ALIGN(width0, 2) * 2;
> + height0 = ALIGN(height0, 2) * 2;
> + break;
> + case 8:
> + width0 = ALIGN(width0, 2) * 4;
> + height0 = ALIGN(height0, 2) * 2;
> + break;
> + default:
> + /* num_samples should already have been quantized
> to 0, 1, 4, or
> + * 8.
> + */
> + assert(false);
> + }
> + } else {
> + /* Non-interleaved */
> + depth0 *= num_samples;
> + }
> + }
> +
> /* array_spacing_lod0 is only used for non-IMS MSAA
> surfaces. TODO: can we
> * use it elsewhere?
> */
> - switch (msaa_layout) {
> + switch (mt->msaa_layout) {
> case INTEL_MSAA_LAYOUT_NONE:
> case INTEL_MSAA_LAYOUT_IMS:
> mt->array_spacing_lod0 = false;
> @@ -164,30 +223,28 @@ intel_miptree_create_internal(__struct
> intel_context *intel,
>
> if (target == GL_TEXTURE_CUBE_MAP) {
> assert(depth0 == 1);
> - mt->depth0 = 6;
> - } else {
> - mt->depth0 = depth0;
> + depth0 = 6;
>
>
> This is basically converting depth0 from logical to physical. We
> had discussed that this could cause problems with future cubemap
> arrays. I may not be following the code completely, but does this
> potential future problem still loom?
>
>
> I think we're ok w.r.t. cubemap arrays. Once we get around to
> supporting them, all we should have to do is remove the "assert(depth0
> == 1)" line and replace "depth0 = 6" with "depth0 *= 6".
>
> I was tempted to just go ahead and do that in this patch, but I decided
> that for now keeping the assertion around is a nice way to verify that
> we don't accidentally feed physical dimsensions to
> intel_miptree_create_internal when logical dimensions are intended.
Okay... and the 6 (or 6*depth0) created here can't get fed back around
in the cubemap array case because the receiver of depth0 (below) knows
it's the physical size, and it will feed the logical size (still 1) back
around. Right?
More information about the mesa-dev
mailing list