[Mesa-dev] [PATCH 2/5] i965/miptree: Use cpu tiling/detiling when mapping
Scott D Phillips
scott.d.phillips at intel.com
Tue Mar 6 16:01:47 UTC 2018
Jason Ekstrand <jason at jlekstrand.net> writes:
> On Mon, Mar 5, 2018 at 11:39 AM, Nanley Chery <nanleychery at gmail.com> wrote:
>
>> On Tue, Jan 09, 2018 at 11:16:59PM -0800, Scott D Phillips wrote:
>> > Rename the (un)map_gtt functions to (un)map_map (map by
>> > returning a map) and add new functions (un)map_tiled_memcpy that
>> > return a shadow buffer populated with the intel_tiled_memcpy
>> > functions.
>>
>> Could you mention some of the rationale?
One is that gtt maps cannot detile the newer Yf and Ys layouts. Not sure
if Jason or Ken know other reasons. I think we suspected some
performance difference, but I didn't really see any with well formed
bulk uploads or downloads.
>> > ---
>> > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 95
>> ++++++++++++++++++++++++---
>> > 1 file changed, 86 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> > index ead0c359c0..7a90dafa1e 100644
>> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> > @@ -31,6 +31,7 @@
>> > #include "intel_image.h"
>> > #include "intel_mipmap_tree.h"
>> > #include "intel_tex.h"
>> > +#include "intel_tiled_memcpy.h"
>> > #include "intel_blit.h"
>> > #include "intel_fbo.h"
>> >
>> > @@ -3031,10 +3032,10 @@ intel_miptree_unmap_raw(struct intel_mipmap_tree
>> *mt)
>> > }
>> >
>> > static void
>> > -intel_miptree_map_gtt(struct brw_context *brw,
>> > - struct intel_mipmap_tree *mt,
>> > - struct intel_miptree_map *map,
>> > - unsigned int level, unsigned int slice)
>> > +intel_miptree_map_map(struct brw_context *brw,
>> > + struct intel_mipmap_tree *mt,
>> > + struct intel_miptree_map *map,
>> > + unsigned int level, unsigned int slice)
>> > {
>> > unsigned int bw, bh;
>> > void *base;
>> > @@ -3052,7 +3053,7 @@ intel_miptree_map_gtt(struct brw_context *brw,
>> > y /= bh;
>> > x /= bw;
>> >
>> > - base = intel_miptree_map_raw(brw, mt, map->mode);
>> > + base = intel_miptree_map_raw(brw, mt, map->mode | MAP_RAW);
>> >
>> > if (base == NULL)
>> > map->ptr = NULL;
>> > @@ -3078,11 +3079,80 @@ intel_miptree_map_gtt(struct brw_context *brw,
>> > }
>> >
>> > static void
>> > -intel_miptree_unmap_gtt(struct intel_mipmap_tree *mt)
>> > +intel_miptree_unmap_map(struct intel_mipmap_tree *mt)
>> > {
>> > intel_miptree_unmap_raw(mt);
>> > }
>> >
>> > +/* Compute extent parameters for use with tiled_memcpy functions.
>> > + * xs are in units of bytes and ys are in units of strides. */
>> > +static inline void
>> > +tile_extents(struct intel_mipmap_tree *mt, struct intel_miptree_map
>> *map,
>> > + unsigned int level, unsigned int slice, unsigned int *x1,
>> > + unsigned int *x2, unsigned int *y1, unsigned int *y2)
>>
>> It would be nice to give these variables units:
>> x1_B, y1_el, etc.
Sure, will do
>> > +{
>> > + unsigned int block_width, block_height, block_bytes;
>> > + unsigned int x0_el, y0_el;
>> > +
>> > + _mesa_get_format_block_size(mt->format, &block_width,
>> &block_height);
>> > + block_bytes = _mesa_get_format_bytes(mt->format);
>>
>> block_bytes == mt->cpp (in theory anyways)
Ok, will change to that
>> > +
>> > + assert(map->x % block_width == 0);
>> > + assert(map->y % block_height == 0);
>> > +
>> > + intel_miptree_get_image_offset(mt, level, slice, &x0_el, &y0_el);
>> > + *x1 = (map->x / block_width + x0_el) * block_bytes;
>> > + *y1 = map->y / block_height + y0_el;
>> > + *x2 = *x1 + DIV_ROUND_UP(map->w, block_width) * block_bytes;
>> > + *y2 = *y1 + DIV_ROUND_UP(map->h, block_height);
>> > +}
>> > +
>> > +static void
>> > +intel_miptree_map_tiled_memcpy(struct brw_context *brw,
>> > + struct intel_mipmap_tree *mt,
>> > + struct intel_miptree_map *map,
>> > + unsigned int level, unsigned int slice)
>> > +{
>> > + unsigned int x1, x2, y1, y2;
>> > + tile_extents(mt, map, level, slice, &x1, &x2, &y1, &y2);
>> > + map->stride = _mesa_format_row_stride(mt->format, map->w);
>> > + map->buffer = map->ptr = malloc(map->stride * (y2 - y1));
>>
>> Using _mesa_align_malloc should improve memory accesses for our 128bit
>> formats.
>>
>> We should probably also assert(map->ptr) or throw a GL_OUT_OF_MEMORY
>> error if the alloc fails.
Good suggestions, will do that
>> > +
>> > + if (!(map->mode & GL_MAP_INVALIDATE_RANGE_BIT)) {
>> > + char *src = intel_miptree_map_raw(brw, mt, map->mode | MAP_RAW);
>> > + src += mt->offset;
>> > +
>> > + tiled_to_linear(x1, x2, y1, y2, map->ptr, src, map->stride,
>> > + mt->surf.row_pitch, brw->has_swizzling,
>> mt->surf.tiling,
>> > + memcpy);
>> > +
>> > + intel_miptree_unmap_raw(mt);
>> > + }
>> > +}
>> > +
>> > +static void
>> > +intel_miptree_unmap_tiled_memcpy(struct brw_context *brw,
>> > + struct intel_mipmap_tree *mt,
>> > + struct intel_miptree_map *map,
>> > + unsigned int level,
>> > + unsigned int slice)
>> > +{
>> > + if (map->mode & GL_MAP_WRITE_BIT) {
>> > + unsigned int x1, x2, y1, y2;
>> > + tile_extents(mt, map, level, slice, &x1, &x2, &y1, &y2);
>> > +
>> > + char *dst = intel_miptree_map_raw(brw, mt, map->mode | MAP_RAW);
>> > + dst += mt->offset;
>> > +
>> > + linear_to_tiled(x1, x2, y1, y2, dst, map->ptr, mt->surf.row_pitch,
>> > + map->stride, brw->has_swizzling, mt->surf.tiling,
>> memcpy);
>> > +
>> > + intel_miptree_unmap_raw(mt);
>> > + }
>> > + free(map->buffer);
>> > + map->buffer = map->ptr = NULL;
>> > +}
>> > +
>> > static void
>> > intel_miptree_map_blit(struct brw_context *brw,
>> > struct intel_mipmap_tree *mt,
>> > @@ -3640,8 +3710,10 @@ intel_miptree_map(struct brw_context *brw,
>> > (mt->surf.row_pitch % 16 == 0)) {
>> > intel_miptree_map_movntdqa(brw, mt, map, level, slice);
>> > #endif
>> > + } else if (mt->surf.tiling != ISL_TILING_LINEAR) {
>>
>> What happens to clients who try to map a tiled surface with the MAP_RAW
>> flag set? It looks like they expect a tiled buffer but get a linear one
>> instead.
MAP_RAW is just a construct of brw_bufmgr, not a normal mapping flag, so
that's not a case that can happen currently. We could add a check here
for that and go to map_map if we add some use like that in the future.
>> Also, in a number of places where tiling memcpy is used, the following
>> restriction is present (though the comment varies):
>>
>> /* linear_to_tiled() assumes that if the object is swizzled, it is using
>> * I915_BIT6_SWIZZLE_9_10 for X and I915_BIT6_SWIZZLE_9 for Y. This is
>> only
>> * true on gen5 and above.
>> *
>> * The killer on top is that some gen4 have an L-shaped swizzle mode,
>> where
>> * parts of the memory aren't swizzled at all. Userspace just can't
>> handle
>> * that.
>> */
>> if (devinfo->gen < 5 && brw->has_swizzling)
>> return false;
>>
>
> Yikes. I didn't realize that gen4 still had crazy swizzling. I'm a bit
> surprised this didn't come up in testing.
Hmm, ya looking at i915_gem_fence_reg.c, it looks like just i965gm and
gm45 could have this, and just some memory controller configurations on
those platforms, not all. I'm guessing our CI doesn't have such a
machine with such a configuration, or it should have failed tests.
I guess we just need to stick with gtt maps on gen4.
> --Jason
>
>
>> > + intel_miptree_map_tiled_memcpy(brw, mt, map, level, slice);
>> > } else {
>> > - intel_miptree_map_gtt(brw, mt, map, level, slice);
>> > + intel_miptree_map_map(brw, mt, map, level, slice);
>> > }
>> >
>> > *out_ptr = map->ptr;
>> > @@ -3677,11 +3749,16 @@ intel_miptree_unmap(struct brw_context *brw,
>> > } else if (map->linear_mt) {
>> > intel_miptree_unmap_blit(brw, mt, map, level, slice);
>> > #if defined(USE_SSE41)
>> > - } else if (map->buffer && cpu_has_sse4_1) {
>> > + } else if (!(map->mode & GL_MAP_WRITE_BIT) &&
>> > + !mt->compressed && cpu_has_sse4_1 &&
>> > + (mt->surf.row_pitch % 16 == 0) &&
>> > + map->buffer) {
>>
>> This hunk looks unrelated. Should this be a separate patch?
This is actually needed so that unmap_movntdqa doesn't get called for
maps that were made with map_tiled_memcpy. The tests for map/unmap
movntdqa were asymetric, but there was not a case before that would
result in asymetric behavior.
More information about the mesa-dev
mailing list