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

[Christian König]

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.

Regards,
Christian.

>
> Cheers,
> Nicolai
>
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98512
>>> Cc: 13.0 <mesa-stable at lists.freedesktop.org>
>>> ---
>>>   src/mesa/state_tracker/st_vdpau.c | 12 ------------
>>>   1 file changed, 12 deletions(-)
>>>
>>> diff --git a/src/mesa/state_tracker/st_vdpau.c
>>> b/src/mesa/state_tracker/st_vdpau.c
>>> index 7912057..770f0ff 100644
>>> --- a/src/mesa/state_tracker/st_vdpau.c
>>> +++ b/src/mesa/state_tracker/st_vdpau.c
>>> @@ -182,21 +182,20 @@ static void
>>>   st_vdpau_map_surface(struct gl_context *ctx, GLenum target, GLenum
>>> access,
>>>                        GLboolean output, struct gl_texture_object
>>> *texObj,
>>>                        struct gl_texture_image *texImage,
>>>                        const void *vdpSurface, GLuint index)
>>>   {
>>>      struct st_context *st = st_context(ctx);
>>>      struct st_texture_object *stObj = st_texture_object(texObj);
>>>      struct st_texture_image *stImage = st_texture_image(texImage);
>>>        struct pipe_resource *res;
>>> -   struct pipe_sampler_view templ, **sampler_view;
>>>      mesa_format texFormat;
>>>        if (output) {
>>>         res = st_vdpau_output_surface_dma_buf(ctx, vdpSurface);
>>>           if (!res)
>>>            res = st_vdpau_output_surface_gallium(ctx, vdpSurface);
>>>        } else {
>>>         res = st_vdpau_video_surface_dma_buf(ctx, vdpSurface, index);
>>> @@ -226,31 +225,20 @@ st_vdpau_map_surface(struct gl_context *ctx,
>>> GLenum target, GLenum access,
>>>      texFormat = st_pipe_format_to_mesa_format(res->format);
>>>        _mesa_init_teximage_fields(ctx, texImage,
>>>                                 res->width0, res->height0, 1, 0, 
>>> GL_RGBA,
>>>                                 texFormat);
>>>        pipe_resource_reference(&stObj->pt, res);
>>>      st_texture_release_all_sampler_views(st, stObj);
>>>      pipe_resource_reference(&stImage->pt, res);
>>>   -   u_sampler_view_default_template(&templ, res, res->format);
>>> -   templ.u.tex.first_layer = index & 1;
>>> -   templ.u.tex.last_layer = index & 1;
>>> -   templ.swizzle_r = GET_SWZ(stObj->base._Swizzle, 0);
>>> -   templ.swizzle_g = GET_SWZ(stObj->base._Swizzle, 1);
>>> -   templ.swizzle_b = GET_SWZ(stObj->base._Swizzle, 2);
>>> -   templ.swizzle_a = GET_SWZ(stObj->base._Swizzle, 3);
>>> -
>>> -   sampler_view = st_texture_get_sampler_view(st, stObj);
>>> -   *sampler_view = st->pipe->create_sampler_view(st->pipe, res, 
>>> &templ);
>>> -
>>>      stObj->surface_format = res->format;
>>>        _mesa_dirty_texobj(ctx, texObj);
>>>      pipe_resource_reference(&res, NULL);
>>>   }
>>>     static void
>>>   st_vdpau_unmap_surface(struct gl_context *ctx, GLenum target, GLenum
>>> access,
>>>                          GLboolean output, struct gl_texture_object
>>> *texObj,
>>>                          struct gl_texture_image *texImage,
>>
>>