[Mesa-dev] [PATCH 4/9] intel: Fix software copying of miptree faces for weird formats.
Chad Versace
chad.versace at linux.intel.com
Tue Nov 13 15:05:52 PST 2012
Some weird formats are omitted: ETC1. This patch will raise an assertion
if intel_miptree_copy_slice is called on an ETC1 texture and intelEmitCopyBlit
fails. However, I'm not certain if copy_slice can even be called on ETC1.
Other than the ETC1 problem, the patch looks good. I have a few nit requests
that you can ignore if you wish.
I'm withholding review until I'm convinced that copy_slice will not be called
on ETC1 or I can think of a fix.
On 11/05/2012 04:48 PM, Eric Anholt wrote:
> Now that we have W-tiled S8, we can't just region_map and poke at bits --
> there has to be some swizzling. Rely on intel_miptree_map to get that job
> done. This should also get the highest performance path we know of for the
> mapping.
> ---
> src/mesa/drivers/dri/intel/intel_mipmap_tree.c | 101 +++++++++++++++++++-----
> src/mesa/drivers/dri/intel/intel_regions.c | 35 --------
> src/mesa/drivers/dri/intel/intel_regions.h | 10 ---
> 3 files changed, 83 insertions(+), 63 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
> index c70c1de..db0bac5 100644
> --- a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
> @@ -671,6 +671,68 @@ intel_miptree_get_image_offset(struct intel_mipmap_tree *mt,
> }
>
> static void
> +intel_miptree_copy_slice_sw(struct intel_context *intel,
> + struct intel_mipmap_tree *dst_mt,
> + struct intel_mipmap_tree *src_mt,
> + int level,
> + int slice,
> + int width,
> + int height)
> +{
> + void *src, *dst;
> + int src_stride, dst_stride;
> + int cpp = dst_mt->cpp;
Below in intel_miptree_copy_slice, you add
assert(src_mt->format == dst_mt->format);
I'd like to see that assertion repeated here for documentation.
> +
> + /* If we're mapping separate stencil, the format we get mapped is not
> + * mt->format!
> + */
> + if (dst_mt->stencil_mt) {
I would like a little comment here saying that, at time of
miptree creation, the originally requested format was
MESA_FORMAT_Z32_FLOAT_X24S8. Without such a comment, the
`cpp = 8` is difficult to grok. It may seem obvious to you,
the patch author, but I wasted a minute untying
the if-tree's assumptions and the reason for `cpp = 8`.
But I won't hold that comment nit against the patch.
> + if (dst_mt->format == MESA_FORMAT_Z32_FLOAT)
> + cpp = 8;
> + else {
Ditto for MESA_FORMAT_S8_Z24.
> + assert(dst_mt->format == MESA_FORMAT_X8_Z24);
> + assert(cpp == 4);
> + }
> + }
> +
> + intel_miptree_map(intel, src_mt,
> + level, slice,
> + 0, 0,
> + width, height,
> + GL_MAP_READ_BIT,
> + &src, &src_stride);
> +
> + intel_miptree_map(intel, dst_mt,
> + level, slice,
> + 0, 0,
> + width, height,
> + GL_MAP_WRITE_BIT | GL_MAP_INVALIDATE_RANGE_BIT,
> + &dst, &dst_stride);
> +
> + DBG("sw blit %s mt %p %p/%d -> %s mt %p %p/%d (%dx%d)\n",
> + _mesa_get_format_name(src_mt->format),
> + src_mt, src, src_stride,
> + _mesa_get_format_name(dst_mt->format),
> + dst_mt, dst, dst_stride,
> + width, height);
> +
> + int row_size = cpp * width;
> + if (src_stride == row_size &&
> + dst_stride == row_size) {
> + memcpy(dst, src, row_size * height);
> + } else {
> + for (int i = 0; i < height; i++) {
> + memcpy(dst, src, row_size);
> + dst += dst_stride;
> + src += src_stride;
> + }
> + }
> +
> + intel_miptree_unmap(intel, dst_mt, level, slice);
> + intel_miptree_unmap(intel, src_mt, level, slice);
> +}
> +
> +static void
> intel_miptree_copy_slice(struct intel_context *intel,
> struct intel_mipmap_tree *dst_mt,
> struct intel_mipmap_tree *src_mt,
> @@ -682,14 +744,33 @@ intel_miptree_copy_slice(struct intel_context *intel,
> gl_format format = src_mt->format;
> uint32_t width = src_mt->level[level].width;
> uint32_t height = src_mt->level[level].height;
> + int slice;
> +
> + if (face > 0)
> + slice = face;
> + else
> + slice = depth;
>
> assert(depth < src_mt->level[level].depth);
> + assert(src_mt->format == dst_mt->format);
>
> if (dst_mt->compressed) {
> height = ALIGN(height, dst_mt->align_h) / dst_mt->align_h;
> width = ALIGN(width, dst_mt->align_w);
> }
>
> + /* If it's a packed depth/stencil buffer with separate stencil, we want to
> + * just map the depth/stencil miptree and copy both depth and stencil
> + * across in one go.
> + */
> + if (src_mt->stencil_mt) {
> + intel_miptree_copy_slice_sw(intel,
> + dst_mt, src_mt,
> + level, slice,
> + width, height);
> + return;
> + }
> +
> uint32_t dst_x, dst_y, src_x, src_y;
> intel_miptree_get_image_offset(dst_mt, level, face, depth,
> &dst_x, &dst_y);
> @@ -716,25 +797,9 @@ intel_miptree_copy_slice(struct intel_context *intel,
>
> fallback_debug("miptree validate blit for %s failed\n",
> _mesa_get_format_name(format));
> - void *dst = intel_region_map(intel, dst_mt->region, GL_MAP_WRITE_BIT);
> - void *src = intel_region_map(intel, src_mt->region, GL_MAP_READ_BIT);
> -
> - _mesa_copy_rect(dst,
> - dst_mt->cpp,
> - dst_mt->region->pitch,
> - dst_x, dst_y,
> - width, height,
> - src, src_mt->region->pitch,
> - src_x, src_y);
> -
> - intel_region_unmap(intel, dst_mt->region);
> - intel_region_unmap(intel, src_mt->region);
> - }
>
> - if (src_mt->stencil_mt) {
> - intel_miptree_copy_slice(intel,
> - dst_mt->stencil_mt, src_mt->stencil_mt,
> - level, face, depth);
> + intel_miptree_copy_slice_sw(intel, dst_mt, src_mt, level, slice,
> + width, height);
> }
> }
>
> diff --git a/src/mesa/drivers/dri/intel/intel_regions.c b/src/mesa/drivers/dri/intel/intel_regions.c
> index 7cb008c..15c8ab9 100644
> --- a/src/mesa/drivers/dri/intel/intel_regions.c
> +++ b/src/mesa/drivers/dri/intel/intel_regions.c
> @@ -328,41 +328,6 @@ intel_region_release(struct intel_region **region_handle)
> *region_handle = NULL;
> }
>
> -/*
> - * XXX Move this into core Mesa?
> - */
> -void
> -_mesa_copy_rect(GLubyte * dst,
> - GLuint cpp,
> - GLuint dst_pitch,
> - GLuint dst_x,
> - GLuint dst_y,
> - GLuint width,
> - GLuint height,
> - const GLubyte * src,
> - GLuint src_pitch, GLuint src_x, GLuint src_y)
> -{
> - GLuint i;
> -
> - dst_pitch *= cpp;
> - src_pitch *= cpp;
> - dst += dst_x * cpp;
> - src += src_x * cpp;
> - dst += dst_y * dst_pitch;
> - src += src_y * src_pitch;
> - width *= cpp;
> -
> - if (width == dst_pitch && width == src_pitch)
> - memcpy(dst, src, height * width);
> - else {
> - for (i = 0; i < height; i++) {
> - memcpy(dst, src, width);
> - dst += dst_pitch;
> - src += src_pitch;
> - }
> - }
> -}
> -
> /* Copy rectangular sub-regions. Need better logic about when to
> * push buffers into AGP - will currently do so whenever possible.
> */
> diff --git a/src/mesa/drivers/dri/intel/intel_regions.h b/src/mesa/drivers/dri/intel/intel_regions.h
> index 8737a6d..a463b04 100644
> --- a/src/mesa/drivers/dri/intel/intel_regions.h
> +++ b/src/mesa/drivers/dri/intel/intel_regions.h
> @@ -123,16 +123,6 @@ intel_region_copy(struct intel_context *intel,
> bool flip,
> GLenum logicop);
>
> -void _mesa_copy_rect(GLubyte * dst,
> - GLuint cpp,
> - GLuint dst_pitch,
> - GLuint dst_x,
> - GLuint dst_y,
> - GLuint width,
> - GLuint height,
> - const GLubyte * src,
> - GLuint src_pitch, GLuint src_x, GLuint src_y);
> -
> void
> intel_region_get_tile_masks(struct intel_region *region,
> uint32_t *mask_x, uint32_t *mask_y,
>
More information about the mesa-dev
mailing list