Module: Mesa Branch: main Commit: eafaac2b1e3c5fa2c347488c2e5c94f895392b81 URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=eafaac2b1e3c5fa2c347488c2e5c94f895392b81
Author: Kenneth Graunke <[email protected]> Date: Tue Oct 4 14:01:27 2022 -0700 iris: Only copy existing data into staging images with PIPE_MAP_READ When performing transfer maps on images that require staging buffers (say, for presenting a linear view of tiled memory), we were reading the existing contents of the buffer into the staging resource on map unless PIPE_MAP_DISCARD_RANGE was set. The thinking was to support partial writes. If you map a subrectangle of an image, but then only write selective pixels - should it preserve the existing contents of the mapped region? I believed that it should, unless you pass PIPE_MAP_DISCARD_RANGE to explicitly say that that it's okay to invalidate the destination region. However, that does not appear to be the interpretation favored by other Mesa developers (in particular Michel Dänzer and Marek Olšák). The radeonsi driver does not do this readback from the destination region to the staging buffer unless you pass PIPE_MAP_READ. If you want to do a partial write and preserve contents, you need to pass both flags: (PIPE_MAP_READ | PIPE_MAP_WRITE). Passing READ is expected to come with an associated cost. OpenGL defines GL_MAP_INVALIDATE_RANGE_BIT for mapping buffer objects, which is translated to PIPE_MAP_DISCARD_RANGE. However, unextended OpenGL doesn't define mapping textures. There are two main sources of image maps: our internal MapTextureImage() hook, and gbm_bo_map(). I've audited our internal MapTextureImage() calls, and while some do pass PIPE_MAP_DISCARD_RANGE, almost all of them wholly overwrite the mapped region, and those that care about combining with existing image contents all pass PIPE_MAP_READ. So this should work there. GBM defines three flags: GBM_BO_TRANSFER_READ, WRITE, and READ_WRITE. There is no defined "invalidate range" bit. In issue #6020, Matthias Treydte notes that this extra readback can cause performance problems, and with iris's current interpretation, there's no way to avoid it. During that discussion, Michel and Matthias both argued that GBM_BO_TRANSFER_WRITE should invalidate the destination contents and avoid the readback, while GBM_BO_TRANSFER_READ_WRITE would preserve it. This patch makes iris follow that model for image mappings, removing readback on staging maps for both detiling and stall avoidance, unless PIPE_MAP_READ is passed. I believe we can change this with impunity. For buffer objects, Ian Romanick and I both agree that partial writes should be supported, and GL_MAP_INVALIDATE_RANGE_BIT exists precisely to indicate that you should spend effort preserving existing contents. So we continue doing readback for buffers unless PIPE_MAP_DISCARD_RANGE is flagged, for now. While I think this is work, it also seems to be undertested in the CTS and Piglit. Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/6020 Reviewed-by: Ian Romanick <[email protected]> Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/19209> --- src/gallium/drivers/iris/iris_resource.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/gallium/drivers/iris/iris_resource.c b/src/gallium/drivers/iris/iris_resource.c index c620ef4cd3e..7f7e2793c3b 100644 --- a/src/gallium/drivers/iris/iris_resource.c +++ b/src/gallium/drivers/iris/iris_resource.c @@ -2069,7 +2069,9 @@ iris_map_copy_region(struct iris_transfer *map) xfer->layer_stride = isl_surf_get_array_pitch(surf); } - if (!(xfer->usage & PIPE_MAP_DISCARD_RANGE)) { + if ((xfer->usage & PIPE_MAP_READ) || + (res->base.b.target == PIPE_BUFFER && + !(xfer->usage & PIPE_MAP_DISCARD_RANGE))) { iris_copy_region(map->blorp, map->batch, map->staging, 0, extra, 0, 0, xfer->resource, xfer->level, box); /* Ensure writes to the staging BO land before we map it below. */ @@ -2207,7 +2209,7 @@ iris_map_s8(struct iris_transfer *map) * invalidate is set, since we'll be writing the whole rectangle from our * temporary buffer back out. */ - if (!(xfer->usage & PIPE_MAP_DISCARD_RANGE)) { + if (xfer->usage & PIPE_MAP_READ) { uint8_t *untiled_s8_map = map->ptr; uint8_t *tiled_s8_map = res->offset + iris_bo_map(map->dbg, res->bo, (xfer->usage | MAP_RAW) & MAP_FLAGS); @@ -2310,7 +2312,7 @@ iris_map_tiled_memcpy(struct iris_transfer *map) const bool has_swizzling = false; - if (!(xfer->usage & PIPE_MAP_DISCARD_RANGE)) { + if (xfer->usage & PIPE_MAP_READ) { char *src = res->offset + iris_bo_map(map->dbg, res->bo, (xfer->usage | MAP_RAW) & MAP_FLAGS);
