On Fri, Nov 4, 2016 at 11:47 AM, Christian König <[email protected]> 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 <[email protected]> >>>> >>>> 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 _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
