[Mesa-dev] [PATCH 5/5] st/mesa: use texture_barrier before CopyPixels blits where src == dst

[Nicolai Hähnle]

On 06.06.2017 11:34, Marek Olšák wrote:
> On Tue, Jun 6, 2017 at 4:28 AM, Michel Dänzer <michel at daenzer.net> wrote:
>> On 06/06/17 01:50 AM, Marek Olšák wrote:
>>> From: Marek Olšák <marek.olsak at 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.

Do we have test cases for these issues? That would really help clarify 
things...

Meanwhile, patches 1-4 are:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>


> 
> Marek
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.