[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:48:57 PST 2013
On 10 January 2013 13:23, Ian Romanick <idr at freedesktop.org> wrote:
> 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?
>
>
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).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130110/47ce25ae/attachment-0001.html>
More information about the mesa-dev
mailing list