[Mesa-dev] [PATCH v2 1/2] gallium: Enable ASIMD/NEON on aarch64.
NEON (now called ASIMD) is available on all aarch64 CPUs. Our code was
missing an aarch64 path, leading to util_cpu_caps.has_neon always being
false on aarch64.
---
Here's the simpler patch to just always enable NEON on aarch64. It suits
my purposes, but I can imagine that you may prefer the original patch if
you ever want to do runtime detection of other features on aarch64.
src/util/u_cpu_detect.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/util/u_cpu_detect.c b/src/util/u_cpu_detect.c
index 52b9ae547d4..4df10c62ef5 100644
--- a/src/util/u_cpu_detect.c
+++ b/src/util/u_cpu_detect.c
@@ -365,7 +365,14 @@ check_os_arm_support(void)
}
#endif /* PIPE_OS_LINUX */
}
-#endif /* PIPE_ARCH_ARM */
+
+#elif defined(PIPE_ARCH_AARCH64)
+static void
+check_os_arm_support(void)
+{
+util_cpu_caps.has_neon = true;
+}
+#endif /* PIPE_ARCH_ARM || PIPE_ARCH_AARCH64 */
static void
get_cpu_topology(void)
@@ -534,7 +541,7 @@ util_cpu_detect_once(void)
}
#endif /* PIPE_ARCH_X86 || PIPE_ARCH_X86_64 */
-#if defined(PIPE_ARCH_ARM)
+#if defined(PIPE_ARCH_ARM) || defined(PIPE_ARCH_AARCH64)
check_os_arm_support();
#endif
--
2.19.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] etnaviv: hook up linear texture sampling support
Am Mo., 21. Jan. 2019 um 10:10 Uhr schrieb Lucas Stach :
>
> Am Montag, den 21.01.2019, 07:50 +0100 schrieb Christian Gmeiner:
> > If the GPU supports linear sampling, linear addressing mode
> > will be used as default.
> >
> > > Signed-off-by: Christian Gmeiner
> > ---
> > src/gallium/drivers/etnaviv/etnaviv_resource.c | 10 +++---
> > src/gallium/drivers/etnaviv/etnaviv_texture.c | 4 +++-
> > 2 files changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/gallium/drivers/etnaviv/etnaviv_resource.c
> > b/src/gallium/drivers/etnaviv/etnaviv_resource.c
> > index 9a7ebf3064e..7d24b1f03bd 100644
> > --- a/src/gallium/drivers/etnaviv/etnaviv_resource.c
> > +++ b/src/gallium/drivers/etnaviv/etnaviv_resource.c
> > @@ -318,9 +318,9 @@ etna_resource_create(struct pipe_screen *pscreen,
> > {
> > struct etna_screen *screen = etna_screen(pscreen);
> >
> > - /* Figure out what tiling and address mode to use -- for now, assume
> > that
> > -* texture cannot be linear. there is a capability
> > LINEAR_TEXTURE_SUPPORT
> > -* (supported on gc880 and gc2000 at least), but not sure how it works.
> > + /* Figure out what tiling and address mode to use.
> > +* Textures are TILED or LINEAR. If LINEAR_TEXTURE_SUPPORT capability is
> > +* available LINEAR gets prefered.
> > * Buffers always have LINEAR layout.
> > */
> > unsigned layout = ETNA_LAYOUT_LINEAR;
> > @@ -334,6 +334,10 @@ etna_resource_create(struct pipe_screen *pscreen,
> >
> >if (util_format_is_compressed(templat->format))
> > layout = ETNA_LAYOUT_LINEAR;
> > + else if (VIV_FEATURE(screen, chipMinorFeatures1,
> > LINEAR_TEXTURE_SUPPORT)) {
> > + layout = ETNA_LAYOUT_LINEAR;
> > + mode = ETNA_ADDRESSING_MODE_LINEAR;
> > + }
>
> Did you do any performance measurements with this change? I don't think
> we generally want to prefer linear textures, as in theory they have
> much worse texture cache hit rates. Also a lot of the async transfer
> stuff currently depends on hitting the RS linear->tiled blit path for
> optimal performance on uploads.
>
I have not done any performance measurements yet - I only tried to get it
render correctly (piglit and amoeba) and get some feedback asap.
But I will keep an eye on perf for v2.
Regarding the async transfer staff I have the feeling that we lose the shadow
resource (etna_transfer->rsc) handling if we are using linear, which saves us
from some RS blits. Or?
> There are 2 cases where I think linear textures are useful:
>
> 1. Imported external buffers, where we might need to update the
> internal tiled copy on each resource update. Getting rid of this blit
> should help performance a good bit.
>
You are taking about etna_resource_from_handle(..). I *think* for this we
need support for linear in the pixel engine too - based on the binding flag
combinations I have seen.
> 2. 8bpp formats that can't be tiled with the RS and would hit the
> software fallback path. The tradeoff software tiling path vs. reduced
> texture cache hit rates might still prefer linear textures.
>
Yes that I something to look into.
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] etnaviv: extend etna_resource with an addressing mode
Hi Lucas Am Mo., 21. Jan. 2019 um 10:03 Uhr schrieb Lucas Stach : > > Hi Christian, > > first of all, thanks for figuring this out. This is really nice > to finally know how it works. > > Am Montag, den 21.01.2019, 07:49 +0100 schrieb Christian Gmeiner: > > Defines how sampler (and pixel pipes) needs to access the data > > represented with a resource. The used default is mode is > > ETNA_ADDRESSING_MODE_TILED. > > Do you see any reason why we need a separate property for this? IMHO > etna_resource is already a bit too fat and from this set of patches I > can't see why we can't infer the addressing mode from the layout. Do > you have something specific in mind, that I don't see right now? > We are using ETNA_LAYOUT_LINEAR for compressed textures with an addressing mode of TILED (register value of 0). That is the root cause why was forced to add something new to etna_resource as I can not trust that ETNA_LAYOUT_LINEAR for sampler resources implies an linear addressing mode. -- greets -- Christian Gmeiner, MSc https://christian-gmeiner.info ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] pl111: Rename the pl111 driver to "kmsro".
Hi Eric
Am Do., 25. Okt. 2018 um 18:39 Uhr schrieb Eric Anholt :
>
> The vc4 driver can do prime sharing to many different KMS-only devices,
> such as the various tinydrm drivers for SPI-attached displays. Rename the
> driver away from "pl111" to represent what it will actually support:
> various sorts of KMS displays with the renderonly layer used to attach a
> GPU.
I like the idea of this patch and it is good starting point torwards a
generic solution
for kms displays with the renderonly framework.
Reviewed-by: Christian Gmeiner
> ---
> .travis.yml | 2 +-
> Android.mk | 4 ++--
> Makefile.am | 2 +-
> configure.ac | 16
> meson.build | 8
> meson_options.txt| 2 +-
> src/gallium/Android.mk | 2 +-
> src/gallium/Makefile.am | 4 ++--
> .../auxiliary/pipe-loader/pipe_loader_drm.c | 2 +-
> .../auxiliary/target-helpers/drm_helper.h| 12 ++--
> .../auxiliary/target-helpers/drm_helper_public.h | 2 +-
> src/gallium/drivers/{pl111 => kmsro}/Android.mk | 6 +++---
> src/gallium/drivers/kmsro/Automake.inc | 9 +
> src/gallium/drivers/{pl111 => kmsro}/Makefile.am | 4 ++--
> .../drivers/{pl111 => kmsro}/Makefile.sources| 0
> src/gallium/drivers/pl111/Automake.inc | 9 -
> src/gallium/meson.build | 6 +++---
> src/gallium/targets/dri/Makefile.am | 2 +-
> src/gallium/targets/dri/meson.build | 4 ++--
> src/gallium/targets/dri/target.c | 2 +-
> .../winsys/{pl111 => kmsro}/drm/Android.mk | 2 +-
> .../winsys/{pl111 => kmsro}/drm/Makefile.am | 4 ++--
> src/gallium/winsys/kmsro/drm/Makefile.sources| 3 +++
> .../drm/kmsro_drm_public.h} | 8
> .../drm/kmsro_drm_winsys.c} | 6 +++---
> .../winsys/{pl111 => kmsro}/drm/meson.build | 12 ++--
> src/gallium/winsys/pl111/drm/Makefile.sources| 3 ---
> 27 files changed, 68 insertions(+), 68 deletions(-)
> rename src/gallium/drivers/{pl111 => kmsro}/Android.mk (91%)
> create mode 100644 src/gallium/drivers/kmsro/Automake.inc
> rename src/gallium/drivers/{pl111 => kmsro}/Makefile.am (55%)
> rename src/gallium/drivers/{pl111 => kmsro}/Makefile.sources (100%)
> delete mode 100644 src/gallium/drivers/pl111/Automake.inc
> rename src/gallium/winsys/{pl111 => kmsro}/drm/Android.mk (97%)
> rename src/gallium/winsys/{pl111 => kmsro}/drm/Makefile.am (94%)
> create mode 100644 src/gallium/winsys/kmsro/drm/Makefile.sources
> rename src/gallium/winsys/{pl111/drm/pl111_drm_public.h =>
> kmsro/drm/kmsro_drm_public.h} (89%)
> rename src/gallium/winsys/{pl111/drm/pl111_drm_winsys.c =>
> kmsro/drm/kmsro_drm_winsys.c} (92%)
> rename src/gallium/winsys/{pl111 => kmsro}/drm/meson.build (87%)
> delete mode 100644 src/gallium/winsys/pl111/drm/Makefile.sources
>
> diff --git a/.travis.yml b/.travis.yml
> index 78e6d251ae4b..8bcd77143569 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -165,7 +165,7 @@ matrix:
> - DRI_LOADERS="--disable-glx --disable-gbm --disable-egl"
> - DRI_DRIVERS=""
> - GALLIUM_ST="--enable-dri --disable-opencl --disable-xa
> --disable-nine --disable-xvmc --disable-vdpau --disable-va
> --disable-omx-bellagio --disable-gallium-osmesa"
> --
> GALLIUM_DRIVERS="i915,nouveau,pl111,r300,r600,freedreno,svga,swrast,v3d,vc4,virgl,etnaviv,imx"
> +-
> GALLIUM_DRIVERS="i915,nouveau,kmsro,r300,r600,freedreno,svga,swrast,v3d,vc4,virgl,etnaviv,imx"
> - VULKAN_DRIVERS=""
> - LIBUNWIND_FLAGS="--enable-libunwind"
>addons:
> diff --git a/Android.mk b/Android.mk
> index 914854c27d63..1a0bdd1736cf 100644
> --- a/Android.mk
> +++ b/Android.mk
> @@ -24,7 +24,7 @@
> # BOARD_GPU_DRIVERS should be defined. The valid values are
> #
> # classic drivers: i915 i965
> -# gallium drivers: swrast freedreno i915g nouveau pl111 r300g r600g
> radeonsi vc4 virgl vmwgfx etnaviv imx
> +# gallium drivers: swrast freedreno i915g nouveau kmsro r300g r600g
> radeonsi vc4 virgl vmwgfx etnaviv imx
> #
> # The main target is libGLES_mesa. For each classic driver enabled, a DRI
> # module will also be built. DRI modules will be loaded by libGLES_mesa.
> @@ -52,7 +52,7 @@ gallium_drivers := \
> freedreno.HAVE_GALLIUM_FREEDRENO \
> i915g.HAVE_GALLIUM_I915 \
> nouveau.HAVE_GALLIUM_NOUVEAU \
> - pl111.HAVE_GALLIUM_PL111 \
> + kmsro.HAVE_GALLIUM_KMSRO \
> r300g.HAVE_GALLIUM_R300 \
> r600g.HAVE_GALLIUM_R600 \
> radeonsi.HAVE_GALLIUM_RADEONSI \
> diff --git a/Makefile.am b/Makefile.am
> index 9e27db046e52..62c755aeca7f 100644
> ---
[Mesa-dev] [Bug 109446] Shadow of the Tomb Raider Trial freezes the system at startup
https://bugs.freedesktop.org/show_bug.cgi?id=109446 Bug ID: 109446 Summary: Shadow of the Tomb Raider Trial freezes the system at startup Product: Mesa Version: git Hardware: x86-64 (AMD64) OS: Linux (All) Status: NEW Severity: blocker Priority: medium Component: Drivers/Vulkan/radeon Assignee: mesa-dev@lists.freedesktop.org Reporter: fin4...@hotmail.com QA Contact: mesa-dev@lists.freedesktop.org The game did work a month ago. When using AMDVLK there is no freeze, the game just wont start by other problems. Using DXVK 0.95 I tested with kernel from 4.19 to 5.0-rc3. Only garbage strings in /var/log/messages when the system freezes. I must use the power button to restart the PC. The game works with nvidia drivers according to: https://www.protondb.com/app/750920 Install the game to latest wine-staging: wine Steam.exe -applaunch 974630 I did see something like this messages when switching to a virtual terminal: amdgpu_sysfs_ioctl ERROR amdgpu parser failed. System: Host: ryzenpc Kernel: 5.0.0-rc3 x86_64 bits: 64 Desktop: Xfce 4.12.4 Distro: Debian GNU/Linux buster/sid Machine: Type: Desktop Mobo: ASUSTeK model: PRIME B350M-K v: Rev X.0x serial: UEFI [Legacy]: American Megatrends v: 4207 date: 12/07/2018 CPU: 6-Core: AMD Ryzen 5 1600 type: MT MCP speed: 2963 MHz Graphics: Device-1: AMD Ellesmere [Radeon RX 470/480] driver: amdgpu v: kernel Display: x11 server: X.Org 1.20.3 driver: amdgpu resolution: 3840x2160~60Hz OpenGL: renderer: Radeon RX 570 Series (POLARIS10 DRM 3.27.0 5.0.0-rc3 LLVM 7.0.1) v: 4.5 Mesa 19.0.0-devel (git-8e26d53 2019-01-23 cosmic-oibaf-ppa) -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug.___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] radv/ac: fix some fp16 handling
Fixes: b722b29f10d4 ("radv: add support for 16bit input/output")
---
Compile tested only. Noticed when passing by.
src/amd/common/ac_nir_to_llvm.c | 2 +-
src/amd/vulkan/radv_nir_to_llvm.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index f509fc31dff..012c656a7be 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -3980,7 +3980,7 @@ ac_handle_shader_output_decl(struct ac_llvm_context *ctx,
}
}
- bool is_16bit = glsl_type_is_16bit(variable->type);
+ bool is_16bit = glsl_type_is_16bit(glsl_without_array(variable->type));
LLVMTypeRef type = is_16bit ? ctx->f16 : ctx->f32;
for (unsigned i = 0; i < attrib_count; ++i) {
for (unsigned chan = 0; chan < 4; chan++) {
diff --git a/src/amd/vulkan/radv_nir_to_llvm.c
b/src/amd/vulkan/radv_nir_to_llvm.c
index 40812fa7ffb..8693809b90b 100644
--- a/src/amd/vulkan/radv_nir_to_llvm.c
+++ b/src/amd/vulkan/radv_nir_to_llvm.c
@@ -2163,7 +2163,7 @@ handle_fs_input_decl(struct radv_shader_context *ctx,
interp = lookup_interp_param(>abi,
variable->data.interpolation, interp_type);
}
- bool is_16bit = glsl_type_is_16bit(variable->type);
+ bool is_16bit = glsl_type_is_16bit(glsl_without_array(variable->type));
LLVMTypeRef type = is_16bit ? ctx->ac.i16 : ctx->ac.i32;
if (interp == NULL)
interp = LLVMGetUndef(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 v3] anv/allocator: Avoid race condition in anv_block_pool_map.
Reviewed-by: Jason Ekstrand
On Wed, Jan 23, 2019 at 6:41 PM Rafael Antognolli <
rafael.antogno...@intel.com> wrote:
> Accessing bo->map and then pool->center_bo_offset without a lock is
> racy. One way of avoiding such race condition is to store the bo->map +
> center_bo_offset into pool->map at the time the block pool is growing,
> which happens within a lock.
>
> v2: Only set pool->map if not using softpin (Jason).
> v3: Move things around and only update center_bo_offset if not using
> softpin too (Jason).
>
> Cc: Jason Ekstrand
> Reported-by: Ian Romanick
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109442
> Fixes: fc3f58832015cbb177179e7f3420d3611479b4a9
> ---
> src/intel/vulkan/anv_allocator.c | 20 ++--
> src/intel/vulkan/anv_private.h | 13 +
> 2 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_allocator.c
> b/src/intel/vulkan/anv_allocator.c
> index 89f26789c85..006175c8c65 100644
> --- a/src/intel/vulkan/anv_allocator.c
> +++ b/src/intel/vulkan/anv_allocator.c
> @@ -436,7 +436,9 @@ anv_block_pool_init(struct anv_block_pool *pool,
> pool->bo_flags = bo_flags;
> pool->nbos = 0;
> pool->size = 0;
> + pool->center_bo_offset = 0;
> pool->start_address = gen_canonical_address(start_address);
> + pool->map = NULL;
>
> /* This pointer will always point to the first BO in the list */
> pool->bo = >bos[0];
> @@ -536,6 +538,7 @@ anv_block_pool_expand_range(struct anv_block_pool
> *pool,
>if (map == MAP_FAILED)
> return vk_errorf(pool->device->instance, pool->device,
>VK_ERROR_MEMORY_MAP_FAILED, "gem mmap failed:
> %m");
> + assert(center_bo_offset == 0);
> } else {
>/* Just leak the old map until we destroy the pool. We can't
> munmap it
> * without races or imposing locking on the block allocate fast
> path. On
> @@ -549,6 +552,11 @@ anv_block_pool_expand_range(struct anv_block_pool
> *pool,
>if (map == MAP_FAILED)
> return vk_errorf(pool->device->instance, pool->device,
>VK_ERROR_MEMORY_MAP_FAILED, "mmap failed: %m");
> +
> + /* Now that we mapped the new memory, we can write the new
> + * center_bo_offset back into pool and update pool->map. */
> + pool->center_bo_offset = center_bo_offset;
> + pool->map = map + center_bo_offset;
>gem_handle = anv_gem_userptr(pool->device, map, size);
>if (gem_handle == 0) {
> munmap(map, size);
> @@ -573,10 +581,6 @@ anv_block_pool_expand_range(struct anv_block_pool
> *pool,
> if (!pool->device->info.has_llc)
>anv_gem_set_caching(pool->device, gem_handle, I915_CACHING_CACHED);
>
> - /* Now that we successfull allocated everything, we can write the new
> -* center_bo_offset back into pool. */
> - pool->center_bo_offset = center_bo_offset;
> -
> /* For block pool BOs we have to be a bit careful about where we place
> them
> * in the GTT. There are two documented workarounds for state base
> address
> * placement : Wa32bitGeneralStateOffset and
> Wa32bitInstructionBaseOffset
> @@ -670,8 +674,12 @@ anv_block_pool_get_bo(struct anv_block_pool *pool,
> int32_t *offset)
> void*
> anv_block_pool_map(struct anv_block_pool *pool, int32_t offset)
> {
> - struct anv_bo *bo = anv_block_pool_get_bo(pool, );
> - return bo->map + pool->center_bo_offset + offset;
> + if (pool->bo_flags & EXEC_OBJECT_PINNED) {
> + struct anv_bo *bo = anv_block_pool_get_bo(pool, );
> + return bo->map + offset;
> + } else {
> + return pool->map + offset;
> + }
> }
>
> /** Grows and re-centers the block pool.
> diff --git a/src/intel/vulkan/anv_private.h
> b/src/intel/vulkan/anv_private.h
> index 3889065c93c..110b2ccf023 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -663,6 +663,19 @@ struct anv_block_pool {
> */
> uint32_t center_bo_offset;
>
> + /* Current memory map of the block pool. This pointer may or may not
> +* point to the actual beginning of the block pool memory. If
> +* anv_block_pool_alloc_back has ever been called, then this pointer
> +* will point to the "center" position of the buffer and all offsets
> +* (negative or positive) given out by the block pool alloc functions
> +* will be valid relative to this pointer.
> +*
> +* In particular, map == bo.map + center_offset
> +*
> +* DO NOT access this pointer directly. Use anv_block_pool_map()
> instead,
> +* since it will handle the softpin case as well, where this points to
> NULL.
> +*/
> + void *map;
> int fd;
>
> /**
> --
> 2.17.2
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 109361] [KBL-G][GL-es] several shader test cases failed to compile
https://bugs.freedesktop.org/show_bug.cgi?id=109361 Hai changed: What|Removed |Added Resolution|--- |WONTFIX Status|NEW |RESOLVED -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug.___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] tgsi: remove culldist semantic from docs
The semantic was removed in e6d93893662d. --- src/gallium/docs/source/tgsi.rst | 18 -- 1 file changed, 18 deletions(-) diff --git a/src/gallium/docs/source/tgsi.rst b/src/gallium/docs/source/tgsi.rst index 277f25ca41b..59410cfd663 100644 --- a/src/gallium/docs/source/tgsi.rst +++ b/src/gallium/docs/source/tgsi.rst @@ -3205,24 +3205,6 @@ tessellation evaluation shaders, respectively. Only the value written in the last vertex processing stage is used. -TGSI_SEMANTIC_CULLDIST -"" - -Used as distance to plane for performing application-defined culling -of individual primitives against a plane. When components of vertex -elements are given this label, these values are assumed to be a -float32 signed distance to a plane. Primitives will be completely -discarded if the plane distance for all of the vertices in the -primitive are < 0. If a vertex has a cull distance of NaN, that -vertex counts as "out" (as if its < 0); -The limits on both clip and cull distances are bound -by the PIPE_MAX_CLIP_OR_CULL_DISTANCE_COUNT define which defines -the maximum number of components that can be used to hold the -distances and by the PIPE_MAX_CLIP_OR_CULL_DISTANCE_ELEMENT_COUNT -which specifies the maximum number of registers which can be -annotated with those semantics. - - TGSI_SEMANTIC_CLIPDIST "" -- 2.20.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v3] anv/allocator: Avoid race condition in anv_block_pool_map.
Accessing bo->map and then pool->center_bo_offset without a lock is
racy. One way of avoiding such race condition is to store the bo->map +
center_bo_offset into pool->map at the time the block pool is growing,
which happens within a lock.
v2: Only set pool->map if not using softpin (Jason).
v3: Move things around and only update center_bo_offset if not using
softpin too (Jason).
Cc: Jason Ekstrand
Reported-by: Ian Romanick
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109442
Fixes: fc3f58832015cbb177179e7f3420d3611479b4a9
---
src/intel/vulkan/anv_allocator.c | 20 ++--
src/intel/vulkan/anv_private.h | 13 +
2 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/intel/vulkan/anv_allocator.c b/src/intel/vulkan/anv_allocator.c
index 89f26789c85..006175c8c65 100644
--- a/src/intel/vulkan/anv_allocator.c
+++ b/src/intel/vulkan/anv_allocator.c
@@ -436,7 +436,9 @@ anv_block_pool_init(struct anv_block_pool *pool,
pool->bo_flags = bo_flags;
pool->nbos = 0;
pool->size = 0;
+ pool->center_bo_offset = 0;
pool->start_address = gen_canonical_address(start_address);
+ pool->map = NULL;
/* This pointer will always point to the first BO in the list */
pool->bo = >bos[0];
@@ -536,6 +538,7 @@ anv_block_pool_expand_range(struct anv_block_pool *pool,
if (map == MAP_FAILED)
return vk_errorf(pool->device->instance, pool->device,
VK_ERROR_MEMORY_MAP_FAILED, "gem mmap failed: %m");
+ assert(center_bo_offset == 0);
} else {
/* Just leak the old map until we destroy the pool. We can't munmap it
* without races or imposing locking on the block allocate fast path. On
@@ -549,6 +552,11 @@ anv_block_pool_expand_range(struct anv_block_pool *pool,
if (map == MAP_FAILED)
return vk_errorf(pool->device->instance, pool->device,
VK_ERROR_MEMORY_MAP_FAILED, "mmap failed: %m");
+
+ /* Now that we mapped the new memory, we can write the new
+ * center_bo_offset back into pool and update pool->map. */
+ pool->center_bo_offset = center_bo_offset;
+ pool->map = map + center_bo_offset;
gem_handle = anv_gem_userptr(pool->device, map, size);
if (gem_handle == 0) {
munmap(map, size);
@@ -573,10 +581,6 @@ anv_block_pool_expand_range(struct anv_block_pool *pool,
if (!pool->device->info.has_llc)
anv_gem_set_caching(pool->device, gem_handle, I915_CACHING_CACHED);
- /* Now that we successfull allocated everything, we can write the new
-* center_bo_offset back into pool. */
- pool->center_bo_offset = center_bo_offset;
-
/* For block pool BOs we have to be a bit careful about where we place them
* in the GTT. There are two documented workarounds for state base address
* placement : Wa32bitGeneralStateOffset and Wa32bitInstructionBaseOffset
@@ -670,8 +674,12 @@ anv_block_pool_get_bo(struct anv_block_pool *pool, int32_t
*offset)
void*
anv_block_pool_map(struct anv_block_pool *pool, int32_t offset)
{
- struct anv_bo *bo = anv_block_pool_get_bo(pool, );
- return bo->map + pool->center_bo_offset + offset;
+ if (pool->bo_flags & EXEC_OBJECT_PINNED) {
+ struct anv_bo *bo = anv_block_pool_get_bo(pool, );
+ return bo->map + offset;
+ } else {
+ return pool->map + offset;
+ }
}
/** Grows and re-centers the block pool.
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 3889065c93c..110b2ccf023 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -663,6 +663,19 @@ struct anv_block_pool {
*/
uint32_t center_bo_offset;
+ /* Current memory map of the block pool. This pointer may or may not
+* point to the actual beginning of the block pool memory. If
+* anv_block_pool_alloc_back has ever been called, then this pointer
+* will point to the "center" position of the buffer and all offsets
+* (negative or positive) given out by the block pool alloc functions
+* will be valid relative to this pointer.
+*
+* In particular, map == bo.map + center_offset
+*
+* DO NOT access this pointer directly. Use anv_block_pool_map() instead,
+* since it will handle the softpin case as well, where this points to NULL.
+*/
+ void *map;
int fd;
/**
--
2.17.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] anv/allocator: Avoid race condition in anv_block_pool_map.
On Wed, Jan 23, 2019 at 06:08:50PM -0600, Jason Ekstrand wrote:
> On Wed, Jan 23, 2019 at 5:26 PM Rafael Antognolli
>
> wrote:
>
> Accessing bo->map and then pool->center_bo_offset without a lock is
> racy. One way of avoiding such race condition is to store the bo->map +
> center_bo_offset into pool->map at the time the block pool is growing,
> which happens within a lock.
>
> v2: Only set pool->map if not using softpin (Jason).
>
> Cc: Jason Ekstrand
> Reported-by: Ian Romanick
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109442
> Fixes: fc3f58832015cbb177179e7f3420d3611479b4a9
> ---
> src/intel/vulkan/anv_allocator.c | 11 +--
> src/intel/vulkan/anv_private.h | 13 +
> 2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_allocator.c b/src/intel/vulkan/
> anv_allocator.c
> index 89f26789c85..67f9c2a948d 100644
> --- a/src/intel/vulkan/anv_allocator.c
> +++ b/src/intel/vulkan/anv_allocator.c
> @@ -437,6 +437,7 @@ anv_block_pool_init(struct anv_block_pool *pool,
> pool->nbos = 0;
> pool->size = 0;
> pool->start_address = gen_canonical_address(start_address);
> + pool->map = NULL;
>
> /* This pointer will always point to the first BO in the list */
> pool->bo = >bos[0];
> @@ -576,6 +577,8 @@ anv_block_pool_expand_range(struct anv_block_pool
> *pool,
> /* Now that we successfull allocated everything, we can write the new
> * center_bo_offset back into pool. */
> pool->center_bo_offset = center_bo_offset;
> + if (!use_softpin)
> + pool->map = map + center_bo_offset;
>
>
> We could also put this a bit higher up right after where we actually call
> mmap. That would reduce the number of "if (use_softpin)" blocks and probably
> make things more readable.
>
> Come to think of it, we could also set pool->center_bo_offset there and just
> assert(center_bo_offset == 0) in the softpin case. I like that. Maybe that's
> a second patch?
I wouldn't mind sending a v3. In fact, I have it ready and will right
after this email). But I can also simply push this one and send a new
patch tomorrow, if it's too late to get a review on the v3.
> In any case, this fixes today's bug
>
> Reviewed-by: Jason Ekstrand
Thanks!
Rafael
> --Jason
>
>
>
> /* For block pool BOs we have to be a bit careful about where we place
> them
> * in the GTT. There are two documented workarounds for state base
> address
> @@ -670,8 +673,12 @@ anv_block_pool_get_bo(struct anv_block_pool *pool,
> int32_t *offset)
> void*
> anv_block_pool_map(struct anv_block_pool *pool, int32_t offset)
> {
> - struct anv_bo *bo = anv_block_pool_get_bo(pool, );
> - return bo->map + pool->center_bo_offset + offset;
> + if (pool->bo_flags & EXEC_OBJECT_PINNED) {
> + struct anv_bo *bo = anv_block_pool_get_bo(pool, );
> + return bo->map + offset;
> + } else {
> + return pool->map + offset;
> + }
> }
>
> /** Grows and re-centers the block pool.
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/
> anv_private.h
> index 3889065c93c..110b2ccf023 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -663,6 +663,19 @@ struct anv_block_pool {
> */
> uint32_t center_bo_offset;
>
> + /* Current memory map of the block pool. This pointer may or may not
> +* point to the actual beginning of the block pool memory. If
> +* anv_block_pool_alloc_back has ever been called, then this pointer
> +* will point to the "center" position of the buffer and all offsets
> +* (negative or positive) given out by the block pool alloc functions
> +* will be valid relative to this pointer.
> +*
> +* In particular, map == bo.map + center_offset
> +*
> +* DO NOT access this pointer directly. Use anv_block_pool_map()
> instead,
> +* since it will handle the softpin case as well, where this points to
> NULL.
> +*/
> + void *map;
> int fd;
>
> /**
> --
> 2.17.2
>
>
> ___
> 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] Thoughts after hitting 100 merge requests?
I guess as discovered with https://gitlab.freedesktop.org/mesa/mesa/merge_requests/47 maybe we should wait to turn on merging MRs via web until we have at least some basic build-test CI wired up.. the downside is slower 'maintainer' response (if I am working on some long running branch I tend to postpone pushing changes from others until I am in a good state to rebase, but at least when I merge things via cmdline I do a build test and sanity test on at at least one driver ;-)) BR, -R On Fri, Jan 11, 2019 at 11:57 AM Jason Ekstrand wrote: > > All, > > The mesa project has now hit 100 merge requests (36 are still open). I (and > I'm sure others) would be curious to hear people's initial thoughts on the > process. What's working well? What's not working? Is it total fail and > should we go back to mailing lists? > > --Jason > ___ > 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] anv/allocator: Avoid race condition in anv_block_pool_map.
On Wed, Jan 23, 2019 at 5:26 PM Rafael Antognolli <
rafael.antogno...@intel.com> wrote:
> Accessing bo->map and then pool->center_bo_offset without a lock is
> racy. One way of avoiding such race condition is to store the bo->map +
> center_bo_offset into pool->map at the time the block pool is growing,
> which happens within a lock.
>
> v2: Only set pool->map if not using softpin (Jason).
>
> Cc: Jason Ekstrand
> Reported-by: Ian Romanick
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109442
> Fixes: fc3f58832015cbb177179e7f3420d3611479b4a9
> ---
> src/intel/vulkan/anv_allocator.c | 11 +--
> src/intel/vulkan/anv_private.h | 13 +
> 2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_allocator.c
> b/src/intel/vulkan/anv_allocator.c
> index 89f26789c85..67f9c2a948d 100644
> --- a/src/intel/vulkan/anv_allocator.c
> +++ b/src/intel/vulkan/anv_allocator.c
> @@ -437,6 +437,7 @@ anv_block_pool_init(struct anv_block_pool *pool,
> pool->nbos = 0;
> pool->size = 0;
> pool->start_address = gen_canonical_address(start_address);
> + pool->map = NULL;
>
> /* This pointer will always point to the first BO in the list */
> pool->bo = >bos[0];
> @@ -576,6 +577,8 @@ anv_block_pool_expand_range(struct anv_block_pool
> *pool,
> /* Now that we successfull allocated everything, we can write the new
> * center_bo_offset back into pool. */
> pool->center_bo_offset = center_bo_offset;
> + if (!use_softpin)
> + pool->map = map + center_bo_offset;
>
We could also put this a bit higher up right after where we actually call
mmap. That would reduce the number of "if (use_softpin)" blocks and
probably make things more readable.
Come to think of it, we could also set pool->center_bo_offset there and
just assert(center_bo_offset == 0) in the softpin case. I like that.
Maybe that's a second patch?
In any case, this fixes today's bug
Reviewed-by: Jason Ekstrand
--Jason
>
> /* For block pool BOs we have to be a bit careful about where we place
> them
> * in the GTT. There are two documented workarounds for state base
> address
> @@ -670,8 +673,12 @@ anv_block_pool_get_bo(struct anv_block_pool *pool,
> int32_t *offset)
> void*
> anv_block_pool_map(struct anv_block_pool *pool, int32_t offset)
> {
> - struct anv_bo *bo = anv_block_pool_get_bo(pool, );
> - return bo->map + pool->center_bo_offset + offset;
> + if (pool->bo_flags & EXEC_OBJECT_PINNED) {
> + struct anv_bo *bo = anv_block_pool_get_bo(pool, );
> + return bo->map + offset;
> + } else {
> + return pool->map + offset;
> + }
> }
>
> /** Grows and re-centers the block pool.
> diff --git a/src/intel/vulkan/anv_private.h
> b/src/intel/vulkan/anv_private.h
> index 3889065c93c..110b2ccf023 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -663,6 +663,19 @@ struct anv_block_pool {
> */
> uint32_t center_bo_offset;
>
> + /* Current memory map of the block pool. This pointer may or may not
> +* point to the actual beginning of the block pool memory. If
> +* anv_block_pool_alloc_back has ever been called, then this pointer
> +* will point to the "center" position of the buffer and all offsets
> +* (negative or positive) given out by the block pool alloc functions
> +* will be valid relative to this pointer.
> +*
> +* In particular, map == bo.map + center_offset
> +*
> +* DO NOT access this pointer directly. Use anv_block_pool_map()
> instead,
> +* since it will handle the softpin case as well, where this points to
> NULL.
> +*/
> + void *map;
> int fd;
>
> /**
> --
> 2.17.2
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] pl111: Rename the pl111 driver to "kmsro".
> I've started looking at the lima and panfrost drivers. The many > combinations of Mali GPUs and DC isn't going to scale. The lima and > panfrost trees can't even co-exist as both define a rockchip winsys > which load different GPU drivers. The same will be true for meson, > hisilicon, allwinner, etc. i.MX is about to be in the same boat > needing to support both etnaviv and freedreno. As Rob stated, Mali being used by basically everyone at one point or another has led to a nightmare in the winsys. I agree that dealing with the loader can happen later, but honestly, just having the centralised kmsro winsys (that all of pl111/rockchip/meson/sunxi/etc point to) that tries all of vc4/v3d/panfrost/lima/etc would be a marked improvement on the present situation. There are a lot of DRM drivers out there, sure, and it _is_ better to handle something generically in the loader. But for the much more immediate goal of letting both Lima and Panfrost coexist on Rockchip/Meson, this is a good start. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] gallium: Enable aarch64 NEON CPU detection.
On Wed, Jan 23, 2019 at 3:26 PM Eric Anholt wrote:
>
> Matt Turner writes:
>
> > NEON (now called ASIMD) is available on all aarch64 CPUs. It seems that
> > our code was missing an aarch64 path, leading to util_cpu_caps.has_neon
> > always being false on aarch64. I think that means that the NEON tiling
> > code in vc4 would not be enabled on aarch64 (vc4_load_lt_image_neon,
> > etc).
> > ---
> > I have very little clue about aarch64 ABIs, so I don't know if there's
> > another case that needs to be handled -- aarch32 maybe? Does
> > PIPE_ARCH_AARCH64 just mean ARMv8 and so we should check something else
> > for the ABI and choose Elf{32,64} based on that?
>
> Do we actually need to do runtime detection? It sounds like "standard"
> armv8 is guaranteed NEON:
>
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0024a/CEGDJGGC.html
I think that's right.
https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html seems to
agree:
‘simd’
Enable Advanced SIMD instructions. This also enables floating-point
instructions. This is on by default for all possible values for
options -march and -mcpu.
I'll send a new patch that just sets util_cpu_caps.has_neon = true for aarch64.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/2] radeonsi: handle render_condition_enable in si_compute_clear_render_target
From: Marek Olšák
---
src/gallium/drivers/radeonsi/si_clear.c| 3 ++-
src/gallium/drivers/radeonsi/si_compute_blit.c | 5 -
src/gallium/drivers/radeonsi/si_pipe.h | 3 ++-
3 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/gallium/drivers/radeonsi/si_clear.c
b/src/gallium/drivers/radeonsi/si_clear.c
index 8afc01f2ccc..9a00bb73b94 100644
--- a/src/gallium/drivers/radeonsi/si_clear.c
+++ b/src/gallium/drivers/radeonsi/si_clear.c
@@ -667,21 +667,22 @@ static void si_clear_render_target(struct pipe_context
*ctx,
struct pipe_surface *dst,
const union pipe_color_union *color,
unsigned dstx, unsigned dsty,
unsigned width, unsigned height,
bool render_condition_enabled)
{
struct si_context *sctx = (struct si_context *)ctx;
struct si_texture *sdst = (struct si_texture*)dst->texture;
if (dst->texture->nr_samples <= 1 && !sdst->dcc_offset) {
- si_compute_clear_render_target(ctx, dst, color, dstx, dsty,
width, height);
+ si_compute_clear_render_target(ctx, dst, color, dstx, dsty,
width,
+ height,
render_condition_enabled);
return;
}
si_blitter_begin(sctx, SI_CLEAR_SURFACE |
(render_condition_enabled ? 0 :
SI_DISABLE_RENDER_COND));
util_blitter_clear_render_target(sctx->blitter, dst, color,
dstx, dsty, width, height);
si_blitter_end(sctx);
}
diff --git a/src/gallium/drivers/radeonsi/si_compute_blit.c
b/src/gallium/drivers/radeonsi/si_compute_blit.c
index f06497f4dac..1ea0d7517df 100644
--- a/src/gallium/drivers/radeonsi/si_compute_blit.c
+++ b/src/gallium/drivers/radeonsi/si_compute_blit.c
@@ -425,40 +425,43 @@ void si_compute_copy_image(struct si_context *sctx,
void si_init_compute_blit_functions(struct si_context *sctx)
{
sctx->b.clear_buffer = si_pipe_clear_buffer;
}
/* Clear a region of a color surface to a constant value. */
void si_compute_clear_render_target(struct pipe_context *ctx,
struct pipe_surface *dstsurf,
const union pipe_color_union *color,
unsigned dstx, unsigned dsty,
- unsigned width, unsigned height)
+ unsigned width, unsigned height,
+ bool render_condition_enabled)
{
struct si_context *sctx = (struct si_context *)ctx;
unsigned num_layers = dstsurf->u.tex.last_layer -
dstsurf->u.tex.first_layer + 1;
unsigned data[4 + sizeof(color->ui)] = {dstx, dsty,
dstsurf->u.tex.first_layer, 0};
if (width == 0 || height == 0)
return;
if (util_format_is_srgb(dstsurf->format)) {
union pipe_color_union color_srgb;
for (int i = 0; i < 3; i++)
color_srgb.f[i] =
util_format_linear_to_srgb_float(color->f[i]);
color_srgb.f[3] = color->f[3];
memcpy(data + 4, color_srgb.ui, sizeof(color->ui));
} else {
memcpy(data + 4, color->ui, sizeof(color->ui));
}
si_compute_internal_begin(sctx);
+ sctx->render_cond_force_off = !render_condition_enabled;
+
sctx->flags |= SI_CONTEXT_CS_PARTIAL_FLUSH |
si_get_flush_flags(sctx, SI_COHERENCY_SHADER, L2_STREAM);
si_make_CB_shader_coherent(sctx, dstsurf->texture->nr_samples, true);
struct pipe_constant_buffer saved_cb = {};
si_get_pipe_constant_buffer(sctx, PIPE_SHADER_COMPUTE, 0, _cb);
struct si_images *images = >images[PIPE_SHADER_COMPUTE];
struct pipe_image_view saved_image = {0};
util_copy_image_view(_image, >views[0]);
diff --git a/src/gallium/drivers/radeonsi/si_pipe.h
b/src/gallium/drivers/radeonsi/si_pipe.h
index 437144316d0..1af3c5ff9b7 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.h
+++ b/src/gallium/drivers/radeonsi/si_pipe.h
@@ -1188,21 +1188,22 @@ void si_compute_copy_image(struct si_context *sctx,
struct pipe_resource *dst,
unsigned dst_level,
struct pipe_resource *src,
unsigned src_level,
unsigned dstx, unsigned dsty, unsigned dstz,
const struct pipe_box *src_box);
void si_compute_clear_render_target(struct pipe_context *ctx,
struct pipe_surface *dstsurf,
const union pipe_color_union *color,
unsigned dstx, unsigned dsty,
-unsigned
[Mesa-dev] [PATCH 1/2] radeonsi: use compute for clear_render_target when possible
From: Sonny Jiang
Signed-off-by: Sonny Jiang
Signed-off-by: Marek Olšák
---
src/gallium/drivers/radeonsi/si_clear.c | 6 ++
.../drivers/radeonsi/si_compute_blit.c| 96 +++
src/gallium/drivers/radeonsi/si_pipe.c| 4 +
src/gallium/drivers/radeonsi/si_pipe.h| 9 ++
.../drivers/radeonsi/si_shaderlib_tgsi.c | 69 +
5 files changed, 184 insertions(+)
diff --git a/src/gallium/drivers/radeonsi/si_clear.c
b/src/gallium/drivers/radeonsi/si_clear.c
index b3910a4651c..8afc01f2ccc 100644
--- a/src/gallium/drivers/radeonsi/si_clear.c
+++ b/src/gallium/drivers/radeonsi/si_clear.c
@@ -664,20 +664,26 @@ static void si_clear(struct pipe_context *ctx, unsigned
buffers,
}
static void si_clear_render_target(struct pipe_context *ctx,
struct pipe_surface *dst,
const union pipe_color_union *color,
unsigned dstx, unsigned dsty,
unsigned width, unsigned height,
bool render_condition_enabled)
{
struct si_context *sctx = (struct si_context *)ctx;
+ struct si_texture *sdst = (struct si_texture*)dst->texture;
+
+ if (dst->texture->nr_samples <= 1 && !sdst->dcc_offset) {
+ si_compute_clear_render_target(ctx, dst, color, dstx, dsty,
width, height);
+ return;
+ }
si_blitter_begin(sctx, SI_CLEAR_SURFACE |
(render_condition_enabled ? 0 :
SI_DISABLE_RENDER_COND));
util_blitter_clear_render_target(sctx->blitter, dst, color,
dstx, dsty, width, height);
si_blitter_end(sctx);
}
static void si_clear_depth_stencil(struct pipe_context *ctx,
struct pipe_surface *dst,
diff --git a/src/gallium/drivers/radeonsi/si_compute_blit.c
b/src/gallium/drivers/radeonsi/si_compute_blit.c
index 38c48c30be9..f06497f4dac 100644
--- a/src/gallium/drivers/radeonsi/si_compute_blit.c
+++ b/src/gallium/drivers/radeonsi/si_compute_blit.c
@@ -18,20 +18,21 @@
* FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
* THE AUTHOR(S) AND/OR THEIR SUPPLIERS BE LIABLE FOR ANY CLAIM,
* DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
* OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
* USE OR OTHER DEALINGS IN THE SOFTWARE.
*
*/
#include "si_pipe.h"
#include "util/u_format.h"
+#include "util/format_srgb.h"
/* Note: Compute shaders always use SI_COMPUTE_DST_CACHE_POLICY for dst
* and L2_STREAM for src.
*/
static enum si_cache_policy get_cache_policy(struct si_context *sctx,
enum si_coherency coher,
uint64_t size)
{
if ((sctx->chip_class >= GFX9 && (coher == SI_COHERENCY_CB_META ||
coher == SI_COHERENCY_CP)) ||
@@ -418,10 +419,105 @@ void si_compute_copy_image(struct si_context *sctx,
ctx->bind_compute_state(ctx, saved_cs);
ctx->set_shader_images(ctx, PIPE_SHADER_COMPUTE, 0, 2, saved_image);
ctx->set_constant_buffer(ctx, PIPE_SHADER_COMPUTE, 0, _cb);
si_compute_internal_end(sctx);
}
void si_init_compute_blit_functions(struct si_context *sctx)
{
sctx->b.clear_buffer = si_pipe_clear_buffer;
}
+
+/* Clear a region of a color surface to a constant value. */
+void si_compute_clear_render_target(struct pipe_context *ctx,
+ struct pipe_surface *dstsurf,
+ const union pipe_color_union *color,
+ unsigned dstx, unsigned dsty,
+ unsigned width, unsigned height)
+{
+ struct si_context *sctx = (struct si_context *)ctx;
+ unsigned num_layers = dstsurf->u.tex.last_layer -
dstsurf->u.tex.first_layer + 1;
+ unsigned data[4 + sizeof(color->ui)] = {dstx, dsty,
dstsurf->u.tex.first_layer, 0};
+
+ if (width == 0 || height == 0)
+ return;
+
+ if (util_format_is_srgb(dstsurf->format)) {
+ union pipe_color_union color_srgb;
+ for (int i = 0; i < 3; i++)
+ color_srgb.f[i] =
util_format_linear_to_srgb_float(color->f[i]);
+ color_srgb.f[3] = color->f[3];
+ memcpy(data + 4, color_srgb.ui, sizeof(color->ui));
+ } else {
+ memcpy(data + 4, color->ui, sizeof(color->ui));
+ }
+
+ si_compute_internal_begin(sctx);
+ sctx->flags |= SI_CONTEXT_CS_PARTIAL_FLUSH |
+ si_get_flush_flags(sctx, SI_COHERENCY_SHADER, L2_STREAM);
+ si_make_CB_shader_coherent(sctx, dstsurf->texture->nr_samples, true);
+
+ struct pipe_constant_buffer saved_cb = {};
+ si_get_pipe_constant_buffer(sctx,
Re: [Mesa-dev] [PATCH 1/2] gallium: Enable aarch64 NEON CPU detection.
Matt Turner writes:
> NEON (now called ASIMD) is available on all aarch64 CPUs. It seems that
> our code was missing an aarch64 path, leading to util_cpu_caps.has_neon
> always being false on aarch64. I think that means that the NEON tiling
> code in vc4 would not be enabled on aarch64 (vc4_load_lt_image_neon,
> etc).
> ---
> I have very little clue about aarch64 ABIs, so I don't know if there's
> another case that needs to be handled -- aarch32 maybe? Does
> PIPE_ARCH_AARCH64 just mean ARMv8 and so we should check something else
> for the ABI and choose Elf{32,64} based on that?
Do we actually need to do runtime detection? It sounds like "standard"
armv8 is guaranteed NEON:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0024a/CEGDJGGC.html
For vc4, the aarch64 build uses neon in the _base version of the tiling
implementation.
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] anv/allocator: Avoid race condition in anv_block_pool_map.
Accessing bo->map and then pool->center_bo_offset without a lock is
racy. One way of avoiding such race condition is to store the bo->map +
center_bo_offset into pool->map at the time the block pool is growing,
which happens within a lock.
v2: Only set pool->map if not using softpin (Jason).
Cc: Jason Ekstrand
Reported-by: Ian Romanick
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109442
Fixes: fc3f58832015cbb177179e7f3420d3611479b4a9
---
src/intel/vulkan/anv_allocator.c | 11 +--
src/intel/vulkan/anv_private.h | 13 +
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/src/intel/vulkan/anv_allocator.c b/src/intel/vulkan/anv_allocator.c
index 89f26789c85..67f9c2a948d 100644
--- a/src/intel/vulkan/anv_allocator.c
+++ b/src/intel/vulkan/anv_allocator.c
@@ -437,6 +437,7 @@ anv_block_pool_init(struct anv_block_pool *pool,
pool->nbos = 0;
pool->size = 0;
pool->start_address = gen_canonical_address(start_address);
+ pool->map = NULL;
/* This pointer will always point to the first BO in the list */
pool->bo = >bos[0];
@@ -576,6 +577,8 @@ anv_block_pool_expand_range(struct anv_block_pool *pool,
/* Now that we successfull allocated everything, we can write the new
* center_bo_offset back into pool. */
pool->center_bo_offset = center_bo_offset;
+ if (!use_softpin)
+ pool->map = map + center_bo_offset;
/* For block pool BOs we have to be a bit careful about where we place them
* in the GTT. There are two documented workarounds for state base address
@@ -670,8 +673,12 @@ anv_block_pool_get_bo(struct anv_block_pool *pool, int32_t
*offset)
void*
anv_block_pool_map(struct anv_block_pool *pool, int32_t offset)
{
- struct anv_bo *bo = anv_block_pool_get_bo(pool, );
- return bo->map + pool->center_bo_offset + offset;
+ if (pool->bo_flags & EXEC_OBJECT_PINNED) {
+ struct anv_bo *bo = anv_block_pool_get_bo(pool, );
+ return bo->map + offset;
+ } else {
+ return pool->map + offset;
+ }
}
/** Grows and re-centers the block pool.
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 3889065c93c..110b2ccf023 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -663,6 +663,19 @@ struct anv_block_pool {
*/
uint32_t center_bo_offset;
+ /* Current memory map of the block pool. This pointer may or may not
+* point to the actual beginning of the block pool memory. If
+* anv_block_pool_alloc_back has ever been called, then this pointer
+* will point to the "center" position of the buffer and all offsets
+* (negative or positive) given out by the block pool alloc functions
+* will be valid relative to this pointer.
+*
+* In particular, map == bo.map + center_offset
+*
+* DO NOT access this pointer directly. Use anv_block_pool_map() instead,
+* since it will handle the softpin case as well, where this points to NULL.
+*/
+ void *map;
int fd;
/**
--
2.17.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 07/32] intel/isl: Rename ISL_TILING_Yf/s to ISL_TILING_GEN9_Yf/s
On Fri, Oct 12, 2018 at 01:46:37PM -0500, Jason Ekstrand wrote:
> The Yf and Ys tilings change a bit between gen9 and gen10 so we have to
> be able to distinguish between them.
> ---
> src/intel/isl/isl.c | 12 ++--
> src/intel/isl/isl.h | 16
> src/intel/isl/isl_drm.c | 4 ++--
> src/intel/isl/isl_gen7.c | 8
> src/intel/isl/isl_gen9.c | 2 +-
> src/intel/isl/isl_surface_state.c | 4 ++--
> 6 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index d6beee987b5..392c15ca3fb 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -217,9 +217,9 @@ isl_tiling_get_info(enum isl_tiling tiling,
>phys_B = isl_extent2d(128, 32);
>break;
>
> - case ISL_TILING_Yf:
> - case ISL_TILING_Ys: {
> - bool is_Ys = tiling == ISL_TILING_Ys;
> + case ISL_TILING_GEN9_Yf:
> + case ISL_TILING_GEN9_Ys: {
> + bool is_Ys = tiling == ISL_TILING_GEN9_Ys;
>
>assert(bs > 0);
>unsigned width = 1 << (6 + (ffs(bs) / 2) + (2 * is_Ys));
> @@ -375,8 +375,8 @@ isl_surf_choose_tiling(const struct isl_device *dev,
>CHOOSE(ISL_TILING_LINEAR);
> }
>
> - CHOOSE(ISL_TILING_Ys);
> - CHOOSE(ISL_TILING_Yf);
> + CHOOSE(ISL_TILING_GEN9_Ys);
> + CHOOSE(ISL_TILING_GEN9_Yf);
> CHOOSE(ISL_TILING_Y0);
> CHOOSE(ISL_TILING_X);
> CHOOSE(ISL_TILING_W);
> @@ -715,7 +715,7 @@ isl_calc_phys_level0_extent_sa(const struct isl_device
> *dev,
> assert(dim_layout == ISL_DIM_LAYOUT_GEN4_2D ||
> dim_layout == ISL_DIM_LAYOUT_GEN6_STENCIL_HIZ);
>
> - if (tiling == ISL_TILING_Ys && info->samples > 1)
> + if (tiling == ISL_TILING_GEN9_Ys && info->samples > 1)
> isl_finishme("%s:%s: multisample TileYs layout", __FILE__,
> __func__);
>
Shouldn't the next patch be updated with a similar change?
>switch (msaa_layout) {
> diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> index 4f8d38e22fb..1c7990f2dc7 100644
> --- a/src/intel/isl/isl.h
> +++ b/src/intel/isl/isl.h
> @@ -460,8 +460,8 @@ enum isl_tiling {
> ISL_TILING_W,
> ISL_TILING_X,
> ISL_TILING_Y0, /**< Legacy Y tiling */
> - ISL_TILING_Yf, /**< Standard 4K tiling. The 'f' means "four". */
> - ISL_TILING_Ys, /**< Standard 64K tiling. The 's' means "sixty-four". */
> + ISL_TILING_GEN9_Yf, /**< Standard 4K tiling. The 'f' means "four". */
> + ISL_TILING_GEN9_Ys, /**< Standard 64K tiling. The 's' means "sixty-four".
> */
> ISL_TILING_HIZ, /**< Tiling format for HiZ surfaces */
> ISL_TILING_CCS, /**< Tiling format for CCS surfaces */
> };
> @@ -475,8 +475,8 @@ typedef uint32_t isl_tiling_flags_t;
> #define ISL_TILING_W_BIT (1u << ISL_TILING_W)
> #define ISL_TILING_X_BIT (1u << ISL_TILING_X)
> #define ISL_TILING_Y0_BIT (1u << ISL_TILING_Y0)
> -#define ISL_TILING_Yf_BIT (1u << ISL_TILING_Yf)
> -#define ISL_TILING_Ys_BIT (1u << ISL_TILING_Ys)
> +#define ISL_TILING_GEN9_Yf_BIT(1u << ISL_TILING_GEN9_Yf)
> +#define ISL_TILING_GEN9_Ys_BIT(1u << ISL_TILING_GEN9_Ys)
> #define ISL_TILING_HIZ_BIT(1u << ISL_TILING_HIZ)
> #define ISL_TILING_CCS_BIT(1u << ISL_TILING_CCS)
> #define ISL_TILING_ANY_MASK (~0u)
> @@ -484,12 +484,12 @@ typedef uint32_t isl_tiling_flags_t;
>
> /** Any Y tiling, including legacy Y tiling. */
> #define ISL_TILING_ANY_Y_MASK (ISL_TILING_Y0_BIT | \
> - ISL_TILING_Yf_BIT | \
> - ISL_TILING_Ys_BIT)
> + ISL_TILING_GEN9_Yf_BIT | \
> + ISL_TILING_GEN9_Ys_BIT)
>
> /** The Skylake BSpec refers to Yf and Ys as "standard tiling formats". */
> -#define ISL_TILING_STD_Y_MASK (ISL_TILING_Yf_BIT | \
> - ISL_TILING_Ys_BIT)
> +#define ISL_TILING_STD_Y_MASK (ISL_TILING_GEN9_Yf_BIT | \
> + ISL_TILING_GEN9_Ys_BIT)
> /** @} */
>
> /**
> diff --git a/src/intel/isl/isl_drm.c b/src/intel/isl/isl_drm.c
> index e16d7b63917..62fdd22d10d 100644
> --- a/src/intel/isl/isl_drm.c
> +++ b/src/intel/isl/isl_drm.c
> @@ -44,8 +44,8 @@ isl_tiling_to_i915_tiling(enum isl_tiling tiling)
>return I915_TILING_Y;
>
> case ISL_TILING_W:
> - case ISL_TILING_Yf:
> - case ISL_TILING_Ys:
> + case ISL_TILING_GEN9_Yf:
> + case ISL_TILING_GEN9_Ys:
> case ISL_TILING_HIZ:
> case ISL_TILING_CCS:
>return I915_TILING_NONE;
> diff --git a/src/intel/isl/isl_gen7.c b/src/intel/isl/isl_gen7.c
> index a9db21fba52..91cea299abc 100644
> --- a/src/intel/isl/isl_gen7.c
> +++ b/src/intel/isl/isl_gen7.c
> @@ -198,15 +198,15 @@
Re: [Mesa-dev] [PATCH] anv/allocator: Avoid race condition in anv_block_pool_map.
On Wed, Jan 23, 2019 at 2:45 PM Rafael Antognolli <
rafael.antogno...@intel.com> wrote:
> Accessing bo->map and then pool->center_bo_offset without a lock is
> racy. One way of avoiding such race condition is to store the bo->map +
> center_bo_offset into pool->map at the time the block pool is growing,
> which happens within a lock.
>
> Cc: Jason Ekstrand
> Reported-by: Ian Romanick
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109442
> Fixes: fc3f58832015cbb177179e7f3420d3611479b4a9
> ---
> src/intel/vulkan/anv_allocator.c | 10 --
> src/intel/vulkan/anv_private.h | 13 +
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_allocator.c
> b/src/intel/vulkan/anv_allocator.c
> index 89f26789c85..0bfe55bf684 100644
> --- a/src/intel/vulkan/anv_allocator.c
> +++ b/src/intel/vulkan/anv_allocator.c
> @@ -437,6 +437,7 @@ anv_block_pool_init(struct anv_block_pool *pool,
> pool->nbos = 0;
> pool->size = 0;
> pool->start_address = gen_canonical_address(start_address);
> + pool->map = NULL;
>
> /* This pointer will always point to the first BO in the list */
> pool->bo = >bos[0];
> @@ -575,6 +576,7 @@ anv_block_pool_expand_range(struct anv_block_pool
> *pool,
>
> /* Now that we successfull allocated everything, we can write the new
> * center_bo_offset back into pool. */
> + pool->map = map + center_bo_offset;
>
We should only set this when we are NOT using softpin. Otherwise, we'll
have a non-NULL map pointer that doesn't do what it looks like it does.
--Jason
> pool->center_bo_offset = center_bo_offset;
>
> /* For block pool BOs we have to be a bit careful about where we place
> them
> @@ -670,8 +672,12 @@ anv_block_pool_get_bo(struct anv_block_pool *pool,
> int32_t *offset)
> void*
> anv_block_pool_map(struct anv_block_pool *pool, int32_t offset)
> {
> - struct anv_bo *bo = anv_block_pool_get_bo(pool, );
> - return bo->map + pool->center_bo_offset + offset;
> + if (pool->bo_flags & EXEC_OBJECT_PINNED) {
> + struct anv_bo *bo = anv_block_pool_get_bo(pool, );
> + return bo->map + offset;
> + } else {
> + return pool->map + offset;
> + }
> }
>
> /** Grows and re-centers the block pool.
> diff --git a/src/intel/vulkan/anv_private.h
> b/src/intel/vulkan/anv_private.h
> index 3889065c93c..110b2ccf023 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -663,6 +663,19 @@ struct anv_block_pool {
> */
> uint32_t center_bo_offset;
>
> + /* Current memory map of the block pool. This pointer may or may not
> +* point to the actual beginning of the block pool memory. If
> +* anv_block_pool_alloc_back has ever been called, then this pointer
> +* will point to the "center" position of the buffer and all offsets
> +* (negative or positive) given out by the block pool alloc functions
> +* will be valid relative to this pointer.
> +*
> +* In particular, map == bo.map + center_offset
> +*
> +* DO NOT access this pointer directly. Use anv_block_pool_map()
> instead,
> +* since it will handle the softpin case as well, where this points to
> NULL.
> +*/
> + void *map;
> int fd;
>
> /**
> --
> 2.17.2
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: add note about sending merge-requests from forks
On Wed, 23 Jan 2019 at 17:21, Erik Faye-Lund wrote: > > On Wed, 2019-01-23 at 17:03 +, Emil Velikov wrote: > > Hi all, > > > > In case the patch is still outstanding, feel free to add > > Reviewed-by: Emil Velikov > > > > Thanks, pushed. > > > On Wed, 23 Jan 2019 at 11:20, Erik Faye-Lund > > wrote: > > > On Wed, 2019-01-23 at 10:42 +, Eric Engestrom wrote: > > > > On Wednesday, 2019-01-23 11:21:27 +0100, Erik Faye-Lund wrote: > > > > > On Wed, 2019-01-23 at 10:14 +, Daniel Stone wrote: > > > > > > Hi, > > > > > > > > > > > > On Wed, 23 Jan 2019 at 10:09, Eric Engestrom < > > > > > > eric.engest...@intel.com> wrote: > > > > > > > On Wednesday, 2019-01-23 10:36:15 +0100, Erik Faye-Lund > > > > > > > wrote: > > > > > > > > Sending MRs from the main Mesa repository increase > > > > > > > > clutter in > > > > > > > > the > > > > > > > > repository, and decrease visibility of project-wide > > > > > > > > branches. > > > > > > > > So > > > > > > > > it's > > > > > > > > better if MRs are sent from forks instead. > > > > > > > > > > > > > > > > Let's add a note about this, in case its not obvious to > > > > > > > > everyone. > > > > > > > > > > > > > > Yes please, we already have way too many dead branches in > > > > > > > the > > > > > > > main > > > > > > > repo > > > > > > > without adding that kind of things to it. > > > > > > > > > > > > > > One other thing is that (last time I checked) the scripts > > > > > > > propagating > > > > > > > the repo to github et al. don't propagate branch deletions, > > > > > > > which > > > > > > > means > > > > > > > the clutter never gets cleaned there. > > > > > > > > > > > > They're propagated from gitlab.fd.o to git.fd.o, as the hook > > > > > > is > > > > > > run > > > > > > within regular post-receive, but you're right that pushing > > > > > > from > > > > > > git.fd.o to GitHub doesn't notice old branches, as it just > > > > > > pushes > > > > > > everything present in the git.fd.o repo to GitHub, and > > > > > > doesn't > > > > > > notice > > > > > > any branches no longer present. > > > > > > > > > > Yeah, and then add the problem that forking in Github copies > > > > > *all* > > > > > branches as well (not just the default branch), means that > > > > > these > > > > > branches keeps getting replicated over there. It gets nasty > > > > > quickly. > > > > > > > > > > Perhaps we should do a manual spring cleaning in the repo soon, > > > > > moving > > > > > old, stale branches out to some historical repo and making sure > > > > > official forks don't have any cruft? > > > > > > > > I looked at that a year or two ago, but I quickly gave up. You're > > > > welcome > > > > to give it a go, but be aware that there's a very large number of > > > > very old > > > > branches. > > > > > > Looks quite managable to me. There's really only a few branches > > > that I > > > think should be kept int he main repo, and I think they can be > > > enumerated like this: > > > > > > git for-each-ref refs/remotes/origin --format "%(refname:lstrip=3)" > > > | > > > grep -E "^[0-9]+\.[0-9]+$|^mesa_[0-9]+_[0-9]+_branch$|^staging/[0- > > > 9]+\.[0-9]+$" > > > > > > Those seems to be *actual* release-branches. There's some other > > > "almost > > > release-branches", like these: > > > > > > $ git for-each-ref refs/remotes/origin --format > > > "%(refname:lstrip=3)" | > > > grep -vE "^[0-9]+\.[0-9]+$|^mesa_[0-9]+_[0-9]+_branch$|^staging/[0- > > > 9]+\.[0-9]+$" | grep -E "^mesa_[0-9]+_branch$|^staging/|^[0- > > > 9]+\.[0- > > > 9]+" > > > 18.1-proposed > > > 7.8-gles > > > mesa_20040127_branch > > > mesa_20040309_branch > > > mesa_20050114_branch > > > staging/18.2-ci > > > > > > I suppose to be conservative we could include these as well. > > > > > > But perhaps someone like Emil could chime in here? > > > > > > Personally, I think we could go as far as removing all "closed" > > > release > > > branches as well; we can just branch out from the most recent tag > > > if we > > > need to back-port something in the future. > > > > > Before we start removing things I think we should consider: > > - what extra size savings are we talking about > > If it's something like, say 10MiB personally I would not bother. > > > > - but most importantly, what the goal is - a) remove merged work, b) > > reduce size c) list only active branches, d) other > > Personally a) and b) sound perfectly reasonable, while c) seems like > > bike shedding. > > I'm not sure that's the correct use of the term "bike shedding". You > might think that it's vanity? > Agreed, vanity is closer to what I was thinking off. > Anyway, the goal isn't really either of those, but it's more closely > related to c) than any of the other points. > > The goal is really to make it easier to browse the repository. Having > everyone's pet branch around in the main repo doesn't really make sense > now that forking is literally a press of a button. And it clutters up > the repository, making browsing the branches harder. > Fwiw
[Mesa-dev] [Bug 104166] swr: Scons build - Add support for AVX512 configuration
https://bugs.freedesktop.org/show_bug.cgi?id=104166 --- Comment #1 from Alex Granni --- It looks this won't be done. Which means the only option for swr AVX512 windows binaries is Meson build with Mingw. I don't know if this works in practice. -- You are receiving this mail because: You are the assignee for the bug. You are the QA Contact for the bug.___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 106843] Cannot build osmesa with GLES (shared glapi) using Scons and MSVC
https://bugs.freedesktop.org/show_bug.cgi?id=106843 Alex Granni changed: What|Removed |Added Resolution|--- |FIXED Status|NEW |RESOLVED --- Comment #10 from Alex Granni --- This got fixed by having offending feature removed. -- You are receiving this mail because: You are the assignee for the bug. You are the QA Contact for the bug.___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 109443] Build failure with MSVC 2017 when using Scons >= 3.0.3
https://bugs.freedesktop.org/show_bug.cgi?id=109443 Bug ID: 109443 Summary: Build failure with MSVC 2017 when using Scons >= 3.0.3 Product: Mesa Version: unspecified Hardware: Other OS: All Status: NEW Severity: normal Priority: medium Component: glsl-compiler Assignee: mesa-dev@lists.freedesktop.org Reporter: liviupro...@yahoo.com QA Contact: intel-3d-b...@lists.freedesktop.org Created attachment 143208 --> https://bugs.freedesktop.org/attachment.cgi?id=143208=edit Mesa3D MSVC build fails with Scons >= 3.0.3 It appears that Scons 3.0.3 and newer (3.0.4 at the time of writing this) cannot be used to build Mesa3D. Have to stick to 3.0.1. Scons 3.0.2 has been quickly pulled. Something trips the build into looking for a missing header. -- You are receiving this mail because: You are the assignee for the bug.___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] anv/allocator: Avoid race condition in anv_block_pool_map.
Accessing bo->map and then pool->center_bo_offset without a lock is
racy. One way of avoiding such race condition is to store the bo->map +
center_bo_offset into pool->map at the time the block pool is growing,
which happens within a lock.
Cc: Jason Ekstrand
Reported-by: Ian Romanick
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109442
Fixes: fc3f58832015cbb177179e7f3420d3611479b4a9
---
src/intel/vulkan/anv_allocator.c | 10 --
src/intel/vulkan/anv_private.h | 13 +
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/intel/vulkan/anv_allocator.c b/src/intel/vulkan/anv_allocator.c
index 89f26789c85..0bfe55bf684 100644
--- a/src/intel/vulkan/anv_allocator.c
+++ b/src/intel/vulkan/anv_allocator.c
@@ -437,6 +437,7 @@ anv_block_pool_init(struct anv_block_pool *pool,
pool->nbos = 0;
pool->size = 0;
pool->start_address = gen_canonical_address(start_address);
+ pool->map = NULL;
/* This pointer will always point to the first BO in the list */
pool->bo = >bos[0];
@@ -575,6 +576,7 @@ anv_block_pool_expand_range(struct anv_block_pool *pool,
/* Now that we successfull allocated everything, we can write the new
* center_bo_offset back into pool. */
+ pool->map = map + center_bo_offset;
pool->center_bo_offset = center_bo_offset;
/* For block pool BOs we have to be a bit careful about where we place them
@@ -670,8 +672,12 @@ anv_block_pool_get_bo(struct anv_block_pool *pool, int32_t
*offset)
void*
anv_block_pool_map(struct anv_block_pool *pool, int32_t offset)
{
- struct anv_bo *bo = anv_block_pool_get_bo(pool, );
- return bo->map + pool->center_bo_offset + offset;
+ if (pool->bo_flags & EXEC_OBJECT_PINNED) {
+ struct anv_bo *bo = anv_block_pool_get_bo(pool, );
+ return bo->map + offset;
+ } else {
+ return pool->map + offset;
+ }
}
/** Grows and re-centers the block pool.
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 3889065c93c..110b2ccf023 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -663,6 +663,19 @@ struct anv_block_pool {
*/
uint32_t center_bo_offset;
+ /* Current memory map of the block pool. This pointer may or may not
+* point to the actual beginning of the block pool memory. If
+* anv_block_pool_alloc_back has ever been called, then this pointer
+* will point to the "center" position of the buffer and all offsets
+* (negative or positive) given out by the block pool alloc functions
+* will be valid relative to this pointer.
+*
+* In particular, map == bo.map + center_offset
+*
+* DO NOT access this pointer directly. Use anv_block_pool_map() instead,
+* since it will handle the softpin case as well, where this points to NULL.
+*/
+ void *map;
int fd;
/**
--
2.17.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v1] nir: Length of boolean vtn_value now is 1
On Wed, Jan 23, 2019 at 7:33 AM Sergii Romantsov
wrote:
> Hello, Jason. Sorry for belated answer.
> Why length of 1 was selected: primary type SpvOpTypeBool is already using
> length 1 (and description "For Workgroup pointers, this is the size of the
> referenced type.").
>
> Additionally with length of 4 CI gives 3 failed tests:
> https://mesa-ci.01.org/global_logic/builds/56/group/63a9f0ea7bb98050796b649e85481845
> .
> With length of 1 it passes:
> https://mesa-ci.01.org/global_logic/builds/57/group/63a9f0ea7bb98050796b649e85481845
>
I think those failures are spurious. I've not been able to reproduce
either of them locally. I'm re-running the CI run and I expect it'll
pass. If it does, I'll push this with 4.
--Jason
> So do we still need to use 4?
>
> On Thu, Jan 17, 2019 at 7:13 PM Jason Ekstrand
> wrote:
>
>> I think this is probably mostly ok. There's still some question about
>> exactly what gets stored there and who is storing it.
>>
>> On Thu, Jan 17, 2019 at 4:34 AM apinheiro wrote:
>>
>>> I was waiting Jason to chime in, but just in case he is too busy I took
>>> a look in detail to the patch. It LGTM, so assuming it passes intel CI:
>>>
>>> Reviewed-by: Alejandro Piñeiro
>>>
>>> On 15/1/19 12:08, Sergii Romantsov wrote:
>>> > During conversion type-length was lost due to math.
>>> >
>>> > CC: Jason Ekstrand
>>> > Fixes: 44227453ec03 (nir: Switch to using 1-bit Booleans for almost
>>> everything)
>>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109353
>>> > Signed-off-by: Sergii Romantsov
>>> > ---
>>> > src/compiler/spirv/spirv_to_nir.c | 9 ++---
>>> > 1 file changed, 6 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/src/compiler/spirv/spirv_to_nir.c
>>> b/src/compiler/spirv/spirv_to_nir.c
>>> > index e3dc619..faad771 100644
>>> > --- a/src/compiler/spirv/spirv_to_nir.c
>>> > +++ b/src/compiler/spirv/spirv_to_nir.c
>>> > @@ -1042,14 +1042,16 @@ vtn_type_layout_std430(struct vtn_builder *b,
>>> struct vtn_type *type,
>>> > {
>>> > switch (type->base_type) {
>>> > case vtn_base_type_scalar: {
>>> > - uint32_t comp_size = glsl_get_bit_size(type->type) / 8;
>>> > + uint32_t comp_size = glsl_type_is_boolean(type->type)
>>> > + ? 1 : glsl_get_bit_size(type->type) / 8;
>>>
>>
>> Because we don't know in spirv_to_nir what size the back-end is going to
>> use for booleans, we should assume they're 32-bit and use 4 here and
>> below. Otherwise, we may end up with booleans overlapping other stuff.
>>
>>
>>> > *size_out = comp_size;
>>> > *align_out = comp_size;
>>> > return type;
>>> > }
>>> >
>>> > case vtn_base_type_vector: {
>>> > - uint32_t comp_size = glsl_get_bit_size(type->type) / 8;
>>> > + uint32_t comp_size = glsl_type_is_boolean(type->type)
>>> > + ? 1 : glsl_get_bit_size(type->type) / 8;
>>> > unsigned align_comps = type->length == 3 ? 4 : type->length;
>>> > *size_out = comp_size * type->length,
>>> > *align_out = comp_size * align_comps;
>>> > @@ -1168,7 +1170,8 @@ vtn_handle_type(struct vtn_builder *b, SpvOp
>>> opcode,
>>> > val->type->base_type = vtn_base_type_vector;
>>> > val->type->type =
>>> glsl_vector_type(glsl_get_base_type(base->type), elems);
>>> > val->type->length = elems;
>>> > - val->type->stride = glsl_get_bit_size(base->type) / 8;
>>> > + val->type->stride = glsl_type_is_boolean(val->type->type)
>>> > + ? 1 : glsl_get_bit_size(base->type) / 8;
>>> > val->type->array_element = base;
>>> > break;
>>> > }
>>> ___
>>> 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/2] pl111: Rename the pl111 driver to "kmsro".
On Thu, Oct 25, 2018 at 11:39 AM Eric Anholt wrote: > > The vc4 driver can do prime sharing to many different KMS-only devices, > such as the various tinydrm drivers for SPI-attached displays. Rename the > driver away from "pl111" to represent what it will actually support: > various sorts of KMS displays with the renderonly layer used to attach a > GPU. I was about to start writing this same patch... I've started looking at the lima and panfrost drivers. The many combinations of Mali GPUs and DC isn't going to scale. The lima and panfrost trees can't even co-exist as both define a rockchip winsys which load different GPU drivers. The same will be true for meson, hisilicon, allwinner, etc. i.MX is about to be in the same boat needing to support both etnaviv and freedreno. What do we need to do to merge this? There was some discussion on patch 2, but I think dealing with the loader is a separate issue. With this patch, we're left with only some very simple boilerplate to add new kms drivers. Rob ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: add note about sending merge-requests from forks
On Wed, 2019-01-23 at 17:03 +, Emil Velikov wrote: > Hi all, > > In case the patch is still outstanding, feel free to add > Reviewed-by: Emil Velikov > Thanks, pushed. > On Wed, 23 Jan 2019 at 11:20, Erik Faye-Lund > wrote: > > On Wed, 2019-01-23 at 10:42 +, Eric Engestrom wrote: > > > On Wednesday, 2019-01-23 11:21:27 +0100, Erik Faye-Lund wrote: > > > > On Wed, 2019-01-23 at 10:14 +, Daniel Stone wrote: > > > > > Hi, > > > > > > > > > > On Wed, 23 Jan 2019 at 10:09, Eric Engestrom < > > > > > eric.engest...@intel.com> wrote: > > > > > > On Wednesday, 2019-01-23 10:36:15 +0100, Erik Faye-Lund > > > > > > wrote: > > > > > > > Sending MRs from the main Mesa repository increase > > > > > > > clutter in > > > > > > > the > > > > > > > repository, and decrease visibility of project-wide > > > > > > > branches. > > > > > > > So > > > > > > > it's > > > > > > > better if MRs are sent from forks instead. > > > > > > > > > > > > > > Let's add a note about this, in case its not obvious to > > > > > > > everyone. > > > > > > > > > > > > Yes please, we already have way too many dead branches in > > > > > > the > > > > > > main > > > > > > repo > > > > > > without adding that kind of things to it. > > > > > > > > > > > > One other thing is that (last time I checked) the scripts > > > > > > propagating > > > > > > the repo to github et al. don't propagate branch deletions, > > > > > > which > > > > > > means > > > > > > the clutter never gets cleaned there. > > > > > > > > > > They're propagated from gitlab.fd.o to git.fd.o, as the hook > > > > > is > > > > > run > > > > > within regular post-receive, but you're right that pushing > > > > > from > > > > > git.fd.o to GitHub doesn't notice old branches, as it just > > > > > pushes > > > > > everything present in the git.fd.o repo to GitHub, and > > > > > doesn't > > > > > notice > > > > > any branches no longer present. > > > > > > > > Yeah, and then add the problem that forking in Github copies > > > > *all* > > > > branches as well (not just the default branch), means that > > > > these > > > > branches keeps getting replicated over there. It gets nasty > > > > quickly. > > > > > > > > Perhaps we should do a manual spring cleaning in the repo soon, > > > > moving > > > > old, stale branches out to some historical repo and making sure > > > > official forks don't have any cruft? > > > > > > I looked at that a year or two ago, but I quickly gave up. You're > > > welcome > > > to give it a go, but be aware that there's a very large number of > > > very old > > > branches. > > > > Looks quite managable to me. There's really only a few branches > > that I > > think should be kept int he main repo, and I think they can be > > enumerated like this: > > > > git for-each-ref refs/remotes/origin --format "%(refname:lstrip=3)" > > | > > grep -E "^[0-9]+\.[0-9]+$|^mesa_[0-9]+_[0-9]+_branch$|^staging/[0- > > 9]+\.[0-9]+$" > > > > Those seems to be *actual* release-branches. There's some other > > "almost > > release-branches", like these: > > > > $ git for-each-ref refs/remotes/origin --format > > "%(refname:lstrip=3)" | > > grep -vE "^[0-9]+\.[0-9]+$|^mesa_[0-9]+_[0-9]+_branch$|^staging/[0- > > 9]+\.[0-9]+$" | grep -E "^mesa_[0-9]+_branch$|^staging/|^[0- > > 9]+\.[0- > > 9]+" > > 18.1-proposed > > 7.8-gles > > mesa_20040127_branch > > mesa_20040309_branch > > mesa_20050114_branch > > staging/18.2-ci > > > > I suppose to be conservative we could include these as well. > > > > But perhaps someone like Emil could chime in here? > > > > Personally, I think we could go as far as removing all "closed" > > release > > branches as well; we can just branch out from the most recent tag > > if we > > need to back-port something in the future. > > > Before we start removing things I think we should consider: > - what extra size savings are we talking about > If it's something like, say 10MiB personally I would not bother. > > - but most importantly, what the goal is - a) remove merged work, b) > reduce size c) list only active branches, d) other > Personally a) and b) sound perfectly reasonable, while c) seems like > bike shedding. I'm not sure that's the correct use of the term "bike shedding". You might think that it's vanity? Anyway, the goal isn't really either of those, but it's more closely related to c) than any of the other points. The goal is really to make it easier to browse the repository. Having everyone's pet branch around in the main repo doesn't really make sense now that forking is literally a press of a button. And it clutters up the repository, making browsing the branches harder. So yes, arguably it *is* very much a vanity thing, but I don't think it is in a bad way; we're not losing any data, but we *are* moving some historical data. I doubt anyone will care apart from it being easier to find what you're looking for. > If people want to go with c) the first regular expression from Erik, > that lists all the release branches,
Re: [Mesa-dev] Thoughts after hitting 100 merge requests?
On Wed, Jan 23, 2019 at 11:01:46AM +, Eric Engestrom wrote:
> On Friday, 2019-01-11 09:50:25 -0800, Caio Marcelo de Oliveira Filho wrote:
> [snip]
> > - To find the discussion associated with a commit in master, I'd
> > search the title in the mailing list archives. With MRs, the usual
> > way that this connection is made would be the reference to the MR as
> > part of the merge commit message, but in Mesa we don't currently use
> > merge commits. I've tried to search for commit titles in MR
> > interface but it doesn't find them.
>
> If you go on the gitlab page for a commit that comes from an MR, eg.
> https://gitlab.freedesktop.org/mesa/mesa/commit/41a0c0039225753b26f2ce61b49fef8d45c616ad
>
> In the gray box between the commit message and the diff, you can see
> a "merge" logo followed by:
> > 1 merge request !135 travis: fix autotools builds after --enable-autotools
> > switch addition
Thanks, Daniel Stone pointed that out earlier in the thread. I'm also
using the "web API" to script this interaction.
commit=$(git rev-parse $1)
curl -s
https://gitlab.freedesktop.org/api/v4/projects/mesa%2fmesa/repository/commits/${commit}/merge_requests
| jq -r '.[0].title, .[0].web_url'
Caio
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: add note about sending merge-requests from forks
Hi all, In case the patch is still outstanding, feel free to add Reviewed-by: Emil Velikov On Wed, 23 Jan 2019 at 11:20, Erik Faye-Lund wrote: > > On Wed, 2019-01-23 at 10:42 +, Eric Engestrom wrote: > > On Wednesday, 2019-01-23 11:21:27 +0100, Erik Faye-Lund wrote: > > > On Wed, 2019-01-23 at 10:14 +, Daniel Stone wrote: > > > > Hi, > > > > > > > > On Wed, 23 Jan 2019 at 10:09, Eric Engestrom < > > > > eric.engest...@intel.com> wrote: > > > > > On Wednesday, 2019-01-23 10:36:15 +0100, Erik Faye-Lund wrote: > > > > > > Sending MRs from the main Mesa repository increase clutter in > > > > > > the > > > > > > repository, and decrease visibility of project-wide branches. > > > > > > So > > > > > > it's > > > > > > better if MRs are sent from forks instead. > > > > > > > > > > > > Let's add a note about this, in case its not obvious to > > > > > > everyone. > > > > > > > > > > Yes please, we already have way too many dead branches in the > > > > > main > > > > > repo > > > > > without adding that kind of things to it. > > > > > > > > > > One other thing is that (last time I checked) the scripts > > > > > propagating > > > > > the repo to github et al. don't propagate branch deletions, > > > > > which > > > > > means > > > > > the clutter never gets cleaned there. > > > > > > > > They're propagated from gitlab.fd.o to git.fd.o, as the hook is > > > > run > > > > within regular post-receive, but you're right that pushing from > > > > git.fd.o to GitHub doesn't notice old branches, as it just pushes > > > > everything present in the git.fd.o repo to GitHub, and doesn't > > > > notice > > > > any branches no longer present. > > > > > > Yeah, and then add the problem that forking in Github copies *all* > > > branches as well (not just the default branch), means that these > > > branches keeps getting replicated over there. It gets nasty > > > quickly. > > > > > > Perhaps we should do a manual spring cleaning in the repo soon, > > > moving > > > old, stale branches out to some historical repo and making sure > > > official forks don't have any cruft? > > > > I looked at that a year or two ago, but I quickly gave up. You're > > welcome > > to give it a go, but be aware that there's a very large number of > > very old > > branches. > > Looks quite managable to me. There's really only a few branches that I > think should be kept int he main repo, and I think they can be > enumerated like this: > > git for-each-ref refs/remotes/origin --format "%(refname:lstrip=3)" | > grep -E "^[0-9]+\.[0-9]+$|^mesa_[0-9]+_[0-9]+_branch$|^staging/[0- > 9]+\.[0-9]+$" > > Those seems to be *actual* release-branches. There's some other "almost > release-branches", like these: > > $ git for-each-ref refs/remotes/origin --format "%(refname:lstrip=3)" | > grep -vE "^[0-9]+\.[0-9]+$|^mesa_[0-9]+_[0-9]+_branch$|^staging/[0- > 9]+\.[0-9]+$" | grep -E "^mesa_[0-9]+_branch$|^staging/|^[0-9]+\.[0- > 9]+" > 18.1-proposed > 7.8-gles > mesa_20040127_branch > mesa_20040309_branch > mesa_20050114_branch > staging/18.2-ci > > I suppose to be conservative we could include these as well. > > But perhaps someone like Emil could chime in here? > > Personally, I think we could go as far as removing all "closed" release > branches as well; we can just branch out from the most recent tag if we > need to back-port something in the future. > Before we start removing things I think we should consider: - what extra size savings are we talking about If it's something like, say 10MiB personally I would not bother. - but most importantly, what the goal is - a) remove merged work, b) reduce size c) list only active branches, d) other Personally a) and b) sound perfectly reasonable, while c) seems like bike shedding. If people want to go with c) the first regular expression from Erik, that lists all the release branches, feels list the best solution. Considering memory serves me right, for [very] old release branches the history/tags is non-linear and some tags are detached. Hence dropping them sounds like a bad idea. HTH Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [MR] spirv/nir: handle location decorations on block interface members, plus cleaning
While doing a review of the current ARB_gl_spirv series, I found that this one also applies to Vulkan, and fixes some Vulkan tests, so I preferred to send it in advance, and keep any ARB_gl_spirv series to review to only apply to OpenGL. FWIW, this was sent a long time ago [1], but since then the patch needed several updates. We are also dropping the third patch of that series, as right now it is not working/fixing as intended (investigate it just added to my TODO, but will not have too much priority). As part of this, I also took the Vulkan tests from a custom vkrunner branch, and imported to piglit. You can find them here [2], but I will send in short to the piglit mailing list to be reviewed. [1] https://patchwork.freedesktop.org/series/40797/ [2] https://github.com/Igalia/piglit/tree/review/handle-block-member-location https://gitlab.freedesktop.org/mesa/mesa/merge_requests/142 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Thoughts after hitting 100 merge requests?
On Thursday, 2019-01-17 15:55:44 +0100, Erik Faye-Lund wrote: > It could have been made obvious for instance by showing the commit- > graph next to the commit-list, decorating it with the branch head in > one end and clear continuation in the other. I'd love that, it would really help a lot! Care to raise an issue at gitlab asking for this? :) https://gitlab.com/gitlab-org/gitlab-ce/issues/new ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 5/9] intel/compiler: relax brw_eu_validate for byte raw movs
El 23/1/19 a las 7:26, Matt Turner escribió:
> On Sun, Jul 8, 2018 at 5:27 PM, Jose Maria Casanova Crespo
> wrote:
>> When the destination is a BYTE type allow raw movs
>> even if the stride is not exact multiple of destination
>> type and exec type, execution type is Word and its size is 2.
>>
>> This restriction was only allowing stride==2 destinations
>> for 8-bit types.
>
> Super late review, obviously... it's been on my todo list but fp64 was
> taking all my time.
>
> I can't figure this commit out. What I know:
>
> - byte destination
> - raw mov (which means destination stride == 1)
I think that stride == 1 is a requirement for a raw mov based on how it
is written at the PRM KBL Vol2A, "mov - Move" (Page 1081, PDF Page 1099).
"A mov with the same source and destination type, no source modifier,
and no saturation is a raw move."
But just after this text it is a reference for stride restriction when
destination stride is 1 for byte types.
"A packed byte destination region (B or UB type with HorzStride == 1 and
ExecSize > 1) can only be written using raw move."
> - execution type of a byte operation is "word"
>
> The original code
>
>> if (exec_type_size > dst_type_size) {
>>ERROR_IF(dst_stride * dst_type_size != exec_type_size,
>> "Destination stride must be equal to the ratio of the sizes
>> of "
>> "the execution data type to the destination type");
>> }
>
> would not have worked for an instruction like
>
>> mov(8) g0<1>B g0<8,8,1>B
>
> But, that's okay because it didn't need to since the block right above
> it does this:
>
>if (dst_type_is_byte) {
> if (is_packed(exec_size * dst_stride, exec_size, dst_stride)) {
> if (!inst_is_raw_move(devinfo, inst)) {
> ERROR("Only raw MOV supports a packed-byte destination");
> return error_msg;
> } else {
> return (struct string){};
> }
> }
>}
>
> That is, if it's a raw move, return no-error.
Packed raw MOVs.
> It would be easier to understand what you were fixing if you had added
> a unit test to test_eu_validate.cpp, or (if my suspicions are correct)
> it would have proven to you that this patch wasn't correct.
Agree. And I should have also included in the commit log an example of
the kind of operations that we would like to allow.
> Was this just something that you noticed by inspection?
This issue was found because the validator was raising errors at the
shuffle/unshuffle operations when multiple 8-bit components were
prepared to be written as one 32-bit components for example for
store_ssbo So we had operations like these:
mov(8) g9<4>Bg3<8,8,1>B{ align1 1H };
mov(8) g9.1<4>B g3.8<8,8,1>B{ align1 1H };
mov(8) g9.2<4>B g3.16<8,8,1>B{ align1 1H };
mov(8) g9.3<4>B g3.24<8,8,1>B{ align1 1H };
In theses case we have not packed raw movs, and the instructions worked
perfectly fine they were tested on the HW. But reading the PRM you would
have doubts if they were really allowed because as is stated
"Destination stride must be equal to the ratio of the sizes of the
execution data type to the destination type" and the execution size of 2
for bytes makes it more complex to understand.
I suppose that the think is that in this case the PRM seems to be
under-documented, and we can consider that this is implicitly allowed.
So we need to relax the validation.
>> Reviewed-by: Jason Ekstrand
>
> Sigh.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
Iago Toral Quiroga writes:
> Commit c84ec70b3a72 implemented execution type promotion to 32-bit for
> conversions involving half-float registers, which empirical testing suggested
> was required, but it did not incorporate this change into the assembly
> validator
> logic. This commits adds that, preventing validation errors like this:
>
I don't think we should be validating empirical assumptions in the EU
validator.
> mov(16) g9<4>B g3<16,8,2>HF { align1 1H };
> ERROR: Destination stride must be equal to the ratio of the sizes of the
>execution data type to the destination type
>
> Fixes: c84ec70b3a72 "intel/fs: Promote execution type to 32-bit when any
> half-float conversion is needed."
I don't think this "fixes" anything that ever worked. The validator is
still missing an implementation of the quirky HF restrictions, and it
wasn't the purpose of c84ec70b3a72 to do such a thing. You *should*
definitely implement those restrictions (as they're stated in the
hardware spec, without empirical assumptions) in the validator as part
of your VK_KHR_shader_float16_int8 series, if anything because currently
it will reject working code that uses HF types.
> ---
> src/intel/compiler/brw_eu_validate.c | 27 ++-
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/src/intel/compiler/brw_eu_validate.c
> b/src/intel/compiler/brw_eu_validate.c
> index a25010b225c..3bb37677672 100644
> --- a/src/intel/compiler/brw_eu_validate.c
> +++ b/src/intel/compiler/brw_eu_validate.c
> @@ -325,17 +325,20 @@ execution_type(const struct gen_device_info *devinfo,
> const brw_inst *inst)
> unsigned num_sources = num_sources_from_inst(devinfo, inst);
> enum brw_reg_type src0_exec_type, src1_exec_type;
>
> - /* Execution data type is independent of destination data type, except in
> -* mixed F/HF instructions on CHV and SKL+.
> + /* Empirical testing suggests that type conversions involving half-float
> +* promote execution type to 32-bit. See get_exec_type() in brw_ir_fs.h.
> */
> enum brw_reg_type dst_exec_type = brw_inst_dst_type(devinfo, inst);
>
> src0_exec_type = execution_type_for_type(brw_inst_src0_type(devinfo,
> inst));
> if (num_sources == 1) {
> - if ((devinfo->gen >= 9 || devinfo->is_cherryview) &&
> - src0_exec_type == BRW_REGISTER_TYPE_HF) {
> - return dst_exec_type;
> + if (type_sz(src0_exec_type) == 2 && dst_exec_type != src0_exec_type) {
> + if (src0_exec_type == BRW_REGISTER_TYPE_HF)
> +return BRW_REGISTER_TYPE_F;
> + else if (dst_exec_type == BRW_REGISTER_TYPE_HF)
> +return BRW_REGISTER_TYPE_D;
>}
> +
>return src0_exec_type;
> }
>
> @@ -367,14 +370,12 @@ execution_type(const struct gen_device_info *devinfo,
> const brw_inst *inst)
> src1_exec_type == BRW_REGISTER_TYPE_DF)
>return BRW_REGISTER_TYPE_DF;
>
> - if (devinfo->gen >= 9 || devinfo->is_cherryview) {
> - if (dst_exec_type == BRW_REGISTER_TYPE_F ||
> - src0_exec_type == BRW_REGISTER_TYPE_F ||
> - src1_exec_type == BRW_REGISTER_TYPE_F) {
> - return BRW_REGISTER_TYPE_F;
> - } else {
> - return BRW_REGISTER_TYPE_HF;
> - }
> + if (dst_exec_type == BRW_REGISTER_TYPE_F ||
> + src0_exec_type == BRW_REGISTER_TYPE_F ||
> + src1_exec_type == BRW_REGISTER_TYPE_F) {
> + return BRW_REGISTER_TYPE_F;
> + } else {
> + return BRW_REGISTER_TYPE_HF;
> }
>
> assert(src0_exec_type == BRW_REGISTER_TYPE_F);
> --
> 2.17.1
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v1] nir: Length of boolean vtn_value now is 1
Hello, Jason. Sorry for belated answer.
Why length of 1 was selected: primary type SpvOpTypeBool is already using
length 1 (and description "For Workgroup pointers, this is the size of the
referenced type.").
Additionally with length of 4 CI gives 3 failed tests:
https://mesa-ci.01.org/global_logic/builds/56/group/63a9f0ea7bb98050796b649e85481845
.
With length of 1 it passes:
https://mesa-ci.01.org/global_logic/builds/57/group/63a9f0ea7bb98050796b649e85481845
So do we still need to use 4?
On Thu, Jan 17, 2019 at 7:13 PM Jason Ekstrand wrote:
> I think this is probably mostly ok. There's still some question about
> exactly what gets stored there and who is storing it.
>
> On Thu, Jan 17, 2019 at 4:34 AM apinheiro wrote:
>
>> I was waiting Jason to chime in, but just in case he is too busy I took
>> a look in detail to the patch. It LGTM, so assuming it passes intel CI:
>>
>> Reviewed-by: Alejandro Piñeiro
>>
>> On 15/1/19 12:08, Sergii Romantsov wrote:
>> > During conversion type-length was lost due to math.
>> >
>> > CC: Jason Ekstrand
>> > Fixes: 44227453ec03 (nir: Switch to using 1-bit Booleans for almost
>> everything)
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109353
>> > Signed-off-by: Sergii Romantsov
>> > ---
>> > src/compiler/spirv/spirv_to_nir.c | 9 ++---
>> > 1 file changed, 6 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/src/compiler/spirv/spirv_to_nir.c
>> b/src/compiler/spirv/spirv_to_nir.c
>> > index e3dc619..faad771 100644
>> > --- a/src/compiler/spirv/spirv_to_nir.c
>> > +++ b/src/compiler/spirv/spirv_to_nir.c
>> > @@ -1042,14 +1042,16 @@ vtn_type_layout_std430(struct vtn_builder *b,
>> struct vtn_type *type,
>> > {
>> > switch (type->base_type) {
>> > case vtn_base_type_scalar: {
>> > - uint32_t comp_size = glsl_get_bit_size(type->type) / 8;
>> > + uint32_t comp_size = glsl_type_is_boolean(type->type)
>> > + ? 1 : glsl_get_bit_size(type->type) / 8;
>>
>
> Because we don't know in spirv_to_nir what size the back-end is going to
> use for booleans, we should assume they're 32-bit and use 4 here and
> below. Otherwise, we may end up with booleans overlapping other stuff.
>
>
>> > *size_out = comp_size;
>> > *align_out = comp_size;
>> > return type;
>> > }
>> >
>> > case vtn_base_type_vector: {
>> > - uint32_t comp_size = glsl_get_bit_size(type->type) / 8;
>> > + uint32_t comp_size = glsl_type_is_boolean(type->type)
>> > + ? 1 : glsl_get_bit_size(type->type) / 8;
>> > unsigned align_comps = type->length == 3 ? 4 : type->length;
>> > *size_out = comp_size * type->length,
>> > *align_out = comp_size * align_comps;
>> > @@ -1168,7 +1170,8 @@ vtn_handle_type(struct vtn_builder *b, SpvOp
>> opcode,
>> > val->type->base_type = vtn_base_type_vector;
>> > val->type->type =
>> glsl_vector_type(glsl_get_base_type(base->type), elems);
>> > val->type->length = elems;
>> > - val->type->stride = glsl_get_bit_size(base->type) / 8;
>> > + val->type->stride = glsl_type_is_boolean(val->type->type)
>> > + ? 1 : glsl_get_bit_size(base->type) / 8;
>> > val->type->array_element = base;
>> > break;
>> > }
>> ___
>> 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
[Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion
Commit c84ec70b3a72 implemented execution type promotion to 32-bit for
conversions involving half-float registers, which empirical testing suggested
was required, but it did not incorporate this change into the assembly validator
logic. This commits adds that, preventing validation errors like this:
mov(16) g9<4>B g3<16,8,2>HF { align1 1H };
ERROR: Destination stride must be equal to the ratio of the sizes of the
execution data type to the destination type
Fixes: c84ec70b3a72 "intel/fs: Promote execution type to 32-bit when any
half-float conversion is needed."
---
src/intel/compiler/brw_eu_validate.c | 27 ++-
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/intel/compiler/brw_eu_validate.c
b/src/intel/compiler/brw_eu_validate.c
index a25010b225c..3bb37677672 100644
--- a/src/intel/compiler/brw_eu_validate.c
+++ b/src/intel/compiler/brw_eu_validate.c
@@ -325,17 +325,20 @@ execution_type(const struct gen_device_info *devinfo,
const brw_inst *inst)
unsigned num_sources = num_sources_from_inst(devinfo, inst);
enum brw_reg_type src0_exec_type, src1_exec_type;
- /* Execution data type is independent of destination data type, except in
-* mixed F/HF instructions on CHV and SKL+.
+ /* Empirical testing suggests that type conversions involving half-float
+* promote execution type to 32-bit. See get_exec_type() in brw_ir_fs.h.
*/
enum brw_reg_type dst_exec_type = brw_inst_dst_type(devinfo, inst);
src0_exec_type = execution_type_for_type(brw_inst_src0_type(devinfo, inst));
if (num_sources == 1) {
- if ((devinfo->gen >= 9 || devinfo->is_cherryview) &&
- src0_exec_type == BRW_REGISTER_TYPE_HF) {
- return dst_exec_type;
+ if (type_sz(src0_exec_type) == 2 && dst_exec_type != src0_exec_type) {
+ if (src0_exec_type == BRW_REGISTER_TYPE_HF)
+return BRW_REGISTER_TYPE_F;
+ else if (dst_exec_type == BRW_REGISTER_TYPE_HF)
+return BRW_REGISTER_TYPE_D;
}
+
return src0_exec_type;
}
@@ -367,14 +370,12 @@ execution_type(const struct gen_device_info *devinfo,
const brw_inst *inst)
src1_exec_type == BRW_REGISTER_TYPE_DF)
return BRW_REGISTER_TYPE_DF;
- if (devinfo->gen >= 9 || devinfo->is_cherryview) {
- if (dst_exec_type == BRW_REGISTER_TYPE_F ||
- src0_exec_type == BRW_REGISTER_TYPE_F ||
- src1_exec_type == BRW_REGISTER_TYPE_F) {
- return BRW_REGISTER_TYPE_F;
- } else {
- return BRW_REGISTER_TYPE_HF;
- }
+ if (dst_exec_type == BRW_REGISTER_TYPE_F ||
+ src0_exec_type == BRW_REGISTER_TYPE_F ||
+ src1_exec_type == BRW_REGISTER_TYPE_F) {
+ return BRW_REGISTER_TYPE_F;
+ } else {
+ return BRW_REGISTER_TYPE_HF;
}
assert(src0_exec_type == BRW_REGISTER_TYPE_F);
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: add note about sending merge-requests from forks
On Wed, 2019-01-23 at 10:42 +, Eric Engestrom wrote: > On Wednesday, 2019-01-23 11:21:27 +0100, Erik Faye-Lund wrote: > > On Wed, 2019-01-23 at 10:14 +, Daniel Stone wrote: > > > Hi, > > > > > > On Wed, 23 Jan 2019 at 10:09, Eric Engestrom < > > > eric.engest...@intel.com> wrote: > > > > On Wednesday, 2019-01-23 10:36:15 +0100, Erik Faye-Lund wrote: > > > > > Sending MRs from the main Mesa repository increase clutter in > > > > > the > > > > > repository, and decrease visibility of project-wide branches. > > > > > So > > > > > it's > > > > > better if MRs are sent from forks instead. > > > > > > > > > > Let's add a note about this, in case its not obvious to > > > > > everyone. > > > > > > > > Yes please, we already have way too many dead branches in the > > > > main > > > > repo > > > > without adding that kind of things to it. > > > > > > > > One other thing is that (last time I checked) the scripts > > > > propagating > > > > the repo to github et al. don't propagate branch deletions, > > > > which > > > > means > > > > the clutter never gets cleaned there. > > > > > > They're propagated from gitlab.fd.o to git.fd.o, as the hook is > > > run > > > within regular post-receive, but you're right that pushing from > > > git.fd.o to GitHub doesn't notice old branches, as it just pushes > > > everything present in the git.fd.o repo to GitHub, and doesn't > > > notice > > > any branches no longer present. > > > > Yeah, and then add the problem that forking in Github copies *all* > > branches as well (not just the default branch), means that these > > branches keeps getting replicated over there. It gets nasty > > quickly. > > > > Perhaps we should do a manual spring cleaning in the repo soon, > > moving > > old, stale branches out to some historical repo and making sure > > official forks don't have any cruft? > > I looked at that a year or two ago, but I quickly gave up. You're > welcome > to give it a go, but be aware that there's a very large number of > very old > branches. Looks quite managable to me. There's really only a few branches that I think should be kept int he main repo, and I think they can be enumerated like this: git for-each-ref refs/remotes/origin --format "%(refname:lstrip=3)" | grep -E "^[0-9]+\.[0-9]+$|^mesa_[0-9]+_[0-9]+_branch$|^staging/[0- 9]+\.[0-9]+$" Those seems to be *actual* release-branches. There's some other "almost release-branches", like these: $ git for-each-ref refs/remotes/origin --format "%(refname:lstrip=3)" | grep -vE "^[0-9]+\.[0-9]+$|^mesa_[0-9]+_[0-9]+_branch$|^staging/[0- 9]+\.[0-9]+$" | grep -E "^mesa_[0-9]+_branch$|^staging/|^[0-9]+\.[0- 9]+" 18.1-proposed 7.8-gles mesa_20040127_branch mesa_20040309_branch mesa_20050114_branch staging/18.2-ci I suppose to be conservative we could include these as well. But perhaps someone like Emil could chime in here? Personally, I think we could go as far as removing all "closed" release branches as well; we can just branch out from the most recent tag if we need to back-port something in the future. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Thoughts after hitting 100 merge requests?
On Friday, 2019-01-11 09:50:25 -0800, Caio Marcelo de Oliveira Filho wrote: [snip] > - To find the discussion associated with a commit in master, I'd > search the title in the mailing list archives. With MRs, the usual > way that this connection is made would be the reference to the MR as > part of the merge commit message, but in Mesa we don't currently use > merge commits. I've tried to search for commit titles in MR > interface but it doesn't find them. If you go on the gitlab page for a commit that comes from an MR, eg. https://gitlab.freedesktop.org/mesa/mesa/commit/41a0c0039225753b26f2ce61b49fef8d45c616ad In the gray box between the commit message and the diff, you can see a "merge" logo followed by: > 1 merge request !135 travis: fix autotools builds after --enable-autotools > switch addition ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: fix PRIMITIVES_GENERATED query after the "pipeline stat single" changes
On Wednesday, January 23, 2019 1:26:25 AM PST Erik Faye-Lund wrote:
> On Fri, 2019-01-18 at 11:27 -0500, Marek Olšák wrote:
> > From: Marek Olšák
> >
> > ---
> > src/mesa/state_tracker/st_cb_queryobj.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/state_tracker/st_cb_queryobj.c
> > b/src/mesa/state_tracker/st_cb_queryobj.c
> > index abb126547c9..642b901d05a 100644
> > --- a/src/mesa/state_tracker/st_cb_queryobj.c
> > +++ b/src/mesa/state_tracker/st_cb_queryobj.c
> > @@ -84,21 +84,22 @@ st_DeleteQuery(struct gl_context *ctx, struct
> > gl_query_object *q)
> > struct st_query_object *stq = st_query_object(q);
> >
> > free_queries(pipe, stq);
> >
> > free(stq);
> > }
> >
> > static int
> > target_to_index(const struct st_context *st, const struct
> > gl_query_object *q)
> > {
> > - if (q->Target == GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN ||
> > + if (q->Target == GL_PRIMITIVES_GENERATED ||
> > + q->Target == GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN ||
> > q->Target == GL_TRANSFORM_FEEDBACK_STREAM_OVERFLOW_ARB)
> >return q->Stream;
> >
> > if (st->has_single_pipe_stat) {
> >switch (q->Target) {
> >case GL_VERTICES_SUBMITTED_ARB:
> > return PIPE_STAT_QUERY_IA_VERTICES;
> >case GL_PRIMITIVES_SUBMITTED_ARB:
> > return PIPE_STAT_QUERY_IA_PRIMITIVES;
> >case GL_VERTEX_SHADER_INVOCATIONS_ARB:
>
> The change itself looks good to me.
>
> However, I think a commit message saying, well, *something* would be a
> good idea (and probably would have made it easier to find reviewers in
> the first place). Something like this, perhaps?
>
> "When this functionality was added, the PRIMITIVES_GENERATED query was
> accidentally omitted. This causes issues for drivers that support
> transform feedback."
>
> I also think this should have a Fixes tag:
>
> Fixes: d644698b443 ("gallium: Add the ability to query a single
> pipeline statistics counter")
>
> With those things changed:
>
> Reviewed-by: Erik Faye-Lund
>
> Also, I added Kenneth Grauke who wrote the commit in question to the CC
> list. Perhaps he has some thoughts.
Yikes. Sorry, Marek, not sure how I didn't catch this. :(
Reviewed-by: Kenneth Graunke
signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: add note about sending merge-requests from forks
On Wednesday, 2019-01-23 11:28:14 +0100, Erik Faye-Lund wrote: > On Wed, 2019-01-23 at 10:08 +, Eric Engestrom wrote: > > On Wednesday, 2019-01-23 10:36:15 +0100, Erik Faye-Lund wrote: > > > Sending MRs from the main Mesa repository increase clutter in the > > > repository, and decrease visibility of project-wide branches. So > > > it's > > > better if MRs are sent from forks instead. > > > > > > Let's add a note about this, in case its not obvious to everyone. > > > > Yes please, we already have way too many dead branches in the main > > repo > > without adding that kind of things to it. > > > > One other thing is that (last time I checked) the scripts propagating > > the repo to github et al. don't propagate branch deletions, which > > means > > the clutter never gets cleaned there. > > > > > Signed-off-by: Erik Faye-Lund > > > --- > > > docs/submittingpatches.html | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/docs/submittingpatches.html > > > b/docs/submittingpatches.html > > > index 736cbd64de8..b5a70523acd 100644 > > > --- a/docs/submittingpatches.html > > > +++ b/docs/submittingpatches.html > > > @@ -258,6 +258,8 @@ your email administrator for this.) > > >rebased > > > Make sure your MR is closed if your patches get pushed > > > outside > > >of GitLab > > > +Please send MRs from a personal fork rather than from the > > > main > > > + Mesa repository > > > > I would add "as it clutters it unnecessarily" as you mentioned in the > > commit message. > > I was considering this, but decided against it as the other things in > this list didn't really specify motivation either. But I think you're > right, it's better to include it in this case. > > Fixed locally (but I added a comma first, which I *think* is correct?): It *is* correct :) r-b > > diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html > index 736cbd64de8..137e39c925d 100644 > --- a/docs/submittingpatches.html > +++ b/docs/submittingpatches.html > @@ -258,6 +258,8 @@ your email administrator for this.) >rebased > Make sure your MR is closed if your patches get pushed outside >of GitLab > +Please send MRs from a personal fork rather than from the main > + Mesa repository, as it clutters it unnecessarily. > > > > > ___ > 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] docs: add note about sending merge-requests from forks
On Wednesday, 2019-01-23 11:21:27 +0100, Erik Faye-Lund wrote: > On Wed, 2019-01-23 at 10:14 +, Daniel Stone wrote: > > Hi, > > > > On Wed, 23 Jan 2019 at 10:09, Eric Engestrom < > > eric.engest...@intel.com> wrote: > > > On Wednesday, 2019-01-23 10:36:15 +0100, Erik Faye-Lund wrote: > > > > Sending MRs from the main Mesa repository increase clutter in the > > > > repository, and decrease visibility of project-wide branches. So > > > > it's > > > > better if MRs are sent from forks instead. > > > > > > > > Let's add a note about this, in case its not obvious to everyone. > > > > > > Yes please, we already have way too many dead branches in the main > > > repo > > > without adding that kind of things to it. > > > > > > One other thing is that (last time I checked) the scripts > > > propagating > > > the repo to github et al. don't propagate branch deletions, which > > > means > > > the clutter never gets cleaned there. > > > > They're propagated from gitlab.fd.o to git.fd.o, as the hook is run > > within regular post-receive, but you're right that pushing from > > git.fd.o to GitHub doesn't notice old branches, as it just pushes > > everything present in the git.fd.o repo to GitHub, and doesn't notice > > any branches no longer present. > > Yeah, and then add the problem that forking in Github copies *all* > branches as well (not just the default branch), means that these > branches keeps getting replicated over there. It gets nasty quickly. > > Perhaps we should do a manual spring cleaning in the repo soon, moving > old, stale branches out to some historical repo and making sure > official forks don't have any cruft? I looked at that a year or two ago, but I quickly gave up. You're welcome to give it a go, but be aware that there's a very large number of very old branches. One thing we could do is restrict new-branch pushes to the main repo to only allow the \d+\.\d of release branches and their staging/* and *-proposed counterparts. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: add note about sending merge-requests from forks
On Wed, 2019-01-23 at 10:08 +, Eric Engestrom wrote: > On Wednesday, 2019-01-23 10:36:15 +0100, Erik Faye-Lund wrote: > > Sending MRs from the main Mesa repository increase clutter in the > > repository, and decrease visibility of project-wide branches. So > > it's > > better if MRs are sent from forks instead. > > > > Let's add a note about this, in case its not obvious to everyone. > > Yes please, we already have way too many dead branches in the main > repo > without adding that kind of things to it. > > One other thing is that (last time I checked) the scripts propagating > the repo to github et al. don't propagate branch deletions, which > means > the clutter never gets cleaned there. > > > Signed-off-by: Erik Faye-Lund > > --- > > docs/submittingpatches.html | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/docs/submittingpatches.html > > b/docs/submittingpatches.html > > index 736cbd64de8..b5a70523acd 100644 > > --- a/docs/submittingpatches.html > > +++ b/docs/submittingpatches.html > > @@ -258,6 +258,8 @@ your email administrator for this.) > >rebased > > Make sure your MR is closed if your patches get pushed > > outside > >of GitLab > > +Please send MRs from a personal fork rather than from the > > main > > + Mesa repository > > I would add "as it clutters it unnecessarily" as you mentioned in the > commit message. I was considering this, but decided against it as the other things in this list didn't really specify motivation either. But I think you're right, it's better to include it in this case. Fixed locally (but I added a comma first, which I *think* is correct?): diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html index 736cbd64de8..137e39c925d 100644 --- a/docs/submittingpatches.html +++ b/docs/submittingpatches.html @@ -258,6 +258,8 @@ your email administrator for this.) rebased Make sure your MR is closed if your patches get pushed outside of GitLab +Please send MRs from a personal fork rather than from the main + Mesa repository, as it clutters it unnecessarily. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: add note about sending merge-requests from forks
On Wed, 2019-01-23 at 10:14 +, Daniel Stone wrote: > Hi, > > On Wed, 23 Jan 2019 at 10:09, Eric Engestrom < > eric.engest...@intel.com> wrote: > > On Wednesday, 2019-01-23 10:36:15 +0100, Erik Faye-Lund wrote: > > > Sending MRs from the main Mesa repository increase clutter in the > > > repository, and decrease visibility of project-wide branches. So > > > it's > > > better if MRs are sent from forks instead. > > > > > > Let's add a note about this, in case its not obvious to everyone. > > > > Yes please, we already have way too many dead branches in the main > > repo > > without adding that kind of things to it. > > > > One other thing is that (last time I checked) the scripts > > propagating > > the repo to github et al. don't propagate branch deletions, which > > means > > the clutter never gets cleaned there. > > They're propagated from gitlab.fd.o to git.fd.o, as the hook is run > within regular post-receive, but you're right that pushing from > git.fd.o to GitHub doesn't notice old branches, as it just pushes > everything present in the git.fd.o repo to GitHub, and doesn't notice > any branches no longer present. Yeah, and then add the problem that forking in Github copies *all* branches as well (not just the default branch), means that these branches keeps getting replicated over there. It gets nasty quickly. Perhaps we should do a manual spring cleaning in the repo soon, moving old, stale branches out to some historical repo and making sure official forks don't have any cruft? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: add note about sending merge-requests from forks
Hi, On Wed, 23 Jan 2019 at 10:09, Eric Engestrom wrote: > On Wednesday, 2019-01-23 10:36:15 +0100, Erik Faye-Lund wrote: > > Sending MRs from the main Mesa repository increase clutter in the > > repository, and decrease visibility of project-wide branches. So it's > > better if MRs are sent from forks instead. > > > > Let's add a note about this, in case its not obvious to everyone. > > Yes please, we already have way too many dead branches in the main repo > without adding that kind of things to it. > > One other thing is that (last time I checked) the scripts propagating > the repo to github et al. don't propagate branch deletions, which means > the clutter never gets cleaned there. They're propagated from gitlab.fd.o to git.fd.o, as the hook is run within regular post-receive, but you're right that pushing from git.fd.o to GitHub doesn't notice old branches, as it just pushes everything present in the git.fd.o repo to GitHub, and doesn't notice any branches no longer present. Cheers, Daniel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] docs: add note about sending merge-requests from forks
On Wednesday, 2019-01-23 10:36:15 +0100, Erik Faye-Lund wrote: > Sending MRs from the main Mesa repository increase clutter in the > repository, and decrease visibility of project-wide branches. So it's > better if MRs are sent from forks instead. > > Let's add a note about this, in case its not obvious to everyone. Yes please, we already have way too many dead branches in the main repo without adding that kind of things to it. One other thing is that (last time I checked) the scripts propagating the repo to github et al. don't propagate branch deletions, which means the clutter never gets cleaned there. > > Signed-off-by: Erik Faye-Lund > --- > docs/submittingpatches.html | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html > index 736cbd64de8..b5a70523acd 100644 > --- a/docs/submittingpatches.html > +++ b/docs/submittingpatches.html > @@ -258,6 +258,8 @@ your email administrator for this.) >rebased > Make sure your MR is closed if your patches get pushed outside >of GitLab > +Please send MRs from a personal fork rather than from the main > + Mesa repository I would add "as it clutters it unnecessarily" as you mentioned in the commit message. Reviewed-by: Eric Engestrom > > > > -- > 2.20.1 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Thoughts after hitting 100 merge requests?
On Thu, 2019-01-17 at 08:38 +0100, Erik Faye-Lund wrote: > On Fri, 2019-01-11 at 10:57 -0600, Jason Ekstrand wrote: > > All, > > > > The mesa project has now hit 100 merge requests (36 are still > > open). > > I (and I'm sure others) would be curious to hear people's initial > > thoughts on the process. What's working well? What's not > > working? > > Is it total fail and should we go back to mailing lists? > > > > So, overall I think it works pretty well. I have some things I think > maybe we could do better, some of which has already been pointed out: > > 1. New MRs should probably get their cover-letter automatically sent > to > the mailing list for incrased visibility. OK, I think that after having enabled e-mail notifications and spent some time setting up mail filters, this is much less of a pressing issue for me personally. I'm not even sure I think this is worth the time any more, as the notification support in GitLab is *really* good, and allows nice and finely-grained control, much better than what an automatic mail-bot could do. This doesn't mean that I would opose it, though. Just that I think I would strike this from *my* personal list of things to improve. > 2. Perhaps we should ban sending MRs from the main mesa repo? With > gitlab, it's trivial to make your own fork, and you can delegate > permissions to other users for collaborators. I don't think there's > any > reason to clutter up the main mesa repo with all kinds of branches. > But > it seems some people send their MRs from the main-repo anyway. > Perhaps > we should document that this isn't how to send MRs? I've sent out a patch to add a note about this. > 3. There's some browsing-pain with the commit list. For instance, I > always second-guess if the latest commit is at the top or bottom. > Some > times this is not a problem due to timestamps, but sometimes this > isn't > clear from that either. I also tend to get a bit lost in context. > Some > of this is probably habit, though. > And I don't really think this is a big deal, especially after the discussion on this point below; GitHub is the oddball here, and that's not GitLab's fault. So all in all, I think all of my issues with the process has been resolved (assuming we land the patch with the MR-note in some form, and if we don't that's probably also for a good reason). So from my point of view, I would love to see us move to a MR-only workflow as soon as possible. Doing both is a little bit messy (as was anticipated). ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Android + tegra + mesa
On Fri, Jan 18, 2019 at 01:26:30PM +0300, Artyom Bambalov wrote:
> Do you have some notes on how to reproduce your setup? I've been meaning
> to try this out but never found enough time to see it through. Having a
> set of simple instructions for getting this built and booted would help
> reproduce the behavior and debug.
>
> My device is xiaomi mipad 1(codename is mocha). It's similar with with
> shieldtablet(tn8), but uses different lcd panel, pmic(tps65913 like in t114
> instead of as3xxx) and some other secondary hardware.
> My device tree(lineage-16.0 branch): https://github.com/art/
> android_device_xiaomi_mocha_mainline
> My kernel(android-4.19_mocha-changes branch): https://github.com/Insei/linux
>
>
>
> That said, it looks like you've got a completely white display, which
> usually means that you're getting page faults from the SMMU, so maybe
> that'd be an interesting place to look at. You should be seeing errors
> from the SMMU in the kernel log in that case.
>
> The display can be white or black. It depends on real picture color.
>
>
>
> There was a brief period where the SMMU wasn't working properly with
> Nouveau, which could be related to this, especially since it was around
> the timeframe of 4.19. That issue was fixed with this commit:
>
> b59fb482b522 ("drm/nouveau: tegra: Detach from ARM DMA/IOMMU
> mapping")
>
> which was merged into v4.19, but maybe it's not in the tree from Google
> that you're using (for whatever reason).
>
> Come to think of it, that wouldn't really explain the white display,
> though, since that usually happens on SMMU faults for reads by the
> display controller. In any case, the kernel log would hopefully contain
> some clues as to what could be wrong.
>
> Thierry
>
>
> I attached dmesg. It contains pretty much nouveau errors.
[...]
> [ 202.341300] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 0d78 data 0004
> [ 202.341332] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 0d78 data 0004
> [ 202.341359] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 0d78 data 0004
> [ 202.341383] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 17e0 data 001e
> [ 202.341418] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 0d78 data 0004
> [ 202.341453] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 0d78 data 0004
> [ 202.341476] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 0d78 data 0004
> [ 202.341499] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 17e0 data 001e
> [ 202.415128] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 0d78 data 0004
> [ 202.415162] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 0d78 data 0004
> [ 202.415188] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 0d78 data 0004
> [ 202.415213] nouveau 5700.gpu: gr: DATA_ERROR 009c [] ch 6
> [0400441000 RenderThread[924]] subc 0 class a297 mthd 17e0 data 001e
These errors certainly look like something's wrong with the IOMMU. Can
you post the contents of this:
$ ls -l /sys/class/iommu/70019000.memory-controller/devices
from your system?
Thierry
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] docs: add note about sending merge-requests from forks
Sending MRs from the main Mesa repository increase clutter in the repository, and decrease visibility of project-wide branches. So it's better if MRs are sent from forks instead. Let's add a note about this, in case its not obvious to everyone. Signed-off-by: Erik Faye-Lund --- docs/submittingpatches.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html index 736cbd64de8..b5a70523acd 100644 --- a/docs/submittingpatches.html +++ b/docs/submittingpatches.html @@ -258,6 +258,8 @@ your email administrator for this.) rebased Make sure your MR is closed if your patches get pushed outside of GitLab +Please send MRs from a personal fork rather than from the main + Mesa repository -- 2.20.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/dri2: try to bind old context if bindContext failed
Thanks for reviewing this Frank. I'd wait some more for any further comments
before uploading a v2.
Frank Binns writes:
> Hi Luigi,
>
> Luigi Santivetti writes:
>
>> Before this change, if bindContext() failed then dri2_make_current() would
>> rebind the old EGL context and surfaces and return EGL_BAD_MATCH. However,
>> it wouldn't rebind the DRI context and surfaces, thus leaving it in an
>> inconsistent and unrecoverable state.
>>
>> After this change, dri2_make_current() tries to bind the old DRI context
>> and surfaces when bindContext() failed. If unable to do so, it leaves EGL
>> and the DRI driver in a consistent state, it reports an error and returns
>> EGL_BAD_MATCH.
>>
>> Fixes: 4e8f95f64d004aa1 ("egl_dri2: Always unbind old contexts")
>>
>> Signed-off-by: Luigi Santivetti
>> ---
>> src/egl/drivers/dri2/egl_dri2.c | 54 ++---
>> 1 file changed, 43 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.c
>> b/src/egl/drivers/dri2/egl_dri2.c
>> index dca22762047..016a3ced96d 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.c
>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>> @@ -1648,8 +1648,9 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp,
>> _EGLSurface *dsurf,
>> _EGLSurface *old_dsurf, *old_rsurf;
>> _EGLSurface *tmp_dsurf, *tmp_rsurf;
>> __DRIdrawable *ddraw, *rdraw;
>> - __DRIcontext *cctx;
>> + __DRIcontext *cctx, *old_cctx;
>> EGLBoolean unbind;
>> + EGLint egl_error;
>>
>> if (!dri2_dpy)
>>return _eglError(EGL_NOT_INITIALIZED, "eglMakeCurrent");
>> @@ -1674,7 +1675,7 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp,
>> _EGLSurface *dsurf,
>> cctx = (dri2_ctx) ? dri2_ctx->dri_context : NULL;
>>
>> if (old_ctx) {
>> - __DRIcontext *old_cctx = dri2_egl_context(old_ctx)->dri_context;
>> + old_cctx = dri2_egl_context(old_ctx)->dri_context;
>>
>>if (old_dsurf)
>> dri2_surf_update_fence_fd(old_ctx, disp, old_dsurf);
>> @@ -1691,17 +1692,24 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay
>> *disp, _EGLSurface *dsurf,
>> unbind = (cctx == NULL && ddraw == NULL && rdraw == NULL);
>>
>> if (!unbind && !dri2_dpy->core->bindContext(cctx, ddraw, rdraw)) {
>> + __DRIdrawable *old_ddraw, *old_rdraw;
>> +
>> + /* dri2_dpy->core->bindContext failed. We cannot tell for sure why,
>> but
>> + * setting the error to EGL_BAD_MATCH is surely better than leaving it
>> + * as EGL_SUCCESS.
>> + */
>> + egl_error = EGL_BAD_MATCH;
>> +
>> + old_ddraw = (old_dsurf) ? dri2_dpy->vtbl->get_dri_drawable(old_dsurf)
>> : NULL;
>> + old_rdraw = (old_rsurf) ? dri2_dpy->vtbl->get_dri_drawable(old_rsurf)
>> : NULL;
>> + old_cctx = (old_ctx) ? dri2_egl_context(old_ctx)->dri_context : NULL;
>> +
>>/* undo the previous _eglBindContext */
>>_eglBindContext(old_ctx, old_dsurf, old_rsurf, , _dsurf,
>> _rsurf);
>>assert(_ctx->base == ctx &&
>> tmp_dsurf == dsurf &&
>> tmp_rsurf == rsurf);
>>
>> - if (old_dsurf && _eglSurfaceInSharedBufferMode(old_dsurf) &&
>> - old_dri2_dpy->vtbl->set_shared_buffer_mode) {
>> - old_dri2_dpy->vtbl->set_shared_buffer_mode(old_disp, old_dsurf,
>> true);
>> - }
>> -
>>_eglPutSurface(dsurf);
>>_eglPutSurface(rsurf);
>>_eglPutContext(ctx);
>> @@ -1710,11 +1718,32 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay
>> *disp, _EGLSurface *dsurf,
>>_eglPutSurface(old_rsurf);
>>_eglPutContext(old_ctx);
>>
>> - /* dri2_dpy->core->bindContext failed. We cannot tell for sure why,
>> but
>> - * setting the error to EGL_BAD_MATCH is surely better than leaving it
>> - * as EGL_SUCCESS.
>> + /* undo the previous dri2_dpy->core->unbindContext */
>> + if (dri2_dpy->core->bindContext(old_cctx, old_ddraw, old_rdraw)) {
>> + if (old_dsurf && _eglSurfaceInSharedBufferMode(old_dsurf) &&
>> + old_dri2_dpy->vtbl->set_shared_buffer_mode) {
>> +old_dri2_dpy->vtbl->set_shared_buffer_mode(old_disp, old_dsurf,
>> true);
>> + }
>> +
>> + return _eglError(egl_error, "eglMakeCurrent");
>> + }
>> +
>> + /* We cannot restore the same state as it was before calling
>> + * eglMakeCurrent(), but we can keep EGL in a consistent state with
>> + * the DRI driver by unbinding the old EGL context and surfaces.
>> */
>> - return _eglError(EGL_BAD_MATCH, "eglMakeCurrent");
>> + ctx = dsurf = rsurf = NULL;
>
> This line causes:
> warning: assignment from incompatible pointer type
> [-Wincompatible-pointer-types]
>
> With that fixed this gets my:
> Reviewed-by: Frank Binns
>
I'll have to split ctx on a different line.
>> + unbind = true;
>> +
>> + _eglBindContext(ctx, dsurf, rsurf, _ctx, _dsurf, _rsurf);
>> + assert(_ctx->base == old_ctx &&
>> + tmp_dsurf == old_dsurf &&
>>
[Mesa-dev] [PATCH] radv: fix computing number of user SGPRs for streamout buffers
Streamout buffers are emitted like push constants. Signed-off-by: Samuel Pitoiset --- src/amd/vulkan/radv_nir_to_llvm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c index 40812fa7ffb..7f1aa17b0d5 100644 --- a/src/amd/vulkan/radv_nir_to_llvm.c +++ b/src/amd/vulkan/radv_nir_to_llvm.c @@ -701,6 +701,9 @@ static void allocate_user_sgprs(struct radv_shader_context *ctx, if (ctx->shader_info->info.loads_push_constants) user_sgpr_count++; + if (ctx->streamout_buffers) + user_sgpr_count++; + uint32_t available_sgprs = ctx->options->chip_class >= GFX9 && stage != MESA_SHADER_COMPUTE ? 32 : 16; uint32_t remaining_sgprs = available_sgprs - user_sgpr_count; uint32_t num_desc_set = -- 2.20.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: fix PRIMITIVES_GENERATED query after the "pipeline stat single" changes
On Fri, 2019-01-18 at 11:27 -0500, Marek Olšák wrote:
> From: Marek Olšák
>
> ---
> src/mesa/state_tracker/st_cb_queryobj.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_queryobj.c
> b/src/mesa/state_tracker/st_cb_queryobj.c
> index abb126547c9..642b901d05a 100644
> --- a/src/mesa/state_tracker/st_cb_queryobj.c
> +++ b/src/mesa/state_tracker/st_cb_queryobj.c
> @@ -84,21 +84,22 @@ st_DeleteQuery(struct gl_context *ctx, struct
> gl_query_object *q)
> struct st_query_object *stq = st_query_object(q);
>
> free_queries(pipe, stq);
>
> free(stq);
> }
>
> static int
> target_to_index(const struct st_context *st, const struct
> gl_query_object *q)
> {
> - if (q->Target == GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN ||
> + if (q->Target == GL_PRIMITIVES_GENERATED ||
> + q->Target == GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN ||
> q->Target == GL_TRANSFORM_FEEDBACK_STREAM_OVERFLOW_ARB)
>return q->Stream;
>
> if (st->has_single_pipe_stat) {
>switch (q->Target) {
>case GL_VERTICES_SUBMITTED_ARB:
> return PIPE_STAT_QUERY_IA_VERTICES;
>case GL_PRIMITIVES_SUBMITTED_ARB:
> return PIPE_STAT_QUERY_IA_PRIMITIVES;
>case GL_VERTEX_SHADER_INVOCATIONS_ARB:
The change itself looks good to me.
However, I think a commit message saying, well, *something* would be a
good idea (and probably would have made it easier to find reviewers in
the first place). Something like this, perhaps?
"When this functionality was added, the PRIMITIVES_GENERATED query was
accidentally omitted. This causes issues for drivers that support
transform feedback."
I also think this should have a Fixes tag:
Fixes: d644698b443 ("gallium: Add the ability to query a single
pipeline statistics counter")
With those things changed:
Reviewed-by: Erik Faye-Lund
Also, I added Kenneth Grauke who wrote the commit in question to the CC
list. Perhaps he has some thoughts.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 17/42] intel/compiler: add new half-float register type for 3-src instructions
On Tue, 2019-01-22 at 15:46 -0800, Matt Turner wrote:
> On Tue, Jan 15, 2019 at 5:55 AM Iago Toral Quiroga > wrote:
> >
> > This is available since gen8.
> >
> > v2: restore previously existing assertion.
> >
> > Reviewed-by: Topi Pohjolainen (v1)
> > ---
> > src/intel/compiler/brw_reg_type.c | 36
> > +++
> > 1 file changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/intel/compiler/brw_reg_type.c
> > b/src/intel/compiler/brw_reg_type.c
> > index 60240ba1513..09b3ea61d4c 100644
> > --- a/src/intel/compiler/brw_reg_type.c
> > +++ b/src/intel/compiler/brw_reg_type.c
> > @@ -138,6 +138,7 @@ enum hw_3src_reg_type {
> > GEN7_3SRC_TYPE_D = 1,
> > GEN7_3SRC_TYPE_UD = 2,
> > GEN7_3SRC_TYPE_DF = 3,
> > + GEN8_3SRC_TYPE_HF = 4,
> >
> > /** When ExecutionDatatype is 1: @{ */
> > GEN10_ALIGN1_3SRC_REG_TYPE_HF = 0b000,
> > @@ -166,6 +167,14 @@ static const struct hw_3src_type {
> > [BRW_REGISTER_TYPE_D] = { GEN7_3SRC_TYPE_D },
> > [BRW_REGISTER_TYPE_UD] = { GEN7_3SRC_TYPE_UD },
> > [BRW_REGISTER_TYPE_DF] = { GEN7_3SRC_TYPE_DF },
> > +}, gen8_hw_3src_type[] = {
> > + [0 ... BRW_REGISTER_TYPE_LAST] = { INVALID },
> > +
> > + [BRW_REGISTER_TYPE_F] = { GEN7_3SRC_TYPE_F },
> > + [BRW_REGISTER_TYPE_D] = { GEN7_3SRC_TYPE_D },
> > + [BRW_REGISTER_TYPE_UD] = { GEN7_3SRC_TYPE_UD },
> > + [BRW_REGISTER_TYPE_DF] = { GEN7_3SRC_TYPE_DF },
> > + [BRW_REGISTER_TYPE_HF] = { GEN8_3SRC_TYPE_HF },
> > }, gen10_hw_3src_align1_type[] = {
> > #define E(x) BRW_ALIGN1_3SRC_EXEC_TYPE_##x
> > [0 ... BRW_REGISTER_TYPE_LAST] = { INVALID },
> > @@ -249,6 +258,20 @@ brw_hw_type_to_reg_type(const struct
> > gen_device_info *devinfo,
> > unreachable("not reached");
> > }
> >
> > +static inline const struct hw_3src_type *
> > +get_hw_3src_type_map(const struct gen_device_info *devinfo,
> > uint32_t *size)
> > +{
> > + if (devinfo->gen < 8) {
> > + if (size)
> > + *size = ARRAY_SIZE(gen7_hw_3src_type);
> > + return gen7_hw_3src_type;
> > + } else {
> > + if (size)
> > + *size = ARRAY_SIZE(gen8_hw_3src_type);
> > + return gen8_hw_3src_type;
> > + }
> > +}
>
> I would rather inline this code and remove the function, like we
> already do for example:
>
>const struct hw_type *table;
>
>if (devinfo->gen >= 11) {
> assert(type < ARRAY_SIZE(gen11_hw_type));
> table = gen11_hw_type;
>} else {
> assert(type < ARRAY_SIZE(gen4_hw_type));
> table = gen4_hw_type;
>}
>
> But I'm not even sure that separate gen7 vs gen8 tables are required,
> since gen8 just adds one additional value. I thought we had some code
> that essentially did assert(devinfo->gen >= 8 || type !=
> BRW_REGISTER_TYPE_HF), but I don't see it now.
I am liking the idea though, as you say, there is only one value that
changes after all... should I make that change? (and rename the table
to be prefixed by gen8_ instead of gen_7.
> We have checks that Q/UQ/DF are only used when 64-bit hw support is
> available, so maybe that's what I'm thinking of.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 24/42] intel/compiler: fix ddy for half-float in gen8
On Tue, 2019-01-22 at 16:36 -0800, Matt Turner wrote:
> On Tue, Jan 15, 2019 at 5:54 AM Iago Toral Quiroga > wrote:
> >
> > We use ALign16 mode for this, since it is more convenient, but the
> > PRM
> > for Broadwell states in Volume 3D Media GPGPU, Chapter 'Register
> > region
> > restrictions', Section '1. Special Restrictions':
> >
> >"In Align16 mode, the channel selects and channel enables apply
> > to a
> > pair of half-floats, because these parameters are defined for
> > DWord
> > elements ONLY. This is applicable when both source and
> > destination
> > are half-floats."
> >
> > This means that we cannot select individual HF elements using
> > swizzles
> > like we do with 32-bit floats so we can't implement the required
> > regioning for this.
> >
> > Use the gen11 path for this instead, which uses Align1 mode.
> >
> > The restriction is not present in gen9 or gen10, where the Align16
> > implementation seems to work just fine.
> >
> > Reviewed-by: Jason Ekstrand
> > ---
> > src/intel/compiler/brw_fs_generator.cpp | 10 --
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/intel/compiler/brw_fs_generator.cpp
> > b/src/intel/compiler/brw_fs_generator.cpp
> > index d0cc4a6d231..4310f0b7fdc 100644
> > --- a/src/intel/compiler/brw_fs_generator.cpp
> > +++ b/src/intel/compiler/brw_fs_generator.cpp
> > @@ -1339,8 +1339,14 @@ fs_generator::generate_ddy(const fs_inst
> > *inst,
> > const uint32_t type_size = type_sz(src.type);
> >
> > if (inst->opcode == FS_OPCODE_DDY_FINE) {
> > - /* produce accurate derivatives */
> > - if (devinfo->gen >= 11) {
> > + /* produce accurate derivatives. We can do this easily in
> > Align16
> > + * but this is not supported in gen11+ and gen8 Align16
> > swizzles
> > + * for Half-Float operands work in units of 32-bit and
> > always
> > + * select pairs of consecutive half-float elements, so we
> > can't use
> > + * use it for this.
> > + */
>
> Let's break this comment up and include (or move) the BSpec text from
> the commit message here. I wouldn't mention the "this is not
> supported
> in gen11+" because it's slightly unclear whether you're talking about
> "accurate derivatives" or "Align16". How about:
>
>
> /* produce accurate derivatives.
>*
>* From the Broadwell PRM, Volume 7 (3D-Media-GPGPU)
>* "Register Region Restrictions", Section "1. Special
> Restrictions":
>*
>*"In Align16 mode, the channel selects and channel enables
> apply to
>* a pair of half-floats, because these parameters are
> defined for
>* DWord elements ONLY. This is applicable when both source
> and
>* destination are half-floats."
>*
>* So for half-float operations we use the Gen11+ Align1 path.
> CHV
>* inherits its FP16 hardware from SKL, so it is not affected.
>*/
>
>
> > + if (devinfo->gen >= 11 ||
> > + (devinfo->gen == 8 && src.type == BRW_REGISTER_TYPE_HF))
> > {
>
>
> The docs are bad about telling us whether a BDW restriction applies
> to
> CHV as well, but in this case I suspect the answer is no. CHV seems
> to
> inherit its FP16 hw from SKL, which doesn't have the restriction as
> you say.
>
> So I suspect you want devinfo->is_broadwell instead of devinfo->gen
> == 8.
I have just confirmed that your suspicion is correct, thanks for
bringing this up!
> With that (and confirmation that CHV isn't affected), this patch is
>
> Reviewed-by: Matt Turner
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radv: try to select better export formats for chips without Rb+
On 1/22/19 10:50 PM, Bas Nieuwenhuizen wrote:
On Tue, Jan 22, 2019 at 4:32 PM Samuel Pitoiset
wrote:
For some R8 formats, it's useless to export two channels
when no alpha blending is used. I assume the CB should
automatically clamps its inputs.
29077 shaders in 15096 tests
Totals:
SGPRS: 1321106 -> 1320970 (-0.01 %)
VGPRS: 935936 -> 935948 (0.00 %)
Spilled SGPRs: 25186 -> 25204 (0.07 %)
Code Size: 49813556 -> 49796944 (-0.03 %) bytes
Max Waves: 242091 -> 242088 (-0.00 %)
Totals from affected shaders:
SGPRS: 170608 -> 170472 (-0.08 %)
VGPRS: 99752 -> 99764 (0.01 %)
Spilled SGPRs: 10377 -> 10395 (0.17 %)
Code Size: 6332492 -> 6315880 (-0.26 %) bytes
Max Waves: 12317 -> 12314 (-0.02 %)
This helps some Rise Of Tomb Raider shaders, especially
when an expcnt instruction is added between two MRT exports.
Signed-off-by: Samuel Pitoiset
---
src/amd/vulkan/radv_pipeline.c | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/src/amd/vulkan/radv_pipeline.c b/src/amd/vulkan/radv_pipeline.c
index 138e153f9a4..25281c3c6da 100644
--- a/src/amd/vulkan/radv_pipeline.c
+++ b/src/amd/vulkan/radv_pipeline.c
@@ -372,9 +372,10 @@ static bool is_dual_src(VkBlendFactor factor)
}
}
-static unsigned si_choose_spi_color_format(VkFormat vk_format,
- bool blend_enable,
- bool blend_need_alpha)
+static unsigned si_choose_spi_color_format(struct radv_device *device,
+ VkFormat vk_format,
+ bool blend_enable,
+ bool blend_need_alpha)
{
const struct vk_format_description *desc =
vk_format_description(vk_format);
unsigned format, ntype, swap;
@@ -486,6 +487,20 @@ static unsigned si_choose_spi_color_format(VkFormat
vk_format,
unreachable("unhandled blend format");
}
+ if (device && /* NULL for internal meta formats */
+ !device->physical_device->rbplus_allowed) {
+ /* Try to select better export formats for chips without Rb+. */
+ if (desc->nr_channels == 1 &&
+ desc->channel[0].size == 8 &&
Why do we only allow this for 8 bit components? Why not also for
16/32-bit components?
For 32-bit, that should be already handled in the switch above. For
16-bit, that should work but this hits a crash in LLVM.
I should probably add a TODO.
+ swap == V_028C70_SWAP_STD && /* R */
+ !vk_format_is_srgb(vk_format)) {
+ /* Do not need to export two channels for some R8
+* formats when alpha blending isn't used.
+*/
+ blend = normal = V_028714_SPI_SHADER_32_R;
+ }
+ }
+
if (blend_enable && blend_need_alpha)
return blend_alpha;
else if(blend_need_alpha)
@@ -516,7 +531,8 @@ radv_pipeline_compute_spi_color_formats(struct
radv_pipeline *pipeline,
bool blend_enable =
blend->blend_enable_4bit & (0xfu << (i * 4));
- cf = si_choose_spi_color_format(attachment->format,
+ cf = si_choose_spi_color_format(pipeline->device,
+ attachment->format,
blend_enable,
blend->need_src_alpha & (1
<< i));
}
@@ -586,7 +602,8 @@ const VkFormat
radv_fs_key_format_exemplars[NUM_META_FS_KEYS] = {
unsigned radv_format_meta_fs_key(VkFormat format)
{
- unsigned col_format = si_choose_spi_color_format(format, false, false);
+ unsigned col_format =
+ si_choose_spi_color_format(NULL, format, false, false);
I think we properly need to plumb through the device here?
Yeah, but that breaks a bunch of tests. This function looks a bit hacky
to me.
Requires investigations.
assert(col_format != V_028714_SPI_SHADER_32_AR);
if (col_format >= V_028714_SPI_SHADER_32_AR)
--
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 v3 22/42] intel/compiler: don't propagate HF immediates to 3-src instructions
On Tue, 2019-01-22 at 16:18 -0800, Matt Turner wrote: > On Tue, Jan 15, 2019 at 5:54 AM Iago Toral Quiroga > wrote: > > > > 3-src instructions don't support immediates, but since > > 36bc5f06dd22, > > we allow them on MAD and LRP relying on the combine constants pass > > to > > fix it up later. However, that pass is specialized for 32-bit float > > immediates and can't handle HF constants at present, so this patch > > ensures that copy-propagation only does this for 32-bit constants. > > There's a patch later in the series that adds HF support to constant > combining (and presumably removes this code). Maybe it's the best > thing to add and remove the code in the same series, but it's good to > at least mention that in the commit message so that reviewers > understand what the plan is. Yes, makes sense. > I see from a later thread that you're going to try to handle more > than > just F/HF types in constant combining, and I guess you'll resend this > series. If that's the case, I'd just leave this patch out if it's > possible to reorder things. Yes, I'll do that. > Oh, another thing: Gen10+ can take *1* immediate HF argument in 3-src > instructions. We haven't added that support yet, though all the > low-level brw_inst_* functions exist. Not asking you to do that > without hardware, but just thought you'd be interested to know :) Sure, that's good to know, thanks for the info! :) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
