[Mesa-stable] [Mesa-dev] [PATCH] st/mesa: fix the layer of VDPAU surface samplers

[Marek Olšák]

On Fri, Nov 4, 2016 at 11:47 AM, Christian König
<deathsimple at vodafone.de> wrote:
> Am 04.11.2016 um 10:11 schrieb Nicolai Hähnle:
>>
>> On 04.11.2016 09:47, Christian König wrote:
>>>
>>> Am 03.11.2016 um 22:06 schrieb Nicolai Hähnle:
>>>>
>>>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>>>
>>>> A latent bug in VDPAU interop was exposed by commit
>>>> e5cc84dd43be066c1dd418e32f5ad258e31a150a.
>>>>
>>>> Before that commit, the st_vdpau code created samplers with
>>>> first_layer == last_layer == 1 that the general texture handling code
>>>> would immediately delete and re-create, because the layer does not match
>>>> the information in the GL texture object.
>>>>
>>>> This was correct behavior, because the imported resource because the
>>>> imported resource is supposed to have the correct offset already
>>>> applied.
>>>>
>>>> After that commit, the state tracker assumes that an existing sampler is
>>>> correct at all times.  Existing samplers are supposed to be deleted when
>>>> they may become invalid, and they will be created on-demand. This meant
>>>> that the sampler with first_layer == last_layer == 1 stuck around,
>>>> leading
>>>> to rendering artefacts (on radeonsi), command stream failures (on
>>>> r600), and
>>>> assertions (in debug builds).
>>>>
>>>> This patch fixes the problem by simply not creating a sampler at all in
>>>> st_vdpau_map_surface and relying on the generic texture code to do the
>>>> right
>>>> thing.
>>>
>>>
>>> A big NAK on this, cause this way GL clearly samples from the wrong
>>> layer for the classic interop case.
>>>
>>> What we probably should rather do is setting index=0 on line 202 when
>>> the DMA-buf import succeeded, cause resources imported using the DMA-buf
>>> method just have one layer.
>>
>>
>> Then I'm confused about how this was ever supposed to work. Take a look at
>> st_get_texture_sampler_view_from_stobj, in particular the problematic commit
>> e5cc84dd43be066c1dd418e32f5ad258e31a150a.
>>
>> Note that before the change, there was:
>>
>>    if (*sv) {
>>       if (check_sampler_swizzle(st, stObj, *sv, glsl_version) ||
>>           (format != (*sv)->format) ||
>>           gl_target_to_pipe(stObj->base.Target) != (*sv)->target ||
>>           stObj->base.MinLevel + stObj->base.BaseLevel !=
>> (*sv)->u.tex.first_level ||
>>           last_level(stObj) != (*sv)->u.tex.last_level ||
>>           stObj->base.MinLayer != (*sv)->u.tex.first_layer ||
>>           last_layer(stObj) != (*sv)->u.tex.last_layer) {
>>          pipe_sampler_view_reference(sv, NULL);
>>       }
>>    }
>>
>>    if (!*sv) {
>>       *sv = st_create_texture_sampler_view_from_stobj(st, stObj,
>>                                                       format,
>> glsl_version);
>>
>> In particular, note the stObj->base.MinLayer != (*sv)->u.tex.first_layer
>> part. Sure, the st_vdpau code would create a sampler with first_layer == 1,
>> but that sampler would almost immediately get replaced by one with
>> first_layer == 0...
>
>
> Mhm, that clearly looks like a bug to me. The samplers from the old
> implementation should clearly stick around, otherwise we would sample from
> the wrong plane.
>
>> So at least I don't see how the patch I sent could regress anything (even
>> in combination with Brian's earlier commit).
>
>
> I'm pretty sure that worked as expected when I wrote the initial code, we
> used to use this with r600 and radeonsi as well. But yes could be that this
> broke in the meantime.
>
>> But yes, it's quite possible that the other path was simply broken before
>> without anybody noticing it. How can I test exercising the non-DMABUF path?
>
>
> Get NVidia hardware, install nouveau and test with Kodi.

Or wait for a nouveau bug report to appear...

We obviously can't set pipe_sampler_view manually in st_vdpau.c. We
can set gl_texture_object though and that should propagate to
pipe_sampler_view automatically.

Marek