[Mesa-dev] [PATCH 10/41] intel: Refactor intel_miptree_copy_teximage()
Extract the body of the inner loop into a new function,
intel_miptree_copy_slice().
This is in preparation for adding support for separate stencil and HiZ to
intel_miptree_copy_teximage(). When copying a slice of a depthstencil
miptree that uses separate stencil, we will also need to copy the
corresponding slice of the stencil miptree. The easiest way to do this
will be to call intel_miptree_copy_slice() recursively. Analogous
reasoning applies to copying a slice of a depth miptree with HiZ.
Signed-off-by: Chad Versace
---
src/mesa/drivers/dri/intel/intel_mipmap_tree.c | 122 +---
1 files changed, 66 insertions(+), 56 deletions(-)
diff --git a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
index 7f9e606..8f10101 100644
--- a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
@@ -354,6 +354,69 @@ intel_miptree_get_image_offset(struct intel_mipmap_tree
*mt,
}
}
+static void
+intel_miptree_copy_slice(struct intel_context *intel,
+struct intel_mipmap_tree *dst_mt,
+struct intel_mipmap_tree *src_mt,
+int level,
+int face,
+int depth)
+
+{
+ gl_format format = src_mt->format;
+ uint32_t width = src_mt->level[level].width;
+ uint32_t height = src_mt->level[level].height;
+
+ assert(depth < src_mt->level[level].depth);
+
+ if (dst_mt->compressed) {
+ uint32_t align_w, align_h;
+ intel_get_texture_alignment_unit(format,
+ &align_w, &align_h);
+ height = ALIGN(height, align_h) / align_h;
+ width = ALIGN(width, align_w);
+ }
+
+ uint32_t dst_x, dst_y, src_x, src_y;
+ intel_miptree_get_image_offset(dst_mt, level, face, depth,
+ &dst_x, &dst_y);
+ intel_miptree_get_image_offset(src_mt, level, face, depth,
+ &src_x, &src_y);
+
+ DBG("validate blit mt %p %d,%d/%d -> mt %p %d,%d/%d (%dx%d)\n",
+ src_mt, src_x, src_y, src_mt->region->pitch * src_mt->region->cpp,
+ dst_mt, dst_x, dst_y, dst_mt->region->pitch * dst_mt->region->cpp,
+ width, height);
+
+ if (!intelEmitCopyBlit(intel,
+ dst_mt->region->cpp,
+ src_mt->region->pitch, src_mt->region->bo,
+ 0, src_mt->region->tiling,
+ dst_mt->region->pitch, dst_mt->region->bo,
+ 0, dst_mt->region->tiling,
+ src_x, src_y,
+ dst_x, dst_y,
+ width, height,
+ GL_COPY)) {
+
+ 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);
+ }
+}
+
/**
* Copies the image's current data to the given miptree, and associates that
* miptree with the image.
@@ -366,65 +429,12 @@ intel_miptree_copy_teximage(struct intel_context *intel,
struct intel_mipmap_tree *src_mt = intelImage->mt;
int level = intelImage->base.Base.Level;
int face = intelImage->base.Base.Face;
- GLuint width = src_mt->level[level].width;
- GLuint height = src_mt->level[level].height;
GLuint depth = src_mt->level[level].depth;
- int slice;
- void *src, *dst;
-
- if (dst_mt->compressed) {
- unsigned int align_w, align_h;
- intel_get_texture_alignment_unit(intelImage->base.Base.TexFormat,
- &align_w, &align_h);
- height = ALIGN(height, align_h) / align_h;
- width = ALIGN(width, align_w);
- }
-
- for (slice = 0; slice < depth; slice++) {
- unsigned int dst_x, dst_y, src_x, src_y;
-
- intel_miptree_get_image_offset(dst_mt, level, face, slice,
-&dst_x, &dst_y);
-
- /* Copy potentially with the blitter:
- */
- intel_miptree_get_image_offset(src_mt, level, face, slice,
-&src_x, &src_y);
-
- DBG("validate blit mt %p %d,%d/%d -> mt %p %d,%d/%d (%dx%d)\n",
- src_mt, src_x, src_y, src_mt->region->pitch * src_mt->region->cpp,
- dst_mt, dst_x, dst_y, dst_mt->region->pitch * dst_mt->region->cpp,
- width, height);
-
- if (!intelEmitCopyBlit(intel,
-dst_mt->region->cpp,
-src_
Re: [Mesa-dev] [PATCH 10/41] intel: Refactor intel_miptree_copy_teximage()
On 11/17/2011 07:58 PM, Chad Versace wrote:
> Extract the body of the inner loop into a new function,
> intel_miptree_copy_slice().
>
> This is in preparation for adding support for separate stencil and HiZ to
> intel_miptree_copy_teximage(). When copying a slice of a depthstencil
> miptree that uses separate stencil, we will also need to copy the
> corresponding slice of the stencil miptree. The easiest way to do this
> will be to call intel_miptree_copy_slice() recursively. Analogous
> reasoning applies to copying a slice of a depth miptree with HiZ.
>
> Signed-off-by: Chad Versace
> ---
> src/mesa/drivers/dri/intel/intel_mipmap_tree.c | 122
> +---
> 1 files changed, 66 insertions(+), 56 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
> index 7f9e606..8f10101 100644
> --- a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
> @@ -354,6 +354,69 @@ intel_miptree_get_image_offset(struct intel_mipmap_tree
> *mt,
> }
> }
>
> +static void
> +intel_miptree_copy_slice(struct intel_context *intel,
> + struct intel_mipmap_tree *dst_mt,
> + struct intel_mipmap_tree *src_mt,
> + int level,
> + int face,
> + int depth)
> +
> +{
> + gl_format format = src_mt->format;
> + uint32_t width = src_mt->level[level].width;
> + uint32_t height = src_mt->level[level].height;
> +
> + assert(depth < src_mt->level[level].depth);
> +
> + if (dst_mt->compressed) {
> + uint32_t align_w, align_h;
> + intel_get_texture_alignment_unit(format,
> +&align_w, &align_h);
> + height = ALIGN(height, align_h) / align_h;
> + width = ALIGN(width, align_w);
> + }
This wasn't originally inside the loop; you've effectively moved it
there. Since intel_get_texture_alignment_unit actually does some work
these days, I'm wondering if this could be a performance hit.
At any rate, it doesn't seem necessary...I'd probably just add
height/width function parameters and move this hunk back.
> + uint32_t dst_x, dst_y, src_x, src_y;
> + intel_miptree_get_image_offset(dst_mt, level, face, depth,
> + &dst_x, &dst_y);
> + intel_miptree_get_image_offset(src_mt, level, face, depth,
> + &src_x, &src_y);
> +
> + DBG("validate blit mt %p %d,%d/%d -> mt %p %d,%d/%d (%dx%d)\n",
> + src_mt, src_x, src_y, src_mt->region->pitch * src_mt->region->cpp,
> + dst_mt, dst_x, dst_y, dst_mt->region->pitch * dst_mt->region->cpp,
> + width, height);
> +
> + if (!intelEmitCopyBlit(intel,
> + dst_mt->region->cpp,
> + src_mt->region->pitch, src_mt->region->bo,
> + 0, src_mt->region->tiling,
> + dst_mt->region->pitch, dst_mt->region->bo,
> + 0, dst_mt->region->tiling,
> + src_x, src_y,
> + dst_x, dst_y,
> + width, height,
> + GL_COPY)) {
> +
> + 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);
> + }
> +}
> +
> /**
> * Copies the image's current data to the given miptree, and associates that
> * miptree with the image.
> @@ -366,65 +429,12 @@ intel_miptree_copy_teximage(struct intel_context *intel,
> struct intel_mipmap_tree *src_mt = intelImage->mt;
> int level = intelImage->base.Base.Level;
> int face = intelImage->base.Base.Face;
> - GLuint width = src_mt->level[level].width;
> - GLuint height = src_mt->level[level].height;
> GLuint depth = src_mt->level[level].depth;
> - int slice;
> - void *src, *dst;
> -
> - if (dst_mt->compressed) {
> - unsigned int align_w, align_h;
>
> - intel_get_texture_alignment_unit(intelImage->base.Base.TexFormat,
> -&align_w, &align_h);
> - height = ALIGN(height, align_h) / align_h;
> - width = ALIGN(width, align_w);
> - }
> -
> - for (slice = 0; slice < depth; slice++) {
> - unsigned int dst_x, dst_y, src_x, src_y;
> -
> - intel_miptree_get_image_offset(dst_mt, level, face, slice,
> - &dst_x, &dst_y);
> -
> -
Re: [Mesa-dev] [PATCH 10/41] intel: Refactor intel_miptree_copy_teximage()
On 11/18/2011 03:42 PM, Kenneth Graunke wrote:
> On 11/17/2011 07:58 PM, Chad Versace wrote:
>> Extract the body of the inner loop into a new function,
>> intel_miptree_copy_slice().
>>
>> This is in preparation for adding support for separate stencil and HiZ to
>> intel_miptree_copy_teximage(). When copying a slice of a depthstencil
>> miptree that uses separate stencil, we will also need to copy the
>> corresponding slice of the stencil miptree. The easiest way to do this
>> will be to call intel_miptree_copy_slice() recursively. Analogous
>> reasoning applies to copying a slice of a depth miptree with HiZ.
>>
>> Signed-off-by: Chad Versace
>> ---
>> src/mesa/drivers/dri/intel/intel_mipmap_tree.c | 122
>> +---
>> 1 files changed, 66 insertions(+), 56 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>> b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>> index 7f9e606..8f10101 100644
>> --- a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>> +++ b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>> @@ -354,6 +354,69 @@ intel_miptree_get_image_offset(struct intel_mipmap_tree
>> *mt,
>> }
>> }
>>
>> +static void
>> +intel_miptree_copy_slice(struct intel_context *intel,
>> + struct intel_mipmap_tree *dst_mt,
>> + struct intel_mipmap_tree *src_mt,
>> + int level,
>> + int face,
>> + int depth)
>> +
>> +{
>> + gl_format format = src_mt->format;
>> + uint32_t width = src_mt->level[level].width;
>> + uint32_t height = src_mt->level[level].height;
>> +
>> + assert(depth < src_mt->level[level].depth);
>> +
>> + if (dst_mt->compressed) {
>> + uint32_t align_w, align_h;
>> + intel_get_texture_alignment_unit(format,
>> + &align_w, &align_h);
>> + height = ALIGN(height, align_h) / align_h;
>> + width = ALIGN(width, align_w);
>> + }
>
> This wasn't originally inside the loop; you've effectively moved it
> there. Since intel_get_texture_alignment_unit actually does some work
> these days, I'm wondering if this could be a performance hit.
Patch 36/41 "Store miptree alignment units in the miptree" remove this call
intel_get_texture_alignment_unit(). Does that solve your performance fear?
> At any rate, it doesn't seem necessary...I'd probably just add
> height/width function parameters and move this hunk back.
It feels really strange to me to pass width and height parameters
into intel_miptree_copy_slice(). I imagine someone in the future
encountering the function and being puzzled: "The slice specifier
(level, face, depth) is already passed into intel_miptree_copy_slice().
The width and height are determined by the slice, so why are width and height
also passed? Are we perhaps not copying the entire slice?" I'd rather not
make a potentially confusing design choice for the sake of a potential
performance penalty that will be rendered moot in a future commit.
Chad Versace
chad.vers...@linux.intel.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 10/41] intel: Refactor intel_miptree_copy_teximage()
On 11/18/2011 04:04 PM, Chad Versace wrote:
> On 11/18/2011 03:42 PM, Kenneth Graunke wrote:
>> On 11/17/2011 07:58 PM, Chad Versace wrote:
>>> Extract the body of the inner loop into a new function,
>>> intel_miptree_copy_slice().
>>>
>>> This is in preparation for adding support for separate stencil and HiZ to
>>> intel_miptree_copy_teximage(). When copying a slice of a depthstencil
>>> miptree that uses separate stencil, we will also need to copy the
>>> corresponding slice of the stencil miptree. The easiest way to do this
>>> will be to call intel_miptree_copy_slice() recursively. Analogous
>>> reasoning applies to copying a slice of a depth miptree with HiZ.
>>>
>>> Signed-off-by: Chad Versace
>>> ---
>>> src/mesa/drivers/dri/intel/intel_mipmap_tree.c | 122
>>> +---
>>> 1 files changed, 66 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>>> b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>>> index 7f9e606..8f10101 100644
>>> --- a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>>> +++ b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>>> @@ -354,6 +354,69 @@ intel_miptree_get_image_offset(struct
>>> intel_mipmap_tree *mt,
>>> }
>>> }
>>>
>>> +static void
>>> +intel_miptree_copy_slice(struct intel_context *intel,
>>> +struct intel_mipmap_tree *dst_mt,
>>> +struct intel_mipmap_tree *src_mt,
>>> +int level,
>>> +int face,
>>> +int depth)
>>> +
>>> +{
>>> + gl_format format = src_mt->format;
>>> + uint32_t width = src_mt->level[level].width;
>>> + uint32_t height = src_mt->level[level].height;
>>> +
>>> + assert(depth < src_mt->level[level].depth);
>>> +
>>> + if (dst_mt->compressed) {
>>> + uint32_t align_w, align_h;
>>> + intel_get_texture_alignment_unit(format,
>>> + &align_w, &align_h);
>>> + height = ALIGN(height, align_h) / align_h;
>>> + width = ALIGN(width, align_w);
>>> + }
>>
>> This wasn't originally inside the loop; you've effectively moved it
>> there. Since intel_get_texture_alignment_unit actually does some work
>> these days, I'm wondering if this could be a performance hit.
>
> Patch 36/41 "Store miptree alignment units in the miptree" remove this call
> intel_get_texture_alignment_unit(). Does that solve your performance fear?
Oh... :) *approval*
For both patches (10 and 36):
Reviewed-by: Kenneth Graunke
I definitely like storing the values in the miptree itself.
>> At any rate, it doesn't seem necessary...I'd probably just add
>> height/width function parameters and move this hunk back.
>
> It feels really strange to me to pass width and height parameters
> into intel_miptree_copy_slice(). I imagine someone in the future
> encountering the function and being puzzled: "The slice specifier
> (level, face, depth) is already passed into intel_miptree_copy_slice().
> The width and height are determined by the slice, so why are width and height
> also passed? Are we perhaps not copying the entire slice?" I'd rather not
> make a potentially confusing design choice for the sake of a potential
> performance penalty that will be rendered moot in a future commit.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
