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

[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...

So at least I don't see how the patch I sent could regress anything 
(even in combination with Brian's earlier commit).

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?

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,
>
>