[Mesa-dev] [PATCH] swr: invalidate attachment on transition change

Discussion:

George Kyriazis

2017-06-20 16:42:14 UTC

Consider the following RT attachment order:
1. Attach surfaces attachments 0 & 1, and render with them
2. Detach 0 & 1
3. Re-attach 0 & 1 to different surfaces
4. Render with the new attachment

The definition of a tile being resolved is that local changes have been
flushed out to the surface, hence there is no need to reload the tile before
it's written to. For an invalid tile, the tile has to be reloaded from
the surface before rendering.

Stage (2) was marking hot tiles for attachements 0 & 1 as RESOLVED,
which means that the hot tiles can be written out to memory with no
need to read them back in (they are "clean"). They need to be marked as
resolved here, because a surface may be destroyed after a detach, and we
don't want to have un-resolved tiles that may force a readback from a
NULL (destroyed) surface. (Part of a destroy is detach all attachments first)

Stage (3), during the no att -> att transition, we need to realize that the
"new" surface tiles need to be fetched fresh from the new surface, instead
of using the resolved tiles, that belong to a stale attachment.

This is done by marking the hot tiles as invalid in stage (3), when we realize
that a new attachment is being made, so that they are re-fetched during
rendering in stage (4).

Also note that hot tiles are indexed by attachment.

- Fixes VTK dual depth-peeling tests.
- No piglit changes
---
src/gallium/drivers/swr/swr_draw.cpp | 19 +++++++++++++++++++
src/gallium/drivers/swr/swr_resource.h | 4 ++++
src/gallium/drivers/swr/swr_state.cpp | 5 +++++
3 files changed, 28 insertions(+)

diff --git a/src/gallium/drivers/swr/swr_draw.cpp b/src/gallium/drivers/swr/swr_draw.cpp
index 03c82a7..ac300e2 100644
--- a/src/gallium/drivers/swr/swr_draw.cpp
+++ b/src/gallium/drivers/swr/swr_draw.cpp
@@ -215,6 +215,25 @@ swr_finish(struct pipe_context *pipe)
swr_fence_reference(pipe->screen, &fence, NULL);
}

+/*
+ * Invalidate tiles so they can be reloaded back when needed
+ */
+void
+swr_invalidate_render_target(struct pipe_context *pipe,
+ uint32_t attachment,
+ uint16_t width, uint16_t height)
+{
+ struct swr_context *ctx = swr_context(pipe);
+
+ /* grab the rect from the passed in arguments */
+ swr_update_draw_context(ctx);
+ SWR_RECT full_rect =
+ {0, 0, (int32_t)width, (int32_t)height};
+ SwrInvalidateTiles(ctx->swrContext,
+ 1 << attachment,
+ full_rect);
+}
+

/*
* Store SWR HotTiles back to renderTarget surface.
diff --git a/src/gallium/drivers/swr/swr_resource.h b/src/gallium/drivers/swr/swr_resource.h
index ae9954c..4effd46 100644
--- a/src/gallium/drivers/swr/swr_resource.h
+++ b/src/gallium/drivers/swr/swr_resource.h
@@ -96,6 +96,10 @@ swr_resource_data(struct pipe_resource *resource)
}

+void swr_invalidate_render_target(struct pipe_context *pipe,
+ uint32_t attachment,
+ uint16_t width, uint16_t height);
+
void swr_store_render_target(struct pipe_context *pipe,
uint32_t attachment,
enum SWR_TILE_STATE post_tile_state);
diff --git a/src/gallium/drivers/swr/swr_state.cpp b/src/gallium/drivers/swr/swr_state.cpp
index 08549e5..deae4e6 100644
--- a/src/gallium/drivers/swr/swr_state.cpp
+++ b/src/gallium/drivers/swr/swr_state.cpp
@@ -933,6 +933,11 @@ swr_change_rt(struct swr_context *ctx,
* INVALID so they are reloaded from surface. */
swr_store_render_target(&ctx->pipe, attachment, SWR_TILE_INVALID);
need_fence = true;
+ } else {
+ /* if no previous attachment, invalidate tiles that may be marked
+ * RESOLVED because of an old attachment */
+ swr_invalidate_render_target(&ctx->pipe, attachment, sf->width, sf->height);
+ /* no need to set fence here */
}

/* Make new attachment */

--
2.7.4

Rowley, Timothy O

2017-06-22 14:15:09 UTC

Permalink

Reviewed-by: Tim Rowley <***@intel.com<mailto:***@intel.com>>

On Jun 20, 2017, at 11:42 AM, George Kyriazis <***@intel.com<mailto:***@intel.com>> wrote:

Consider the following RT attachment order:
1. Attach surfaces attachments 0 & 1, and render with them
2. Detach 0 & 1
3. Re-attach 0 & 1 to different surfaces
4. Render with the new attachment

The definition of a tile being resolved is that local changes have been
flushed out to the surface, hence there is no need to reload the tile before
it's written to. For an invalid tile, the tile has to be reloaded from
the surface before rendering.

Stage (2) was marking hot tiles for attachements 0 & 1 as RESOLVED,
which means that the hot tiles can be written out to memory with no
need to read them back in (they are "clean"). They need to be marked as
resolved here, because a surface may be destroyed after a detach, and we
don't want to have un-resolved tiles that may force a readback from a
NULL (destroyed) surface. (Part of a destroy is detach all attachments first)

Stage (3), during the no att -> att transition, we need to realize that the
"new" surface tiles need to be fetched fresh from the new surface, instead
of using the resolved tiles, that belong to a stale attachment.

This is done by marking the hot tiles as invalid in stage (3), when we realize
that a new attachment is being made, so that they are re-fetched during
rendering in stage (4).

Also note that hot tiles are indexed by attachment.

- Fixes VTK dual depth-peeling tests.
- No piglit changes
---
src/gallium/drivers/swr/swr_draw.cpp | 19 +++++++++++++++++++
src/gallium/drivers/swr/swr_resource.h | 4 ++++
src/gallium/drivers/swr/swr_state.cpp | 5 +++++
3 files changed, 28 insertions(+)

diff --git a/src/gallium/drivers/swr/swr_draw.cpp b/src/gallium/drivers/swr/swr_draw.cpp
index 03c82a7..ac300e2 100644
--- a/src/gallium/drivers/swr/swr_draw.cpp
+++ b/src/gallium/drivers/swr/swr_draw.cpp
@@ -215,6 +215,25 @@ swr_finish(struct pipe_context *pipe)
swr_fence_reference(pipe->screen, &fence, NULL);
}

+/*
+ * Invalidate tiles so they can be reloaded back when needed
+ */
+void
+swr_invalidate_render_target(struct pipe_context *pipe,
+ uint32_t attachment,
+ uint16_t width, uint16_t height)
+{
+ struct swr_context *ctx = swr_context(pipe);
+
+ /* grab the rect from the passed in arguments */
+ swr_update_draw_context(ctx);
+ SWR_RECT full_rect =
+ {0, 0, (int32_t)width, (int32_t)height};
+ SwrInvalidateTiles(ctx->swrContext,
+ 1 << attachment,
+ full_rect);
+}
+

/*
* Store SWR HotTiles back to renderTarget surface.
diff --git a/src/gallium/drivers/swr/swr_resource.h b/src/gallium/drivers/swr/swr_resource.h
index ae9954c..4effd46 100644
--- a/src/gallium/drivers/swr/swr_resource.h
+++ b/src/gallium/drivers/swr/swr_resource.h
@@ -96,6 +96,10 @@ swr_resource_data(struct pipe_resource *resource)
}

+void swr_invalidate_render_target(struct pipe_context *pipe,
+ uint32_t attachment,
+ uint16_t width, uint16_t height);
+
void swr_store_render_target(struct pipe_context *pipe,
uint32_t attachment,
enum SWR_TILE_STATE post_tile_state);
diff --git a/src/gallium/drivers/swr/swr_state.cpp b/src/gallium/drivers/swr/swr_state.cpp
index 08549e5..deae4e6 100644
--- a/src/gallium/drivers/swr/swr_state.cpp
+++ b/src/gallium/drivers/swr/swr_state.cpp
@@ -933,6 +933,11 @@ swr_change_rt(struct swr_context *ctx,
* INVALID so they are reloaded from surface. */
swr_store_render_target(&ctx->pipe, attachment, SWR_TILE_INVALID);
need_fence = true;
+ } else {
+ /* if no previous attachment, invalidate tiles that may be marked
+ * RESOLVED because of an old attachment */
+ swr_invalidate_render_target(&ctx->pipe, attachment, sf->width, sf->height);
+ /* no need to set fence here */
}

/* Make new attachment */
--
2.7.4

Andres Gomez

2017-07-07 21:11:10 UTC

Permalink

George, would we want this patch in -stable or we shouldn't bother ?

Post by George Kyriazis
1. Attach surfaces attachments 0 & 1, and render with them
2. Detach 0 & 1
3. Re-attach 0 & 1 to different surfaces
4. Render with the new attachment
The definition of a tile being resolved is that local changes have been
flushed out to the surface, hence there is no need to reload the tile before
it's written to. For an invalid tile, the tile has to be reloaded from
the surface before rendering.
Stage (2) was marking hot tiles for attachements 0 & 1 as RESOLVED,
which means that the hot tiles can be written out to memory with no
need to read them back in (they are "clean"). They need to be marked as
resolved here, because a surface may be destroyed after a detach, and we
don't want to have un-resolved tiles that may force a readback from a
NULL (destroyed) surface. (Part of a destroy is detach all attachments first)
Stage (3), during the no att -> att transition, we need to realize that the
"new" surface tiles need to be fetched fresh from the new surface, instead
of using the resolved tiles, that belong to a stale attachment.
This is done by marking the hot tiles as invalid in stage (3), when we realize
that a new attachment is being made, so that they are re-fetched during
rendering in stage (4).
Also note that hot tiles are indexed by attachment.
- Fixes VTK dual depth-peeling tests.
- No piglit changes
---
src/gallium/drivers/swr/swr_draw.cpp | 19 +++++++++++++++++++
src/gallium/drivers/swr/swr_resource.h | 4 ++++
src/gallium/drivers/swr/swr_state.cpp | 5 +++++
3 files changed, 28 insertions(+)
diff --git a/src/gallium/drivers/swr/swr_draw.cpp b/src/gallium/drivers/swr/swr_draw.cpp
index 03c82a7..ac300e2 100644
--- a/src/gallium/drivers/swr/swr_draw.cpp
+++ b/src/gallium/drivers/swr/swr_draw.cpp
@@ -215,6 +215,25 @@ swr_finish(struct pipe_context *pipe)
swr_fence_reference(pipe->screen, &fence, NULL);
}
+/*
+ * Invalidate tiles so they can be reloaded back when needed
+ */
+void
+swr_invalidate_render_target(struct pipe_context *pipe,
+ uint32_t attachment,
+ uint16_t width, uint16_t height)
+{
+ struct swr_context *ctx = swr_context(pipe);
+
+ /* grab the rect from the passed in arguments */
+ swr_update_draw_context(ctx);
+ SWR_RECT full_rect =
+ {0, 0, (int32_t)width, (int32_t)height};
+ SwrInvalidateTiles(ctx->swrContext,
+ 1 << attachment,
+ full_rect);
+}
+
/*
* Store SWR HotTiles back to renderTarget surface.
diff --git a/src/gallium/drivers/swr/swr_resource.h b/src/gallium/drivers/swr/swr_resource.h
index ae9954c..4effd46 100644
--- a/src/gallium/drivers/swr/swr_resource.h
+++ b/src/gallium/drivers/swr/swr_resource.h
@@ -96,6 +96,10 @@ swr_resource_data(struct pipe_resource *resource)
}
+void swr_invalidate_render_target(struct pipe_context *pipe,
+ uint32_t attachment,
+ uint16_t width, uint16_t height);
+
void swr_store_render_target(struct pipe_context *pipe,
uint32_t attachment,
enum SWR_TILE_STATE post_tile_state);
diff --git a/src/gallium/drivers/swr/swr_state.cpp b/src/gallium/drivers/swr/swr_state.cpp
index 08549e5..deae4e6 100644
--- a/src/gallium/drivers/swr/swr_state.cpp
+++ b/src/gallium/drivers/swr/swr_state.cpp
@@ -933,6 +933,11 @@ swr_change_rt(struct swr_context *ctx,
* INVALID so they are reloaded from surface. */
swr_store_render_target(&ctx->pipe, attachment, SWR_TILE_INVALID);
need_fence = true;
+ } else {
+ /* if no previous attachment, invalidate tiles that may be marked
+ * RESOLVED because of an old attachment */
+ swr_invalidate_render_target(&ctx->pipe, attachment, sf->width, sf->height);
+ /* no need to set fence here */
}
/* Make new attachment */

--
Br,

Andres

Kyriazis, George

2017-07-07 21:32:21 UTC

Permalink

+Bruce

Andres,

VTK/Kitware has already modified their code to enable dual depth peeling for Mesa version >= 17.2. From that perspective, it doesn’t matter if the change goes into the 17.1.x releases or 17.2.

Having said that, we haven’t seen any issues with that check-in, so if it makes it to a release branch earlier than 17.2, it’s still a good thing for us.

Thanks!

George

Post by Andres Gomez
George, would we want this patch in -stable or we shouldn't bother ?

--
Br,
Andres