[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:52:15 PST 2013
On 01/10/2013 01:48 PM, Paul Berry wrote:
> On 10 January 2013 13:23, Ian Romanick <idr at freedesktop.org
> <mailto: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>
> <mailto: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).
Excellent. The series is
Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
More information about the mesa-dev
mailing list