[Mesa-dev] [Bug 91123] [swrast] piglit egl-create-pbuffer-surface regression
https://bugs.freedesktop.org/show_bug.cgi?id=91123 --- Comment #1 from Marek Olšák --- The bisected commit can't have caused any regression. The problem is that you didn't enable GLES1 and GLES2 in Mesa, so the test skipped (or passed) before that. It's possible that this isn't even a regression, because swrast EGL support if pretty new. -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] egl/x11: handle when invalid drawable is passed in create_surface
Reviewed-by: Marek Olšák
Marek
On Fri, Jun 26, 2015 at 2:04 PM, Emil Velikov wrote:
> 0 is not used as a valid drawable id, as such there is no point in
> attempting to query it's geometry. Just bail out early and provide the
> more meaningful EGL_BAD_NATIVE_WINDOW to the user.
>
> Signed-off-by: Emil Velikov
> ---
> src/egl/drivers/dri2/platform_x11.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/src/egl/drivers/dri2/platform_x11.c
> b/src/egl/drivers/dri2/platform_x11.c
> index 56c1428..9114074 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -235,6 +235,10 @@ dri2_x11_create_surface(_EGLDriver *drv, _EGLDisplay
> *disp, EGLint type,
> dri2_surf->drawable, screen->root,
> dri2_surf->base.Width, dri2_surf->base.Height);
> } else {
> + if (!drawable) {
> + _eglError(EGL_BAD_NATIVE_WINDOW, "dri2_create_surface");
> + goto cleanup_surf;
> + }
>dri2_surf->drawable = drawable;
> }
>
> --
> 2.4.4
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] egl/drm: plug memory leak
For the whole series:
Reviewed-by: Marek Olšák
Marek
On Fri, Jun 26, 2015 at 2:04 PM, Emil Velikov wrote:
> Free the memory for dri2_surf in the unlikely case that one provides
> NULL for native_window. Also set the relevant EGL_ERROR to provide
> feedback to the user.
>
> Signed-off-by: Emil Velikov
> ---
> src/egl/drivers/dri2/platform_drm.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/platform_drm.c
> b/src/egl/drivers/dri2/platform_drm.c
> index a62da41..0d1f4c6 100644
> --- a/src/egl/drivers/dri2/platform_drm.c
> +++ b/src/egl/drivers/dri2/platform_drm.c
> @@ -115,8 +115,11 @@ dri2_drm_create_surface(_EGLDriver *drv, _EGLDisplay
> *disp, EGLint type,
>
> switch (type) {
> case EGL_WINDOW_BIT:
> - if (!window)
> - return NULL;
> + if (!window) {
> + _eglError(EGL_BAD_NATIVE_WINDOW, "dri2_create_surface");
> + goto cleanup_surf;
> + }
> +
>surf = gbm_dri_surface(window);
>dri2_surf->gbm_surf = surf;
>dri2_surf->base.Width = surf->base.width;
> --
> 2.4.4
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 5/5] osx: fix asm support on darwin
On 19/06/2015 00:36, Julien Isorce wrote: On 18 June 2015 at 19:46, Emil Velikov wrote: On 18 June 2015 at 06:53, Julien Isorce wrote: Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90908 I don't think it's necessary to create a bugzilla entry just to link to it in your patch. A bit more commentary about what this patch is doing and why it is right would help review it. This patch seems to be doing 2 things: Changing xform4.S to use assyntax.h, and turning on the use of asm for darwin, so might be best broken in two. diff --git a/src/mesa/x86-64/xform4.S b/src/mesa/x86-64/xform4.S index c185f62..17eb7fa 100644 --- a/src/mesa/x86-64/xform4.S +++ b/src/mesa/x86-64/xform4.S @@ -24,14 +24,15 @@ #ifdef USE_X86_64_ASM +#include "x86/assyntax.h" #include "matypes.h" .text .align 16 -.globl _mesa_x86_64_cpuid -.hidden _mesa_x86_64_cpuid -_mesa_x86_64_cpuid: +GLOBL GLNAME(_mesa_x86_64_cpuid) +HIDDEN(_mesa_x86_64_cpuid) +GLNAME(_mesa_x86_64_cpuid): Whilst the use of HIDDEN is necessary, I'm don't think there are any x86_64 platforms which use a leading underscore, so I'm not sure if using GLNAME makes sense in an x86_64 specific file. - + +#if defined (__ELF__) && defined (__linux__) .section .rodata +#endif This looks wrong. I don't see why this is linux/ELF specific. Perhaps something like: #ifdef __APPLE__ .const #else .section .rodata #endif maybe that needs to be conditional on __MACH__? maybe that needs to explicitly use a .section directive with segment and section? --- a/src/mesa/x86/assyntax.h +++ b/src/mesa/x86/assyntax.h @@ -255,7 +255,7 @@ #endif /* ACK_ASSEMBLER */ -#if defined(__QNX__) || defined(Lynx) || (defined(SYSV) || defined(SVR4)) && !defined(ACK_ASSEMBLER) || defined(__ELF__) || defined(__GNU__) || defined(__GNUC__) && !defined(__MINGW32__) +#if defined(__QNX__) || defined(Lynx) || (defined(SYSV) || defined(SVR4)) && !defined(ACK_ASSEMBLER) || defined(__ELF__) || defined(__GNU__) || defined(__GNUC__) && !defined(__MINGW32__) && !defined(__APPLE__) #define GLNAME(a) a #else #define GLNAME(a) CONCAT(_,a) Considering that this is a fragile area in mesa, can you confirm what kind of testing you've done ? Would be nice to avoid breaking things in various subtle ways. Very minimal tests: only glxgears and es2gears_x11, on osx. Also "make check" succeeds until it reaches glx-test which fails to build (link error). I need to submit a bug. In my OSX dev enviroment (which I haven't used for a year :D), I have CFLAGS containing "-arch i386 -arch x86_64" (because that's what [1] says to use). With this patch applied, that fails to build for i386. That's a tricky problem to solve within the mesa build system. I don't know if building for OSX i386 is useful anymore? [1] http://xquartz.macosforge.org/trac/wiki/DeveloperInfo ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 6/6] mesa/main: free locale at exit
On Fri, Jun 26, 2015 at 9:05 PM, Brian Paul wrote: > On 06/26/2015 12:06 PM, Erik Faye-Lund wrote: >> >> In order to save a small leak if mesa is continously loaded and >> unloaded, let's free the locale when the shared object is unloaded. >> >> Signed-off-by: Erik Faye-Lund >> Reviewed-by: Matt Turner >> --- >> src/mesa/main/context.c | 12 +++- >> src/util/strtod.c | 8 >> src/util/strtod.h | 3 +++ >> 3 files changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c >> index e68de68..dee1fa8 100644 >> --- a/src/mesa/main/context.c >> +++ b/src/mesa/main/context.c >> @@ -346,6 +346,16 @@ _mesa_destroy_visual( struct gl_config *vis ) >> mtx_t OneTimeLock = _MTX_INITIALIZER_NP; >> >> >> +/** >> + * Calls all the various one-time-fini functions in Mesa >> + */ >> + >> +static void >> +one_time_fini() > > > I think that should be one_time_fini(void) to be consistent and to avoid > warnings with some compilers. Good eyes, I've fixed that locally. Thanks :) > Otherwise, the series looks good to me. Nice to see patch 1. I remember > writing that dumb code years ago. > > Reviewed-by: Brian Paul Thanks. Is that Reviewed-by for this patch, or the whole series? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/5] applegl: Provide requirements of _SET_DrawBuffers
On 19/06/2015 12:51, Emil Velikov wrote: On 19 June 2015 at 00:05, Julien Isorce wrote: Sorry for removing the XXX line. Original message is here: https://bugs.freedesktop.org/attachment.cgi?id=115539 At the time src/glx/apple/apple_glapi.c has been developed this patch was not needed I guess so I wonder which change made the regression. I'm not sure bisecting is going to work too well, as you might find it's apple-glx isn't buildable for most of the range, and you will probably land on a commit which say "rewrite glapi (again)" :D That's one way to get to the bottom of it. Alternatively we can take a look at how other glx (indirect, xlib) handle it. As is, the patch gets a big fat NACK, sorry. This smells to me like there's something not quite right about the partitioning of stuff between libmesa and libglapi, but I don't have any insight into what exactly the problem is. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 90817] swrast fails to load with certain remote X servers
https://bugs.freedesktop.org/show_bug.cgi?id=90817 --- Comment #5 from Jon TURNEY --- Something like this is also needed when the server is XWin on Cygwin/MinGW using WGL, where the set of fbConfigs comes from the native driver and don't match swrast's expectations. -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 91106] glx: make check fails to build on osx
https://bugs.freedesktop.org/show_bug.cgi?id=91106 --- Comment #3 from Jon TURNEY --- (In reply to Emil Velikov from comment #1) > +#if defined(GLX_USE_APPLEGL) > +#define _ATTRIBUTE_WEAK __attribute__((weak)) > +#else > +#define _ATTRIBUTE_WEAK > +#endif > + > (If we end up keeping this) Can we avoid redefining it X times ? Ian (the > original author of these tests) should know the details as to why/how we > don't end up with duplicated/conflicting symbols. These really are duplicates that the test harness provides to override the real versions. Unfortunately, ld on OSX doesn't behave that way, and the option -multiply_defined suppress seems to be obsolete. I'm not very keen on scattering __attribute__(weak) around to fix this, but I guess the only other approach is to split those symbols out into a separate object, which is linked with to produce libglx.a, but not into a convenience library which is used by the tests. > + $(top_builddir)/src/mapi/glapi/libglapi.la \ > Which symbols are missing if we omit this ? Noone else seems to need it. glapi_create_table_from_handle, which only exists on APPLE, if memory serves. But linking with glapi and shared-glapi doesn't seem right, so I think this is a sign that something else is wrong. -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 91106] glx: make check fails to build on osx
https://bugs.freedesktop.org/show_bug.cgi?id=91106 --- Comment #4 from Jon TURNEY --- (In reply to Emil Velikov from comment #2) > Iirc Jon had some > branches which rework/clean things up which seems like the way forward imho. Sorry, all the clean-up I did for __APPLE__ is already upstream, with the exception of the 2 patches which clearly need to be done in a better way. -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
I had a quick look at enabling this for freedreno.. although looks
like it is going to take some work in tgsi_to_nir and/or nir.. the
problem is we end up w/ a register array (rather than variable array
like we do for TEMP arrays).. ie. for something copying from a TEMP
array to an OUT array, we end up like:
vec4 ssa_35 = intrinsic load_var () (arr_2[0]) ()
vec4 ssa_36 = fmov ssa_35
r1[0] = fmov ssa_36
vec4 ssa_37 = intrinsic load_var () (arr_2[1]) ()
vec4 ssa_38 = fmov ssa_37
r1[1] = fmov ssa_38
vec4 ssa_39 = intrinsic load_var () (arr_2[2]) ()
vec4 ssa_40 = fmov ssa_39
r1[2] = fmov ssa_40
vec4 ssa_41 = intrinsic load_var () (arr_2[3]) ()
vec4 ssa_42 = fmov ssa_41
r1[3] = fmov ssa_42
vec4 ssa_43 = fmov r2
r0 = fmov ssa_43
intrinsic store_output (r0) () (0)
intrinsic store_output (r1[0]) () (1)
intrinsic store_output (r1[1]) () (2)
intrinsic store_output (r1[2]) () (3)
intrinsic store_output (r1[3]) () (4)
the r1[] array ends up not getting converted into SSA and hitting an
assert when we try to lower to scalar.. I'm not really sure the best
way to handle this
BR,
-R
On Wed, Jun 24, 2015 at 12:31 PM, Marek Olšák wrote:
> Yes, ArrayID != 0 means it's an array, otherwise it's just a compact
> way to add declarations.
>
> I recently added the array support for inputs and outputs, we just
> need to enable it on non-radeon non-swrast drivers:
> http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6ebe7eabf54936a02acc0968e718e0c264a73f5
>
> It would be nice to enable the arrays on all drivers instead of
> working around it indefinitely.
>
> Marek
>
> On Wed, Jun 24, 2015 at 1:31 PM, Rob Clark wrote:
>> Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it
>> is safe to assume that we always get ArrayID that makes it much
>> easier.
>>
>> BR,
>> -R
>>
>> On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote:
>>> It's not an array, because the ArrayID is 0. It's a valid non-array
>>> declaration. If any TGSI user doesn't understand it, that user should
>>> be fixed.
>>>
>>> Marek
>>>
>>> On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote:
From: Rob Clark
Ok, so actually there is a ttn issue here to fix as well.. but it
brought up a question in my mind. When ttn sees something like
DCL IN[0..1]
it will treat that as an array (which in the end will result in
constraints about where the registers get allocated. Which is not
really ideal.
With glsl we don't actually get input arrays (but instead a bunch
of MOV's to a TEMP array) currently. So I'm not quite sure how
an actual input array should look. (But my preference would be
IN[a..b] for arrays and IN[c] otherwise)
---
src/gallium/auxiliary/hud/hud_context.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/gallium/auxiliary/hud/hud_context.c
b/src/gallium/auxiliary/hud/hud_context.c
index 6a124f7..2b6d3a7 100644
--- a/src/gallium/auxiliary/hud/hud_context.c
+++ b/src/gallium/auxiliary/hud/hud_context.c
@@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct
cso_context *cso)
{
static const char *vertex_shader_text = {
"VERT\n"
- "DCL IN[0..1]\n"
+ "DCL IN[0]\n"
+ "DCL IN[1]\n"
"DCL OUT[0], POSITION\n"
"DCL OUT[1], COLOR[0]\n" /* color */
"DCL OUT[2], GENERIC[0]\n" /* texcoord */
--
2.4.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] gallium: add PIPE_COMPUTE_CAP_SUBGROUP_SIZE
Grigori Goronzy writes:
> We need this to implement OpenCL's
> CL_KERNEL_PREFERRED_WORK_GROUP_SIZE_MULTIPLE.
Reviewed-by: Francisco Jerez
Thanks.
> ---
> src/gallium/docs/source/screen.rst | 2 ++
> src/gallium/drivers/ilo/ilo_screen.c | 8
> src/gallium/drivers/nouveau/nvc0/nvc0_screen.c | 4
> src/gallium/drivers/radeon/r600_pipe_common.c | 6 ++
> src/gallium/drivers/radeon/r600_pipe_common.h | 20
> src/gallium/include/pipe/p_defines.h | 3 ++-
> 6 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/docs/source/screen.rst
> b/src/gallium/docs/source/screen.rst
> index 416ef2d..32c1e87 100644
> --- a/src/gallium/docs/source/screen.rst
> +++ b/src/gallium/docs/source/screen.rst
> @@ -382,6 +382,8 @@ pipe_screen::get_compute_param.
>Value type: ``uint32_t``
> * ``PIPE_COMPUTE_CAP_IMAGES_SUPPORTED``: Whether images are supported
>non-zero means yes, zero means no. Value type: ``uint32_t``
> +* ``PIPE_COMPUTE_CAP_SUBGROUP_SIZE``: The size of a basic execution unit in
> + threads. Also known as wavefront size, warp size or SIMD width.
>
> .. _pipe_bind:
>
> diff --git a/src/gallium/drivers/ilo/ilo_screen.c
> b/src/gallium/drivers/ilo/ilo_screen.c
> index b0fed73..f2a18b2 100644
> --- a/src/gallium/drivers/ilo/ilo_screen.c
> +++ b/src/gallium/drivers/ilo/ilo_screen.c
> @@ -195,6 +195,7 @@ ilo_get_compute_param(struct pipe_screen *screen,
>uint32_t max_clock_frequency;
>uint32_t max_compute_units;
>uint32_t images_supported;
> + uint32_t subgroup_size;
> } val;
> const void *ptr;
> int size;
> @@ -286,6 +287,13 @@ ilo_get_compute_param(struct pipe_screen *screen,
>ptr = &val.images_supported;
>size = sizeof(val.images_supported);
>break;
> + case PIPE_COMPUTE_CAP_SUBGROUP_SIZE:
> + /* best case is SIMD32 */
> + val.subgroup_size = 32;
> +
> + ptr = &val.subgroup_size;
> + size = sizeof(val.subgroup_size);
> + break;
> default:
>ptr = NULL;
>size = 0;
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> index 1ca997a..f6bef83 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> @@ -340,6 +340,7 @@ nvc0_screen_get_compute_param(struct pipe_screen *pscreen,
>enum pipe_compute_cap param, void *data)
> {
> uint64_t *data64 = (uint64_t *)data;
> + uint32_t *data32 = (uint32_t *)data;
> const uint16_t obj_class = nvc0_screen(pscreen)->compute->oclass;
>
> switch (param) {
> @@ -371,6 +372,9 @@ nvc0_screen_get_compute_param(struct pipe_screen *pscreen,
> case PIPE_COMPUTE_CAP_MAX_INPUT_SIZE: /* c[], arbitrary limit */
>data64[0] = 4096;
>return 8;
> + case PIPE_COMPUTE_CAP_SUBGROUP_SIZE:
> + data32[0] = 32;
> + return 4;
> default:
>return 0;
> }
> diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c
> b/src/gallium/drivers/radeon/r600_pipe_common.c
> index 42e681d..5494cb3 100644
> --- a/src/gallium/drivers/radeon/r600_pipe_common.c
> +++ b/src/gallium/drivers/radeon/r600_pipe_common.c
> @@ -637,6 +637,12 @@ static int r600_get_compute_param(struct pipe_screen
> *screen,
> return sizeof(uint32_t);
> case PIPE_COMPUTE_CAP_MAX_PRIVATE_SIZE:
> break; /* unused */
> + case PIPE_COMPUTE_CAP_SUBGROUP_SIZE:
> + if (ret) {
> + uint32_t *subgroup_size = ret;
> + *subgroup_size = r600_wavefront_size(rscreen->family);
> + }
> + return sizeof(uint32_t);
> }
>
> fprintf(stderr, "unknown PIPE_COMPUTE_CAP %d\n", param);
> diff --git a/src/gallium/drivers/radeon/r600_pipe_common.h
> b/src/gallium/drivers/radeon/r600_pipe_common.h
> index 6ce81d3..51fd016 100644
> --- a/src/gallium/drivers/radeon/r600_pipe_common.h
> +++ b/src/gallium/drivers/radeon/r600_pipe_common.h
> @@ -570,6 +570,26 @@ static inline unsigned r600_tex_aniso_filter(unsigned
> filter)
>/* else */return 4;
> }
>
> +static inline unsigned r600_wavefront_size(enum radeon_family family)
> +{
> + switch (family) {
> + case CHIP_RV610:
> + case CHIP_RS780:
> + case CHIP_RV620:
> + case CHIP_RS880:
> + return 16;
> + case CHIP_RV630:
> + case CHIP_RV635:
> + case CHIP_RV730:
> + case CHIP_RV710:
> + case CHIP_PALM:
> + case CHIP_CEDAR:
> + return 32;
> + default:
> + return 64;
> + }
> +}
> +
> #define COMPUTE_DBG(rscreen, fmt, args...) \
> do { \
> if ((rscreen->b.debug_flags & DBG_COMPUTE)) fprintf(stderr,
> fmt, ##args); \
> diff --git a/src/gallium/include/pipe/p_defines.h
> b/src/gallium/include/pipe/p_defines.h
> index 8fabf5e..b50ae2b 100644
>
[Mesa-dev] [PATCH 5/7] gallium/ttn: don't upset nir_validate w/ BRK's
From: Rob Clark
Previously we were unconditionally doing ttn_get_src() even for
instructions with no src's. Which created a lot of unnecessary
load_const instructions. These were mostly harmless since NIR opt
passes would strip them back out. But for an ENDIF following a
BRK, it would result in load_const instructions created after the
NIR break instruction. Which nir_validate dislikes.
But we can actually just dtrt by using NumSrcRegs instead.
Signed-off-by: Rob Clark
---
src/gallium/auxiliary/nir/tgsi_to_nir.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
b/src/gallium/auxiliary/nir/tgsi_to_nir.c
index 1ad8c68..bf7eb2f 100644
--- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
+++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
@@ -1510,7 +1510,7 @@ ttn_emit_instruction(struct ttn_compile *c)
return;
nir_ssa_def *src[TGSI_FULL_MAX_SRC_REGISTERS];
- for (i = 0; i < TGSI_FULL_MAX_SRC_REGISTERS; i++) {
+ for (i = 0; i < tgsi_inst->Instruction.NumSrcRegs; i++) {
src[i] = ttn_get_src(c, &tgsi_inst->Src[i]);
}
nir_alu_dest dest = ttn_get_dest(c, tgsi_dst);
--
2.4.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 4/7] gallium/ttn: add TXB2
From: Rob Clark
Signed-off-by: Rob Clark
---
src/gallium/auxiliary/nir/tgsi_to_nir.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
b/src/gallium/auxiliary/nir/tgsi_to_nir.c
index e202ce6..1ad8c68 100644
--- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
+++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
@@ -1087,6 +1087,11 @@ ttn_tex(struct ttn_compile *c, nir_alu_dest dest,
nir_ssa_def **src)
op = nir_texop_txb;
num_srcs = 2;
break;
+ case TGSI_OPCODE_TXB2:
+ op = nir_texop_txb;
+ num_srcs = 2;
+ samp = 2;
+ break;
case TGSI_OPCODE_TXL:
op = nir_texop_txl;
num_srcs = 2;
@@ -1188,6 +1193,12 @@ ttn_tex(struct ttn_compile *c, nir_alu_dest dest,
nir_ssa_def **src)
src_number++;
}
+ if (tgsi_inst->Instruction.Opcode == TGSI_OPCODE_TXB2) {
+ instr->src[src_number].src = nir_src_for_ssa(ttn_channel(b, src[1], X));
+ instr->src[src_number].src_type = nir_tex_src_bias;
+ src_number++;
+ }
+
if (tgsi_inst->Instruction.Opcode == TGSI_OPCODE_TXL) {
instr->src[src_number].src = nir_src_for_ssa(ttn_channel(b, src[0], W));
instr->src[src_number].src_type = nir_tex_src_lod;
--
2.4.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 7/7] nir: cleanup open-coded instruction casts
From: Rob Clark
Signed-off-by: Rob Clark
---
src/glsl/nir/nir_lower_alu_to_scalar.c | 2 +-
src/glsl/nir/nir_lower_vec_to_movs.c | 2 +-
src/glsl/nir/nir_search.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/glsl/nir/nir_lower_alu_to_scalar.c
b/src/glsl/nir/nir_lower_alu_to_scalar.c
index 25bba4e..5d15fb2 100644
--- a/src/glsl/nir/nir_lower_alu_to_scalar.c
+++ b/src/glsl/nir/nir_lower_alu_to_scalar.c
@@ -164,7 +164,7 @@ lower_alu_to_scalar_block(nir_block *block, void *data)
{
nir_foreach_instr_safe(block, instr) {
if (instr->type == nir_instr_type_alu)
- lower_alu_instr_scalar((nir_alu_instr *)instr, data);
+ lower_alu_instr_scalar(nir_instr_as_alu(instr), data);
}
return true;
diff --git a/src/glsl/nir/nir_lower_vec_to_movs.c
b/src/glsl/nir/nir_lower_vec_to_movs.c
index 602853e..e6d522f 100644
--- a/src/glsl/nir/nir_lower_vec_to_movs.c
+++ b/src/glsl/nir/nir_lower_vec_to_movs.c
@@ -90,7 +90,7 @@ lower_vec_to_movs_block(nir_block *block, void *mem_ctx)
if (instr->type != nir_instr_type_alu)
continue;
- nir_alu_instr *vec = (nir_alu_instr *)instr;
+ nir_alu_instr *vec = nir_instr_as_alu(instr);
switch (vec->op) {
case nir_op_vec2:
diff --git a/src/glsl/nir/nir_search.c b/src/glsl/nir/nir_search.c
index 0c4e48c..c33d6c3 100644
--- a/src/glsl/nir/nir_search.c
+++ b/src/glsl/nir/nir_search.c
@@ -48,7 +48,7 @@ src_is_bool(nir_src src)
return false;
if (src.ssa->parent_instr->type != nir_instr_type_alu)
return false;
- return alu_instr_is_bool((nir_alu_instr *)src.ssa->parent_instr);
+ return alu_instr_is_bool(nir_instr_as_alu(src.ssa->parent_instr));
}
static bool
--
2.4.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/7] gallium/ttn: partial fix for output arrays
From: Rob Clark
It isn't quite yet practical to enable TGSI_ANY_INOUT_DECL_RANGE shader
cap yet, at least not in drivers that need lower_to_scalar pass (which
right now is all of the ttn users), since the register arrays do not get
converted to SSA, which angers nir_lower_alu_to_scalar.
Signed-off-by: Rob Clark
---
src/gallium/auxiliary/nir/tgsi_to_nir.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
b/src/gallium/auxiliary/nir/tgsi_to_nir.c
index a189559..e202ce6 100644
--- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
+++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
@@ -1735,9 +1735,11 @@ ttn_add_output_stores(struct ttn_compile *c)
for (i = 0; i < array_len; i++) {
nir_intrinsic_instr *store =
nir_intrinsic_instr_create(b->shader, nir_intrinsic_store_output);
+ unsigned loc = var->data.driver_location + i;
store->num_components = 4;
- store->const_index[0] = var->data.driver_location + i;
- store->src[0].reg.reg = c->output_regs[var->data.driver_location].reg;
+ store->const_index[0] = loc;
+ store->src[0].reg.reg = c->output_regs[loc].reg;
+ store->src[0].reg.base_offset = c->output_regs[loc].offset;
nir_instr_insert_after_cf_list(b->cf_node_list, &store->instr);
}
}
--
2.4.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/7] gallium/ttn: IN/OUT are only array if ArrayID != 0
From: Rob Clark
Signed-off-by: Rob Clark
---
src/gallium/auxiliary/nir/tgsi_to_nir.c | 143 ++--
1 file changed, 81 insertions(+), 62 deletions(-)
diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
b/src/gallium/auxiliary/nir/tgsi_to_nir.c
index 065bbf0..a189559 100644
--- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
+++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
@@ -184,7 +184,8 @@ ttn_emit_declaration(struct ttn_compile *c)
c->samp_types[decl->Range.First + i] = type;
}
} else {
- nir_variable *var;
+ bool is_array = (array_size > 1);
+
assert(file == TGSI_FILE_INPUT ||
file == TGSI_FILE_OUTPUT ||
file == TGSI_FILE_CONSTANT);
@@ -193,76 +194,94 @@ ttn_emit_declaration(struct ttn_compile *c)
if ((file == TGSI_FILE_CONSTANT) && decl->Declaration.Dimension)
return;
- var = rzalloc(b->shader, nir_variable);
- var->data.driver_location = decl->Range.First;
-
- var->type = glsl_vec4_type();
- if (array_size > 1)
- var->type = glsl_array_type(var->type, array_size);
+ if ((file == TGSI_FILE_INPUT) || (file == TGSI_FILE_OUTPUT)) {
+ is_array = (is_array && decl->Declaration.Array &&
+ (decl->Array.ArrayID != 0));
+ }
- switch (file) {
- case TGSI_FILE_INPUT:
- var->data.read_only = true;
- var->data.mode = nir_var_shader_in;
- var->name = ralloc_asprintf(var, "in_%d", decl->Range.First);
+ for (i = 0; i < array_size; i++) {
+ unsigned idx = decl->Range.First + i;
+ nir_variable *var = rzalloc(b->shader, nir_variable);
- /* We should probably translate to a VERT_ATTRIB_* or VARYING_SLOT_*
- * instead, but nothing in NIR core is looking at the value
- * currently, and this is less change to drivers.
- */
- var->data.location = decl->Semantic.Name;
- var->data.index = decl->Semantic.Index;
+ var->data.driver_location = idx;
- /* We definitely need to translate the interpolation field, because
- * nir_print will decode it.
- */
- switch (decl->Interp.Interpolate) {
- case TGSI_INTERPOLATE_CONSTANT:
-var->data.interpolation = INTERP_QUALIFIER_FLAT;
-break;
- case TGSI_INTERPOLATE_LINEAR:
-var->data.interpolation = INTERP_QUALIFIER_NOPERSPECTIVE;
-break;
- case TGSI_INTERPOLATE_PERSPECTIVE:
-var->data.interpolation = INTERP_QUALIFIER_SMOOTH;
-break;
- }
-
- exec_list_push_tail(&b->shader->inputs, &var->node);
- break;
- case TGSI_FILE_OUTPUT: {
- /* Since we can't load from outputs in the IR, we make temporaries
- * for the outputs and emit stores to the real outputs at the end of
- * the shader.
- */
- nir_register *reg = nir_local_reg_create(b->impl);
- reg->num_components = 4;
- if (array_size > 1)
-reg->num_array_elems = array_size;
+ var->type = glsl_vec4_type();
+ if (is_array)
+var->type = glsl_array_type(var->type, array_size);
- var->data.mode = nir_var_shader_out;
- var->name = ralloc_asprintf(var, "out_%d", decl->Range.First);
+ switch (file) {
+ case TGSI_FILE_INPUT:
+var->data.read_only = true;
+var->data.mode = nir_var_shader_in;
+var->name = ralloc_asprintf(var, "in_%d", idx);
- var->data.location = decl->Semantic.Name;
- var->data.index = decl->Semantic.Index;
+/* We should probably translate to a VERT_ATTRIB_* or
VARYING_SLOT_*
+ * instead, but nothing in NIR core is looking at the value
+ * currently, and this is less change to drivers.
+ */
+var->data.location = decl->Semantic.Name;
+var->data.index = decl->Semantic.Index;
- for (i = 0; i < array_size; i++) {
-c->output_regs[decl->Range.First + i].offset = i;
-c->output_regs[decl->Range.First + i].reg = reg;
+/* We definitely need to translate the interpolation field, because
+ * nir_print will decode it.
+ */
+switch (decl->Interp.Interpolate) {
+case TGSI_INTERPOLATE_CONSTANT:
+ var->data.interpolation = INTERP_QUALIFIER_FLAT;
+ break;
+case TGSI_INTERPOLATE_LINEAR:
+ var->data.interpolation = INTERP_QUALIFIER_NOPERSPECTIVE;
+ break;
+case TGSI_INTERPOLATE_PERSPECTIVE:
+ var->data.interpolation = INTERP_QUALIFIER_SMOOTH;
+ break;
+}
+
+exec_list_push_tail(&b->shader->inputs, &var->node);
+break;
+ case TGSI_FILE_OUTPUT: {
+/* Since we can't load from outputs in the IR, we make temporari
[Mesa-dev] [PATCH 6/7] nir/lower_phis_to_scalar: undef is trivially scalarizable
From: Rob Clark Signed-off-by: Rob Clark --- src/glsl/nir/nir_lower_phis_to_scalar.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/glsl/nir/nir_lower_phis_to_scalar.c b/src/glsl/nir/nir_lower_phis_to_scalar.c index a57d253..739170d 100644 --- a/src/glsl/nir/nir_lower_phis_to_scalar.c +++ b/src/glsl/nir/nir_lower_phis_to_scalar.c @@ -75,6 +75,7 @@ is_phi_src_scalarizable(nir_phi_src *src, return should_lower_phi(nir_instr_as_phi(src_instr), state); case nir_instr_type_load_const: + case nir_instr_type_ssa_undef: /* These are trivially scalarizable */ return true; -- 2.4.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/7] smattering of tgsi/ttn/nir fixes
From: Rob Clark Mostly ttn fixes, plus a couple small nir fixes/cleanups, and a doc patch to clairify ArrayID usage Rob Clark (7): tgsi: update docs for ArrayID usage gallium/ttn: IN/OUT are only array if ArrayID != 0 gallium/ttn: partial fix for output arrays gallium/ttn: add TXB2 gallium/ttn: don't upset nir_validate w/ BRK's nir/lower_phis_to_scalar: undef is trivially scalarizable nir: cleanup open-coded instruction casts src/gallium/auxiliary/nir/tgsi_to_nir.c | 162 +++- src/gallium/docs/source/tgsi.rst| 1 + src/glsl/nir/nir_lower_alu_to_scalar.c | 2 +- src/glsl/nir/nir_lower_phis_to_scalar.c | 1 + src/glsl/nir/nir_lower_vec_to_movs.c| 2 +- src/glsl/nir/nir_search.c | 2 +- 6 files changed, 102 insertions(+), 68 deletions(-) -- 2.4.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/7] tgsi: update docs for ArrayID usage
From: Rob Clark Signed-off-by: Rob Clark --- src/gallium/docs/source/tgsi.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/src/gallium/docs/source/tgsi.rst b/src/gallium/docs/source/tgsi.rst index 89ca172..4e869e7 100644 --- a/src/gallium/docs/source/tgsi.rst +++ b/src/gallium/docs/source/tgsi.rst @@ -2603,6 +2603,7 @@ not relative to the specified declaration If no ArrayID is specified with an indirect addressing operand the whole register file might be accessed by this operand. This is strongly discouraged and will prevent packing of scalar/vec2 arrays and effective alias analysis. +This is only legal for TEMP and CONST register files. Declaration Semantic -- 2.4.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 7/7] nir: cleanup open-coded instruction casts
Thanks!
R-B me
On Jun 27, 2015 7:57 AM, "Rob Clark" wrote:
> From: Rob Clark
>
> Signed-off-by: Rob Clark
> ---
> src/glsl/nir/nir_lower_alu_to_scalar.c | 2 +-
> src/glsl/nir/nir_lower_vec_to_movs.c | 2 +-
> src/glsl/nir/nir_search.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/glsl/nir/nir_lower_alu_to_scalar.c
> b/src/glsl/nir/nir_lower_alu_to_scalar.c
> index 25bba4e..5d15fb2 100644
> --- a/src/glsl/nir/nir_lower_alu_to_scalar.c
> +++ b/src/glsl/nir/nir_lower_alu_to_scalar.c
> @@ -164,7 +164,7 @@ lower_alu_to_scalar_block(nir_block *block, void *data)
> {
> nir_foreach_instr_safe(block, instr) {
>if (instr->type == nir_instr_type_alu)
> - lower_alu_instr_scalar((nir_alu_instr *)instr, data);
> + lower_alu_instr_scalar(nir_instr_as_alu(instr), data);
> }
>
> return true;
> diff --git a/src/glsl/nir/nir_lower_vec_to_movs.c
> b/src/glsl/nir/nir_lower_vec_to_movs.c
> index 602853e..e6d522f 100644
> --- a/src/glsl/nir/nir_lower_vec_to_movs.c
> +++ b/src/glsl/nir/nir_lower_vec_to_movs.c
> @@ -90,7 +90,7 @@ lower_vec_to_movs_block(nir_block *block, void *mem_ctx)
>if (instr->type != nir_instr_type_alu)
> continue;
>
> - nir_alu_instr *vec = (nir_alu_instr *)instr;
> + nir_alu_instr *vec = nir_instr_as_alu(instr);
>
>switch (vec->op) {
>case nir_op_vec2:
> diff --git a/src/glsl/nir/nir_search.c b/src/glsl/nir/nir_search.c
> index 0c4e48c..c33d6c3 100644
> --- a/src/glsl/nir/nir_search.c
> +++ b/src/glsl/nir/nir_search.c
> @@ -48,7 +48,7 @@ src_is_bool(nir_src src)
>return false;
> if (src.ssa->parent_instr->type != nir_instr_type_alu)
>return false;
> - return alu_instr_is_bool((nir_alu_instr *)src.ssa->parent_instr);
> + return alu_instr_is_bool(nir_instr_as_alu(src.ssa->parent_instr));
> }
>
> static bool
> --
> 2.4.3
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] Allow setting GL_TEXTURE_COMPARE_MODE on a sampler object without ARB_shadow support.
On 26 June 2015 at 17:38, Brian Paul wrote: > Digging up the patch from March for reference: > >> This fixes a GL error warning on r200 in Wine. >> >> The GL_ARB_sampler_objects extension does not specify a dependency on >> GL_ARB_shadow or GL_ARB_depth_texture for this value. Just set the value >> and don't do anything else. It won't matter without a depth texture >> being assigned anyway. > > So I take it that Wine calls glSamplerParameteri(s, GL_TEXTURE_COMPARE_MODE, > mode) even when GL_ARB_shadow is not supported and you get a bunch of GL > errors? > > Has this been reported upstream to Wine so they can fix it? > Stefan is part of upstream, so yeah, we're aware of this. > I see this sort of thing all the time in Windows OpenGL apps and it's > tempting to just silence some obscure GL errors, but it's a slippery slope. > When I'm debugging a new GL app, seeing GL errors for all the corner cases > can actually be very helpful and I'd hate to lose that. > Well, the argument is that it's actually a bug in Mesa (if perhaps an obscure one) to generate an GL error in this case. We could add a workaround in Wine if we really have to, but the way we're reading the ARB_sampler_objects spec that shouldn't be needed. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 6/6] mesa/main: free locale at exit
On Sat, Jun 27, 2015 at 5:08 AM, Erik Faye-Lund wrote: > On Fri, Jun 26, 2015 at 9:05 PM, Brian Paul wrote: > > On 06/26/2015 12:06 PM, Erik Faye-Lund wrote: > >> > >> In order to save a small leak if mesa is continously loaded and > >> unloaded, let's free the locale when the shared object is unloaded. > >> > >> Signed-off-by: Erik Faye-Lund > >> Reviewed-by: Matt Turner > >> --- > >> src/mesa/main/context.c | 12 +++- > >> src/util/strtod.c | 8 > >> src/util/strtod.h | 3 +++ > >> 3 files changed, 22 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c > >> index e68de68..dee1fa8 100644 > >> --- a/src/mesa/main/context.c > >> +++ b/src/mesa/main/context.c > >> @@ -346,6 +346,16 @@ _mesa_destroy_visual( struct gl_config *vis ) > >> mtx_t OneTimeLock = _MTX_INITIALIZER_NP; > >> > >> > >> +/** > >> + * Calls all the various one-time-fini functions in Mesa > >> + */ > >> + > >> +static void > >> +one_time_fini() > > > > > > I think that should be one_time_fini(void) to be consistent and to avoid > > warnings with some compilers. > > Good eyes, I've fixed that locally. Thanks :) > > > Otherwise, the series looks good to me. Nice to see patch 1. I remember > > writing that dumb code years ago. > > > > Reviewed-by: Brian Paul > > Thanks. Is that Reviewed-by for this patch, or the whole series? > Whole series. -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
On Sat, Jun 27, 2015 at 6:48 AM, Rob Clark wrote:
> I had a quick look at enabling this for freedreno.. although looks
> like it is going to take some work in tgsi_to_nir and/or nir.. the
> problem is we end up w/ a register array (rather than variable array
> like we do for TEMP arrays).. ie. for something copying from a TEMP
> array to an OUT array, we end up like:
>
> vec4 ssa_35 = intrinsic load_var () (arr_2[0]) ()
> vec4 ssa_36 = fmov ssa_35
> r1[0] = fmov ssa_36
> vec4 ssa_37 = intrinsic load_var () (arr_2[1]) ()
> vec4 ssa_38 = fmov ssa_37
> r1[1] = fmov ssa_38
> vec4 ssa_39 = intrinsic load_var () (arr_2[2]) ()
> vec4 ssa_40 = fmov ssa_39
> r1[2] = fmov ssa_40
> vec4 ssa_41 = intrinsic load_var () (arr_2[3]) ()
> vec4 ssa_42 = fmov ssa_41
> r1[3] = fmov ssa_42
> vec4 ssa_43 = fmov r2
> r0 = fmov ssa_43
> intrinsic store_output (r0) () (0)
> intrinsic store_output (r1[0]) () (1)
> intrinsic store_output (r1[1]) () (2)
> intrinsic store_output (r1[2]) () (3)
> intrinsic store_output (r1[3]) () (4)
>
> the r1[] array ends up not getting converted into SSA and hitting an
> assert when we try to lower to scalar.. I'm not really sure the best
> way to handle this
Well, the frontend shouldn't really be generating register arrays...
they're for backends, not frontends, and it's curently impossible to
eliminate them without extra information about the stride that I've
been hesitant to add, and even then they're more of a pain for
optimization passes. I'm not sure the exact place where ttn is
emitting a register array, but if you fix it to use a variable
instead, vars_to_ssa should clean it up.
>
> BR,
> -R
>
> On Wed, Jun 24, 2015 at 12:31 PM, Marek Olšák wrote:
>> Yes, ArrayID != 0 means it's an array, otherwise it's just a compact
>> way to add declarations.
>>
>> I recently added the array support for inputs and outputs, we just
>> need to enable it on non-radeon non-swrast drivers:
>> http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6ebe7eabf54936a02acc0968e718e0c264a73f5
>>
>> It would be nice to enable the arrays on all drivers instead of
>> working around it indefinitely.
>>
>> Marek
>>
>> On Wed, Jun 24, 2015 at 1:31 PM, Rob Clark wrote:
>>> Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it
>>> is safe to assume that we always get ArrayID that makes it much
>>> easier.
>>>
>>> BR,
>>> -R
>>>
>>> On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote:
It's not an array, because the ArrayID is 0. It's a valid non-array
declaration. If any TGSI user doesn't understand it, that user should
be fixed.
Marek
On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote:
> From: Rob Clark
>
> Ok, so actually there is a ttn issue here to fix as well.. but it
> brought up a question in my mind. When ttn sees something like
>
> DCL IN[0..1]
>
> it will treat that as an array (which in the end will result in
> constraints about where the registers get allocated. Which is not
> really ideal.
>
> With glsl we don't actually get input arrays (but instead a bunch
> of MOV's to a TEMP array) currently. So I'm not quite sure how
> an actual input array should look. (But my preference would be
> IN[a..b] for arrays and IN[c] otherwise)
> ---
> src/gallium/auxiliary/hud/hud_context.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/auxiliary/hud/hud_context.c
> b/src/gallium/auxiliary/hud/hud_context.c
> index 6a124f7..2b6d3a7 100644
> --- a/src/gallium/auxiliary/hud/hud_context.c
> +++ b/src/gallium/auxiliary/hud/hud_context.c
> @@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct
> cso_context *cso)
> {
>static const char *vertex_shader_text = {
> "VERT\n"
> - "DCL IN[0..1]\n"
> + "DCL IN[0]\n"
> + "DCL IN[1]\n"
> "DCL OUT[0], POSITION\n"
> "DCL OUT[1], COLOR[0]\n" /* color */
> "DCL OUT[2], GENERIC[0]\n" /* texcoord */
> --
> 2.4.3
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
On Sat, Jun 27, 2015 at 12:57 PM, Connor Abbott wrote:
> On Sat, Jun 27, 2015 at 6:48 AM, Rob Clark wrote:
>> I had a quick look at enabling this for freedreno.. although looks
>> like it is going to take some work in tgsi_to_nir and/or nir.. the
>> problem is we end up w/ a register array (rather than variable array
>> like we do for TEMP arrays).. ie. for something copying from a TEMP
>> array to an OUT array, we end up like:
>>
>> vec4 ssa_35 = intrinsic load_var () (arr_2[0]) ()
>> vec4 ssa_36 = fmov ssa_35
>> r1[0] = fmov ssa_36
>> vec4 ssa_37 = intrinsic load_var () (arr_2[1]) ()
>> vec4 ssa_38 = fmov ssa_37
>> r1[1] = fmov ssa_38
>> vec4 ssa_39 = intrinsic load_var () (arr_2[2]) ()
>> vec4 ssa_40 = fmov ssa_39
>> r1[2] = fmov ssa_40
>> vec4 ssa_41 = intrinsic load_var () (arr_2[3]) ()
>> vec4 ssa_42 = fmov ssa_41
>> r1[3] = fmov ssa_42
>> vec4 ssa_43 = fmov r2
>> r0 = fmov ssa_43
>> intrinsic store_output (r0) () (0)
>> intrinsic store_output (r1[0]) () (1)
>> intrinsic store_output (r1[1]) () (2)
>> intrinsic store_output (r1[2]) () (3)
>> intrinsic store_output (r1[3]) () (4)
>>
>> the r1[] array ends up not getting converted into SSA and hitting an
>> assert when we try to lower to scalar.. I'm not really sure the best
>> way to handle this
>
> Well, the frontend shouldn't really be generating register arrays...
> they're for backends, not frontends, and it's curently impossible to
> eliminate them without extra information about the stride that I've
> been hesitant to add, and even then they're more of a pain for
> optimization passes. I'm not sure the exact place where ttn is
> emitting a register array, but if you fix it to use a variable
> instead, vars_to_ssa should clean it up.
I was thinking about this, since I'd already done the same for TEMP
file arrays..
How should that end up looking, though, from the perspective of
store_output? Can store_output reference a variable, or do I have to
do a move into ssa and then pass the ssa src to store_output
(effectively duplicating the lowering IN/OUT arrays that tgsi
currently does in ttn)
BR,
-R
>>
>> BR,
>> -R
>>
>> On Wed, Jun 24, 2015 at 12:31 PM, Marek Olšák wrote:
>>> Yes, ArrayID != 0 means it's an array, otherwise it's just a compact
>>> way to add declarations.
>>>
>>> I recently added the array support for inputs and outputs, we just
>>> need to enable it on non-radeon non-swrast drivers:
>>> http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6ebe7eabf54936a02acc0968e718e0c264a73f5
>>>
>>> It would be nice to enable the arrays on all drivers instead of
>>> working around it indefinitely.
>>>
>>> Marek
>>>
>>> On Wed, Jun 24, 2015 at 1:31 PM, Rob Clark wrote:
Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it
is safe to assume that we always get ArrayID that makes it much
easier.
BR,
-R
On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote:
> It's not an array, because the ArrayID is 0. It's a valid non-array
> declaration. If any TGSI user doesn't understand it, that user should
> be fixed.
>
> Marek
>
> On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote:
>> From: Rob Clark
>>
>> Ok, so actually there is a ttn issue here to fix as well.. but it
>> brought up a question in my mind. When ttn sees something like
>>
>> DCL IN[0..1]
>>
>> it will treat that as an array (which in the end will result in
>> constraints about where the registers get allocated. Which is not
>> really ideal.
>>
>> With glsl we don't actually get input arrays (but instead a bunch
>> of MOV's to a TEMP array) currently. So I'm not quite sure how
>> an actual input array should look. (But my preference would be
>> IN[a..b] for arrays and IN[c] otherwise)
>> ---
>> src/gallium/auxiliary/hud/hud_context.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/auxiliary/hud/hud_context.c
>> b/src/gallium/auxiliary/hud/hud_context.c
>> index 6a124f7..2b6d3a7 100644
>> --- a/src/gallium/auxiliary/hud/hud_context.c
>> +++ b/src/gallium/auxiliary/hud/hud_context.c
>> @@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct
>> cso_context *cso)
>> {
>>static const char *vertex_shader_text = {
>> "VERT\n"
>> - "DCL IN[0..1]\n"
>> + "DCL IN[0]\n"
>> + "DCL IN[1]\n"
>> "DCL OUT[0], POSITION\n"
>> "DCL OUT[1], COLOR[0]\n" /* color */
>> "DCL OUT[2], GENERIC[0]\n" /* texcoord */
>> --
>> 2.4.3
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mail
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
On Sat, Jun 27, 2015 at 11:39 AM, Rob Clark wrote: > On Sat, Jun 27, 2015 at 12:57 PM, Connor Abbott wrote: >> On Sat, Jun 27, 2015 at 6:48 AM, Rob Clark wrote: >>> I had a quick look at enabling this for freedreno.. although looks >>> like it is going to take some work in tgsi_to_nir and/or nir.. the >>> problem is we end up w/ a register array (rather than variable array >>> like we do for TEMP arrays).. ie. for something copying from a TEMP >>> array to an OUT array, we end up like: >>> >>> vec4 ssa_35 = intrinsic load_var () (arr_2[0]) () >>> vec4 ssa_36 = fmov ssa_35 >>> r1[0] = fmov ssa_36 >>> vec4 ssa_37 = intrinsic load_var () (arr_2[1]) () >>> vec4 ssa_38 = fmov ssa_37 >>> r1[1] = fmov ssa_38 >>> vec4 ssa_39 = intrinsic load_var () (arr_2[2]) () >>> vec4 ssa_40 = fmov ssa_39 >>> r1[2] = fmov ssa_40 >>> vec4 ssa_41 = intrinsic load_var () (arr_2[3]) () >>> vec4 ssa_42 = fmov ssa_41 >>> r1[3] = fmov ssa_42 >>> vec4 ssa_43 = fmov r2 >>> r0 = fmov ssa_43 >>> intrinsic store_output (r0) () (0) >>> intrinsic store_output (r1[0]) () (1) >>> intrinsic store_output (r1[1]) () (2) >>> intrinsic store_output (r1[2]) () (3) >>> intrinsic store_output (r1[3]) () (4) >>> >>> the r1[] array ends up not getting converted into SSA and hitting an >>> assert when we try to lower to scalar.. I'm not really sure the best >>> way to handle this >> >> Well, the frontend shouldn't really be generating register arrays... >> they're for backends, not frontends, and it's curently impossible to >> eliminate them without extra information about the stride that I've >> been hesitant to add, and even then they're more of a pain for >> optimization passes. I'm not sure the exact place where ttn is >> emitting a register array, but if you fix it to use a variable >> instead, vars_to_ssa should clean it up. > > I was thinking about this, since I'd already done the same for TEMP > file arrays.. > > How should that end up looking, though, from the perspective of > store_output? Can store_output reference a variable, or do I have to > do a move into ssa and then pass the ssa src to store_output > (effectively duplicating the lowering IN/OUT arrays that tgsi > currently does in ttn) You can create an output variable (i.e. one with its data.mode set to nir_var_shader_out and stored in the outputs array), and nir_lower_io() will then take care of lowering stores to that variable (i.e. store_var intrinsics with the variable set to it) to store_output intrinsics. Note that if you have an indirect output write (i.e. a store_var intrinsic with an indirect array deref), you'll start getting store_output_indirect intrinsics, which you can't get from GLSL since the varying packer lowers away the only possible way a shader can write to an output indirectly. I'm not sure if you care about that, though, since this sounds like it's only for some internal TGSI shaders that Gallium hands you that don't have indirect accesses. In fact, this is the way both inputs and outputs work in glsl -> nir (i.e. first create a variable and then lower it)... it's just that when Eric wrote ttn, he sidestepped that part. It sounds like in this situation, though, it's easier to create a variable for each output array. > > BR, > -R > > >>> >>> BR, >>> -R >>> >>> On Wed, Jun 24, 2015 at 12:31 PM, Marek Olšák wrote: Yes, ArrayID != 0 means it's an array, otherwise it's just a compact way to add declarations. I recently added the array support for inputs and outputs, we just need to enable it on non-radeon non-swrast drivers: http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6ebe7eabf54936a02acc0968e718e0c264a73f5 It would be nice to enable the arrays on all drivers instead of working around it indefinitely. Marek On Wed, Jun 24, 2015 at 1:31 PM, Rob Clark wrote: > Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it > is safe to assume that we always get ArrayID that makes it much > easier. > > BR, > -R > > On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote: >> It's not an array, because the ArrayID is 0. It's a valid non-array >> declaration. If any TGSI user doesn't understand it, that user should >> be fixed. >> >> Marek >> >> On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote: >>> From: Rob Clark >>> >>> Ok, so actually there is a ttn issue here to fix as well.. but it >>> brought up a question in my mind. When ttn sees something like >>> >>> DCL IN[0..1] >>> >>> it will treat that as an array (which in the end will result in >>> constraints about where the registers get allocated. Which is not >>> really ideal. >>> >>> With glsl we don't actually get input arrays (but instead a bunch >>> of MOV's to a TEMP array) currently. So I'm not quite sure how >>> an actual input array shou
[Mesa-dev] [PATCH] gallium/ttn: mark location specially in nir for color0-writes-all
We need to distinguish a shader that has separate writes to each MRT
from one which is supposed to write the data from MRT 0 to all the MRTs.
In TGSI this is done with a property. NIR doesn't have that, so encode
it as a funny location and decode on the other end.
Signed-off-by: Ilia Mirkin
---
This fixes bin/fbo-drawbuffers-none gl_FragColor when I additionally
initialize the "default" colors to a register other than r0.x -- as is
this happens to work by luck.
Also fix up vc4 to ignore this for now.
src/gallium/auxiliary/nir/tgsi_to_nir.c | 7 ++-
src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c | 4
src/gallium/drivers/vc4/vc4_program.c| 6 ++
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
b/src/gallium/auxiliary/nir/tgsi_to_nir.c
index bf7eb2f..4130697 100644
--- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
+++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
@@ -253,7 +253,12 @@ ttn_emit_declaration(struct ttn_compile *c)
var->name = ralloc_asprintf(var, "out_%d", idx);
var->data.location = decl->Semantic.Name;
-var->data.index = decl->Semantic.Index;
+if (decl->Semantic.Name == TGSI_SEMANTIC_COLOR &&
+decl->Semantic.Index == 0 &&
+c->scan->properties[TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS])
+ var->data.index = -1;
+else
+ var->data.index = decl->Semantic.Index;
if (is_array) {
unsigned j;
diff --git a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
index 3b36114..fa13c40 100644
--- a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
+++ b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
@@ -2110,6 +2110,10 @@ setup_output(struct ir3_compile *ctx, nir_variable *out)
so->writes_pos = true;
break;
case TGSI_SEMANTIC_COLOR:
+ if (semantic_index == -1) {
+ semantic_index = 0;
+ so->color0_mrt = 1;
+ }
break;
default:
compile_error(ctx, "unknown FS semantic name: %s\n",
diff --git a/src/gallium/drivers/vc4/vc4_program.c
b/src/gallium/drivers/vc4/vc4_program.c
index 2061631..728ecc6 100644
--- a/src/gallium/drivers/vc4/vc4_program.c
+++ b/src/gallium/drivers/vc4/vc4_program.c
@@ -1783,6 +1783,12 @@ ntq_setup_outputs(struct vc4_compile *c)
assert(array_len == 1);
+/* NIR hack to pass through
+ * TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS */
+if (semantic_name == TGSI_SEMANTIC_COLOR &&
+semantic_index == -1)
+semantic_index = 0;
+
for (int i = 0; i < 4; i++) {
add_output(c,
loc + i,
--
2.3.6
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 91130] Mesa's cl.h defines CL_VERSION_1_2 even though it is missing some OpenCL 1.2 functions
https://bugs.freedesktop.org/show_bug.cgi?id=91130 Bug ID: 91130 Summary: Mesa's cl.h defines CL_VERSION_1_2 even though it is missing some OpenCL 1.2 functions Product: Mesa Version: 10.5 Hardware: Other OS: All Status: NEW Severity: normal Priority: medium Component: Other Assignee: mesa-dev@lists.freedesktop.org Reporter: devuran...@gmx.net QA Contact: mesa-dev@lists.freedesktop.org `clEnqueueFillBuffer` is part of OpenCL 1.2 [1] [2]. Mesa claims to support OpenCL 1.2: ``` # grep -B1 CL_VERSION /usr/include/CL/cl.h /* OpenCL Version */ #define CL_VERSION_1_0 1 #define CL_VERSION_1_1 1 #define CL_VERSION_1_2 1 ``` It declares `clEnqueueFillBuffer`: ``` # grep -B1 -A8 clEnqueueFillBuffer /usr/include/CL/cl.h extern CL_API_ENTRY cl_int CL_API_CALL clEnqueueFillBuffer(cl_command_queue /* command_queue */, cl_mem /* buffer */, const void * /* pattern */, size_t /* pattern_size */, size_t /* offset */, size_t /* size */, cl_uint/* num_events_in_wait_list */, const cl_event * /* event_wait_list */, cl_event * /* event */) CL_API_SUFFIX__VERSION_1_2; ``` But does not define it: ``` # scanelf -s clEnqueueFillBuffer /usr/lib/libOpenCL.so TYPE SYM FILE ET_DYN - /usr/lib/libOpenCL.so ``` Logically, it also seems to be missing in the API dispatcher [1]. I assume Mesa should not claim to support OpenCL 1.2 via `CL_VERSION_1_2`, if at least one of the OpenCL 1.2 functions is not implemented. [1]: https://www.khronos.org/registry/cl/sdk/1.2/docs/man/xhtml/clEnqueueFillBuffer.html [2]: https://www.khronos.org/registry/cl/specs/opencl-1.2.pdf#page=85 [3]: http://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/state_trackers/clover/api/dispatch.cpp#n129 -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 91130] Mesa's cl.h defines CL_VERSION_1_2 even though it is missing some OpenCL 1.2 functions
https://bugs.freedesktop.org/show_bug.cgi?id=91130 Dennis Schridde changed: What|Removed |Added See Also||https://github.com/magnumri ||pper/JohnTheRipper/issues/1 ||470 -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 91130] Mesa's cl.h defines CL_VERSION_1_2 even though it is missing some OpenCL 1.2 functions
https://bugs.freedesktop.org/show_bug.cgi?id=91130 --- Comment #1 from Dennis Schridde --- This issue turned up when trying to fix a linker error in JTR: https://github.com/magnumripper/JohnTheRipper/issues/1470 -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallium/ttn: mark location specially in nir for color0-writes-all
Connor, btw, how does glsl_to_nir handle this?
I guess at some point we want to try to align ttn w/ gtn (ie. cleanup
the whole nir with a tgsi accent thing)..
not that this would be the worst of the ttn vs gtn diff's in the
current state, so it is fine if there isn't a better way to do it
right now.. but I would like to start moving towards using nir
#define's for semantic names/indexes, etc, rather than tgsi or glsl
specific ones.. in the end the driver shouldn't have to care if the
nir came from tgsi/glsl/spirv/etc
BR,
-R
On Sat, Jun 27, 2015 at 5:38 PM, Ilia Mirkin wrote:
> We need to distinguish a shader that has separate writes to each MRT
> from one which is supposed to write the data from MRT 0 to all the MRTs.
> In TGSI this is done with a property. NIR doesn't have that, so encode
> it as a funny location and decode on the other end.
>
> Signed-off-by: Ilia Mirkin
> ---
>
> This fixes bin/fbo-drawbuffers-none gl_FragColor when I additionally
> initialize the "default" colors to a register other than r0.x -- as is
> this happens to work by luck.
>
> Also fix up vc4 to ignore this for now.
>
> src/gallium/auxiliary/nir/tgsi_to_nir.c | 7 ++-
> src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c | 4
> src/gallium/drivers/vc4/vc4_program.c| 6 ++
> 3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
> b/src/gallium/auxiliary/nir/tgsi_to_nir.c
> index bf7eb2f..4130697 100644
> --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
> +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
> @@ -253,7 +253,12 @@ ttn_emit_declaration(struct ttn_compile *c)
> var->name = ralloc_asprintf(var, "out_%d", idx);
>
> var->data.location = decl->Semantic.Name;
> -var->data.index = decl->Semantic.Index;
> +if (decl->Semantic.Name == TGSI_SEMANTIC_COLOR &&
> +decl->Semantic.Index == 0 &&
> +
> c->scan->properties[TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS])
> + var->data.index = -1;
> +else
> + var->data.index = decl->Semantic.Index;
>
> if (is_array) {
> unsigned j;
> diff --git a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
> b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
> index 3b36114..fa13c40 100644
> --- a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
> +++ b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
> @@ -2110,6 +2110,10 @@ setup_output(struct ir3_compile *ctx, nir_variable
> *out)
> so->writes_pos = true;
> break;
> case TGSI_SEMANTIC_COLOR:
> + if (semantic_index == -1) {
> + semantic_index = 0;
> + so->color0_mrt = 1;
> + }
> break;
> default:
> compile_error(ctx, "unknown FS semantic name: %s\n",
> diff --git a/src/gallium/drivers/vc4/vc4_program.c
> b/src/gallium/drivers/vc4/vc4_program.c
> index 2061631..728ecc6 100644
> --- a/src/gallium/drivers/vc4/vc4_program.c
> +++ b/src/gallium/drivers/vc4/vc4_program.c
> @@ -1783,6 +1783,12 @@ ntq_setup_outputs(struct vc4_compile *c)
>
> assert(array_len == 1);
>
> +/* NIR hack to pass through
> + * TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS */
> +if (semantic_name == TGSI_SEMANTIC_COLOR &&
> +semantic_index == -1)
> +semantic_index = 0;
> +
> for (int i = 0; i < 4; i++) {
> add_output(c,
> loc + i,
> --
> 2.3.6
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallium/ttn: mark location specially in nir for color0-writes-all
On Sat, Jun 27, 2015 at 2:54 PM, Rob Clark wrote:
> Connor, btw, how does glsl_to_nir handle this?
The variable for gl_FragColor a special location (FRAG_RESULT_COLOR),
whereas normal outputs (i.e. gl_FragData[] or out variables) start at
FRAG_RESULT_DATA0.
>
> I guess at some point we want to try to align ttn w/ gtn (ie. cleanup
> the whole nir with a tgsi accent thing)..
>
> not that this would be the worst of the ttn vs gtn diff's in the
> current state, so it is fine if there isn't a better way to do it
> right now.. but I would like to start moving towards using nir
> #define's for semantic names/indexes, etc, rather than tgsi or glsl
> specific ones.. in the end the driver shouldn't have to care if the
> nir came from tgsi/glsl/spirv/etc
Indeed, it would be nice to have NIR do it's own, well-documented
thing that's consistent between all the frontends... I've been looking
at SPIR-V inputs/outputs, and it has its own way of doing things
that's a little more flexible than GLSL in some ways (e.g. right now
you can have arbitrary structs of structs and one of the members can
be marked as flat or noperspective), although it's not really clear
how much of this is intentional and how much is because it wasn't
specified or accidentally allowed. The plan has been to map that onto
how the existing Mesa infrastructure does things, although the current
underspecification of SPIR-V combined with the underspecification of
how it all works in Mesa has been a bit frustrating. I'm not really
familiar with how it works in Gallium at all, though, so I'm not sure
I'd be the one for the job, or if there even is a person really
qualified for it.
>
> BR,
> -R
>
>
> On Sat, Jun 27, 2015 at 5:38 PM, Ilia Mirkin wrote:
>> We need to distinguish a shader that has separate writes to each MRT
>> from one which is supposed to write the data from MRT 0 to all the MRTs.
>> In TGSI this is done with a property. NIR doesn't have that, so encode
>> it as a funny location and decode on the other end.
>>
>> Signed-off-by: Ilia Mirkin
>> ---
>>
>> This fixes bin/fbo-drawbuffers-none gl_FragColor when I additionally
>> initialize the "default" colors to a register other than r0.x -- as is
>> this happens to work by luck.
>>
>> Also fix up vc4 to ignore this for now.
>>
>> src/gallium/auxiliary/nir/tgsi_to_nir.c | 7 ++-
>> src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c | 4
>> src/gallium/drivers/vc4/vc4_program.c| 6 ++
>> 3 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
>> b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>> index bf7eb2f..4130697 100644
>> --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
>> +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>> @@ -253,7 +253,12 @@ ttn_emit_declaration(struct ttn_compile *c)
>> var->name = ralloc_asprintf(var, "out_%d", idx);
>>
>> var->data.location = decl->Semantic.Name;
>> -var->data.index = decl->Semantic.Index;
>> +if (decl->Semantic.Name == TGSI_SEMANTIC_COLOR &&
>> +decl->Semantic.Index == 0 &&
>> +
>> c->scan->properties[TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS])
>> + var->data.index = -1;
>> +else
>> + var->data.index = decl->Semantic.Index;
>>
>> if (is_array) {
>> unsigned j;
>> diff --git a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
>> b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
>> index 3b36114..fa13c40 100644
>> --- a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
>> +++ b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
>> @@ -2110,6 +2110,10 @@ setup_output(struct ir3_compile *ctx, nir_variable
>> *out)
>> so->writes_pos = true;
>> break;
>> case TGSI_SEMANTIC_COLOR:
>> + if (semantic_index == -1) {
>> + semantic_index = 0;
>> + so->color0_mrt = 1;
>> + }
>> break;
>> default:
>> compile_error(ctx, "unknown FS semantic name: %s\n",
>> diff --git a/src/gallium/drivers/vc4/vc4_program.c
>> b/src/gallium/drivers/vc4/vc4_program.c
>> index 2061631..728ecc6 100644
>> --- a/src/gallium/drivers/vc4/vc4_program.c
>> +++ b/src/gallium/drivers/vc4/vc4_program.c
>> @@ -1783,6 +1783,12 @@ ntq_setup_outputs(struct vc4_compile *c)
>>
>> assert(array_len == 1);
>>
>> +/* NIR hack to pass through
>> + * TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS */
>> +if (semantic_name == TGSI_SEMANTIC_COLOR &&
>> +semantic_index == -1)
>> +semantic_index = 0;
>> +
>> for (int i = 0; i < 4; i++) {
>> add_output(c,
>>
Re: [Mesa-dev] [PATCH] i965: Switch on shader stage in nir_setup_outputs().
On 2015-06-26 16:03:21, Kenneth Graunke wrote:
> Adding new shader stages to a switch statement is less confusing than an
> if-else-if ladder where all but the first case are fragment shader
> specific (but don't claim to be).
>
> Signed-off-by: Kenneth Graunke
> ---
> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 59
> +-
> 1 file changed, 33 insertions(+), 26 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 59081ea..8bcd5e2 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -133,38 +133,45 @@ fs_visitor::nir_setup_outputs(nir_shader *shader)
> var->type->is_array() ? var->type->fields.array->vector_elements
> : var->type->vector_elements;
>
> - if (stage == MESA_SHADER_VERTEX) {
> + switch (stage) {
> + case MESA_SHADER_VERTEX:
> for (int i = 0; i < ALIGN(type_size(var->type), 4) / 4; i++) {
> int output = var->data.location + i;
> this->outputs[output] = offset(reg, 4 * i);
> this->output_components[output] = vector_elements;
> }
> - } else if (var->data.index > 0) {
> - assert(var->data.location == FRAG_RESULT_DATA0);
> - assert(var->data.index == 1);
> - this->dual_src_output = reg;
> - this->do_dual_src = true;
> - } else if (var->data.location == FRAG_RESULT_COLOR) {
> - /* Writing gl_FragColor outputs to all color regions. */
> - for (unsigned int i = 0; i < MAX2(key->nr_color_regions, 1); i++) {
> -this->outputs[i] = reg;
> -this->output_components[i] = 4;
> - }
> - } else if (var->data.location == FRAG_RESULT_DEPTH) {
> - this->frag_depth = reg;
> - } else if (var->data.location == FRAG_RESULT_SAMPLE_MASK) {
> - this->sample_mask = reg;
> - } else {
> - /* gl_FragData or a user-defined FS output */
> - assert(var->data.location >= FRAG_RESULT_DATA0 &&
> -var->data.location < FRAG_RESULT_DATA0 +
> BRW_MAX_DRAW_BUFFERS);
> -
> - /* General color output. */
> - for (unsigned int i = 0; i < MAX2(1, var->type->length); i++) {
> -int output = var->data.location - FRAG_RESULT_DATA0 + i;
> -this->outputs[output] = offset(reg, vector_elements * i);
> -this->output_components[output] = vector_elements;
> + break;
> + case MESA_SHADER_FRAGMENT:
> + if (var->data.index > 0) {
> +assert(var->data.location == FRAG_RESULT_DATA0);
> +assert(var->data.index == 1);
> +this->dual_src_output = reg;
> +this->do_dual_src = true;
> + } else if (var->data.location == FRAG_RESULT_COLOR) {
> +/* Writing gl_FragColor outputs to all color regions. */
> +for (unsigned int i = 0; i < MAX2(key->nr_color_regions, 1);
> i++) {
> + this->outputs[i] = reg;
> + this->output_components[i] = 4;
> +}
> + } else if (var->data.location == FRAG_RESULT_DEPTH) {
> +this->frag_depth = reg;
> + } else if (var->data.location == FRAG_RESULT_SAMPLE_MASK) {
> +this->sample_mask = reg;
> + } else {
> +/* gl_FragData or a user-defined FS output */
> +assert(var->data.location >= FRAG_RESULT_DATA0 &&
> + var->data.location <
> FRAG_RESULT_DATA0+BRW_MAX_DRAW_BUFFERS);
> +
> +/* General color output. */
> +for (unsigned int i = 0; i < MAX2(1, var->type->length); i++) {
> + int output = var->data.location - FRAG_RESULT_DATA0 + i;
> + this->outputs[output] = offset(reg, vector_elements * i);
> + this->output_components[output] = vector_elements;
> +}
I noticed that it looks like MESA_SHADER_FRAGMENT case could use a
switch as well on var->data.location. Does it help, or is it better to
keep the if-ladder there?
switch (var->data.location) {
case FRAG_RESULT_COLOR;
/* do stuff */
break;
case FRAG_RESULT_DEPTH:
/* do stuff */
break;
case FRAG_RESULT_SAMPLE_MASK:
/* do stuff */
break;
case FRAG_RESULT_DATA0:
if (var->data.index > 0) {
/* do stuff */
break;
}
/* fall-through */
default:
assert(var->data.index == 0);
/* do stuff */
break;
}
-Jordan
> }
> + break;
> + default:
> + unreachable("unhandled shader stage");
>}
> }
> }
> --
> 1.7.10.4
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists
Re: [Mesa-dev] [PATCH] i965: Switch on shader stage in nir_setup_outputs().
On Saturday, June 27, 2015 05:00:22 PM Jordan Justen wrote:
> On 2015-06-26 16:03:21, Kenneth Graunke wrote:
> > Adding new shader stages to a switch statement is less confusing than an
> > if-else-if ladder where all but the first case are fragment shader
> > specific (but don't claim to be).
> >
> > Signed-off-by: Kenneth Graunke
> > ---
> > src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 59
> > +-
> > 1 file changed, 33 insertions(+), 26 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > index 59081ea..8bcd5e2 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > @@ -133,38 +133,45 @@ fs_visitor::nir_setup_outputs(nir_shader *shader)
> > var->type->is_array() ? var->type->fields.array->vector_elements
> > : var->type->vector_elements;
> >
> > - if (stage == MESA_SHADER_VERTEX) {
> > + switch (stage) {
> > + case MESA_SHADER_VERTEX:
> > for (int i = 0; i < ALIGN(type_size(var->type), 4) / 4; i++) {
> > int output = var->data.location + i;
> > this->outputs[output] = offset(reg, 4 * i);
> > this->output_components[output] = vector_elements;
> > }
> > - } else if (var->data.index > 0) {
> > - assert(var->data.location == FRAG_RESULT_DATA0);
> > - assert(var->data.index == 1);
> > - this->dual_src_output = reg;
> > - this->do_dual_src = true;
> > - } else if (var->data.location == FRAG_RESULT_COLOR) {
> > - /* Writing gl_FragColor outputs to all color regions. */
> > - for (unsigned int i = 0; i < MAX2(key->nr_color_regions, 1); i++)
> > {
> > -this->outputs[i] = reg;
> > -this->output_components[i] = 4;
> > - }
> > - } else if (var->data.location == FRAG_RESULT_DEPTH) {
> > - this->frag_depth = reg;
> > - } else if (var->data.location == FRAG_RESULT_SAMPLE_MASK) {
> > - this->sample_mask = reg;
> > - } else {
> > - /* gl_FragData or a user-defined FS output */
> > - assert(var->data.location >= FRAG_RESULT_DATA0 &&
> > -var->data.location < FRAG_RESULT_DATA0 +
> > BRW_MAX_DRAW_BUFFERS);
> > -
> > - /* General color output. */
> > - for (unsigned int i = 0; i < MAX2(1, var->type->length); i++) {
> > -int output = var->data.location - FRAG_RESULT_DATA0 + i;
> > -this->outputs[output] = offset(reg, vector_elements * i);
> > -this->output_components[output] = vector_elements;
> > + break;
> > + case MESA_SHADER_FRAGMENT:
> > + if (var->data.index > 0) {
> > +assert(var->data.location == FRAG_RESULT_DATA0);
> > +assert(var->data.index == 1);
> > +this->dual_src_output = reg;
> > +this->do_dual_src = true;
> > + } else if (var->data.location == FRAG_RESULT_COLOR) {
> > +/* Writing gl_FragColor outputs to all color regions. */
> > +for (unsigned int i = 0; i < MAX2(key->nr_color_regions, 1);
> > i++) {
> > + this->outputs[i] = reg;
> > + this->output_components[i] = 4;
> > +}
> > + } else if (var->data.location == FRAG_RESULT_DEPTH) {
> > +this->frag_depth = reg;
> > + } else if (var->data.location == FRAG_RESULT_SAMPLE_MASK) {
> > +this->sample_mask = reg;
> > + } else {
> > +/* gl_FragData or a user-defined FS output */
> > +assert(var->data.location >= FRAG_RESULT_DATA0 &&
> > + var->data.location <
> > FRAG_RESULT_DATA0+BRW_MAX_DRAW_BUFFERS);
> > +
> > +/* General color output. */
> > +for (unsigned int i = 0; i < MAX2(1, var->type->length); i++) {
> > + int output = var->data.location - FRAG_RESULT_DATA0 + i;
> > + this->outputs[output] = offset(reg, vector_elements * i);
> > + this->output_components[output] = vector_elements;
> > +}
>
> I noticed that it looks like MESA_SHADER_FRAGMENT case could use a
> switch as well on var->data.location. Does it help, or is it better to
> keep the if-ladder there?
>
> switch (var->data.location) {
>case FRAG_RESULT_COLOR;
> /* do stuff */
> break;
>case FRAG_RESULT_DEPTH:
> /* do stuff */
> break;
>case FRAG_RESULT_SAMPLE_MASK:
> /* do stuff */
> break;
>case FRAG_RESULT_DATA0:
> if (var->data.index > 0) {
> /* do stuff */
> break;
> }
> /* fall-through */
>default:
> assert(var->data.index == 0);
> /* do stuff */
> break;
> }
>
> -Jordan
*shrug*. It doesn't really affect me, and I don't have a strong
preference one way or another. I mostly wanted there to be a sensibl
Re: [Mesa-dev] [PATCH] gallium/ttn: mark location specially in nir for color0-writes-all
On Sat, Jun 27, 2015 at 7:47 PM, Connor Abbott wrote:
> On Sat, Jun 27, 2015 at 2:54 PM, Rob Clark wrote:
>> Connor, btw, how does glsl_to_nir handle this?
>
> The variable for gl_FragColor a special location (FRAG_RESULT_COLOR),
> whereas normal outputs (i.e. gl_FragData[] or out variables) start at
> FRAG_RESULT_DATA0.
>
>>
>> I guess at some point we want to try to align ttn w/ gtn (ie. cleanup
>> the whole nir with a tgsi accent thing)..
>>
>> not that this would be the worst of the ttn vs gtn diff's in the
>> current state, so it is fine if there isn't a better way to do it
>> right now.. but I would like to start moving towards using nir
>> #define's for semantic names/indexes, etc, rather than tgsi or glsl
>> specific ones.. in the end the driver shouldn't have to care if the
>> nir came from tgsi/glsl/spirv/etc
>
> Indeed, it would be nice to have NIR do it's own, well-documented
> thing that's consistent between all the frontends... I've been looking
> at SPIR-V inputs/outputs, and it has its own way of doing things
> that's a little more flexible than GLSL in some ways (e.g. right now
> you can have arbitrary structs of structs and one of the members can
> be marked as flat or noperspective), although it's not really clear
> how much of this is intentional and how much is because it wasn't
> specified or accidentally allowed. The plan has been to map that onto
> how the existing Mesa infrastructure does things, although the current
> underspecification of SPIR-V combined with the underspecification of
> how it all works in Mesa has been a bit frustrating. I'm not really
> familiar with how it works in Gallium at all, though, so I'm not sure
> I'd be the one for the job, or if there even is a person really
> qualified for it.
the one good thing about tgsi is it is pretty well specified, and in a
way that is independent of what sits upstream of tgsi ;-)
I'd be a fan of specifying things in NIR (w/ it's own
enums/#defines/etc) and mapping glsl/spirv/etc onto that.. current
re-use of glsl specific stuff in NIR is a bit of a pain point for
gallium drivers (since gallium drivers can be used for things other
than gl..)
BR,
-R
>>
>> BR,
>> -R
>>
>>
>> On Sat, Jun 27, 2015 at 5:38 PM, Ilia Mirkin wrote:
>>> We need to distinguish a shader that has separate writes to each MRT
>>> from one which is supposed to write the data from MRT 0 to all the MRTs.
>>> In TGSI this is done with a property. NIR doesn't have that, so encode
>>> it as a funny location and decode on the other end.
>>>
>>> Signed-off-by: Ilia Mirkin
>>> ---
>>>
>>> This fixes bin/fbo-drawbuffers-none gl_FragColor when I additionally
>>> initialize the "default" colors to a register other than r0.x -- as is
>>> this happens to work by luck.
>>>
>>> Also fix up vc4 to ignore this for now.
>>>
>>> src/gallium/auxiliary/nir/tgsi_to_nir.c | 7 ++-
>>> src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c | 4
>>> src/gallium/drivers/vc4/vc4_program.c| 6 ++
>>> 3 files changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
>>> b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>>> index bf7eb2f..4130697 100644
>>> --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
>>> +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>>> @@ -253,7 +253,12 @@ ttn_emit_declaration(struct ttn_compile *c)
>>> var->name = ralloc_asprintf(var, "out_%d", idx);
>>>
>>> var->data.location = decl->Semantic.Name;
>>> -var->data.index = decl->Semantic.Index;
>>> +if (decl->Semantic.Name == TGSI_SEMANTIC_COLOR &&
>>> +decl->Semantic.Index == 0 &&
>>> +
>>> c->scan->properties[TGSI_PROPERTY_FS_COLOR0_WRITES_ALL_CBUFS])
>>> + var->data.index = -1;
>>> +else
>>> + var->data.index = decl->Semantic.Index;
>>>
>>> if (is_array) {
>>> unsigned j;
>>> diff --git a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
>>> b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
>>> index 3b36114..fa13c40 100644
>>> --- a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
>>> +++ b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c
>>> @@ -2110,6 +2110,10 @@ setup_output(struct ir3_compile *ctx, nir_variable
>>> *out)
>>> so->writes_pos = true;
>>> break;
>>> case TGSI_SEMANTIC_COLOR:
>>> + if (semantic_index == -1) {
>>> + semantic_index = 0;
>>> + so->color0_mrt = 1;
>>> + }
>>> break;
>>> default:
>>> compile_error(ctx, "unknown FS semantic name: %s\n",
>>> diff --git a/src/gallium/drivers/vc4/vc4_program.c
>>> b/src/gallium/drivers/vc4/vc4_program.c
>>> index 2061631..728ecc6 100644
>>> --- a/src/gallium/drivers/vc4/vc4_pr
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
On 06/05/2015 03:57 PM, Mario Kleiner wrote: On 06/05/2015 03:50 PM, Ilia Mirkin wrote: This scheme is copied from radeon, does it need a similar fix? I'm away from computers for another week or so, will be able to look then. For some reason, no. Testing on Radeon multi-x-screen ZaphodHeads never showed such problems over multiple years and many mesa versions. Apparently it does something slightly different, although i saw the hashing logic with fstat() is the same. Maybe correct by slightly different design, or maybe we just get lucky because the same fd's aren't recycled on successive glXQueryVersion() calls? thanks, -mario And to contradict myself, yes, it needs a similar fix for radeon, as both code inspection and testing showed. Not sure why this (seemed?) to work without problems for me for a long time, but at least on Mesa 10.5+ it crashes nicely in the expected way. I'll send out a patch in the next minute which fixes the problem, as tested on a triple-display Radeon HD-5770 setup with different single x-screen and multi-x-screen ZaphodHeads configs. thanks, -mario On Jun 5, 2015 4:37 PM, "Mario Kleiner" mailto:mario.kleiner...@gmail.com>> wrote: The dup'ed fd owned by the nouveau_screen for a device node must also be used as key for the winsys hash table, instead of using the original fd passed in for a screen, to make multi-x-screen ZaphodHeads configurations work on nouveau. This prevents the following crash scenario that was observed when a dynamically loaded rendering plugin used OpenGL on a ZaphodHeads setup, e.g., on a dual x-screen setup. At first load the plugin worked, but after unloading and reloading it, the next rendering operation crashed: 1. Client, e.g., a plugin, calls glXQueryVersion. 2. DRI screens for the x-screens 0 and 1 are created, one shared nouveau_screen is created for the shared device node of both screens, but the original fd of x-screen 0 is used as identifying key in the hash table, instead of the dup()ed fd of x-screen 0 which is owned by the nouveau_screen. nouveau_screen's refcount is now 2. 3. Regular rendering happens by the client plugin, then the plugin gets unloaded. 4. XCloseDisplay(). x-screen 0 gets its DRI screen destroyed, nouveau_drm_screen_unref() drops the refcount to 1, calling mesa code then closes the fd of x-screen 0, so now the fd which is used as key in the hash table is invalid. x-screen 1 gets destroyed, nouveau_drm_screen_unref() drops the refcount to 0, the nouveau_screen gets destroyed, but removal of its entry in the hash table fails, because the invalid fd in the hash table no longer matches anything (fstat() on the fd is used for hashing and key comparison, but fstat() on an already closed fd fails and returns bogus results). x-screen 1 closes its fd. Now all fd's are closed, the nouveau_screen destroyed, but there is a dangling reference to the nouveau_screen in the hash table. 5. Some OpenGL client plugin gets loaded again and calls glXQueryVersion. Step 2 above repeats, but because a dangling reference with a matching fd is found in the winsys hash table, no new nouveau_screen is created this time. Instead the invalid pointer to the old nouveau_screen is recycled, which points to nirvana -> Crash. This problem is avoided by use of the dup()ed fd which is owned by the nouveau_screen and has the same lifetime as the nouveau_screen itself. Cc: "10.3 10.4 10.5 10.6" mailto:mesa-sta...@lists.freedesktop.org>> Signed-off-by: Mario Kleiner mailto:mario.kleiner...@gmail.com>> Cc: Ilia Mirkin mailto:imir...@alum.mit.edu>> --- src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c index 0635246..dbc3cae 100644 --- a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c +++ b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c @@ -120,7 +120,8 @@ nouveau_drm_screen_create(int fd) if (!screen) goto err; - util_hash_table_set(fd_tab, intptr_to_pointer(fd), screen); + /* Use dupfd in hash table, to avoid crashes in ZaphodHeads configs */ + util_hash_table_set(fd_tab, intptr_to_pointer(dupfd), screen); screen->refcount = 1; pipe_mutex_unlock(nouveau_screen_mutex); return &screen->base; -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] radeon: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
Same problem and fix as for nouveau's ZaphodHeads trouble.
See patch ...
"nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads."
... for reference.
Cc: "10.3 10.4 10.5 10.6"
Signed-off-by: Mario Kleiner
Cc: Ilia Mirkin
---
src/gallium/winsys/radeon/drm/radeon_drm_winsys.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
index ba8d143..ebdb19e 100644
--- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
+++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
@@ -484,6 +484,10 @@ static void radeon_winsys_destroy(struct radeon_winsys
*rws)
if (ws->gen >= DRV_R600) {
radeon_surface_manager_free(ws->surf_man);
}
+
+if (ws->fd)
+close(ws->fd);
+
FREE(rws);
}
@@ -696,7 +700,7 @@ radeon_drm_winsys_create(int fd, radeon_screen_create_t
screen_create)
return NULL;
}
-ws->fd = fd;
+ws->fd = dup(fd);
if (!do_winsys_init(ws))
goto fail;
@@ -712,7 +716,7 @@ radeon_drm_winsys_create(int fd, radeon_screen_create_t
screen_create)
goto fail;
if (ws->gen >= DRV_R600) {
-ws->surf_man = radeon_surface_manager_new(fd);
+ws->surf_man = radeon_surface_manager_new(ws->fd);
if (!ws->surf_man)
goto fail;
}
@@ -753,7 +757,7 @@ radeon_drm_winsys_create(int fd, radeon_screen_create_t
screen_create)
return NULL;
}
-util_hash_table_set(fd_tab, intptr_to_pointer(fd), ws);
+util_hash_table_set(fd_tab, intptr_to_pointer(ws->fd), ws);
/* We must unlock the mutex once the winsys is fully initialized, so that
* other threads attempting to create the winsys from the same fd will
@@ -770,6 +774,9 @@ fail:
ws->kman->destroy(ws->kman);
if (ws->surf_man)
radeon_surface_manager_free(ws->surf_man);
+if (ws->fd)
+close(ws->fd);
+
FREE(ws);
return NULL;
}
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner
wrote:
> The dup'ed fd owned by the nouveau_screen for a device node
> must also be used as key for the winsys hash table, instead
> of using the original fd passed in for a screen, to make
> multi-x-screen ZaphodHeads configurations work on nouveau.
>
> This prevents the following crash scenario that was observed
> when a dynamically loaded rendering plugin used OpenGL on a
> ZaphodHeads setup, e.g., on a dual x-screen setup. At first
> load the plugin worked, but after unloading and reloading it,
> the next rendering operation crashed:
>
> 1. Client, e.g., a plugin, calls glXQueryVersion.
>
> 2. DRI screens for the x-screens 0 and 1 are created, one shared
>nouveau_screen is created for the shared device node of both
>screens, but the original fd of x-screen 0 is used as identifying
>key in the hash table, instead of the dup()ed fd of x-screen 0
>which is owned by the nouveau_screen. nouveau_screen's refcount
>is now 2.
See below, but it shouldn't matter which fd gets used.
>
> 3. Regular rendering happens by the client plugin, then the plugin
>gets unloaded.
>
> 4. XCloseDisplay(). x-screen 0 gets its DRI screen destroyed,
>nouveau_drm_screen_unref() drops the refcount to 1, calling mesa
>code then closes the fd of x-screen 0, so now the fd which is
>used as key in the hash table is invalid. x-screen 1 gets
>destroyed, nouveau_drm_screen_unref() drops the refcount to 0,
>the nouveau_screen gets destroyed, but removal of its entry
>in the hash table fails, because the invalid fd in the hash
>table no longer matches anything (fstat() on the fd is used
>for hashing and key comparison, but fstat() on an already closed
>fd fails and returns bogus results). x-screen 1 closes its fd.
>
>Now all fd's are closed, the nouveau_screen destroyed, but
>there is a dangling reference to the nouveau_screen in the
>hash table.
>
> 5. Some OpenGL client plugin gets loaded again and calls
>glXQueryVersion. Step 2 above repeats, but because a
>dangling reference with a matching fd is found in the winsys
>hash table, no new nouveau_screen is created this time. Instead
>the invalid pointer to the old nouveau_screen is recycled,
>which points to nirvana -> Crash.
>
> This problem is avoided by use of the dup()ed fd which is
> owned by the nouveau_screen and has the same lifetime as
> the nouveau_screen itself.
I need to think about this some more, but... this shouldn't happen :)
In fact, the whole dupfd thing was added there for ZaphodHeads screens
in the first place. See
https://bugs.freedesktop.org/show_bug.cgi?id=79823 and commit
a59f2bb17bcc which fixed it.
Note that the hash has the following hash/eq functions:
static unsigned hash_fd(void *key)
{
int fd = pointer_to_intptr(key);
struct stat stat;
fstat(fd, &stat);
return stat.st_dev ^ stat.st_ino ^ stat.st_rdev;
}
static int compare_fd(void *key1, void *key2)
{
int fd1 = pointer_to_intptr(key1);
int fd2 = pointer_to_intptr(key2);
struct stat stat1, stat2;
fstat(fd1, &stat1);
fstat(fd2, &stat2);
return stat1.st_dev != stat2.st_dev ||
stat1.st_ino != stat2.st_ino ||
stat1.st_rdev != stat2.st_rdev;
}
so fd and dupfd should get hashed to the same thing. I suspect there's
something else going on in your application...
>
> Cc: "10.3 10.4 10.5 10.6"
>
> Signed-off-by: Mario Kleiner
> Cc: Ilia Mirkin
> ---
> src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
> b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
> index 0635246..dbc3cae 100644
> --- a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
> +++ b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
> @@ -120,7 +120,8 @@ nouveau_drm_screen_create(int fd)
> if (!screen)
> goto err;
>
> - util_hash_table_set(fd_tab, intptr_to_pointer(fd), screen);
> + /* Use dupfd in hash table, to avoid crashes in ZaphodHeads configs */
> + util_hash_table_set(fd_tab, intptr_to_pointer(dupfd), screen);
> screen->refcount = 1;
> pipe_mutex_unlock(nouveau_screen_mutex);
> return &screen->base;
> --
> 2.1.4
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
On 06/28/2015 03:48 AM, Ilia Mirkin wrote:
On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner
wrote:
The dup'ed fd owned by the nouveau_screen for a device node
must also be used as key for the winsys hash table, instead
of using the original fd passed in for a screen, to make
multi-x-screen ZaphodHeads configurations work on nouveau.
This prevents the following crash scenario that was observed
when a dynamically loaded rendering plugin used OpenGL on a
ZaphodHeads setup, e.g., on a dual x-screen setup. At first
load the plugin worked, but after unloading and reloading it,
the next rendering operation crashed:
1. Client, e.g., a plugin, calls glXQueryVersion.
2. DRI screens for the x-screens 0 and 1 are created, one shared
nouveau_screen is created for the shared device node of both
screens, but the original fd of x-screen 0 is used as identifying
key in the hash table, instead of the dup()ed fd of x-screen 0
which is owned by the nouveau_screen. nouveau_screen's refcount
is now 2.
See below, but it shouldn't matter which fd gets used.
3. Regular rendering happens by the client plugin, then the plugin
gets unloaded.
4. XCloseDisplay(). x-screen 0 gets its DRI screen destroyed,
nouveau_drm_screen_unref() drops the refcount to 1, calling mesa
code then closes the fd of x-screen 0, so now the fd which is
used as key in the hash table is invalid. x-screen 1 gets
destroyed, nouveau_drm_screen_unref() drops the refcount to 0,
the nouveau_screen gets destroyed, but removal of its entry
in the hash table fails, because the invalid fd in the hash
table no longer matches anything (fstat() on the fd is used
for hashing and key comparison, but fstat() on an already closed
fd fails and returns bogus results). x-screen 1 closes its fd.
Now all fd's are closed, the nouveau_screen destroyed, but
there is a dangling reference to the nouveau_screen in the
hash table.
5. Some OpenGL client plugin gets loaded again and calls
glXQueryVersion. Step 2 above repeats, but because a
dangling reference with a matching fd is found in the winsys
hash table, no new nouveau_screen is created this time. Instead
the invalid pointer to the old nouveau_screen is recycled,
which points to nirvana -> Crash.
This problem is avoided by use of the dup()ed fd which is
owned by the nouveau_screen and has the same lifetime as
the nouveau_screen itself.
I need to think about this some more, but... this shouldn't happen :)
In fact, the whole dupfd thing was added there for ZaphodHeads screens
in the first place. See
https://bugs.freedesktop.org/show_bug.cgi?id=79823 and commit
a59f2bb17bcc which fixed it.
Note that the hash has the following hash/eq functions:
static unsigned hash_fd(void *key)
{
int fd = pointer_to_intptr(key);
struct stat stat;
fstat(fd, &stat);
return stat.st_dev ^ stat.st_ino ^ stat.st_rdev;
}
static int compare_fd(void *key1, void *key2)
{
int fd1 = pointer_to_intptr(key1);
int fd2 = pointer_to_intptr(key2);
struct stat stat1, stat2;
fstat(fd1, &stat1);
fstat(fd2, &stat2);
return stat1.st_dev != stat2.st_dev ||
stat1.st_ino != stat2.st_ino ||
stat1.st_rdev != stat2.st_rdev;
}
so fd and dupfd should get hashed to the same thing. I suspect there's
something else going on in your application...
My application is a set of dynamically loaded plugins running inside
another host app (Psychtoolbox-3 inside GNU/Octave), so what happens
often is that the OpenGL using plugin gets unloaded and reloaded at
runtime, so a after a OpenGL session has ended with a XCloseDisplay()
tearing the winsys down, you can easily have it restart at plugin reload
with another XOpenDisplay -> glXQueryVersion -> sequence, where the
new call to glXQueryVersion will trigger a recreation of the winsys,
which will find the stale entry in the hash table pointing to nowhere
instead of the previously released nouveau_screen -> use-after-free -> boom!
The reason this fails is because during destruction of the 2nd, 3rd etc.
x-screen, the already closed fd associated with the 1st x-screen is fed
into the compare_fd function, so fstat() errors out on the invalid fd. I
added printf's etc. on both nouveau and now radeon to verify the fstat
gives me EBADF errors. So the hash calculation goes wrong when trying to
find the matching element in the hash table with a fd that has a
matching hash -> The element which should be removed is ignored/not
removed because it contains an already closed fd for which no proper
hash can be calculated anymore -> hash comparison during search goes wrong.
This is because multiple x-screens, e.g., 2 x-screens are destroyed in
order 0, 1 by FreeScreenConfigs() as part of XCloseDisplay().
FreeScreenConfigs() calls dri2DestroyScreen() in dri2.c or
dri3_destroy_screen in dri3.c, which are essentially identical, e.g.,:
static void
dri2
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
Oh duh. Thanks for the super-detailed explanation. To rephrase what
you said in a slightly shorter manner:
See bug https://bugs.freedesktop.org/show_bug.cgi?id=79823 for why we
need to dupfd (which we are already, although radeon might not be).
Except instead of sticking the dup'd fd into the hash table, whose
lifetime matches that of the device, we stick the other one in, which
is effectively owned by the X server. As a result, those fstat's might
fail down the line, and all sorts of hell will break loose.
Would you be opposed to me rewriting your commit message to basically
reflect the above? Perhaps your original one did as well, but it
wasn't clear to me. I'd also like to throw some assert's in to make
sure the fstat's don't fail -- does that sound reasonable?
Another solution, btw, is to stick as the real key in the hash, although that'd involve
making pointers. Probably not worth it.
Cheers,
-ilia
On Sat, Jun 27, 2015 at 11:13 PM, Mario Kleiner
wrote:
> On 06/28/2015 03:48 AM, Ilia Mirkin wrote:
>>
>> On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner
>> wrote:
>>>
>>> The dup'ed fd owned by the nouveau_screen for a device node
>>> must also be used as key for the winsys hash table, instead
>>> of using the original fd passed in for a screen, to make
>>> multi-x-screen ZaphodHeads configurations work on nouveau.
>>>
>>> This prevents the following crash scenario that was observed
>>> when a dynamically loaded rendering plugin used OpenGL on a
>>> ZaphodHeads setup, e.g., on a dual x-screen setup. At first
>>> load the plugin worked, but after unloading and reloading it,
>>> the next rendering operation crashed:
>>>
>>> 1. Client, e.g., a plugin, calls glXQueryVersion.
>>>
>>> 2. DRI screens for the x-screens 0 and 1 are created, one shared
>>> nouveau_screen is created for the shared device node of both
>>> screens, but the original fd of x-screen 0 is used as identifying
>>> key in the hash table, instead of the dup()ed fd of x-screen 0
>>> which is owned by the nouveau_screen. nouveau_screen's refcount
>>> is now 2.
>>
>>
>> See below, but it shouldn't matter which fd gets used.
>>
>>>
>>> 3. Regular rendering happens by the client plugin, then the plugin
>>> gets unloaded.
>>>
>>> 4. XCloseDisplay(). x-screen 0 gets its DRI screen destroyed,
>>> nouveau_drm_screen_unref() drops the refcount to 1, calling mesa
>>> code then closes the fd of x-screen 0, so now the fd which is
>>> used as key in the hash table is invalid. x-screen 1 gets
>>> destroyed, nouveau_drm_screen_unref() drops the refcount to 0,
>>> the nouveau_screen gets destroyed, but removal of its entry
>>> in the hash table fails, because the invalid fd in the hash
>>> table no longer matches anything (fstat() on the fd is used
>>> for hashing and key comparison, but fstat() on an already closed
>>> fd fails and returns bogus results). x-screen 1 closes its fd.
>>>
>>> Now all fd's are closed, the nouveau_screen destroyed, but
>>> there is a dangling reference to the nouveau_screen in the
>>> hash table.
>>>
>>> 5. Some OpenGL client plugin gets loaded again and calls
>>> glXQueryVersion. Step 2 above repeats, but because a
>>> dangling reference with a matching fd is found in the winsys
>>> hash table, no new nouveau_screen is created this time. Instead
>>> the invalid pointer to the old nouveau_screen is recycled,
>>> which points to nirvana -> Crash.
>>>
>>> This problem is avoided by use of the dup()ed fd which is
>>> owned by the nouveau_screen and has the same lifetime as
>>> the nouveau_screen itself.
>>
>>
>> I need to think about this some more, but... this shouldn't happen :)
>>
>> In fact, the whole dupfd thing was added there for ZaphodHeads screens
>> in the first place. See
>> https://bugs.freedesktop.org/show_bug.cgi?id=79823 and commit
>> a59f2bb17bcc which fixed it.
>>
>> Note that the hash has the following hash/eq functions:
>>
>> static unsigned hash_fd(void *key)
>> {
>> int fd = pointer_to_intptr(key);
>> struct stat stat;
>> fstat(fd, &stat);
>>
>> return stat.st_dev ^ stat.st_ino ^ stat.st_rdev;
>> }
>>
>> static int compare_fd(void *key1, void *key2)
>> {
>> int fd1 = pointer_to_intptr(key1);
>> int fd2 = pointer_to_intptr(key2);
>> struct stat stat1, stat2;
>> fstat(fd1, &stat1);
>> fstat(fd2, &stat2);
>>
>> return stat1.st_dev != stat2.st_dev ||
>> stat1.st_ino != stat2.st_ino ||
>> stat1.st_rdev != stat2.st_rdev;
>> }
>>
>> so fd and dupfd should get hashed to the same thing. I suspect there's
>> something else going on in your application...
>>
>
> My application is a set of dynamically loaded plugins running inside another
> host app (Psychtoolbox-3 inside GNU/Octave), so what happens often is that
> the OpenGL using plugin gets unloaded and reloaded at runtime, so a after a
> OpenGL session has ended with a XCloseDisplay() tearing the w
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
On 06/28/2015 05:41 AM, Ilia Mirkin wrote:
Oh duh. Thanks for the super-detailed explanation. To rephrase what
you said in a slightly shorter manner:
See bug https://bugs.freedesktop.org/show_bug.cgi?id=79823 for why we
need to dupfd (which we are already, although radeon might not be).
Radeon doesn't so far. My 2nd patch for radeon from earlier today adds that.
Except instead of sticking the dup'd fd into the hash table, whose
lifetime matches that of the device, we stick the other one in, which
is effectively owned by the X server. As a result, those fstat's might
fail down the line, and all sorts of hell will break loose.
Yes. Essentially we should make sure the fd's we keep around have the
same lifetime as the data structure in which we need to use it. That was
the point, the server owned fd went away while we still needed it.
Would you be opposed to me rewriting your commit message to basically
reflect the above? Perhaps your original one did as well, but it
wasn't clear to me. I'd also like to throw some assert's in to make
sure the fstat's don't fail -- does that sound reasonable?
Oh please, yes! My commit messages often have this disease that they are
either too terse, when i think the problem is obvious, or too long and
somewhat convoluted when i'm going overboard in the other direction. Any
editing/shortening for clarity more than happily accepted :)
thanks,
-mario
Another solution, btw, is to stick as the real key in the hash, although that'd involve
making pointers. Probably not worth it.
Cheers,
-ilia
On Sat, Jun 27, 2015 at 11:13 PM, Mario Kleiner
wrote:
On 06/28/2015 03:48 AM, Ilia Mirkin wrote:
On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner
wrote:
The dup'ed fd owned by the nouveau_screen for a device node
must also be used as key for the winsys hash table, instead
of using the original fd passed in for a screen, to make
multi-x-screen ZaphodHeads configurations work on nouveau.
This prevents the following crash scenario that was observed
when a dynamically loaded rendering plugin used OpenGL on a
ZaphodHeads setup, e.g., on a dual x-screen setup. At first
load the plugin worked, but after unloading and reloading it,
the next rendering operation crashed:
1. Client, e.g., a plugin, calls glXQueryVersion.
2. DRI screens for the x-screens 0 and 1 are created, one shared
nouveau_screen is created for the shared device node of both
screens, but the original fd of x-screen 0 is used as identifying
key in the hash table, instead of the dup()ed fd of x-screen 0
which is owned by the nouveau_screen. nouveau_screen's refcount
is now 2.
See below, but it shouldn't matter which fd gets used.
3. Regular rendering happens by the client plugin, then the plugin
gets unloaded.
4. XCloseDisplay(). x-screen 0 gets its DRI screen destroyed,
nouveau_drm_screen_unref() drops the refcount to 1, calling mesa
code then closes the fd of x-screen 0, so now the fd which is
used as key in the hash table is invalid. x-screen 1 gets
destroyed, nouveau_drm_screen_unref() drops the refcount to 0,
the nouveau_screen gets destroyed, but removal of its entry
in the hash table fails, because the invalid fd in the hash
table no longer matches anything (fstat() on the fd is used
for hashing and key comparison, but fstat() on an already closed
fd fails and returns bogus results). x-screen 1 closes its fd.
Now all fd's are closed, the nouveau_screen destroyed, but
there is a dangling reference to the nouveau_screen in the
hash table.
5. Some OpenGL client plugin gets loaded again and calls
glXQueryVersion. Step 2 above repeats, but because a
dangling reference with a matching fd is found in the winsys
hash table, no new nouveau_screen is created this time. Instead
the invalid pointer to the old nouveau_screen is recycled,
which points to nirvana -> Crash.
This problem is avoided by use of the dup()ed fd which is
owned by the nouveau_screen and has the same lifetime as
the nouveau_screen itself.
I need to think about this some more, but... this shouldn't happen :)
In fact, the whole dupfd thing was added there for ZaphodHeads screens
in the first place. See
https://bugs.freedesktop.org/show_bug.cgi?id=79823 and commit
a59f2bb17bcc which fixed it.
Note that the hash has the following hash/eq functions:
static unsigned hash_fd(void *key)
{
int fd = pointer_to_intptr(key);
struct stat stat;
fstat(fd, &stat);
return stat.st_dev ^ stat.st_ino ^ stat.st_rdev;
}
static int compare_fd(void *key1, void *key2)
{
int fd1 = pointer_to_intptr(key1);
int fd2 = pointer_to_intptr(key2);
struct stat stat1, stat2;
fstat(fd1, &stat1);
fstat(fd2, &stat2);
return stat1.st_dev != stat2.st_dev ||
stat1.st_ino != stat2.st_ino ||
stat1.st_rdev != stat2.st_rdev;
}
so fd and dupfd should get hashed t
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
How's this? I updated both the commit message and the comment in the code. If this is OK with you, I'll push. commit 627cd0d8c0f0052033c213dcef3ea7c6e414e942 Author: Mario Kleiner Date: Fri Jun 5 15:36:52 2015 +0200 nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads. The dup'ed fd owned by the nouveau_screen for a device node must also be used as key for the winsys hash table, instead of using the original fd passed in for a screen, to make multi-x-screen ZaphodHeads configurations work on nouveau. The original fd is owned by the X server, and its lifetime differs from that of the nouveau_screen stored in the hash. Since the hash key is the fd, and in order to compare hash entries we fstat them, the fd must be around for as long as the screen is. This is an extension of the fix in commit a59f2bb1 (nouveau: dup fd before passing it to device). Cc: "10.3 10.4 10.5 10.6" Signed-off-by: Mario Kleiner Reviewed-by: Ilia Mirkin diff --git a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c index 0635246..e08105f 100644 --- a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c +++ b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c @@ -120,7 +120,10 @@ nouveau_drm_screen_create(int fd) if (!screen) goto err; - util_hash_table_set(fd_tab, intptr_to_pointer(fd), screen); + /* Use dupfd in hash table, to avoid errors if the original fd gets +* closed by the X server +*/ + util_hash_table_set(fd_tab, intptr_to_pointer(dupfd), screen); screen->refcount = 1; pipe_mutex_unlock(nouveau_screen_mutex); return &screen->base; On Sun, Jun 28, 2015 at 12:03 AM, Mario Kleiner wrote: > On 06/28/2015 05:41 AM, Ilia Mirkin wrote: >> >> Oh duh. Thanks for the super-detailed explanation. To rephrase what >> you said in a slightly shorter manner: >> >> See bug https://bugs.freedesktop.org/show_bug.cgi?id=79823 for why we >> need to dupfd (which we are already, although radeon might not be). >> > > Radeon doesn't so far. My 2nd patch for radeon from earlier today adds that. > >> Except instead of sticking the dup'd fd into the hash table, whose >> lifetime matches that of the device, we stick the other one in, which >> is effectively owned by the X server. As a result, those fstat's might >> fail down the line, and all sorts of hell will break loose. >> > > Yes. Essentially we should make sure the fd's we keep around have the same > lifetime as the data structure in which we need to use it. That was the > point, the server owned fd went away while we still needed it. > >> Would you be opposed to me rewriting your commit message to basically >> reflect the above? Perhaps your original one did as well, but it >> wasn't clear to me. I'd also like to throw some assert's in to make >> sure the fstat's don't fail -- does that sound reasonable? >> > > Oh please, yes! My commit messages often have this disease that they are > either too terse, when i think the problem is obvious, or too long and > somewhat convoluted when i'm going overboard in the other direction. Any > editing/shortening for clarity more than happily accepted :) > > thanks, > -mario > > >> Another solution, btw, is to stick > stat.st_rdev> as the real key in the hash, although that'd involve >> making pointers. Probably not worth it. >> >> Cheers, >> >>-ilia >> >> On Sat, Jun 27, 2015 at 11:13 PM, Mario Kleiner >> wrote: >>> >>> On 06/28/2015 03:48 AM, Ilia Mirkin wrote: On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner wrote: > > > The dup'ed fd owned by the nouveau_screen for a device node > must also be used as key for the winsys hash table, instead > of using the original fd passed in for a screen, to make > multi-x-screen ZaphodHeads configurations work on nouveau. > > This prevents the following crash scenario that was observed > when a dynamically loaded rendering plugin used OpenGL on a > ZaphodHeads setup, e.g., on a dual x-screen setup. At first > load the plugin worked, but after unloading and reloading it, > the next rendering operation crashed: > > 1. Client, e.g., a plugin, calls glXQueryVersion. > > 2. DRI screens for the x-screens 0 and 1 are created, one shared > nouveau_screen is created for the shared device node of both > screens, but the original fd of x-screen 0 is used as identifying > key in the hash table, instead of the dup()ed fd of x-screen 0 > which is owned by the nouveau_screen. nouveau_screen's refcount > is now 2. See below, but it shouldn't matter which fd gets used. > > 3. Regular rendering happens by the client plugin, then the plugin > gets unloaded. > > 4. XCloseDisplay(). x-screen 0 gets its DRI screen destroyed, > nouveau
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
Ok, maybe one thing for the commit message, as i just made the same
mixup in my reply: The fd's are not owned by the x-server, but by the
mesa screens (pipe screens? dri screens?) they represent, so if such a
screen goes away, the fd goes away. Using "X server" would be confusing,
especially under dri3, although each such "mesa screen" corresponds to a
x-screen.
-mario
On 06/28/2015 06:03 AM, Mario Kleiner wrote:
On 06/28/2015 05:41 AM, Ilia Mirkin wrote:
Oh duh. Thanks for the super-detailed explanation. To rephrase what
you said in a slightly shorter manner:
See bug https://bugs.freedesktop.org/show_bug.cgi?id=79823 for why we
need to dupfd (which we are already, although radeon might not be).
Radeon doesn't so far. My 2nd patch for radeon from earlier today adds
that.
Except instead of sticking the dup'd fd into the hash table, whose
lifetime matches that of the device, we stick the other one in, which
is effectively owned by the X server. As a result, those fstat's might
fail down the line, and all sorts of hell will break loose.
Yes. Essentially we should make sure the fd's we keep around have the
same lifetime as the data structure in which we need to use it. That was
the point, the server owned fd went away while we still needed it.
Would you be opposed to me rewriting your commit message to basically
reflect the above? Perhaps your original one did as well, but it
wasn't clear to me. I'd also like to throw some assert's in to make
sure the fstat's don't fail -- does that sound reasonable?
Oh please, yes! My commit messages often have this disease that they are
either too terse, when i think the problem is obvious, or too long and
somewhat convoluted when i'm going overboard in the other direction. Any
editing/shortening for clarity more than happily accepted :)
thanks,
-mario
Another solution, btw, is to stick as the real key in the hash, although that'd involve
making pointers. Probably not worth it.
Cheers,
-ilia
On Sat, Jun 27, 2015 at 11:13 PM, Mario Kleiner
wrote:
On 06/28/2015 03:48 AM, Ilia Mirkin wrote:
On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner
wrote:
The dup'ed fd owned by the nouveau_screen for a device node
must also be used as key for the winsys hash table, instead
of using the original fd passed in for a screen, to make
multi-x-screen ZaphodHeads configurations work on nouveau.
This prevents the following crash scenario that was observed
when a dynamically loaded rendering plugin used OpenGL on a
ZaphodHeads setup, e.g., on a dual x-screen setup. At first
load the plugin worked, but after unloading and reloading it,
the next rendering operation crashed:
1. Client, e.g., a plugin, calls glXQueryVersion.
2. DRI screens for the x-screens 0 and 1 are created, one shared
nouveau_screen is created for the shared device node of both
screens, but the original fd of x-screen 0 is used as identifying
key in the hash table, instead of the dup()ed fd of x-screen 0
which is owned by the nouveau_screen. nouveau_screen's refcount
is now 2.
See below, but it shouldn't matter which fd gets used.
3. Regular rendering happens by the client plugin, then the plugin
gets unloaded.
4. XCloseDisplay(). x-screen 0 gets its DRI screen destroyed,
nouveau_drm_screen_unref() drops the refcount to 1, calling mesa
code then closes the fd of x-screen 0, so now the fd which is
used as key in the hash table is invalid. x-screen 1 gets
destroyed, nouveau_drm_screen_unref() drops the refcount to 0,
the nouveau_screen gets destroyed, but removal of its entry
in the hash table fails, because the invalid fd in the hash
table no longer matches anything (fstat() on the fd is used
for hashing and key comparison, but fstat() on an already closed
fd fails and returns bogus results). x-screen 1 closes its fd.
Now all fd's are closed, the nouveau_screen destroyed, but
there is a dangling reference to the nouveau_screen in the
hash table.
5. Some OpenGL client plugin gets loaded again and calls
glXQueryVersion. Step 2 above repeats, but because a
dangling reference with a matching fd is found in the winsys
hash table, no new nouveau_screen is created this time. Instead
the invalid pointer to the old nouveau_screen is recycled,
which points to nirvana -> Crash.
This problem is avoided by use of the dup()ed fd which is
owned by the nouveau_screen and has the same lifetime as
the nouveau_screen itself.
I need to think about this some more, but... this shouldn't happen :)
In fact, the whole dupfd thing was added there for ZaphodHeads screens
in the first place. See
https://bugs.freedesktop.org/show_bug.cgi?id=79823 and commit
a59f2bb17bcc which fixed it.
Note that the hash has the following hash/eq functions:
static unsigned hash_fd(void *key)
{
int fd = pointer_to_intptr(key);
struct stat stat;
fstat(fd, &stat);
return stat.st_dev ^ s
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
But that's the thing... in this case, the fd lifetime is owned by the X server. In fact, nouveau doesn't touch that fd in mesa beyond dup'ing it, and then exclusively using the dup'd fd. On Sun, Jun 28, 2015 at 12:23 AM, Mario Kleiner wrote: > Ok, maybe one thing for the commit message, as i just made the same mixup in > my reply: The fd's are not owned by the x-server, but by the mesa screens > (pipe screens? dri screens?) they represent, so if such a screen goes away, > the fd goes away. Using "X server" would be confusing, especially under > dri3, although each such "mesa screen" corresponds to a x-screen. > > -mario > > > On 06/28/2015 06:03 AM, Mario Kleiner wrote: >> >> On 06/28/2015 05:41 AM, Ilia Mirkin wrote: >>> >>> Oh duh. Thanks for the super-detailed explanation. To rephrase what >>> you said in a slightly shorter manner: >>> >>> See bug https://bugs.freedesktop.org/show_bug.cgi?id=79823 for why we >>> need to dupfd (which we are already, although radeon might not be). >>> >> >> Radeon doesn't so far. My 2nd patch for radeon from earlier today adds >> that. >> >>> Except instead of sticking the dup'd fd into the hash table, whose >>> lifetime matches that of the device, we stick the other one in, which >>> is effectively owned by the X server. As a result, those fstat's might >>> fail down the line, and all sorts of hell will break loose. >>> >> >> Yes. Essentially we should make sure the fd's we keep around have the >> same lifetime as the data structure in which we need to use it. That was >> the point, the server owned fd went away while we still needed it. >> >>> Would you be opposed to me rewriting your commit message to basically >>> reflect the above? Perhaps your original one did as well, but it >>> wasn't clear to me. I'd also like to throw some assert's in to make >>> sure the fstat's don't fail -- does that sound reasonable? >>> >> >> Oh please, yes! My commit messages often have this disease that they are >> either too terse, when i think the problem is obvious, or too long and >> somewhat convoluted when i'm going overboard in the other direction. Any >> editing/shortening for clarity more than happily accepted :) >> >> thanks, >> -mario >> >>> Another solution, btw, is to stick >> stat.st_rdev> as the real key in the hash, although that'd involve >>> making pointers. Probably not worth it. >>> >>> Cheers, >>> >>>-ilia >>> >>> On Sat, Jun 27, 2015 at 11:13 PM, Mario Kleiner >>> wrote: On 06/28/2015 03:48 AM, Ilia Mirkin wrote: > > > On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner > wrote: >> >> >> The dup'ed fd owned by the nouveau_screen for a device node >> must also be used as key for the winsys hash table, instead >> of using the original fd passed in for a screen, to make >> multi-x-screen ZaphodHeads configurations work on nouveau. >> >> This prevents the following crash scenario that was observed >> when a dynamically loaded rendering plugin used OpenGL on a >> ZaphodHeads setup, e.g., on a dual x-screen setup. At first >> load the plugin worked, but after unloading and reloading it, >> the next rendering operation crashed: >> >> 1. Client, e.g., a plugin, calls glXQueryVersion. >> >> 2. DRI screens for the x-screens 0 and 1 are created, one shared >> nouveau_screen is created for the shared device node of both >> screens, but the original fd of x-screen 0 is used as identifying >> key in the hash table, instead of the dup()ed fd of x-screen 0 >> which is owned by the nouveau_screen. nouveau_screen's refcount >> is now 2. > > > > See below, but it shouldn't matter which fd gets used. > >> >> 3. Regular rendering happens by the client plugin, then the plugin >> gets unloaded. >> >> 4. XCloseDisplay(). x-screen 0 gets its DRI screen destroyed, >> nouveau_drm_screen_unref() drops the refcount to 1, calling mesa >> code then closes the fd of x-screen 0, so now the fd which is >> used as key in the hash table is invalid. x-screen 1 gets >> destroyed, nouveau_drm_screen_unref() drops the refcount to 0, >> the nouveau_screen gets destroyed, but removal of its entry >> in the hash table fails, because the invalid fd in the hash >> table no longer matches anything (fstat() on the fd is used >> for hashing and key comparison, but fstat() on an already closed >> fd fails and returns bogus results). x-screen 1 closes its fd. >> >> Now all fd's are closed, the nouveau_screen destroyed, but >> there is a dangling reference to the nouveau_screen in the >> hash table. >> >> 5. Some OpenGL client plugin gets loaded again and calls >> glXQueryVersion. Step 2 above repeats, but because a >> dangling reference with a matching fd is found in the winsys >> hash table,
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
On 06/28/2015 06:25 AM, Ilia Mirkin wrote: But that's the thing... in this case, the fd lifetime is owned by the X server. In fact, nouveau doesn't touch that fd in mesa beyond dup'ing it, and then exclusively using the dup'd fd. True, it's not owned by nouveau, but instead by mesa core code? I think the x-server itself is only involved in authenticating the fd under DRI2, after mesa has opened it? Although under DRI3 the ready made fd comes from a xcb_dri3_open() trip to the server. See the open() call on the device file: dri2CreateScreen() in mesa/src/glx/dri2_glx.c xcb_dri3_open(): dri3_create_screen() in mesa/src/glx/dri3_glx.c And the close() calls for the fd's are also inside those files. Maybe those winsys functions we fixed here are also involved in things like EGL for wayland etc.? Haven't checked this. Other than that your new commit message is fine. Also i am possibly just confused about this, and your commit message is an improvement in clarity anyway :) So i think it is fine to leave it as is. thanks, -mario On Sun, Jun 28, 2015 at 12:23 AM, Mario Kleiner wrote: Ok, maybe one thing for the commit message, as i just made the same mixup in my reply: The fd's are not owned by the x-server, but by the mesa screens (pipe screens? dri screens?) they represent, so if such a screen goes away, the fd goes away. Using "X server" would be confusing, especially under dri3, although each such "mesa screen" corresponds to a x-screen. -mario On 06/28/2015 06:03 AM, Mario Kleiner wrote: On 06/28/2015 05:41 AM, Ilia Mirkin wrote: Oh duh. Thanks for the super-detailed explanation. To rephrase what you said in a slightly shorter manner: See bug https://bugs.freedesktop.org/show_bug.cgi?id=79823 for why we need to dupfd (which we are already, although radeon might not be). Radeon doesn't so far. My 2nd patch for radeon from earlier today adds that. Except instead of sticking the dup'd fd into the hash table, whose lifetime matches that of the device, we stick the other one in, which is effectively owned by the X server. As a result, those fstat's might fail down the line, and all sorts of hell will break loose. Yes. Essentially we should make sure the fd's we keep around have the same lifetime as the data structure in which we need to use it. That was the point, the server owned fd went away while we still needed it. Would you be opposed to me rewriting your commit message to basically reflect the above? Perhaps your original one did as well, but it wasn't clear to me. I'd also like to throw some assert's in to make sure the fstat's don't fail -- does that sound reasonable? Oh please, yes! My commit messages often have this disease that they are either too terse, when i think the problem is obvious, or too long and somewhat convoluted when i'm going overboard in the other direction. Any editing/shortening for clarity more than happily accepted :) thanks, -mario Another solution, btw, is to stick as the real key in the hash, although that'd involve making pointers. Probably not worth it. Cheers, -ilia On Sat, Jun 27, 2015 at 11:13 PM, Mario Kleiner wrote: On 06/28/2015 03:48 AM, Ilia Mirkin wrote: On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner wrote: The dup'ed fd owned by the nouveau_screen for a device node must also be used as key for the winsys hash table, instead of using the original fd passed in for a screen, to make multi-x-screen ZaphodHeads configurations work on nouveau. This prevents the following crash scenario that was observed when a dynamically loaded rendering plugin used OpenGL on a ZaphodHeads setup, e.g., on a dual x-screen setup. At first load the plugin worked, but after unloading and reloading it, the next rendering operation crashed: 1. Client, e.g., a plugin, calls glXQueryVersion. 2. DRI screens for the x-screens 0 and 1 are created, one shared nouveau_screen is created for the shared device node of both screens, but the original fd of x-screen 0 is used as identifying key in the hash table, instead of the dup()ed fd of x-screen 0 which is owned by the nouveau_screen. nouveau_screen's refcount is now 2. See below, but it shouldn't matter which fd gets used. 3. Regular rendering happens by the client plugin, then the plugin gets unloaded. 4. XCloseDisplay(). x-screen 0 gets its DRI screen destroyed, nouveau_drm_screen_unref() drops the refcount to 1, calling mesa code then closes the fd of x-screen 0, so now the fd which is used as key in the hash table is invalid. x-screen 1 gets destroyed, nouveau_drm_screen_unref() drops the refcount to 0, the nouveau_screen gets destroyed, but removal of its entry in the hash table fails, because the invalid fd in the hash table no longer matches anything (fstat() on the fd is used for hashing and key comparison, but fstat() on an already closed fd fails and returns bogus results). x-scr
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
How about: /* Use dupfd in hash table, to avoid errors if the original fd gets * closed by its owner. The hash key needs to live at least as long as * the screen. */ On Sun, Jun 28, 2015 at 12:57 AM, Mario Kleiner wrote: > On 06/28/2015 06:25 AM, Ilia Mirkin wrote: >> >> But that's the thing... in this case, the fd lifetime is owned by the >> X server. In fact, nouveau doesn't touch that fd in mesa beyond >> dup'ing it, and then exclusively using the dup'd fd. >> > > True, it's not owned by nouveau, but instead by mesa core code? I think the > x-server itself is only involved in authenticating the fd under DRI2, after > mesa has opened it? Although under DRI3 the ready made fd comes from a > xcb_dri3_open() trip to the server. > > See the open() call on the device file: > dri2CreateScreen() in mesa/src/glx/dri2_glx.c > > xcb_dri3_open(): > dri3_create_screen() in mesa/src/glx/dri3_glx.c > > And the close() calls for the fd's are also inside those files. > > Maybe those winsys functions we fixed here are also involved in things like > EGL for wayland etc.? Haven't checked this. > > Other than that your new commit message is fine. Also i am possibly just > confused about this, and your commit message is an improvement in clarity > anyway :) > > So i think it is fine to leave it as is. > > thanks, > -mario > > > >> On Sun, Jun 28, 2015 at 12:23 AM, Mario Kleiner >> wrote: >>> >>> Ok, maybe one thing for the commit message, as i just made the same mixup >>> in >>> my reply: The fd's are not owned by the x-server, but by the mesa screens >>> (pipe screens? dri screens?) they represent, so if such a screen goes >>> away, >>> the fd goes away. Using "X server" would be confusing, especially under >>> dri3, although each such "mesa screen" corresponds to a x-screen. >>> >>> -mario >>> >>> >>> On 06/28/2015 06:03 AM, Mario Kleiner wrote: On 06/28/2015 05:41 AM, Ilia Mirkin wrote: > > > Oh duh. Thanks for the super-detailed explanation. To rephrase what > you said in a slightly shorter manner: > > See bug https://bugs.freedesktop.org/show_bug.cgi?id=79823 for why we > need to dupfd (which we are already, although radeon might not be). > Radeon doesn't so far. My 2nd patch for radeon from earlier today adds that. > Except instead of sticking the dup'd fd into the hash table, whose > lifetime matches that of the device, we stick the other one in, which > is effectively owned by the X server. As a result, those fstat's might > fail down the line, and all sorts of hell will break loose. > Yes. Essentially we should make sure the fd's we keep around have the same lifetime as the data structure in which we need to use it. That was the point, the server owned fd went away while we still needed it. > Would you be opposed to me rewriting your commit message to basically > reflect the above? Perhaps your original one did as well, but it > wasn't clear to me. I'd also like to throw some assert's in to make > sure the fstat's don't fail -- does that sound reasonable? > Oh please, yes! My commit messages often have this disease that they are either too terse, when i think the problem is obvious, or too long and somewhat convoluted when i'm going overboard in the other direction. Any editing/shortening for clarity more than happily accepted :) thanks, -mario > Another solution, btw, is to stick stat.st_rdev> as the real key in the hash, although that'd involve > making pointers. Probably not worth it. > > Cheers, > > -ilia > > On Sat, Jun 27, 2015 at 11:13 PM, Mario Kleiner > wrote: >> >> >> On 06/28/2015 03:48 AM, Ilia Mirkin wrote: >>> >>> >>> >>> On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner >>> wrote: The dup'ed fd owned by the nouveau_screen for a device node must also be used as key for the winsys hash table, instead of using the original fd passed in for a screen, to make multi-x-screen ZaphodHeads configurations work on nouveau. This prevents the following crash scenario that was observed when a dynamically loaded rendering plugin used OpenGL on a ZaphodHeads setup, e.g., on a dual x-screen setup. At first load the plugin worked, but after unloading and reloading it, the next rendering operation crashed: 1. Client, e.g., a plugin, calls glXQueryVersion. 2. DRI screens for the x-screens 0 and 1 are created, one shared nouveau_screen is created for the shared device node of both screens, but the original fd of x-screen 0 is used as identifying key in the hash table, instead of the dup()ed fd of x-screen 0 which is owned by the nouveau_screen. nouveau
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
Yes, that's good. On 06/28/2015 07:00 AM, Ilia Mirkin wrote: How about: /* Use dupfd in hash table, to avoid errors if the original fd gets * closed by its owner. The hash key needs to live at least as long as * the screen. */ On Sun, Jun 28, 2015 at 12:57 AM, Mario Kleiner wrote: On 06/28/2015 06:25 AM, Ilia Mirkin wrote: But that's the thing... in this case, the fd lifetime is owned by the X server. In fact, nouveau doesn't touch that fd in mesa beyond dup'ing it, and then exclusively using the dup'd fd. True, it's not owned by nouveau, but instead by mesa core code? I think the x-server itself is only involved in authenticating the fd under DRI2, after mesa has opened it? Although under DRI3 the ready made fd comes from a xcb_dri3_open() trip to the server. See the open() call on the device file: dri2CreateScreen() in mesa/src/glx/dri2_glx.c xcb_dri3_open(): dri3_create_screen() in mesa/src/glx/dri3_glx.c And the close() calls for the fd's are also inside those files. Maybe those winsys functions we fixed here are also involved in things like EGL for wayland etc.? Haven't checked this. Other than that your new commit message is fine. Also i am possibly just confused about this, and your commit message is an improvement in clarity anyway :) So i think it is fine to leave it as is. thanks, -mario On Sun, Jun 28, 2015 at 12:23 AM, Mario Kleiner wrote: Ok, maybe one thing for the commit message, as i just made the same mixup in my reply: The fd's are not owned by the x-server, but by the mesa screens (pipe screens? dri screens?) they represent, so if such a screen goes away, the fd goes away. Using "X server" would be confusing, especially under dri3, although each such "mesa screen" corresponds to a x-screen. -mario On 06/28/2015 06:03 AM, Mario Kleiner wrote: On 06/28/2015 05:41 AM, Ilia Mirkin wrote: Oh duh. Thanks for the super-detailed explanation. To rephrase what you said in a slightly shorter manner: See bug https://bugs.freedesktop.org/show_bug.cgi?id=79823 for why we need to dupfd (which we are already, although radeon might not be). Radeon doesn't so far. My 2nd patch for radeon from earlier today adds that. Except instead of sticking the dup'd fd into the hash table, whose lifetime matches that of the device, we stick the other one in, which is effectively owned by the X server. As a result, those fstat's might fail down the line, and all sorts of hell will break loose. Yes. Essentially we should make sure the fd's we keep around have the same lifetime as the data structure in which we need to use it. That was the point, the server owned fd went away while we still needed it. Would you be opposed to me rewriting your commit message to basically reflect the above? Perhaps your original one did as well, but it wasn't clear to me. I'd also like to throw some assert's in to make sure the fstat's don't fail -- does that sound reasonable? Oh please, yes! My commit messages often have this disease that they are either too terse, when i think the problem is obvious, or too long and somewhat convoluted when i'm going overboard in the other direction. Any editing/shortening for clarity more than happily accepted :) thanks, -mario Another solution, btw, is to stick as the real key in the hash, although that'd involve making pointers. Probably not worth it. Cheers, -ilia On Sat, Jun 27, 2015 at 11:13 PM, Mario Kleiner wrote: On 06/28/2015 03:48 AM, Ilia Mirkin wrote: On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner wrote: The dup'ed fd owned by the nouveau_screen for a device node must also be used as key for the winsys hash table, instead of using the original fd passed in for a screen, to make multi-x-screen ZaphodHeads configurations work on nouveau. This prevents the following crash scenario that was observed when a dynamically loaded rendering plugin used OpenGL on a ZaphodHeads setup, e.g., on a dual x-screen setup. At first load the plugin worked, but after unloading and reloading it, the next rendering operation crashed: 1. Client, e.g., a plugin, calls glXQueryVersion. 2. DRI screens for the x-screens 0 and 1 are created, one shared nouveau_screen is created for the shared device node of both screens, but the original fd of x-screen 0 is used as identifying key in the hash table, instead of the dup()ed fd of x-screen 0 which is owned by the nouveau_screen. nouveau_screen's refcount is now 2. See below, but it shouldn't matter which fd gets used. 3. Regular rendering happens by the client plugin, then the plugin gets unloaded. 4. XCloseDisplay(). x-screen 0 gets its DRI screen destroyed, nouveau_drm_screen_unref() drops the refcount to 1, calling mesa code then closes the fd of x-screen 0, so now the fd which is used as key in the hash table is invalid. x-screen 1 gets destroyed, nouveau_drm_screen_unref() drops the refcount to 0, the nouvea
Re: [Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
Pushed. Thanks for the fix and for bearing with me! On Sun, Jun 28, 2015 at 1:01 AM, Mario Kleiner wrote: > Yes, that's good. > > > On 06/28/2015 07:00 AM, Ilia Mirkin wrote: >> >> How about: >> >> /* Use dupfd in hash table, to avoid errors if the original fd gets >> * closed by its owner. The hash key needs to live at least as long as >> * the screen. >> */ >> >> >> On Sun, Jun 28, 2015 at 12:57 AM, Mario Kleiner >> wrote: >>> >>> On 06/28/2015 06:25 AM, Ilia Mirkin wrote: But that's the thing... in this case, the fd lifetime is owned by the X server. In fact, nouveau doesn't touch that fd in mesa beyond dup'ing it, and then exclusively using the dup'd fd. >>> >>> True, it's not owned by nouveau, but instead by mesa core code? I think >>> the >>> x-server itself is only involved in authenticating the fd under DRI2, >>> after >>> mesa has opened it? Although under DRI3 the ready made fd comes from a >>> xcb_dri3_open() trip to the server. >>> >>> See the open() call on the device file: >>> dri2CreateScreen() in mesa/src/glx/dri2_glx.c >>> >>> xcb_dri3_open(): >>> dri3_create_screen() in mesa/src/glx/dri3_glx.c >>> >>> And the close() calls for the fd's are also inside those files. >>> >>> Maybe those winsys functions we fixed here are also involved in things >>> like >>> EGL for wayland etc.? Haven't checked this. >>> >>> Other than that your new commit message is fine. Also i am possibly just >>> confused about this, and your commit message is an improvement in clarity >>> anyway :) >>> >>> So i think it is fine to leave it as is. >>> >>> thanks, >>> -mario >>> >>> >>> On Sun, Jun 28, 2015 at 12:23 AM, Mario Kleiner wrote: > > > Ok, maybe one thing for the commit message, as i just made the same > mixup > in > my reply: The fd's are not owned by the x-server, but by the mesa > screens > (pipe screens? dri screens?) they represent, so if such a screen goes > away, > the fd goes away. Using "X server" would be confusing, especially under > dri3, although each such "mesa screen" corresponds to a x-screen. > > -mario > > > On 06/28/2015 06:03 AM, Mario Kleiner wrote: >> >> >> >> On 06/28/2015 05:41 AM, Ilia Mirkin wrote: >>> >>> >>> >>> Oh duh. Thanks for the super-detailed explanation. To rephrase what >>> you said in a slightly shorter manner: >>> >>> See bug https://bugs.freedesktop.org/show_bug.cgi?id=79823 for why we >>> need to dupfd (which we are already, although radeon might not be). >>> >> >> Radeon doesn't so far. My 2nd patch for radeon from earlier today adds >> that. >> >>> Except instead of sticking the dup'd fd into the hash table, whose >>> lifetime matches that of the device, we stick the other one in, which >>> is effectively owned by the X server. As a result, those fstat's >>> might >>> fail down the line, and all sorts of hell will break loose. >>> >> >> Yes. Essentially we should make sure the fd's we keep around have the >> same lifetime as the data structure in which we need to use it. That >> was >> the point, the server owned fd went away while we still needed it. >> >>> Would you be opposed to me rewriting your commit message to basically >>> reflect the above? Perhaps your original one did as well, but it >>> wasn't clear to me. I'd also like to throw some assert's in to make >>> sure the fstat's don't fail -- does that sound reasonable? >>> >> >> Oh please, yes! My commit messages often have this disease that they >> are >> either too terse, when i think the problem is obvious, or too long and >> somewhat convoluted when i'm going overboard in the other direction. >> Any >> editing/shortening for clarity more than happily accepted :) >> >> thanks, >> -mario >> >>> Another solution, btw, is to stick >> stat.st_rdev> as the real key in the hash, although that'd involve >>> making pointers. Probably not worth it. >>> >>> Cheers, >>> >>> -ilia >>> >>> On Sat, Jun 27, 2015 at 11:13 PM, Mario Kleiner >>> wrote: On 06/28/2015 03:48 AM, Ilia Mirkin wrote: > > > > > On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner > wrote: >> >> >> >> >> The dup'ed fd owned by the nouveau_screen for a device node >> must also be used as key for the winsys hash table, instead >> of using the original fd passed in for a screen, to make >> multi-x-screen ZaphodHeads configurations work on nouveau. >> >> This prevents the following crash scenario that was observed >> when a dynamically loaded rendering plugin used OpenGL on a >> ZaphodHeads setup, e.g., on a dual x-screen setup. At first >> load th
Re: [Mesa-dev] [PATCH] radeon: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
Without being intimately familiar with the radeon winsys code, this
seems sane (esp after re-reviewing the situation for nouveau).
Reviewed-by: Ilia Mirkin . But one of the radeon
people definitely needs to look at this before pushing.
On Sat, Jun 27, 2015 at 9:02 PM, Mario Kleiner
wrote:
> Same problem and fix as for nouveau's ZaphodHeads trouble.
>
> See patch ...
>
> "nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads."
>
> ... for reference.
>
> Cc: "10.3 10.4 10.5 10.6"
>
> Signed-off-by: Mario Kleiner
> Cc: Ilia Mirkin
> ---
> src/gallium/winsys/radeon/drm/radeon_drm_winsys.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> index ba8d143..ebdb19e 100644
> --- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> +++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> @@ -484,6 +484,10 @@ static void radeon_winsys_destroy(struct radeon_winsys
> *rws)
> if (ws->gen >= DRV_R600) {
> radeon_surface_manager_free(ws->surf_man);
> }
> +
> +if (ws->fd)
> +close(ws->fd);
> +
> FREE(rws);
> }
>
> @@ -696,7 +700,7 @@ radeon_drm_winsys_create(int fd, radeon_screen_create_t
> screen_create)
> return NULL;
> }
>
> -ws->fd = fd;
> +ws->fd = dup(fd);
>
> if (!do_winsys_init(ws))
> goto fail;
> @@ -712,7 +716,7 @@ radeon_drm_winsys_create(int fd, radeon_screen_create_t
> screen_create)
> goto fail;
>
> if (ws->gen >= DRV_R600) {
> -ws->surf_man = radeon_surface_manager_new(fd);
> +ws->surf_man = radeon_surface_manager_new(ws->fd);
> if (!ws->surf_man)
> goto fail;
> }
> @@ -753,7 +757,7 @@ radeon_drm_winsys_create(int fd, radeon_screen_create_t
> screen_create)
> return NULL;
> }
>
> -util_hash_table_set(fd_tab, intptr_to_pointer(fd), ws);
> +util_hash_table_set(fd_tab, intptr_to_pointer(ws->fd), ws);
>
> /* We must unlock the mutex once the winsys is fully initialized, so that
> * other threads attempting to create the winsys from the same fd will
> @@ -770,6 +774,9 @@ fail:
> ws->kman->destroy(ws->kman);
> if (ws->surf_man)
> radeon_surface_manager_free(ws->surf_man);
> +if (ws->fd)
> +close(ws->fd);
> +
> FREE(ws);
> return NULL;
> }
> --
> 2.1.4
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radeon: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
Thanks for the review Ilia. Just saw that the new amdgpu winsys driver
will need the same fix, once this one is confirmed to be ok.
thanks,
-mario
On 06/28/2015 07:23 AM, Ilia Mirkin wrote:
Without being intimately familiar with the radeon winsys code, this
seems sane (esp after re-reviewing the situation for nouveau).
Reviewed-by: Ilia Mirkin . But one of the radeon
people definitely needs to look at this before pushing.
On Sat, Jun 27, 2015 at 9:02 PM, Mario Kleiner
wrote:
Same problem and fix as for nouveau's ZaphodHeads trouble.
See patch ...
"nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads."
... for reference.
Cc: "10.3 10.4 10.5 10.6"
Signed-off-by: Mario Kleiner
Cc: Ilia Mirkin
---
src/gallium/winsys/radeon/drm/radeon_drm_winsys.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
index ba8d143..ebdb19e 100644
--- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
+++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
@@ -484,6 +484,10 @@ static void radeon_winsys_destroy(struct radeon_winsys
*rws)
if (ws->gen >= DRV_R600) {
radeon_surface_manager_free(ws->surf_man);
}
+
+if (ws->fd)
+close(ws->fd);
+
FREE(rws);
}
@@ -696,7 +700,7 @@ radeon_drm_winsys_create(int fd, radeon_screen_create_t
screen_create)
return NULL;
}
-ws->fd = fd;
+ws->fd = dup(fd);
if (!do_winsys_init(ws))
goto fail;
@@ -712,7 +716,7 @@ radeon_drm_winsys_create(int fd, radeon_screen_create_t
screen_create)
goto fail;
if (ws->gen >= DRV_R600) {
-ws->surf_man = radeon_surface_manager_new(fd);
+ws->surf_man = radeon_surface_manager_new(ws->fd);
if (!ws->surf_man)
goto fail;
}
@@ -753,7 +757,7 @@ radeon_drm_winsys_create(int fd, radeon_screen_create_t
screen_create)
return NULL;
}
-util_hash_table_set(fd_tab, intptr_to_pointer(fd), ws);
+util_hash_table_set(fd_tab, intptr_to_pointer(ws->fd), ws);
/* We must unlock the mutex once the winsys is fully initialized, so that
* other threads attempting to create the winsys from the same fd will
@@ -770,6 +774,9 @@ fail:
ws->kman->destroy(ws->kman);
if (ws->surf_man)
radeon_surface_manager_free(ws->surf_man);
+if (ws->fd)
+close(ws->fd);
+
FREE(ws);
return NULL;
}
--
2.1.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
