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

[Marek Olšák]

On Wed, Jun 7, 2017 at 1:29 PM, Nicolai Hähnle <nhaehnle at 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 <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.

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).

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.

AFAIK, we only have a piglit for CopyPixels, which is fixed by this patch.

Marek