[Mesa-dev] [PATCH 3/3] intel: Clean up confusion between logical and physical surface dimensions.
Paul Berry
stereotype441 at gmail.com
Thu Jan 10 13:03:58 PST 2013
On 10 January 2013 12:01, Ian Romanick <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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130110/cb1e0264/attachment-0001.html>
More information about the mesa-dev
mailing list