On 07.06.2017 19:32, Marek Olšák wrote:
On Wed, Jun 7, 2017 at 1:29 PM, Nicolai Hähnle <nhaeh...@gmail.com> wrote:
On 06.06.2017 11:34, Marek Olšák wrote:
On Tue, Jun 6, 2017 at 4:28 AM, Michel Dänzer <mic...@daenzer.net> wrote:
On 06/06/17 01:50 AM, Marek Olšák wrote:
From: Marek Olšák <marek.ol...@amd.com>
radeonsi won't flush caches if set_framebuffer_state doesn't change
anything.
---
src/mesa/state_tracker/st_cb_drawpixels.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c
b/src/mesa/state_tracker/st_cb_drawpixels.c
index 33d10f6..0ef05ef 100644
--- a/src/mesa/state_tracker/st_cb_drawpixels.c
+++ b/src/mesa/state_tracker/st_cb_drawpixels.c
@@ -1400,20 +1400,27 @@ blit_copy_pixels(struct gl_context *ctx, GLint
srcx, GLint srcy,
st_window_rectangles_to_blit(ctx, &blit);
if (screen->is_format_supported(screen, blit.src.format,
blit.src.resource->target,
blit.src.resource->nr_samples,
PIPE_BIND_SAMPLER_VIEW) &&
screen->is_format_supported(screen, blit.dst.format,
blit.dst.resource->target,
blit.dst.resource->nr_samples,
PIPE_BIND_RENDER_TARGET)) {
+ /* If src == dst, make sure src is coherent with recent dst
+ * updates.
+ */
+ if (blit.src.resource == blit.dst.resource &&
+ screen->get_param(screen, PIPE_CAP_TEXTURE_BARRIER))
+ pipe->texture_barrier(pipe,
PIPE_TEXTURE_BARRIER_SAMPLER);
+
pipe->blit(pipe, &blit);
Maybe this should be handled within the pipe->blit hook? E.g., is this
necessary when using the SDMA engine in radeonsi?
It's not necessary in that case, yet it's a CopyPixels optimization
not worth spending time on.
There are other callers that might be affected by this, though. What about
BlitFramebuffer, for example? That would suggest that this flush should be
handled in si_blit or lower.
IIUC, the issue is sequences of glCopyPixels, glCopyTexImage, and
glBlitFramebuffer that copy within the same texture. All of those need a
flush inserted in between.
And actually, the same operations can be performed by plain old draw calls,
and they will be correct per OpenGL if the app explicitly re-binds the
framebuffer between draw calls.
If there is an issue, it's because the CSO context filters out the
framebuffer changes, right? So perhaps this should actually be fixed in
cso_set_framebuffer, *especially* if the flush is also needed for plain old
draw calls.
cso_set_framebuffer does filter things, but my new radeonsi patch for
si_set_framebuffer_state uses a smarter filtering. IMO,
set_framebuffer_state is a wrong place to do the flush if the state
change is a no-op or only a format change, which is typical for
glEnable(GL_FRAMEBUFFER_SRGB).
Okay, so that's one example where setting the framebuffer state
*doesn't* imply a flush. Are there any others?
The only issue I see with pipe->blit and resource_copy_region is when:
- src == dst
- the current framebuffer is dst
We could put the texture barrier into si_blit and
si_resource_copy_region based those constraints.
I guess that works, too. Though we'd still really need to cover the case
of explicitly re-binding the framebuffer between regular draws. I guess
we need a test for that.
And it'd really suck if the only reason for not handling this via
set_framebuffer_state is that the sRGB toggle takes that path.
It almost makes me want to take the sRGB toggle out of
pipe_framebuffer_state :/
Cheers,
Nicolai
AFAIK, we only have a piglit for CopyPixels, which is fixed by this patch.
Marek
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
