On 10 January 2013 13:23, Ian Romanick <i...@freedesktop.org> wrote:
> On 01/10/2013 01:03 PM, Paul Berry wrote: > >> On 10 January 2013 12:01, Ian Romanick <i...@freedesktop.org >> <mailto:i...@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? > > Yes, correct--after this patch is applied, of course. (Before this patch the 6 would occasionally get fed back around causing an assertion failure--I think that contributed to one of our GLES3 conformance failures).
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
