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

[Nicolai Hähnle]

On 07.06.2017 19:32, Marek Olšák wrote:
> 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).

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.