<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Mar 5, 2018 at 11:39 AM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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? <div><div class="h5"> > --- > 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) </div></div>It would be nice to give these variables units: x1_B, y1_el, etc. > +{ > + 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) > + > + 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. <div><div class="h5"> > + > + 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) { </div></div>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. 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; </blockquote><div> </div><div>Yikes. I didn't realize that gen4 still had crazy swizzling. I'm a bit surprised this didn't come up in testing. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + 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? -Nanley > intel_miptree_unmap_movntdqa(brw, mt, map, level, slice); > #endif > + } else if (mt->surf.tiling != ISL_TILING_LINEAR) { > + intel_miptree_unmap_tiled_memcpy(brw, mt, map, level, slice); > } else { > - intel_miptree_unmap_gtt(mt); > + intel_miptree_unmap_map(mt); > } > > intel_miptree_release_map(mt, level, slice); > -- > 2.14.3 > <div class="HOEnZb"><div class="h5">> _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>