<div dir="ltr">On 10 January 2013 12:01, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On 01/08/2013 02:27 PM, Paul Berry wrote: </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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; </blockquote> </div></div> 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?<div class="HOEnZb"> <div class="h5"> </div></div></blockquote><div> </div><div>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". </div><div>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. </div></div></div></div>