On Tue, Apr 03, 2018 at 11:53:19AM -0700, Scott D Phillips wrote:
> Nanley Chery <nanleych...@gmail.com> writes:
>
> > On Tue, Jan 09, 2018 at 11:17:02PM -0800, Scott D Phillips wrote:
> >> Instead of gtt mapping, call out to other map functions (map_map
> >> or map_tiled_memcpy) for the depth surface. Removes a place where
> >> gtt mapping is used.
> >> ---
> >> This is a bit icky, perhaps something like mapping z_mt with
> >> BRW_MAP_DIRECT_BIT could be cleaner (but in that case the
> >> depthstencil mapping and the DIRECT one would fight for the map
> >> slot in mt->level[level].slice[slice].map).
> >>
> >> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 48
> >> +++++++++++++++++----------
> >> 1 file changed, 30 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> >> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> >> index fa4ae06399..0b9aafe205 100644
> >> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> >> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> >> @@ -3460,16 +3460,21 @@ intel_miptree_map_depthstencil(struct brw_context
> >> *brw,
> >> * temporary buffer back out.
> >> */
> >> if (!(map->mode & GL_MAP_INVALIDATE_RANGE_BIT)) {
> >> + struct intel_miptree_map z_mt_map = {
> >> + .mode = map->mode & ~GL_MAP_WRITE_BIT, .x = map->x, .y = map->y,
> >
> > The old paths were simpler in that they constants instead of map->mode.
> > Why the change?
>
> The reason is that the map/unmap_tiled_memcpy will do the tiling at both
> map and unmap time depending on the mode flags. If we didn't alter the
> flags then both map_depthstencil and unmap_depthstencil could do both a
> read and a write, doubling the memory traffic needlessly.
>
I didn't mean to say that we shouldn't alter the map mode. I was
wondering why we're no longer using constants (e.g. GL_MAP_READ_BIT)
like we did in the intel_miptree_map_raw() calls.
-Nanley
> >> + .w = map->w, .h = map->h,
> >> + };
> >> + if (z_mt->surf.tiling == ISL_TILING_LINEAR)
> >> + intel_miptree_map_map(brw, z_mt, &z_mt_map, level, slice);
> >> + else
> >> + intel_miptree_map_tiled_memcpy(brw, z_mt, &z_mt_map, level,
> >> slice);
> >> + uint32_t *z_map = z_mt_map.ptr;
> >> uint32_t *packed_map = map->ptr;
> >> uint8_t *s_map = intel_miptree_map_raw(brw, s_mt, GL_MAP_READ_BIT);
> >> - uint32_t *z_map = intel_miptree_map_raw(brw, z_mt, GL_MAP_READ_BIT);
> >> unsigned int s_image_x, s_image_y;
> >> - unsigned int z_image_x, z_image_y;
> >>
> >> intel_miptree_get_image_offset(s_mt, level, slice,
> >> &s_image_x, &s_image_y);
> >> - intel_miptree_get_image_offset(z_mt, level, slice,
> >> - &z_image_x, &z_image_y);
> >>
> >> for (uint32_t y = 0; y < map->h; y++) {
> >> for (uint32_t x = 0; x < map->w; x++) {
> >> @@ -3478,9 +3483,7 @@ intel_miptree_map_depthstencil(struct brw_context
> >> *brw,
> >> map_x + s_image_x,
> >> map_y + s_image_y,
> >> brw->has_swizzling);
> >> - ptrdiff_t z_offset = ((map_y + z_image_y) *
> >> - (z_mt->surf.row_pitch / 4) +
> >> - (map_x + z_image_x));
> >> + ptrdiff_t z_offset = y * (z_mt_map.stride / 4) + x;
> >> uint8_t s = s_map[s_offset];
> >> uint32_t z = z_map[z_offset];
> >>
> >> @@ -3494,12 +3497,15 @@ intel_miptree_map_depthstencil(struct brw_context
> >> *brw,
> >> }
> >>
> >> intel_miptree_unmap_raw(s_mt);
> >> - intel_miptree_unmap_raw(z_mt);
> >> + if (z_mt->surf.tiling == ISL_TILING_LINEAR)
> >> + intel_miptree_unmap_map(z_mt);
> >> + else
> >> + intel_miptree_unmap_tiled_memcpy(brw, z_mt, &z_mt_map, level,
> >> slice);
> >>
> >> DBG("%s: %d,%d %dx%d from z mt %p %d,%d, s mt %p %d,%d = %p/%d\n",
> >> __func__,
> >> map->x, map->y, map->w, map->h,
> >> - z_mt, map->x + z_image_x, map->y + z_image_y,
> >> + z_mt, map->x, map->y,
> >
> > I can see this update and the similar one below leading to confusion for
> > a user reading the debug output if they aren't aware of this change. The
> > user may map a rectangle that's not at (level,slice) (0,0) and be
> > surprised that the second x,y coordinate is unchanged from the first.
> > One solution would be to instead print the level and slice for the z mt.
>
> Good idea, I've added that to the patch.
>
> > -Nanley
> >
> >> s_mt, map->x + s_image_x, map->y + s_image_y,
> >> map->ptr, map->stride);
> >> } else {
> >> @@ -3521,16 +3527,21 @@ intel_miptree_unmap_depthstencil(struct
> >> brw_context *brw,
> >> bool map_z32f_x24s8 = mt->format == MESA_FORMAT_Z_FLOAT32;
> >>
> >> if (map->mode & GL_MAP_WRITE_BIT) {
> >> + struct intel_miptree_map z_mt_map = {
> >> + .mode = map->mode | GL_MAP_INVALIDATE_RANGE_BIT, .x = map->x,
> >> + .y = map->y, .w = map->w, .h = map->h,
> >> + };
> >> + if (z_mt->surf.tiling == ISL_TILING_LINEAR)
> >> + intel_miptree_map_map(brw, z_mt, &z_mt_map, level, slice);
> >> + else
> >> + intel_miptree_map_tiled_memcpy(brw, z_mt, &z_mt_map, level,
> >> slice);
> >> + uint32_t *z_map = z_mt_map.ptr;
> >> uint32_t *packed_map = map->ptr;
> >> uint8_t *s_map = intel_miptree_map_raw(brw, s_mt, GL_MAP_WRITE_BIT);
> >> - uint32_t *z_map = intel_miptree_map_raw(brw, z_mt,
> >> GL_MAP_WRITE_BIT);
> >> unsigned int s_image_x, s_image_y;
> >> - unsigned int z_image_x, z_image_y;
> >>
> >> intel_miptree_get_image_offset(s_mt, level, slice,
> >> &s_image_x, &s_image_y);
> >> - intel_miptree_get_image_offset(z_mt, level, slice,
> >> - &z_image_x, &z_image_y);
> >>
> >> for (uint32_t y = 0; y < map->h; y++) {
> >> for (uint32_t x = 0; x < map->w; x++) {
> >> @@ -3538,9 +3549,7 @@ intel_miptree_unmap_depthstencil(struct brw_context
> >> *brw,
> >> x + s_image_x + map->x,
> >> y + s_image_y + map->y,
> >> brw->has_swizzling);
> >> - ptrdiff_t z_offset = ((y + z_image_y + map->y) *
> >> - (z_mt->surf.row_pitch / 4) +
> >> - (x + z_image_x + map->x));
> >> + ptrdiff_t z_offset = y * (z_mt_map.stride / 4) + x;
> >>
> >> if (map_z32f_x24s8) {
> >> z_map[z_offset] = packed_map[(y * map->w + x) * 2 + 0];
> >> @@ -3554,13 +3563,16 @@ intel_miptree_unmap_depthstencil(struct
> >> brw_context *brw,
> >> }
> >>
> >> intel_miptree_unmap_raw(s_mt);
> >> - intel_miptree_unmap_raw(z_mt);
> >> + if (z_mt->surf.tiling == ISL_TILING_LINEAR)
> >> + intel_miptree_unmap_map(z_mt);
> >> + else
> >> + intel_miptree_unmap_tiled_memcpy(brw, z_mt, &z_mt_map, level,
> >> slice);
> >>
> >> DBG("%s: %d,%d %dx%d from z mt %p (%s) %d,%d, s mt %p %d,%d =
> >> %p/%d\n",
> >> __func__,
> >> map->x, map->y, map->w, map->h,
> >> z_mt, _mesa_get_format_name(z_mt->format),
> >> - map->x + z_image_x, map->y + z_image_y,
> >> + map->x, map->y,
> >> s_mt, map->x + s_image_x, map->y + s_image_y,
> >> map->ptr, map->stride);
> >> }
> >> --
> >> 2.14.3
> >>
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
