[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 8:16 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> 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 :/

The sRGB toggle only affects color buffers which were created as SRGB
in the first place. Linear color buffers are unaffected, so a toggle
in pipe_blend_state mapped to CB_COLOR_CONTROL.DEGAMMA_ENABLE won't
work. A toggle in pipe_blend_state re-emitting the framebuffer state
might be OK though. Another problem is that the window-system color
buffers might not be created as SRGB in the first place. That means we
would need an 8-bit srgb_enable mask in pipe_blend_state. Now it's
starting to get complicated for something that's used rarely.

Marek