Re: [Mesa-dev] radeonsi: glmark2 - regression (GL_INVALID_OPERATION in glFramebufferTexture2D) - bisected
Hi, On Tuesday, 2 July 2019 10:25:41 CEST Michel Dänzer wrote: > On 2019-07-02 3:44 a.m., Dieter Nützel wrote: > > > > /opt/mesa> git bisect good > > b5697c311b6f29dee40b96c48bad3279e3667c1e is the first bad commit > > commit b5697c311b6f29dee40b96c48bad3279e3667c1e > > Author: Marek Olšák > > Date: Thu May 9 21:04:23 2019 -0400 > > > > Change a few frequented uses of DEBUG to !NDEBUG > > > > debugoptimized builds don't define NDEBUG, but they also don't define > > DEBUG. We want to enable cheap debug code for these builds. > > I only chose those occurences that I care about. > > > > Reviewed-by: Mathias Fröhlich > > > > src/gallium/auxiliary/tgsi/tgsi_ureg.c | 2 +- > > src/gallium/drivers/radeonsi/si_descriptors.c | 2 +- > > src/gallium/drivers/radeonsi/si_pipe.h | 2 +- > > src/gallium/drivers/radeonsi/si_shader_tgsi_setup.c | 6 +++--- > > src/gallium/drivers/radeonsi/si_state.c | 4 ++-- > > src/mesa/main/context.c | 2 +- > > src/mesa/main/debug.c | 4 ++-- > > src/mesa/main/errors.c | 6 +++--- > > src/mesa/main/feedback.c| 2 +- > > src/mesa/main/formats.c | 2 -- > > src/mesa/main/imports.c | 4 ++-- > > src/mesa/main/mtypes.h | 2 +- > > src/mesa/main/shaderapi.c | 2 +- > > src/mesa/state_tracker/st_atom_framebuffer.c| 2 +- > > src/mesa/state_tracker/st_format.c | 2 +- > > src/mesa/vbo/vbo_exec.h | 2 +- > > src/mesa/vbo/vbo_exec_api.c | 6 +++--- > > src/util/slab.c | 4 ++-- > > 18 files changed, 27 insertions(+), 29 deletions(-) > > The changes to src/mesa/main/errors.c mean that Mesa now prints messages > on GL API usage errors by default when assertions are enabled, whereas > previously it only did so for debugging builds. This should probably be > reverted, since these messages can be pretty noisy with some apps. My rationale was that it used to print before meson so it should print now again. But I have no strong opinion regarding debug builds being noisy. For production code compiled in release mode this would be a no go - for sure! We have way more code snippets guarded by DEBUG. Lots of them probably useful. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: Make EGL_EXT_device_drm optional.
Emil, Ok, I thought Ilja is a reference here. In this case forget that change! Thanks! best Mathias On Friday, 28 June 2019 18:26:07 CEST Emil Velikov wrote: > On 2019/06/28, mathias.froehl...@gmx.net wrote: > > From: Mathias Fröhlich > > > > Hi, > > > > Ilia mentioned that there are drm rendernode only drivers out there. > > To support an egl device on those platforms, make the EGL_EXT_device_drm > > device extension optional. > > > Currently DRM core mandates that primary node is available, even for GPU > only devices. > > A while back, we had a chat with kernel people why we do so. IIRC the > conclustion was that existing userspace will just work, since it already > must handle cases when you unplug your only monitor. > > I think Ilia was having a pretty reasonable assumption, that's why I did > before digging deeper, as opposed to saying there _are_ such cases. > > HTH > Emil > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: Don't add hardware device if there is no render node v2.
On Monday, 17 June 2019 19:33:26 CEST Emil Velikov wrote:
> On 2019/06/17, mathias.froehl...@gmx.net wrote:
> > From: Mathias Fröhlich
> >
> >
> > Emil,
> >
> > that one probably matches your original intent then.
> >
> > please review
> > Thanks
> >
> > Mathias
> >
> >
> > Do not offer a hardware drm backed egl device if no render node
> > is available. The current implementation will fail on this
> > egl device. On top it issues a warning that is actually missleading.
> > There are finally more error paths that can fail on the way to a
> > hardware backed egl device. Fixing all of them would kind of require
> > opening the drm device and see if there is a usable driver associated
> > with the device. The taken approach avoids a full probe and fixes at
> > least this kind of problem on kvm virtualization hosts I observe here.
> >
> Thanks
>
> Fixes: dbb4457d985 ("egl: add EGL_EXT_device_drm support")
> Reviewed-by: Emil Velikov
Thanks!
Pushed that!
best
Mathias
>
> -Emil
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] swrast: Avoid creating front buffers with __DRI_SWRAST_LOADER < 3.
Hi Emil, On Monday, 17 June 2019 19:35:10 CEST Emil Velikov wrote: > On 2019/06/06, mathias.froehl...@gmx.net wrote: > > From: Mathias Fröhlich > > > > Align classic swrast with galliums software renderer with respect > > to front buffer creation. > > In case of front buffers swrast uses the __DRI_SWRAST_LOADER extensions > > getImage/putImage callbacks to keep the front buffer in sync between > > the X server and the mesa software rendering. Gallium sofware rasterizers > > only allocates front buffers if the __DRI_SWRAST_LOADER extension version > > is >= 3. By that the getImage/putImage NULL pointers of the recent > > EGL_platform_device, which announces __DRI_SWRAST_LOADER version 1, are > > never called and thus this combination works fine. > > The change makes classic swrast work together with EGL_platform_device > > by using the same __DRI_SWRAST_LOADER version check than egl. > > For reference see drisw_allocate_textures in > > src/gallium/state_trackers/dri/drisw.c. > > > > Signed-off-by: Mathias Fröhlich > > --- > > src/mesa/drivers/dri/swrast/swrast.c | 5 - > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/src/mesa/drivers/dri/swrast/swrast.c > > b/src/mesa/drivers/dri/swrast/swrast.c > > index 8e8d6bd628e..832a45f1d88 100644 > > --- a/src/mesa/drivers/dri/swrast/swrast.c > > +++ b/src/mesa/drivers/dri/swrast/swrast.c > > @@ -572,7 +572,10 @@ dri_create_buffer(__DRIscreen * sPriv, > > _mesa_initialize_window_framebuffer(fb, visual); > > > > /* add front renderbuffer */ > > -frontrb = swrast_new_renderbuffer(visual, dPriv, GL_TRUE); > > +if (sPriv->swrast_loader->base.version >= 3) > > + frontrb = swrast_new_renderbuffer(visual, dPriv, GL_TRUE); > > +else > > + frontrb = swrast_new_renderbuffer(visual, dPriv, GL_FALSE); > > _mesa_attach_and_own_rb(fb, BUFFER_FRONT_LEFT, >Base.Base); > > > Doubt I'll be able to look at the classic swrast, anytime soon. Sorry :-\ Thanks for that information. Anybody else? Or can I do something else to make that work? Emil, I provided already working implementations as ready to be squashed updates to your egl device branch that implemented getImage/putImage, but you did not want to implement getImage/putImage by any means. Now that is the port of the if clause that that makes llvmpipe work with your implementation of the egl device without getImage/putImage. So what do you suggest? best and thanks Mathias > > -Emil > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl: Don't add hardware device if there is no render node.
On Monday, 10 June 2019 12:11:40 CEST Mathias Fröhlich wrote: > Hi Emil, > > On Friday, 7 June 2019 15:43:48 CEST Emil Velikov wrote: > > On Thu, 6 Jun 2019 at 12:10, wrote: > > > > > > From: Mathias Fröhlich > > > > > > Do not offer a hardware drm backed egl device if no render node > > > is available. > > As far as I can see current implementation does _not_ add the DRM > > device if its missing render node (and a primary one). > > Looking at the change below, it's effectively making the primary node > > optional. > > > > Hence the comment does not alight with the code - old and new. Can you > > elaborate? > > The currently pushed implementation asks drmGetDevices2 for a list of > devices and all devices with either a render node or a master node or both > are added as hardware device. This check is the bitmasking test with > device->available_nodes that you have put near the top of _eglAddDRMDevice. > So, if there is no render node the hardware device is added. > Later on the filename of the render node as returned from > _eglGetDRMDeviceRenderNode is opened which does not succeed in that case. > egInitialize fails then which is not nice. > > Past my change, a pure hardware device is not added if there is no render > node. > That is decided by this above mentioned bitmask test. > > The codepath that adds devices via _eglAddDevice from all the platforms is > untouched as this still uses the same bitmask as before. > I did not check this code path specifically above the call to _eglAddDevice > as this patch does not change the behavior of this case. > > best > > Mathias > > > > > I have not thought exactly how primary node-less DRM will work out > > esp. since the EGL_EXT_device_drm extension explicitly mentions one. Emil, ok, now I see. You also want the master node and bail out if that is not there. The problem is that the current EGLDevice code *needs* a user accessible render node to function, but does not enforce it to be there. It just says if we have either a render node or a master node or both go ahead. Your bitmaks test needs to read int wanted_nodes =(1 << DRM_NODE_PRIMARY | 1 << DRM_NODE_RENDER); if ((device->available_nodes & wanted_nodes) != wanted_nodes) return -1; if you want to have *both* bits set for a hardware device. Means with your current pushed code the master node is 'optional' as well!! From the use case that mostly drove the device extension originally - put the EGL_EXT_device_drm extension away - do make sense once you have a user accessible render node device file with a mesa driver that the loader can access. No matter if there is a master device file and no matter what (historic?) statement about a master node device file the EGL_EXT_device_drm makes ... Also, if the master node is returned from libdrm, it is returned from the string query function. Is that sufficient? Can we make the EGL_EXT_device_drm optional then? Means render node accessible by user and driver name returned by libdrm results in an EGLdevice ready to use. best Mathias > > > > -Emil > > > > > > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl: Don't add hardware device if there is no render node.
Hi Emil, On Friday, 7 June 2019 15:43:48 CEST Emil Velikov wrote: > On Thu, 6 Jun 2019 at 12:10, wrote: > > > > From: Mathias Fröhlich > > > > Do not offer a hardware drm backed egl device if no render node > > is available. > As far as I can see current implementation does _not_ add the DRM > device if its missing render node (and a primary one). > Looking at the change below, it's effectively making the primary node > optional. > > Hence the comment does not alight with the code - old and new. Can you > elaborate? The currently pushed implementation asks drmGetDevices2 for a list of devices and all devices with either a render node or a master node or both are added as hardware device. This check is the bitmasking test with device->available_nodes that you have put near the top of _eglAddDRMDevice. So, if there is no render node the hardware device is added. Later on the filename of the render node as returned from _eglGetDRMDeviceRenderNode is opened which does not succeed in that case. egInitialize fails then which is not nice. Past my change, a pure hardware device is not added if there is no render node. That is decided by this above mentioned bitmask test. The codepath that adds devices via _eglAddDevice from all the platforms is untouched as this still uses the same bitmask as before. I did not check this code path specifically above the call to _eglAddDevice as this patch does not change the behavior of this case. best Mathias > > I have not thought exactly how primary node-less DRM will work out > esp. since the EGL_EXT_device_drm extension explicitly mentions one. > > -Emil > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Crash in iris_bufmgr.c
Kenneth, > Ouch. Thanks for letting me know. Fixed by: > > commit 53878f7a8989879b0f3ca37df9fd1fb37f2525ca > Author: Kenneth Graunke > Date: Wed May 29 23:20:31 2019 -0700 > > iris: Be lazy about cleaning up purged BOs in the cache. > Thanks for fixing! Works again! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Crash in iris_bufmgr.c
Hi Kenneth, since your recent changes, I get a zero pointer dereference in alloc_bo_from_cache on one workload here: What I get is Thread 2 "" received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fffef776700 (LWP 20924)] list_del (item=0x7fffe8ae4020) at ../src/util/list.h:91 91 item->prev->next = item->next; (gdb) p item->prev $1 = (struct list_head *) 0x0 (gdb) where #0 list_del (item=0x7fffe8ae4020) at ../src/util/list.h:91 #1 0x7469072f in alloc_bo_from_cache (bufmgr=0x4d8370, bucket=0x4d8418, memzone=IRIS_MEMZONE_OTHER, flags=0, match_zone=true) at ../src/gallium/drivers/iris/iris_bufmgr.c:387 [...] I think the reason is that you use list_for_each_entry_safe in alloc_bo_from_cache in the outer loop and store a pointer to the next list entry. But when you call into iris_bo_cache_purge_bucket from inside that loop the function body there may walk the same list node than __next from list_for_each_entry_safe in alloc_bo_from_cache points to and may zero the list pointers out. You may know best how to fix that. best and thanks Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa: Prevent classic swrast crash on a surfaceless context v2.
Pushed that.
Thanks!
best
Mathias
On Monday, 27 May 2019 17:34:29 CEST Marek Olšák wrote:
> Reviewed-by: Marek Olšák
>
> M.
>
> On Mon, May 27, 2019, 4:17 AM wrote:
>
> > From: Mathias Fröhlich
> >
> > Hi Emil,
> >
> > thanks for that hint to look at _mesa_get_incomplete_framebuffer.
> > That one seems definitely more appropriate!
> >
> > Though, I miss a bit the idea how I can create either a sensible
> > helper function for that task or how I can create something above
> > in the call stack to the MakeCurrent call that already catches
> > this case. Since that incomplete framebuffer is a mesa side thing I
> > cannot easily pull that above the __DriverAPIRec::MakeCurrent call.
> > But really putting those hand full lines of code into a helper does
> > as well not gain much. So I implemented that for the swrast case
> > directly.
> >
> > So, to mention, I sent that change including our egl device code
> > with some unrelated egl device fixes through intels CI and did not
> > get regressions.
> >
> > please review
> >
> > thanks and best
> > Mathias
> >
> >
> >
> >
> >
> > This fixes the egl_mesa_platform_surfaceless piglit test as well
> > as the new device egl extensions piglit test on classic swrast.
> >
> > v2: Fix swrast surfaceless contexts on the driver side.
> >
> > Signed-off-by: Mathias Fröhlich
> > ---
> > src/mesa/drivers/dri/swrast/swrast.c | 9 -
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/drivers/dri/swrast/swrast.c
> > b/src/mesa/drivers/dri/swrast/swrast.c
> > index 36cf11334cb..4899fb2df95 100644
> > --- a/src/mesa/drivers/dri/swrast/swrast.c
> > +++ b/src/mesa/drivers/dri/swrast/swrast.c
> > @@ -36,6 +36,7 @@
> > #include "main/api_exec.h"
> > #include "main/context.h"
> > #include "main/extensions.h"
> > +#include "main/fbobject.h"
> > #include "main/formats.h"
> > #include "main/framebuffer.h"
> > #include "main/imports.h"
> > @@ -686,7 +687,7 @@ swrast_check_and_update_window_size( struct gl_context
> > *ctx, struct gl_framebuff
> > {
> > GLsizei width, height;
> >
> > -if (!fb)
> > +if (!fb || fb == _mesa_get_incomplete_framebuffer())
> > return;
> >
> > get_window_size(fb, , );
> > @@ -884,6 +885,12 @@ dri_make_current(__DRIcontext * cPriv,
> > mesaDraw = >Base;
> > mesaRead = >Base;
> > }
> > +else {
> > + struct gl_framebuffer *incomplete
> > + = _mesa_get_incomplete_framebuffer();
> > + mesaDraw = incomplete;
> > + mesaRead = incomplete;
> > +}
> >
> > /* check for same context and buffer */
> > if (mesaCtx == _mesa_get_current_context()
> > --
> > 2.21.0
> >
> >
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] Change a few frequented uses of DEBUG to !NDEBUG
Marek,
sure, for that part:
Reviewed-by: Mathias Fröhlich
Well, there are really magnitudes of DEBUG defines that are inactive since
meson.
Either let meson builds also define DEBUG or lets
sed -i 's,#ifdef DEBUG,#ifndef NDEBUG,g'
them now?
best
Mathias
On Friday, 10 May 2019 07:21:15 CEST Marek Olšák wrote:
> From: Marek Olšák
>
> debugoptimized builds don't define NDEBUG, but they also don't define
> DEBUG. We want to enable cheap debug code for these builds.
> I only chose those occurences that I care about.
> ---
> src/gallium/auxiliary/tgsi/tgsi_ureg.c | 2 +-
> src/gallium/drivers/radeonsi/si_descriptors.c | 2 +-
> src/gallium/drivers/radeonsi/si_pipe.h | 2 +-
> src/gallium/drivers/radeonsi/si_shader_tgsi_setup.c | 6 +++---
> src/gallium/drivers/radeonsi/si_state.c | 4 ++--
> src/mesa/main/context.c | 2 +-
> src/mesa/main/debug.c | 4 ++--
> src/mesa/main/errors.c | 6 +++---
> src/mesa/main/feedback.c| 2 +-
> src/mesa/main/formats.c | 2 --
> src/mesa/main/imports.c | 4 ++--
> src/mesa/main/mtypes.h | 2 +-
> src/mesa/main/shaderapi.c | 2 +-
> src/mesa/state_tracker/st_atom_framebuffer.c| 2 +-
> src/mesa/state_tracker/st_format.c | 2 +-
> src/mesa/vbo/vbo_exec.h | 2 +-
> src/mesa/vbo/vbo_exec_api.c | 6 +++---
> src/util/slab.c | 4 ++--
> 18 files changed, 27 insertions(+), 29 deletions(-)
>
> diff --git a/src/gallium/auxiliary/tgsi/tgsi_ureg.c
> b/src/gallium/auxiliary/tgsi/tgsi_ureg.c
> index c1c8851486e..55ed7fc62b4 100644
> --- a/src/gallium/auxiliary/tgsi/tgsi_ureg.c
> +++ b/src/gallium/auxiliary/tgsi/tgsi_ureg.c
> @@ -1234,21 +1234,21 @@ ureg_emit_dst( struct ureg_program *ureg,
> }
>
> assert(n == size);
> }
>
>
> static void validate( enum tgsi_opcode opcode,
>unsigned nr_dst,
>unsigned nr_src )
> {
> -#ifdef DEBUG
> +#ifndef NDEBUG
> const struct tgsi_opcode_info *info = tgsi_get_opcode_info( opcode );
> assert(info);
> if (info) {
>assert(nr_dst == info->num_dst);
>assert(nr_src == info->num_src);
> }
> #endif
> }
>
> struct ureg_emit_insn_result
> diff --git a/src/gallium/drivers/radeonsi/si_descriptors.c
> b/src/gallium/drivers/radeonsi/si_descriptors.c
> index 6a4dcacc0f3..68b5430446e 100644
> --- a/src/gallium/drivers/radeonsi/si_descriptors.c
> +++ b/src/gallium/drivers/radeonsi/si_descriptors.c
> @@ -953,21 +953,21 @@ static void si_bind_sampler_states(struct pipe_context
> *ctx,
> return;
>
> for (i = 0; i < count; i++) {
> unsigned slot = start + i;
> unsigned desc_slot = si_get_sampler_slot(slot);
>
> if (!sstates[i] ||
> sstates[i] == samplers->sampler_states[slot])
> continue;
>
> -#ifdef DEBUG
> +#ifndef NDEBUG
> assert(sstates[i]->magic == SI_SAMPLER_STATE_MAGIC);
> #endif
> samplers->sampler_states[slot] = sstates[i];
>
> /* If FMASK is bound, don't overwrite it.
>* The sampler state will be set after FMASK is unbound.
>*/
> struct si_sampler_view *sview =
> (struct si_sampler_view *)samplers->views[slot];
>
> diff --git a/src/gallium/drivers/radeonsi/si_pipe.h
> b/src/gallium/drivers/radeonsi/si_pipe.h
> index 4061bf139fb..2f1acec8ef2 100644
> --- a/src/gallium/drivers/radeonsi/si_pipe.h
> +++ b/src/gallium/drivers/radeonsi/si_pipe.h
> @@ -596,21 +596,21 @@ struct si_sampler_view {
> ubyte base_level;
> ubyte block_width;
> bool is_stencil_sampler;
> bool is_integer;
> bool dcc_incompatible;
> };
>
> #define SI_SAMPLER_STATE_MAGIC 0x34f1c35a
>
> struct si_sampler_state {
> -#ifdef DEBUG
> +#ifndef NDEBUG
> unsignedmagic;
> #endif
> uint32_tval[4];
> uint32_tinteger_val[4];
> uint32_tupgraded_depth_val[4];
> };
>
> struct si_cs_shader_state {
> struct si_compute *program;
> struct si_compute *emitted_program;
> diff --git a/src/ga
Re: [Mesa-dev] [PATCH 6/8] egl/dri: flesh out and use dri2_create_drawable()
Hi,
I did not have time to really look into the series, but a quick retest:
wflinfo --platform=gbm -a gl
still crashes on my site.
Can you recheck that?
best
Mathias
On Thursday, 16 May 2019 19:01:38 CEST Emil Velikov wrote:
> From: Emil Velikov
>
> Wrap the loader->createNewDrawable() dance into a helper and use it
> throughout the codebase.
>
> This addresses a cases like surfaceless (SL) on swrast (SL on kms_swrast
> is fine) where we'd attempt using the wrong driver and crash out.
>
> v2: fixup quirky GBM (Mathias)
>
> Cc: mesa-sta...@lists.freedesktop.org
> Cc: Mathias Fröhlich
> Reviewed-by: Mathias Fröhlich (v1)
> Reviewed-by: Marek Olšák (v1)
> Signed-off-by: Emil Velikov
> ---
> src/egl/drivers/dri2/egl_dri2.c | 39 +
> src/egl/drivers/dri2/egl_dri2.h | 5 +++
> src/egl/drivers/dri2/platform_android.c | 12 +--
> src/egl/drivers/dri2/platform_drm.c | 17 +
> src/egl/drivers/dri2/platform_surfaceless.c | 7 +---
> src/egl/drivers/dri2/platform_wayland.c | 14 +---
> src/egl/drivers/dri2/platform_x11.c | 15 +---
> 7 files changed, 49 insertions(+), 60 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 09d6315b19e..17a0176b646 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1425,6 +1425,45 @@ dri2_surf_update_fence_fd(_EGLContext *ctx,
> dri2_surface_set_out_fence_fd(surf, fence_fd);
> }
>
> +static inline _EGLDisplay *
> +dri2_display_to_egl_display(struct dri2_egl_display *dri2_dpy)
> +{
> + const size_t offset = offsetof(_EGLDisplay, DriverData);
> + return (_EGLDisplay *)(((void*) dri2_dpy) - offset);
> +}
> +
> +EGLBoolean
> +dri2_create_drawable(struct dri2_egl_display *dri2_dpy,
> + const __DRIconfig *config,
> + struct dri2_egl_surface *dri2_surf)
> +{
> + __DRIcreateNewDrawableFunc createNewDrawable;
> + _EGLDisplay *disp = dri2_display_to_egl_display(dri2_dpy);
> + void *loaderPrivate = dri2_surf;
> +
> + if (dri2_dpy->image_driver)
> + createNewDrawable = dri2_dpy->image_driver->createNewDrawable;
> + else if (dri2_dpy->dri2)
> + createNewDrawable = dri2_dpy->dri2->createNewDrawable;
> + else if (dri2_dpy->swrast)
> + createNewDrawable = dri2_dpy->swrast->createNewDrawable;
> + else
> + return _eglError(EGL_BAD_ALLOC, "no createNewDrawable");
> +
> + /* As always gbm is a bit special.. */
> +#ifdef HAVE_DRM_PLATFORM
> + if (disp->Platform == _EGL_PLATFORM_DRM)
> + loaderPrivate = dri2_surf->gbm_surf;
> +#endif
> +
> + dri2_surf->dri_drawable = (*createNewDrawable)(dri2_dpy->dri_screen,
> + config, loaderPrivate);
> + if (dri2_surf->dri_drawable == NULL)
> + return _eglError(EGL_BAD_ALLOC, "createNewDrawable");
> +
> + return EGL_TRUE;
> +}
> +
> /**
> * Called via eglMakeCurrent(), drv->API.MakeCurrent().
> */
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index 4918517b572..bcea3ab 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -541,6 +541,11 @@ dri2_init_surface(_EGLSurface *surf, _EGLDisplay *disp,
> EGLint type,
> void
> dri2_fini_surface(_EGLSurface *surf);
>
> +EGLBoolean
> +dri2_create_drawable(struct dri2_egl_display *dri2_dpy,
> + const __DRIconfig *config,
> + struct dri2_egl_surface *dri2_surf);
> +
> static inline uint64_t
> combine_u32_into_u64(uint32_t hi, uint32_t lo)
> {
> diff --git a/src/egl/drivers/dri2/platform_android.c
> b/src/egl/drivers/dri2/platform_android.c
> index fece214d939..e7c7df7 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -341,7 +341,6 @@ droid_create_surface(_EGLDriver *drv, _EGLDisplay *disp,
> EGLint type,
> _EGLConfig *conf, void *native_window,
> const EGLint *attrib_list)
> {
> - __DRIcreateNewDrawableFunc createNewDrawable;
> struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> struct dri2_egl_config *dri2_conf = dri2_egl_config(conf);
> struct dri2_egl_surface *dri2_surf;
> @@ -386,17 +385,8 @@ droid_create_surface(_EGLDriver *drv, _EGLDisplay *disp,
> EGLint type,
>goto cleanup_surface;
> }
>
> - if (dri2_dpy->image_driver)
> - createNewDrawable = dri2_dpy->image_driver->createNewDrawable;
> -
Re: [Mesa-dev] [PATCH 6/6] mesa: Set _NEW_VARYING_VP_INPUTS iff varying_vp_inputs are set.
Hi Marek,
> Reviewed-by: Marek Olšák
Thanks for the review!
Pushed!
best
Mathias
>
> Marek
>
> On Sun, May 12, 2019 at 9:05 AM wrote:
>
> > From: Mathias Fröhlich
> >
> > Signed-off-by: Mathias Fröhlich
> > ---
> > src/mesa/main/state.c | 13 ++---
> > 1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/mesa/main/state.c b/src/mesa/main/state.c
> > index 9d8964952cd..3e2eb28dcc5 100644
> > --- a/src/mesa/main/state.c
> > +++ b/src/mesa/main/state.c
> > @@ -430,15 +430,14 @@ set_varying_vp_inputs(struct gl_context *ctx,
> > GLbitfield varying_inputs)
> > if (VP_MODE_FF != ctx->VertexProgram._VPMode)
> >return;
> >
> > + /* Only fixed-func generated programs ever uses varying_vp_inputs. */
> > + if (!ctx->VertexProgram._MaintainTnlProgram &&
> > + !ctx->FragmentProgram._MaintainTexEnvProgram)
> > + return;
> > +
> > if (ctx->varying_vp_inputs != varying_inputs) {
> >ctx->varying_vp_inputs = varying_inputs;
> > -
> > - /* Only fixed-func generated programs ever use varying_vp_inputs. */
> > - if (ctx->VertexProgram._MaintainTnlProgram ||
> > - ctx->FragmentProgram._MaintainTexEnvProgram) {
> > - ctx->NewState |= _NEW_VARYING_VP_INPUTS;
> > - }
> > - /*printf("%s %x\n", __func__, varying_inputs);*/
> > + ctx->NewState |= _NEW_VARYING_VP_INPUTS;
> > }
> > }
> >
> > --
> > 2.21.0
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 7/9] egl: keep the software device at the end of the list
Hi Emil,
The patches 1-7 are as well:
Reviewed-by: Mathias Fröhlich
plenty thanks and best
Mathias
On Monday, 6 May 2019 17:01:30 CEST Emil Velikov wrote:
> From: Emil Velikov
>
> By default, the user is likely to pick the first device so it should
> not be the least performant (aka software) one.
>
> Suggested-by: Marek Olšák
> Signed-off-by: Emil Velikov
> ---
> src/egl/main/egldevice.c | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/src/egl/main/egldevice.c b/src/egl/main/egldevice.c
> index c5c9a21273a..328d9ea08c5 100644
> --- a/src/egl/main/egldevice.c
> +++ b/src/egl/main/egldevice.c
> @@ -293,13 +293,26 @@ _eglQueryDevicesEXT(EGLint max_devices,
>goto out;
> }
>
> + /* Push the first device (the software one) to the end of the list.
> +* Sending it to the user only if they've requested the full list.
> +*
> +* By default, the user is likely to pick the first device so having the
> +* software (aka least performant) one is not a good idea.
> +*/
> *num_devices = MIN2(num_devs, max_devices);
>
> - for (i = 0, dev = devs; i < *num_devices; i++) {
> + for (i = 0, dev = devs->Next; dev && i < max_devices; i++) {
>devices[i] = dev;
>dev = dev->Next;
> }
>
> + /* User requested the full device list, add the sofware device. */
> + if (max_devices >= num_devs) {
> + /* The first device is always software */
> + assert(_eglDeviceSupports(devs, _EGL_DEVICE_SOFTWARE));
> + devices[num_devs - 1] = devs;
> + }
> +
> out:
> mtx_unlock(_eglGlobal.Mutex);
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/10] mesa: Implement _mesa_array_element by walking enabled arrays.
Hi Brian, On Friday, 3 May 2019 14:40:26 CEST Brian Paul wrote: > All your suggested changes look good. > > Reviewed-by: Brian Paul > > Thanks. Pushed Thanks! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl: add EGL_platform_device support
Good Morning, On Wednesday, 1 May 2019 21:43:08 CEST Marek Olšák wrote: > BTW, swrast doesn't have to exist on the system. It's not uncommon for me > to have no swrast on my development system. Ok. I see. I use swrast regularly to test changes with different backend drivers. Also especially classic swrast as something that is close to the good old swtnl drivers - to catch bad interactions with those. Anyhow, with a very old swrast I think you will get test failures. But else if the system swrast is found in the hopefully not so distant future the tests should even pass - well depends on what Emil now does to get a better overall swrast behavior. On a production system with a full set of driver packages I do expect to find swrast, right? At least on a workstation grade linux distribution. I start to see the actual problem for AMD there. Not your test system at home, but the pro driver that needs to ship and QA swrast then. Anyhow, I do not actually understand the way how we walk all installed egl driver implementations - including closed drivers - finally and present all those devices. In a perfect world *for the customer* I could enumerate all devices - including oss i965 and the closed nvidia bumblebee device - on my laptop for example. Means - if that works fine AMD could hook into that mechanism and provide further devices. Well - in the long term. Thinking ... best Mathias > > Marek > > On Wed, May 1, 2019 at 4:30 AM Mathias Fröhlich > wrote: > > > Hi Marek, Emil, > > > > On Tuesday, 30 April 2019 15:36:08 CEST Emil Velikov wrote: > > > On Mon, 29 Apr 2019 at 22:50, Marek Olšák wrote: > > > > > > > > On Mon, Apr 29, 2019 at 4:00 AM Pekka Paalanen > > wrote: > > > >> > > > >> On Sat, 27 Apr 2019 09:38:27 -0400 > > > >> Marek Olšák wrote: > > > >> > > > >> > Those are all valid reasons, but I don't wanna expose swrast for > > AMD's > > > >> > customers. > > > >> > > > >> Hi Marek, > > > >> > > > >> is you objection that you will never want to see any software renderer > > > >> in the list, or that you don't want to see a software renderer only as > > > >> long as it doesn't actually work? > > > >> > > > >> Why do you not "wanna expose swrast for AMD's customers"? > > > >> > > > >> Are you talking about swrast specifically or all the software > > renderers > > > >> in Mesa? > > > >> > > > >> I'm not an AMD customer if you mean someone paying for support rather > > > >> than just buying their hardware, so I cannot speak for them. However, > > I > > > >> would be very happy to have a software renderer available to be picked > > > >> the same way as any other hardware renderer, so that I can use it in > > > >> graphical test suites (a point from Anholt) testing also the EGL glue > > > >> in addition to the pixels produced. > > > >> > > > >> The thing will be run on boxes with AMD GPUs, and in those cases there > > > >> must be a way to *not* use the AMD GPU, and use the software renderer > > > >> instead when wanted. Not by environment variables or anything hacky > > > >> like that, but by deliberately choosing the software renderer in the > > > >> program. It will need an EGLSurface too. > > > >> > > > >> How would this be made to work in the future? > > > > > > > > > > > > A software renderer could be exposed by adding a new EGL function on > > top of EGL_EXT_device_base, for example: > > > > > > > > // EGL_MESA_device_software > > > > > > > > EGLBoolean eglQuerySoftwareDeviceMESA(EGLDeviceEXT *swdevice); > > > > > > > > > > > > You would get the swrast device from that function instead of > > eglQueryDevicesEXT. It clearly separates hw and sw devices but keeps > > everything else the same. > > > > > > > IMHO the current EGL_MESA_device_software approach is perfectly valid. > > > The Query API provides the devices and one can query their > > > capabilities/device extensions. > > > > I agree with that. Well to repeat myself. > > > > For me there is also a basic consistency argument behind. > > So, Marek, you propose that the swrast device should only show > > up when there is no other device. That means swrast qualifies in > > principle as a 'device'. > > But if the
Re: [Mesa-dev] [PATCH 03/10] mesa: Implement _mesa_array_element by walking enabled arrays.
Hi Brian,
On Friday, 3 May 2019 00:17:51 CEST Brian Paul wrote:
> On 05/02/2019 03:27 AM, mathias.froehl...@gmx.net wrote:
> > From: Mathias Fröhlich
> >
> > In glArrayElement, use the bitmask trick to just walk the enabled
> > vao arrays. This should be about equivalent in execution time to
> > walk the prepare aelt_context list. Finally this will allow us to
> > reduce the _mesa_update_state calls in a few patches.
> >
> > Signed-off-by: Mathias Fröhlich
> > ---
> > src/mesa/main/api_arrayelt.c | 78
> > 1 file changed, 61 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/mesa/main/api_arrayelt.c b/src/mesa/main/api_arrayelt.c
> > index d46c8d14b68..62f1e73ca4c 100644
> > --- a/src/mesa/main/api_arrayelt.c
> > +++ b/src/mesa/main/api_arrayelt.c
> > @@ -1541,32 +1541,76 @@ _ae_update_state(struct gl_context *ctx)
> > }
> >
> >
> > +static inline attrib_func
> > +func_nv(const struct gl_vertex_format *vformat)
> > +{
> > + return AttribFuncsNV[vformat->Normalized][vformat->Size-1]
> > + [TYPE_IDX(vformat->Type)];
> > +}
> > +
> > +
> > +static inline attrib_func
> > +func_arb(const struct gl_vertex_format *vformat)
> > +{
> > + return AttribFuncsARB[NORM_IDX(vformat)][vformat->Size-1]
> > + [TYPE_IDX(vformat->Type)];
> > +}
> > +
> > +
> > +static inline const void *
> > +attrib_src(const struct gl_vertex_array_object *vao,
> > + const struct gl_array_attributes *array, GLint elt)
> > +{
> > + const struct gl_vertex_buffer_binding *binding =
> > + >BufferBinding[array->BufferBindingIndex];
> > + const GLubyte *src
> > + = ADD_POINTERS(binding->BufferObj->Mappings[MAP_INTERNAL].Pointer,
> > + _mesa_vertex_attrib_address(array, binding))
> > + + elt * binding->Stride;
> > + return src;
> > +}
>
> Could you add some brief comments on those functions to explain what
> they do?
Added brief comments:
/*
* Return VertexAttrib*NV function pointer matching the provided vertex format.
*/
static inline attrib_func
func_nv(const struct gl_vertex_format *vformat)
[...]
/*
* Return VertexAttrib*ARB function pointer matching the provided vertex format.
*/
static inline attrib_func
func_arb(const struct gl_vertex_format *vformat)
[...]
/*
* Return the address of the array attribute array at elt in the
* vertex array object vao.
*/
static inline const void *
attrib_src(const struct gl_vertex_array_object *vao,
const struct gl_array_attributes *array, GLint elt)
> Otherwise, for the rest of the series,
> Reviewed-by: Brian Paul
>
> Nice work!!
Thanks for looking at the patches!
best
Mathias
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 02/10] mesa: Use glVertexAttrib*NV functions for fixed function attribs.
On Friday, 3 May 2019 00:17:45 CEST Brian Paul wrote:
> On 05/02/2019 03:27 AM, mathias.froehl...@gmx.net wrote:
> > From: Mathias Fröhlich
> >
> > In the glArrayElement implementation, use glVertexAttrib*NV type
> > functions for fixed function attributes. We do the same in display
> > execution when the list is replayed using immediate mode attribute
> > functions. By that use a loop to walk the attributes.
>
> I'm not sure I understand that last sentence.
So, how about:
mesa: Use glVertexAttrib*NV functions for fixed function attribs.
In the glArrayElement implementation, use glVertexAttrib*NV type
functions for fixed function attributes. We do the same in display
execution when the list is replayed using immediate mode attribute
functions. Using a single set of function pointers enables to
use a unified loop to walk the vertex array attributes.
best and Thanks!
Mathias
>
> The code looks fine.
>
> Reviewed-by: Brian Paul
>
>
> >
> > Signed-off-by: Mathias Fröhlich
> > ---
> > src/mesa/main/api_arrayelt.c | 185 ++-
> > 1 file changed, 28 insertions(+), 157 deletions(-)
> >
> > diff --git a/src/mesa/main/api_arrayelt.c b/src/mesa/main/api_arrayelt.c
> > index 3f16e18b44d..d46c8d14b68 100644
> > --- a/src/mesa/main/api_arrayelt.c
> > +++ b/src/mesa/main/api_arrayelt.c
> > @@ -117,89 +117,6 @@ _ae_is_state_dirty(struct gl_context *ctx)
> > #define NUM_TYPES 8
> >
> >
> > -static const int ColorFuncs[2][NUM_TYPES] = {
> > - {
> > - _gloffset_Color3bv,
> > - _gloffset_Color3ubv,
> > - _gloffset_Color3sv,
> > - _gloffset_Color3usv,
> > - _gloffset_Color3iv,
> > - _gloffset_Color3uiv,
> > - _gloffset_Color3fv,
> > - _gloffset_Color3dv,
> > - },
> > - {
> > - _gloffset_Color4bv,
> > - _gloffset_Color4ubv,
> > - _gloffset_Color4sv,
> > - _gloffset_Color4usv,
> > - _gloffset_Color4iv,
> > - _gloffset_Color4uiv,
> > - _gloffset_Color4fv,
> > - _gloffset_Color4dv,
> > - },
> > -};
> > -
> > -static const int VertexFuncs[3][NUM_TYPES] = {
> > - {
> > - -1,
> > - -1,
> > - _gloffset_Vertex2sv,
> > - -1,
> > - _gloffset_Vertex2iv,
> > - -1,
> > - _gloffset_Vertex2fv,
> > - _gloffset_Vertex2dv,
> > - },
> > - {
> > - -1,
> > - -1,
> > - _gloffset_Vertex3sv,
> > - -1,
> > - _gloffset_Vertex3iv,
> > - -1,
> > - _gloffset_Vertex3fv,
> > - _gloffset_Vertex3dv,
> > - },
> > - {
> > - -1,
> > - -1,
> > - _gloffset_Vertex4sv,
> > - -1,
> > - _gloffset_Vertex4iv,
> > - -1,
> > - _gloffset_Vertex4fv,
> > - _gloffset_Vertex4dv,
> > - },
> > -};
> > -
> > -static const int IndexFuncs[NUM_TYPES] = {
> > - -1,
> > - _gloffset_Indexubv,
> > - _gloffset_Indexsv,
> > - -1,
> > - _gloffset_Indexiv,
> > - -1,
> > - _gloffset_Indexfv,
> > - _gloffset_Indexdv,
> > -};
> > -
> > -static const int NormalFuncs[NUM_TYPES] = {
> > - _gloffset_Normal3bv,
> > - -1,
> > - _gloffset_Normal3sv,
> > - -1,
> > - _gloffset_Normal3iv,
> > - -1,
> > - _gloffset_Normal3fv,
> > - _gloffset_Normal3dv,
> > -};
> > -
> > -/* Note: _gloffset_* for these may not be a compile-time constant. */
> > -static int SecondaryColorFuncs[NUM_TYPES];
> > -static int FogCoordFuncs[NUM_TYPES];
> > -
> > -
> > /**
> >** GL_NV_vertex_program
> >**/
> > @@ -1508,25 +1425,6 @@ _ae_create_context(struct gl_context *ctx)
> > if (ctx->aelt_context)
> > return GL_TRUE;
> >
> > - /* These _gloffset_* values may not be compile-time constants */
> > - SecondaryColorFuncs[0] = _gloffset_SecondaryColor3bv;
> > - SecondaryColorFuncs[1] = _gloffset_SecondaryColor3ubv;
> > - SecondaryColorFuncs[2] = _gloffset_SecondaryColor3sv;
> > - SecondaryColorFuncs[3] = _gloffset_SecondaryColor3usv;
> > - SecondaryColorFuncs[4] = _gloffset_SecondaryColor3iv;
> > - SecondaryColorFuncs[5] = _gloffset_SecondaryColor3uiv;
> > - SecondaryColorFuncs[6] = _gloffset_SecondaryColor3fvEXT;
> > - SecondaryColorFuncs[7] = _gloffset_SecondaryColor3dv;
> > -
> > - FogCoordFuncs[0] = -1;
> > - FogCoordFuncs[1] = -
Re: [Mesa-dev] [PATCH 01/10] mesa: Factor out index function that will have multiple use.
Good Morning,
On Friday, 3 May 2019 00:17:38 CEST Brian Paul wrote:
> On 05/02/2019 03:27 AM, mathias.froehl...@gmx.net wrote:
> > From: Mathias Fröhlich
> >
> > For access to glArrayElement methods factor out a function to
> > get the table lookup index for normalized/integer/double access.
> > The function will be used in the next patch at least twice.
> >
> > Signed-off-by: Mathias Fröhlich
> > ---
> > src/mesa/main/api_arrayelt.c | 29 ++---
> > 1 file changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/mesa/main/api_arrayelt.c b/src/mesa/main/api_arrayelt.c
> > index 1c086bbcda4..3f16e18b44d 100644
> > --- a/src/mesa/main/api_arrayelt.c
> > +++ b/src/mesa/main/api_arrayelt.c
> > @@ -90,6 +90,23 @@ TYPE_IDX(GLenum t)
> > }
> >
> >
> > +/*
> > + * Convert normalized/integer/double to the range [0, 3].
> > + */
> > +static inline int
> > +NORM_IDX(const struct gl_vertex_format *vformat)
>
> Maybe we could find a better name. How about vertex_format_to_index()?
Sure! Changed that to your suggestion!
Thanks!
best
Mathias
>
> -Brian
>
>
> > +{
> > + if (vformat->Doubles)
> > + return 3;
> > + else if (vformat->Integer)
> > + return 2;
> > + else if (vformat->Normalized)
> > + return 1;
> > + else
> > + return 0;
> > +}
> > +
> > +
> > bool
> > _ae_is_state_dirty(struct gl_context *ctx)
> > {
> > @@ -1610,7 +1627,6 @@ _ae_update_state(struct gl_context *ctx)
> > if (vao->Enabled & VERT_BIT_GENERIC(i)) {
> >struct gl_array_attributes *attribArray =
> > >VertexAttrib[VERT_ATTRIB_GENERIC(i)];
> > - GLint intOrNorm;
> >at->array = attribArray;
> >at->binding =
> > >BufferBinding[attribArray->BufferBindingIndex];
> >/* Note: we can't grab the _glapi_Dispatch->VertexAttrib1fvNV
> > @@ -1618,16 +1634,7 @@ _ae_update_state(struct gl_context *ctx)
> > * change from one execution of _ae_ArrayElement() to
> > * the next. Doing so caused UT to break.
> > */
> > - if (at->array->Format.Doubles)
> > -intOrNorm = 3;
> > - else if (at->array->Format.Integer)
> > -intOrNorm = 2;
> > - else if (at->array->Format.Normalized)
> > -intOrNorm = 1;
> > - else
> > -intOrNorm = 0;
> > -
> > - at->func = AttribFuncsARB[intOrNorm]
> > + at->func = AttribFuncsARB[NORM_IDX(>array->Format)]
> > [at->array->Format.Size-1]
> > [TYPE_IDX(at->array->Format.Type)];
> >
> > --
> > 2.20.1
> >
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] mesa: rework error handling in glDrawBuffers
Hi Marek,
On Tuesday, 30 April 2019 01:10:42 CEST Marek Olšák wrote:
> I'm adding this hunk, which makes the test pass:
>
> diff --git a/src/mesa/main/buffers.c b/src/mesa/main/buffers.c
> index 1ac0d5d0798..a46599a2872 100644
> --- a/src/mesa/main/buffers.c
> +++ b/src/mesa/main/buffers.c
> @@ -485,7 +485,9 @@ draw_buffers(struct gl_context *ctx, struct
> gl_framebuffer *fb, GLsizei n,
> } else if (buffers[output] == GL_FRONT ||
> buffers[output] == GL_LEFT ||
> buffers[output] == GL_RIGHT ||
> -buffers[output] == GL_FRONT_AND_BACK) {
> +buffers[output] == GL_FRONT_AND_BACK ||
> +(buffers[output] == GL_BACK &&
> + _mesa_is_desktop_gl(ctx))) {
> _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
> caller, _mesa_enum_to_string(buffers[output]));
> return;
If we finally go that route by this series then this patch looks good to me.
Lets see if we need a better overall fix for pbuffers.
best
Mathias
>
> Marek
>
> On Sat, Apr 27, 2019 at 10:23 AM Mathias Fröhlich
> wrote:
>
> > Hi Marek,
> >
> > one comment/failure inline:
> >
> > On Saturday, 27 April 2019 05:31:45 CEST Marek Olšák wrote:
> > > From: Marek Olšák
> > >
> > > It's needed by the next pbuffer fix, which changes the behavior of
> > > draw_buffer_enum_to_bitmask, so it can't be used to help with error
> > > checking.
> > > ---
> > > src/mesa/main/buffers.c | 53 ++---
> > > 1 file changed, 29 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/src/mesa/main/buffers.c b/src/mesa/main/buffers.c
> > > index d98c015bb24..2148fa1316c 100644
> > > --- a/src/mesa/main/buffers.c
> > > +++ b/src/mesa/main/buffers.c
> > > @@ -430,65 +430,70 @@ draw_buffers(struct gl_context *ctx, struct
> > gl_framebuffer *fb, GLsizei n,
> > > _mesa_error(ctx, GL_INVALID_OPERATION, "%s(invalid buffers)",
> > caller);
> > > return;
> > >}
> > > }
> > >
> > > supportedMask = supported_buffer_bitmask(ctx, fb);
> > > usedBufferMask = 0x0;
> > >
> > > /* complicated error checking... */
> > > for (output = 0; output < n; output++) {
> > > - destMask[output] = draw_buffer_enum_to_bitmask(ctx,
> > buffers[output]);
> > > -
> > >if (!no_error) {
> > > - /* From the OpenGL 3.0 specification, page 258:
> > > - * "Each buffer listed in bufs must be one of the values from
> > tables
> > > - * 4.5 or 4.6. Otherwise, an INVALID_ENUM error is generated.
> > > - */
> > > - if (destMask[output] == BAD_MASK) {
> > > -_mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
> > > -caller, _mesa_enum_to_string(buffers[output]));
> > > -return;
> > > - }
> > > -
> > > /* From the OpenGL 4.5 specification, page 493 (page 515 of
> > the PDF)
> > >* "An INVALID_ENUM error is generated if any value in bufs is
> > FRONT,
> > >* LEFT, RIGHT, or FRONT_AND_BACK . This restriction applies
> > to both
> > >* the default framebuffer and framebuffer objects, and exists
> > because
> > >* these constants may themselves refer to multiple buffers,
> > as shown
> > >* in table 17.4."
> > >*
> > > - * And on page 492 (page 514 of the PDF):
> > > + * From the OpenGL 4.5 specification, page 492 (page 514 of
> > the PDF):
> > >* "If the default framebuffer is affected, then each of the
> > constants
> > >* must be one of the values listed in table 17.6 or the
> > special value
> > >* BACK. When BACK is used, n must be 1 and color values are
> > written
> > >* into the left buffer for single-buffered contexts, or into
> > the back
> > >* left buffer for double-buffered contexts."
> > >*
> > >* Note "special value BACK". GL_BACK also refers to multiple
> > buffers,
> > >* but it is consider a special case here. This is a change on
> > 4.5.
> > >* For
Re: [Mesa-dev] [PATCH 2/2] egl: add EGL_platform_device support
Hi Marek, Emil, On Tuesday, 30 April 2019 15:36:08 CEST Emil Velikov wrote: > On Mon, 29 Apr 2019 at 22:50, Marek Olšák wrote: > > > > On Mon, Apr 29, 2019 at 4:00 AM Pekka Paalanen wrote: > >> > >> On Sat, 27 Apr 2019 09:38:27 -0400 > >> Marek Olšák wrote: > >> > >> > Those are all valid reasons, but I don't wanna expose swrast for AMD's > >> > customers. > >> > >> Hi Marek, > >> > >> is you objection that you will never want to see any software renderer > >> in the list, or that you don't want to see a software renderer only as > >> long as it doesn't actually work? > >> > >> Why do you not "wanna expose swrast for AMD's customers"? > >> > >> Are you talking about swrast specifically or all the software renderers > >> in Mesa? > >> > >> I'm not an AMD customer if you mean someone paying for support rather > >> than just buying their hardware, so I cannot speak for them. However, I > >> would be very happy to have a software renderer available to be picked > >> the same way as any other hardware renderer, so that I can use it in > >> graphical test suites (a point from Anholt) testing also the EGL glue > >> in addition to the pixels produced. > >> > >> The thing will be run on boxes with AMD GPUs, and in those cases there > >> must be a way to *not* use the AMD GPU, and use the software renderer > >> instead when wanted. Not by environment variables or anything hacky > >> like that, but by deliberately choosing the software renderer in the > >> program. It will need an EGLSurface too. > >> > >> How would this be made to work in the future? > > > > > > A software renderer could be exposed by adding a new EGL function on top of > > EGL_EXT_device_base, for example: > > > > // EGL_MESA_device_software > > > > EGLBoolean eglQuerySoftwareDeviceMESA(EGLDeviceEXT *swdevice); > > > > > > You would get the swrast device from that function instead of > > eglQueryDevicesEXT. It clearly separates hw and sw devices but keeps > > everything else the same. > > > IMHO the current EGL_MESA_device_software approach is perfectly valid. > The Query API provides the devices and one can query their > capabilities/device extensions. I agree with that. Well to repeat myself. For me there is also a basic consistency argument behind. So, Marek, you propose that the swrast device should only show up when there is no other device. That means swrast qualifies in principle as a 'device'. But if there is an other 'device' then swrast is not a 'device' anymore? I can not quite understand that from the outside. Means if swrast qualifies as a device in one configuration if should also be a device in any other configuration. There is also a next problem. I can understand that AMD wants to avoid for a customer to grab the swrast device by accident and get worse performance. But where do you draw the line then? Do you want to block out a drm device that comes up as a administration console device in such a typical compute cluster when there is an AMD device available? I mean for the same reason, where you want to avoid an application to grab the matrox device on that board that is put there for some sort of framebuffer console? If does AMD then negotiate with the Intel guys if their chip qualifies as 'device' in this sense? Do you then maintain a list of 'hardware devices' that qualify for a 'real device' then? As I said I can understand that the first device should be a GPU hardware backed device when possible to catch those simple example programs that only grab the first device. And I am still in favor of sorting this device list in a different way than it is today. Looking at that, it's probably a bit more educative to have the swrast device there in any case. That makes the average developer making use of that api aware that he has to look a bit closer at the devices before blindly using the first one. This awareness makes the administrative console device case being handled in the using application. That in turn will probably lead to the final effect that I would like to see as a GPU vendor, that is applications grab a GPU backed device and users are happy with the performance. As Emil mentions, you can find out if you grabed a swrast device or not. There is the case that your device is DRM backed, or swrast, or nothing - then its nvidia closed. > As far as I can see you have valid concerns: > - HW devices should be the first/default > - the SW device should work, if exposed > - one may want to omit the SW one all together > > At the same time: > - the device_base extension is explicit that at least one device must > be present > - manipulating the EGL client extension string, before we determine > the driver is a PITA > > How about: > - whip a quick (admittedly gross) hack that makes SW work > - flip the order so SW is always the last one in the list > - add a hack that disables SW all-together? I would vote for reordering the device list to have swrast in the last position. > > The first two
Re: [Mesa-dev] [PATCH 2/3] mesa: fix pbuffers because internally they are front buffers
Adam, Marek, On Monday, 29 April 2019 18:28:21 CEST Adam Jackson wrote: > On Fri, 2019-04-26 at 23:31 -0400, Marek Olšák wrote: > > I don't claim to know what this series is trying to fix, but: > > > +* 2) Pbuffers are back buffers from the application point of view, > > +*but they are front buffers from the Mesa point of view, > > +*because they are always single buffered. > > +*/ > > The EGL spec (back to 1.0!) says: > > "The resulting pbuffer will contain color buffers and ancillary buffers > as specified by config." > > This appears to be copied from GLX, which has something more elaborate: > > "The resulting pbuffer will contain color buffers and ancillary buffers > as specified by config. It is possible to create a pbuffer with back > buffers and to swap the front and back buffers by calling > glXSwapBuffers. Note that pbuffers use framebuffer resources so > applications should consider deallocating them when they are not in > use." > > So I'm not convinced that pbuffers are "always single-buffered". The > back buffer is definitely a color buffer, and at least under GLX it > seems like it should be possible to draw red to back, swap, draw blue > to back, glReadBuffer(GL_FRONT), and expect glReadPixels to return red. Hmm, probably Marek's escape could be 2.2.2 Rendering Models form the eglspec: [...] Pbuffer surfaces have a back buffer but no associated window, so the back buffer need not be copied. [...] Not sure myself what that means in its total consequence, but can't we read that as there is only undefined behavior associated with the pbuffer front buffer? Why, do I get to that? IIRC somewhere in EGL there is a note that there is basically no front buffer rendering in EGL. And the above tells that the implementation may behave as if there is no way to swap the back buffers content into the front buffer. Is there some path left using egl images or egl copy buffers? Or what else do I miss? best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] mesa: rework error handling in glDrawBuffers
Hi Marek,
one comment/failure inline:
On Saturday, 27 April 2019 05:31:45 CEST Marek Olšák wrote:
> From: Marek Olšák
>
> It's needed by the next pbuffer fix, which changes the behavior of
> draw_buffer_enum_to_bitmask, so it can't be used to help with error
> checking.
> ---
> src/mesa/main/buffers.c | 53 ++---
> 1 file changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/src/mesa/main/buffers.c b/src/mesa/main/buffers.c
> index d98c015bb24..2148fa1316c 100644
> --- a/src/mesa/main/buffers.c
> +++ b/src/mesa/main/buffers.c
> @@ -430,65 +430,70 @@ draw_buffers(struct gl_context *ctx, struct
> gl_framebuffer *fb, GLsizei n,
> _mesa_error(ctx, GL_INVALID_OPERATION, "%s(invalid buffers)",
> caller);
> return;
>}
> }
>
> supportedMask = supported_buffer_bitmask(ctx, fb);
> usedBufferMask = 0x0;
>
> /* complicated error checking... */
> for (output = 0; output < n; output++) {
> - destMask[output] = draw_buffer_enum_to_bitmask(ctx, buffers[output]);
> -
>if (!no_error) {
> - /* From the OpenGL 3.0 specification, page 258:
> - * "Each buffer listed in bufs must be one of the values from tables
> - * 4.5 or 4.6. Otherwise, an INVALID_ENUM error is generated.
> - */
> - if (destMask[output] == BAD_MASK) {
> -_mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
> -caller, _mesa_enum_to_string(buffers[output]));
> -return;
> - }
> -
> /* From the OpenGL 4.5 specification, page 493 (page 515 of the PDF)
>* "An INVALID_ENUM error is generated if any value in bufs is
> FRONT,
>* LEFT, RIGHT, or FRONT_AND_BACK . This restriction applies to both
>* the default framebuffer and framebuffer objects, and exists
> because
>* these constants may themselves refer to multiple buffers, as
> shown
>* in table 17.4."
>*
> - * And on page 492 (page 514 of the PDF):
> + * From the OpenGL 4.5 specification, page 492 (page 514 of the
> PDF):
>* "If the default framebuffer is affected, then each of the
> constants
>* must be one of the values listed in table 17.6 or the special
> value
>* BACK. When BACK is used, n must be 1 and color values are written
>* into the left buffer for single-buffered contexts, or into the
> back
>* left buffer for double-buffered contexts."
>*
>* Note "special value BACK". GL_BACK also refers to multiple
> buffers,
>* but it is consider a special case here. This is a change on 4.5.
>* For OpenGL 4.x we check that behaviour. For any previous version
> we
>* keep considering it wrong (as INVALID_ENUM).
>*/
> - if (util_bitcount(destMask[output]) > 1) {
> -if (_mesa_is_winsys_fbo(fb) && ctx->Version >= 40 &&
> -buffers[output] == GL_BACK) {
> - if (n != 1) {
> - _mesa_error(ctx, GL_INVALID_OPERATION, "%s(with GL_BACK n
> must be 1)",
> - caller);
> - return;
> - }
> -} else {
> - _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
> - caller, _mesa_enum_to_string(buffers[output]));
> + if (buffers[output] == GL_BACK &&
> + _mesa_is_winsys_fbo(fb) &&
> + _mesa_is_desktop_gl(ctx) &&
> + ctx->Version >= 40) {
> +if (n != 1) {
> + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(with GL_BACK n
> must be 1)",
> + caller);
> return;
> }
> + } else if (buffers[output] == GL_FRONT ||
> +buffers[output] == GL_LEFT ||
> +buffers[output] == GL_RIGHT ||
> +buffers[output] == GL_FRONT_AND_BACK) {
> +_mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
> +caller, _mesa_enum_to_string(buffers[output]));
> +return;
> + }
> + }
According to the comment above "Note "special value BACK". GL_BACK...",
there must also be a GL_INVALID_ENUM on GL_BACK when not in opengles. Right?
That specific case can be tested using
MESA_GL_VERSION_OVERRIDE=3.3 piglit/bin/gl-3.1-draw-buffers-errors
best
Mathias
> +
> + destMask[output] = draw_buffer_enum_to_bitmask(ctx, buffers[output]);
> +
> + if (!no_error) {
> + /* From the OpenGL 3.0 specification, page 258:
> + * "Each buffer listed in bufs must be one of the values from tables
> + * 4.5 or 4.6. Otherwise, an INVALID_ENUM error is generated.
> + */
> + if (destMask[output] == BAD_MASK) {
> +_mesa_error(ctx, GL_INVALID_ENUM,
Re: [Mesa-dev] [PATCH 2/2] egl: add EGL_platform_device support
Hi Marek, On Wednesday, 24 April 2019 02:01:42 CEST Marek Olšák wrote: > Adam, did you notice my original suggestion "If there is at least 1 drm > device, swrast won't be in the list."? which means swrast would be in the > list for your "dumb" GPUs. Imagine a box with a low end drm capable hardware chip like you find sometimes in server type boxes (intel/matrox...). Otherwise the box is equipped with lots of cpu power. This is something that you will find a lot in that major engineering application environment. Your application will be glad to find the swrast renderer that is finally more capable than the 'GPU' mostly there to drive an administration console. You do not want to lock a swrast 'device' (or however you want to call it) out by a may be less capable 'console GPU'. Beside that having a second type of 'normalized renderer' like Eric was telling about is an other one. As well as sometimes it may make sense to utilize the GPU with one set of work and a second GPU with an other set of work in parallel. When you only find a single gpu device in one box, you may be glad to find a swrast device that you can make use of in parallel with the gpu without the need to put up different code paths in your application. May be I can come up with other cases, but thats the 5 minutes for now ... best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl: add EGL_platform_device support
Hi, On Tuesday, 23 April 2019 22:23:45 CEST Marek Olšák wrote: > On Tue, Apr 23, 2019 at 4:05 PM Mathias Fröhlich > wrote: > > > Hi Marek, > > > > On Tuesday, 23 April 2019 20:22:15 CEST Marek Olšák wrote: > > > I'd like to remove swrast from devices. It doesn't work (eglInitialize > > > fails) and I don't think I like swrast there. Any objections? > > > > Yes, how do you guarantee that at least one device can be returned > > in any case? Even if no drm device is found? > > The egl device query extension guarantees that there is at least one > > device available. > > Beside that swrast may make sense for itself, that is the reason > > the swrast device is there. > > > > If no device can be returned, the extension won't be exposed. Yes, I was triggering this solution back in the days. There was (is?) no infrastructure to enable or disable an egl extension just past initialization or at runtime. Finally Emil took the route to add swrast. The longer I saw the swrast device the better It appeared to me. ... from the users point of view. >OR > If there is at least 1 drm device, swrast won't be in the list. > > From my perspective, radeonsi is always in the list and I wouldn't like > swrast to be in the list if radeonsi is in the list. I see - at least I think. But really the swrast device is good as is. I see plenty use cases for that. Having that only present when there is no hardware device makes it less useful. What I don't like with the current solution is that you get the swrast as the first device on the list. The usual application that I know of just uses something closely derived from the nvidia sample code. And that one just takes the first device. So I personally want to make sure that the first device is a hardware accelerated device. Putting the swrast device at the end of the list would be sufficient I think. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl: add EGL_platform_device support
Hi Marek, On Tuesday, 23 April 2019 20:22:15 CEST Marek Olšák wrote: > I'd like to remove swrast from devices. It doesn't work (eglInitialize > fails) and I don't think I like swrast there. Any objections? Yes, how do you guarantee that at least one device can be returned in any case? Even if no drm device is found? The egl device query extension guarantees that there is at least one device available. Beside that swrast may make sense for itself, that is the reason the swrast device is there. Also, I think I found what crashes. Finally a test bug. The change to piglit is sent to that list. I cleaned up the patch series with the egl extension a bit on my gitlab repo. That with the updated test seems to work here. Still, it would be good if you could test the part that changes the visuals and the pbuffers. best Mathias > > Marek > > On Wed, Apr 17, 2019 at 12:38 AM Mathias Fröhlich > wrote: > > > > > Hi, > > > > On Tuesday, 16 April 2019 17:50:33 CEST Marek Olšák wrote: > > > On Wed, Apr 10, 2019 at 5:37 AM Mathias Fröhlich < > > mathias.froehl...@gmx.net> > > > wrote: > > > > > > > Hi Emil, > > > > > > > > On Monday, 8 April 2019 12:28:55 CEST Emil Velikov wrote: > > > > > > Now that I have been putting together a test case for the the > > actual > > > > use > > > > > > I do see some issues with the pbuffer code path. Well - still > > > > investigating > > > > > > if the test is wrong or the result. > > > > > > > > > > > Actually I do recall some issues with surfaceless (which is > > > > > practically identical to this code) and some (with alpha?) formats. > > > > > My memory is pretty flaky - I would suggest trying various configs: > > > > > with, w/o alpha, 32/24 etc. > > > > > > > > Ok, yesterday evening I spent some time to look into that. > > > > Finally the egl config mechanism puts double buffer visuals > > > > to the pbuffer surface which in turn makes mesa think if should look > > > > at the back buffer which is in turn not set in the pbuffer > > gl_framebuffer. > > > > That one makes my drivers just throw away the rendering result so a > > > > read back does not show anything usable. > > > > > > > > With that fixed I get in principle correct results. > > > > > > > > My gitlab repo contains a branch that sketches my quick fix that I use > > to > > > > make > > > > at least the new piglit test case work. No further testing done. > > > > But may be that helps in getting something reasonable out. > > > > > > > > > > So we can push this series now, right? > > > > Have you been looking at that branch? > > > > Its a sketch at best, and under some circumstance it still crashes > > somewhere > > in the back and forth of virtual methods in the loader and driver. > > I think it's going the riht direction, but until that is fixed I don't > > want to have my > > name tag on that unfinished stuff. > > > > If you want to help out, you can take a look at the egl config change in > > that branch. One needs to check if that 'double buffering visual' filter > > there > > works as expected. Throw test cases at it. At least punch that code through > > piglit and whatever test suite you have access. > > > > best > > > > Mathias > > > > > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl: add EGL_platform_device support
Hi, On Tuesday, 16 April 2019 17:50:33 CEST Marek Olšák wrote: > On Wed, Apr 10, 2019 at 5:37 AM Mathias Fröhlich > wrote: > > > Hi Emil, > > > > On Monday, 8 April 2019 12:28:55 CEST Emil Velikov wrote: > > > > Now that I have been putting together a test case for the the actual > > use > > > > I do see some issues with the pbuffer code path. Well - still > > investigating > > > > if the test is wrong or the result. > > > > > > > Actually I do recall some issues with surfaceless (which is > > > practically identical to this code) and some (with alpha?) formats. > > > My memory is pretty flaky - I would suggest trying various configs: > > > with, w/o alpha, 32/24 etc. > > > > Ok, yesterday evening I spent some time to look into that. > > Finally the egl config mechanism puts double buffer visuals > > to the pbuffer surface which in turn makes mesa think if should look > > at the back buffer which is in turn not set in the pbuffer gl_framebuffer. > > That one makes my drivers just throw away the rendering result so a > > read back does not show anything usable. > > > > With that fixed I get in principle correct results. > > > > My gitlab repo contains a branch that sketches my quick fix that I use to > > make > > at least the new piglit test case work. No further testing done. > > But may be that helps in getting something reasonable out. > > > > So we can push this series now, right? Have you been looking at that branch? Its a sketch at best, and under some circumstance it still crashes somewhere in the back and forth of virtual methods in the loader and driver. I think it's going the riht direction, but until that is fixed I don't want to have my name tag on that unfinished stuff. If you want to help out, you can take a look at the egl config change in that branch. One needs to check if that 'double buffering visual' filter there works as expected. Throw test cases at it. At least punch that code through piglit and whatever test suite you have access. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl: add EGL_platform_device support
Hi Emil, On Monday, 8 April 2019 12:28:55 CEST Emil Velikov wrote: > > Now that I have been putting together a test case for the the actual use > > I do see some issues with the pbuffer code path. Well - still investigating > > if the test is wrong or the result. > > > Actually I do recall some issues with surfaceless (which is > practically identical to this code) and some (with alpha?) formats. > My memory is pretty flaky - I would suggest trying various configs: > with, w/o alpha, 32/24 etc. Ok, yesterday evening I spent some time to look into that. Finally the egl config mechanism puts double buffer visuals to the pbuffer surface which in turn makes mesa think if should look at the back buffer which is in turn not set in the pbuffer gl_framebuffer. That one makes my drivers just throw away the rendering result so a read back does not show anything usable. With that fixed I get in principle correct results. My gitlab repo contains a branch that sketches my quick fix that I use to make at least the new piglit test case work. No further testing done. But may be that helps in getting something reasonable out. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl: add EGL_platform_device support
Hi Emil, Now that I have been putting together a test case for the the actual use I do see some issues with the pbuffer code path. Well - still investigating if the test is wrong or the result. In the mean time some small comments inline below ... Mathias On Thursday, 4 April 2019 17:25:28 CEST Emil Velikov wrote: > This new 'platform' is added by default with no guards. > > It is effectively a copy of the surfaceless one, with updated function > names and brand new probe function. > > Due to the reuse, some of the ifdef HAVE_SURFACELESS_PLATFORM guards > have been dropped. > > A worthy mention are the changes in _egFindDisplay, since the original > and dup'd fd are required, we make use of the plat_opt argument. > > Note that no hacks for eglGetDisplay are added - the API works only with > the eglGetPlatformDisplay* API. > > v3: > - s/dpy/disp/g > - drop swap_buffers* callbacks > - drop loader_set_logger() > - drop local define > - re-introduce _eglGetDRMDeviceRenderNode() > - EGL_WARN on ForceSoftware with HW device - continue using the HW device > - bail out for "EGL_MESA_device_software" until it's fixed > - wire-up the Android build > > v2: > - s/_eglCompareDeviceDisplay/_eglSameDeviceDisplay/ (Eric) > - let ^^ return bool (Eric) > - fixup meson build, move files() further up (Eric) > - copy from plat. surfaceless w/o the visual cleanups > - close and free when destroying the dpy > - sprinkle a few _eglDeviceSupports > - split fd handling into separate function > - use directly the render node if no FD is given (Mathias) > > Cc: Mathias Fröhlich > Cc: Marek Olšák > Signed-off-by: Emil Velikov > --- > src/egl/Android.mk | 1 + > src/egl/Makefile.am| 3 + > src/egl/drivers/dri2/egl_dri2.c| 3 + > src/egl/drivers/dri2/egl_dri2.h| 13 +- > src/egl/drivers/dri2/platform_device.c | 388 + > src/egl/main/eglapi.c | 13 +- > src/egl/main/egldevice.c | 16 + > src/egl/main/egldevice.h | 3 + > src/egl/main/egldisplay.c | 109 ++- > src/egl/main/egldisplay.h | 11 + > src/egl/main/eglglobals.c | 1 + > src/egl/meson.build| 1 + > 12 files changed, 550 insertions(+), 12 deletions(-) > create mode 100644 src/egl/drivers/dri2/platform_device.c > > diff --git a/src/egl/Android.mk b/src/egl/Android.mk > index a9319f56ae7..6c886a5aefb 100644 > --- a/src/egl/Android.mk > +++ b/src/egl/Android.mk > @@ -36,6 +36,7 @@ include $(CLEAR_VARS) > LOCAL_SRC_FILES := \ > $(LIBEGL_C_FILES) \ > $(dri2_backend_core_FILES) \ > + drivers/dri2/platform_android.c \ > drivers/dri2/platform_android.c That one duplicates the file. The hunk is probably no longer needed? > > LOCAL_CFLAGS := \ > diff --git a/src/egl/Makefile.am b/src/egl/Makefile.am > index 349779feb4e..6a74e692b59 100644 > --- a/src/egl/Makefile.am > +++ b/src/egl/Makefile.am > @@ -106,6 +106,9 @@ if HAVE_PLATFORM_SURFACELESS > dri2_backend_FILES += drivers/dri2/platform_surfaceless.c > endif > > +# Unconditionally enable platform_device > +dri2_backend_FILES += drivers/dri2/platform_device.c > + > if HAVE_PLATFORM_ANDROID > AM_CFLAGS += $(ANDROID_CFLAGS) > libEGL_common_la_LIBADD += $(ANDROID_LIBS) > diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c > index 6acc99aa62a..d0e8a70b48e 100644 > --- a/src/egl/drivers/dri2/egl_dri2.c > +++ b/src/egl/drivers/dri2/egl_dri2.c > @@ -874,6 +874,9 @@ dri2_initialize(_EGLDriver *drv, _EGLDisplay *disp) > case _EGL_PLATFORM_SURFACELESS: >ret = dri2_initialize_surfaceless(drv, disp); >break; > + case _EGL_PLATFORM_DEVICE: > + ret = dri2_initialize_device(drv, disp); > + break; > case _EGL_PLATFORM_X11: >ret = dri2_initialize_x11(drv, disp); >break; > diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h > index aa143deb867..a4064f04043 100644 > --- a/src/egl/drivers/dri2/egl_dri2.h > +++ b/src/egl/drivers/dri2/egl_dri2.h > @@ -331,10 +331,10 @@ struct dri2_egl_surface > } color_buffers[3], *back; > #endif > > -#if defined(HAVE_SURFACELESS_PLATFORM) > - __DRIimage *front; > - unsigned int visual; > -#endif > + /* surfaceless and device */ > + __DRIimage *front; > + unsigned int visual; > + > int out_fence_fd; > EGLBoolean enable_out_fence; > }; > @@ -492,6 +492,11 @@ dri2_initialize_surfaceless(_EGLDriver *drv, _EGLDisplay > *disp) > } >
Re: [Mesa-dev] [PATCH v2 8/8] egl: add EGL_platform_device support
Good morning, On Wednesday, 3 April 2019 16:51:03 CEST Marek Olšák wrote: > On Wed, Apr 3, 2019 at 10:13 AM Mathias Fröhlich > wrote: > > > > What is missing for merging this? > > > > I saw the pbuffer swrast crash and proposed to disable them via the > > 3rd patch that I pushed into my branch for you. > > Emil never responded to that proposal directly. In one mail he mentioned > > that he wanted to care for swrast to cope with the pbuffers. So I hoped > > that > > he continues the work he mentioned... > > > > For me it would be ok to squash patch #2 and #3 into one and for now > > disable pbuffers on swrast from device. > > > > What I don't want to have is mesa crashing on the egl pbuffer on a swrast > > device as crashing is not an option. > > > > Sounds reasonable. So you basically said that the branch is ready to be > merged, right? Hmm, Emil sent a new series. I spent the day to plug together all the use cases that should be tested finally. I just sent that to the piglit list. With that I hope not to mention anymore /some/ test cases here around that you don't know. An may be you both take a look at the test so that we can continuously test these code paths. I will take a look at Emils changes as my branch only got rebased to keep it compiling. I am not sure my branch still works sufficiently. > I'm being pressured to get EGL_EXT_platform_device into master ASAP. Which I completely understand. The feature is really a killer feature for the CAE industry! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 8/8] egl: add EGL_platform_device support
> What is missing for merging this? I saw the pbuffer swrast crash and proposed to disable them via the 3rd patch that I pushed into my branch for you. Emil never responded to that proposal directly. In one mail he mentioned that he wanted to care for swrast to cope with the pbuffers. So I hoped that he continues the work he mentioned... For me it would be ok to squash patch #2 and #3 into one and for now disable pbuffers on swrast from device. What I don't want to have is mesa crashing on the egl pbuffer on a swrast device as crashing is not an option. best Mathias > > Thanks, > Marek > > On Wed, Apr 3, 2019 at 12:30 AM Mathias Fröhlich > wrote: > > > Marek, > > > > On Tuesday, 2 April 2019 23:07:50 CEST Marek Olšák wrote: > > > Do you have a branch with patch 7/8 and 8/8? I'm interested in > > > EGL_EXT_platform_device on radeonsi. > > > > You can take a look at the egl-device-4 branch on > > https://gitlab.freedesktop.org/frohlich/mesa.git > > I pushed a rebased and slightly changed wip for you. > > > > I have also some more tests in my local piglit as well as separate > > tests/examples that I initially wrote as an example to make use of the > > extension. > > But I have to dig out these first... > > > > best > > > > Mathias > > > > > > > > Thanks, > > > Marek > > > > > > On Wed, Oct 3, 2018 at 4:36 AM Mathias Fröhlich < > > mathias.froehl...@gmx.net> > > > wrote: > > > > > > > Hi Emil, > > > > > > > > Ok, thanks for picking that up. > > > > > > > > On Tuesday, 2 October 2018 12:23:30 CEST Emil Velikov wrote: > > > > > On Thu, 20 Sep 2018 at 15:13, Mathias Fröhlich > > > > > wrote: > > > > > > > > > > > > > > > > > If I replace the above with > > > > > > > > > > > > EGLint surface_type = 0; > > > > > > /* Only advertise pbuffer configs for non swrast devices > > */ > > > > > > if (dri2_dpy->image_driver) > > > > > > surface_type = EGL_PBUFFER_BIT; > > > > > > > > > > > > dri2_conf = dri2_add_config(dpy, > > dri2_dpy->driver_configs[i], > > > > > >config_count + 1, surface_type, NULL, > > > > > >visuals[j].rgba_masks); > > > > > > > > > > > > then I can easily prohibit the crash that I mentioned when trying > > to > > > > > > create a pbuffer surface on the swrast device. > > > > > > At least I do no longer get a valid pbuffer config from > > eglChooseConfig > > > > > > and without that we cannot reach the crashing > > > > > > dri2_dpy->image_driver->createNewDrawable > > > > > > call somewhere from eglCreatePbufferSurface anymore. > > > > > > > > > > > > Still getting a surface less context on swrast should work... > > > > > > > > > > > Issue is that we do not know how to create a "pbuffer only" swrast. > > > > > > > > > > Hence one resolves to hacks like the ones we have in > > > > platform_surfaceless. > > > > > Effectively pilling hacks upon hacks - see swrast_loader_extensions > > > > > and "software path w/o DRM.". > > > > > > > > > > That said, I'm working on proper solution but since it will take some > > > > > time to finish/merge I'll drop this and 7/8 for now. > > > > > > > > > > > > That's fine too. > > > > > > > > What I wanted to avoid is that people using the extension correctly > > fail > > > > with a > > > > crash in the application when running on mesa. > > > > Think about it. Thats really bad from an application writers point of > > view > > > > as you do all right with checking extensions and that and then the > > opengl > > > > stack suddenly crashes. > > > > > > > > best > > > > > > > > Mathias > > > > > > > > > > > > ___ > > > > mesa-dev mailing list > > > > mesa-dev@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > > > > > > > > > > > > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 8/8] egl: add EGL_platform_device support
Marek, On Tuesday, 2 April 2019 23:07:50 CEST Marek Olšák wrote: > Do you have a branch with patch 7/8 and 8/8? I'm interested in > EGL_EXT_platform_device on radeonsi. You can take a look at the egl-device-4 branch on https://gitlab.freedesktop.org/frohlich/mesa.git I pushed a rebased and slightly changed wip for you. I have also some more tests in my local piglit as well as separate tests/examples that I initially wrote as an example to make use of the extension. But I have to dig out these first... best Mathias > > Thanks, > Marek > > On Wed, Oct 3, 2018 at 4:36 AM Mathias Fröhlich > wrote: > > > Hi Emil, > > > > Ok, thanks for picking that up. > > > > On Tuesday, 2 October 2018 12:23:30 CEST Emil Velikov wrote: > > > On Thu, 20 Sep 2018 at 15:13, Mathias Fröhlich > > > wrote: > > > > > > > > > > > If I replace the above with > > > > > > > > EGLint surface_type = 0; > > > > /* Only advertise pbuffer configs for non swrast devices */ > > > > if (dri2_dpy->image_driver) > > > > surface_type = EGL_PBUFFER_BIT; > > > > > > > > dri2_conf = dri2_add_config(dpy, dri2_dpy->driver_configs[i], > > > >config_count + 1, surface_type, NULL, > > > >visuals[j].rgba_masks); > > > > > > > > then I can easily prohibit the crash that I mentioned when trying to > > > > create a pbuffer surface on the swrast device. > > > > At least I do no longer get a valid pbuffer config from eglChooseConfig > > > > and without that we cannot reach the crashing > > > > dri2_dpy->image_driver->createNewDrawable > > > > call somewhere from eglCreatePbufferSurface anymore. > > > > > > > > Still getting a surface less context on swrast should work... > > > > > > > Issue is that we do not know how to create a "pbuffer only" swrast. > > > > > > Hence one resolves to hacks like the ones we have in > > platform_surfaceless. > > > Effectively pilling hacks upon hacks - see swrast_loader_extensions > > > and "software path w/o DRM.". > > > > > > That said, I'm working on proper solution but since it will take some > > > time to finish/merge I'll drop this and 7/8 for now. > > > > > > That's fine too. > > > > What I wanted to avoid is that people using the extension correctly fail > > with a > > crash in the application when running on mesa. > > Think about it. Thats really bad from an application writers point of view > > as you do all right with checking extensions and that and then the opengl > > stack suddenly crashes. > > > > best > > > > Mathias > > > > > > ___ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/6] st/mesa: fix sampler view context mismatch issue
Hi Brian, On Thursday, 14 March 2019 15:04:28 CET Brian Paul wrote: > > Ok, so basically you are saying that you improve the current situation with > > this > > current series, still leaving a race condition open. But a good final fix > > to the race > > condition is underway. > > Yeah, I can probably stage the multi-thread/race patch series before > this context-fix-up series. Ok, combining that all with the rest of the fix makes highly sense! Thanks for explaining! And sorry for my late replies. Here without family and sickness. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/8] st/mesa: implement "zombie" sampler views
Hi Brian,
the full package looks great and makes a lot of sense!
Reviewed-by: Mathias Fröhlich
best
Mathias
On Thursday, 14 March 2019 20:37:09 CET Brian Paul wrote:
> When st_texture_release_all_sampler_views() is called the texture may
> have sampler views belonging to several contexts. If we unreference a
> sampler view and its refcount hits zero, we need to be sure to destroy
> the sampler view with the same context which created it.
>
> This was not the case with the previous code which used
> pipe_sampler_view_release(). That function could end up freeing a
> sampler view with a context different than the one which created it.
> In the case of the VMware svga driver, we detected this but leaked the
> sampler view. This led to a crash with google-chrome when the kernel
> module had too many sampler views. VMware bug 2274734.
>
> Alternately, if we try to delete a sampler view with the correct
> context, we may be "reaching into" a context which is active on
> another thread. That's not safe.
>
> To fix these issues this patch adds a per-context list of "zombie"
> sampler views. These are views which are to be freed at some point
> when the context is active. Other contexts may safely add sampler
> views to the zombie list at any time (it's mutex protected). This
> avoids the context/view ownership mix-ups we had before.
>
> Tested with: google-chrome, google earth, Redway3D Watch/Turbine demos
> a few Linux games. If anyone can recomment some other multi-threaded,
> multi-context GL apps to test, please let me know.
> ---
> src/mesa/state_tracker/st_cb_flush.c | 6 +++
> src/mesa/state_tracker/st_context.c | 81
>
> src/mesa/state_tracker/st_context.h | 25 ++
> src/mesa/state_tracker/st_sampler_view.c | 27 +--
> src/mesa/state_tracker/st_texture.h | 3 ++
> 5 files changed, 138 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_flush.c
> b/src/mesa/state_tracker/st_cb_flush.c
> index 5b3188c..81e5338 100644
> --- a/src/mesa/state_tracker/st_cb_flush.c
> +++ b/src/mesa/state_tracker/st_cb_flush.c
> @@ -39,6 +39,7 @@
> #include "st_cb_flush.h"
> #include "st_cb_clear.h"
> #include "st_cb_fbo.h"
> +#include "st_context.h"
> #include "st_manager.h"
> #include "pipe/p_context.h"
> #include "pipe/p_defines.h"
> @@ -53,6 +54,11 @@ st_flush(struct st_context *st,
> {
> st_flush_bitmap_cache(st);
>
> + /* We want to call this function periodically.
> +* Typically, it has nothing to do so it shouldn't be expensive.
> +*/
> + st_context_free_zombie_objects(st);
> +
> st->pipe->flush(st->pipe, fence, flags);
> }
>
> diff --git a/src/mesa/state_tracker/st_context.c
> b/src/mesa/state_tracker/st_context.c
> index 2898279..bd919da 100644
> --- a/src/mesa/state_tracker/st_context.c
> +++ b/src/mesa/state_tracker/st_context.c
> @@ -261,6 +261,80 @@ st_invalidate_state(struct gl_context *ctx)
> }
>
>
> +/*
> + * In some circumstances (such as running google-chrome) the state
> + * tracker may try to delete a resource view from a context different
> + * than when it was created. We don't want to do that.
> + * In that situation, st_texture_release_all_sampler_views() calls this
> + * function to save the view for later deletion. The context here is
> + * expected to be the context which created the view.
> + */
> +void
> +st_save_zombie_sampler_view(struct st_context *st,
> +struct pipe_sampler_view *view)
> +{
> + struct st_zombie_sampler_view_node *entry;
> +
> + assert(view->context == st->pipe);
> + assert(view->reference.count == 1);
> +
> + entry = MALLOC_STRUCT(st_zombie_sampler_view_node);
> + if (!entry)
> + return;
> +
> + entry->view = view;
> +
> + /* We need a mutex since this function may be called from one thread
> +* while free_zombie_resource_views() is called from another.
> +*/
> + mtx_lock(>zombie_sampler_views.mutex);
> + LIST_ADDTAIL(>node, >zombie_sampler_views.list.node);
> + mtx_unlock(>zombie_sampler_views.mutex);
> +}
> +
> +
> +/*
> + * Free any zombie sampler views that may be attached to this context.
> + */
> +static void
> +free_zombie_sampler_views(struct st_context *st)
> +{
> + struct st_zombie_sampler_view_node *entry, *next;
> +
> + if (LIST_IS_EMPTY(>zombie_sampler_views.list.node)) {
> + return;
> + }
> +
> + mtx_lock(>zombie_sampler_views.mutex);
> +
> + LIST
Re: [Mesa-dev] [PATCH 9/9] mesa: Add assert to _mesa_primitive_restart_index.
Hi Brian, On Thursday, 14 March 2019 14:55:11 CET Brian Paul wrote: > yeah, looks good. Thanks. > > Reviewed-by: Brian Paul > > I don't remember- do you need me to push your patches for you? Thanks for the review! I have rights to push. Thanks for asking!! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] RFC: Workaround for pthread_setaffinity_np() seccomp filtering
Hi, On Wednesday, 13 March 2019 14:29:28 CET Marc-André Lureau wrote: > > On Wednesday, 13 March 2019 00:03:26 CET Marek Olšák wrote: > > > The env var workaround is fine. > > > > > > Thread affinity is used for cache topology related optimizations. I think > > > it's a mistake to treat it only as a resource allocation tool. > > > > For a shorter term solution to the problem. > > One Idea that comes into my mind: > > > > Can we check the currently set thread affinity mask if it still contains the > > cpu we are aiming for and narrow the mask down to our cpu if we can do > > that by narrowing. If we would need to assign our thread to a cpu that > > we are not bound anymore just do nothing. > > > > getaffinity() is also blocked by current qemu policy. > > It should be possible to allow a narrower setaffinity(), with some complex > rule. > > > That would obviously require that we can still call into > > pthread_setaffinity_np > > without being just killed straight away because we touch something that > > somebody else wants to control. And that we even succeed in just narrowing > > down the allowed set of cpus. > > Marc-Andre, would that still work with qemu then? > > For now, Daniel proposed "seccomp: don't kill process for resource > control syscalls": the resource control syscalls will return -1/EPERM. Thanks a lot!! I saw the patch! Nevertheless, I think you should consider that point of view from a calling application. If that is not wise to allow something security wise, please find a different solution than just killing processes. The only workaround that I had in my back would have been to start a child process to test if we can call into a well known and always available system call to see if it gets killed or if it succeeds. We could have solved the special case in mesa with environment variables, but the basic problem still remains. best Mathias > > > > > Of course this still leaves a small race condition open if somebody changes > > the > > affinitiy mask of the current thread in between our call to > > pthread_getaffinity_np > > and pthread_setaffinity_np from the outside of our linux task. Then we may > > experience a non narrowing set affinity operation anymore because of an > > other set > > operation that came in between and we may get killed then. > > ... which is an other argument against just killing. But ok ... > > IMO this condition happens sufficiently seldom to accept that. > > > > Could that solve our problem?? > > > > best > > Mathias > > > > > > > > > > Marek > > > > > > On Tue, Mar 12, 2019, 1:59 AM Marc-André Lureau > > > > > > wrote: > > > > > > > Hi > > > > > > > > On Fri, Mar 1, 2019 at 12:13 PM Mathias Fröhlich > > > > wrote: > > > > > > > > > > On Friday, 1 March 2019 12:15:08 CET Eero Tamminen wrote: > > > > > > Hi, > > > > > > > > > > > > On 1.3.2019 11.12, Michel Dänzer wrote: > > > > > > > On 2019-02-28 8:41 p.m., Marek Olšák wrote: > > > > > > >>> On Thu, Feb 28, 2019 at 1:37 PM Eero Tamminen < > > > > eero.t.tammi...@intel.com> > > > > > > >>>> Why distro versions of Qemu filter sched_setaffinity() syscall? > > > > > > >>> > > > > > > >>> (https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1815889) > > > > > > >>> > > > > > > >>> Daniel Berrange (berrange) wrote on 2019-02-27: #19 > > > > > > >>> > > > > > > >>> "IMHO that mesa change is not valid. It is settings its > > > > > > >>> affinity to > > > > > > >>> run on all threads which is definitely *NOT* something we want > > > > > > >>> to > > > > be > > > > > > >>> allowed. Management applications want to control which CPUs QEMU > > > > runs > > > > > > >>> on, and as such Mesa should honour the CPU placement that the > > > > > > >>> QEMU > > > > > > >>> process has. > > > > > > >>> > > > > > > >>> This is a great example of why QEMU wants to use seccomp to > > > > > > >>> block > > > > > > >>> affinity chang
Re: [Mesa-dev] [PATCH 1/6] st/mesa: fix sampler view context mismatch issue
Hi Brian, On Sunday, 10 March 2019 21:32:39 CET Brian Paul wrote: > > On Friday, 8 March 2019 23:52:09 CET Brian Paul wrote: > > > After a while of running google-chrome and viewing Bing maps, we'd see > > > "context mismatch in svga_sampler_view_destroy()" messages and > > > eventually crash because we leaked too many sampler views (the kernel > > > module would have too many sampler views). > > > > > > When a texture object is being deleted, we call > > > st_texture_release_all_sampler_views() to release all the sampler > > > views. In the list, there may sampler views which were created from > > > other contexts. > > > > > > Previously, we called pipe_sampler_view_release(pipe, view) which would > > > use the given pipe context to destroy the view if the refcount hit > > > zero. The svga error was triggered because we were calling > > > pipe->sampler_view_destroy(pipe, view) and the pipe context didn't > > > match the view's parent context. > > > > > > Instead, call pipe_sampler_reference(, NULL). That way, if > > > the refcount hits zero, we'll use the view's parent context to > > > destroy the view. That's what we want. > > > > That sounds plausible so far. > > > > What I wonder, is calling into an 'arbitrary' different context and do > > work there > > thread safe for any gallium driver? > > > > Right. Before this patch there was the problem of trying to destroy a > sampler view with a context different from the one which created it. > That's fixed, but now there's the multi-threading issue as you say, and > which I believe, is a less-common situation. Hmm, you are saying we tolerate a race condition then? > > Especially since pipe_sampler_view_reference in its documentation says > > that the > > views provided need to originate from the same context and that this must > > be the 'current'. > > > > I have been taking a quick look into iris and radeonsi and both seem to be > > safe > > in terms of threads for dereferencing the views finally. > > But either the documentation of pipe_sampler_view_reference should be > > updated > > or I may miss an other piece of the puzzle to see that it is still save to > > do so. > > > > Last week I posted a patch series for the VMware driver which handled the > thread issue with a list of "zombie" sampler views. Jose suggested moving > that up into the state tracker, and that is a better long-term solution. > > I'm almost done with a patch series that does that. I'll likely post it > tomorrow. Ok, so basically you are saying that you improve the current situation with this current series, still leaving a race condition open. But a good final fix to the race condition is underway. Anyhow, to me it looks like all the reference counting inlines in gallium seem to assume being truely thread safe and do not assume that the context that created the view needs to be current on dereferencing. Means: is this comment at the pipe_sampler_view_*reference that states that the owning context needs to be current really correct? Such a comment implies to me that the context possibly has a non thread safe list of views that may get corrupted if being called from an other current context may be current in an other thread. ... well or something similar to that. Do you remember where this comment comes from? Is there a driver that requires the owning context needs to be current property? best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] RFC: Workaround for pthread_setaffinity_np() seccomp filtering
Marek, Marc-Andre, On Wednesday, 13 March 2019 00:03:26 CET Marek Olšák wrote: > The env var workaround is fine. > > Thread affinity is used for cache topology related optimizations. I think > it's a mistake to treat it only as a resource allocation tool. For a shorter term solution to the problem. One Idea that comes into my mind: Can we check the currently set thread affinity mask if it still contains the cpu we are aiming for and narrow the mask down to our cpu if we can do that by narrowing. If we would need to assign our thread to a cpu that we are not bound anymore just do nothing. That would obviously require that we can still call into pthread_setaffinity_np without being just killed straight away because we touch something that somebody else wants to control. And that we even succeed in just narrowing down the allowed set of cpus. Marc-Andre, would that still work with qemu then? Of course this still leaves a small race condition open if somebody changes the affinitiy mask of the current thread in between our call to pthread_getaffinity_np and pthread_setaffinity_np from the outside of our linux task. Then we may experience a non narrowing set affinity operation anymore because of an other set operation that came in between and we may get killed then. ... which is an other argument against just killing. But ok ... IMO this condition happens sufficiently seldom to accept that. Could that solve our problem?? best Mathias > > Marek > > On Tue, Mar 12, 2019, 1:59 AM Marc-André Lureau > wrote: > > > Hi > > > > On Fri, Mar 1, 2019 at 12:13 PM Mathias Fröhlich > > wrote: > > > > > > On Friday, 1 March 2019 12:15:08 CET Eero Tamminen wrote: > > > > Hi, > > > > > > > > On 1.3.2019 11.12, Michel Dänzer wrote: > > > > > On 2019-02-28 8:41 p.m., Marek Olšák wrote: > > > > >>> On Thu, Feb 28, 2019 at 1:37 PM Eero Tamminen < > > eero.t.tammi...@intel.com> > > > > >>>> Why distro versions of Qemu filter sched_setaffinity() syscall? > > > > >>> > > > > >>> (https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1815889) > > > > >>> > > > > >>> Daniel Berrange (berrange) wrote on 2019-02-27: #19 > > > > >>> > > > > >>> "IMHO that mesa change is not valid. It is settings its affinity to > > > > >>> run on all threads which is definitely *NOT* something we want to > > be > > > > >>> allowed. Management applications want to control which CPUs QEMU > > runs > > > > >>> on, and as such Mesa should honour the CPU placement that the QEMU > > > > >>> process has. > > > > >>> > > > > >>> This is a great example of why QEMU wants to use seccomp to block > > > > >>> affinity changes to prevent something silently trying to use more > > CPUs > > > > >>> than are assigned to this QEMU." > > > > >>> > > > > >> > > > > >> Mesa uses thread affinity to optimize memory access performance on > > some > > > > >> CPUs (see util_pin_thread_to_L3). Other places in Mesa need to > > restore the > > > > >> original thread affinity for some child threads. Additionally, if > > games > > > > >> limit the thread affinity, Mesa needs to restore the full thread > > affinity > > > > >> for some of its child threads. > > > > > > > > > > The last part sounds like Mesa clearly overstepping its authority. > > > > > > > > > > > > > > >> In essence, the thread affinity should only be considered a hint > > for the > > > > >> kernel for optimal performance. There is no reason to kill the > > process if > > > > >> it's disallowed. Just ignore the call or modify the thread mask to > > make it > > > > >> legal. > > > > > > > > > > The fundamental issue here is that Mesa is using the thread affinity > > API > > > > > for something else than it's intended for. If there was an API for > > what > > > > > Mesa wants (encouraging certain sets of threads to run on > > topologically > > > > > close cores), there should be no need to block that. > > > > > > > > Why such process needs to be killed instead the request being masked > > > > suitably, is there some program that breaks subtly if affinity request > > > >
Re: [Mesa-dev] [PATCH] RFC: Workaround for pthread_setaffinity_np() seccomp filtering
Hi, On Tuesday, 12 March 2019 09:59:17 CET Marc-André Lureau wrote: > Hi > > On Fri, Mar 1, 2019 at 12:13 PM Mathias Fröhlich > wrote: > > > > On Friday, 1 March 2019 12:15:08 CET Eero Tamminen wrote: > > > Hi, > > > > > > On 1.3.2019 11.12, Michel Dänzer wrote: > > > > On 2019-02-28 8:41 p.m., Marek Olšák wrote: > > > >>> On Thu, Feb 28, 2019 at 1:37 PM Eero Tamminen > > > >>> > > > >>>> Why distro versions of Qemu filter sched_setaffinity() syscall? > > > >>> > > > >>> (https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1815889) > > > >>> > > > >>> Daniel Berrange (berrange) wrote on 2019-02-27: #19 > > > >>> > > > >>> "IMHO that mesa change is not valid. It is settings its affinity to > > > >>> run on all threads which is definitely *NOT* something we want to be > > > >>> allowed. Management applications want to control which CPUs QEMU runs > > > >>> on, and as such Mesa should honour the CPU placement that the QEMU > > > >>> process has. > > > >>> > > > >>> This is a great example of why QEMU wants to use seccomp to block > > > >>> affinity changes to prevent something silently trying to use more CPUs > > > >>> than are assigned to this QEMU." > > > >>> > > > >> > > > >> Mesa uses thread affinity to optimize memory access performance on some > > > >> CPUs (see util_pin_thread_to_L3). Other places in Mesa need to restore > > > >> the > > > >> original thread affinity for some child threads. Additionally, if games > > > >> limit the thread affinity, Mesa needs to restore the full thread > > > >> affinity > > > >> for some of its child threads. > > > > > > > > The last part sounds like Mesa clearly overstepping its authority. > > > > > > > > > > > >> In essence, the thread affinity should only be considered a hint for > > > >> the > > > >> kernel for optimal performance. There is no reason to kill the process > > > >> if > > > >> it's disallowed. Just ignore the call or modify the thread mask to > > > >> make it > > > >> legal. > > > > > > > > The fundamental issue here is that Mesa is using the thread affinity API > > > > for something else than it's intended for. If there was an API for what > > > > Mesa wants (encouraging certain sets of threads to run on topologically > > > > close cores), there should be no need to block that. > > > > > > Why such process needs to be killed instead the request being masked > > > suitably, is there some program that breaks subtly if affinity request > > > is masked (and that being worse than the program being killed)? > > > > But that is still a situation that could be nicely handled with a > > EPERM error return. Way better than just kill a process. > > That 'badly affected' program still can call abort then. > > But nicely working programs don't get just killed then!! > > > Returning an error seems less secure that prohibiting it completely. > And it may lead to subtle bugs in rarely tested code paths. > > It's legitimate that QEMU and management layers want to prevent > arbitrary code from changing resource allocation etc. I *never* saw this api as resource allocation. Such a call finally dates back into the IRIX threads (the api *before* pthreads on those very old OpenGL dinosaurs from Silicon Graphics) where this was pretty much used in contexts like exactly what Marek wanted to. To make use hardware topology that you can access specific hardware faster from specific cpu's. Or aiming for cache locality between threads that exchange lots of data between each other. Think of high performance computing applications for cache locality or VR application middle end libraries for hardware OpenGL access. That api was replaced on that ancient operating system by pthreads that contained the equivalent api call. And later on the linux pthread implementation gained the equivalent pthread_setaffinity_np call when SMP linux systems got used more often. Means if you just kill an application that tries to optimize for valid uses using an API that used to work for that purpose for years you will just break existing API's. Beside breaking exiting behavior, just think of what you are doing from an applications view. I think as an application writer no
Re: [Mesa-dev] [PATCH 1/6] st/mesa: fix sampler view context mismatch issue
Brian,
One question also for my own education inline below:
On Friday, 8 March 2019 23:52:09 CET Brian Paul wrote:
> After a while of running google-chrome and viewing Bing maps, we'd see
> "context mismatch in svga_sampler_view_destroy()" messages and
> eventually crash because we leaked too many sampler views (the kernel
> module would have too many sampler views).
>
> When a texture object is being deleted, we call
> st_texture_release_all_sampler_views() to release all the sampler
> views. In the list, there may sampler views which were created from
> other contexts.
>
> Previously, we called pipe_sampler_view_release(pipe, view) which would
> use the given pipe context to destroy the view if the refcount hit
> zero. The svga error was triggered because we were calling
> pipe->sampler_view_destroy(pipe, view) and the pipe context didn't
> match the view's parent context.
>
> Instead, call pipe_sampler_reference(, NULL). That way, if
> the refcount hits zero, we'll use the view's parent context to
> destroy the view. That's what we want.
That sounds plausible so far.
What I wonder, is calling into an 'arbitrary' different context and do work
there
thread safe for any gallium driver?
Especially since pipe_sampler_view_reference in its documentation says that the
views provided need to originate from the same context and that this must
be the 'current'.
I have been taking a quick look into iris and radeonsi and both seem to be safe
in terms of threads for dereferencing the views finally.
But either the documentation of pipe_sampler_view_reference should be updated
or I may miss an other piece of the puzzle to see that it is still save to do
so.
Can you tell me?
thanks
Mathias
>
> The pipe_sampler_view_release() function was introduced years ago to
> avoid freeing a sampler view with a context that was already deleted.
>
> But since then we've improved sampler view and context tracking.
> When a context is destroyed, the state tracker walks over all
> texture objects and frees all sampler views which belong to that
> context. So, we should never end up deleting a sampler view after
> its context is deleted.
>
> After this, we can remove all calls to pipe_sampler_view_release()
> in the drivers.
>
> Finally, it appears that we need to implement a similar tear-down
> mechanism for shaders and programs since we may generate per-context
> shader variants.
>
> Testing done: google chrome, misc GL demos, games
> ---
> src/mesa/state_tracker/st_context.c | 3 +--
> src/mesa/state_tracker/st_sampler_view.c | 8
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_context.c
> b/src/mesa/state_tracker/st_context.c
> index 2898279..a7464fd 100644
> --- a/src/mesa/state_tracker/st_context.c
> +++ b/src/mesa/state_tracker/st_context.c
> @@ -278,8 +278,7 @@ st_destroy_context_priv(struct st_context *st, bool
> destroy_pipe)
> st_destroy_bound_image_handles(st);
>
> for (i = 0; i < ARRAY_SIZE(st->state.frag_sampler_views); i++) {
> - pipe_sampler_view_release(st->pipe,
> ->state.frag_sampler_views[i]);
> + pipe_sampler_view_reference(>state.frag_sampler_views[i], NULL);
> }
>
> /* free glReadPixels cache data */
> diff --git a/src/mesa/state_tracker/st_sampler_view.c
> b/src/mesa/state_tracker/st_sampler_view.c
> index e4eaf39..650a2b0 100644
> --- a/src/mesa/state_tracker/st_sampler_view.c
> +++ b/src/mesa/state_tracker/st_sampler_view.c
> @@ -74,7 +74,7 @@ st_texture_set_sampler_view(struct st_context *st,
>if (sv->view) {
> /* check if the context matches */
> if (sv->view->context == st->pipe) {
> -pipe_sampler_view_release(st->pipe, >view);
> +pipe_sampler_view_reference(>view, NULL);
> goto found;
> }
>} else {
> @@ -94,13 +94,13 @@ st_texture_set_sampler_view(struct st_context *st,
>
> if (new_max < views->max ||
> new_max > (UINT_MAX - sizeof(*views)) /
> sizeof(views->views[0])) {
> -pipe_sampler_view_release(st->pipe, );
> +pipe_sampler_view_reference(, NULL);
> goto out;
> }
>
> struct st_sampler_views *new_views = malloc(new_size);
> if (!new_views) {
> -pipe_sampler_view_release(st->pipe, );
> +pipe_sampler_view_reference(, NULL);
> goto out;
> }
>
> @@ -225,7 +225,7 @@ st_texture_release_all_sampler_views(struct st_context
> *st,
> simple_mtx_lock(>validate_mutex);
> struct st_sampler_views *views = stObj->sampler_views;
> for (i = 0; i < views->count; ++i)
> - pipe_sampler_view_release(st->pipe, >views[i].view);
> + pipe_sampler_view_reference(>views[i].view, NULL);
> simple_mtx_unlock(>validate_mutex);
> }
>
>
___
mesa-dev mailing list
Re: [Mesa-dev] [PATCH 1/2] drisw: fix incomplete type compilation failure
Hi Brian, Both of these fixes: Reviewed-by: Mathias Fröhlich best Mathias On Friday, 8 March 2019 23:53:32 CET Brian Paul wrote: > Fixes: > ../src/gallium/winsys/sw/dri/dri_sw_winsys.c: In function > ‘dri_sw_displaytarget_display’: > ../src/gallium/winsys/sw/dri/dri_sw_winsys.c:255:39: error: dereferencing > pointer to incomplete type ‘struct pipe_box’ >offset = dri_sw_dt->stride * box->y; >^ > --- > src/gallium/winsys/sw/dri/dri_sw_winsys.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/gallium/winsys/sw/dri/dri_sw_winsys.c > b/src/gallium/winsys/sw/dri/dri_sw_winsys.c > index c0200f9..3273813 100644 > --- a/src/gallium/winsys/sw/dri/dri_sw_winsys.c > +++ b/src/gallium/winsys/sw/dri/dri_sw_winsys.c > @@ -35,6 +35,7 @@ > > #include "pipe/p_compiler.h" > #include "pipe/p_format.h" > +#include "pipe/p_state.h" > #include "util/u_inlines.h" > #include "util/u_format.h" > #include "util/u_math.h" > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] svga: init fill variable to avoid compiler warning
Hi Brian,
For both, if you need:
Reviewed-by: Mathias Fröhlich
best
Mathias
On Monday, 4 March 2019 18:20:57 CET Brian Paul wrote:
> MinGW release builds warns about use of a possbily uninitialized
> variable here.
> ---
> src/gallium/drivers/svga/svga_pipe_rasterizer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/gallium/drivers/svga/svga_pipe_rasterizer.c
> b/src/gallium/drivers/svga/svga_pipe_rasterizer.c
> index 1dbf5b6..7d5936f 100644
> --- a/src/gallium/drivers/svga/svga_pipe_rasterizer.c
> +++ b/src/gallium/drivers/svga/svga_pipe_rasterizer.c
> @@ -257,7 +257,7 @@ svga_create_rasterizer_state(struct pipe_context *pipe,
> {
>int fill_front = templ->fill_front;
>int fill_back = templ->fill_back;
> - int fill;
> + int fill = PIPE_POLYGON_MODE_FILL;
>boolean offset_front = util_get_offset(templ, fill_front);
>boolean offset_back = util_get_offset(templ, fill_back);
>boolean offset = FALSE;
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] st/mesa: Invalidate the gallium array atom only if needed.
Hi, On Friday, 1 March 2019 16:50:13 CET Brian Paul wrote: > The series looks OK to me. > > Reviewed-by: Brian Paul Thanks! Pushed! Mathias > > On 02/28/2019 11:18 PM, mathias.froehl...@gmx.net wrote: > > From: Mathias Fröhlich > > > > Now that the buffer object usage history tracks if it is > > being used as vertex buffer object, we can restrict setting > > the ST_NEW_VERTEX_ARRAYS bit to dirty on glBufferData calls to > > buffers that are potentially used as vertex buffer object. > > Also put a note that the same could be done for index arrays > > used in indexed draws. > > > > Signed-off-by: Mathias Fröhlich > > --- > > src/mesa/state_tracker/st_cb_bufferobjects.c | 6 -- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/src/mesa/state_tracker/st_cb_bufferobjects.c > > b/src/mesa/state_tracker/st_cb_bufferobjects.c > > index 5ebe94f4545..b05f2516980 100644 > > --- a/src/mesa/state_tracker/st_cb_bufferobjects.c > > +++ b/src/mesa/state_tracker/st_cb_bufferobjects.c > > @@ -357,8 +357,10 @@ bufferobj_data(struct gl_context *ctx, > > /* The current buffer may be bound, so we have to revalidate all atoms > > that > > * might be using it. > > */ > > - /* TODO: Add arrays to usage history */ > > - ctx->NewDriverState |= ST_NEW_VERTEX_ARRAYS; > > + if (st_obj->Base.UsageHistory & USAGE_ARRAY_BUFFER) > > + ctx->NewDriverState |= ST_NEW_VERTEX_ARRAYS; > > + /* if (st_obj->Base.UsageHistory & USAGE_ELEMENT_ARRAY_BUFFER) */ > > + /*ctx->NewDriverState |= TODO: Handle indices as gallium state; */ > > if (st_obj->Base.UsageHistory & USAGE_UNIFORM_BUFFER) > > ctx->NewDriverState |= ST_NEW_UNIFORM_BUFFER; > > if (st_obj->Base.UsageHistory & USAGE_SHADER_STORAGE_BUFFER) > > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] RFC: Workaround for pthread_setaffinity_np() seccomp filtering
On Friday, 1 March 2019 12:15:08 CET Eero Tamminen wrote: > Hi, > > On 1.3.2019 11.12, Michel Dänzer wrote: > > On 2019-02-28 8:41 p.m., Marek Olšák wrote: > >>> On Thu, Feb 28, 2019 at 1:37 PM Eero Tamminen > Why distro versions of Qemu filter sched_setaffinity() syscall? > >>> > >>> (https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1815889) > >>> > >>> Daniel Berrange (berrange) wrote on 2019-02-27: #19 > >>> > >>> "IMHO that mesa change is not valid. It is settings its affinity to > >>> run on all threads which is definitely *NOT* something we want to be > >>> allowed. Management applications want to control which CPUs QEMU runs > >>> on, and as such Mesa should honour the CPU placement that the QEMU > >>> process has. > >>> > >>> This is a great example of why QEMU wants to use seccomp to block > >>> affinity changes to prevent something silently trying to use more CPUs > >>> than are assigned to this QEMU." > >>> > >> > >> Mesa uses thread affinity to optimize memory access performance on some > >> CPUs (see util_pin_thread_to_L3). Other places in Mesa need to restore the > >> original thread affinity for some child threads. Additionally, if games > >> limit the thread affinity, Mesa needs to restore the full thread affinity > >> for some of its child threads. > > > > The last part sounds like Mesa clearly overstepping its authority. > > > > > >> In essence, the thread affinity should only be considered a hint for the > >> kernel for optimal performance. There is no reason to kill the process if > >> it's disallowed. Just ignore the call or modify the thread mask to make it > >> legal. > > > > The fundamental issue here is that Mesa is using the thread affinity API > > for something else than it's intended for. If there was an API for what > > Mesa wants (encouraging certain sets of threads to run on topologically > > close cores), there should be no need to block that. > > Why such process needs to be killed instead the request being masked > suitably, is there some program that breaks subtly if affinity request > is masked (and that being worse than the program being killed)? But that is still a situation that could be nicely handled with a EPERM error return. Way better than just kill a process. That 'badly affected' program still can call abort then. But nicely working programs don't get just killed then!! best Mathias > > > - eero > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: Reduce array updates due to current changes.
Hi,
Thanks, Brian and Marek for the review!
best
Mathias
On Monday, 25 February 2019 22:44:37 CET Marek Olšák wrote:
> Reviewed-by: Marek Olšák
>
> Marek
>
> On Sun, Feb 24, 2019 at 1:46 AM wrote:
>
> > From: Mathias Fröhlich
> >
> > Hi Brian,
> >
> > Following a small optimization in the gallium state tracker to
> > avoid flagging ST_NEW_VERTEX_ARRAYS a bit more often:
> >
> > please review!
> >
> > best
> >
> > Mathias
> >
> >
> >
> >
> > Since using bitmasks we can easily check if we have any
> > current value that is potentially uploaded on array setup.
> > So check for any potential vertex program input that is not
> > already a vao enabled array. Only flag array update if there is
> > a potential overlap.
> >
> > Signed-off-by: Mathias Fröhlich
> > ---
> > src/mesa/state_tracker/st_context.c | 2 +-
> > src/mesa/state_tracker/st_context.h | 9 +
> > 2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/state_tracker/st_context.c
> > b/src/mesa/state_tracker/st_context.c
> > index 0a0bd8ba1ca..45451531df9 100644
> > --- a/src/mesa/state_tracker/st_context.c
> > +++ b/src/mesa/state_tracker/st_context.c
> > @@ -224,7 +224,7 @@ st_invalidate_state(struct gl_context *ctx)
> > if (new_state & _NEW_PIXEL)
> >st->dirty |= ST_NEW_PIXEL_TRANSFER;
> >
> > - if (new_state & _NEW_CURRENT_ATTRIB)
> > + if (new_state & _NEW_CURRENT_ATTRIB && st_vp_uses_current_values(ctx))
> >st->dirty |= ST_NEW_VERTEX_ARRAYS;
> >
> > /* Update the vertex shader if ctx->Light._ClampVertexColor was
> > changed. */
> > diff --git a/src/mesa/state_tracker/st_context.h
> > b/src/mesa/state_tracker/st_context.h
> > index ed69e3d4873..324a7f24178 100644
> > --- a/src/mesa/state_tracker/st_context.h
> > +++ b/src/mesa/state_tracker/st_context.h
> > @@ -28,6 +28,7 @@
> > #ifndef ST_CONTEXT_H
> > #define ST_CONTEXT_H
> >
> > +#include "main/arrayobj.h"
> > #include "main/mtypes.h"
> > #include "state_tracker/st_api.h"
> > #include "main/fbobject.h"
> > @@ -398,6 +399,14 @@ st_user_clip_planes_enabled(struct gl_context *ctx)
> >ctx->Transform.ClipPlanesEnabled;
> > }
> >
> > +
> > +static inline bool
> > +st_vp_uses_current_values(const struct gl_context *ctx)
> > +{
> > + const uint64_t inputs = ctx->VertexProgram._Current->info.inputs_read;
> > + return _mesa_draw_current_bits(ctx) & inputs;
> > +}
> > +
> > /** clear-alloc a struct-sized object, with casting */
> > #define ST_CALLOC_STRUCT(T) (struct T *) calloc(1, sizeof(struct T))
> >
> > --
> > 2.20.1
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] egl/sl: split out swrast probe into separate function
Emil,
indeed this is easier to read.
For patch #1 and #2 you get
Reviewed-by: Mathias Fröhlich
For patch #3, I don't know vgem good enough to judge the big picture.
The change within mesa itself looks technically correct.
best
Mathias
On Tuesday, 5 February 2019 16:31:06 CET Emil Velikov wrote:
> From: Emil Velikov
>
> Make the code a bit easier to read.
>
> As a bonus point this makes it obvious that we forgot to call
> _eglAddDevice() for the device - do so.
>
> Signed-off-by: Emil Velikov
> ---
> src/egl/drivers/dri2/platform_surfaceless.c | 46 -
> 1 file changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/platform_surfaceless.c
> b/src/egl/drivers/dri2/platform_surfaceless.c
> index f9809561611..d6e48ba11b2 100644
> --- a/src/egl/drivers/dri2/platform_surfaceless.c
> +++ b/src/egl/drivers/dri2/platform_surfaceless.c
> @@ -322,25 +322,27 @@ surfaceless_probe_device(_EGLDisplay *dpy, bool swrast)
>dri2_dpy->loader_extensions = NULL;
> }
>
> - /* No DRM device, so attempt to fall back to software path w/o DRM. */
> - if (swrast) {
> - _eglLog(_EGL_DEBUG, "Falling back to surfaceless swrast without DRM.");
> - dri2_dpy->fd = -1;
> - dri2_dpy->driver_name = strdup("swrast");
> - if (!dri2_dpy->driver_name) {
> - return false;
> - }
> + return false;
> +}
>
> - if (dri2_load_driver_swrast(dpy)) {
> - dri2_dpy->loader_extensions = swrast_loader_extensions;
> - return true;
> - }
> +static bool
> +surfaceless_probe_device_sw(_EGLDisplay *dpy)
> +{
> + struct dri2_egl_display *dri2_dpy = dpy->DriverData;
>
> - free(dri2_dpy->driver_name);
> - dri2_dpy->driver_name = NULL;
> - }
> + dri2_dpy->fd = -1;
> + dpy->Device = _eglAddDevice(dri2_dpy->fd, true);
> + assert(dpy->Device);
>
> - return false;
> + dri2_dpy->driver_name = strdup("swrast");
> + if (!dri2_dpy->driver_name)
> + return false;
> +
> + if (!dri2_load_driver_swrast(dpy))
> + return false;
> +
> + dri2_dpy->loader_extensions = swrast_loader_extensions;
> + return true;
> }
>
> EGLBoolean
> @@ -364,9 +366,15 @@ dri2_initialize_surfaceless(_EGLDriver *drv, _EGLDisplay
> *disp)
> "No hardware driver found, falling back to software
> rendering");
> }
>
> - if (!driver_loaded && !surfaceless_probe_device(disp, true)) {
> - err = "DRI2: failed to load driver";
> - goto cleanup;
> + if (!driver_loaded)
> + driver_loaded = surfaceless_probe_device(disp, true);
> +
> + if (!driver_loaded) {
> + _eglLog(_EGL_DEBUG, "Falling back to surfaceless swrast without DRM.");
> + if (!surfaceless_probe_device_sw(disp)) {
> + err = "DRI2: failed to load driver";
> + goto cleanup;
> + }
> }
>
> if (!dri2_create_screen(disp)) {
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] egl/egldevice: Fix broken reference to dev->device without LIBDRM
Hi, On Saturday, 29 December 2018 17:05:16 CET Ilia Mirkin wrote: > On Sat, Dec 29, 2018 at 6:17 AM Mathias Fröhlich > wrote: > > In Emils patches building on top, _eglGetDRMDeviceRenderNode is only called > > from > > code paths guarded with HAVE_LIBDRM. > > OK, so we could also guard the function's existence on HAVE_LIBDRM as > well. I think that's the most logical way forward, rather than having > a function which unexpectedly returns NULL. If you want. I don't have a strong preference for either. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] egl/egldevice: Fix broken reference to dev->device without LIBDRM
Hi all,
given what I see in Emils patches and in the assumption that this helps already
for haiku.
For both:
Reviewed-by: Mathias Fröhlich
best
Mathias
On Thursday, 27 December 2018 21:41:46 CET Alexander von Gluck IV wrote:
> ---
> src/egl/main/egldevice.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/egl/main/egldevice.c b/src/egl/main/egldevice.c
> index 4878039be0..96042f4c5c 100644
> --- a/src/egl/main/egldevice.c
> +++ b/src/egl/main/egldevice.c
> @@ -211,7 +211,11 @@ _eglDeviceSupports(_EGLDevice *dev, _EGLDeviceExtension
> ext)
> const char *
> _eglGetDRMDeviceRenderNode(_EGLDevice *dev)
> {
> - return dev->device->nodes[DRM_NODE_RENDER];
> + const char* result = NULL;
> +#ifdef HAVE_LIBDRM
> + result = dev->device->nodes[DRM_NODE_RENDER];
> +#endif
> + return result;
> }
>
> EGLBoolean
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] egl/egldevice: Fix broken reference to dev->device without LIBDRM
Good morning, On Thursday, 27 December 2018 21:59:47 CET Ilia Mirkin wrote: > I don't see this function used anywhere... it's not exported either. > > It was added in dbb4457d9858fa977246aa5e9cabe83455022dfe by Emil as > part of EGL_EXT_device_drm, but appears unused in that commit as well. > > Emil - can this function just be removed perhaps? Not Emil, so he probably knows more about that, especially the current state. That function was introduced as a preparation for finally getting EGL_EXT_platform_device upported. For those final patches Emil wanted to clean up something in setting up a software renderer when I pointed out a crash in the pbuffer code path. I was always waiting for him to come up with that cleanup. I personally had a 'workaround' that we could use to announce EGL_EXT_platform_device but not offer any pbuffer config finally until that is cleaned up. So, as of now the function is unused, but it will be used in the hopefully not so distant future as I think we should have EGL_EXT_platform_device finally. In Emils patches building on top, _eglGetDRMDeviceRenderNode is only called from code paths guarded with HAVE_LIBDRM. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] docs: Document GitLab merge request process (email alternative)
Hi, On Thursday, 13 December 2018 17:40:49 CET Jason Ekstrand wrote: > On Thu, Dec 13, 2018 at 9:56 AM Ilia Mirkin wrote: > > > On Thu, Dec 13, 2018 at 10:52 AM Alex Deucher > > wrote: > > > > > > On Wed, Dec 12, 2018 at 3:42 AM Samuel Pitoiset > > > wrote: > > > > > > > > Personally, I will continue to use the list, at least for a simplicity > > > > point of view. I'm not sure if using a new tool will improve quality > > and > > > > code review process. > > > > > > > > Though, if the majority reports that is really nice to use, I will > > > > probably change my mind. Not a strong reject. > > > > > > I agree. We've been using the MR interface for xf86-video-amdgpu and > > > I find it awkward compared to the mailing list. Maybe it just takes > > > getting used to. I also feel less inclined to do drive by patch > > > review if I have to explicitly delve into the browser to look at the > > > outstanding MRs. Over email, sometimes I see a patch set in my in box > > > that piques my interest and I find some time to review it when I might > > > not have otherwise if the bar were higher. > > > > FWIW I also do a lot of drive-by reviews. Perhaps those aren't > > valuable -- dunno. Either way, if it's not in email, I won't end up > > seeing it. > > > > I find this commentary on drive-by reviews interesting. Personally, I > don't find going to the web interface to be a problem at all but that may > be because my e-mail is also in my web browser and it's as easy as clicking > a link. Also, since I'm an active contributor who's likely to be making > MRs and commenting on various MRs on a regular basis, I'm already on the MR > page looking at it. If you randomly scrolled through the MR list in the > same way you randomly scroll through the mailing list, would you be equally > inclined to give drive-by reviews? That's an honest hypothetical question. Initially it seemed to me that I am about the only one sticking with mailing lists. And I personally feel like a too small contributor to really try to influence your decisions too much. But these recent hand full of mails all tell me that I am not that alone. I personally did contribute to several projects during the past years. All that only in part time, thus it had to be *very* efficient for myself. And that is something that I achieved by a consistent 'interface' to all those projects. Just my widely used and highly convenient mail client. So, all that worked in a sufficiently efficient way because I could combine this kind of 'work' even with my private mail that I could handle in between with that single 'interface'. So going to any web site there is already a detour and having multiple of them for each such project gives an even longer detour. Okay today it's mostly mesa that is left as well as a communication middle end used in vizsim applications. But going away too far away from a mailing list will be mostly a loss of efficiency for me. As I said, my two cents, that should not keep you all from doing changes that finally increase your all efficiency ... best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] virgl: work around bad assumptions in virglrenderer
Good Morning,
> > One thing, may be. Do you want to add some documentation beside the
> > git log message why we do something surprising like replicating out
> > the
> > buffers and assigning new buffer indices? Just something that allows
> > a reader to get an idea why non straight forward things happen here.
> > The git annotate references to the commit messages tend to vanish
> > over time behind further changes in the code.
> >
>
> Yeah, that's a good point. This deserves a comment to clear things up a
> bit.
>
> How about something like this squashed in?
>
> ---8<---
> diff --git a/src/gallium/drivers/virgl/virgl_context.c
> b/src/gallium/drivers/virgl/virgl_context.c
> index ce72f73a0f6..2e3202b04e9 100644
> --- a/src/gallium/drivers/virgl/virgl_context.c
> +++ b/src/gallium/drivers/virgl/virgl_context.c
> @@ -399,6 +399,10 @@ static void
> *virgl_create_vertex_elements_state(struct pipe_context *ctx,
>
> for (int i = 0; i < num_elements; ++i) {
>if (elements[i].instance_divisor) {
> + /* Virglrenderer doesn't deal with instance_divisor correctly
> if
> + * there isn't a 1:1 relationship between elements and
> bindings.
> + * So let's make sure there is, by duplicating bindings.
> + */
>for (int j = 0; j < num_elements; ++j) {
> new_elements[j] = elements[j];
> new_elements[j].vertex_buffer_index = j;
> ---8<---
That's good for me.
Also the squashed get my RB.
best and Thanks
Mathias
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] virgl: work around bad assumptions in virglrenderer
Erik, On Tuesday, 11 December 2018 15:29:49 CET Erik Faye-Lund wrote: > On Tue, 2018-12-11 at 15:26 +0100, Erik Faye-Lund wrote: > > Virglrenderer does the wrong thing when given an instance divisor; > > it tries to use the element-index rather than the binding-index as > > the argument to glVertexBindingDivisor(). This worked fine as long > > as there was a 1:1 relationship between elements and bindings, > > which was the case util 19a91841c34 "st/mesa: Use Array._DrawVAO in > > st_atom_array.c.". > > > > So let's detect instance divisors, and restore a 1:1 relationship in > > that case. This will make old versions of virglrenderer behave > > correctly. For newer versions, we can consider making a better > > interface, where the instance divisor isn't specified per element, > > but rather per binding. But let's save that for another day. > > > > Signed-off-by: Erik Faye-Lund I don't exactly know what kind of coding standards and that you use in virgl. But the patch series looks good and should help for the issues we observed. One thing, may be. Do you want to add some documentation beside the git log message why we do something surprising like replicating out the buffers and assigning new buffer indices? Just something that allows a reader to get an idea why non straight forward things happen here. The git annotate references to the commit messages tend to vanish over time behind further changes in the code. > I suppose this should have had: > > Fixes: 19a91841c34 "st/mesa: Use Array._DrawVAO in st_atom_array.c." Probably at least for patch #3 and #4. With or without such comment and for the series: Reviewed-by: Mathias Fröhlich best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] st/mesa: Use Array._DrawVAO in st_atom_array.c.
Erik, > Yeah, in both D3D11 and D3D12, InstanceDataStepRate is per element, not > per input slot. So I guess this is the most flexible way of describing > this, and we'll have to duplicate the bindings in cases like these. More an answer to your below question. But I think you are giving the answer to your below question yourself. If we have have something like nine running on top of virgl, we have to assume that you get in data using DirectX semantics. The argument is just you can potentially get in data by the current 'on the virtio wire' protocol that cannot be handled correctly in virglrenderer, so fix that. OTOH, you probably want to work around that issue in virgil as well as you may find yourself with a new distribution with a new mesa running on top of an older unpatched virglrenderer. To make that work you problably have to emit something that is known to work with an old and unfixed virgelrenderer in case your lower layers on the driver stack - virglrenderer - is too old. Just replicate out the pipe_vertex_buffers for each pipe_vertex_element into the 'virtio wire' when sending that to a too old and unfixed virglrenderer. Means probably you have to fix both sides finally to make them cope with any combination of fixed and unfixed. Sorry for braking that in the first place. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] st/mesa: Use Array._DrawVAO in st_atom_array.c.
Hey, On Tuesday, 11 December 2018 10:19:47 CET Erik Faye-Lund wrote: > On Mon, 2018-12-10 at 18:23 +0100, Mathias Fröhlich wrote: > > Hi Erik, > > > > Not sure if this is our problem as I think that I only saw simple > > bindings with a zero instance divisor while debugging supertux kart. > > > > But at least I think that this is a problem in virglrenderer. The > > glVertexBindingDivisor is per binding and not per vertex attribute > > in OpenGL. > > ... you probably want to solve that differently, but for now this > > should > > quick band aid to pinpoint the problem that we observe. > > > > Does the attached patch to virglrenderer fix our problem? > > > > It does! Thanks a lot :) > > I'll find out what the proper fix is, and submit a patch in your name! > Again, thanks a lot :) You are welcome! And I don't need credits. Its a bit of a pity that the vertex element/buffer structure in gallium is different than it is in OpenGL. OTOH, does it match the way it is done in DirectX? Well, in theory I would expect virglrenderer to just cope with anything that comes potentially in. But that means that you may need to split single binding point into two bindings for OpenGL. Just assuming that they were originally from OpenGL and by that should just work is a bit too crude for my taste. But you have to decide that finally. And if you work out that split, my original one line change has nothing to do anymore with a proper solution. For me it's good that I know I can continue exploiting the VAO state. Thanks for testing and reporting!! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] st/mesa: Use Array._DrawVAO in st_atom_array.c.
Hi Erik, Not sure if this is our problem as I think that I only saw simple bindings with a zero instance divisor while debugging supertux kart. But at least I think that this is a problem in virglrenderer. The glVertexBindingDivisor is per binding and not per vertex attribute in OpenGL. ... you probably want to solve that differently, but for now this should quick band aid to pinpoint the problem that we observe. Does the attached patch to virglrenderer fix our problem? best Mathias >From 257262929cd9f42dd9c20c31bde8c171ad569a8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Fr=C3=B6hlich?= Date: Mon, 10 Dec 2018 15:34:03 +0100 Subject: [PATCH] quick-fix --- src/vrend_renderer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c index 7ed0f41..ce6f3bc 100644 --- a/src/vrend_renderer.c +++ b/src/vrend_renderer.c @@ -2338,7 +2338,7 @@ int vrend_create_vertex_elements_state(struct vrend_context *ctx, else glVertexAttribFormat(i, ve->nr_chan, ve->type, ve->norm, ve->base.src_offset); glVertexAttribBinding(i, ve->base.vertex_buffer_index); - glVertexBindingDivisor(i, ve->base.instance_divisor); + glVertexBindingDivisor(ve->base.vertex_buffer_index, ve->base.instance_divisor); glEnableVertexAttribArray(i); } } -- 2.19.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] st/mesa: Use Array._DrawVAO in st_atom_array.c.
Good Morning,
Again sorry, but since I only work here in the spare time, I did not find
enough to respond earlier.
On Tuesday, 4 December 2018 10:35:58 CET Erik Faye-Lund wrote:
> > Sounds to me like that, or even worse something with the
> > supertuxkart.
> > I have not yet understood what they are doing in detail with the
> > VAO's.
> > But I was slightly looking into the direction of mmapping the buffer
> > objects
> > and not flushing them correctly. That could potentially also lead to
> > such failures. Especially since some people observe and some not.
> > Not finally finished with investigating, but up to now I did not see
> > something
> > wrong there.
> >
>
> One more breadcrumb:
>
> Gert informed me (through other channels) that he had isolated this
> issue to only trigger for indirect draws.
>
> That might clear up a bit why this seems to happen in so few
> applications; there's probably some combination of input layouts that
> together with indirect draws become a very rare combination.
That does not directly ring a bell for me, but At least that narrows down
the issue somehow.
Gert, do you know, indirect draws with the
struct Draw{Arrays,Elements}IndirectCommand
in user space memory or in a buffer object?
> > > > Hmm, one question, on the mentioned setup on virgl. How does
> > > > glxgears render on that setup? Or alternatively how do other
> > > > OpenGL
> > > > applications different from supertuxkart on that setup?
> > >
> > > glxgears renders just fine. We'er also passing pretty much all of
> > > the
> > > OpenGL 4.3 CTS and most of piglit. Generally speaking, this doesn't
> > > trigger.
> >
> > That was my expectation as well as it took so long to find something.
> > But still, not impossible that something is wrong.
> >
> > > I just got a notice that Serious Sam 3 has a similar problem (I
> > > haven't
> > > tested this myself)... So perhaps there's some pattern that can be
> > > found?
> >
> > One observation that I saw with supertuxkart.
> > They really have a VAO that ends up with two effective bindings used
> > by 3 and 2 vertex attributes. That is the old gallium array
> > translation code
> > did produce 5 vertex_element struct entries and each of that has a
> > vertex_buffer struct assigned. The minimal pipe_vertex_buffer
> > translation
> > only happened in the old code if it could be reduced to a single
> > vertex buffer entry.
> > Now the code produces that 3 pipe_vertex_element referencing 1
> > pipe_vertex_buffer and 2 pipe_vertex_element referencing an other
> > pipe_vertex_buffer. Which should be more optimal now but is it
> > possible
> > that virgl somewhere down the road only handles the n elements to one
> > buffer
> > and the n element to n buffer case. So the question is is there a bug
> > in the n elements
> > to 1 < m < n buffer case?
> > Do you know what I mean with the effective binding?
>
> I'm not quite sure I follow here. What's n and m in this case (I seem
> to see three definitions of n, where two are similar, and none of m)?
>
> But looking through both virgl and virglrenderer, I can't spot anything
> obviously wrong with the way inputs are being set up...
Ok, thanks for looking. I meant with M:N a layout something like
pipe_vertex_elements:
{ vertex_buffer_index = 0, src_offset = 0, ...},
{ vertex_buffer_index = 0, src_offset = 12, ...},
{ vertex_buffer_index = 1, src_offset = 0, ...},
{ vertex_buffer_index = 1, src_offset = 8, ...}
pipe_vertex_buffer:
{ buffer_offset = 0, buffer.resource = },
{ buffer_offset = 0, buffer.resource = }
Finally this is a 4:2 mapping.
The previous code did only produce either a N:1 mapping like
pipe_vertex_elements:
{ vertex_buffer_index = 0, src_offset = 0, ...},
{ vertex_buffer_index = 0, src_offset = 12, ...},
{ vertex_buffer_index = 0, src_offset = 24, ...},
{ vertex_buffer_index = 0, src_offset = 36, ...}
pipe_vertex_buffer:
{ buffer_offset = 0, buffer.resource = }
this one, if there was a single buffer object used that allows this kind of
layout.
So there are *all* pipe_vertex_elements refering to a single pipe_vertex_buffer.
Or a N:N mapping like
pipe_vertex_elements:
{ vertex_buffer_index = 0, src_offset = 0, ...},
{ vertex_buffer_index = 1, src_offset = 0, ...},
{ vertex_buffer_index = 2, src_offset = 0, ...},
{ vertex_buffer_index = 3, src_offset = 0, ...}
pipe_vertex_buffer:
{ buffer_offset = 0, buffer.resource = },
{ buffer_offset = 12, buffer.resource = },
{ buffer_offset = 36, buffer.resource = },
{ buffer_offset = 24, buffer.resource = }
where you have one pipe_vertex_buffer per pipe_vertex_element.
So, If the backing driver somehow 'knew' that we could only get N:1
or N:N and not something like the 4:2 example above, we could easily
fail with the change you found.
best
Mathias
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] st/mesa: Use Array._DrawVAO in st_atom_array.c.
Good Morning, On Tuesday, 4 December 2018 13:33:29 CET Gert Wollny wrote: > Am Dienstag, den 04.12.2018, 10:35 +0100 schrieb Erik Faye-Lund: > > > > But looking through both virgl and virglrenderer, I can't spot > > anything obviously wrong with the way inputs are being set up... > > > > > May be you can observe the same type of VAO in Serious Sam 3? > > > > I don't have Serious Sam 3 running myself. Gert? > > I have not yet looked in detail, but disabling ARB_draw_indirect > doesn't help neither with SS3 nor with Shadow Warrior - at least the > latter renders fine on the r600 host, the SS3 doesn't properly start > currently, but the Talos Principle runs fine on the host (takes forever > to load in the guest so I didn't check with virgl) and this is more or > less the same engine. So, that means it's not just happening with indirect draws. Hmm ... best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] st/mesa: Use Array._DrawVAO in st_atom_array.c.
Hey, On Monday, 3 December 2018 12:15:17 CET Erik Faye-Lund wrote: > Yeah. An important thing to note is that virgl is pretty widely tested > by now, and we don't see this pop up in other places... And that sounds > a bit strange to me. Good to know, I don't actually know how wide virgl is already in use. I was surprised to find a direct knob already in fedoras version of libvirt. And I am now disappointed that after upgrading to the newest fedora on the bottom side it just crashes somewhere. I thought may be I can reproduce that with something newer that I need anyhow in the not so distant future ... So, I am currently again without a basic virgl system. > > The good side is that I set up at least what was easy to set up here, > > that is > > a fedora 29 using virgl on a fedora 28 host using an Intel skylake > > type GPU. > > > > Linux 4.19.4-300.fc29.x86_64 #1 SMP Fri Nov 23 13:03:11 UTC 2018 > > x86_64 > > [info ] IrrDriver: OpenGL version: 3.3 > > [info ] IrrDriver: OpenGL vendor: Red Hat > > [info ] IrrDriver: OpenGL renderer: virgl > > [info ] IrrDriver: OpenGL version string: 3.3 (Core Profile) Mesa > > 18.2.4 > > > > But, that just works. supertuxkart runs without any vertex corruption > > problems > > on that combination. The installed default rpm of mesa is not patched > > in any way > > that makes me suspicious regarding our problem. And git log mesa- > > 18.2.4 tells > > me that the patch you mention is included in 18.2.4. > > > > Means either I do not yet reproduce the problem correctly on the > > application side. > > Well, seems like already the initial screen to configure the track > > and that > > should show problems, which run already fine on my combination. > > > > Or we have a side effect somewhere in the complete chain down to the > > host system, which is triggered by that patch. > > > > Right. This is super-strange to me; we (Collabora) have multiple people > reproducing it independently (CC'ed Gert). What version of > virglrenderer are you using? Perhaps we have some misbehavior in newer > versions of it that was just masked without this patch? Sounds to me like that, or even worse something with the supertuxkart. I have not yet understood what they are doing in detail with the VAO's. But I was slightly looking into the direction of mmapping the buffer objects and not flushing them correctly. That could potentially also lead to such failures. Especially since some people observe and some not. Not finally finished with investigating, but up to now I did not see something wrong there. > > Hmm, one question, on the mentioned setup on virgl. How does > > glxgears render on that setup? Or alternatively how do other OpenGL > > applications different from supertuxkart on that setup? > > glxgears renders just fine. We'er also passing pretty much all of the > OpenGL 4.3 CTS and most of piglit. Generally speaking, this doesn't > trigger. That was my expectation as well as it took so long to find something. But still, not impossible that something is wrong. > I just got a notice that Serious Sam 3 has a similar problem (I haven't > tested this myself)... So perhaps there's some pattern that can be > found? One observation that I saw with supertuxkart. They really have a VAO that ends up with two effective bindings used by 3 and 2 vertex attributes. That is the old gallium array translation code did produce 5 vertex_element struct entries and each of that has a vertex_buffer struct assigned. The minimal pipe_vertex_buffer translation only happened in the old code if it could be reduced to a single vertex buffer entry. Now the code produces that 3 pipe_vertex_element referencing 1 pipe_vertex_buffer and 2 pipe_vertex_element referencing an other pipe_vertex_buffer. Which should be more optimal now but is it possible that virgl somewhere down the road only handles the n elements to one buffer and the n element to n buffer case. So the question is is there a bug in the n elements to 1 < m < n buffer case? Do you know what I mean with the effective binding? May be you can observe the same type of VAO in Serious Sam 3? Well I have confidence that this triggers something. At least debugging into the upper parts in mesa then into i965 showed nothing unexpected. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] st/mesa: Use Array._DrawVAO in st_atom_array.c.
Good Morning, On Tuesday, 27 November 2018 10:17:07 CET Erik Faye-Lund wrote: > On Tue, 2018-11-27 at 07:11 +0100, Mathias Fröhlich wrote: > > Hi Erik, > > > > > > On Monday, 26 November 2018 19:39:50 CET Erik Faye-Lund wrote: > > > > > I know this is *very* late notice, but this commit broke Super > > > > > Tux > > > > > Kart > > > > > on VirGL. Both the player-models as as well as the level data > > > > > renders > > > > > with gibberish vertex positions since this commit. > > > > > > > > > > The fix that Rob Clark did on top does not fix the problem (and > > > > > shouldn't have; VirGL doesn't use NIR). > > > > > > > > Do you have any idea how I can reproduce that issue with the > > > > least > > > > effort? > > > > > > > > > > Sadly, no. I run a qemu VM where I run super tux cart. It's a > > > rather > > > convoluted setup, I'm afraid. If you're interested in that Robert > > > Foss > > > has written an article about how to set something like this up > > > here: > > > https://memcpy.io/virtualizing-gpu-access.html > > > > > > ...But I totally understand if this is asking a bit too much. I can > > > help out with any information you need... > > > > Thanks! > > That, just means that looking into has to wait at least until the > > weekend. > > Probably even later. > > > > And thanks for looking up the constants. > > The effective binding computation depends on these and may change > > the set up combined buffer objects. So these are interesting to know. > > > > I have been putting a lot of internal verification into the code > > paths > > especially _mesa_update_vao_derived_arrays contains a larger > > #ifndef NDEBUG part that may tell us if there is something > > unexpected. > > > > I assume you did run also with asserts enabled in the build? > > Yes, I ran with asserts on, and none triggered. Ok, there should not be a problem. At least nothing that I anticipated goes wrong. > > I can observe some flicker in supertuxcart on i965. The nvidia blob > > seems > > not to flicker here. Also when running through valgrind I don't get > > that flicker > > on i965. Is that flashing - initially looked like a lighting effect > > of the game to > > me - what you observe too? > > No, the models are completely garbled. You can find some example > screenshots here: > > https://gitlab.freedesktop.org/virgl/virglrenderer/issues/59 > > > Also what are the game options? Are shaders enabled in some way? > > I'm playing with the default settings. I'm not sure what you mean with > "are shaders enabled"; VirGL is a gallium-driver, everything uses > shaders. Ok, that is about what I meant. Sometimes that goes back to a pre shader render engine behind the scenens. Yes, gallium is always shaders, but depending on the higher level render techinque used you may need different array setups with more or less vertex attributes. Thing of tangent space that you do not need for a simple renderer. So, finally that may influence the setup and trigger things. But what you report sounds pretty fundamental. The good side is that I set up at least what was easy to set up here, that is a fedora 29 using virgl on a fedora 28 host using an Intel skylake type GPU. Linux 4.19.4-300.fc29.x86_64 #1 SMP Fri Nov 23 13:03:11 UTC 2018 x86_64 [info ] IrrDriver: OpenGL version: 3.3 [info ] IrrDriver: OpenGL vendor: Red Hat [info ] IrrDriver: OpenGL renderer: virgl [info ] IrrDriver: OpenGL version string: 3.3 (Core Profile) Mesa 18.2.4 But, that just works. supertuxkart runs without any vertex corruption problems on that combination. The installed default rpm of mesa is not patched in any way that makes me suspicious regarding our problem. And git log mesa-18.2.4 tells me that the patch you mention is included in 18.2.4. Means either I do not yet reproduce the problem correctly on the application side. Well, seems like already the initial screen to configure the track and that should show problems, which run already fine on my combination. Or we have a side effect somewhere in the complete chain down to the host system, which is triggered by that patch. So, looking at that game directly on the host system, that one has flaws like the mentioned flashlight like frames that are probably missing some geometry in one way or the other. Means here: game + mesa-18.0.5 + Skylake GT2 -> fail game + mesa-18.2.4 + virgl + mesa-18.0.5 + Skylake GT2 -> works Hmm, one question, on the mentioned setup on virgl. How does glxgears render on that setup? Or alternatively how do other OpenGL applications different from supertuxkart on that setup? best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] st/mesa: Use Array._DrawVAO in st_atom_array.c.
Hi Erik, > > On Monday, 26 November 2018 19:39:50 CET Erik Faye-Lund wrote: > > > I know this is *very* late notice, but this commit broke Super Tux > > > Kart > > > on VirGL. Both the player-models as as well as the level data > > > renders > > > with gibberish vertex positions since this commit. > > > > > > The fix that Rob Clark did on top does not fix the problem (and > > > shouldn't have; VirGL doesn't use NIR). > > > > Do you have any idea how I can reproduce that issue with the least > > effort? > > > > Sadly, no. I run a qemu VM where I run super tux cart. It's a rather > convoluted setup, I'm afraid. If you're interested in that Robert Foss > has written an article about how to set something like this up here: > https://memcpy.io/virtualizing-gpu-access.html > > ...But I totally understand if this is asking a bit too much. I can > help out with any information you need... Thanks! That, just means that looking into has to wait at least until the weekend. Probably even later. And thanks for looking up the constants. The effective binding computation depends on these and may change the set up combined buffer objects. So these are interesting to know. I have been putting a lot of internal verification into the code paths especially _mesa_update_vao_derived_arrays contains a larger #ifndef NDEBUG part that may tell us if there is something unexpected. I assume you did run also with asserts enabled in the build? I can observe some flicker in supertuxcart on i965. The nvidia blob seems not to flicker here. Also when running through valgrind I don't get that flicker on i965. Is that flashing - initially looked like a lighting effect of the game to me - what you observe too? Also what are the game options? Are shaders enabled in some way? What does change if you change the game settings? May be that gives us some hints? best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 04/11] st/mesa: Use Array._DrawVAO in st_atom_array.c.
Hi, On Monday, 26 November 2018 19:39:50 CET Erik Faye-Lund wrote: > I know this is *very* late notice, but this commit broke Super Tux Kart > on VirGL. Both the player-models as as well as the level data renders > with gibberish vertex positions since this commit. > > The fix that Rob Clark did on top does not fix the problem (and > shouldn't have; VirGL doesn't use NIR). Do you have any idea how I can reproduce that issue with the least effort? Can you tell me what the virgl driver sees at Const.MaxVertexAttribRelativeOffset and Const.MaxVertexAttribStride? best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/6] Use common code for gallium array setup.
On Saturday, 24 November 2018 04:03:06 CET Marek Olšák wrote: > For the series: > > Reviewed-by: Marek Olšák Thank You! Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/6] mesa/st: Factor out array and buffer setup from st_atom_array.c.
Hi, On Friday, 23 November 2018 18:38:26 CET Chris Wilson wrote: > > I tried to reproduce this, but valgrind does not show any failures with > > drawoverhead > > on radeonsi. > > What driver backend do you use? > > iris, but we don't hit backends before the error on this path. I know, I thought may be due to different caps present in different backends. But iris is beyond my test systems. Sorry! > Thanks for checking. You're welcome! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/6] mesa/st: Factor out array and buffer setup from st_atom_array.c.
Hi Chris, On Friday, 23 November 2018 16:12:38 CET Chris Wilson wrote: > > Something to note here is that valgrind reports > (piglit/bin/drawoverhead): > > ==492== Use of uninitialised value of size 8 > ==492==at 0x6EB8FA9: cso_hash_find_node (cso_hash.c:198) > ==492==by 0x6EB9026: cso_hash_insert (cso_hash.c:213) > ==492==by 0x6EB6730: cso_set_vertex_elements (cso_context.c:1092) > ==492==by 0x714C76F: set_vertex_attribs (st_atom_array.c:384) > ==492==by 0x714C76F: st_update_array (st_atom_array.c:512) > ==492==by 0x71073F3: st_validate_state (st_atom.c:261) > ==492==by 0x70615D1: prepare_draw (st_draw.c:123) > ==492==by 0x70615D1: st_draw_vbo (st_draw.c:149) > ==492==by 0x70F5BF4: _mesa_validated_drawrangeelements (draw.c:850) > ==492==by 0x70F5BF4: _mesa_validated_drawrangeelements (draw.c:782) > ==492==by 0x70F5F32: _mesa_DrawElements (draw.c:1004) > ==492==by 0x48F6C74: stub_glDrawElements (piglit-dispatch-gen.c:12618) > ==492==by 0x10B3F4: draw (drawoverhead.c:275) > ==492==by 0x10D070: perf_measure_rate (common.c:56) > ==492==by 0x10C69E: perf_run (drawoverhead.c:645) > ==492== Uninitialised value was created by a stack allocation > ==492==at 0x714C25D: st_update_array (st_atom_array.c:389) > > from velements being used sparsely. > > Does your patch prevent this? I tried to reproduce this, but valgrind does not show any failures with drawoverhead on radeonsi. What driver backend do you use? best Mathias > -Chris ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 29/30] mesa/main: do not require float-texture filtering for es3
Hi Erik, The series looks very reasonable and I could not spot loosing any negating ! in the query logic. Even if I have not been able time wise to double checked every move when which texture format got introduced in which ES GL version. So, what can I tell now? Is that already a reviewed by? What tests did you run on the series? May be I can convince myself to go forward using this testing information ... Well - and given the amount of discussion about #30, I mean #1-#29. best Mathias On Monday, 19 November 2018 13:15:05 CET Erik Faye-Lund wrote: > The OpenGL ES 3.0 specification, table 3.13 lists half-float textures as > filterable, but not float textures. So we shouldn't depend on > ARB_float_texture, which requires full filtering support for both. > > Signed-off-by: Erik Faye-Lund > --- > src/mesa/main/version.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/mesa/main/version.c b/src/mesa/main/version.c > index 210caad097e..fb5e816db32 100644 > --- a/src/mesa/main/version.c > +++ b/src/mesa/main/version.c > @@ -509,7 +509,9 @@ compute_version_es2(const struct gl_extensions > *extensions, > extensions->ARB_internalformat_query && > extensions->ARB_map_buffer_range && > extensions->ARB_shader_texture_lod && > - extensions->ARB_texture_float && > + extensions->OES_texture_float && > + extensions->OES_texture_half_float && > + extensions->OES_texture_half_float_linear && > extensions->ARB_texture_rg && > extensions->ARB_depth_buffer_float && > /* extensions->ARB_framebuffer_object && */ > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 30/30] mesa/st: require linear interpolation for ARB_texture_float
Hi, On Monday, 19 November 2018 20:17:38 CET Roland Scheidegger wrote: > FWIW this looks like a rather similar incident to me what happened when mesa > began to verify the max vertex stride (which needs to be 2048 with GL 4.4 > whereas r600 can only do 2047) where I argued it's a much better idea to lie > about the GL version there rather than the specific vertex stride bit, but I > was rather unsuccessful and not everybody apparently shares this view... If I had fully tracked the mailing list, Roland, you would have gotten at least my +1 on the lie in the GL version instead of the max stride. The using application would still have had the chance to query the limiting value - may be with an off by one surprise wrt the standard paper, but with a value that you can finally rely on. Now with lying about the stride, the application side has no way anymore to query what the real limit is. The application just may not work correctly if really everything that you can query is within limits. For this max stride value, the only excuse is that it's highly unlikely to find these huge strides for vertex attributes, so the problem is very unlikely to show up. IMO, the applications point of view or the applications authors point of view is the one that should drive decisions. At first because this driver library has that one really major purpose to serve exactly those applications with a reliable and predictable 3d api. But what is predictable when an application cannot rely on the extensions and constants it queries? Two cents of somebody writing mostly such applications. best Mathias > > > From: mesa-dev on behalf of Ilia > Mirkin > Sent: Monday, November 19, 2018 5:37:58 PM > To: Erik Faye-Lund > Cc: ML Mesa-dev; Timothy Arceri; Emil Velikov > Subject: Re: [Mesa-dev] [PATCH 30/30] mesa/st: require linear interpolation > for ARB_texture_float > > On Mon, Nov 19, 2018 at 11:30 AM Erik Faye-Lund > wrote: > > > > On Mon, 2018-11-19 at 11:13 -0500, Ilia Mirkin wrote: > > > On Mon, Nov 19, 2018 at 10:40 AM Erik Faye-Lund > > > wrote: > > > > On Mon, 2018-11-19 at 10:02 -0500, Ilia Mirkin wrote: > > > > > Unfortunately this will drop GL 3.0 from Adreno A3xx. I think > > > > > we'd > > > > > rather fake linear interpolation with F32 textures which are > > > > > never > > > > > used than lose GL 3.0 there... > > > > > > > > Right... > > > > > > > > I guess this means that this GPU never really did support OpenGL > > > > 3.0, > > > > and will make some applications misbehave. There's definately > > > > applications out there that will lead to surprisingly bad problems > > > > when > > > > features like these are not supported. > > > > > > > > For instance if an application tries to take a local gradient by > > > > sampling a texture twice with a tiny epsilon (a common trick in > > > > tangent-free normal mapping, for instance), it will essentially get > > > > garbage, which can cause close to useless rendering. > > > > > > > > I've worked on applications that would have had problems like these > > > > if > > > > drivers report the wrong version, but could work correctly if they > > > > report the right version. > > > > > > > > Either way, I don't believe faking like that belongs in core Mesa. > > > > So > > > > if the Freedreno developers really want this kind of behavior, > > > > perhaps > > > > something like this could be a better move? > > > > > > > > ---8<--- > > > > diff --git a/src/gallium/drivers/freedreno/freedreno_screen.c > > > > b/src/gallium/drivers/freedreno/freedreno_screen.c > > > > index 88d91a91234..de811371f05 100644 > > > > --- a/src/gallium/drivers/freedreno/freedreno_screen.c > > > > +++ b/src/gallium/drivers/freedreno/freedreno_screen.c > > > > @@ -260,6 +260,11 @@ fd_screen_get_param(struct pipe_screen > > > > *pscreen, > > > > enum pipe_cap param) > > > > return 0; > > > > > > > > case PIPE_CAP_TEXTURE_FLOAT_LINEAR: > > > > + /* HACK: A330 doesn't support linear interpolation > > > > of > > > > FP32 textures, but > > > > +* to keep OpenGL 3.0 support, we lie about it > > > > here. > > > > +*/ > > > > + return is_a3xx(screen) || is_a4xx(screen) || > > > > is_a5xx(screen) || is_a6xx(screen); > > > > + > > > > case PIPE_CAP_CUBE_MAP_ARRAY: > > > > case PIPE_CAP_SAMPLER_VIEW_TARGET: > > > > case PIPE_CAP_TEXTURE_QUERY_LOD: > > > > ---8<--- > > > > > > > > Alternatively, they could ask users to override the GL-version for > > > > applications that need GL 3.0, but doesn't have problems with the > > > > lack > > > > of FP32-interpolation... > > > > > > GL 3.0 brings SO much stuff in though, and GL 3.1 brings core > > > profiles. > > > > > > Your proposed solution will also expose the OES_bla ext, which we > > > definitely don't want to do. I'd instead keep it loose. The hardware > > > that doesn't support this stuff is generally targeted
Re: [Mesa-dev] [PATCH 01/10] mesa: Rename gl_vertex_array_object::_Enabled -> Enabled.
Good Morning, On Tuesday, 20 November 2018 23:49:28 CET Marek Olšák wrote: > For the series: > > Reviewed-by: Marek Olšák Thanks! and pushed now! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 10/10] mesa: Remove unneeded bitfield widths from the VAO.
Hi Timothy, On Tuesday, 20 November 2018 12:16:58 CET Timothy Arceri wrote: > I have mixed feeling about the comment as things like that tend to get > out of date. I think this change is fine as is. Ok, so I will leave that as is. > By the way thanks for all your work cleaning all this stuff up. I > haven't been actively reviewing but I've been skimming over your various > series, things are looking much better. That goes hand in hand with having more obvious and direct algorithms that take less effort to execute. During all that changes I tried not to argue that much about execution speed as I saw questions to proof that which I don't have enough resources to do actual proofs. But I gained already up to a factor of 2 for some (closed) use cases I actually care about. Mostly due to cleaning up and tie together some loose ends. Anyhow, thanks for saying that! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 10/10] mesa: Remove unneeded bitfield widths from the VAO.
Hi, > Yes thank you. In more actively changed code I guess it would make more > sense to leave the bitfield width but as this is unlikely to change much > in future you have convinced me its probably not worth leaving it. Thanks. Oh, you mean as it would have documented what value ranges you can put into? If you like to know that, I can extend the comments also to mention the value range. So that any future introduction of a bit width does not need to check what can legally end in there ... best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 10/10] mesa: Remove unneeded bitfield widths from the VAO.
Hi, On Tuesday, 20 November 2018 09:23:53 CET Timothy Arceri wrote: > Just curious. Why is this change better? Why not just leave these as is > if it's not hurting anything? Well, you mean the unsigned char -> GLubyte? That very similar part Brian requested with patch #9. So I assume this is what he intents also. And yes there tends to be the rule that if its api visible use a gl type, if not use plain c types. Sometimes I got review requests in the past about the opposite ... I personally don't care and there is not much reason to care. Finally some of you seem to like plain c types and some others don't. Brian usually tends to GL types ... Intel people more often request plain c types ... ... is what experience tells. ... usually this is what I anticipate, this time I did not, so I got the change request :-). For dropping the bitfield widths? If this is not gaining some space to have them, I tend not to use bitfields. When using bitfields the compiler has to mask out other bits on read an may be load the previous content on write. So all together more instructions and more traffic on the cache at least. These extra instructions may not take significant time, but why do that extra work if not needed and beneficial? If we need an extra bit in that struct in the future, we can reintroduce bitfields again. So, finally bitfields potentially do hurt IMO a small bit. But they may pay off by either resolving memory pressure issues, which is something you cant really trade against potential speed gains as the weighting depends on your problem. Or they pay by a smaller cache footprint finally. Alignment improvements may be... That is all nothing guaranteed, but again my thought behind is that I cant see to further reduce the effective struct size of gl_array_attributes in a way that putting that into an array like it is used in the VAO does not again blow up due to alignment requirements of the pointer stored in there. So, why mess with the potential hit of a bitfield where there is no gain by using them ... Does that explain? best Mathias > > On 20/11/18 6:24 pm, mathias.froehl...@gmx.net wrote: > > From: Mathias Fröhlich > > > > With the current VAO layout we do not need to make these > > fields a bitfield. We get a tight struct layout with this change > > for VAO attributes. > > > > v2: Change unsigned char -> GLubyte. > > > > Reviewed-by: Brian Paul > > Signed-off-by: Mathias Fröhlich > > --- > > src/mesa/main/mtypes.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h > > index 62d3b75a36..157d45bc0b 100644 > > --- a/src/mesa/main/mtypes.h > > +++ b/src/mesa/main/mtypes.h > > @@ -1439,7 +1439,7 @@ struct gl_array_attributes > > /** Stride as specified with gl*Pointer() */ > > GLshort Stride; > > /** Index into gl_vertex_array_object::BufferBinding[] array */ > > - unsigned BufferBindingIndex:6; > > + GLubyte BufferBindingIndex; > > > > /** > > * Derived effective buffer binding index > > @@ -1454,7 +1454,7 @@ struct gl_array_attributes > > * Note that _mesa_update_vao_derived_arrays is called when binding > > * the VAO to Array._DrawVAO. > > */ > > - unsigned _EffBufferBindingIndex:6; > > + GLubyte _EffBufferBindingIndex; > > /** > > * Derived effective relative offset. > > * > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 09/10] mesa: Factor out struct gl_vertex_format.
Hi Brian, On Sunday, 18 November 2018 20:54:50 CET Brian Paul wrote: > The series looks great. Just a few minor things below. > > Reviewed-by: Brian Paul Thank you! For reference, the requested changes in the following mail. I have also used GLubyte for Patch #10 then. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] util: use *unsigned* ints for bit operations
Good Morning, I can confirm that the new build gcc-8.2.1-5 from jakub already available in *-testing fixes the problem for me. Thanks!! Mathias On Saturday, 3 November 2018 15:31:32 CET Mathias Fröhlich wrote: > Hi, > > > > Before filing a bug report at gcc I wanted to verify that we are not > > > doing anything > > > wrong like with aliasing for example. Which is the reason the bug is not > > > filed yet. > > > > FYI I filed a bug in fedora, and Jakub tracked it down and is working > > it upstream at: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87859 > > Thanks a lot!!! > > Mathias > > > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] util: use *unsigned* ints for bit operations
Hi, > > Before filing a bug report at gcc I wanted to verify that we are not doing > > anything > > wrong like with aliasing for example. Which is the reason the bug is not > > filed yet. > > FYI I filed a bug in fedora, and Jakub tracked it down and is working > it upstream at: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87859 Thanks a lot!!! Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] util: use *unsigned* ints for bit operations
Hi,
On Friday, 2 November 2018 06:22:13 CET Dave Airlie wrote:
> On Tue, 23 Oct 2018 at 10:57, Eric Anholt wrote:
> >
> > Eric Engestrom writes:
> >
> > > Fixes errors thrown by GCC's Undefined Behaviour sanitizer (ubsan) every
> > > time this macro is used.
>
> This seems to be causing problems for me here on gcc8 (8.0.1 and
> 8.2.1) in Fedora 28.
>
> ./bin/texelFetch fs usampler2DMS 2
>
> is failing here with:
> Failed to compile fragment shader: 0:6(1): error: invalid input layout
> qualifier used
>
> gcc is probably broken, but we might want to revert his from the tree
> and upcoming release until we can figure it out.
I have been experiencing something similar on fedora 28.
The observation is that gcc-8.2.1-3 works correct and gcc-8.2.1-4 does
show this problem. So for myself I have been nailing down the rpm version
of the gcc packet not to update. That helps as first aid but is not exactly
nice.
There is now also a reduced source that does not require a full mesa compile
to ease a bug report. The file is attached here. You need -O2 to make it fail.
Before filing a bug report at gcc I wanted to verify that we are not doing
anything
wrong like with aliasing for example. Which is the reason the bug is not filed
yet.
Now, I don't find time to put more work into that at the weekend and
probably also not next week. So if anybody of you wants accelerate the process,
double checks that we don't do something wrong and hand that over to gcc
I would not mind!
There is also an observation that the new gcc rpm includes the fix for gcc
bug 87137 that actually turns out to be a bitfield layouting problem.
If fedora wants to experiment with a fedora only fix its probably a good
starting point to check if omitting the corresponding patch for bug 87137
does actually help on linux. Well that is pure speculation from my side.
... so far the dump from my side on that problem.
best
Mathias#include
#include
#define BITSET_WORD unsigned int
#define BITSET_WORDBITS (sizeof (BITSET_WORD) * 8)
#define BITSET_WORDS(bits) (((bits) + BITSET_WORDBITS - 1) / BITSET_WORDBITS)
#define BITSET_EQUAL(x, y) (std::memcmp( (x), (y), sizeof (x) ) == 0)
#define DECLARE_BITSET_T(T, N) struct T { \
\
T & \
operator=(int x) \
{ \
const T c = {{ (BITSET_WORD)x }}; \
return *this = c; \
} \
\
friend bool \
operator==(const T , const T )\
{ \
return BITSET_EQUAL(b.words, c.words); \
} \
\
friend bool \
operator!=(const T , const T )\
{ \
return !(b == c); \
} \
\
friend bool \
operator==(const T , int x) \
{ \
const T c = {{ (BITSET_WORD)x }}; \
return b == c; \
} \
\
friend bool \
operator!=(const T , int x) \
{ \
return !(b == x); \
} \
\
friend T \
operator~(const T ) \
{ \
T c; \
for (unsigned i = 0; i < BITSET_WORDS(N); i++) \
c.words[i] = ~b.words[i]; \
return c; \
} \
Re: [Mesa-dev] [PATCH] mesa: Remove needless indirection in some draw functions.
Hi Brian, On Thursday, 1 November 2018 17:02:38 CET Brian Paul wrote: > Reviewed-by: Brian Paul Thanks! there is more in the queue. Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: Fix eglentrypoint.h sort order.
Hi, On Thursday, 1 November 2018 17:28:48 CET Eric Engestrom wrote: > Pushed now, but travis still fails: > https://travis-ci.org/mesa3d/mesa/jobs/449405406 > > I'm really confused here, because the LANG is now fixed, so it shouldn't > behave differently on different environments? > > If anyone has any clue, I'm all ears :) May be LC_ALL=C like the sort man page claims or as Michel meant, LC_COLLATE=C which is finally the last specific variable to overwrite the behavior? > > Do I also remember right that the reson for sorting was a binary > > search being done on the names for getprocaddress or something? > > That's exactly right :) > > > So, the ascii table sorting matters, right? > > Not sure what you mean? Are you talking about this issue, or something > else? If the binary search is the reason behind then all is answered. Thanks! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: Fix eglentrypoint.h sort order.
Hi Michel, > > There seems to be a failure with the latest patches you pushed. > > The patch fixes a failure with the sort order in egl symbols. > > Well, now it's broken with other locales. :) > > I guess src/egl/egl-entrypoint-check itself should make sure sort runs > with locale settings resulting in the expected order. I was just about to send something, when Eric sent something. Out of curiosity, which locale does invert the capitals with respect to ascii number ordering? Do I also remember right that the reson for sorting was a binary search being done on the names for getprocaddress or something? So, the ascii table sorting matters, right? > BTW, this kind of commentary should be after the --- line, otherwise > it's considered part of the commit log by Git tools. Ok, thanks! Well I should have mentioned, that I usually check if git just rebases fine on a branch when it is pushed. And that sent series did not get just eaten up by the pushed variant on rebase. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] egl: fix entrypoint sorting test
Hi, On Thursday, 1 November 2018 16:35:30 CET Eric Engestrom wrote: > Fixes: 68dc591af16ebb36814e "egl: Fix eglentrypoint.h sort order." > Cc: Emil Velikov > Cc: Mathias Fröhlich > Cc: Tapani Pälli > Signed-off-by: Eric Engestrom > --- > src/egl/egl-entrypoint-check | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/egl/egl-entrypoint-check b/src/egl/egl-entrypoint-check > index 36ee5c615306708d2784..bb7d6a777a71be5842fc 100755 > --- a/src/egl/egl-entrypoint-check > +++ b/src/egl/egl-entrypoint-check > @@ -7,5 +7,5 @@ then > fi > > entrypoints=$(grep EGL_ENTRYPOINT "$srcdir"/main/eglentrypoint.h) > -sorted=$(sort <<< "$entrypoints") > +sorted=$(LANG=C sort <<< "$entrypoints") > test "$entrypoints" = "$sorted" > Of course: Reviewed-by: Mathias Fröhlich Thanks! Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallium/util: don't let children of fork & exec inherit our thread affinity
Hi, On Thursday, 1 November 2018 11:04:27 CET Pekka Paalanen wrote: > On Wed, 31 Oct 2018 16:41:47 -0400 > Marek Olšák wrote: > > > On Wed, Oct 31, 2018 at 11:26 AM Michel Dänzer wrote: > > > > > On 2018-10-31 12:39 a.m., Gustaw Smolarczyk wrote: > > > > śr., 31 paź 2018 o 00:23 Marek Olšák napisał(a): > > ... > > > > >> As far as we know, it hurts *only* Blender. > > > > > > Why are you still saying that in the light of > > > https://bugs.freedesktop.org/108330 ? > > > > > > > Another candidate for a drirc setting. > > Isn't GTK 4 going to use the GPU (OpenGL or Vulkan) for the UI > rendering? > > If GTK 4 uses OpenGL, would you not need to blacklist all GTK 4 apps, > because there would be a high chance that GTK initializes OpenGL before > the app creates any worker threads? And these would be all kinds of GTK > apps that divide work over multiple cores, not limited to what one > would call GL apps or games. > > I don't know GTK, so I hope someone corrects or verifies this. You have to expect Qt to use OpenGL for gui rendering. Not the only implementation, but the one that Qt is heading for in most cases. IMO think the side effects of binding the thread making the context current at a cpu is too bad. As an application developer I would not expect that to happen. You can easily bind all *internal* threads that are never visible to any calling code to whatever you want but not a thread that is under the control of the application. Only slightly related, but related: Linux does some kind of numa aware scheduling, that is it tries to keep tasks on that numa node where most of the physical memory referenced by the task is directly attached to. Well at least kind of something like that. So, one question, does amdgpu try to allocate cpu memory on the cpu that has the pcie lanes of the gpu directly attached to? As a side effect of that, you would probably observe numa scheduling pulling the task back to the node where most of the memory is residing. That is, by designed coincidence, the same cpu. Is something like that exploitable for the problem to be solved? There are sched_domains in the kernel that can tell something about the topology of the cpus in the system. My be there is a way to assign a 'prefered sched_domain' to a task internally? ... I have not looked into the kernel side apis for some time. May be somebody from the scheduler guys can easily provide something like that? best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 12/14] mesa/vbo: Move src/mesa/vbo/vbo_exec_array.c -> src/mesa/main/draw.c
Hi Brian,
On Tuesday, 30 October 2018 13:06:50 CET Brian Paul wrote:
> The series looks great, Mathias.
>
> Reviewed-by: Brian Paul
Thanks for that one!
> > @@ -2072,7 +2073,7 @@ vbo_initialize_exec_dispatch(const struct gl_context
> > *ctx,
> > void GLAPIENTRY
> > _mesa_DrawArrays(GLenum mode, GLint first, GLsizei count)
> > {
> > - vbo_exec_DrawArrays(mode, first, count);
> > + _mesa_exec_DrawArrays(mode, first, count);
> > }
>
> It would be nice to avoid this pattern of simply calling another
> function like this.
>
> The complication, though, is that the _mesa_exec_Draw*() functions get
> put into the dispatch table, but the _mesa_Draw*() functions are
> decorated with GLAPIENTRY since they're Windows dll export functions.
>
> We'd have to decorate some entries in the dispatch table with
> GLAPIENTRY, but that could involve some messy python script work.
Looking at that, at least both functions _mesa_DrawArrays and
vbo_exec_DrawArrays/_mesa_exec_DrawArrays have the GLAPIENTRY.
It's probably the pattern for all of these.
May be I don't quite understand, but to me that looks just like a naming
issue that got solved at one point by calling one function from an other?
And don't all dispatch table functions have the GLAPIENTRY as well as
the GLvertexformat functions?
> What do you think?
Now confused ...
I think it's just a level of indirection that was introduced for a reason
that went away at one point in the past. May be the reason was just the naming
that the vbo module used to have these vbo_ prefixes that are changed by this
patch?
... I still suspect that I have missed something obvious from your question -
sorry.
best
Mathias
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 09/14] vbo: Make no_current_update an argument to vbo_save_NotifyBegin.
Good Morning,
On Tuesday, 30 October 2018 13:24:50 CET Eric Engestrom wrote:
> > @@ -1294,7 +1294,7 @@ static void GLAPIENTRY
> > _save_OBE_Rectf(GLfloat x1, GLfloat y1, GLfloat x2, GLfloat y2)
> > {
> > GET_CURRENT_CONTEXT(ctx);
> > - vbo_save_NotifyBegin(ctx, GL_QUADS);
> > + vbo_save_NotifyBegin(ctx, GL_QUADS, true);
>
> I won't pretend to understand any of this code, but logic-wise I think
> this should be `false`, not `true`.
> If the change was on purpose, maybe it belongs in a separate patch with
> an explanation?
Good catch! That was actually not intended!
I have changed that locally to false!
Thanks
Mathias
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 02/14] vbo: Remove the VBO_SAFE_FALLBACK flag.
Hi, > > On finishing a display list playback the VBO_SAFE_FALLBACK bit > > > > s/SAFE/SAVE/g (here and in the title) > > Regards, > Gustaw Smolarczyk Thanks, changed locally ... best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/7] EGLDevice, take 2.1
Hi Emil,
On Wednesday, 3 October 2018 16:02:40 CEST Emil Velikov wrote:
> This re-spin of the series includes:
> - correct flipped asserts
> - cosmetic wording/comment fixes
> - drop EGL_EXT_platform_device patches (swrast is broken)
> - add the EGL_MESA_device_software spec patch
>
> At this point we should be pretty much set, so any formal Ack/Rb will
> be appreciated.
Sure and sorry for the long delay!
Patch #1-#6 is:
Reviewed-by: Mathias Fröhlich
The #7 is already reviewed by Dylan, so the series should be complete now.
Thanks for iterating on that.
And looking forward to the rest of the patches enabling display
creation from a device!!
best
Mathias
>
> Thanks
> Emil
>
> Cc: Adam Jackson
> Cc: Eric Engestrom
> Cc: Mathias Fröhlich
>
> Adam Jackson (1):
> specs: Add EGL_MESA_device_software
>
> Emil Velikov (6):
> egl: add base EGL_EXT_device_base implementation
> egl: add EGL_MESA_device_software support
> egl: add EGL_EXT_device_drm support
> egl: set the EGLDevice when creating a display
> egl: enable EGL_EXT_device_{base,enumeration,query}
> meson: egl: group dri2 bits separately from haiku
>
> docs/specs/EGL_MESA_device_software.txt | 82 +
> src/egl/Makefile.sources| 2 +
> src/egl/drivers/dri2/egl_dri2.h | 1 +
> src/egl/drivers/dri2/platform_android.c | 9 +
> src/egl/drivers/dri2/platform_drm.c | 9 +
> src/egl/drivers/dri2/platform_surfaceless.c | 10 +-
> src/egl/drivers/dri2/platform_wayland.c | 18 ++
> src/egl/drivers/dri2/platform_x11.c | 27 ++
> src/egl/drivers/haiku/egl_haiku.cpp | 8 +
> src/egl/main/eglapi.c | 64
> src/egl/main/egldevice.c| 319
> src/egl/main/egldevice.h| 89 ++
> src/egl/main/egldisplay.h | 1 +
> src/egl/main/eglentrypoint.h| 4 +
> src/egl/main/eglglobals.c | 12 +-
> src/egl/main/eglglobals.h | 2 +
> src/egl/main/egltypedefs.h | 2 +
> src/egl/meson.build | 73 ++---
> 18 files changed, 692 insertions(+), 40 deletions(-)
> create mode 100644 docs/specs/EGL_MESA_device_software.txt
> create mode 100644 src/egl/main/egldevice.c
> create mode 100644 src/egl/main/egldevice.h
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 8/8] egl: add EGL_platform_device support
Hi Emil, Ok, thanks for picking that up. On Tuesday, 2 October 2018 12:23:30 CEST Emil Velikov wrote: > On Thu, 20 Sep 2018 at 15:13, Mathias Fröhlich > wrote: > > > > > If I replace the above with > > > > EGLint surface_type = 0; > > /* Only advertise pbuffer configs for non swrast devices */ > > if (dri2_dpy->image_driver) > > surface_type = EGL_PBUFFER_BIT; > > > > dri2_conf = dri2_add_config(dpy, dri2_dpy->driver_configs[i], > >config_count + 1, surface_type, NULL, > >visuals[j].rgba_masks); > > > > then I can easily prohibit the crash that I mentioned when trying to > > create a pbuffer surface on the swrast device. > > At least I do no longer get a valid pbuffer config from eglChooseConfig > > and without that we cannot reach the crashing > > dri2_dpy->image_driver->createNewDrawable > > call somewhere from eglCreatePbufferSurface anymore. > > > > Still getting a surface less context on swrast should work... > > > Issue is that we do not know how to create a "pbuffer only" swrast. > > Hence one resolves to hacks like the ones we have in platform_surfaceless. > Effectively pilling hacks upon hacks - see swrast_loader_extensions > and "software path w/o DRM.". > > That said, I'm working on proper solution but since it will take some > time to finish/merge I'll drop this and 7/8 for now. That's fine too. What I wanted to avoid is that people using the extension correctly fail with a crash in the application when running on mesa. Think about it. Thats really bad from an application writers point of view as you do all right with checking extensions and that and then the opengl stack suddenly crashes. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/8] egl: add EGL_EXT_device_drm support
Hi Emil, On Tuesday, 2 October 2018 11:59:23 CEST Emil Velikov wrote: > > > + return dev->device->nodes[DRM_NODE_PRIMARY]; > > ... we probably want > > return _eglGetDRMDeviceRenderNode(dev); > > > That isn't quite possible, as discussed in 2016's thread > "EGL_EXT_*_drm - primary vs render node". > > The extensions is (was?) not too clear that a card node must be > returned, yet there are applications depend on it. Hmm, I feared that I get back something like that when I wrote the comment. Then let it be it. What happens here is that your piglit test that takes the file returned here, opens it and wants to create a new display from it fails on my test system. ... obviously... > As mentioned in said thread we could add another extension which adds > support for EGL_DRM_RENDER_DEVICE_FILE_EXT. > But I'd suggest keeping that as a follow-up - hence the comment above > _eglGetDRMDeviceRenderNode() ... but yes. Probably the better approach then. It's more like making all tests pass. May be we should not fail the test of opening the file from there is failing because of permissions. Anyhow. That's fine then. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 8/8] egl: add EGL_platform_device support
Hi Emil,
some comments below:
On Tuesday, 4 September 2018 20:33:05 CEST Emil Velikov wrote:
> From: Emil Velikov
>
> This new 'platform' is added by default with no guards.
>
> It is effectively a copy of the surfaceless one, with updated function
> names and brand new probe function.
>
> Due to the reuse, some of the ifdef HAVE_SURFACELESS_PLATFORM guards
> have been dropped.
>
> A worthy mention are the changes in _egFindDisplay, since the original
> and dup'd fd are required, we make use of the plat_opt argument.
>
> Note that no hacks for eglGetDisplay are added - the API works only with
> the eglGetPlatformDisplay* API.
>
> v2:
> - s/_eglCompareDeviceDisplay/_eglSameDeviceDisplay/ (Eric)
> - let ^^ return bool (Eric)
> - fixup meson build, move files() further up (Eric)
> - copy from plat. surfaceless w/o the visual cleanups
> - close and free when destroying the dpy
> - sprinkle a few _eglDeviceSupports
> - split fd handling into separate function
> - use directly the render node if no FD is given (Mathias)
>
> Signed-off-by: Emil Velikov
> ---
> TODO/XXX:
> - fold/reuse more of surfaceless
> - swrast is hacky/broken - needs some DRI changes that I'm working atm.
> - remove local define
> ---
> src/egl/Makefile.am| 3 +
> src/egl/drivers/dri2/egl_dri2.c| 3 +
> src/egl/drivers/dri2/egl_dri2.h| 13 +-
> src/egl/drivers/dri2/platform_device.c | 398 +
> src/egl/main/eglapi.c | 13 +-
> src/egl/main/egldisplay.c | 112 ++-
> src/egl/main/egldisplay.h | 11 +
> src/egl/main/eglglobals.c | 1 +
> src/egl/meson.build| 1 +
> 9 files changed, 543 insertions(+), 12 deletions(-)
> create mode 100644 src/egl/drivers/dri2/platform_device.c
>
> diff --git a/src/egl/Makefile.am b/src/egl/Makefile.am
> index bf7f87015b7..6699f4ea003 100644
> --- a/src/egl/Makefile.am
> +++ b/src/egl/Makefile.am
> @@ -107,6 +107,9 @@ if HAVE_PLATFORM_SURFACELESS
> dri2_backend_FILES += drivers/dri2/platform_surfaceless.c
> endif
>
> +# Unconditionally enable platform_device
> +dri2_backend_FILES += drivers/dri2/platform_device.c
> +
> if HAVE_PLATFORM_ANDROID
> AM_CFLAGS += $(ANDROID_CFLAGS)
> libEGL_common_la_LIBADD += $(ANDROID_LIBS)
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index c5fa935657e..7c2521f120f 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -904,6 +904,9 @@ dri2_initialize(_EGLDriver *drv, _EGLDisplay *disp)
> case _EGL_PLATFORM_SURFACELESS:
>ret = dri2_initialize_surfaceless(drv, disp);
>break;
> + case _EGL_PLATFORM_DEVICE:
> + ret = dri2_initialize_device(drv, disp);
> + break;
> case _EGL_PLATFORM_X11:
>ret = dri2_initialize_x11(drv, disp);
>break;
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index 4abe1ba1952..1a08b2dfdcc 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -330,10 +330,10 @@ struct dri2_egl_surface
> } color_buffers[3], *back;
> #endif
>
> -#if defined(HAVE_SURFACELESS_PLATFORM)
> - __DRIimage *front;
> - unsigned int visual;
> -#endif
> + /* surfaceless and device */
> + __DRIimage *front;
> + unsigned int visual;
> +
> int out_fence_fd;
> EGLBoolean enable_out_fence;
> };
> @@ -489,6 +489,11 @@ dri2_initialize_surfaceless(_EGLDriver *drv, _EGLDisplay
> *disp)
> }
> #endif
>
> +EGLBoolean
> +dri2_initialize_device(_EGLDriver *drv, _EGLDisplay *disp);
> +static inline void
> +dri2_teardown_device(struct dri2_egl_display *dri2_dpy) { /* noop */ }
> +
> void
> dri2_flush_drawable_for_swapbuffers(_EGLDisplay *disp, _EGLSurface *draw);
>
> diff --git a/src/egl/drivers/dri2/platform_device.c
> b/src/egl/drivers/dri2/platform_device.c
> new file mode 100644
> index 000..15d69cedfd0
> --- /dev/null
> +++ b/src/egl/drivers/dri2/platform_device.c
> @@ -0,0 +1,398 @@
> +/*
> + * Mesa 3-D graphics library
> + *
> + * Copyright 2018 Collabora
> + *
> + * Based on platform_surfaceless, which has:
> + *
> + * Copyright (c) 2014 The Chromium OS Authors.
> + * Copyright © 2011 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included
> + * in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS
Re: [Mesa-dev] [PATCH v2 3/8] egl: add EGL_EXT_device_drm support
Hi Emil,
There are some comments inline below:
On Tuesday, 4 September 2018 20:33:00 CEST Emil Velikov wrote:
> From: Emil Velikov
>
> Add implementation based around the drmDevice API. As such it's only
> available only when building with libdrm. With the latter already a
> requirement when using !SW code paths in the platform code.
>
> Note: the current code will work if a device is hot-plugged. Yet
> hot-unplugged is not implemented, since I have no ways of testing it.
>
> v2:
> - ddd some _eglDeviceSupports checks
> - require DRM_NODE_RENDER
> - add _eglGetDRMDeviceRenderNode helper
>
> Signed-off-by: Emil Velikov
> ---
> src/egl/main/egldevice.c | 113 +++
> src/egl/main/egldevice.h | 4 ++
> 2 files changed, 117 insertions(+)
>
> diff --git a/src/egl/main/egldevice.c b/src/egl/main/egldevice.c
> index 1d67fd71191..6bd584e025b 100644
> --- a/src/egl/main/egldevice.c
> +++ b/src/egl/main/egldevice.c
> @@ -25,10 +25,14 @@
> *
> **/
>
> +#ifdef HAVE_LIBDRM
> +#include
> +#endif
> #include "util/macros.h"
>
> #include "eglcurrent.h"
> #include "egldevice.h"
> +#include "egllog.h"
> #include "eglglobals.h"
> #include "egltypedefs.h"
>
> @@ -39,6 +43,11 @@ struct _egl_device {
> const char *extensions;
>
> EGLBoolean MESA_device_software;
> + EGLBoolean EXT_device_drm;
> +
> +#ifdef HAVE_LIBDRM
> + drmDevicePtr device;
> +#endif
> };
>
> void
> @@ -60,6 +69,10 @@ _eglFiniDevice(void)
>dev = dev_list;
>dev_list = dev_list->Next;
>
> +#ifdef HAVE_LIBDRM
> + assert(!_eglDeviceSupports(dev_list, _EGL_DEVICE_DRM));
You probably mean dev instead of dev_list also the ! is probably wrong:
assert(_eglDeviceSupports(dev, _EGL_DEVICE_DRM));
> + drmFreeDevice(>device);
> +#endif
>free(dev);
> }
>
> @@ -87,6 +100,55 @@ _EGLDevice _eglSoftwareDevice = {
> .MESA_device_software = EGL_TRUE,
> };
>
> +#ifdef HAVE_LIBDRM
> +/*
> + * Negative value on error, zero if newly added, one if already in list.
> + */
> +static int
> +_eglAddDRMDevice(drmDevicePtr device, _EGLDevice **out_dev)
> +{
> + _EGLDevice *dev;
> +
> + if ((device->available_nodes & (1 << DRM_NODE_PRIMARY |
> + 1 << DRM_NODE_RENDER)) == 0)
> + return -1;
> +
> + dev = _eglGlobal.DeviceList;
> +
> + /* The first device is always software */
> + assert(!dev);
> + assert(!_eglDeviceSupports(dev, _EGL_DEVICE_SOFTWARE));
> +
> + while (dev->Next) {
> + dev = dev->Next;
> +
> + assert(!_eglDeviceSupports(dev_list, _EGL_DEVICE_DRM));
That one does not compile with assertions enabled.
There is no dev_list.
Also the ! is probably wrong.
> + if (drmDevicesEqual(device, dev->device) != 0) {
> + if (out_dev)
> +*out_dev = dev;
> + return 1;
> + }
> + }
> +
> + dev->Next = calloc(1, sizeof(_EGLDevice));
> + if (!dev->Next) {
> + if (out_dev)
> + *out_dev = NULL;
> + return -1;
> + }
> +
> + dev = dev->Next;
> + dev->extensions = "EGL_EXT_device_drm";
> + dev->EXT_device_drm = EGL_TRUE;
> + dev->device = device;
> +
> + if (out_dev)
> + *out_dev = dev;
> +
> + return 0;
> +}
> +#endif
> +
> /* Adds a device in DeviceList, if needed for the given fd.
> *
> * If a software device, the fd is ignored.
> @@ -105,7 +167,21 @@ _eglAddDevice(int fd, bool software)
> if (software)
>goto out;
>
> +#ifdef HAVE_LIBDRM
> + drmDevicePtr device;
> +
> + if (drmGetDevice2(fd, 0, ) != 0) {
> + dev = NULL;
> + goto out;
> + }
> +
> + /* Device is not added - error or already present */
> + if (_eglAddDRMDevice(device, ) != 0)
> + drmFreeDevice();
> +#else
> + _eglLog(_EGL_FATAL, "Driver bug: Built without libdrm, yet looking for HW
> device");
> dev = NULL;
> +#endif
>
> out:
> mtx_unlock(_eglGlobal.Mutex);
> @@ -118,12 +194,26 @@ _eglDeviceSupports(_EGLDevice *dev, _EGLDeviceExtension
> ext)
> switch (ext) {
> case _EGL_DEVICE_SOFTWARE:
>return dev->MESA_device_software;
> + case _EGL_DEVICE_DRM:
> + return dev->EXT_device_drm;
> default:
>assert(0);
>return EGL_FALSE;
> };
> }
>
> +/* Ideally we'll have an extension which passes the render node,
> + * instead of the card one + magic.
> + *
> + * Then we can move this in _eglQueryDeviceStringEXT below. Until then
> + * keep it separate.
> + */
> +const char *
> +_eglGetDRMDeviceRenderNode(_EGLDevice *dev)
> +{
> + return dev->device->nodes[DRM_NODE_RENDER];
> +}
> +
> EGLBoolean
> _eglQueryDeviceAttribEXT(_EGLDevice *dev, EGLint attribute,
> EGLAttrib *value)
> @@ -141,6 +231,12 @@ _eglQueryDeviceStringEXT(_EGLDevice *dev, EGLint name)
> switch (name) {
> case EGL_EXTENSIONS:
>return dev->extensions;
> +#ifdef HAVE_LIBDRM
> +
Re: [Mesa-dev] [PATCH v2 2/8] egl: add EGL_MESA_device_software support
Hi Emil,
Some comments inline below:
On Tuesday, 4 September 2018 20:32:59 CEST Emil Velikov wrote:
> From: Emil Velikov
>
> Add a plain software device, which is always available.
>
> We can safely assign it as the first/initial device in _eglGlobals,
> although we ensure that's the case with a handful of _eglDeviceSupports
> checks throughout the code.
>
> v2:
> - s/_eglFindDevice/_eglAddDevice/ (Eric)
> - s/_eglLookupAllDevices/_eglRefreshDeviceList/ (Eric)
> - move ^^ helpers into a earlier patch (Eric, Mathias)
> - set the SW device on _eglGlobal init. (Eric)
> - add a number of _eglDeviceSupports checks (Mathias)
> - split Device/Display attach to a separate patch
>
> Signed-off-by: Emil Velikov
> ---
> src/egl/main/egldevice.c | 27 +++
> src/egl/main/egldevice.h | 4 +++-
> src/egl/main/eglglobals.c | 1 +
> 3 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/src/egl/main/egldevice.c b/src/egl/main/egldevice.c
> index bbc9f2060d1..1d67fd71191 100644
> --- a/src/egl/main/egldevice.c
> +++ b/src/egl/main/egldevice.c
> @@ -37,6 +37,8 @@ struct _egl_device {
> _EGLDevice *Next;
>
> const char *extensions;
> +
> + EGLBoolean MESA_device_software;
> };
>
> void
> @@ -47,6 +49,12 @@ _eglFiniDevice(void)
> /* atexit function is called with global mutex locked */
>
> dev_list = _eglGlobal.DeviceList;
> +
> + /* The first device is on-stack allocated SW device */
May be I name that wrong as non native english, but 'on-stack'
would be something different for me. The stack is more or less
the function local allocation scope.
The sw device is much more a 'static allocated SW device'.
Right?
The following asserts just flag in some of my tests:
> + assert(!dev_list);
> + assert(!_eglDeviceSupports(dev_list, _EGL_DEVICE_SOFTWARE));
Looking at the comment above I assume you mean:
assert(dev_list);
assert(_eglDeviceSupports(dev_list, _EGL_DEVICE_SOFTWARE));
> + dev_list = dev_list->Next;
> +
> while (dev_list) {
>/* pop list head */
>dev = dev_list;
> @@ -74,6 +82,11 @@ _eglCheckDeviceHandle(EGLDeviceEXT device)
> return (cur != NULL);
> }
>
> +_EGLDevice _eglSoftwareDevice = {
> + .extensions = "EGL_MESA_device_software",
> + .MESA_device_software = EGL_TRUE,
> +};
> +
> /* Adds a device in DeviceList, if needed for the given fd.
> *
> * If a software device, the fd is ignored.
> @@ -84,6 +97,13 @@ _eglAddDevice(int fd, bool software)
> _EGLDevice *dev;
>
> mtx_lock(_eglGlobal.Mutex);
> + dev = _eglGlobal.DeviceList;
> +
> + /* The first device is always software */
> + assert(!dev);
> + assert(!_eglDeviceSupports(dev, _EGL_DEVICE_SOFTWARE));
The same two asserts with the probably inverted logic like above.
> + if (software)
> + goto out;
>
> dev = NULL;
>
> @@ -96,6 +116,8 @@ EGLBoolean
> _eglDeviceSupports(_EGLDevice *dev, _EGLDeviceExtension ext)
> {
> switch (ext) {
> + case _EGL_DEVICE_SOFTWARE:
> + return dev->MESA_device_software;
> default:
>assert(0);
>return EGL_FALSE;
> @@ -140,6 +162,11 @@ _eglRefreshDeviceList(void)
>
> dev = _eglGlobal.DeviceList;
>
> + /* The first device is always software */
> + assert(!dev);
> + assert(!_eglDeviceSupports(dev, _EGL_DEVICE_SOFTWARE));
... and again.
best
Mathias
> + count++;
> +
> return count;
> }
>
> diff --git a/src/egl/main/egldevice.h b/src/egl/main/egldevice.h
> index 72d250393f4..74b5ddeee5c 100644
> --- a/src/egl/main/egldevice.h
> +++ b/src/egl/main/egldevice.h
> @@ -38,6 +38,8 @@
> extern "C" {
> #endif
>
> +extern _EGLDevice _eglSoftwareDevice;
> +
> void
> _eglFiniDevice(void);
>
> @@ -57,7 +59,7 @@ _EGLDevice *
> _eglAddDevice(int fd, bool software);
>
> enum _egl_device_extension {
> - EGL_FOOBAR,
> + _EGL_DEVICE_SOFTWARE,
> };
>
> typedef enum _egl_device_extension _EGLDeviceExtension;
> diff --git a/src/egl/main/eglglobals.c b/src/egl/main/eglglobals.c
> index ac8bb3f328a..db81fcaf2b5 100644
> --- a/src/egl/main/eglglobals.c
> +++ b/src/egl/main/eglglobals.c
> @@ -52,6 +52,7 @@ struct _egl_global _eglGlobal =
> {
> .Mutex = &_eglGlobalMutex,
> .DisplayList = NULL,
> + .DeviceList = &_eglSoftwareDevice,
> .NumAtExitCalls = 3,
> .AtExitCalls = {
>/* default AtExitCalls, called in reverse order */
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 1/8] egl: add base EGL_EXT_device_base implementation
Hi Emil,
On Tuesday, 4 September 2018 20:32:58 CEST Emil Velikov wrote:
> From: Emil Velikov
>
> Introduce the API for device query and enumeration. Those at the moment
> produce nothing useful since zero devices are actually available.
>
> That contradicts with the spec, so the extension isn't advertised just
> yet.
>
> With later commits we'll add support for software (always) and hardware
> devices. Each one exposing the respective extension string.
>
> v2:
> - fold API boilerplate into this patch
> - move _eglAddDevice, _eglDeviceSupports, _eglRefreshDeviceList to this
> patch (Eric, Mathias)
> - make _eglFiniDevice the one called last
Thanks for the updated series.
Nevertheless, patches #1, #2, #3, #8 have comments.
Especialy there are lots of asserts that either dont compile or assert on else
working code.
You should double check what they are supposed to do.
So you will find also here some comments inline:
>
> Signed-off-by: Emil Velikov
> ---
> src/egl/Makefile.sources | 2 +
> src/egl/main/eglapi.c| 64 +
> src/egl/main/egldevice.c | 179 +++
> src/egl/main/egldevice.h | 83
> src/egl/main/egldisplay.h| 1 +
> src/egl/main/eglentrypoint.h | 4 +
> src/egl/main/eglglobals.c| 8 +-
> src/egl/main/eglglobals.h| 2 +
> src/egl/main/egltypedefs.h | 2 +
> src/egl/meson.build | 2 +
> 10 files changed, 344 insertions(+), 3 deletions(-)
> create mode 100644 src/egl/main/egldevice.c
> create mode 100644 src/egl/main/egldevice.h
>
> diff --git a/src/egl/Makefile.sources b/src/egl/Makefile.sources
> index 82f13ad3cbd..0cc5f1bbfef 100644
> --- a/src/egl/Makefile.sources
> +++ b/src/egl/Makefile.sources
> @@ -10,6 +10,8 @@ LIBEGL_C_FILES := \
> main/eglcurrent.c \
> main/eglcurrent.h \
> main/egldefines.h \
> + main/egldevice.c \
> + main/egldevice.h \
> main/egldisplay.c \
> main/egldisplay.h \
> main/egldriver.c \
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index c8c6a50f6ad..6df5e841463 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -95,6 +95,7 @@
> #include "egldisplay.h"
> #include "egltypedefs.h"
> #include "eglcurrent.h"
> +#include "egldevice.h"
> #include "egldriver.h"
> #include "eglsurface.h"
> #include "eglconfig.h"
> @@ -2575,6 +2576,69 @@ eglSetBlobCacheFuncsANDROID(EGLDisplay *dpy,
> EGLSetBlobFuncANDROID set,
> _eglUnlockDisplay(disp);
> }
>
> +static EGLBoolean EGLAPIENTRY
> +eglQueryDeviceAttribEXT(EGLDeviceEXT device,
> +EGLint attribute,
> +EGLAttrib *value)
> +{
> + _EGLDevice *dev = _eglLookupDevice(device);
> + EGLBoolean ret;
> +
> + _EGL_FUNC_START(NULL, EGL_NONE, NULL, EGL_FALSE);
> + if (!dev)
> + RETURN_EGL_ERROR(NULL, EGL_BAD_DEVICE_EXT, EGL_FALSE);
> +
> + ret = _eglQueryDeviceAttribEXT(dev, attribute, value);
> + RETURN_EGL_EVAL(NULL, ret);
> +}
> +
> +static const char * EGLAPIENTRY
> +eglQueryDeviceStringEXT(EGLDeviceEXT device,
> +EGLint name)
> +{
> + _EGLDevice *dev = _eglLookupDevice(device);
> +
> + _EGL_FUNC_START(NULL, EGL_NONE, NULL, NULL);
> + if (!dev)
> + RETURN_EGL_ERROR(NULL, EGL_BAD_DEVICE_EXT, NULL);
> +
> + RETURN_EGL_EVAL(NULL, _eglQueryDeviceStringEXT(dev, name));
> +}
> +
> +static EGLBoolean EGLAPIENTRY
> +eglQueryDevicesEXT(EGLint max_devices,
> + EGLDeviceEXT *devices,
> + EGLint *num_devices)
> +{
> + EGLBoolean ret;
> +
> + _EGL_FUNC_START(NULL, EGL_NONE, NULL, EGL_FALSE);
> + ret = _eglQueryDevicesEXT(max_devices, (_EGLDevice **) devices,
> + num_devices);
> + RETURN_EGL_EVAL(NULL, ret);
> +}
> +
> +static EGLBoolean EGLAPIENTRY
> +eglQueryDisplayAttribEXT(EGLDisplay dpy,
> + EGLint attribute,
> + EGLAttrib *value)
> +{
> + _EGLDisplay *disp = _eglLockDisplay(dpy);
> + _EGLDriver *drv;
> +
> + _EGL_FUNC_START(NULL, EGL_NONE, NULL, EGL_FALSE);
> + _EGL_CHECK_DISPLAY(disp, EGL_FALSE, drv);
> +
> + switch (attribute) {
> + case EGL_DEVICE_EXT:
> + *value = (EGLAttrib) disp->Device;
> + break;
> + default:
> + RETURN_EGL_ERROR(disp, EGL_BAD_ATTRIBUTE, EGL_FALSE);
> + }
> + RETURN_EGL_SUCCESS(disp, EGL_TRUE);
> +}
> +
> __eglMustCastToProperFunctionPointerType EGLAPIENTRY
> eglGetProcAddress(const char *procname)
> {
> diff --git a/src/egl/main/egldevice.c b/src/egl/main/egldevice.c
> new file mode 100644
> index 000..bbc9f2060d1
> --- /dev/null
> +++ b/src/egl/main/egldevice.c
> @@ -0,0 +1,179 @@
> +/**
> + *
> + * Copyright 2015, 2018 Collabora
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and
Re: [Mesa-dev] [PATCH 1/3] mesa: use GLsizeiptrARB, GLintptrARB in bufferobj.c
Hi Brian, > gl.xml doesn't seem relevant to this. In GL/glext.h we have: I thought that somewere at khronos the headers are generated from the xml. And the new headers are usually imported then together with the matching xml into mesa? > typedef khronos_ssize_t GLsizeiptr; > [...] > typedef ptrdiff_t GLsizeiptrARB; > > and in KHR/khrplatform.h: > > typedef unsigned long int khronos_uintptr_t; > > and in stdef.h: > > typedef __PTRDIFF_TYPE__ ptrdiff_t; > > And it looks like __PTRDIFF_TYPE__ is a compiler built-in. > > So, gcc is warning that __PTRDIFF_TYPE__ is not the same as unsigned > long int. Seems reasonable. Uh, ok. Thanks for the explanation! Ok, yes I see now that khronos' version of the xml file changed the data type already to khronos_uintptr_t. So, xml and headers are obviously not always imported at the same time ... Anyhow, thanks! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] mesa: use GLsizeiptrARB, GLintptrARB in bufferobj.c
Good Morning,
On Tuesday, 18 September 2018 05:10:04 CEST Brian Paul wrote:
> The function pointer declarations in dd.h for the BufferData() and
> BufferSubData() use the ARB-suffixed datatypes. This patch changes
> the buffer_data_fallback() and buffer_sub_data_fallback() functions
> to use those datatypes too.
>
> This fixes a build warning when building 32-bit libraries. Evidently,
> GLsizeiptrARB and GLsizeiptr are defined differently in that situation.
>
> All all implementations of these driver hooks use the ARB-suffixed
> types.
Hmm, all of GL{int,sizei}ptr{,ARB} are typedefs to ptrdiff_t by gl.xml.
That makes me think that you may be running into some other problem
for your build that you may want to know the reason for?
Anyhow, for mesa itself, it seems reasonable to assign functions with
exactly the same argument signature to the appropriate driver function pointers.
This one as well as the other two warning fixes:
Reviewed-by: Mathias Fröhlich
best
Mathias
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] gallium: New cap PIPE_CAP_MAX_VERTEX_ELEMENT_SRC_OFFSET.
The patches are pushed now.
Thanks for the review!
best
Mathias
On Thursday, 6 September 2018 16:42:51 CEST Brian Paul wrote:
> The series looks good to me.
>
> Reviewed-by: Brian Paul
>
> On 09/06/2018 08:31 AM, mathias.froehl...@gmx.net wrote:
> > From: Mathias Fröhlich
> >
> > Introduce a new capability for the maximum value of
> > pipe_vertex_element::src_offset. Initially just every driver
> > backend returns the value previously set from _mesa_init_constants.
> > So this shall end up in no functional change.
> >
> > Signed-off-by: Mathias Fröhlich
> > ---
> > src/gallium/auxiliary/util/u_screen.c | 3 +++
> > src/gallium/docs/source/screen.rst | 3 +++
> > src/gallium/include/pipe/p_defines.h | 1 +
> > src/mesa/state_tracker/st_extensions.c | 6 ++
> > 4 files changed, 13 insertions(+)
> >
> > diff --git a/src/gallium/auxiliary/util/u_screen.c
> > b/src/gallium/auxiliary/util/u_screen.c
> > index c41e28820b..d97c3cf5d9 100644
> > --- a/src/gallium/auxiliary/util/u_screen.c
> > +++ b/src/gallium/auxiliary/util/u_screen.c
> > @@ -318,6 +318,9 @@ u_pipe_screen_get_param_defaults(struct pipe_screen
> > *pscreen,
> > case PIPE_CAP_TEXTURE_MIRROR_CLAMP_TO_EDGE:
> > return 0;
> >
> > + case PIPE_CAP_MAX_VERTEX_ELEMENT_SRC_OFFSET:
> > + return 2047;
> > +
> > default:
> > unreachable("bad PIPE_CAP_*");
> > }
> > diff --git a/src/gallium/docs/source/screen.rst
> > b/src/gallium/docs/source/screen.rst
> > index 93415a5df1..6e5d123107 100644
> > --- a/src/gallium/docs/source/screen.rst
> > +++ b/src/gallium/docs/source/screen.rst
> > @@ -466,6 +466,9 @@ subpixel precision bias in bits during conservative
> > rasterization.
> > * ``PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS``: Maximum total
> > number of
> > atomic counter buffers. A value of 0 means the sum of all per-shader
> > stage
> > maximums (see ``PIPE_SHADER_CAP_MAX_HW_ATOMIC_COUNTER_BUFFERS``).
> > +* ``PIPE_CAP_MAX_VERTEX_ELEMENT_SRC_OFFSET``: The maximum supported value
> > for
> > + of pipe_vertex_element::src_offset.
> > +
> >
> > .. _pipe_capf:
> >
> > diff --git a/src/gallium/include/pipe/p_defines.h
> > b/src/gallium/include/pipe/p_defines.h
> > index bdd3f4680f..3a686c4cc3 100644
> > --- a/src/gallium/include/pipe/p_defines.h
> > +++ b/src/gallium/include/pipe/p_defines.h
> > @@ -821,6 +821,7 @@ enum pipe_cap
> > PIPE_CAP_MAX_COMBINED_SHADER_BUFFERS,
> > PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS,
> > PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS,
> > + PIPE_CAP_MAX_VERTEX_ELEMENT_SRC_OFFSET,
> > };
> >
> > /**
> > diff --git a/src/mesa/state_tracker/st_extensions.c
> > b/src/mesa/state_tracker/st_extensions.c
> > index 244c12595e..43ef6a40f9 100644
> > --- a/src/mesa/state_tracker/st_extensions.c
> > +++ b/src/mesa/state_tracker/st_extensions.c
> > @@ -403,6 +403,12 @@ void st_init_limits(struct pipe_screen *screen,
> > c->MaxVertexAttribStride
> > = screen->get_param(screen, PIPE_CAP_MAX_VERTEX_ATTRIB_STRIDE);
> >
> > + /* The value cannot be larger than that since
> > pipe_vertex_buffer::src_offset
> > +* is only 16 bits.
> > +*/
> > + temp = screen->get_param(screen,
> > PIPE_CAP_MAX_VERTEX_ELEMENT_SRC_OFFSET);
> > + c->MaxVertexAttribRelativeOffset = MIN2(0x, temp);
> > +
> > c->StripTextureBorder = GL_TRUE;
> >
> > c->GLSLSkipStrictMaxUniformLimitCheck =
> >
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] gallium: New cap PIPE_CAP_MAX_VERTEX_ELEMENT_SRC_OFFSET.
Hi, On Thursday, 6 September 2018 23:43:41 CEST Roland Scheidegger wrote: > Looks alright to me, albeit seems a bit weird your hw can have offset of > 255 but only max stride of 128 - max offset being larger than max stride > doesn't really make a whole lot of sense. (Could you handle stride 255 > maybe?) Yes, I have been looking at the the bitfields in the driver and the 255 is what the offset can set in terms of bits. And the stride would be 255 as well in that terms. The point here is to provide etnaviv with realtive offsets within their hardwares bounds. I will leave raising the stride value to something higher to the guys that know etnaviv better than me. Means I was wondering about the 128 too and thought it may have a reason that I don't see immediately. > I think in d3d10 these properties are intrinsically tied together, the > max struct size can be 2048, so this dictates both the max stride (2048) > as well as the max offset (2047 if it's a 8bit value). Not so > coincidentally, these values are also the required minimum values of > newer GL versions. Then again there's hw like r600 which can only > support 2047 in reality for stride, at least natively... Might even be > common that hw uses the same field width for stride as well as offsets... > > In any case, > Reviewed-by: Roland Scheidegger Thanks! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] tnl: Fix green gun regression in xonotic.
Hi Dylan,
On Thursday, 6 September 2018 18:11:55 CEST Dylan Baker wrote:
> > Fix an other regression of patch 64d2a2048054
> > mesa: Make gl_vertex_array contain pointers to first order VAO members.
> > The regression showed up with drivers using the tnl module and
> > was reproducible using xonotic-glx -benchmark demos/the-big-keybench.dem.
> >
> > This patch survives intels CI system without changes in the tests.
> >
> > Tested-by: Ville Syrjälä
> > Signed-off-by: Mathias Fröhlich
> > ---
> > src/mesa/tnl/t_split_copy.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/tnl/t_split_copy.c b/src/mesa/tnl/t_split_copy.c
> > index cbb7eb409f..085ae9a28c 100644
> > --- a/src/mesa/tnl/t_split_copy.c
> > +++ b/src/mesa/tnl/t_split_copy.c
> > @@ -531,7 +531,7 @@ replay_init(struct copy_context *copy)
> > for (offset = 0, i = 0; i < copy->nr_varying; i++) {
> >const struct tnl_vertex_array *src = copy->varying[i].array;
> >const struct gl_array_attributes *srcattr = src->VertexAttrib;
> > - struct tnl_vertex_array *dst = >dstarray[i];
> > + struct tnl_vertex_array *dst =
> > >dstarray[copy->varying[i].attr];
> >struct gl_vertex_buffer_binding *dstbind =
> > >varying[i].dstbinding;
> >struct gl_array_attributes *dstattr = >varying[i].dstattribs;
> >
>
> Hi Mathias,
>
> This patch was nominated for 18.1 via the Fixes: tag, however, it doesn't
> apply
> due to not having "mesa/vbo/tnl: Move gl_vertex_array related stuff to tnl.".
> I
> have attempted to resolve the conflicts (they're fairly trivial), but would
> appreciate if you could have a look at the commit and make sure it's correct:
> https://gitlab.freedesktop.org/mesa/mesa/commit/0807ec76b5d1d6a404670c87ecd2789b21be35c5
That one looks perfect.
I tried to recap all the fixes that I had to apply to wipe up after my VAO
changes.
From what I can see we should be good with 18.1.
Thanks!
best
Mathias
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 0/8] EGLDevice, take 2
Hi Emil,
On Tuesday, 4 September 2018 20:32:57 CEST Emil Velikov wrote:
> Hi all,
>
> Here is a re-spin of the EGLDevice series.
>
> Changelog highlights:
> - rebased on top of "egl/android: rework device probing"
> - better patch split
> - surfaceless cleanups are left to another series
> - use better names for a handful of functions
> - teardown nitpicks (completely leak-free, correct order)
> - simplify SW device handling
> - a number of DeviceSupports sanity checks
> - enable the extensions when _fully_ supported
> - directly use the render node, when no FD is given
>
> Note: SW device support is still rough - I'd strongly recommend against
> trying to use it.
>
> As always, review and feedback is greatly appreciated.
I think you want to compile and run with --enable-debug or the meson
build equivalent enabling asserts().
I did not look too close, today but applied to todays master I get some
asserts. One when compiling and at least two when running some test cases.
Anyhow thanks for updating so far!
best
Mathias
>
> Thanks
> Emil
>
>
> Emil Velikov (8):
> egl: add base EGL_EXT_device_base implementation
> egl: add EGL_MESA_device_software support
> egl: add EGL_EXT_device_drm support
> egl: set the EGLDevice when creating a display
> egl: enable EGL_EXT_device_{base,enumeration,query}
> meson: egl: group dri2 bits separately from haiku
> egl: add optional plat_opt to _eglFindDisplay()
> egl: add EGL_platform_device support
>
> src/egl/Makefile.am | 3 +
> src/egl/Makefile.sources| 2 +
> src/egl/drivers/dri2/egl_dri2.c | 3 +
> src/egl/drivers/dri2/egl_dri2.h | 14 +-
> src/egl/drivers/dri2/platform_android.c | 9 +
> src/egl/drivers/dri2/platform_device.c | 398
> src/egl/drivers/dri2/platform_drm.c | 9 +
> src/egl/drivers/dri2/platform_surfaceless.c | 10 +-
> src/egl/drivers/dri2/platform_wayland.c | 18 +
> src/egl/drivers/dri2/platform_x11.c | 27 ++
> src/egl/drivers/haiku/egl_haiku.cpp | 8 +
> src/egl/main/eglapi.c | 79 +++-
> src/egl/main/egldevice.c| 319
> src/egl/main/egldevice.h| 89 +
> src/egl/main/egldisplay.c | 124 +-
> src/egl/main/egldisplay.h | 14 +-
> src/egl/main/eglentrypoint.h| 4 +
> src/egl/main/eglglobals.c | 13 +-
> src/egl/main/eglglobals.h | 2 +
> src/egl/main/egltypedefs.h | 2 +
> src/egl/meson.build | 74 ++--
> 21 files changed, 1162 insertions(+), 59 deletions(-)
> create mode 100644 src/egl/drivers/dri2/platform_device.c
> create mode 100644 src/egl/main/egldevice.c
> create mode 100644 src/egl/main/egldevice.h
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 00/10] Let us welcome EGLDevice
Hi Emil, On Tuesday, 4 September 2018 22:16:18 CEST Emil Velikov wrote: > Aaand it out, yet I forgot to CC you :-\ Never mind. I have seen that but did not find the time so far to test and review. But that's on my list. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Etnaviv on mesa master vs. Array._DrawVAO
Hi, I have sent out a patch to introduce the capability in question as well as reducing it for etnaviv. Can you test this one if that really helps for your failure and may be review the patch? Thanks and best Mathias On Friday, 31 August 2018 12:40:01 CEST Guido Günther wrote: > Hi, > I'm looking into forward porting laanwj's patches for GC7000 support to > current mesa master. Luckily most of it already got merged last November > with mostly only the texture descriptor support missing( > https://github.com/laanwj/mesa/commit/b71802207432543745dff471c68fbc40495b4858) > > Putting this on current master leads to this assertion in mesa when running > kmscube like: > > kmscube: ../src/gallium/drivers/etnaviv/etnaviv_state.c:536: > etna_vertex_elements_state_create: Assertion `element_size != 0 && end_offset > <= 256' failed. > > Printing the value there gives: > > etna_vertex_elements_state_create:535: size: 12, offset: 0, end_offset: 12 > etna_vertex_elements_state_create:535: size: 12, offset: 576, end_offset: > 588 > > And the bt is: > >Stack trace of thread 2158: >#0 0xa93427c0 raise (libc.so.6) >#1 0xa934274c raise (libc.so.6) >#2 0xa9343bac abort (libc.so.6) >#3 0xa933bd2c n/a (libc.so.6) >#4 0xa933bdac __assert_fail (libc.so.6) >#5 0xa8de468c etna_vertex_elements_state_create > (imx-drm_dri.so) >#6 0xa8757178 u_vbuf_set_vertex_elements_internal > (imx-drm_dri.so) >#7 0xa875726c u_vbuf_set_vertex_elements (imx-drm_dri.so) >#8 0xa8711bcc cso_set_vertex_elements (imx-drm_dri.so) >#9 0xa8b28760 set_vertex_attribs (imx-drm_dri.so) >#10 0xa8b28c9c st_update_array (imx-drm_dri.so) >#11 0xa8ad4e44 st_validate_state (imx-drm_dri.so) >#12 0xa8a1e410 prepare_draw (imx-drm_dri.so) >#13 0xa8a1e488 st_draw_vbo (imx-drm_dri.so) >#14 0xa8a0ea9c vbo_draw_arrays (imx-drm_dri.so) >#15 0xa8a0f408 vbo_exec_DrawArrays (imx-drm_dri.so) >#16 0xa953f7e0 glDrawArrays (libGLESv2.so.2) >#17 0xd7e62ea0 draw_cube_smooth (kmscube) >#18 0xd7e64160 atomic_run (kmscube) >#19 0xa93306e0 __libc_start_main (libc.so.6) >#20 0xd7e621fc $x (kmscube) >#21 0xd7e621fc $x (kmscube) > > I've traced this back to this commit: > > 19a91841c347107d877bc750371c5fa4e9b4de19 is the first bad commit > commit 19a91841c347107d877bc750371c5fa4e9b4de19 > Author: Mathias Fröhlich > Date: Sun Apr 1 20:18:36 2018 +0200 > > st/mesa: Use Array._DrawVAO in st_atom_array.c. > > Finally make use of the binding information in the VAO when > setting up arrays for draw. > > v2: Emit less relocations also for interleaved userspace arrays. > > Reviewed-by: Brian Paul > Signed-off-by: Mathias Fröhlich > > And indeed commits prior to that one work as expected. Any hints what > would be the right fix to not trigger the assert? > > Cheers, > -- Guido > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] tnl: Fix green gun regression in xonotic.
Hi, On Monday, 3 September 2018 03:11:37 CEST Ian Romanick wrote: > I don't know this code very well, but this seems to better match the > original (before 64d2a204805). Since it works for Ville and passes the > Intel CI, > > Reviewed-by: Ian Romanick Thanks for looking into that! I have changed the commit message like requested. It's pushed now. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 00/10] Let us welcome EGLDevice
Hi Emil, On Tuesday, 4 September 2018 16:21:49 CEST you wrote: > > ssh different-machine.somewhere > > > > Then you will see that you are not added to the card0 acl as you are not > > logged to any console. > > Ouch, I should have noticed the "rw" for "other" of your render node. > Looking at a Arch, Debian, Fedora and Ubuntu box - they all use 0660 > for both card and render nodes. > Which would explain why your described use case was not working, here ;-) > > Out of curiosity: you change that locally or it's a distro choice - > which distro is that? I have been tracking down to systemd's git at https://github.com/systemd/systemd.git: commit 4e15a7343cb389e97f3eb4f49699161862d8b8b2 Author: Tom Stellard Date: Tue Oct 31 08:46:24 2017 -0700 udev-rules: Permission changes for /dev/dri/renderD* - Remove the uaccess tag from /dev/dri/renderD*. - Change the owning group from video to render. - Change default mode to 0666. - Add an option to allow users to set the access mode for these devices at compile time. ... which pretty much makes perfect sense to me given the idea of render nodes. ... did I mention that already? :-) I run currently on fedora-28 which runs systemd-udev-238-9.git0e0aa59.fc28.x86_64 as rpm package. > That said, v2 opens the render node directly, so it should work on your end. > Just double-checking the crash is gone and I'll send them out. Great!! best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] tnl: Fix green gun regression in xonotic.
Hi,
On Wednesday, 22 August 2018 06:57:57 CEST mathias.froehl...@gmx.net wrote:
> From: Mathias Fröhlich
>
> Hi Ville, Brian,
>
> The below patch fixes the regression to tnl drivers that Ville reported
> a hand full weeks ago.
> Please review!
Ping?
Or is the habit in mesa that a Tested-by:... is sufficient to push it?
best
Mathias
>
> best
> Mathias
>
>
> Fix an other regression of patch 64d2a2048054
> mesa: Make gl_vertex_array contain pointers to first order VAO members.
> The regression showed up with drivers using the tnl module and
> was reproducible using xonotic-glx -benchmark demos/the-big-keybench.dem.
>
> This patch survives intels CI system without changes in the tests.
>
> Tested-by: Ville Syrjälä
> Signed-off-by: Mathias Fröhlich
> ---
> src/mesa/tnl/t_split_copy.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/mesa/tnl/t_split_copy.c b/src/mesa/tnl/t_split_copy.c
> index cbb7eb409f..085ae9a28c 100644
> --- a/src/mesa/tnl/t_split_copy.c
> +++ b/src/mesa/tnl/t_split_copy.c
> @@ -531,7 +531,7 @@ replay_init(struct copy_context *copy)
> for (offset = 0, i = 0; i < copy->nr_varying; i++) {
>const struct tnl_vertex_array *src = copy->varying[i].array;
>const struct gl_array_attributes *srcattr = src->VertexAttrib;
> - struct tnl_vertex_array *dst = >dstarray[i];
> + struct tnl_vertex_array *dst =
> >dstarray[copy->varying[i].attr]; struct gl_vertex_buffer_binding
> *dstbind = >varying[i].dstbinding; struct gl_array_attributes
> *dstattr = >varying[i].dstattribs;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Etnaviv on mesa master vs. Array._DrawVAO
Hi, On Friday, 31 August 2018 12:40:01 CEST Guido Günther wrote: > Hi, > I'm looking into forward porting laanwj's patches for GC7000 support to > current mesa master. Luckily most of it already got merged last November > with mostly only the texture descriptor support missing( > https://github.com/laanwj/mesa/commit/b71802207432543745dff471c68fbc40495b48 > 58) > > Putting this on current master leads to this assertion in mesa when running > kmscube like: > > kmscube: ../src/gallium/drivers/etnaviv/etnaviv_state.c:536: > etna_vertex_elements_state_create: Assertion `element_size != 0 && > end_offset <= 256' failed. > > Printing the value there gives: > > etna_vertex_elements_state_create:535: size: 12, offset: 0, end_offset: > 12 etna_vertex_elements_state_create:535: size: 12, offset: 576, > end_offset: 588 > > And the bt is: > >Stack trace of thread 2158: >#0 0xa93427c0 raise (libc.so.6) >#1 0xa934274c raise (libc.so.6) >#2 0xa9343bac abort (libc.so.6) >#3 0xa933bd2c n/a (libc.so.6) >#4 0xa933bdac __assert_fail (libc.so.6) >#5 0xa8de468c etna_vertex_elements_state_create > (imx-drm_dri.so) #6 0xa8757178 u_vbuf_set_vertex_elements_internal > (imx-drm_dri.so) #7 0xa875726c u_vbuf_set_vertex_elements > (imx-drm_dri.so) #8 0xa8711bcc cso_set_vertex_elements > (imx-drm_dri.so) #9 0xa8b28760 set_vertex_attribs (imx-drm_dri.so) >#10 0xa8b28c9c st_update_array (imx-drm_dri.so) >#11 0xa8ad4e44 st_validate_state (imx-drm_dri.so) >#12 0xa8a1e410 prepare_draw (imx-drm_dri.so) >#13 0xa8a1e488 st_draw_vbo (imx-drm_dri.so) >#14 0xa8a0ea9c vbo_draw_arrays (imx-drm_dri.so) >#15 0xa8a0f408 vbo_exec_DrawArrays (imx-drm_dri.so) >#16 0xa953f7e0 glDrawArrays (libGLESv2.so.2) >#17 0xd7e62ea0 draw_cube_smooth (kmscube) >#18 0xd7e64160 atomic_run (kmscube) >#19 0xa93306e0 __libc_start_main (libc.so.6) >#20 0xd7e621fc $x (kmscube) >#21 0xd7e621fc $x (kmscube) > > I've traced this back to this commit: > > 19a91841c347107d877bc750371c5fa4e9b4de19 is the first bad commit > commit 19a91841c347107d877bc750371c5fa4e9b4de19 > Author: Mathias Fröhlich > Date: Sun Apr 1 20:18:36 2018 +0200 > > st/mesa: Use Array._DrawVAO in st_atom_array.c. > > Finally make use of the binding information in the VAO when > setting up arrays for draw. > > v2: Emit less relocations also for interleaved userspace arrays. > > Reviewed-by: Brian Paul > Signed-off-by: Mathias Fröhlich > > And indeed commits prior to that one work as expected. Any hints what > would be the right fix to not trigger the assert? Introduce a gallium cap and feed ctx->Const.MaxVertexAttribRelativeOffset with the value that limits the actual hardware. I have something prepared for that but did not get any response for a some preparation patch so put that issue somehow down on my list. The point for today is, is that my development system is down, means I can prepare this once this machine is back up hopefully by the beginning of this week. Else feel free to just introduce that cap. best Mathias ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
