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).
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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
