Re: [Mesa-dev] Intel: ABI for DRM format modifiers with multi-planar images

[Chad Versace]

That's a reasonable plan for now. For LINEAR, X, and Y, the 
drmFormatModifierCount is the obvious value for the format. That's enough to 
satisfy all the needs of Chrome OS and its zoo of virtual machines. For 
simplicity, we can keep VK_FORMAT_FEATURE_DISJOINT_BIT disabled in 
drmFormatModifierTilingProperties for multi-planar formats. If we ever need to 
squeeze more performance out of shared media images, then we can start 
experiments on compressed modifiers and possibly work on defining the ABI 
(though, it's always better to have it defined before it's needed, due to Mesa 
and kernel release cycles).

On Wed, Jun 9, 2021, at 16:19, Jason Ekstrand wrote:
> I should have said that the minimal support can be for LINEAR, X-tiled
> and Y-tiled.  CCS can and probably should come later.
> 
> On Wed, Jun 9, 2021 at 6:14 PM Yiwei Zhang  wrote:
> >
> > On Wed, Jun 9, 2021 at 2:19 PM Jason Ekstrand  wrote:
> > >
> > > +Nanley
> > >
> > > We've not defined those yet.  We had some internal talks a couple
> > > years ago.  The rough idea we had at the time was to define a modifier
> > > for those cases which put the CCS after each main surface at some
> > > fixed calculation offset based on width, height, and stride.  Then the
> > > one modifier would apply independently to the three different planes.
> > >
> > > --Jason
> > >
> > > On Wed, Jun 9, 2021 at 2:11 PM Chad Versace  wrote:
> > > >
> > > > The Problem: For a given 3-tuple (multi-planar format, DRM format 
> > > > modifier, chipset), we need Intel ABI that decides (a) the value of 
> > > > VkDrmFormatModifierPropertiesEXT::drmFormatModifierPlaneCount and (b) 
> > > > the content of each "modifier" plane.
> > > >
> > > > For example, suppose drmFormatModifierPlaneCount is 2 for 
> > > > (VK_FORMAT_G8_B8R8_2PLANE_420_UNORM, INTEL_FORMAT_MOD_FOO). Is the 
> > > > layout (plane1=g8, plane2=b8r8); or is it (plane1=g8_b8r8, plane2=aux)? 
> > > > The second choice isn't crazy; iirc, some non-Intel hardware (sorry, 
> > > > NDA) uses only 2 modifier planes for color-compressed 3-planar formats, 
> > > > with (plane1=r8_g8_b8, plane2=aux).
> > > >
> > > > I'm asking because Yiwei (cc'd) has begun implementing limited support 
> > > > for multi-planar formats in Anvil in order to pass the Android CTS. To 
> > > > support more media formats (for imminent Chrome OS projects and future 
> > > > vanilla Linux stuff too), we need to decide on some ABI.
> >
> > Hi,
> >
> > I have sent a 
> > MR(https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/11281)
> > to add the minimal multi-planar format support with drm format
> > modifier, so that Venus Android AHB extension layered on top of ANV
> > VK_EXT_image_drm_format_modifier implementation can pass all the
> > interop graphics cts for camera and media.
> >
> > I'd be interested to follow up about the stable ABI for this to expand
> > multi-planar support there.
> >
> > Best,
> > Yiwei
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] Intel: ABI for DRM format modifiers with multi-planar images

[Chad Versace]

The Problem: For a given 3-tuple (multi-planar format, DRM format modifier, 
chipset), we need Intel ABI that decides (a) the value of 
VkDrmFormatModifierPropertiesEXT::drmFormatModifierPlaneCount and (b) the 
content of each "modifier" plane.

For example, suppose drmFormatModifierPlaneCount is 2 for 
(VK_FORMAT_G8_B8R8_2PLANE_420_UNORM, INTEL_FORMAT_MOD_FOO). Is the layout 
(plane1=g8, plane2=b8r8); or is it (plane1=g8_b8r8, plane2=aux)? The second 
choice isn't crazy; iirc, some non-Intel hardware (sorry, NDA) uses only 2 
modifier planes for color-compressed 3-planar formats, with (plane1=r8_g8_b8, 
plane2=aux).

I'm asking because Yiwei (cc'd) has begun implementing limited support for 
multi-planar formats in Anvil in order to pass the Android CTS. To support more 
media formats (for imminent Chrome OS projects and future vanilla Linux stuff 
too), we need to decide on some ABI.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] egl/dri: fix error value with unknown drm format

[Chad Versace]

On Wed 14 Nov 2018, Eric Engestrom wrote:
> On Tuesday, 2018-11-13 14:10:45 +, Lionel Landwerlin wrote:
> > According to the EGL_EXT_image_dma_buf_import spec, creating an EGL
> > image with a DRM format not supported should yield the BAD_MATCH
> > error :
> > 
> > "
> >* If  is EGL_LINUX_DMA_BUF_EXT, and the 
> > EGL_LINUX_DRM_FOURCC_EXT
> >  attribute is set to a format not supported by the EGL, 
> > EGL_BAD_MATCH
> >  is generated.
> > "
> > 
> > Signed-off-by: Lionel Landwerlin 
> > Fixes: 20de7f9f226401 ("egl/dri2: support for creating images out of dma 
> > buffers")
> 
> That matches the spec, so:
> Reviewed-by: Eric Engestrom 
> 
> That said, Topi specifically wrote this, in his commit (20de7f9f226401):
> > v4 (Chad):
> >- in case of invalid format report EGL_BAD_ATTRIBUTE instead
> >  of EGL_BAD_MATCH
> 
> So cc'ing them both so they can mention if they had a need for this?

Huh... I don't recall why I suggested EGL_BAD_ATTRIBUTE. But this patch
matches the spec.

Reviewed-by: Chad Versace 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] freedreno/drm: Move drm_msm.h to include/drm-uapi

[Chad Versace]

So the future Vulkan driver can share it.

I tested Meson, but not Autotools nor Android.mk.

Signed-off-by: Chad Versace 
---
 Makefile.am| 1 +
 .../drivers/freedreno/drm => include/drm-uapi}/msm_drm.h   | 0
 src/gallium/drivers/freedreno/Android.mk   | 1 +
 src/gallium/drivers/freedreno/Makefile.am  | 1 +
 src/gallium/drivers/freedreno/Makefile.sources | 1 -
 src/gallium/drivers/freedreno/meson.build  | 3 +--
 6 files changed, 4 insertions(+), 3 deletions(-)
 rename {src/gallium/drivers/freedreno/drm => include/drm-uapi}/msm_drm.h (100%)

diff --git a/Makefile.am b/Makefile.am
index 9e27db046e5..99234a4af58 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -76,6 +76,7 @@ noinst_HEADERS = \
include/drm-uapi/drm_fourcc.h \
include/drm-uapi/drm_mode.h \
include/drm-uapi/i915_drm.h \
+   include/drm-uapi/msm_drm.h \
include/drm-uapi/tegra_drm.h \
include/drm-uapi/v3d_drm.h \
include/drm-uapi/vc4_drm.h \
diff --git a/src/gallium/drivers/freedreno/drm/msm_drm.h 
b/include/drm-uapi/msm_drm.h
similarity index 100%
rename from src/gallium/drivers/freedreno/drm/msm_drm.h
rename to include/drm-uapi/msm_drm.h
diff --git a/src/gallium/drivers/freedreno/Android.mk 
b/src/gallium/drivers/freedreno/Android.mk
index 9c9d0707ba9..9cf036d0472 100644
--- a/src/gallium/drivers/freedreno/Android.mk
+++ b/src/gallium/drivers/freedreno/Android.mk
@@ -39,6 +39,7 @@ LOCAL_SRC_FILES := \
 #  -Wno-packed-bitfield-compat
 
 LOCAL_C_INCLUDES := \
+   $(MESA_TOP)/include/drm-uapi \
$(LOCAL_PATH)/ir3
 
 LOCAL_GENERATED_SOURCES := $(MESA_GEN_NIR_H)
diff --git a/src/gallium/drivers/freedreno/Makefile.am 
b/src/gallium/drivers/freedreno/Makefile.am
index 5690b6ec884..a6b336ec720 100644
--- a/src/gallium/drivers/freedreno/Makefile.am
+++ b/src/gallium/drivers/freedreno/Makefile.am
@@ -3,6 +3,7 @@ include $(top_srcdir)/src/gallium/Automake.inc
 
 AM_CFLAGS = \
-Wno-packed-bitfield-compat \
+   -I$(top_srcdir)/include/drm-uapi \
-I$(top_srcdir)/src/gallium/drivers/freedreno/ir3 \
-I$(top_builddir)/src/compiler/nir \
-I$(top_srcdir)/src/compiler/nir \
diff --git a/src/gallium/drivers/freedreno/Makefile.sources 
b/src/gallium/drivers/freedreno/Makefile.sources
index 8b4d61c9884..a03f55d53e6 100644
--- a/src/gallium/drivers/freedreno/Makefile.sources
+++ b/src/gallium/drivers/freedreno/Makefile.sources
@@ -51,7 +51,6 @@ drm_SOURCES := \
drm/freedreno_ringbuffer.h \
drm/msm_bo.c \
drm/msm_device.c \
-   drm/msm_drm.h \
drm/msm_pipe.c \
drm/msm_priv.h \
drm/msm_ringbuffer.c \
diff --git a/src/gallium/drivers/freedreno/meson.build 
b/src/gallium/drivers/freedreno/meson.build
index 4024d2fa99f..122eb53b797 100644
--- a/src/gallium/drivers/freedreno/meson.build
+++ b/src/gallium/drivers/freedreno/meson.build
@@ -81,7 +81,6 @@ files_libfreedreno = files(
   'drm/freedreno_ringbuffer.h',
   'drm/msm_bo.c',
   'drm/msm_device.c',
-  'drm/msm_drm.h',
   'drm/msm_pipe.c',
   'drm/msm_priv.h',
   'drm/msm_ringbuffer.c',
@@ -254,7 +253,7 @@ files_libfreedreno = files(
 )
 
 freedreno_includes = [
-  inc_src, inc_include, inc_gallium, inc_gallium_aux,
+  inc_src, inc_include, inc_drm_uapi, inc_gallium, inc_gallium_aux,
   include_directories('ir3')
 ]
 
-- 
2.19.1.930.g4563a0d9d0-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] i965: Fix -Wswitch on INTEL_COPY_STREAMING_LOAD

[Chad Versace]

The warning is emitted when building without INLINE_SSE41.
---
 src/mesa/drivers/dri/i965/intel_tiled_memcpy.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/intel_tiled_memcpy.c 
b/src/mesa/drivers/dri/i965/intel_tiled_memcpy.c
index 836f83d4a43..f9cc020d338 100644
--- a/src/mesa/drivers/dri/i965/intel_tiled_memcpy.c
+++ b/src/mesa/drivers/dri/i965/intel_tiled_memcpy.c
@@ -599,9 +599,11 @@ choose_copy_function(mem_copy_fn_type copy_type)
   return memcpy;
case INTEL_COPY_RGBA8:
   return rgba8_copy;
-#if defined(INLINE_SSE41)
case INTEL_COPY_STREAMING_LOAD:
+#if defined(INLINE_SSE41)
   return _memcpy_streaming_load;
+#else
+  unreachable("INTEL_COPY_STREAMING_LOAD requires sse4.1");
 #endif
case INTEL_COPY_INVALID:
   unreachable("invalid copy_type");
-- 
2.19.1.930.g4563a0d9d0-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Lift restriction in external textures for EGLImage support

[Chad Versace]

On Thu 08 Nov 2018, Chad Versace wrote:
> On Wed 31 Oct 2018, Aditya Swarup wrote:
> > For Intel platforms, we support external textures only for EGLImages
> > created with EGL_EXT_image_dma_buf_import. This restriction seems to
> > be Intel specific and not present for other platforms.
> > 
> > While running SKQP test - unitTest_EGLImageTest, GL_INVALID is sent
> > to the test because of this restriction.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105301
> > Fixes Skqp's unitTest_EGLImageTest test.
> > 
> > Change-Id: I54a162db534d54858319fdb98ba502314d28fc27
> > Signed-off-by: Aditya Swarup 
> > ---
> >  src/mesa/drivers/dri/i965/intel_image.h |  3 ---
> >  src/mesa/drivers/dri/i965/intel_screen.c|  2 --
> >  src/mesa/drivers/dri/i965/intel_tex_image.c | 10 --
> >  3 files changed, 15 deletions(-)
> 
> I'm the person who originally insisted that we restrict
> GL_OES_EGL_image_external to only dma_buf-imported EGLImages. The
> pivotal comment below is mine, according git-blame, from 2015-04-09.
> 
> > -   /* We support external textures only for EGLImages created with
> > -* EGL_EXT_image_dma_buf_import. We may lift that restriction in the 
> > future.
> > -*/
> 
> It is now time to lift that restriction :-)
> Reviewed-by: Chad Versace 
> 
> The original reason was that the driver was not yet robust enough to
> handle all the corner cases of external images, auxiliary surfaces, etc.
> But, I believe the driver became sufficiently robust enough near
> September last year.

To clarify... I believe the driver is robust regarding auxiliary
surfaces and EGLImages (solution: The driver disables auxiliary surfaces
on EGLImages created via this codepath), but not necessarily fully
correct around all the non-compression corner cases of the EGLImage api.
But, as Ken said, we probably still have problems lurking around the
orphaning logic, and we can fix those corner cases as we find them.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Lift restriction in external textures for EGLImage support

[Chad Versace]

On Wed 31 Oct 2018, Aditya Swarup wrote:
> For Intel platforms, we support external textures only for EGLImages
> created with EGL_EXT_image_dma_buf_import. This restriction seems to
> be Intel specific and not present for other platforms.
> 
> While running SKQP test - unitTest_EGLImageTest, GL_INVALID is sent
> to the test because of this restriction.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105301
> Fixes Skqp's unitTest_EGLImageTest test.
> 
> Change-Id: I54a162db534d54858319fdb98ba502314d28fc27
> Signed-off-by: Aditya Swarup 
> ---
>  src/mesa/drivers/dri/i965/intel_image.h |  3 ---
>  src/mesa/drivers/dri/i965/intel_screen.c|  2 --
>  src/mesa/drivers/dri/i965/intel_tex_image.c | 10 --
>  3 files changed, 15 deletions(-)

I'm the person who originally insisted that we restrict
GL_OES_EGL_image_external to only dma_buf-imported EGLImages. The
pivotal comment below is mine, according git-blame, from 2015-04-09.

> -   /* We support external textures only for EGLImages created with
> -* EGL_EXT_image_dma_buf_import. We may lift that restriction in the 
> future.
> -    */

It is now time to lift that restriction :-)
Reviewed-by: Chad Versace 

The original reason was that the driver was not yet robust enough to
handle all the corner cases of external images, auxiliary surfaces, etc.
But, I believe the driver became sufficiently robust enough near
September last year.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vulkan/wsi/wayland: Respect non-blocking AcquireNextImage

[Chad Versace]

On Tue 30 Oct 2018, Daniel Stone wrote:
> If the client has requested that AcquireNextImage not block at all, with
> a timeout of 0, then don't make any non-blocking calls.
> 
> This will still potentially block infinitely given a non-infinte
> timeout, but the fix for that is much more involved.
> 
> Signed-off-by: Daniel Stone 
> Cc: mesa-sta...@lists.freedesktop.org
> Cc: Chad Versace 
> Cc: Jason Ekstrand 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108540

Reviewed-by: Chad Versace 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/5] anv: Drop some VK_IMAGE_TILING_OPTIMAL checks

[Chad Versace]

On Mon 01 Oct 2018, Jason Ekstrand wrote:
> The DRM format modifiers extension adds a TILING_DRM_FORMAT_MODIFIER
> which will be used for modifiers so we can no longer use OPTIMAL to
> indicate tiled inside the driver.
> ---
>  src/intel/vulkan/anv_formats.c | 2 +-
>  src/intel/vulkan/anv_image.c   | 6 +++---
>  src/intel/vulkan/genX_cmd_buffer.c | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)

This patch needs to also fixup some places that test tiling ==
VK_IMAGE_TILING_LINEAR. I found at least one such place in
anv_formats.c. The patch also needs to fixup any functions that use
early returns that are conditioned (perhaps indirectly) on tiling;
anv_get_format_plane() comes to mind.

I quickly tried, as an experiment, to expand this patch into a correct
patch. After a few minutes of typing, I concluded that this patch series
takes the wrong order-of-implementation approach.

For example, what happens to all the calls to anv_get_format_plane() in
anv_blorp.c? Those need fixing too. Simply fixing the tiling checks is
not enough, especially because VK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXT
allows DRM_FORMAT_MOD_LINEAR.

Instead, I think the right way to do this is to incrementally, following
the callchains bottom-up, teach each function to understand
VK_EXT_image_drm_format_modifier. Only when that's complete, then move
WSI to the new structs. Otherwise, there is too much room for
accidential implementation gaps; gaps that may not hurt WSI, but will
make it more difficult to discern what-works-and-what-doesn't while
implementing the full extension.

Just now, I tried writing a patch that starts at the bottom of the
callchain, anv_get_format_plane(), and teaches it about modifiers. The
patch is more invasive than expected. Maybe the patch is messy because
more preliminary cleanups are needed. I'm unsure; I need to take a break
and revisit it in the morning.

> 
> diff --git a/src/intel/vulkan/anv_formats.c b/src/intel/vulkan/anv_formats.c
> index 33faf7cc37f..d44aae1c127 100644
> --- a/src/intel/vulkan/anv_formats.c
> +++ b/src/intel/vulkan/anv_formats.c
> @@ -508,7 +508,7 @@ get_image_format_features(const struct gen_device_info 
> *devinfo,
>return 0;
>  
> struct anv_format_plane base_plane_format = plane_format;
> -   if (vk_tiling == VK_IMAGE_TILING_OPTIMAL) {
> +   if (vk_tiling != VK_IMAGE_TILING_LINEAR) {
>base_plane_format = anv_get_format_plane(devinfo, vk_format,
> VK_IMAGE_ASPECT_COLOR_BIT,
> VK_IMAGE_TILING_LINEAR);
> diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> index b0d8c560adb..70594d6c053 100644
> --- a/src/intel/vulkan/anv_image.c
> +++ b/src/intel/vulkan/anv_image.c
> @@ -334,7 +334,7 @@ make_surface(const struct anv_device *dev,
> bool needs_shadow = false;
> if (dev->info.gen <= 8 &&
> (vk_info->flags & VK_IMAGE_CREATE_BLOCK_TEXEL_VIEW_COMPATIBLE_BIT) &&
> -   vk_info->tiling == VK_IMAGE_TILING_OPTIMAL) {
> +   vk_info->tiling != VK_IMAGE_TILING_LINEAR) {
>assert(isl_format_is_compressed(plane_format.isl_format));
>tiling_flags = ISL_TILING_LINEAR_BIT;
>needs_shadow = true;
> @@ -829,7 +829,7 @@ anv_layout_to_aux_usage(const struct gen_device_info * 
> const devinfo,
>return ISL_AUX_USAGE_NONE;
>  
> /* All images that use an auxiliary surface are required to be tiled. */
> -   assert(image->tiling == VK_IMAGE_TILING_OPTIMAL);
> +   assert(image->planes[plane].surface.isl.tiling != ISL_TILING_LINEAR);
>  
> /* Stencil has no aux */
> assert(aspect != VK_IMAGE_ASPECT_STENCIL_BIT);
> @@ -953,7 +953,7 @@ anv_layout_to_fast_clear_type(const struct 
> gen_device_info * const devinfo,
>return ANV_FAST_CLEAR_NONE;
>  
> /* All images that use an auxiliary surface are required to be tiled. */
> -   assert(image->tiling == VK_IMAGE_TILING_OPTIMAL);
> +   assert(image->planes[plane].surface.isl.tiling != ISL_TILING_LINEAR);
>  
> /* Stencil has no aux */
> assert(aspect != VK_IMAGE_ASPECT_STENCIL_BIT);
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
> b/src/intel/vulkan/genX_cmd_buffer.c
> index 099c30f3d66..821506761e2 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -967,7 +967,7 @@ transition_color_buffer(struct anv_cmd_buffer *cmd_buffer,
> if (base_layer >= anv_image_aux_layers(image, aspect, base_level))
>return;
>  
> -   assert(image->tiling == VK_IMAGE_TILING_OPTIMAL);
> +   assert(image->planes[plane].surface.isl.tiling != ISL_TILING_LINEAR);
>  
> if (initial_layout == VK_IMAGE_LAYOUT_UNDEFINED ||
> initial_layout == VK_IMAGE_LAYOUT_PREINITIALIZED) {
> -- 
> 2.17.1
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] anv: Fill out protected memory properties

[Chad Versace]

On Fri 28 Sep 2018, Jason Ekstrand wrote:
> We don't support the protected memory feature but it's easy enough to
> fill out the properties struct to say that we don't support no-fault
> either.
> 
> Fixes: 9c8b40001dde9 "anv: Support querying for protected memory"
> ---
>  src/intel/vulkan/anv_device.c | 7 +++
>  1 file changed, 7 insertions(+)

Reviewed-by: Chad Versace 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] Status Update: Vulkan DRM format modifiers

[Chad Versace]

tl;dr VK_EXT_image_drm_format_modifier will be in the next weekly
release of the Vulkan spec. But only in the git repo (asciidoc + xml)
not the official html spec nor headers. It should be enabled in the html
and headers in the following week.

...

Hi everyone, VK_EXT_image_drm_format_modifier is now merged into the
Vulkan spec, in Khronos's internal master branch. The extension will be
published in the next weeky spec release (but... see below).

For early viewing, I've uploaded a public spec branch that contains the
extension, as well as prebuilt spec html, at
.

The catch: Bas yesterday identified a fault in the extension that
will unacceptly hurt performance due to the potential of external images
used on a restricted transfer queue (VK_QUEUE_TRANSFER_BIT). And I found
a performance problem too related to mutable image views.

So, in the next spec release, VK_EXT_image_drm_format_odifiers will be
fully present in the spec repo (the asciidoc and xml), but will be
omitted from Khronos's official html spec and the headers. Bas and
I need about a week to fix the problems.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] anv/blorp: Emit depth flush and stall prior to HiZ clears

[Chad Versace]

On Fri 31 Aug 2018, Jason Ekstrand wrote:
> On Fri, Aug 31, 2018 at 4:35 PM Nanley Chery <[1]nanleych...@gmail.com> wrote:
> 
> If that doesn't fix it, I think it'd be good to comment that we've
> observed this pipecontrol be necessary for 3DSTATE_WM_HZ_OP.
> 
> 
>  I'm happy to add some comments.  We just need to decide how big of a hammer 
> to
> use.  Like I said above, in GL we pull out a pretty big one.

Speaking from prior experience, I support big flush hammers for hiz.
When a gpu hang happens, it can be incredibly difficult to diagnose it
as a hiz hang. It's better to prevent most of them with a large hammer
rather than diagnose them individually and try to prevent them with
several, single-focus tiny hammers.

Considering the impact of a gpu hang on Chrome (the entire OS may use
a single GL ctx, and for Vulkan it may do the same, using a small number
of  VkDevices), we should err on the side of avoiding the gpu hang
rather than err on the side of "too many flushes are bad".
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] anv/blorp: Emit depth flush and stall prior to HiZ clears

[Chad Versace]

On Fri 31 Aug 2018, Jason Ekstrand wrote:
> We had the flush/stall after the clear but missed the one that needs to
> go before the clear.
> 
> Cc: mesa-sta...@lists.freedesktop.org
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107760
> ---
>  src/intel/vulkan/anv_blorp.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> index 3dfc8087630..532e8185c0e 100644
> --- a/src/intel/vulkan/anv_blorp.c
> +++ b/src/intel/vulkan/anv_blorp.c
> @@ -1605,6 +1605,16 @@ anv_image_hiz_clear(struct anv_cmd_buffer *cmd_buffer,
> ISL_AUX_USAGE_NONE, );
> }
>  
> +   /* From the Sky Lake PRM Volume 7, "Depth Buffer Clear":
> +*
> +*"If other rendering operations have preceded this clear, a

The hw spec is vague, in my opinion. Does this WA apply to only
preceding "rendering operations" to *this* depth buffer? To *any* depth
buffer? To *any* render target at all?

And, does the hiz clear op reset the WA condition? That is, if two
sucessive 3DPRIMITIVEs perform a hiz clear op, does the hw require the
WA before the second one? After all, no true "rendering operations"
occurred between the two.

Ahh! Why does the specification fail to specify???

Anyway, I convinced myself that genX(blorp_exec) emits the requested
bits before the problematic 3DPRIMITIVE.

Reviewed-by: Chad Versace 

> +*PIPE_CONTROL with depth cache flush enabled, Depth Stall bit enabled
> +*must be issued before the rectangle primitive used for the depth
> +*buffer clear operation."
> +*/
> +   cmd_buffer->state.pending_pipe_bits |=
> +  ANV_PIPE_DEPTH_CACHE_FLUSH_BIT | ANV_PIPE_DEPTH_STALL_BIT;
> +
> blorp_hiz_clear_depth_stencil(, , ,
>   level, base_layer, layer_count,
>   area.offset.x, area.offset.y,
> -- 
> 2.17.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 3/7] dri: Define DRI_MutableRenderBuffer extensions

[Chad Versace]

On Tue 14 Aug 2018, Tapani Pälli wrote:
> Hi Chad;
> 
> On 08/08/2018 06:53 PM, Tapani Pälli wrote:
> > 
> > 
> > On 07.08.2018 21:16, Chad Versace wrote:
> > > On Mon 06 Aug 2018, Chad Versace wrote:
> > > > On Fri 03 Aug 2018, Tapani Pälli wrote:
> > > > > One tiny nit below but for patches 3,4 and 5:
> > > > > 
> > > > > Reviewed-by: Tapani Pälli 
> > > > > 
> > > > > Special thanks for the documentation. I want to go through
> > > > > rest of changes
> > > > > within Android but I'm currently experiencing some horrible
> > > > > issues with the
> > > > > image building. I'm hoping to resolve those soon and get
> > > > > back to business.
> > > 
> > > Any luck with the Android patches?
> > 
> > I have the build working (for O) now but can test stuff only tomorrow,
> > will get back to you then.
> > 
> > > btw, I plan to start working on and/or reviewing
> > > VK_ANDROID_external_memory_android_hardware_buffer this week.
> > > I'll coordinate with you when I start.
> > 
> > OK cool, Celadon is in process of migrating to P and when that is
> > successfully done I'll get back to looking at this too.
> > 
> 
> As an update, I have P building and running fine now. I've run dEQP tests
> and noticed at least these 3 things I've missed and require
> changes/implementation:
> 
> * vkGetPhysicalDeviceImageFormatProperties2
> * vkGetPhysicalDeviceExternalBufferProperties
> * usage of AHardwareBuffer as backing store for VkBuffer objects
> 
> I'm going to work on these areas and try to get dEQP happier. My test app
> using the extension still seems to work fine, it does not use any APIs above
> but just import/export AHardwareBuffer in context of vkImage.

Great! Thanks for the update. (I will be travelling to Europe all next
week, and was out for most of this week. So, I'm behind on reviewing
everything, and may remain behind until I return from Europe).
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/4] vulkan: Add central copy of entrypoints/extensions code.

[Chad Versace]

On Wed 08 Aug 2018, Bas Nieuwenhuizen wrote:
> ---
>  src/vulkan/Makefile.am|   3 +
>  src/vulkan/util/meson.build   |   2 +
>  src/vulkan/util/vk_entrypoints_gen.py | 515 ++
>  src/vulkan/util/vk_extensions.py  |  92 +
>  src/vulkan/util/vk_extensions_gen.py  | 205 ++
>  5 files changed, 817 insertions(+)
>  create mode 100644 src/vulkan/util/vk_entrypoints_gen.py
>  create mode 100644 src/vulkan/util/vk_extensions.py
>  create mode 100644 src/vulkan/util/vk_extensions_gen.py


> +class VkVersion:
> +def __init__(self, string):
> +split = string.split('.')
> +self.major = int(split[0])
> +self.minor = int(split[1])
> +if len(split) > 2:
> +assert len(split) == 3
> +self.patch = int(split[2])
> +else:
> +self.patch = None
> +
> +# Sanity check.  The range bits are required by the definition of the
> +# VK_MAKE_VERSION macro
> +assert self.major < 1024 and self.minor < 1024
> +assert self.patch is None or self.patch < 4096
> +assert(str(self) == string)
> +
> +def __str__(self):
> +ver_list = [str(self.major), str(self.minor)]
> +if self.patch is not None:
> +ver_list.append(str(self.patch))
> +return '.'.join(ver_list)
> +
> +def c_vk_version(self):
> +patch = self.patch if self.patch is not None else 0
> +ver_list = [str(self.major), str(self.minor), str(patch)]
> +return 'VK_MAKE_VERSION(' + ', '.join(ver_list) + ')'
> +
> +def __int_ver(self):
> +# This is just an expansion of VK_VERSION
> +patch = self.patch if self.patch is not None else 0
> +return (self.major << 22) | (self.minor << 12) | patch
> +
> +def __cmp__(self, other):

Patch 3 replaces __cmp__ here with __gt__. Was that on purpose?

> +# If only one of them has a patch version, "ignore" it by making
> +# other's patch version match self.
> +if (self.patch is None) != (other.patch is None):
> +other = copy.copy(other)
> +other.patch = self.patch
> +
> +return self.__int_ver().__cmp__(other.__int_ver())
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/4] Merge vulkan API generators.

[Chad Versace]

On Wed 08 Aug 2018, Bas Nieuwenhuizen wrote:
> radv was always just mirroring a derived version of the anv
> version, sometimes hacked together and sometimes very behind.
> 
> As we grow more vulkan drivers this repetition makes even less
> sense, so lets merge them. I took the anv generators as the
> template and made radv use them.
> 
> This includes some messy stuff in the build system due to 
> difficulties with python includes. I tested with meson and
> autotools. Android.mk is updated but not tested.
> 
> Bas Nieuwenhuizen (4):
>   vulkan: Add central copy of entrypoints/extensions code.
>   vulkan/util: Add support to not generate the trampolines.
>   anv: Use central api generation scripts.
>   radv: Integrate with common generators.

git-am complains that patches 3 and 4 insert trailing whitespace.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] anv: Use central api generation scripts.

[Chad Versace]

On Wed 08 Aug 2018, Bas Nieuwenhuizen wrote:
> This became kind of messy as python imports cannot really look up
> parent/sibling directories. I saw some scripts use sys.path but
> that became even more messy due to import locations.
> 
> I also move the selections of the dispatch table out of the
> generation script because it is not easily shared, and generating
> it did not really win anything anyway.
> ---
>  src/intel/Android.vulkan.mk |   9 +
>  src/intel/Makefile.vulkan.am|  25 +-
>  src/intel/vulkan/anv_device.c   |  46 ++
>  src/intel/vulkan/anv_entrypoints_gen.py | 537 +---
>  src/intel/vulkan/anv_extensions.py  |  68 +--
>  src/intel/vulkan/anv_extensions_gen.py  | 177 +---
>  src/intel/vulkan/meson.build|  15 +-
>  src/vulkan/util/vk_extensions.py|   5 +-
>  8 files changed, 98 insertions(+), 784 deletions(-)
> 
> diff --git a/src/intel/Android.vulkan.mk b/src/intel/Android.vulkan.mk


>  $(intermediates)/vulkan/anv_entrypoints.h: $(intermediates)/vulkan/dummy.c
> + PYTHONPATH=$(MESA_TOP)/src/vulkan/util \
>   $(VK_ENTRYPOINTS_SCRIPT) \
>   --outdir $(dir $@) \
>   --xml $(MESA_TOP)/src/vulkan/registry/vk.xml

Yes, modifying PYTHONPATH is messy, but it seems to me that it's the the
least-messy way.

I'm no expert on build systems, but I think it's wrong to clobber
PYTHONPATH. Instead, you should prepend the dir to PYTHONPATH.

For example, on my machine, PYTHONPATH is already set:

$ echo $PYTHONPATH
/usr/local/buildtools/current/sitecustomize

So, this is more correct...

   $(intermediates)/vulkan/anv_entrypoints.h: $(intermediates)/vulkan/dummy.c
PYTHONPATH="$(MESA_TOP)/src/vulkan/util:$${PYTHONPATH}"
$(VK_ENTRYPOINTS_SCRIPT) \
--outdir $(dir $@) \
--xml $(MESA_TOP)/src/vulkan/registry/vk.xml

but runs the risk of accidentally inserting $PWD into PYTHONPATH, because
Python interprets each empty path in PYTHONPATH as equivalent to $PWD.

So... perhaps it is better to modify sys.path instead of PYTHONPATH. It is
definitely safer.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] anv: Use central api generation scripts.

[Chad Versace]

On Tue 07 Aug 2018, Dylan Baker wrote:
> Quoting Bas Nieuwenhuizen (2018-08-07 16:14:33)   
>   
>   
> 
> > 
> > 
> > 
> > 
> >  anv_extensions_c = custom_target(  
> > 
> > 
> > 
> > @@ -36,10 +37,11 @@ anv_extensions_c = custom_target(   
> > 
> > 
> > 
> >input : ['anv_extensions_gen.py', vk_api_xml],   
> > 
> > 
> > 
> >output : 'anv_extensions.c', 
> > 
> > 
> > 
> >command : [  
> > 
> > 
> > 
> > +   'env', 'PYTHONPATH=@0@'.format(join_paths(meson.source_root(), 
> > 'src/vulkan/util/')),   
> > 
> >  
> 
> This is really gross, you're adding a dependency on a unix console command. I 
>   
>   
> 
> know that anv is only built on Unix-like oses, but this will eventually end 
> up
>   
>   
> being used in some code that needs to run on Windows (or mac, does mac have   
>   
>   
> 
> env?).
>   
>   
> 
> 
> I know that some people will object, but IMHO a better solution than mucking  
>   
>   
> 
> with the python path (either through sys.path or through PYTHONPATH, is to
>   
>   
> 
> put all of the generators in a src/generators directory and be done with it.  
>   
>   
> 
> Sure the intel specific bits (for example) aren't in the src/intel folder, 
> that's a small price to avoid having to call env just to run a python script.

Dylan, I think we should avoid introducing complexity in the build
system for the benefit of operating systems not supported by the driver.
That feels like a serious premature optimazation, to me.  Anvil's usage
of ioctls is highly specific to Linux/Unix, will not work on MacOS, and
definitely does not work on Windows.

(By the way, MacOS has the 'env' command).

Re: [Mesa-dev] [PATCH] egl/wayland: Set width/height of EGLSurface at creation

[Chad Versace]

Ignore this in favor of jasuarez's patch.

On Tue 07 Aug 2018, Chad Versace wrote:
> Fixes dEQP-EGL.functional.color_clears.single_context.gles2.rgb565_window.
> 
> After initializing _EGLSurface::Width and ::Height to 0, Mesa did not
> update the width and height again until the DRI driver fetched its
> buffers. This produced weird behavior:
> 
>   * Immediately after surface creation, eglQuerySurface(EGL_WIDTH)
> would return 0.
> 
>   * At some non-deterministic time between surface creation and before
> the first eglSwapBuffers, likely triggered by a GL call that coerced
> the driver to call DRI*::getBuffers, eglQuerySurface(EGL_WIDTH)
> would return the actual width.
> 
> The dEQP test assertion-failed because eglQuerySurface(EGL_WIDTH)
> returned 0 even though the native window had non-zero extent.
> 
> Cc: mesa-sta...@lists.freedesktop.org
> ---
>  src/egl/drivers/dri2/platform_wayland.c | 26 +++--
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_wayland.c 
> b/src/egl/drivers/dri2/platform_wayland.c
> index dca099500a8..eda17cda903 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -224,6 +224,15 @@ get_wl_surface_proxy(struct wl_egl_window *window)
> return wl_proxy_create_wrapper(window->surface);
>  }
>  
> +static void
> +update_size(struct dri2_egl_surface *dri2_surf)
> +{
> +  dri2_surf->base.Width  = dri2_surf->wl_win->width;
> +  dri2_surf->base.Height = dri2_surf->wl_win->height;
> +  dri2_surf->dx = dri2_surf->wl_win->dx;
> +  dri2_surf->dy = dri2_surf->wl_win->dy;
> +}
> +
>  /**
>   * Called via eglCreateWindowSurface(), drv->API.CreateWindowSurface().
>   */
> @@ -306,6 +315,11 @@ dri2_wl_create_window_surface(_EGLDriver *drv, 
> _EGLDisplay *disp,
> if (dri2_dpy->flush)
>dri2_surf->wl_win->resize_callback = resize_callback;
>  
> +   /* Set width so eglQuerySurface(EGL_WIDTH) will return non-zero. Same for
> +* height.
> +*/
> +   update_size(dri2_surf);
> +
> if (dri2_dpy->image_driver)
>createNewDrawable = dri2_dpy->image_driver->createNewDrawable;
> else if (dri2_dpy->dri2)
> @@ -581,11 +595,7 @@ update_buffers(struct dri2_egl_surface *dri2_surf)
> dri2_surf->base.Height != dri2_surf->wl_win->height) {
>  
>dri2_wl_release_buffers(dri2_surf);
> -
> -  dri2_surf->base.Width  = dri2_surf->wl_win->width;
> -  dri2_surf->base.Height = dri2_surf->wl_win->height;
> -  dri2_surf->dx = dri2_surf->wl_win->dx;
> -  dri2_surf->dy = dri2_surf->wl_win->dy;
> +  update_size(dri2_surf);
> }
>  
> if (get_back_bo(dri2_surf) < 0) {
> @@ -1636,11 +1646,7 @@ swrast_update_buffers(struct dri2_egl_surface 
> *dri2_surf)
> dri2_surf->base.Height != dri2_surf->wl_win->height) {
>  
>dri2_wl_release_buffers(dri2_surf);
> -
> -  dri2_surf->base.Width  = dri2_surf->wl_win->width;
> -  dri2_surf->base.Height = dri2_surf->wl_win->height;
> -  dri2_surf->dx = dri2_surf->wl_win->dx;
> -  dri2_surf->dy = dri2_surf->wl_win->dy;
> +  update_size(dri2_surf);
>dri2_surf->current = NULL;
> }
>  
> -- 
> 2.18.0.597.ga71716f1ad-goog
> 
> ___
> 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 3/7] dri: Define DRI_MutableRenderBuffer extensions

[Chad Versace]

On Mon 06 Aug 2018, Chad Versace wrote:
> On Fri 03 Aug 2018, Tapani Pälli wrote:
> > One tiny nit below but for patches 3,4 and 5:
> > 
> > Reviewed-by: Tapani Pälli 
> > 
> > Special thanks for the documentation. I want to go through rest of changes
> > within Android but I'm currently experiencing some horrible issues with the
> > image building. I'm hoping to resolve those soon and get back to business.

Any luck with the Android patches?

btw, I plan to start working on and/or reviewing
VK_ANDROID_external_memory_android_hardware_buffer this week.
I'll coordinate with you when I start.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v4 1/2] wayland/egl: initialize window surface size to window size

[Chad Versace]

On Tue 07 Aug 2018, Juan A. Suarez Romero wrote:
> When creating a windows surface with eglCreateWindowSurface(), the
> width and height returned by eglQuerySurface(EGL_{WIDTH,HEIGHT}) is
> invalid until buffers are updated (like calling glClear()).
> 
> But according to EGL 1.5 spec, section 3.5.6 ("Surface Attributes"):
> 
>   "Querying EGL_WIDTH and EGL_HEIGHT returns respectively the width and
>height, in pixels, of the surface. For a window or pixmap surface,
>these values are initially equal to the width and height of the
>native window or pixmap with respect to which the surface was
>created"
> 
> This fixes dEQP-EGL.functional.color_clears.* CTS tests
> 
> v2:
> - Do not modify attached_{width,height} (Daniel)
> - Do not update size on resizing window (Brendan)
> 
> CC: Daniel Stone 
> CC: Brendan King 
> CC: mesa-sta...@lists.freedesktop.org
> Tested-by: Eric Engestrom 
> ---
>  src/egl/drivers/dri2/platform_wayland.c | 3 +++
>  1 file changed, 3 insertions(+)

Hah. I just sent an equivalent patch to the list. I'll drop my patch.

Just for patch 1,
Reviewed-by: Chad Versace 
Tested-by: Chad Versace 

I don't fully understand attached_width,height. So no rb on patch 2.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] egl/wayland: Set width/height of EGLSurface at creation

[Chad Versace]

Fixes dEQP-EGL.functional.color_clears.single_context.gles2.rgb565_window.

After initializing _EGLSurface::Width and ::Height to 0, Mesa did not
update the width and height again until the DRI driver fetched its
buffers. This produced weird behavior:

  * Immediately after surface creation, eglQuerySurface(EGL_WIDTH)
would return 0.

  * At some non-deterministic time between surface creation and before
the first eglSwapBuffers, likely triggered by a GL call that coerced
the driver to call DRI*::getBuffers, eglQuerySurface(EGL_WIDTH)
would return the actual width.

The dEQP test assertion-failed because eglQuerySurface(EGL_WIDTH)
returned 0 even though the native window had non-zero extent.

Cc: mesa-sta...@lists.freedesktop.org
---
 src/egl/drivers/dri2/platform_wayland.c | 26 +++--
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_wayland.c 
b/src/egl/drivers/dri2/platform_wayland.c
index dca099500a8..eda17cda903 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -224,6 +224,15 @@ get_wl_surface_proxy(struct wl_egl_window *window)
return wl_proxy_create_wrapper(window->surface);
 }
 
+static void
+update_size(struct dri2_egl_surface *dri2_surf)
+{
+  dri2_surf->base.Width  = dri2_surf->wl_win->width;
+  dri2_surf->base.Height = dri2_surf->wl_win->height;
+  dri2_surf->dx = dri2_surf->wl_win->dx;
+  dri2_surf->dy = dri2_surf->wl_win->dy;
+}
+
 /**
  * Called via eglCreateWindowSurface(), drv->API.CreateWindowSurface().
  */
@@ -306,6 +315,11 @@ dri2_wl_create_window_surface(_EGLDriver *drv, _EGLDisplay 
*disp,
if (dri2_dpy->flush)
   dri2_surf->wl_win->resize_callback = resize_callback;
 
+   /* Set width so eglQuerySurface(EGL_WIDTH) will return non-zero. Same for
+* height.
+*/
+   update_size(dri2_surf);
+
if (dri2_dpy->image_driver)
   createNewDrawable = dri2_dpy->image_driver->createNewDrawable;
else if (dri2_dpy->dri2)
@@ -581,11 +595,7 @@ update_buffers(struct dri2_egl_surface *dri2_surf)
dri2_surf->base.Height != dri2_surf->wl_win->height) {
 
   dri2_wl_release_buffers(dri2_surf);
-
-  dri2_surf->base.Width  = dri2_surf->wl_win->width;
-  dri2_surf->base.Height = dri2_surf->wl_win->height;
-  dri2_surf->dx = dri2_surf->wl_win->dx;
-  dri2_surf->dy = dri2_surf->wl_win->dy;
+  update_size(dri2_surf);
}
 
if (get_back_bo(dri2_surf) < 0) {
@@ -1636,11 +1646,7 @@ swrast_update_buffers(struct dri2_egl_surface *dri2_surf)
dri2_surf->base.Height != dri2_surf->wl_win->height) {
 
   dri2_wl_release_buffers(dri2_surf);
-
-  dri2_surf->base.Width  = dri2_surf->wl_win->width;
-  dri2_surf->base.Height = dri2_surf->wl_win->height;
-  dri2_surf->dx = dri2_surf->wl_win->dx;
-  dri2_surf->dy = dri2_surf->wl_win->dy;
+  update_size(dri2_surf);
   dri2_surf->current = NULL;
}
 
-- 
2.18.0.597.ga71716f1ad-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/7] dri: Define DRI_MutableRenderBuffer extensions

[Chad Versace]

On Fri 03 Aug 2018, Tapani Pälli wrote:
> One tiny nit below but for patches 3,4 and 5:
> 
> Reviewed-by: Tapani Pälli 
> 
> Special thanks for the documentation. I want to go through rest of changes
> within Android but I'm currently experiencing some horrible issues with the
> image building. I'm hoping to resolve those soon and get back to business.
> 
> On 07/31/2018 09:18 PM, Chad Versace wrote:
> > Define extensions DRI_MutableRenderBufferDriver and
> > DRI_MutableRenderBufferLoader. These are the two halves for
> > EGL_KHR_mutable_render_buffer.
> > 
> > Outside the DRI code there is one additional change.  Add
> > gl_config::mutableRenderBuffer to match
> > __DRI_ATTRIB_MUTABLE_RENDER_BUFFER. Neither are used yet.
> > ---
> >   include/GL/internal/dri_interface.h| 138 -
> >   src/mesa/drivers/dri/common/dri_util.c |   2 +
> >   src/mesa/drivers/dri/common/dri_util.h |   4 +
> >   src/mesa/drivers/dri/common/utils.c|   1 +
> >   src/mesa/main/mtypes.h |   3 +
> >   5 files changed, 145 insertions(+), 3 deletions(-)



> > @@ -48,6 +48,7 @@ typedef unsigned int drm_drawable_t;
> >   typedef struct drm_clip_rect drm_clip_rect_t;
> >   #endif
> > +#include 
> 
> This does not look necessary change.

Thanks. This was a leftover from an earlier version.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] egl/surfaceless: Allow DRMless fallback.

[Chad Versace]

On Tue 17 Jul 2018, Eric Engestrom wrote:
> On Thursday, 2018-07-12 16:13:55 -0700, David Riley wrote:
> > Allow platform_surfaceless to use swrast even if DRM is not available.
> > To be used to allow a fuzzer for virgl to be run on a jailed VM without
> > hardware GL or DRM support.
> > 
> > Signed-off-by: David Riley 
> > ---
> >  src/egl/drivers/dri2/platform_surfaceless.c | 20 
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/src/egl/drivers/dri2/platform_surfaceless.c 
> > b/src/egl/drivers/dri2/platform_surfaceless.c
> > index f5fe711..3b17e95 100644
> > --- a/src/egl/drivers/dri2/platform_surfaceless.c
> > +++ b/src/egl/drivers/dri2/platform_surfaceless.c
> > @@ -293,6 +293,7 @@ surfaceless_probe_device(_EGLDisplay *dpy, bool swrast)
> > int fd;
> > int i;
> >  
> > +   // Attempt to find DRM device.
> 
> Nit: we use C-style comments in Mesa (`/* foo */`).
> 
> This can be fixed up when pushing your patches, which are both:
> Reviewed-by: Eric Engestrom 
> 
> Would you like me to push the patches for you?

Hi David & Eric, I pushed these to master.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 6/7] egl/android: Implement EGL_KHR_mutable_render_buffer

[Chad Versace]

Specifically, implement the extension DRI_MutableRenderBufferLoader.
However, the loader enables EGL_KHR_mutable_render_buffer only if the
DRI driver implements its half of the extension,
DRI_MutableRenderBufferDriver.
---
 src/egl/drivers/dri2/egl_dri2.c |  38 +-
 src/egl/drivers/dri2/egl_dri2.h |   7 +
 src/egl/drivers/dri2/platform_android.c | 168 +++-
 3 files changed, 206 insertions(+), 7 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 1208ebb3156..1a7338aa51d 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -272,7 +272,10 @@ dri2_add_config(_EGLDisplay *disp, const __DRIconfig 
*dri_config, int id,
  _eglSetConfigKey(, EGL_MAX_PBUFFER_HEIGHT,
   _EGL_MAX_PBUFFER_HEIGHT);
  break;
-
+  case __DRI_ATTRIB_MUTABLE_RENDER_BUFFER:
+ if (disp->Extensions.KHR_mutable_render_buffer)
+surface_type |= EGL_MUTABLE_RENDER_BUFFER_BIT_KHR;
+ break;
   default:
  key = dri2_to_egl_attribute_map[attrib];
  if (key != 0)
@@ -432,6 +435,7 @@ static const struct dri2_extension_match 
optional_core_extensions[] = {
{ __DRI_IMAGE, 1, offsetof(struct dri2_egl_display, image) },
{ __DRI2_FLUSH_CONTROL, 1, offsetof(struct dri2_egl_display, flush_control) 
},
{ __DRI2_BLOB, 1, offsetof(struct dri2_egl_display, blob) },
+   { __DRI_MUTABLE_RENDER_BUFFER_DRIVER, 1, offsetof(struct dri2_egl_display, 
mutable_render_buffer) },
{ NULL, 0, 0 }
 };
 
@@ -1459,6 +1463,8 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
 {
struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
struct dri2_egl_context *dri2_ctx = dri2_egl_context(ctx);
+   _EGLDisplay *old_disp = NULL;
+   struct dri2_egl_display *old_dri2_dpy = NULL;
_EGLContext *old_ctx;
_EGLSurface *old_dsurf, *old_rsurf;
_EGLSurface *tmp_dsurf, *tmp_rsurf;
@@ -1475,6 +1481,11 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
   return EGL_FALSE;
}
 
+   if (old_ctx) {
+  old_disp = old_ctx->Resource.Display;
+  old_dri2_dpy = dri2_egl_display(old_disp);
+   }
+
/* flush before context switch */
if (old_ctx)
   dri2_gl_flush();
@@ -1488,6 +1499,13 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
 
   if (old_dsurf)
  dri2_surf_update_fence_fd(old_ctx, disp, old_dsurf);
+
+  /* Disable shared buffer mode */
+  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, 
false);
+  }
+
   dri2_dpy->core->unbindContext(old_cctx);
}
 
@@ -1500,6 +1518,11 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
  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);
@@ -1522,11 +1545,22 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
   dri2_dpy->ref_count++;
 
if (old_ctx) {
-  EGLDisplay old_disp = _eglGetDisplayHandle(old_ctx->Resource.Display);
   dri2_destroy_context(drv, disp, old_ctx);
   dri2_display_release(old_disp);
}
 
+   if (dsurf && _eglSurfaceHasMutableRenderBuffer(dsurf) &&
+   dri2_dpy->vtbl->set_shared_buffer_mode) {
+  /* Always update the shared buffer mode. This is obviously needed when
+   * the active EGL_RENDER_BUFFER is EGL_SINGLE_BUFFER. When
+   * EGL_RENDER_BUFFER is EGL_BACK_BUFFER, the update protects us in the
+   * case where external non-EGL API may have changed window's shared
+   * buffer mode since we last saw it.
+   */
+  bool mode = (dsurf->ActiveRenderBuffer == EGL_SINGLE_BUFFER);
+  dri2_dpy->vtbl->set_shared_buffer_mode(disp, dsurf, mode);
+   }
+
return EGL_TRUE;
 }
 
diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
index 5d8fbfa2356..d8a65be7085 100644
--- a/src/egl/drivers/dri2/egl_dri2.h
+++ b/src/egl/drivers/dri2/egl_dri2.h
@@ -151,6 +151,12 @@ struct dri2_egl_display_vtbl {
__DRIdrawable *(*get_dri_drawable)(_EGLSurface *surf);
 
void (*close_screen_notify)(_EGLDisplay *dpy);
+
+   /* Used in EGL_KHR_mutable_render_buffer to update the native window's
+* shared buffer mode.
+*/
+   bool (*set_shared_buffer_mode)(_EGLDisplay *dpy, _EGLSurface *surf,
+  bool mode);
 };
 
 struct dri2_egl_display
@@ -178,6 +184,7 @@ struct dri2_egl_display
const __DRI2blobExtension *blob;
const 

[Mesa-dev] [PATCH 4/7] dri: Add param driCreateConfigs(mutable_render_buffer)

[Chad Versace]

If set, then the config will have __DRI_ATTRIB_MUTABLE_RENDER_BUFFER,
which translates to EGL_MUTABLE_RENDER_BUFFER_BIT_KHR.

Not used yet.
---
 src/gallium/state_trackers/dri/dri_screen.c   | 4 ++--
 src/mesa/drivers/dri/common/utils.c   | 9 +++--
 src/mesa/drivers/dri/common/utils.h   | 3 ++-
 src/mesa/drivers/dri/i915/intel_screen.c  | 4 ++--
 src/mesa/drivers/dri/i965/intel_screen.c  | 6 +++---
 src/mesa/drivers/dri/nouveau/nouveau_screen.c | 2 +-
 src/mesa/drivers/dri/radeon/radeon_screen.c   | 2 +-
 src/mesa/drivers/dri/swrast/swrast.c  | 2 +-
 8 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/src/gallium/state_trackers/dri/dri_screen.c 
b/src/gallium/state_trackers/dri/dri_screen.c
index 87729c190a8..99fb9e595ca 100644
--- a/src/gallium/state_trackers/dri/dri_screen.c
+++ b/src/gallium/state_trackers/dri/dri_screen.c
@@ -274,7 +274,7 @@ dri_fill_in_modes(struct dri_screen *screen)
 depth_buffer_factor, back_buffer_modes,
 ARRAY_SIZE(back_buffer_modes),
 msaa_modes, 1,
-GL_TRUE, !mixed_color_depth);
+GL_TRUE, !mixed_color_depth, GL_FALSE);
  configs = driConcatConfigs(configs, new_configs);
 
  /* Multi-sample configs without an accumulation buffer. */
@@ -284,7 +284,7 @@ dri_fill_in_modes(struct dri_screen *screen)
depth_buffer_factor, 
back_buffer_modes,
ARRAY_SIZE(back_buffer_modes),
msaa_modes+1, num_msaa_modes-1,
-   GL_FALSE, !mixed_color_depth);
+   GL_FALSE, !mixed_color_depth, 
GL_FALSE);
 configs = driConcatConfigs(configs, new_configs);
  }
   }
diff --git a/src/mesa/drivers/dri/common/utils.c 
b/src/mesa/drivers/dri/common/utils.c
index 86169d5c214..88835a7add5 100644
--- a/src/mesa/drivers/dri/common/utils.c
+++ b/src/mesa/drivers/dri/common/utils.c
@@ -147,7 +147,10 @@ driGetRendererString( char * buffer, const char * 
hardware_name,
  * \param color_depth_match Whether the color depth must match the zs depth
  *  This forces 32-bit color to have 24-bit depth, and
  *  16-bit color to have 16-bit depth.
- * 
+ * \param mutable_render_buffer Enable __DRI_ATTRIB_MUTABLE_RENDER_BUFFER,
+ *  which translates to
+ *  EGL_MUTABLE_RENDER_BUFFER_BIT_KHR.
+ *
  * \returns
  * Pointer to any array of pointers to the \c __DRIconfig structures created
  * for the specified formats.  If there is an error, \c NULL is returned.
@@ -160,7 +163,8 @@ driCreateConfigs(mesa_format format,
 unsigned num_depth_stencil_bits,
 const GLenum * db_modes, unsigned num_db_modes,
 const uint8_t * msaa_samples, unsigned num_msaa_modes,
-GLboolean enable_accum, GLboolean color_depth_match)
+GLboolean enable_accum, GLboolean color_depth_match,
+GLboolean mutable_render_buffer)
 {
static const uint32_t masks_table[][4] = {
   /* MESA_FORMAT_B5G6R5_UNORM */
@@ -325,6 +329,7 @@ driCreateConfigs(mesa_format format,
 
modes->yInverted = GL_TRUE;
modes->sRGBCapable = is_srgb;
+   modes->mutableRenderBuffer = mutable_render_buffer;
}
}
}
diff --git a/src/mesa/drivers/dri/common/utils.h 
b/src/mesa/drivers/dri/common/utils.h
index 7be0465c261..7c9719f9f42 100644
--- a/src/mesa/drivers/dri/common/utils.h
+++ b/src/mesa/drivers/dri/common/utils.h
@@ -45,7 +45,8 @@ driCreateConfigs(mesa_format format,
 unsigned num_depth_stencil_bits,
 const GLenum * db_modes, unsigned num_db_modes,
 const uint8_t * msaa_samples, unsigned num_msaa_modes,
-GLboolean enable_accum, GLboolean color_depth_match);
+GLboolean enable_accum, GLboolean color_depth_match,
+GLboolean mutable_render_buffer);
 
 __DRIconfig **driConcatConfigs(__DRIconfig **a,
   __DRIconfig **b);
diff --git a/src/mesa/drivers/dri/i915/intel_screen.c 
b/src/mesa/drivers/dri/i915/intel_screen.c
index 882c498622f..27be9219e47 100644
--- a/src/mesa/drivers/dri/i915/intel_screen.c
+++ b/src/mesa/drivers/dri/i915/intel_screen.c
@@ -1094,7 +1094,7 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
  num_depth_stencil_bits,
  back_buffer_modes, 2,
  singlesample_samples, 1,
- false, false);
+ 

[Mesa-dev] [PATCH 1/7] egl: Simplify queries for EGL_RENDER_BUFFER

[Chad Versace]

There exist *two* queryable EGL_RENDER_BUFFER states in EGL:
eglQuerySurface(EGL_RENDER_BUFFER) and
eglQueryContext(EGL_RENDER_BUFFER).

These changes eliminate potentially very fragile code in the upcoming
EGL_KHR_mutable_render_buffer implementation.

* eglQuerySurface(EGL_RENDER_BUFFER)

  The implementation of eglQuerySurface(EGL_RENDER_BUFFER) contained
  abstruse logic which required comprehending the specification
  complexities of how the two EGL_RENDER_BUFFER states interact.  The
  function sometimes returned _EGLContext::WindowRenderBuffer, sometimes
  _EGLSurface::RenderBuffer. Why? The function tried to encode the
  actual logic from the EGL spec. When did the function return which
  variable? Go study the EGL spec, hope you understand it, then hope
  Mesa mutated the EGL_RENDER_BUFFER state in all the correct places.
  Have fun.

  To simplify eglQuerySurface(EGL_RENDER_BUFFER), and to improve
  confidence in its correctness, flatten its indirect logic. For pixmap
  and pbuffer surfaces, simply return a hard-coded literal value, as the
  spec suggests. For window surfaces, simply return
  _EGLSurface::RequestedRenderBuffer.  Nothing difficult here.

* eglQueryContext(EGL_RENDER_BUFFER)

  The implementation of this suffered from the same issues as
  eglQuerySurface, and the solution is the same.  confidence in its
  correctness, flatten its indirect logic. For pixmap and pbuffer
  surfaces, simply return a hard-coded literal value, as the spec
  suggests. For window surfaces, simply return
  _EGLSurface::ActiveRenderBuffer.
---
 src/egl/drivers/dri2/egl_dri2.c |  6 -
 src/egl/main/eglcontext.c   | 40 +++--
 src/egl/main/eglcontext.h   |  3 ---
 src/egl/main/eglsurface.c   | 28 ---
 src/egl/main/eglsurface.h   | 28 ++-
 5 files changed, 85 insertions(+), 20 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index c3024795a10..e109ad37f55 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1289,12 +1289,6 @@ dri2_create_context(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLConfig *conf,
  dri_config = dri2_config->dri_config[1][0];
   else
  dri_config = dri2_config->dri_config[0][0];
-
-  /* EGL_WINDOW_BIT is set only when there is a double-buffered dri_config.
-   * This makes sure the back buffer will always be used.
-   */
-  if (conf->SurfaceType & EGL_WINDOW_BIT)
- dri2_ctx->base.WindowRenderBuffer = EGL_BACK_BUFFER;
}
else
   dri_config = NULL;
diff --git a/src/egl/main/eglcontext.c b/src/egl/main/eglcontext.c
index 3e5d8ad97bf..ecc546e1132 100644
--- a/src/egl/main/eglcontext.c
+++ b/src/egl/main/eglcontext.c
@@ -588,7 +588,6 @@ _eglInitContext(_EGLContext *ctx, _EGLDisplay *dpy, 
_EGLConfig *conf,
_eglInitResource(>Resource, sizeof(*ctx), dpy);
ctx->ClientAPI = api;
ctx->Config = conf;
-   ctx->WindowRenderBuffer = EGL_NONE;
ctx->Profile = EGL_CONTEXT_OPENGL_CORE_PROFILE_BIT_KHR;
 
ctx->ClientMajorVersion = 1; /* the default, per EGL spec */
@@ -620,15 +619,42 @@ static EGLint
 _eglQueryContextRenderBuffer(_EGLContext *ctx)
 {
_EGLSurface *surf = ctx->DrawSurface;
-   EGLint rb;
 
+   /* From the EGL 1.5 spec:
+*
+*- If the context is not bound to a surface, then EGL_NONE will be
+*  returned.
+*/
if (!surf)
   return EGL_NONE;
-   if (surf->Type == EGL_WINDOW_BIT && ctx->WindowRenderBuffer != EGL_NONE)
-  rb = ctx->WindowRenderBuffer;
-   else
-  rb = surf->RenderBuffer;
-   return rb;
+
+   switch (surf->Type) {
+   default:
+  unreachable("bad EGLSurface type");
+   case EGL_PIXMAP_BIT:
+  /* - If the context is bound to a pixmap surface, then EGL_SINGLE_BUFFER
+   *   will be returned.
+   */
+  return EGL_SINGLE_BUFFER;
+   case EGL_PBUFFER_BIT:
+  /* - If the context is bound to a pbuffer surface, then EGL_BACK_BUFFER
+   *   will be returned.
+   */
+  return EGL_BACK_BUFFER;
+   case EGL_WINDOW_BIT:
+  /* - If the context is bound to a window surface, then either
+   *   EGL_BACK_BUFFER or EGL_SINGLE_BUFFER may be returned. The value
+   *   returned depends on both the buffer requested by the setting of the
+   *   EGL_RENDER_BUFFER property of the surface [...], and on the client
+   *   API (not all client APIs support single-buffer Rendering to window
+   *   surfaces). Some client APIs allow control of whether rendering goes
+   *   to the front or back buffer. This client API-specific choice is not
+   *   reflected in the returned value, which only describes the buffer
+   *   that will be rendered to by default if not overridden by the client
+   *   API.
+   */
+  return surf->ActiveRenderBuffer;
+   }
 }
 
 
diff --git a/src/egl/main/eglcontext.h b/src/egl/main/eglcontext.h
index 8d97ef9eab9..deddefdcaba 

[Mesa-dev] [PATCH 4/7] dri: Add param driCreateConfigs(mutable_render_buffer)

[Chad Versace]

If set, then the config will have __DRI_ATTRIB_MUTABLE_RENDER_BUFFER,
which translates to EGL_MUTABLE_RENDER_BUFFER_BIT_KHR.

Not used yet.
---
 src/gallium/state_trackers/dri/dri_screen.c   | 4 ++--
 src/mesa/drivers/dri/common/utils.c   | 9 +++--
 src/mesa/drivers/dri/common/utils.h   | 3 ++-
 src/mesa/drivers/dri/i915/intel_screen.c  | 4 ++--
 src/mesa/drivers/dri/i965/intel_screen.c  | 6 +++---
 src/mesa/drivers/dri/nouveau/nouveau_screen.c | 2 +-
 src/mesa/drivers/dri/radeon/radeon_screen.c   | 2 +-
 src/mesa/drivers/dri/swrast/swrast.c  | 2 +-
 8 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/src/gallium/state_trackers/dri/dri_screen.c 
b/src/gallium/state_trackers/dri/dri_screen.c
index 87729c190a8..99fb9e595ca 100644
--- a/src/gallium/state_trackers/dri/dri_screen.c
+++ b/src/gallium/state_trackers/dri/dri_screen.c
@@ -274,7 +274,7 @@ dri_fill_in_modes(struct dri_screen *screen)
 depth_buffer_factor, back_buffer_modes,
 ARRAY_SIZE(back_buffer_modes),
 msaa_modes, 1,
-GL_TRUE, !mixed_color_depth);
+GL_TRUE, !mixed_color_depth, GL_FALSE);
  configs = driConcatConfigs(configs, new_configs);
 
  /* Multi-sample configs without an accumulation buffer. */
@@ -284,7 +284,7 @@ dri_fill_in_modes(struct dri_screen *screen)
depth_buffer_factor, 
back_buffer_modes,
ARRAY_SIZE(back_buffer_modes),
msaa_modes+1, num_msaa_modes-1,
-   GL_FALSE, !mixed_color_depth);
+   GL_FALSE, !mixed_color_depth, 
GL_FALSE);
 configs = driConcatConfigs(configs, new_configs);
  }
   }
diff --git a/src/mesa/drivers/dri/common/utils.c 
b/src/mesa/drivers/dri/common/utils.c
index 86169d5c214..88835a7add5 100644
--- a/src/mesa/drivers/dri/common/utils.c
+++ b/src/mesa/drivers/dri/common/utils.c
@@ -147,7 +147,10 @@ driGetRendererString( char * buffer, const char * 
hardware_name,
  * \param color_depth_match Whether the color depth must match the zs depth
  *  This forces 32-bit color to have 24-bit depth, and
  *  16-bit color to have 16-bit depth.
- * 
+ * \param mutable_render_buffer Enable __DRI_ATTRIB_MUTABLE_RENDER_BUFFER,
+ *  which translates to
+ *  EGL_MUTABLE_RENDER_BUFFER_BIT_KHR.
+ *
  * \returns
  * Pointer to any array of pointers to the \c __DRIconfig structures created
  * for the specified formats.  If there is an error, \c NULL is returned.
@@ -160,7 +163,8 @@ driCreateConfigs(mesa_format format,
 unsigned num_depth_stencil_bits,
 const GLenum * db_modes, unsigned num_db_modes,
 const uint8_t * msaa_samples, unsigned num_msaa_modes,
-GLboolean enable_accum, GLboolean color_depth_match)
+GLboolean enable_accum, GLboolean color_depth_match,
+GLboolean mutable_render_buffer)
 {
static const uint32_t masks_table[][4] = {
   /* MESA_FORMAT_B5G6R5_UNORM */
@@ -325,6 +329,7 @@ driCreateConfigs(mesa_format format,
 
modes->yInverted = GL_TRUE;
modes->sRGBCapable = is_srgb;
+   modes->mutableRenderBuffer = mutable_render_buffer;
}
}
}
diff --git a/src/mesa/drivers/dri/common/utils.h 
b/src/mesa/drivers/dri/common/utils.h
index 7be0465c261..7c9719f9f42 100644
--- a/src/mesa/drivers/dri/common/utils.h
+++ b/src/mesa/drivers/dri/common/utils.h
@@ -45,7 +45,8 @@ driCreateConfigs(mesa_format format,
 unsigned num_depth_stencil_bits,
 const GLenum * db_modes, unsigned num_db_modes,
 const uint8_t * msaa_samples, unsigned num_msaa_modes,
-GLboolean enable_accum, GLboolean color_depth_match);
+GLboolean enable_accum, GLboolean color_depth_match,
+GLboolean mutable_render_buffer);
 
 __DRIconfig **driConcatConfigs(__DRIconfig **a,
   __DRIconfig **b);
diff --git a/src/mesa/drivers/dri/i915/intel_screen.c 
b/src/mesa/drivers/dri/i915/intel_screen.c
index 882c498622f..27be9219e47 100644
--- a/src/mesa/drivers/dri/i915/intel_screen.c
+++ b/src/mesa/drivers/dri/i915/intel_screen.c
@@ -1094,7 +1094,7 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
  num_depth_stencil_bits,
  back_buffer_modes, 2,
  singlesample_samples, 1,
- false, false);
+ 

[Mesa-dev] [PATCH 3/7] dri: Define DRI_MutableRenderBuffer extensions

[Chad Versace]

Define extensions DRI_MutableRenderBufferDriver and
DRI_MutableRenderBufferLoader. These are the two halves for
EGL_KHR_mutable_render_buffer.

Outside the DRI code there is one additional change.  Add
gl_config::mutableRenderBuffer to match
__DRI_ATTRIB_MUTABLE_RENDER_BUFFER. Neither are used yet.
---
 include/GL/internal/dri_interface.h| 138 -
 src/mesa/drivers/dri/common/dri_util.c |   2 +
 src/mesa/drivers/dri/common/dri_util.h |   4 +
 src/mesa/drivers/dri/common/utils.c|   1 +
 src/mesa/main/mtypes.h |   3 +
 5 files changed, 145 insertions(+), 3 deletions(-)

diff --git a/include/GL/internal/dri_interface.h 
b/include/GL/internal/dri_interface.h
index c32cdd3767a..245c845832b 100644
--- a/include/GL/internal/dri_interface.h
+++ b/include/GL/internal/dri_interface.h
@@ -48,6 +48,7 @@ typedef unsigned int drm_drawable_t;
 typedef struct drm_clip_rect drm_clip_rect_t;
 #endif
 
+#include 
 #include 
 
 /**
@@ -746,7 +747,8 @@ struct __DRIuseInvalidateExtensionRec {
 #define __DRI_ATTRIB_BIND_TO_TEXTURE_TARGETS   46
 #define __DRI_ATTRIB_YINVERTED 47
 #define __DRI_ATTRIB_FRAMEBUFFER_SRGB_CAPABLE  48
-#define __DRI_ATTRIB_MAX   
(__DRI_ATTRIB_FRAMEBUFFER_SRGB_CAPABLE + 1)
+#define __DRI_ATTRIB_MUTABLE_RENDER_BUFFER 49 /* 
EGL_MUTABLE_RENDER_BUFFER_BIT_KHR */
+#define __DRI_ATTRIB_MAX   50
 
 /* __DRI_ATTRIB_RENDER_TYPE */
 #define __DRI_ATTRIB_RGBA_BIT  0x01
@@ -1888,9 +1890,57 @@ struct __DRI2rendererQueryExtensionRec {
  * Image Loader extension. Drivers use this to allocate color buffers
  */
 
+/**
+ * See __DRIimageLoaderExtensionRec::getBuffers::buffer_mask.
+ */
 enum __DRIimageBufferMask {
__DRI_IMAGE_BUFFER_BACK = (1 << 0),
-   __DRI_IMAGE_BUFFER_FRONT = (1 << 1)
+   __DRI_IMAGE_BUFFER_FRONT = (1 << 1),
+
+   /**
+* A buffer shared between application and compositor. The buffer may be
+* simultaneously accessed by each.
+*
+* A shared buffer is equivalent to an EGLSurface whose EGLConfig contains
+* EGL_MUTABLE_RENDER_BUFFER_BIT_KHR and whose active EGL_RENDER_BUFFER (as
+* opposed to any pending, requested change to EGL_RENDER_BUFFER) is
+* EGL_SINGLE_BUFFER.
+*
+* If buffer_mask contains __DRI_IMAGE_BUFFER_SHARED, then must contains no
+* other bits. As a corollary, a __DRIdrawable that has a "shared" buffer
+* has no front nor back buffer.
+*
+* The loader returns __DRI_IMAGE_BUFFER_SHARED in buffer_mask if and only
+* if:
+* - The loader supports __DRI_MUTABLE_RENDER_BUFFER_LOADER.
+* - The driver supports __DRI_MUTABLE_RENDER_BUFFER_DRIVER.
+* - The EGLConfig of the drawable EGLSurface contains
+*   EGL_MUTABLE_RENDER_BUFFER_BIT_KHR.
+* - The EGLContext's EGL_RENDER_BUFFER is EGL_SINGLE_BUFFER.
+*   Equivalently, the EGLSurface's active EGL_RENDER_BUFFER (as
+*   opposed to any pending, requested change to EGL_RENDER_BUFFER) is
+*   EGL_SINGLE_BUFFER. (See the EGL 1.5 and
+*   EGL_KHR_mutable_render_buffer spec for details about "pending" vs
+*   "active" EGL_RENDER_BUFFER state).
+*
+* A shared buffer is similar to a front buffer in that all rendering to the
+* buffer should appear promptly on the screen. It is different from
+* a front buffer in that its behavior is independent from the
+* GL_DRAW_BUFFER state. Specifically, if GL_DRAW_FRAMEBUFFER is 0 and the
+* __DRIdrawable's buffer_mask is __DRI_IMAGE_BUFFER_SHARED, then all
+* rendering should appear promptly on the screen if GL_DRAW_BUFFER is not
+* GL_NONE.
+*
+* The difference between a shared buffer and a front buffer is motivated
+* by the constraints of Android and OpenGL ES. OpenGL ES does not support
+* front-buffer rendering. Android's SurfaceFlinger protocol provides the
+* EGL driver only a back buffer and no front buffer. The shared buffer
+* mode introduced by EGL_KHR_mutable_render_buffer is a backdoor though
+* EGL that allows Android OpenGL ES applications to render to what is
+* effectively the front buffer, a backdoor that required no change to the
+* OpenGL ES API and little change to the SurfaceFlinger API.
+*/
+   __DRI_IMAGE_BUFFER_SHARED = (1 << 2),
 };
 
 struct __DRIimageList {
@@ -1915,7 +1965,8 @@ struct __DRIimageLoaderExtensionRec {
 * \param stamp  Address of variable to be updated when
 *   getBuffers must be called again
 * \param loaderPrivate  The loaderPrivate for driDrawable
-* \param buffer_maskSet of buffers to allocate
+* \param buffer_maskSet of buffers to allocate. A bitmask of
+*   __DRIimageBufferMask.
 * \param buffersReturned buffers
 */
int (*getBuffers)(__DRIdrawable *driDrawable,
@@ -2029,4 +2080,85 @@ struct 

[Mesa-dev] [PATCH 5/7] egl/main: Add bits for EGL_KHR_mutable_render_buffer

[Chad Versace]

A follow-up patch enables EGL_KHR_mutable_render_buffer for Android.
This patch is separate from the Android patch because I think it's
easier to review the platform-independent bits separately.
---
 src/egl/main/eglapi.c |  1 +
 src/egl/main/eglconfig.c  |  3 ++
 src/egl/main/egldisplay.h |  1 +
 src/egl/main/eglsurface.c | 59 +--
 src/egl/main/eglsurface.h | 33 +-
 5 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index deb479b6d56..f10ec994fde 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -509,6 +509,7 @@ _eglCreateExtensionsString(_EGLDisplay *dpy)
_EGL_CHECK_EXTENSION(KHR_image);
_EGL_CHECK_EXTENSION(KHR_image_base);
_EGL_CHECK_EXTENSION(KHR_image_pixmap);
+   _EGL_CHECK_EXTENSION(KHR_mutable_render_buffer);
_EGL_CHECK_EXTENSION(KHR_no_config_context);
_EGL_CHECK_EXTENSION(KHR_partial_update);
_EGL_CHECK_EXTENSION(KHR_reusable_sync);
diff --git a/src/egl/main/eglconfig.c b/src/egl/main/eglconfig.c
index 2d3b3ddd908..67d11ec9b0b 100644
--- a/src/egl/main/eglconfig.c
+++ b/src/egl/main/eglconfig.c
@@ -272,6 +272,7 @@ static const struct {
 EGLBoolean
 _eglValidateConfig(const _EGLConfig *conf, EGLBoolean for_matching)
 {
+   _EGLDisplay *disp = conf->Display;
EGLint i, attr, val;
EGLBoolean valid = EGL_TRUE;
 
@@ -340,6 +341,8 @@ _eglValidateConfig(const _EGLConfig *conf, EGLBoolean 
for_matching)
EGL_VG_ALPHA_FORMAT_PRE_BIT |
EGL_MULTISAMPLE_RESOLVE_BOX_BIT |
EGL_SWAP_BEHAVIOR_PRESERVED_BIT;
+if (disp->Extensions.KHR_mutable_render_buffer)
+   mask |= EGL_MUTABLE_RENDER_BUFFER_BIT_KHR;
 break;
  case EGL_RENDERABLE_TYPE:
  case EGL_CONFORMANT:
diff --git a/src/egl/main/egldisplay.h b/src/egl/main/egldisplay.h
index d7e51991a45..4a7f248e34c 100644
--- a/src/egl/main/egldisplay.h
+++ b/src/egl/main/egldisplay.h
@@ -127,6 +127,7 @@ struct _egl_extensions
EGLBoolean KHR_image;
EGLBoolean KHR_image_base;
EGLBoolean KHR_image_pixmap;
+   EGLBoolean KHR_mutable_render_buffer;
EGLBoolean KHR_no_config_context;
EGLBoolean KHR_partial_update;
EGLBoolean KHR_reusable_sync;
diff --git a/src/egl/main/eglsurface.c b/src/egl/main/eglsurface.c
index 73c31e11588..926b7ab569a 100644
--- a/src/egl/main/eglsurface.c
+++ b/src/egl/main/eglsurface.c
@@ -123,6 +123,12 @@ _eglParseSurfaceAttribList(_EGLSurface *surf, const EGLint 
*attrib_list)
 break;
  }
  surf->RequestedRenderBuffer = val;
+ if (surf->Config->SurfaceType & EGL_MUTABLE_RENDER_BUFFER_BIT_KHR) {
+/* Unlike normal EGLSurfaces, one with a mutable render buffer
+ * uses the application-chosen render buffer.
+ */
+surf->ActiveRenderBuffer = val;
+ }
  break;
   case EGL_POST_SUB_BUFFER_SUPPORTED_NV:
  if (!dpy->Extensions.NV_post_sub_buffer ||
@@ -359,12 +365,19 @@ _eglQuerySurface(_EGLDriver *drv, _EGLDisplay *dpy, 
_EGLSurface *surface,
   *value = surface->SwapBehavior;
   break;
case EGL_RENDER_BUFFER:
-  /* From the EGL 1.5 spec (2014.08.27):
+  /* From the EGL_KHR_mutable_render_buffer spec (v12):
*
*Querying EGL_RENDER_BUFFER returns the buffer which client API
*rendering is requested to use. For a window surface, this is the
-   *same attribute value specified when the surface was created. For
-   *a pbuffer surface, it is always EGL_BACK_BUFFER . For a pixmap
+   *attribute value specified when the surface was created or last set
+   *via eglSurfaceAttrib.
+   *
+   * In other words, querying a window surface returns the value most
+   * recently *requested* by the user.
+   *
+   * The paragraph continues in the EGL 1.5 spec (2014.08.27):
+   *
+   *For a pbuffer surface, it is always EGL_BACK_BUFFER . For a pixmap
*surface, it is always EGL_SINGLE_BUFFER . To determine the actual
*buffer being rendered to by a context, call eglQueryContext.
*/
@@ -472,6 +485,31 @@ _eglSurfaceAttrib(_EGLDriver *drv, _EGLDisplay *dpy, 
_EGLSurface *surface,
  break;
   surface->MultisampleResolve = value;
   break;
+   case EGL_RENDER_BUFFER:
+  if (!dpy->Extensions.KHR_mutable_render_buffer) {
+ err = EGL_BAD_ATTRIBUTE;
+ break;
+  }
+
+  if (value != EGL_BACK_BUFFER && value != EGL_SINGLE_BUFFER) {
+ err = EGL_BAD_PARAMETER;
+ break;
+  }
+
+  /* From the EGL_KHR_mutable_render_buffer spec (v12):
+   *
+   *If attribute is EGL_RENDER_BUFFER, and the EGL_SURFACE_TYPE
+   *attribute of the EGLConfig used to create surface does not contain
+   *EGL_MUTABLE_RENDER_BUFFER_BIT_KHR, [...] an EGL_BAD_MATCH error is
+   * 

[Mesa-dev] [PATCH 7/7] i965: Implement EGL_KHR_mutable_render_buffer

[Chad Versace]

Tested with a low-latency handwriting application on Android Nougat on
the Chrome OS Pixelbook (codename Eve) with Kabylake.
---
 src/mesa/drivers/dri/i965/brw_context.c  | 86 +++-
 src/mesa/drivers/dri/i965/brw_context.h  | 12 
 src/mesa/drivers/dri/i965/intel_screen.c | 13 +++-
 3 files changed, 107 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index 968fc1d43d6..9dfd9520555 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -239,6 +239,35 @@ intel_flush_front(struct gl_context *ctx)
}
 }
 
+static void
+brw_display_shared_buffer(struct brw_context *brw)
+{
+   __DRIcontext *dri_context = brw->driContext;
+   __DRIdrawable *dri_drawable = dri_context->driDrawablePriv;
+   __DRIscreen *dri_screen = brw->screen->driScrnPriv;
+   int fence_fd = -1;
+
+   if (!brw->is_shared_buffer_bound)
+  return;
+
+   if (!brw->is_shared_buffer_dirty)
+  return;
+
+   if (brw->screen->has_exec_fence) {
+  /* This function is always called during a flush operation, so there is
+   * no need to flush again here. But we want to provide a fence_fd to the
+   * loader, and a redundant flush is the easiest way to acquire one.
+   */
+  if (intel_batchbuffer_flush_fence(brw, -1, _fd))
+ return;
+   }
+
+   dri_screen->mutableRenderBuffer.loader
+  ->displaySharedBuffer(dri_drawable, fence_fd,
+dri_drawable->loaderPrivate);
+   brw->is_shared_buffer_dirty = false;
+}
+
 static void
 intel_glFlush(struct gl_context *ctx)
 {
@@ -246,7 +275,7 @@ intel_glFlush(struct gl_context *ctx)
 
intel_batchbuffer_flush(brw);
intel_flush_front(ctx);
-
+   brw_display_shared_buffer(brw);
brw->need_flush_throttle = true;
 }
 
@@ -1457,6 +1486,11 @@ intel_prepare_render(struct brw_context *brw)
 */
if (_mesa_is_front_buffer_drawing(ctx->DrawBuffer))
   brw->front_buffer_dirty = true;
+
+   if (brw->is_shared_buffer_bound) {
+  /* Subsequent rendering will probably dirty the shared buffer. */
+  brw->is_shared_buffer_dirty = true;
+   }
 }
 
 /**
@@ -1690,8 +1724,12 @@ intel_update_image_buffer(struct brw_context *intel,
else
   last_mt = rb->singlesample_mt;
 
-   if (last_mt && last_mt->bo == buffer->bo)
+   if (last_mt && last_mt->bo == buffer->bo) {
+  if (buffer_type == __DRI_IMAGE_BUFFER_SHARED) {
+ intel_miptree_make_shareable(intel, last_mt);
+  }
   return;
+   }
 
/* Only allow internal compression if samples == 0.  For multisampled
 * window system buffers, the only thing the single-sampled buffer is used
@@ -1720,6 +1758,35 @@ intel_update_image_buffer(struct brw_context *intel,
rb->Base.Base.NumSamples > 1) {
   intel_renderbuffer_upsample(intel, rb);
}
+
+   if (buffer_type == __DRI_IMAGE_BUFFER_SHARED) {
+  /* The compositor and the application may access this image
+   * concurrently. The display hardware may even scanout the image while
+   * the GPU is rendering to it.  Aux surfaces cause difficulty with
+   * concurrent access, so permanently disable aux for this miptree.
+   *
+   * Perhaps we could improve overall application performance by
+   * re-enabling the aux surface when EGL_RENDER_BUFFER transitions to
+   * EGL_BACK_BUFFER, then disabling it again when EGL_RENDER_BUFFER
+   * returns to EGL_SINGLE_BUFFER. I expect the wins and losses with this
+   * approach to be highly dependent on the application's GL usage.
+   *
+   * I [chadv] expect clever disabling/reenabling to be counterproductive
+   * in the use cases I care about: applications that render nearly
+   * realtime handwriting to the surface while possibly undergiong
+   * simultaneously scanout as a display plane. The app requires low
+   * render latency. Even though the app spends most of its time in
+   * shared-buffer mode, it also frequently transitions between
+   * shared-buffer (EGL_SINGLE_BUFFER) and double-buffer (EGL_BACK_BUFFER)
+   * mode.  Visual sutter during the transitions should be avoided.
+   *
+   * In this case, I [chadv] believe reducing the GPU workload at
+   * shared-buffer/double-buffer transitions would offer a smoother app
+   * experience than any savings due to aux compression. But I've
+   * collected no data to prove my theory.
+   */
+  intel_miptree_make_shareable(intel, mt);
+   }
 }
 
 static void
@@ -1780,4 +1847,19 @@ intel_update_image_buffers(struct brw_context *brw, 
__DRIdrawable *drawable)
 images.back,
 __DRI_IMAGE_BUFFER_BACK);
}
+
+   if (images.image_mask & __DRI_IMAGE_BUFFER_SHARED) {
+  assert(images.image_mask == __DRI_IMAGE_BUFFER_SHARED);
+  drawable->w = images.back->width;
+  drawable->h = images.back->height;
+  

[Mesa-dev] [PATCH 6/7] egl/android: Implement EGL_KHR_mutable_render_buffer

[Chad Versace]

Specifically, implement the extension DRI_MutableRenderBufferLoader.
However, the loader enables EGL_KHR_mutable_render_buffer only if the
DRI driver implements its half of the extension,
DRI_MutableRenderBufferDriver.
---
 src/egl/drivers/dri2/egl_dri2.c |  38 +-
 src/egl/drivers/dri2/egl_dri2.h |   7 +
 src/egl/drivers/dri2/platform_android.c | 168 +++-
 3 files changed, 206 insertions(+), 7 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 1208ebb3156..1a7338aa51d 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -272,7 +272,10 @@ dri2_add_config(_EGLDisplay *disp, const __DRIconfig 
*dri_config, int id,
  _eglSetConfigKey(, EGL_MAX_PBUFFER_HEIGHT,
   _EGL_MAX_PBUFFER_HEIGHT);
  break;
-
+  case __DRI_ATTRIB_MUTABLE_RENDER_BUFFER:
+ if (disp->Extensions.KHR_mutable_render_buffer)
+surface_type |= EGL_MUTABLE_RENDER_BUFFER_BIT_KHR;
+ break;
   default:
  key = dri2_to_egl_attribute_map[attrib];
  if (key != 0)
@@ -432,6 +435,7 @@ static const struct dri2_extension_match 
optional_core_extensions[] = {
{ __DRI_IMAGE, 1, offsetof(struct dri2_egl_display, image) },
{ __DRI2_FLUSH_CONTROL, 1, offsetof(struct dri2_egl_display, flush_control) 
},
{ __DRI2_BLOB, 1, offsetof(struct dri2_egl_display, blob) },
+   { __DRI_MUTABLE_RENDER_BUFFER_DRIVER, 1, offsetof(struct dri2_egl_display, 
mutable_render_buffer) },
{ NULL, 0, 0 }
 };
 
@@ -1459,6 +1463,8 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
 {
struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
struct dri2_egl_context *dri2_ctx = dri2_egl_context(ctx);
+   _EGLDisplay *old_disp = NULL;
+   struct dri2_egl_display *old_dri2_dpy = NULL;
_EGLContext *old_ctx;
_EGLSurface *old_dsurf, *old_rsurf;
_EGLSurface *tmp_dsurf, *tmp_rsurf;
@@ -1475,6 +1481,11 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
   return EGL_FALSE;
}
 
+   if (old_ctx) {
+  old_disp = old_ctx->Resource.Display;
+  old_dri2_dpy = dri2_egl_display(old_disp);
+   }
+
/* flush before context switch */
if (old_ctx)
   dri2_gl_flush();
@@ -1488,6 +1499,13 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
 
   if (old_dsurf)
  dri2_surf_update_fence_fd(old_ctx, disp, old_dsurf);
+
+  /* Disable shared buffer mode */
+  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, 
false);
+  }
+
   dri2_dpy->core->unbindContext(old_cctx);
}
 
@@ -1500,6 +1518,11 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
  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);
@@ -1522,11 +1545,22 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
   dri2_dpy->ref_count++;
 
if (old_ctx) {
-  EGLDisplay old_disp = _eglGetDisplayHandle(old_ctx->Resource.Display);
   dri2_destroy_context(drv, disp, old_ctx);
   dri2_display_release(old_disp);
}
 
+   if (dsurf && _eglSurfaceHasMutableRenderBuffer(dsurf) &&
+   dri2_dpy->vtbl->set_shared_buffer_mode) {
+  /* Always update the shared buffer mode. This is obviously needed when
+   * the active EGL_RENDER_BUFFER is EGL_SINGLE_BUFFER. When
+   * EGL_RENDER_BUFFER is EGL_BACK_BUFFER, the update protects us in the
+   * case where external non-EGL API may have changed window's shared
+   * buffer mode since we last saw it.
+   */
+  bool mode = (dsurf->ActiveRenderBuffer == EGL_SINGLE_BUFFER);
+  dri2_dpy->vtbl->set_shared_buffer_mode(disp, dsurf, mode);
+   }
+
return EGL_TRUE;
 }
 
diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
index 5d8fbfa2356..d8a65be7085 100644
--- a/src/egl/drivers/dri2/egl_dri2.h
+++ b/src/egl/drivers/dri2/egl_dri2.h
@@ -151,6 +151,12 @@ struct dri2_egl_display_vtbl {
__DRIdrawable *(*get_dri_drawable)(_EGLSurface *surf);
 
void (*close_screen_notify)(_EGLDisplay *dpy);
+
+   /* Used in EGL_KHR_mutable_render_buffer to update the native window's
+* shared buffer mode.
+*/
+   bool (*set_shared_buffer_mode)(_EGLDisplay *dpy, _EGLSurface *surf,
+  bool mode);
 };
 
 struct dri2_egl_display
@@ -178,6 +184,7 @@ struct dri2_egl_display
const __DRI2blobExtension *blob;
const 

[Mesa-dev] [PATCH 2/7] egl/dri2: In dri2_make_current, return early on failure

[Chad Versace]

This pulls an 'else' block into the function's main body, making the
code easier to follow.

Without this change, the upcoming EGL_KHR_mutable_render_buffer patch
transforms dri2_make_current() into spaghetti.
---
 src/egl/drivers/dri2/egl_dri2.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index e109ad37f55..1208ebb3156 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1493,20 +1493,7 @@ 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)) {
-  dri2_destroy_surface(drv, disp, old_dsurf);
-  dri2_destroy_surface(drv, disp, old_rsurf);
-
-  if (!unbind)
- dri2_dpy->ref_count++;
-  if (old_ctx) {
- EGLDisplay old_disp = _eglGetDisplayHandle(old_ctx->Resource.Display);
- dri2_destroy_context(drv, disp, old_ctx);
- dri2_display_release(old_disp);
-  }
-
-  return EGL_TRUE;
-   } else {
+   if (!unbind && !dri2_dpy->core->bindContext(cctx, ddraw, rdraw)) {
   /* undo the previous _eglBindContext */
   _eglBindContext(old_ctx, old_dsurf, old_rsurf, , _dsurf, 
_rsurf);
   assert(_ctx->base == ctx &&
@@ -1527,6 +1514,20 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
*/
   return _eglError(EGL_BAD_MATCH, "eglMakeCurrent");
}
+
+   dri2_destroy_surface(drv, disp, old_dsurf);
+   dri2_destroy_surface(drv, disp, old_rsurf);
+
+   if (!unbind)
+  dri2_dpy->ref_count++;
+
+   if (old_ctx) {
+  EGLDisplay old_disp = _eglGetDisplayHandle(old_ctx->Resource.Display);
+  dri2_destroy_context(drv, disp, old_ctx);
+  dri2_display_release(old_disp);
+   }
+
+   return EGL_TRUE;
 }
 
 __DRIdrawable *
-- 
2.18.0.345.g5c9ce644c3-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 0/7] egl, i965: Implement EGL_KHR_mutable_render_buffer

[Chad Versace]

Despite the KHR, Android is the only OS that uses this extension, as far
as I know. Essentially, it allows the app to toggle
front-buffer/back-buffer rendering using EGL.

Android requires this extension if the device advertises support for
virtual reality. No Chrome OS device yet supports Android's VR software
requirements, though, and VR is not why I implemented this.

This extension enables handwriting and drawing apps on Chrome and
Android devices to achieve very low latency between touch input
and display response.

Please don't bash me for this oddly-specified Android extension.
I didn't create it. I'm just writing the code that Android requires.

* Patch 3 defines a pair of new DRI extensions, 
DRI_MutableRenderBuffer{Driver,Loader}.
* Patches 1-5 add the generic code for the DRI and EGL extensions.
* Patch 6 implements DRI_MutableRenderBufferLoader for Android EGL.
* Patch 7 implements DRI_MutableRenderBufferDriver for i965.

This series is tagged at:

http://git.kiwitree.net/cgit/~chadv/mesa/log/?h=refs/tags/chadv/review/i965-mutable-render-buffer-v01

Chad Versace (7):
  egl: Simplify queries for EGL_RENDER_BUFFER
  egl/dri2: In dri2_make_current, return early on failure
  dri: Define DRI_MutableRenderBuffer extensions
  dri: Add param driCreateConfigs(mutable_render_buffer)
  egl/main: Add bits for EGL_KHR_mutable_render_buffer
  egl/android: Implement EGL_KHR_mutable_render_buffer
  i965: Implement EGL_KHR_mutable_render_buffer

 include/GL/internal/dri_interface.h   | 138 +-
 src/egl/drivers/dri2/egl_dri2.c   |  71 +---
 src/egl/drivers/dri2/egl_dri2.h   |   7 +
 src/egl/drivers/dri2/platform_android.c   | 168 +-
 src/egl/main/eglapi.c |   1 +
 src/egl/main/eglconfig.c  |   3 +
 src/egl/main/eglcontext.c |  40 -
 src/egl/main/eglcontext.h |   3 -
 src/egl/main/egldisplay.h |   1 +
 src/egl/main/eglsurface.c |  81 -
 src/egl/main/eglsurface.h |  59 +-
 src/gallium/state_trackers/dri/dri_screen.c   |   4 +-
 src/mesa/drivers/dri/common/dri_util.c|   2 +
 src/mesa/drivers/dri/common/dri_util.h|   4 +
 src/mesa/drivers/dri/common/utils.c   |  10 +-
 src/mesa/drivers/dri/common/utils.h   |   3 +-
 src/mesa/drivers/dri/i915/intel_screen.c  |   4 +-
 src/mesa/drivers/dri/i965/brw_context.c   |  86 -
 src/mesa/drivers/dri/i965/brw_context.h   |  12 ++
 src/mesa/drivers/dri/i965/intel_screen.c  |  17 +-
 src/mesa/drivers/dri/nouveau/nouveau_screen.c |   2 +-
 src/mesa/drivers/dri/radeon/radeon_screen.c   |   2 +-
 src/mesa/drivers/dri/swrast/swrast.c  |   2 +-
 src/mesa/main/mtypes.h|   3 +
 24 files changed, 664 insertions(+), 59 deletions(-)

-- 
2.18.0.345.g5c9ce644c3-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/7] egl, i965: Implement EGL_KHR_mutable_render_buffer

[Chad Versace]

Ignore this series. It's incomplete. It seems mesa-dev had hiccups last
night.

I'll resend now.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 7/7] i965: Implement EGL_KHR_mutable_render_buffer

[Chad Versace]

Tested with a low-latency handwriting application on Android Nougat on
the Chrome OS Pixelbook (codename Eve) with Kabylake.
---
 src/mesa/drivers/dri/i965/brw_context.c  | 86 +++-
 src/mesa/drivers/dri/i965/brw_context.h  | 12 
 src/mesa/drivers/dri/i965/intel_screen.c | 13 +++-
 3 files changed, 107 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index 968fc1d43d6..9dfd9520555 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -239,6 +239,35 @@ intel_flush_front(struct gl_context *ctx)
}
 }
 
+static void
+brw_display_shared_buffer(struct brw_context *brw)
+{
+   __DRIcontext *dri_context = brw->driContext;
+   __DRIdrawable *dri_drawable = dri_context->driDrawablePriv;
+   __DRIscreen *dri_screen = brw->screen->driScrnPriv;
+   int fence_fd = -1;
+
+   if (!brw->is_shared_buffer_bound)
+  return;
+
+   if (!brw->is_shared_buffer_dirty)
+  return;
+
+   if (brw->screen->has_exec_fence) {
+  /* This function is always called during a flush operation, so there is
+   * no need to flush again here. But we want to provide a fence_fd to the
+   * loader, and a redundant flush is the easiest way to acquire one.
+   */
+  if (intel_batchbuffer_flush_fence(brw, -1, _fd))
+ return;
+   }
+
+   dri_screen->mutableRenderBuffer.loader
+  ->displaySharedBuffer(dri_drawable, fence_fd,
+dri_drawable->loaderPrivate);
+   brw->is_shared_buffer_dirty = false;
+}
+
 static void
 intel_glFlush(struct gl_context *ctx)
 {
@@ -246,7 +275,7 @@ intel_glFlush(struct gl_context *ctx)
 
intel_batchbuffer_flush(brw);
intel_flush_front(ctx);
-
+   brw_display_shared_buffer(brw);
brw->need_flush_throttle = true;
 }
 
@@ -1457,6 +1486,11 @@ intel_prepare_render(struct brw_context *brw)
 */
if (_mesa_is_front_buffer_drawing(ctx->DrawBuffer))
   brw->front_buffer_dirty = true;
+
+   if (brw->is_shared_buffer_bound) {
+  /* Subsequent rendering will probably dirty the shared buffer. */
+  brw->is_shared_buffer_dirty = true;
+   }
 }
 
 /**
@@ -1690,8 +1724,12 @@ intel_update_image_buffer(struct brw_context *intel,
else
   last_mt = rb->singlesample_mt;
 
-   if (last_mt && last_mt->bo == buffer->bo)
+   if (last_mt && last_mt->bo == buffer->bo) {
+  if (buffer_type == __DRI_IMAGE_BUFFER_SHARED) {
+ intel_miptree_make_shareable(intel, last_mt);
+  }
   return;
+   }
 
/* Only allow internal compression if samples == 0.  For multisampled
 * window system buffers, the only thing the single-sampled buffer is used
@@ -1720,6 +1758,35 @@ intel_update_image_buffer(struct brw_context *intel,
rb->Base.Base.NumSamples > 1) {
   intel_renderbuffer_upsample(intel, rb);
}
+
+   if (buffer_type == __DRI_IMAGE_BUFFER_SHARED) {
+  /* The compositor and the application may access this image
+   * concurrently. The display hardware may even scanout the image while
+   * the GPU is rendering to it.  Aux surfaces cause difficulty with
+   * concurrent access, so permanently disable aux for this miptree.
+   *
+   * Perhaps we could improve overall application performance by
+   * re-enabling the aux surface when EGL_RENDER_BUFFER transitions to
+   * EGL_BACK_BUFFER, then disabling it again when EGL_RENDER_BUFFER
+   * returns to EGL_SINGLE_BUFFER. I expect the wins and losses with this
+   * approach to be highly dependent on the application's GL usage.
+   *
+   * I [chadv] expect clever disabling/reenabling to be counterproductive
+   * in the use cases I care about: applications that render nearly
+   * realtime handwriting to the surface while possibly undergiong
+   * simultaneously scanout as a display plane. The app requires low
+   * render latency. Even though the app spends most of its time in
+   * shared-buffer mode, it also frequently transitions between
+   * shared-buffer (EGL_SINGLE_BUFFER) and double-buffer (EGL_BACK_BUFFER)
+   * mode.  Visual sutter during the transitions should be avoided.
+   *
+   * In this case, I [chadv] believe reducing the GPU workload at
+   * shared-buffer/double-buffer transitions would offer a smoother app
+   * experience than any savings due to aux compression. But I've
+   * collected no data to prove my theory.
+   */
+  intel_miptree_make_shareable(intel, mt);
+   }
 }
 
 static void
@@ -1780,4 +1847,19 @@ intel_update_image_buffers(struct brw_context *brw, 
__DRIdrawable *drawable)
 images.back,
 __DRI_IMAGE_BUFFER_BACK);
}
+
+   if (images.image_mask & __DRI_IMAGE_BUFFER_SHARED) {
+  assert(images.image_mask == __DRI_IMAGE_BUFFER_SHARED);
+  drawable->w = images.back->width;
+  drawable->h = images.back->height;
+  

[Mesa-dev] [PATCH] drisw: Fix build on Android Nougat, which lacks shm (v2)

[Chad Versace]

In commit cf54bd5e8, dri_sw_winsys.c began using  to support
the new functions putImageShm, getImageShm in DRI_SWRastLoader. But
Android began supporting System V shared memory only in Oreo. Nougat has
no shm headers.

Fix the build by ifdef'ing out the shm code on Nougat.

Fixes: cf54bd5e8 "drisw: use shared memory when possible"
Cc: Marc-André Lureau 
Cc: Dave Airlie 
---

Dave, is this what you had in mind?

I considered putting a check in configure.ac for HAVE_SHM, but decided
against it because (1) another Autoconf check would slow down everyone's
build for a niche case that only a few people care about, and (2) it
only affected this file.

 src/gallium/winsys/sw/dri/dri_sw_winsys.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/src/gallium/winsys/sw/dri/dri_sw_winsys.c 
b/src/gallium/winsys/sw/dri/dri_sw_winsys.c
index 40007200a5d..d519bcfedd3 100644
--- a/src/gallium/winsys/sw/dri/dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/dri/dri_sw_winsys.c
@@ -26,8 +26,12 @@
  *
  **/
 
+#if !defined(ANDROID) || ANDROID_API_LEVEL >= 26
+/* Android's libc began supporting shm in Oreo */
+#define HAVE_SHM
 #include 
 #include 
+#endif
 
 #include "pipe/p_compiler.h"
 #include "pipe/p_format.h"
@@ -83,6 +87,7 @@ dri_sw_is_displaytarget_format_supported( struct sw_winsys 
*ws,
return TRUE;
 }
 
+#ifdef HAVE_SHM
 static char *
 alloc_shm(struct dri_sw_displaytarget *dri_sw_dt, unsigned size)
 {
@@ -101,6 +106,7 @@ alloc_shm(struct dri_sw_displaytarget *dri_sw_dt, unsigned 
size)
 
return addr;
 }
+#endif
 
 static struct sw_displaytarget *
 dri_sw_displaytarget_create(struct sw_winsys *winsys,
@@ -131,8 +137,11 @@ dri_sw_displaytarget_create(struct sw_winsys *winsys,
size = dri_sw_dt->stride * nblocksy;
 
dri_sw_dt->shmid = -1;
+
+#ifdef HAVE_SHM
if (ws->lf->put_image_shm)
   dri_sw_dt->data = alloc_shm(dri_sw_dt, size);
+#endif
 
if(!dri_sw_dt->data)
   dri_sw_dt->data = align_malloc(size, alignment);
@@ -156,8 +165,10 @@ dri_sw_displaytarget_destroy(struct sw_winsys *ws,
struct dri_sw_displaytarget *dri_sw_dt = dri_sw_displaytarget(dt);
 
if (dri_sw_dt->shmid >= 0) {
+#ifdef HAVE_SHM
   shmdt(dri_sw_dt->data);
   shmctl(dri_sw_dt->shmid, IPC_RMID, 0);
+#endif
} else {
   align_free(dri_sw_dt->data);
}
-- 
2.18.0.345.g5c9ce644c3-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 3/7] dri: Define DRI_MutableRenderBuffer extensions

[Chad Versace]

Define extensions DRI_MutableRenderBufferDriver and
DRI_MutableRenderBufferLoader. These are the two halves for
EGL_KHR_mutable_render_buffer.

Outside the DRI code there is one additional change.  Add
gl_config::mutableRenderBuffer to match
__DRI_ATTRIB_MUTABLE_RENDER_BUFFER. Neither are used yet.
---
 include/GL/internal/dri_interface.h| 138 -
 src/mesa/drivers/dri/common/dri_util.c |   2 +
 src/mesa/drivers/dri/common/dri_util.h |   4 +
 src/mesa/drivers/dri/common/utils.c|   1 +
 src/mesa/main/mtypes.h |   3 +
 5 files changed, 145 insertions(+), 3 deletions(-)

diff --git a/include/GL/internal/dri_interface.h 
b/include/GL/internal/dri_interface.h
index c32cdd3767a..245c845832b 100644
--- a/include/GL/internal/dri_interface.h
+++ b/include/GL/internal/dri_interface.h
@@ -48,6 +48,7 @@ typedef unsigned int drm_drawable_t;
 typedef struct drm_clip_rect drm_clip_rect_t;
 #endif
 
+#include 
 #include 
 
 /**
@@ -746,7 +747,8 @@ struct __DRIuseInvalidateExtensionRec {
 #define __DRI_ATTRIB_BIND_TO_TEXTURE_TARGETS   46
 #define __DRI_ATTRIB_YINVERTED 47
 #define __DRI_ATTRIB_FRAMEBUFFER_SRGB_CAPABLE  48
-#define __DRI_ATTRIB_MAX   
(__DRI_ATTRIB_FRAMEBUFFER_SRGB_CAPABLE + 1)
+#define __DRI_ATTRIB_MUTABLE_RENDER_BUFFER 49 /* 
EGL_MUTABLE_RENDER_BUFFER_BIT_KHR */
+#define __DRI_ATTRIB_MAX   50
 
 /* __DRI_ATTRIB_RENDER_TYPE */
 #define __DRI_ATTRIB_RGBA_BIT  0x01
@@ -1888,9 +1890,57 @@ struct __DRI2rendererQueryExtensionRec {
  * Image Loader extension. Drivers use this to allocate color buffers
  */
 
+/**
+ * See __DRIimageLoaderExtensionRec::getBuffers::buffer_mask.
+ */
 enum __DRIimageBufferMask {
__DRI_IMAGE_BUFFER_BACK = (1 << 0),
-   __DRI_IMAGE_BUFFER_FRONT = (1 << 1)
+   __DRI_IMAGE_BUFFER_FRONT = (1 << 1),
+
+   /**
+* A buffer shared between application and compositor. The buffer may be
+* simultaneously accessed by each.
+*
+* A shared buffer is equivalent to an EGLSurface whose EGLConfig contains
+* EGL_MUTABLE_RENDER_BUFFER_BIT_KHR and whose active EGL_RENDER_BUFFER (as
+* opposed to any pending, requested change to EGL_RENDER_BUFFER) is
+* EGL_SINGLE_BUFFER.
+*
+* If buffer_mask contains __DRI_IMAGE_BUFFER_SHARED, then must contains no
+* other bits. As a corollary, a __DRIdrawable that has a "shared" buffer
+* has no front nor back buffer.
+*
+* The loader returns __DRI_IMAGE_BUFFER_SHARED in buffer_mask if and only
+* if:
+* - The loader supports __DRI_MUTABLE_RENDER_BUFFER_LOADER.
+* - The driver supports __DRI_MUTABLE_RENDER_BUFFER_DRIVER.
+* - The EGLConfig of the drawable EGLSurface contains
+*   EGL_MUTABLE_RENDER_BUFFER_BIT_KHR.
+* - The EGLContext's EGL_RENDER_BUFFER is EGL_SINGLE_BUFFER.
+*   Equivalently, the EGLSurface's active EGL_RENDER_BUFFER (as
+*   opposed to any pending, requested change to EGL_RENDER_BUFFER) is
+*   EGL_SINGLE_BUFFER. (See the EGL 1.5 and
+*   EGL_KHR_mutable_render_buffer spec for details about "pending" vs
+*   "active" EGL_RENDER_BUFFER state).
+*
+* A shared buffer is similar to a front buffer in that all rendering to the
+* buffer should appear promptly on the screen. It is different from
+* a front buffer in that its behavior is independent from the
+* GL_DRAW_BUFFER state. Specifically, if GL_DRAW_FRAMEBUFFER is 0 and the
+* __DRIdrawable's buffer_mask is __DRI_IMAGE_BUFFER_SHARED, then all
+* rendering should appear promptly on the screen if GL_DRAW_BUFFER is not
+* GL_NONE.
+*
+* The difference between a shared buffer and a front buffer is motivated
+* by the constraints of Android and OpenGL ES. OpenGL ES does not support
+* front-buffer rendering. Android's SurfaceFlinger protocol provides the
+* EGL driver only a back buffer and no front buffer. The shared buffer
+* mode introduced by EGL_KHR_mutable_render_buffer is a backdoor though
+* EGL that allows Android OpenGL ES applications to render to what is
+* effectively the front buffer, a backdoor that required no change to the
+* OpenGL ES API and little change to the SurfaceFlinger API.
+*/
+   __DRI_IMAGE_BUFFER_SHARED = (1 << 2),
 };
 
 struct __DRIimageList {
@@ -1915,7 +1965,8 @@ struct __DRIimageLoaderExtensionRec {
 * \param stamp  Address of variable to be updated when
 *   getBuffers must be called again
 * \param loaderPrivate  The loaderPrivate for driDrawable
-* \param buffer_maskSet of buffers to allocate
+* \param buffer_maskSet of buffers to allocate. A bitmask of
+*   __DRIimageBufferMask.
 * \param buffersReturned buffers
 */
int (*getBuffers)(__DRIdrawable *driDrawable,
@@ -2029,4 +2080,85 @@ struct 

[Mesa-dev] [PATCH 0/7] egl, i965: Implement EGL_KHR_mutable_render_buffer

[Chad Versace]

Despite the KHR, Android is the only OS that uses this extension, as far
as I know. Essentially, it allows the app to toggle
front-buffer/back-buffer rendering using EGL.

Android requires this extension if the device advertises support for
virtual reality. No Chrome OS device yet supports Android's VR software
requirements, though, and VR is not why I implemented this.

This extension enables handwriting and drawing apps on Chrome and
Android devices to achieve very low latency between touch input
and display response.

Please don't bash me for this oddly-specified Android extension.
I didn't create it. I'm just writing the code that Android requires.

* Patch 3 defines a pair of new DRI extensions, 
DRI_MutableRenderBuffer{Driver,Loader}.
* Patches 1-5 add the generic code for the DRI and EGL extensions.
* Patch 6 implements DRI_MutableRenderBufferLoader for Android EGL.
* Patch 7 implements DRI_MutableRenderBufferDriver for i965.

This series is tagged at:

http://git.kiwitree.net/cgit/~chadv/mesa/log/?h=refs/tags/chadv/review/i965-mutable-render-buffer-v01

Chad Versace (7):
  egl: Simplify queries for EGL_RENDER_BUFFER
  egl/dri2: In dri2_make_current, return early on failure
  dri: Define DRI_MutableRenderBuffer extensions
  dri: Add param driCreateConfigs(mutable_render_buffer)
  egl/main: Add bits for EGL_KHR_mutable_render_buffer
  egl/android: Implement EGL_KHR_mutable_render_buffer
  i965: Implement EGL_KHR_mutable_render_buffer

 include/GL/internal/dri_interface.h   | 138 +-
 src/egl/drivers/dri2/egl_dri2.c   |  71 +---
 src/egl/drivers/dri2/egl_dri2.h   |   7 +
 src/egl/drivers/dri2/platform_android.c   | 168 +-
 src/egl/main/eglapi.c |   1 +
 src/egl/main/eglconfig.c  |   3 +
 src/egl/main/eglcontext.c |  40 -
 src/egl/main/eglcontext.h |   3 -
 src/egl/main/egldisplay.h |   1 +
 src/egl/main/eglsurface.c |  81 -
 src/egl/main/eglsurface.h |  59 +-
 src/gallium/state_trackers/dri/dri_screen.c   |   4 +-
 src/mesa/drivers/dri/common/dri_util.c|   2 +
 src/mesa/drivers/dri/common/dri_util.h|   4 +
 src/mesa/drivers/dri/common/utils.c   |  10 +-
 src/mesa/drivers/dri/common/utils.h   |   3 +-
 src/mesa/drivers/dri/i915/intel_screen.c  |   4 +-
 src/mesa/drivers/dri/i965/brw_context.c   |  86 -
 src/mesa/drivers/dri/i965/brw_context.h   |  12 ++
 src/mesa/drivers/dri/i965/intel_screen.c  |  17 +-
 src/mesa/drivers/dri/nouveau/nouveau_screen.c |   2 +-
 src/mesa/drivers/dri/radeon/radeon_screen.c   |   2 +-
 src/mesa/drivers/dri/swrast/swrast.c  |   2 +-
 src/mesa/main/mtypes.h|   3 +
 24 files changed, 664 insertions(+), 59 deletions(-)

-- 
2.18.0.345.g5c9ce644c3-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/7] egl: Simplify queries for EGL_RENDER_BUFFER

[Chad Versace]

There exist *two* queryable EGL_RENDER_BUFFER states in EGL:
eglQuerySurface(EGL_RENDER_BUFFER) and
eglQueryContext(EGL_RENDER_BUFFER).

These changes eliminate potentially very fragile code in the upcoming
EGL_KHR_mutable_render_buffer implementation.

* eglQuerySurface(EGL_RENDER_BUFFER)

  The implementation of eglQuerySurface(EGL_RENDER_BUFFER) contained
  abstruse logic which required comprehending the specification
  complexities of how the two EGL_RENDER_BUFFER states interact.  The
  function sometimes returned _EGLContext::WindowRenderBuffer, sometimes
  _EGLSurface::RenderBuffer. Why? The function tried to encode the
  actual logic from the EGL spec. When did the function return which
  variable? Go study the EGL spec, hope you understand it, then hope
  Mesa mutated the EGL_RENDER_BUFFER state in all the correct places.
  Have fun.

  To simplify eglQuerySurface(EGL_RENDER_BUFFER), and to improve
  confidence in its correctness, flatten its indirect logic. For pixmap
  and pbuffer surfaces, simply return a hard-coded literal value, as the
  spec suggests. For window surfaces, simply return
  _EGLSurface::RequestedRenderBuffer.  Nothing difficult here.

* eglQueryContext(EGL_RENDER_BUFFER)

  The implementation of this suffered from the same issues as
  eglQuerySurface, and the solution is the same.  confidence in its
  correctness, flatten its indirect logic. For pixmap and pbuffer
  surfaces, simply return a hard-coded literal value, as the spec
  suggests. For window surfaces, simply return
  _EGLSurface::ActiveRenderBuffer.
---
 src/egl/drivers/dri2/egl_dri2.c |  6 -
 src/egl/main/eglcontext.c   | 40 +++--
 src/egl/main/eglcontext.h   |  3 ---
 src/egl/main/eglsurface.c   | 28 ---
 src/egl/main/eglsurface.h   | 28 ++-
 5 files changed, 85 insertions(+), 20 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index c3024795a10..e109ad37f55 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1289,12 +1289,6 @@ dri2_create_context(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLConfig *conf,
  dri_config = dri2_config->dri_config[1][0];
   else
  dri_config = dri2_config->dri_config[0][0];
-
-  /* EGL_WINDOW_BIT is set only when there is a double-buffered dri_config.
-   * This makes sure the back buffer will always be used.
-   */
-  if (conf->SurfaceType & EGL_WINDOW_BIT)
- dri2_ctx->base.WindowRenderBuffer = EGL_BACK_BUFFER;
}
else
   dri_config = NULL;
diff --git a/src/egl/main/eglcontext.c b/src/egl/main/eglcontext.c
index 3e5d8ad97bf..ecc546e1132 100644
--- a/src/egl/main/eglcontext.c
+++ b/src/egl/main/eglcontext.c
@@ -588,7 +588,6 @@ _eglInitContext(_EGLContext *ctx, _EGLDisplay *dpy, 
_EGLConfig *conf,
_eglInitResource(>Resource, sizeof(*ctx), dpy);
ctx->ClientAPI = api;
ctx->Config = conf;
-   ctx->WindowRenderBuffer = EGL_NONE;
ctx->Profile = EGL_CONTEXT_OPENGL_CORE_PROFILE_BIT_KHR;
 
ctx->ClientMajorVersion = 1; /* the default, per EGL spec */
@@ -620,15 +619,42 @@ static EGLint
 _eglQueryContextRenderBuffer(_EGLContext *ctx)
 {
_EGLSurface *surf = ctx->DrawSurface;
-   EGLint rb;
 
+   /* From the EGL 1.5 spec:
+*
+*- If the context is not bound to a surface, then EGL_NONE will be
+*  returned.
+*/
if (!surf)
   return EGL_NONE;
-   if (surf->Type == EGL_WINDOW_BIT && ctx->WindowRenderBuffer != EGL_NONE)
-  rb = ctx->WindowRenderBuffer;
-   else
-  rb = surf->RenderBuffer;
-   return rb;
+
+   switch (surf->Type) {
+   default:
+  unreachable("bad EGLSurface type");
+   case EGL_PIXMAP_BIT:
+  /* - If the context is bound to a pixmap surface, then EGL_SINGLE_BUFFER
+   *   will be returned.
+   */
+  return EGL_SINGLE_BUFFER;
+   case EGL_PBUFFER_BIT:
+  /* - If the context is bound to a pbuffer surface, then EGL_BACK_BUFFER
+   *   will be returned.
+   */
+  return EGL_BACK_BUFFER;
+   case EGL_WINDOW_BIT:
+  /* - If the context is bound to a window surface, then either
+   *   EGL_BACK_BUFFER or EGL_SINGLE_BUFFER may be returned. The value
+   *   returned depends on both the buffer requested by the setting of the
+   *   EGL_RENDER_BUFFER property of the surface [...], and on the client
+   *   API (not all client APIs support single-buffer Rendering to window
+   *   surfaces). Some client APIs allow control of whether rendering goes
+   *   to the front or back buffer. This client API-specific choice is not
+   *   reflected in the returned value, which only describes the buffer
+   *   that will be rendered to by default if not overridden by the client
+   *   API.
+   */
+  return surf->ActiveRenderBuffer;
+   }
 }
 
 
diff --git a/src/egl/main/eglcontext.h b/src/egl/main/eglcontext.h
index 8d97ef9eab9..deddefdcaba 

[Mesa-dev] [PATCH 2/7] egl/dri2: In dri2_make_current, return early on failure

[Chad Versace]

This pulls an 'else' block into the function's main body, making the
code easier to follow.

Without this change, the upcoming EGL_KHR_mutable_render_buffer patch
transforms dri2_make_current() into spaghetti.
---
 src/egl/drivers/dri2/egl_dri2.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index e109ad37f55..1208ebb3156 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1493,20 +1493,7 @@ 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)) {
-  dri2_destroy_surface(drv, disp, old_dsurf);
-  dri2_destroy_surface(drv, disp, old_rsurf);
-
-  if (!unbind)
- dri2_dpy->ref_count++;
-  if (old_ctx) {
- EGLDisplay old_disp = _eglGetDisplayHandle(old_ctx->Resource.Display);
- dri2_destroy_context(drv, disp, old_ctx);
- dri2_display_release(old_disp);
-  }
-
-  return EGL_TRUE;
-   } else {
+   if (!unbind && !dri2_dpy->core->bindContext(cctx, ddraw, rdraw)) {
   /* undo the previous _eglBindContext */
   _eglBindContext(old_ctx, old_dsurf, old_rsurf, , _dsurf, 
_rsurf);
   assert(_ctx->base == ctx &&
@@ -1527,6 +1514,20 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
*/
   return _eglError(EGL_BAD_MATCH, "eglMakeCurrent");
}
+
+   dri2_destroy_surface(drv, disp, old_dsurf);
+   dri2_destroy_surface(drv, disp, old_rsurf);
+
+   if (!unbind)
+  dri2_dpy->ref_count++;
+
+   if (old_ctx) {
+  EGLDisplay old_disp = _eglGetDisplayHandle(old_ctx->Resource.Display);
+  dri2_destroy_context(drv, disp, old_ctx);
+  dri2_display_release(old_disp);
+   }
+
+   return EGL_TRUE;
 }
 
 __DRIdrawable *
-- 
2.18.0.345.g5c9ce644c3-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 5/7] egl/main: Add bits for EGL_KHR_mutable_render_buffer

[Chad Versace]

A follow-up patch enables EGL_KHR_mutable_render_buffer for Android.
This patch is separate from the Android patch because I think it's
easier to review the platform-independent bits separately.
---
 src/egl/main/eglapi.c |  1 +
 src/egl/main/eglconfig.c  |  3 ++
 src/egl/main/egldisplay.h |  1 +
 src/egl/main/eglsurface.c | 59 +--
 src/egl/main/eglsurface.h | 33 +-
 5 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index deb479b6d56..f10ec994fde 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -509,6 +509,7 @@ _eglCreateExtensionsString(_EGLDisplay *dpy)
_EGL_CHECK_EXTENSION(KHR_image);
_EGL_CHECK_EXTENSION(KHR_image_base);
_EGL_CHECK_EXTENSION(KHR_image_pixmap);
+   _EGL_CHECK_EXTENSION(KHR_mutable_render_buffer);
_EGL_CHECK_EXTENSION(KHR_no_config_context);
_EGL_CHECK_EXTENSION(KHR_partial_update);
_EGL_CHECK_EXTENSION(KHR_reusable_sync);
diff --git a/src/egl/main/eglconfig.c b/src/egl/main/eglconfig.c
index 2d3b3ddd908..67d11ec9b0b 100644
--- a/src/egl/main/eglconfig.c
+++ b/src/egl/main/eglconfig.c
@@ -272,6 +272,7 @@ static const struct {
 EGLBoolean
 _eglValidateConfig(const _EGLConfig *conf, EGLBoolean for_matching)
 {
+   _EGLDisplay *disp = conf->Display;
EGLint i, attr, val;
EGLBoolean valid = EGL_TRUE;
 
@@ -340,6 +341,8 @@ _eglValidateConfig(const _EGLConfig *conf, EGLBoolean 
for_matching)
EGL_VG_ALPHA_FORMAT_PRE_BIT |
EGL_MULTISAMPLE_RESOLVE_BOX_BIT |
EGL_SWAP_BEHAVIOR_PRESERVED_BIT;
+if (disp->Extensions.KHR_mutable_render_buffer)
+   mask |= EGL_MUTABLE_RENDER_BUFFER_BIT_KHR;
 break;
  case EGL_RENDERABLE_TYPE:
  case EGL_CONFORMANT:
diff --git a/src/egl/main/egldisplay.h b/src/egl/main/egldisplay.h
index d7e51991a45..4a7f248e34c 100644
--- a/src/egl/main/egldisplay.h
+++ b/src/egl/main/egldisplay.h
@@ -127,6 +127,7 @@ struct _egl_extensions
EGLBoolean KHR_image;
EGLBoolean KHR_image_base;
EGLBoolean KHR_image_pixmap;
+   EGLBoolean KHR_mutable_render_buffer;
EGLBoolean KHR_no_config_context;
EGLBoolean KHR_partial_update;
EGLBoolean KHR_reusable_sync;
diff --git a/src/egl/main/eglsurface.c b/src/egl/main/eglsurface.c
index 73c31e11588..926b7ab569a 100644
--- a/src/egl/main/eglsurface.c
+++ b/src/egl/main/eglsurface.c
@@ -123,6 +123,12 @@ _eglParseSurfaceAttribList(_EGLSurface *surf, const EGLint 
*attrib_list)
 break;
  }
  surf->RequestedRenderBuffer = val;
+ if (surf->Config->SurfaceType & EGL_MUTABLE_RENDER_BUFFER_BIT_KHR) {
+/* Unlike normal EGLSurfaces, one with a mutable render buffer
+ * uses the application-chosen render buffer.
+ */
+surf->ActiveRenderBuffer = val;
+ }
  break;
   case EGL_POST_SUB_BUFFER_SUPPORTED_NV:
  if (!dpy->Extensions.NV_post_sub_buffer ||
@@ -359,12 +365,19 @@ _eglQuerySurface(_EGLDriver *drv, _EGLDisplay *dpy, 
_EGLSurface *surface,
   *value = surface->SwapBehavior;
   break;
case EGL_RENDER_BUFFER:
-  /* From the EGL 1.5 spec (2014.08.27):
+  /* From the EGL_KHR_mutable_render_buffer spec (v12):
*
*Querying EGL_RENDER_BUFFER returns the buffer which client API
*rendering is requested to use. For a window surface, this is the
-   *same attribute value specified when the surface was created. For
-   *a pbuffer surface, it is always EGL_BACK_BUFFER . For a pixmap
+   *attribute value specified when the surface was created or last set
+   *via eglSurfaceAttrib.
+   *
+   * In other words, querying a window surface returns the value most
+   * recently *requested* by the user.
+   *
+   * The paragraph continues in the EGL 1.5 spec (2014.08.27):
+   *
+   *For a pbuffer surface, it is always EGL_BACK_BUFFER . For a pixmap
*surface, it is always EGL_SINGLE_BUFFER . To determine the actual
*buffer being rendered to by a context, call eglQueryContext.
*/
@@ -472,6 +485,31 @@ _eglSurfaceAttrib(_EGLDriver *drv, _EGLDisplay *dpy, 
_EGLSurface *surface,
  break;
   surface->MultisampleResolve = value;
   break;
+   case EGL_RENDER_BUFFER:
+  if (!dpy->Extensions.KHR_mutable_render_buffer) {
+ err = EGL_BAD_ATTRIBUTE;
+ break;
+  }
+
+  if (value != EGL_BACK_BUFFER && value != EGL_SINGLE_BUFFER) {
+ err = EGL_BAD_PARAMETER;
+ break;
+  }
+
+  /* From the EGL_KHR_mutable_render_buffer spec (v12):
+   *
+   *If attribute is EGL_RENDER_BUFFER, and the EGL_SURFACE_TYPE
+   *attribute of the EGLConfig used to create surface does not contain
+   *EGL_MUTABLE_RENDER_BUFFER_BIT_KHR, [...] an EGL_BAD_MATCH error is
+   * 

Re: [Mesa-dev] [PATCH mesa] anv: don't crash on vkDestroyDevice(NULL)

[Chad Versace]

On Thu 26 Jul 2018, Eric Engestrom wrote:
> On Thursday, 2018-07-26 02:03:58 -0700, Jason Ekstrand wrote:
> > On Thu, Jul 26, 2018 at 1:50 AM Eric Engestrom 
> > wrote:
> > 
> > > On Wednesday, 2018-07-25 14:00:29 -0700, Dylan Baker wrote:
> > > > Quoting Eric Engestrom (2018-07-25 11:45:56)
> > > > > CovID: 1438132
> > > > > Signed-off-by: Eric Engestrom 
> > > > > ---
> > > > >  src/intel/vulkan/anv_device.c | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/intel/vulkan/anv_device.c
> > > b/src/intel/vulkan/anv_device.c
> > > > > index 04fd6a829ed60081abc4..3664f80c24dc34955196 100644
> > > > > --- a/src/intel/vulkan/anv_device.c
> > > > > +++ b/src/intel/vulkan/anv_device.c
> > > > > @@ -1832,11 +1832,13 @@ void anv_DestroyDevice(
> > > > >  const VkAllocationCallbacks*pAllocator)
> > > > >  {
> > > > > ANV_FROM_HANDLE(anv_device, device, _device);
> > > > > -   struct anv_physical_device *physical_device =
> > > >instance->physicalDevice;
> > > > > +   struct anv_physical_device *physical_device;
> > > >
> > > > Is there a particular reason to create the pointer her but assign it
> > > after the
> > > > null check rather than just move the null check between the
> > > ANV_FROM_HANDLE and
> > > > the anv_pysical_device?
> > >
> > > Just the habit of always putting variable declarations before any logic
> > > ¯\_(ツ)_/¯
> > > I thought that was considered best-practice; has that changed?
> > >
> > 
> > Yup; welcome to C99. :-)
> 
> Haha, I know it's now allowed, but I thought best practice was to still
> keep things grouped, except when there's a reason to have a declaration
> in the middle of the logic.
> 
> Looking at the rest of the file, I see a bunch of examples of logic and
> declarations interleaved, so I guess that rule doesn't apply in anv at
> least; guess I'll change my style to match :)

Even though we live in the future (whoo! 1999!), there are two cases
that I'm aware of that still require declaration-before-assignment.
Neither applies to this patch, though.

  * A goto may force the divoce of declaration and assignment. For
example,

void foo(void) {
if (bad_func_fails())
goto fail;

int fd = open(...);

...;
return;

  fail:
/* compiler warns of comparison of uninitialized value */
if (fd != -1)
   close(fd);
  }

  * The same issue can happen when using __attribute__((cleanup(...))).
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] anv: add more swapchain formats

[Chad Versace]

On Wed 25 Jul 2018, Tapani Pälli wrote:
> This change helps with some of the dEQP-VK.wsi.android.* tests that
> try to create swapchain with using such formats.
> 
> Signed-off-by: Tapani Pälli 
> ---
>  src/intel/vulkan/anv_android.c | 16 +++-
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 

Reviewed-by: Chad Versace 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] xlib: fix build break from _swrast_map_soft_renderbuffer() call

[Chad Versace]

On Fri 27 Jul 2018, Brian Paul wrote:
> We need to pass the new flip_y argument.
> ---
>  src/mesa/drivers/x11/xm_buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks Brian.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] i965: implement MESA_framebuffer_flip_y [v3]

[Chad Versace]

On Mon 23 Jul 2018, Fritz Koenig wrote:
> Instead of using _mesa_is_winsys_fbo or
> _mesa_is_user_fbo to infer if an fbo is
> flipped use the InvertedY flag.
> 
> v2:
> * additional window-system framebuffer checks [for jason]
> v3:
> * s/inverted_y/flip_y/g [for chadv]
> * s/InvertedY/FlipY/g [for chadv]
> ---
>  src/mesa/drivers/dri/i965/brw_blorp.c |  2 +-
>  src/mesa/drivers/dri/i965/brw_meta_util.c |  4 +-
>  src/mesa/drivers/dri/i965/brw_sf.c|  6 +--
>  src/mesa/drivers/dri/i965/genX_state_upload.c | 50 +--
>  src/mesa/drivers/dri/i965/intel_extensions.c  |  1 +
>  src/mesa/drivers/dri/i965/intel_fbo.c | 11 ++--
>  .../drivers/dri/i965/intel_pixel_bitmap.c |  8 +--
>  src/mesa/drivers/dri/i965/intel_pixel_copy.c  |  4 +-
>  src/mesa/drivers/dri/i965/intel_pixel_draw.c  |  2 +-
>  9 files changed, 43 insertions(+), 45 deletions(-)

For the series,
Reviewed-by: Chad Versace 

I'll push soon.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/auxiliary: Fix Autotools on Android (v2)

[Chad Versace]

On Wed 25 Jul 2018, Tomasz Figa wrote:
> Hi Chad,
> 
> On Wed, Jul 25, 2018 at 10:11 AM Chad Versace  
> wrote:
> >
> > Problem 1: u_debug_stack_android.cpp transitively included
> > "pipe/p_compiler.h", but src/gallium/include was missing from the C++
> > include path.
> >
> > Problem 2: Add -std=c++11 to AM_CXXFLAGS. Android's libbacktrace headers
> > require C++11, but the Android toolchain (at least in the Chrome OS SDK)
> > does not enable C++11 by default.
> >
> > v2: Add -std=c++11.
> >
> > Cc: Gurchetan Singh 
> > Cc: Eric Engestrom 
> > ---
> >  src/gallium/auxiliary/Makefile.am | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)

[snip]

> >  if HAVE_PLATFORM_ANDROID
> > +# Android's libbacktrace headers required C++11, but the Android toolchain 
> > (at
> > +# least in the Chrome OS SDK) does not enable C++11 by default.
> > +AM_CXXFLAGS += $(CXX11_CXXFLAGS)
> > +
> 
> This is something that would normally be handled by the .pc file for
> given library. Package build system shouldn't be polluted with such
> system-specific low level dependencies.

Normally, I would agree. "If libbacktrace needs a CXXFLAG, then put in
the pc file". That's reasonable for most flags, because most flags are
*additive*. But the -std flag is not. If backtrace.pc added '-std=c++11'
to CXXFLAGS, but a different package required c++14, how should Mesa
resolve that conflict?

As precedent, I searched all pc files on my Debian machine. The only pc
files that add -std to CFLAGS or CXXFLAGS are the icu-*.pc files. And
those files should serve as anti-precedent in most cases; they are badly
written compared to all other pc files I've seen.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] gallium/auxiliary: Fix Autotools on Android (v2)

[Chad Versace]

Problem 1: u_debug_stack_android.cpp transitively included
"pipe/p_compiler.h", but src/gallium/include was missing from the C++
include path.

Problem 2: Add -std=c++11 to AM_CXXFLAGS. Android's libbacktrace headers
require C++11, but the Android toolchain (at least in the Chrome OS SDK)
does not enable C++11 by default.

v2: Add -std=c++11.

Cc: Gurchetan Singh 
Cc: Eric Engestrom 
---
 src/gallium/auxiliary/Makefile.am | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/gallium/auxiliary/Makefile.am 
b/src/gallium/auxiliary/Makefile.am
index 03908198772..4bfa7648389 100644
--- a/src/gallium/auxiliary/Makefile.am
+++ b/src/gallium/auxiliary/Makefile.am
@@ -13,6 +13,7 @@ AM_CFLAGS = \
$(MSVC2013_COMPAT_CFLAGS)
 
 AM_CXXFLAGS = \
+   $(GALLIUM_CFLAGS) \
$(VISIBILITY_CXXFLAGS) \
$(MSVC2013_COMPAT_CXXFLAGS)
 
@@ -22,6 +23,10 @@ libgallium_la_SOURCES = \
$(GENERATED_SOURCES)
 
 if HAVE_PLATFORM_ANDROID
+# Android's libbacktrace headers required C++11, but the Android toolchain (at
+# least in the Chrome OS SDK) does not enable C++11 by default.
+AM_CXXFLAGS += $(CXX11_CXXFLAGS)
+
 libgallium_la_SOURCES += util/u_debug_stack_android.cpp
 endif
 
@@ -41,7 +46,6 @@ AM_CFLAGS += \
$(LLVM_CFLAGS)
 
 AM_CXXFLAGS += \
-   $(GALLIUM_CFLAGS) \
$(LLVM_CXXFLAGS)
 
 libgallium_la_SOURCES += \
-- 
2.18.0.233.g985f88cf7e-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] gallium/auxiliary: Fix Autotools on Android

[Chad Versace]

u_debug_stack_android.cpp transitively included "pipe/p_compiler.h", but
src/gallium/include was missing from the C++ include path.

Cc: Gurchetan Singh 
Cc: Eric Engestrom 
---
 src/gallium/auxiliary/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gallium/auxiliary/Makefile.am 
b/src/gallium/auxiliary/Makefile.am
index 03908198772..81dfc331df1 100644
--- a/src/gallium/auxiliary/Makefile.am
+++ b/src/gallium/auxiliary/Makefile.am
@@ -13,6 +13,7 @@ AM_CFLAGS = \
$(MSVC2013_COMPAT_CFLAGS)
 
 AM_CXXFLAGS = \
+   $(GALLIUM_CFLAGS) \
$(VISIBILITY_CXXFLAGS) \
$(MSVC2013_COMPAT_CXXFLAGS)
 
@@ -41,7 +42,6 @@ AM_CFLAGS += \
$(LLVM_CFLAGS)
 
 AM_CXXFLAGS += \
-   $(GALLIUM_CFLAGS) \
$(LLVM_CXXFLAGS)
 
 libgallium_la_SOURCES += \
-- 
2.18.0.233.g985f88cf7e-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] drisw: Fix build on Android Nougat

[Chad Versace]

ping for review

On Wed 18 Jul 2018, Chad Versace wrote:
> In commit cf54bd5e8, dri_sw_winsys.c began using  to support
> the new functions putImageShm, getImageShm in DRI_SWRastLoader. But
> Android began supporting System V shared memory only in Oreo. Nougat has
> no shm headers.
> 
> Fix the build by ifdef'ing out the shm code on Nougat.
> 
> Fixes: cf54bd5e8 "drisw: use shared memory when possible"
> Cc: Marc-André Lureau  Cc: Dave Airlie 
> ---
>  src/gallium/winsys/sw/dri/dri_sw_winsys.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/gallium/winsys/sw/dri/dri_sw_winsys.c 
> b/src/gallium/winsys/sw/dri/dri_sw_winsys.c
> index 40007200a5d..147e0f7c430 100644
> --- a/src/gallium/winsys/sw/dri/dri_sw_winsys.c
> +++ b/src/gallium/winsys/sw/dri/dri_sw_winsys.c
> @@ -26,8 +26,10 @@
>   *
>   **/
>  
> +#if !defined(ANDROID) || ANDROID_API_LEVEL >= 26
>  #include 
>  #include 
> +#endif
>  
>  #include "pipe/p_compiler.h"
>  #include "pipe/p_format.h"
> @@ -83,6 +85,7 @@ dri_sw_is_displaytarget_format_supported( struct sw_winsys 
> *ws,
> return TRUE;
>  }
>  
> +#if !defined(ANDROID) || ANDROID_API_LEVEL >= 26
>  static char *
>  alloc_shm(struct dri_sw_displaytarget *dri_sw_dt, unsigned size)
>  {
> @@ -101,6 +104,7 @@ alloc_shm(struct dri_sw_displaytarget *dri_sw_dt, 
> unsigned size)
>  
> return addr;
>  }
> +#endif
>  
>  static struct sw_displaytarget *
>  dri_sw_displaytarget_create(struct sw_winsys *winsys,
> @@ -131,8 +135,11 @@ dri_sw_displaytarget_create(struct sw_winsys *winsys,
> size = dri_sw_dt->stride * nblocksy;
>  
> dri_sw_dt->shmid = -1;
> +
> +#if !defined(ANDROID) || ANDROID_API_LEVEL >= 26
> if (ws->lf->put_image_shm)
>dri_sw_dt->data = alloc_shm(dri_sw_dt, size);
> +#endif
>  
> if(!dri_sw_dt->data)
>dri_sw_dt->data = align_malloc(size, alignment);
> @@ -156,8 +163,10 @@ dri_sw_displaytarget_destroy(struct sw_winsys *ws,
> struct dri_sw_displaytarget *dri_sw_dt = dri_sw_displaytarget(dt);
>  
> if (dri_sw_dt->shmid >= 0) {
> +#if !defined(ANDROID) || ANDROID_API_LEVEL >= 26
>shmdt(dri_sw_dt->data);
>shmctl(dri_sw_dt->shmid, IPC_RMID, 0);
> +#endif
> } else {
>align_free(dri_sw_dt->data);
> }
> -- 
> 2.18.0.233.g985f88cf7e-goog
> 
> ___
> 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] drisw: Fix build on Android Nougat

[Chad Versace]

In commit cf54bd5e8, dri_sw_winsys.c began using  to support
the new functions putImageShm, getImageShm in DRI_SWRastLoader. But
Android began supporting System V shared memory only in Oreo. Nougat has
no shm headers.

Fix the build by ifdef'ing out the shm code on Nougat.

Fixes: cf54bd5e8 "drisw: use shared memory when possible"
Cc: Marc-André Lureau 
---
 src/gallium/winsys/sw/dri/dri_sw_winsys.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/gallium/winsys/sw/dri/dri_sw_winsys.c 
b/src/gallium/winsys/sw/dri/dri_sw_winsys.c
index 40007200a5d..147e0f7c430 100644
--- a/src/gallium/winsys/sw/dri/dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/dri/dri_sw_winsys.c
@@ -26,8 +26,10 @@
  *
  **/
 
+#if !defined(ANDROID) || ANDROID_API_LEVEL >= 26
 #include 
 #include 
+#endif
 
 #include "pipe/p_compiler.h"
 #include "pipe/p_format.h"
@@ -83,6 +85,7 @@ dri_sw_is_displaytarget_format_supported( struct sw_winsys 
*ws,
return TRUE;
 }
 
+#if !defined(ANDROID) || ANDROID_API_LEVEL >= 26
 static char *
 alloc_shm(struct dri_sw_displaytarget *dri_sw_dt, unsigned size)
 {
@@ -101,6 +104,7 @@ alloc_shm(struct dri_sw_displaytarget *dri_sw_dt, unsigned 
size)
 
return addr;
 }
+#endif
 
 static struct sw_displaytarget *
 dri_sw_displaytarget_create(struct sw_winsys *winsys,
@@ -131,8 +135,11 @@ dri_sw_displaytarget_create(struct sw_winsys *winsys,
size = dri_sw_dt->stride * nblocksy;
 
dri_sw_dt->shmid = -1;
+
+#if !defined(ANDROID) || ANDROID_API_LEVEL >= 26
if (ws->lf->put_image_shm)
   dri_sw_dt->data = alloc_shm(dri_sw_dt, size);
+#endif
 
if(!dri_sw_dt->data)
   dri_sw_dt->data = align_malloc(size, alignment);
@@ -156,8 +163,10 @@ dri_sw_displaytarget_destroy(struct sw_winsys *ws,
struct dri_sw_displaytarget *dri_sw_dt = dri_sw_displaytarget(dt);
 
if (dri_sw_dt->shmid >= 0) {
+#if !defined(ANDROID) || ANDROID_API_LEVEL >= 26
   shmdt(dri_sw_dt->data);
   shmctl(dri_sw_dt->shmid, IPC_RMID, 0);
+#endif
} else {
   align_free(dri_sw_dt->data);
}
-- 
2.18.0.233.g985f88cf7e-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 2/2] egl/surfaceless: Allow DRMless fallback.

[Chad Versace]

The two patches look correct to me.
Reviewed-by: Chad Versace 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] intel/blorp: Add a blorp_filter enum for use in blorp_blit

[Chad Versace]

On Thu 12 Jul 2018, Jason Ekstrand wrote:
> On Wed, Jun 27, 2018 at 5:55 PM Chad Versace <[1]chadvers...@chromium.org>
> wrote:
> 
> On Tue 26 Jun 2018, Jason Ekstrand wrote:
> > At the moment, this is entirely internal but we'll expose it to clients
> > of the BLORP API in the next commit.
> > ---
> >  src/intel/blorp/blorp.h      |   8 ++
> >  src/intel/blorp/blorp_blit.c | 212 +++
> >  src/intel/blorp/blorp_priv.h |  12 +-
> >  3 files changed, 123 insertions(+), 109 deletions(-)
> 
> Yup, I still read this list.
> 
> 
> \o/
>  
> 
> This patch makes the code easier to reason about. I like it.
> 
> [snip]
> 
> > +   case BLORP_FILTER_BILINEAR:
> > +      assert(!key->src_tiled_w);
> > +      assert(key->tex_samples == key->src_samples);
> > +      assert(key->tex_layout == key->src_layout);
> 
> What guarantees !key->src_tiled_w ? I can't deduce it from the patch.
> 
> 
> That's stencil and you can't do a filtered scaled blit with stencil, only
> nearest.  I believe this is required/checked fairly high up in the GL API 
> area.
>  
> 
> From my understanding of the patch, the patch allows the deduction
> below. What is the missing step to !key->src_tiled_w? Does GL not allow
> GL_LINEAR on stencil buffers? (If it does, though, then GL is dumb).
> 
> 
> Correct.  Which means that !stencil || LINEAR -> !stencil
>  
> 
> (key.filter == BLORP_FILTER_BILINEAR) <-> ((blend && blit_scaled) ||
> bilinear_filter)
>                                        -> (blend || bilinear_filter)
>                                        -> (!(src_surf.usage &
> ISL_SURF_USAGE_STENCIL_BIT) || (gl_filter == GL_LINEAR))
>                                        ?
>                                        -> !stencil
>                                        -> !key->src_tiled_w
> [snip]
> 
> > +   case BLORP_FILTER_AVERAGE:
> > +      assert(!key->src_tiled_w);
> > +      assert(key->tex_samples == key->src_samples);
> > +      assert(key->tex_layout == key->src_layout);
> > +
> 
> I expected to see assert(key->src_samples > 1) in this case.
> Just an observation.
> 
> [snip]
> 
> > +   /* We are downsampling a non-integer color buffer, so blend.
> 
> This phrase is no longer inside an if.  It should say "If we are...,
> then blend.". Or "Blend if we are...".
> 
> 
> I've changed it to "If we are..."

Ok. This patch is
Reviewed-by: Chad Versace 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] mesa: MESA_framebuffer_flip_y extension [v3]

[Chad Versace]

On Wed 11 Jul 2018, Chad Versace wrote:
> +Ken, I had a question about GLboolean. I call you by name in the
> comments below.
>
> On Fri 29 Jun 2018, Fritz Koenig wrote:
> > Adds an extension to glFramebufferParameteri
> > that will specify if the framebuffer is vertically
> > flipped. Historically system framebuffers are
> > vertically flipped and user framebuffers are not.
> > Checking to see the state was done by looking at
> > the name field.  This adds an explicit field.
> >
> > v2:
> > * updated spec language [for chadv]
> > * correctly specifying ES 3.1 [for chadv]
> > * refactor access to rb->Name [for jason]
> > * handle GetFramebufferParameteriv [for chadv]
> > v3:
> > * correct _mesa_GetMultisamplefv [for kusmabite]
> > ---
>
> >  docs/specs/MESA_framebuffer_flip_y.spec| 84 ++

[snip]

> > diff --git a/src/mesa/drivers/dri/i915/intel_fbo.c 
> > b/src/mesa/drivers/dri/i915/intel_fbo.c
> > index 827a77f722..31b65fb53b 100644
> > --- a/src/mesa/drivers/dri/i915/intel_fbo.c
> > +++ b/src/mesa/drivers/dri/i915/intel_fbo.c
> > @@ -86,7 +86,8 @@ intel_map_renderbuffer(struct gl_context *ctx,
> >GLuint x, GLuint y, GLuint w, GLuint h,
> >GLbitfield mode,
> >GLubyte **out_map,
> > -  GLint *out_stride)
> > +  GLint *out_stride,
> > +  GLboolean inverted_y)

[snip]

> And I believe the internal APIs should use 'bool' instead of
> 'GLboolean'. See commit 786a6472450b50977e6906e27d5f481e00b05d73 .
>
> Ken, should Fritz also use plain 'bool' in struct gl_framebuffer? That
> is, should it be
>
> struct gl_framebuffer {
> ...
> GLboolean FlipY;
> or
> bool FlipY;

I received feedback on #intel-3d about the GLboolean-vs-bool question.

jekstrand | chadv: The extension?  GLboolean.  Internal calls?  bool.
jekstrand | chadv: GLboolean should die!
   Kayden | chadv: definitely bool, GLboolean is evil
   Kayden | anything not touching the GL API explicitly should be bool
   Kayden | even fields in gl_context should get converted IMO
   anholt | agreed
   Kayden | we've had bugs where somebody returned a pointer as a GLboolean
   Kayden | thinking it would get treated as true/false
   Kayden | and instead it truncated to signed char
   Kayden | so based on the address it would have a random truth value
   Kayden | sorry I neglected to email you back :(
jekstrand | unless you're on a 64-bit system and it just happens to be aligned
  | correctly...

Based on their emphatic feedback, definitely use bool in all driver internal 
APIs.
And you should probably use bool in struct framebuffer too.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] mesa: MESA_framebuffer_flip_y extension [v3]

[Chad Versace]

+Ken, I had a question about GLboolean. I call you by name in the
comments below.

On Fri 29 Jun 2018, Fritz Koenig wrote:
> Adds an extension to glFramebufferParameteri
> that will specify if the framebuffer is vertically
> flipped. Historically system framebuffers are
> vertically flipped and user framebuffers are not.
> Checking to see the state was done by looking at
> the name field.  This adds an explicit field.
> 
> v2:
> * updated spec language [for chadv]
> * correctly specifying ES 3.1 [for chadv]
> * refactor access to rb->Name [for jason]
> * handle GetFramebufferParameteriv [for chadv]
> v3:
> * correct _mesa_GetMultisamplefv [for kusmabite]
> ---

>  docs/specs/MESA_framebuffer_flip_y.spec| 84 ++

Use file extension '.txt'. Khronos no longer uses the '.spec' extension.

File docs/specs/enums.txt needs an update too.

>  include/GLES2/gl2ext.h |  5 ++
>  src/mapi/glapi/registry/gl.xml |  6 ++
>  src/mesa/drivers/dri/i915/intel_fbo.c  |  7 +-
>  src/mesa/drivers/dri/i965/intel_fbo.c  |  7 +-
>  src/mesa/drivers/dri/nouveau/nouveau_fbo.c |  7 +-
>  src/mesa/drivers/dri/radeon/radeon_fbo.c   |  7 +-
>  src/mesa/drivers/dri/radeon/radeon_span.c  |  9 ++-
>  src/mesa/drivers/dri/swrast/swrast.c   |  7 +-
>  src/mesa/drivers/osmesa/osmesa.c   |  5 +-
>  src/mesa/drivers/x11/xm_buffer.c   |  3 +-
>  src/mesa/drivers/x11/xmesaP.h  |  3 +-
>  src/mesa/main/accum.c  | 17 +++--
>  src/mesa/main/dd.h |  3 +-
>  src/mesa/main/extensions_table.h   |  1 +
>  src/mesa/main/fbobject.c   | 18 -
>  src/mesa/main/framebuffer.c|  1 +
>  src/mesa/main/glheader.h   |  3 +
>  src/mesa/main/mtypes.h |  3 +
>  src/mesa/main/readpix.c| 20 +++---
>  src/mesa/state_tracker/st_cb_fbo.c |  7 +-
>  src/mesa/swrast/s_blit.c   | 17 +++--
>  src/mesa/swrast/s_clear.c  |  3 +-
>  src/mesa/swrast/s_copypix.c| 11 +--
>  src/mesa/swrast/s_depth.c  |  6 +-
>  src/mesa/swrast/s_drawpix.c| 26 ---
>  src/mesa/swrast/s_renderbuffer.c   |  6 +-
>  src/mesa/swrast/s_renderbuffer.h   |  3 +-
>  src/mesa/swrast/s_stencil.c|  3 +-
>  29 files changed, 241 insertions(+), 57 deletions(-)
>  create mode 100644 docs/specs/MESA_framebuffer_flip_y.spec
> 
> diff --git a/docs/specs/MESA_framebuffer_flip_y.spec 
> b/docs/specs/MESA_framebuffer_flip_y.spec
> new file mode 100644
> index 00..dca77a9541
> --- /dev/null
> +++ b/docs/specs/MESA_framebuffer_flip_y.spec
> @@ -0,0 +1,84 @@
> +Name
> +
> +MESA_framebuffer_flip_y
> +
> +Name Strings
> +
> +GL_MESA_framebuffer_flip_y
> +
> +Contact
> +
> +Fritz Koenig 
> +
> +Contributors
> +
> +Fritz Koenig, Google
> +Kristian Høgsberg, Google
> +Chad Versace, Google
> +
> +Status
> +
> +Proposal
> +
> +Version
> +
> +Version 1, June 7, 2018
> +
> +Number
> +
> +TBD
> +
> +Dependencies
> +
> +OpenGL ES 3.1 is required, for FramebufferParameteri.
> +
> +Overview
> +
> +Rendered buffers are normally returned right side up, as accessed
> +top to bottom.  This extension allows those buffers to be upside down
> +when accessed top to bottom.
> +
> +This extension defines a new framebuffer parameter,
> +GL_FRAMEBUFFER_FLIP_Y_MESA, that changes the behavior of the reads and
> +writes to the framebuffer attachment points. When 
> GL_FRAMEBUFFER_FLIP_Y_MESA
> +is GL_TRUE, render commands and pixel transfer operations access the
> +backing store of each attachment point with an y-inverted coordinate
> +system. This y-inversion is relative to the coordinate system set when
> +GL_FRAMEBUFFER_FLIP_Y_MESA is GL_FALSE.
> +
> +Access through TexSubImage2D and similar calls will notice the effect of
> +the flip when they are not attached to framebuffer objects because
> +GL_FRAMEBUFFER_FLIP_Y_MESA is associated with the framebuffer object and
> +not the attachment points.
> +
> +IP Status
> +
> +None
> +
> +Issues
> +
> +None
> +
> +New Procedures and Functions
> +
> +None
> +
> +New Types
> +
> +None
> +
> +New Tokens
> +
> +Accepted by the  argument of FramebufferParameteri and
> +GetFramebufferParameteriv:
> +
> +GL_FRAMEBUFFER_FLIP_Y_MESA  0x8BBB
> +
> +Errors
> +GL_INVALID_OPERATION is returned from  GetFramebuf

[Mesa-dev] [PATCH 2/2] anv/android: Fix type error in call to vk_errorf()

[Chad Versace]

In a single call to vk_errorf() in the Android code, the arguments were
swapped. The bug has existed since day one. Chrome OS used to forgive
the warning, but it is now a compilation error.

Fixes: 053d4c32 "anv: Implement VK_ANDROID_native_buffer (v9)"
Cc: Tapani Pälli 
Cc: Tomasz Figa 
---
 src/intel/vulkan/anv_android.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/intel/vulkan/anv_android.c b/src/intel/vulkan/anv_android.c
index 3c905d3bdb5..a3bab8087b4 100644
--- a/src/intel/vulkan/anv_android.c
+++ b/src/intel/vulkan/anv_android.c
@@ -180,7 +180,7 @@ anv_image_from_gralloc(VkDevice device_h,
   goto fail_create;
 
if (bo->size < image->size) {
-  result = vk_errorf(device, device->instance,
+  result = vk_errorf(device->instance, device,
  VK_ERROR_INVALID_EXTERNAL_HANDLE_KHR,
  "dma-buf from VkNativeBufferANDROID is too small for "
  "VkImage: %"PRIu64"B < %"PRIu64"B",
-- 
2.18.0.rc2.346.g013aa6912e-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/2] anv/android: Fix Autotools build for VK_ANDROID_native_buffer

[Chad Versace]

Changes to vk.xml and anv_entrypoints_gen.py broke the Autotools build
on Android. The changes undef'd the VK_ANDROID_native_buffer entrypoints in 
anv_entrypoints.h.
Fix it with CPPFLAGS += -DVK_USE_PLATFORM_ANDROID_KHR.

See-Also: 63525ba7 "android: enable VK_ANDROID_native_buffer"
Cc: Tapani Pälli 
Cc: Tomasz Figa 
---
 src/intel/Makefile.vulkan.am | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/intel/Makefile.vulkan.am b/src/intel/Makefile.vulkan.am
index 4a80c3ae412..9555d98095b 100644
--- a/src/intel/Makefile.vulkan.am
+++ b/src/intel/Makefile.vulkan.am
@@ -164,7 +164,10 @@ VULKAN_LIB_DEPS = \
-lm
 
 if HAVE_PLATFORM_ANDROID
-VULKAN_CPPFLAGS += $(ANDROID_CPPFLAGS)
+VULKAN_CPPFLAGS += \
+$(ANDROID_CPPFLAGS) \
+-DVK_USE_PLATFORM_ANDROID_KHR
+
 VULKAN_CFLAGS += $(ANDROID_CFLAGS)
 VULKAN_LIB_DEPS += $(ANDROID_LIBS)
 VULKAN_SOURCES += $(VULKAN_ANDROID_FILES)
-- 
2.18.0.rc2.346.g013aa6912e-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] intel/blorp: Take an explicit filter parameter in blorp_blit

[Chad Versace]

  VK_IMAGE_ASPECT_COLOR_BIT,

[snip]

> @@ -324,6 +324,65 @@ brw_blorp_blit_miptrees(struct brw_context *brw,
>src_format = dst_format = MESA_FORMAT_R_FLOAT32;
> }
>  
> +   enum blorp_filter blorp_filter;
> +   if (fabs(dst_x1 - dst_x0) == fabs(src_x1 - src_x0) &&
> +   fabs(dst_y1 - dst_y0) == fabs(src_y1 - src_y0)) {

You should use fabsf() here because all params are floats.

> +  if (src_mt->surf.samples > 1 && dst_mt->surf.samples <= 1) {
> + /* From the OpenGL ES 3.2 specification, section 16.2.1:
> +  *
> +  *"If the read framebuffer is multisampled (its effective value
> +  *of SAMPLE_BUFFERS is one) and the draw framebuffer is not (its
> +  *value of SAMPLE_BUFFERS is zero), the samples corresponding to
> +  *each pixel location in the source are converted to a single
> +  *sample before being written to the destination.  The filter
> +  *parameter is ignored. If the source formats are integer types
> +  *or stencil values, a single sample’s value is selected for 
> each
> +  *pixel.  If the source formats are floating-point or normalized
> +  *types, the sample values for each pixel are resolved in an
> +  *implementation-dependent manner.  If the source formats are
> +  *depth values, sample values are resolved in an implementation-
> +  *dependent manner where the result will be between the minimum
> +  *and maximum depth values in the pixel."
> +  *
> +  * For depth and stencil resolves, we choose to always use the value
> +  * at sample 0.
> +  */
> + GLenum base_format = _mesa_get_format_base_format(src_mt->format);
> + if (base_format == GL_DEPTH_COMPONENT ||
> + base_format == GL_STENCIL_INDEX ||
> + base_format == GL_DEPTH_STENCIL ||
> + _mesa_is_format_integer(src_mt->format)) {
> +/* The OpenGL ES 3.2 spec says:
> + *
> + *"If the source formats are integer types or stencil values,
> + *a single sample's value is selected for each pixel."
> + *
> + * Just take sample 0 in this case.
> + */
> +blorp_filter = BLORP_FILTER_SAMPLE_0;
> + } else {
> +blorp_filter = BLORP_FILTER_AVERAGE;
> + }
> +  } else {
> + /* From the OpenGL 4.6 specification, section 18.3.1:
> +  *
> +  *"If the source and destination dimensions are identical, no
> +  *filtering is applied."
> +  *
> +  * Using BLORP_FILTER_NONE will also handle the upsample case by
> +  * replicating the one value in the source to all values in the
> +  * destination.
> +  */
> + blorp_filter = BLORP_FILTER_NONE;
> +  }
> +   } else if (gl_filter == GL_LINEAR ||
> +  gl_filter == GL_SCALED_RESOLVE_FASTEST_EXT ||
> +  gl_filter == GL_SCALED_RESOLVE_NICEST_EXT) {
> +  blorp_filter = BLORP_FILTER_BILINEAR;
> +   } else {
> +  blorp_filter = BLORP_FILTER_NEAREST;
> +   }

This hunk is great. Blorp no longer needs to reverse-engineer what GL
was poorly trying to tell it.

With s/fabs/fabsf/, this patch is
Reviewed-by: Chad Versace 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] intel/blorp: Add a blorp_filter enum for use in blorp_blit

[Chad Versace]

On Tue 26 Jun 2018, Jason Ekstrand wrote:
> At the moment, this is entirely internal but we'll expose it to clients
> of the BLORP API in the next commit.
> ---
>  src/intel/blorp/blorp.h  |   8 ++
>  src/intel/blorp/blorp_blit.c | 212 +++
>  src/intel/blorp/blorp_priv.h |  12 +-
>  3 files changed, 123 insertions(+), 109 deletions(-)

Yup, I still read this list.

This patch makes the code easier to reason about. I like it.

[snip]

> +   case BLORP_FILTER_BILINEAR:
> +  assert(!key->src_tiled_w);
> +  assert(key->tex_samples == key->src_samples);
> +  assert(key->tex_layout == key->src_layout);

What guarantees !key->src_tiled_w ? I can't deduce it from the patch.

From my understanding of the patch, the patch allows the deduction
below. What is the missing step to !key->src_tiled_w? Does GL not allow
GL_LINEAR on stencil buffers? (If it does, though, then GL is dumb).


(key.filter == BLORP_FILTER_BILINEAR) <-> ((blend && blit_scaled) || 
bilinear_filter)
   -> (blend || bilinear_filter)
   -> (!(src_surf.usage & 
ISL_SURF_USAGE_STENCIL_BIT) || (gl_filter == GL_LINEAR))
   ?
   -> !stencil
   -> !key->src_tiled_w

[snip]

> +   case BLORP_FILTER_AVERAGE:
> +  assert(!key->src_tiled_w);
> +  assert(key->tex_samples == key->src_samples);
> +  assert(key->tex_layout == key->src_layout);
> +

I expected to see assert(key->src_samples > 1) in this case.
Just an observation.

[snip]

> +   /* We are downsampling a non-integer color buffer, so blend.

This phrase is no longer inside an if.  It should say "If we are...,
then blend.". Or "Blend if we are...".

> +*
> +* Regarding integer color buffers, the OpenGL ES 3.2 spec says:
> +*
> +*"If the source formats are integer types or stencil values, a
> +*single sample's value is selected for each pixel."
> +*
> +* This implies we should not blend in that case.
> +*/
> +   const bool blend =
> +  (params.src.surf.usage & ISL_SURF_USAGE_DEPTH_BIT) == 0 &&
> +  (params.src.surf.usage & ISL_SURF_USAGE_STENCIL_BIT) == 0 &&
> +  !isl_format_has_int_channel(params.src.surf.format) &&
> +  params.src.surf.samples > 1 &&
> +  params.dst.surf.samples <= 1;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] mesa : MESA_framebuffer_flip_y extension

[Chad Versace]

On Wed 27 Jun 2018, Chad Versace wrote:
> On Fri 15 Jun 2018, Fritz Koenig wrote:
> > On Tue, Jun 12, 2018 at 12:28 PM Chad Versace  
> > wrote:
> > >
> > > On Thu 07 Jun 2018, Fritz Koenig wrote:
> 
> > > > --- a/src/mesa/main/extensions_table.h
> > > > +++ b/src/mesa/main/extensions_table.h
> > > > @@ -322,6 +322,7 @@ EXT(KHR_texture_compression_astc_hdr, 
> > > > KHR_texture_compression_astc_hdr
> > > >  EXT(KHR_texture_compression_astc_ldr, 
> > > > KHR_texture_compression_astc_ldr   , GLL, GLC,  x , ES2, 2012)
> > > >  EXT(KHR_texture_compression_astc_sliced_3d  , 
> > > > KHR_texture_compression_astc_sliced_3d , GLL, GLC,  x , ES2, 2015)
> > > >
> > > > +EXT(MESA_framebuffer_flip_y , MESA_framebuffer_flip_y  
> > > >   ,   x,   x,  x , ES2, 2018)
> > >
> > > Since the extension requires GLES 3.1, this row should have s/ES2/31/.
> > >
> > 
> > I don't see the option for an ES31 extension, only ES2 in that file.
> 
> It's non-obvious, but placing a literal 31 in that column does the right
> thing. Search the table, and you will find entries in this column with
> literal values 30 and 31 in addition to ES2.
> 
> A little bit of poorly documented tribal knowledge... In the taxonomy of
> OpenGL (and ES) contexts, there currently exist four top-level branches.
> It took multiple years of API specification experiments, some good and
> other mistakes, to create the top-level taxonomy. Surprisingly, old man
> GLX is the only API to get this correct, in extension
> GLX_EXT_create_context_es2_profile. (GLX got it right probably because
> GLX_EXT_create_context_es2_profile was the last extension to be written
> in the group of "context taxonomy" extensions, and so was written after
> everyone learned from the past mistakes).
> 
> The term used by GLX_EXT_create_context_es2_profile for this hiearchy in
> the taxonomy is "profile". Here are the 4:
> 
>   ES1 := { OpenGL ES x.y where x = 1 }
>= { OpenGL ES 1.0
>  , OpenGL ES 1.1,
>  , and minor variants of which no one remembers the name
>  }
> 
>   ES2 := { OpenGL ES x.y where 2 <= x <= 3 }
> 
>   CORE := { OpenGL x.y where x.y >= 3.2
>  and glGetIntegerv(GL_CONTEXT_PROFILE_MASK) contains 
> GL_CONTEXT_PROFILE_CORE_BIT
>   }
> 
>   COMPATIBILITY := { OpenGL x.y where x.y <= 3.0
>or (x.y >= 3.2
>and 
> glGetIntegerv(GL_CONTEXT_PROFILE_MASK) contains 
> GL_CONTEXT_PROFILE_COMPATIBILITY_BIT)
>}

Correction:

OpenGL (not-ES) 3.1 belongs to no class as I've defined them above.
And OpenGL ES 3.0 is not defined correctly. Their weird, existing the
zone of API experimentation.

Here are the corrected definitions:

  CORE := { OpenGL x.y where (x.y >= 3.2 and GL_CONTEXT_PROFILE_CORE_BIT in 
glGetIntegerv(GL_CONTEXT_PROFILE_MASK))
  or (x.y == 3.1 and GL_ARB_compatibility not in 
glGetString(GL_EXTENSIONS))
  or (x.y == 3.0 and 
GL_CONTEXT_FLAG_FORWARD_COMPATIBLE_BIT in glGetIntegerv(GL_CONTEXT_FLAGS))
  }

  COMPATIBILITY := { OpenGL x.y where (x.y >= 3.2 and 
GL_CONTEXT_PROFILE_COMPATIBILITY_BIT in glGetIntegerv(GL_CONTEXT_PROFILE_MASK))
   or (x.y == 3.1 and GL_ARB_compatibility in 
glGetString(GL_EXTENSIONS))
   or (x.y == 3.0 and 
GL_CONTEXT_FLAG_FORWARD_COMPATIBLE_BIT not in glGetIntegerv(GL_CONTEXT_FLAGS))
   or (x.y < 3.0)
   }

There exist differences in opinion on how to classify the variants of
GL 3.0 and GL 3.1. The above is Mesa's classification.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] mesa : MESA_framebuffer_flip_y extension

[Chad Versace]

On Fri 15 Jun 2018, Fritz Koenig wrote:
> On Tue, Jun 12, 2018 at 12:28 PM Chad Versace  
> wrote:
> >
> > On Thu 07 Jun 2018, Fritz Koenig wrote:

> > > --- a/src/mesa/main/extensions_table.h
> > > +++ b/src/mesa/main/extensions_table.h
> > > @@ -322,6 +322,7 @@ EXT(KHR_texture_compression_astc_hdr, 
> > > KHR_texture_compression_astc_hdr
> > >  EXT(KHR_texture_compression_astc_ldr, 
> > > KHR_texture_compression_astc_ldr   , GLL, GLC,  x , ES2, 2012)
> > >  EXT(KHR_texture_compression_astc_sliced_3d  , 
> > > KHR_texture_compression_astc_sliced_3d , GLL, GLC,  x , ES2, 2015)
> > >
> > > +EXT(MESA_framebuffer_flip_y , MESA_framebuffer_flip_y
> > > ,   x,   x,  x , ES2, 2018)
> >
> > Since the extension requires GLES 3.1, this row should have s/ES2/31/.
> >
> 
> I don't see the option for an ES31 extension, only ES2 in that file.

It's non-obvious, but placing a literal 31 in that column does the right
thing. Search the table, and you will find entries in this column with
literal values 30 and 31 in addition to ES2.

A little bit of poorly documented tribal knowledge... In the taxonomy of
OpenGL (and ES) contexts, there currently exist four top-level branches.
It took multiple years of API specification experiments, some good and
other mistakes, to create the top-level taxonomy. Surprisingly, old man
GLX is the only API to get this correct, in extension
GLX_EXT_create_context_es2_profile. (GLX got it right probably because
GLX_EXT_create_context_es2_profile was the last extension to be written
in the group of "context taxonomy" extensions, and so was written after
everyone learned from the past mistakes).

The term used by GLX_EXT_create_context_es2_profile for this hiearchy in
the taxonomy is "profile". Here are the 4:

  ES1 := { OpenGL ES x.y where x = 1 }
   = { OpenGL ES 1.0
 , OpenGL ES 1.1,
 , and minor variants of which no one remembers the name
 }

  ES2 := { OpenGL ES x.y where 2 <= x <= 3 }

  CORE := { OpenGL x.y where x.y >= 3.2
 and glGetIntegerv(GL_CONTEXT_PROFILE_MASK) contains 
GL_CONTEXT_PROFILE_CORE_BIT
  }

  COMPATIBILITY := { OpenGL x.y where x.y <= 3.0
   or (x.y >= 3.2
   and 
glGetIntegerv(GL_CONTEXT_PROFILE_MASK) contains 
GL_CONTEXT_PROFILE_COMPATIBILITY_BIT)
   }


That's why the file defines no token for ES31. It's a subclass of ES2.


The file defines ES2 = 0, and x = ~0.

When Mesa examines this
table in reponse glGetString(GL_EXTENSIONS), it scans the column for the
current context's profile. For each row it compares
(context_version >= column_value). That's why inserting 31 in the column
magically works. Instead of the macro ES2, we could equivalently insert
20 into the column, or even 0, since no context versions < 2.0 exist in
the ES2 class. (In fact, ES2 is 0).
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] gallium: Fix automake for Android

[Chad Versace]

Chromium OS uses Autotools and pkg-config when building Mesa for
Android. The gallium drivers were failing to find the headers and
libraries for zlib and Android's libbacktrace.

v2:
  - Don't add a check for zlib.pc. configure.ac already checks for
zlib.pc elsewhere. [for tfiga]
  - Check for backtrace.pc separately from the other Android libs.
[for tfiga]
---

Tomasz, I added the suggestions that worked. I tried moving
$(ANDROID_LIBS) and $(BACKTRACE_LIBS) to the hunk in
src/gallium/auxiliary/Makefile.am, but the build no longer worked.
libEGL failed to link.

 configure.ac  | 3 +++
 src/gallium/Automake.inc  | 6 ++
 src/gallium/auxiliary/Makefile.am | 4 
 3 files changed, 13 insertions(+)

diff --git a/configure.ac b/configure.ac
index 7a0e4754208..acdb7f1adb4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1831,6 +1831,9 @@ for plat in $platforms; do
 
 android)
 PKG_CHECK_MODULES([ANDROID], [cutils hardware sync])
+if test -n "$with_gallium_drivers"; then
+PKG_CHECK_MODULES([BACKTRACE], [backtrace])
+fi
 DEFINES="$DEFINES -DHAVE_ANDROID_PLATFORM"
 ;;
 
diff --git a/src/gallium/Automake.inc b/src/gallium/Automake.inc
index 3e21aa71b5c..329c8839e17 100644
--- a/src/gallium/Automake.inc
+++ b/src/gallium/Automake.inc
@@ -59,6 +59,12 @@ GALLIUM_COMMON_LIB_DEPS += \
$(LIBDRM_LIBS)
 endif
 
+if HAVE_PLATFORM_ANDROID
+GALLIUM_COMMON_LIB_DEPS += \
+   $(ANDROID_LIBS) \
+   $(BACKTRACE_LIBS)
+endif
+
 GALLIUM_WINSYS_CFLAGS = \
-I$(top_srcdir)/src \
-I$(top_srcdir)/include \
diff --git a/src/gallium/auxiliary/Makefile.am 
b/src/gallium/auxiliary/Makefile.am
index 6b048b8eebf..03908198772 100644
--- a/src/gallium/auxiliary/Makefile.am
+++ b/src/gallium/auxiliary/Makefile.am
@@ -21,6 +21,10 @@ libgallium_la_SOURCES = \
$(NIR_SOURCES) \
$(GENERATED_SOURCES)
 
+if HAVE_PLATFORM_ANDROID
+libgallium_la_SOURCES += util/u_debug_stack_android.cpp
+endif
+
 if HAVE_LIBDRM
 
 AM_CFLAGS += \
-- 
2.17.0.775.ge144d126d7

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] gallium: Fix automake for Android

[Chad Versace]

Chromium OS uses Autotools and pkg-config when building Mesa for
Android. The gallium drivers were failing to find the headers and
libraries for zlib and Android's libbacktrace.
---
 configure.ac  | 6 +-
 src/gallium/Automake.inc  | 5 +
 src/gallium/auxiliary/Makefile.am | 4 
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 7a0e4754208..5f0792cd8ef 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1830,7 +1830,11 @@ for plat in $platforms; do
 ;;
 
 android)
-PKG_CHECK_MODULES([ANDROID], [cutils hardware sync])
+android_modules='cutils hardware sync zlib'
+if test -n "$with_gallium_drivers"; then
+android_modules+=' backtrace'
+fi
+PKG_CHECK_MODULES([ANDROID], ["$android_modules"])
 DEFINES="$DEFINES -DHAVE_ANDROID_PLATFORM"
 ;;
 
diff --git a/src/gallium/Automake.inc b/src/gallium/Automake.inc
index 3e21aa71b5c..8181927a396 100644
--- a/src/gallium/Automake.inc
+++ b/src/gallium/Automake.inc
@@ -59,6 +59,11 @@ GALLIUM_COMMON_LIB_DEPS += \
$(LIBDRM_LIBS)
 endif
 
+if HAVE_PLATFORM_ANDROID
+GALLIUM_COMMON_LIB_DEPS += \
+   $(ANDROID_LIBS)
+endif
+
 GALLIUM_WINSYS_CFLAGS = \
-I$(top_srcdir)/src \
-I$(top_srcdir)/include \
diff --git a/src/gallium/auxiliary/Makefile.am 
b/src/gallium/auxiliary/Makefile.am
index 6b048b8eebf..03908198772 100644
--- a/src/gallium/auxiliary/Makefile.am
+++ b/src/gallium/auxiliary/Makefile.am
@@ -21,6 +21,10 @@ libgallium_la_SOURCES = \
$(NIR_SOURCES) \
$(GENERATED_SOURCES)
 
+if HAVE_PLATFORM_ANDROID
+libgallium_la_SOURCES += util/u_debug_stack_android.cpp
+endif
+
 if HAVE_LIBDRM
 
 AM_CFLAGS += \
-- 
2.17.0.775.ge144d126d7

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] mesa : MESA_framebuffer_flip_y extension

[Chad Versace]

On Thu 07 Jun 2018, Fritz Koenig wrote:
> Adds an extension to glFramebufferParameteri
> that will specify if the framebuffer is vertically
> flipped. Historically system framebuffers are
> vertically flipped and user framebuffers are not.
> Checking to see the state was done by looking at
> the name field.  This adds an explicit field.
> ---
>  docs/specs/MESA_framebuffer_flip_y.spec | 59 +
>  include/GLES2/gl2ext.h  |  5 +++
>  src/mapi/glapi/registry/gl.xml  |  6 +++
>  src/mesa/main/extensions_table.h|  1 +
>  src/mesa/main/fbobject.c|  8 
>  src/mesa/main/framebuffer.c |  1 +
>  src/mesa/main/glheader.h|  3 ++
>  src/mesa/main/mtypes.h  |  4 ++
>  8 files changed, 87 insertions(+)
>  create mode 100644 docs/specs/MESA_framebuffer_flip_y.spec
> 
> diff --git a/docs/specs/MESA_framebuffer_flip_y.spec 
> b/docs/specs/MESA_framebuffer_flip_y.spec
> new file mode 100644
> index 00..b9867e0683
> --- /dev/null
> +++ b/docs/specs/MESA_framebuffer_flip_y.spec
> @@ -0,0 +1,59 @@
> +Name
> +
> +MESA_framebuffer_flip_y
> +
> +Name Strings
> +
> +GL_MESA_framebuffer_flip_y
> +
> +Contact
> +
> +Fritz Koenig 
> +
> +Status
> +
> +Proposal
> +
> +Version
> +
> +Version 1, June 7, 2018
> +
> +Number
> +
> +TBD
> +
> +Dependencies
> +
> +OpenGLES 3.1 is required.

In specs, s/OpenGLES/OpenGL ES/.

> +
> +Overview
> +
> +This extension adds the ability to specify that the internal framebuffer
> +object is vertically flipped.

Questions:

  a. Is it legal to set and/or query GL_FRAMEBUFFER_FLIP_Y_MESA on the
 default framebuffer (that is, the winsys framebuffer)?
  b. If (a) is legal, then the default framebuffer's initial value of
 GL_FRAMEBUFFER_FLIP_Y_MESA is ambiguous. Does the spec define it
 to be GL_TRUE or GL_FALSE? (I assume the answer is GL_TRUE based on
 the patch's code).

The patch seems to allow toggling the state of winsys framebuffers,
but I was unsure if that decision was intentional.

> +New Tokens
> +
> +Accepted by the  argument of FramebufferParameteri:
> +
> +GL_FRAMEBUFFER_FLIP_Y_EXT   0x8BBB

... and GetFramebufferParameteriv.

> +
> +Revision History
> +
> +Version 1, June, 2018
> +Initial draft (Fritz Koenig)
> diff --git a/include/GLES2/gl2ext.h b/include/GLES2/gl2ext.h
> index a7d19a1fc8..7fb5e9ca5f 100644
> --- a/include/GLES2/gl2ext.h
> +++ b/include/GLES2/gl2ext.h
> @@ -2334,6 +2334,11 @@ GL_APICALL void GL_APIENTRY glGetPerfQueryInfoINTEL 
> (GLuint queryId, GLuint quer
>  #endif
>  #endif /* GL_INTEL_performance_query */
>  
> +#ifndef GL_MESA_framebuffer_flip_y
> +#define GL_MESA_framebuffer_flip_y 1
> +#define GL_FRAMEBUFFER_FLIP_Y_EXT 0x8BBB
> +#endif /* GL_MESA_framebuffer_flip_y */
> +
>  #ifndef GL_MESA_program_binary_formats
>  #define GL_MESA_program_binary_formats 1
>  #define GL_PROGRAM_BINARY_FORMAT_MESA 0x875F
> diff --git a/src/mapi/glapi/registry/gl.xml b/src/mapi/glapi/registry/gl.xml
> index 833478aa51..3a3d4f3d81 100644
> --- a/src/mapi/glapi/registry/gl.xml
> +++ b/src/mapi/glapi/registry/gl.xml
> @@ -6568,6 +6568,7 @@ typedef unsigned int GLhandleARB;
>  
>  
>  
> +
>  
>  
>   comment="Reassigned from AMD to QCOM">
> @@ -44356,6 +44357,11 @@ typedef unsigned int GLhandleARB;
>  
>  
>  
> +
> +
> +
> +
> +
>  
>  
>  
> diff --git a/src/mesa/main/extensions_table.h 
> b/src/mesa/main/extensions_table.h
> index 79ef228b69..03a5c7b345 100644
> --- a/src/mesa/main/extensions_table.h
> +++ b/src/mesa/main/extensions_table.h
> @@ -322,6 +322,7 @@ EXT(KHR_texture_compression_astc_hdr, 
> KHR_texture_compression_astc_hdr
>  EXT(KHR_texture_compression_astc_ldr, 
> KHR_texture_compression_astc_ldr   , GLL, GLC,  x , ES2, 2012)
>  EXT(KHR_texture_compression_astc_sliced_3d  , 
> KHR_texture_compression_astc_sliced_3d , GLL, GLC,  x , ES2, 2015)
>  
> +EXT(MESA_framebuffer_flip_y , MESA_framebuffer_flip_y
> ,   x,   x,  x , ES2, 2018)

Since the extension requires GLES 3.1, this row should have s/ES2/31/.

>  EXT(MESA_pack_invert, MESA_pack_invert   
> , GLL, GLC,  x ,  x , 2002)
>  EXT(MESA_shader_integer_functions   , MESA_shader_integer_functions  
> , GLL, GLC,  x ,  30, 2016)
>  EXT(MESA_texture_signed_rgba, EXT_texture_snorm  
> , GLL, GLC,  x ,  x , 2009)
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index a63e8b8de5..efa000ede7 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -1448,6 +1448,14 @@ framebuffer_parameteri(struct gl_context *ctx, struct 
> gl_framebuffer *fb,
>

Re: [Mesa-dev] [PATCH 2/2] i965: implement MESA_framebuffer_flip_y

[Chad Versace]

On Thu 07 Jun 2018, Jason Ekstrand wrote:
> There are also a pile of places where we look at rb->Name == 0 to determine if
> it's a window-system framebuffer

For the public record, Fritz and I discussed this in person.

I recommend that the patches replace each checking on rb->Name == 0 to
instead check if the framebuffer is in flip mode. In other words,
replace checks on the rb with a check on the fb.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] i965: add {X, A}BGR2101010 to 'intel_image_formats'

[Chad Versace]

On Mon 07 May 2018, Miguel Casas wrote:
> This patch adds {X,A}BGR2101010 entries to the list of supported
> 'intel_image_formats'.
> 
> Bug: https://crbug.com/776093
> ---
>  src/mesa/drivers/dri/i965/intel_screen.c | 6 ++
>  1 file changed, 6 insertions(+)

Reviewed-by: Chad Versace <chadvers...@chromium.org>

I want to see at least an Ack from Intel before pushing these
two patches, because I haven't committed to the driver in a while.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] dri_util: Add R10G10B10{A, X}2 translation between DRI and mesa_format.

[Chad Versace]

On Mon 07 May 2018, Miguel Casas wrote:
> Add R10G10B10{A,X}2 translation between mesa_format and DRI format
> to driGLFormatToImageFormat() and driImageFormatToGLFormat().
> 
> Bug: https://crbug.com/776093
> ---
>  src/mesa/drivers/dri/common/dri_util.c | 8 
>  1 file changed, 8 insertions(+)

Reviewed-by: Chad Versace <chadvers...@chromium.org>

We should drop the space in "{A, X}" before committing,
but the committer can apply that fix. No need to resend.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] i965: add {X, A}BGR2101010 to |intel_image_formats|

[Chad Versace]

On Wed 02 May 2018, Miguel Casas wrote:
> This patch adds {X,A}BGR2101010 entries to the list of supported
> |intel_image_formats|.
> 
> BUG=https://crbug.com/776093
> TEST=Compile and deploy mesa this patch, then playback
> a VP9 Profile 2 video with sw decoder using crrev.com/c/897894.
> ---
>  src/mesa/drivers/dri/i965/intel_screen.c | 6 ++
>  1 file changed, 6 insertions(+)

Everything I said about the previous patch applies here too :)

Also, two new comments:

  * Don't add |bars| around token names. If you use annotate the token
name, then use 'single_quotes'. Sometimes people use `backticks` too.
It's also fine to omit the annotation.

  * In brace expansion, spaces are preserved. Therefore, it should be
{X,A} and not {X, A}.

By the way, most people would squash these two patches together and use
the prefix "i965:" or "i965,dri:". Me, I prefer to keep them separate.
It's your call.

When you resend the patch (or patches), CC Kenneth Graunke too.

> 
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
> b/src/mesa/drivers/dri/i965/intel_screen.c
> index 409f763b64..d3488b9f29 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -190,6 +190,12 @@ static const struct intel_image_format 
> intel_image_formats[] = {
> { __DRI_IMAGE_FOURCC_XRGB2101010, __DRI_IMAGE_COMPONENTS_RGB, 1,
>   { { 0, 0, 0, __DRI_IMAGE_FORMAT_XRGB2101010, 4 } } },
>  
> +   { __DRI_IMAGE_FOURCC_ABGR2101010, __DRI_IMAGE_COMPONENTS_RGBA, 1,
> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_ABGR2101010, 4 } } },
> +
> +   { __DRI_IMAGE_FOURCC_XBGR2101010, __DRI_IMAGE_COMPONENTS_RGB, 1,
> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_XBGR2101010, 4 } } },
> +
> { __DRI_IMAGE_FOURCC_ARGB, __DRI_IMAGE_COMPONENTS_RGBA, 1,
>   { { 0, 0, 0, __DRI_IMAGE_FORMAT_ARGB, 4 } } },
>  
> -- 
> 2.17.0.441.gb46fe60e1d-goog
> 
> ___
> 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] mesa: add R10G10B10{A, X}2 to MESA <-> DRI format translations

[Chad Versace]

Thanks for the patches. The code looks good. All my suggestions are
merely nitpicks to make the patches follow Mesa conventions.

In general, if you have questions about commit message style, examine
the git log for previous patches that touched the same files and
directories as yours. Sometimes, different directories in Mesa can have
very different code style as well as commit style.

First, when a patch touches src/mesa/drivers/dri/common and/or
include/GL/internal/dri_interface.h, and touches nothing else, the patch
subject should probably have the prefix "dri:". For patches that touch
only dri_util.c, like yours, there is also a precedent for using the
"dri_util:" prefix.

On Wed 02 May 2018, Miguel Casas wrote:
> This patch adds R10G10B10{A,X}2 MESA <-> DRI translation entries
> in the appropriate places for dri2 functions to accept them.

"DRI translation entries in the appropriate places for dri2 functions to
accept them" is quite vague but a lot of text. Dense, precise git logs
are the best. Please omit the phrase, or replace it with a precise one.

At risk of over-laboring the point, short-and-sweet-and-precise like any
of the following:

* Add R10G10B10{A,X}2 translation between mesa_format and DRI format.

* Add R10G10B10{A,X}2 translation between mesa_format and DRI format
  to driGLFormatToImageFormat() and driImageFormatToGLFormat().

* Teach driGLFormatToImageFormat() and driImageFormatToGLFormat() to
  translate __DRI_IMAGE_FORMAT_ABGR2101010 and
  __DRI_IMAGE_FORMAT_XBGR2101010.

* Teach dri_util.c to translate R10G10B10{A,X}2 between mesa_format
  and DRI format.

> BUG=https://crbug.com/776093
> TEST=Compile and deploy mesa+this patch, then playback
> a VP9 Profile 2 video with sw decoder using crrev.com/c/897894.

The Chromium-specific taglines BUG= and TEST= appear nowhere in the Mesa
git log.

The BUG line should be converted to any of the following trailer lines:

Bug: https://crbug.com/776903
(This is my favorite).
Fixes: https://crbug.com/776903
(But only use Fixes if it fully fixes the bug).
Reference: https://crbug.com/776903
References: https://crbug.com/776903
(Some people use singular Reference, others plural. Shrug).

The TEST line doesn't have a clear translation. Some people would simply
add a paragraph to the commit message like this:

Tested by playing a VP9 Profile 2 video with sw
decoder using foo.

Other people try to put in a trailer line, like below. If you use
a trailer, then *please* indent wrapped lines with at least two spaces,
just like RFC 822. Read man:git-interpret-trailers(1) if want to learn
more about trailers.

Test: Play a VP9 Profile 2 video with sw
  decoder using foo.

Regardless, in the test description:

* Don't say you built and deployed the patch, *then* ran a test. If
  you ran the test, then we trust you ran it with the patch applied :-)
  Dense git log == good.

* For a test like this, it's critical to mention what GPU you
  used. If you used Eve, then saying "on Kabylake" would be
  sufficient for this patch.

* How is the VP9 video related to DRI images? Did you import each
  frame as a dma_buf into a single EGLImage? Into multiple
  EGLImages, one per plane? I don't understand how VP9 is related to
  this patch without more description.

* Whose software decoder? I don't believe the source of the VP9
  video is important to this patch. You could probably
  s/video with sw decoder/video/ without losing significant
  information. But if you think it's important to mention that the
  video was sw-decoded, then please tell us what decoder you used.

Woo... that was a lot... Thanks for your first Mesa patch!

> ---
>  src/mesa/drivers/dri/common/dri_util.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/common/dri_util.c 
> b/src/mesa/drivers/dri/common/dri_util.c
> index 7cb6248b13..78c6bbf234 100644
> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c
> @@ -886,6 +886,10 @@ driGLFormatToImageFormat(mesa_format format)
>return __DRI_IMAGE_FORMAT_ARGB2101010;
> case MESA_FORMAT_B10G10R10X2_UNORM:
>return __DRI_IMAGE_FORMAT_XRGB2101010;
> +   case MESA_FORMAT_R10G10B10A2_UNORM:
> +  return __DRI_IMAGE_FORMAT_ABGR2101010;
> +   case MESA_FORMAT_R10G10B10X2_UNORM:
> +  return __DRI_IMAGE_FORMAT_XBGR2101010;
> case MESA_FORMAT_B8G8R8A8_UNORM:
>return __DRI_IMAGE_FORMAT_ARGB;
> case MESA_FORMAT_R8G8B8A8_UNORM:
> @@ -923,6 +927,10 @@ driImageFormatToGLFormat(uint32_t image_format)
>return MESA_FORMAT_B10G10R10A2_UNORM;
> case __DRI_IMAGE_FORMAT_XRGB2101010:
>return MESA_FORMAT_B10G10R10X2_UNORM;
> +   case __DRI_IMAGE_FORMAT_ABGR2101010:
> +  return MESA_FORMAT_R10G10B10A2_UNORM;
> +   case __DRI_IMAGE_FORMAT_XBGR2101010:
> +  return 

Re: [Mesa-dev] Allocator Nouveau driver, Mesa EXT_external_objects, and DRM metadata import interfaces

[Chad Versace]

On Wed 21 Feb 2018, Daniel Vetter wrote:
> On Tue, Feb 20, 2018 at 10:14:47PM -0800, Chad Versace wrote:
> > On Thu 21 Dec 2017, Daniel Vetter wrote:
> > > On Thu, Dec 21, 2017 at 12:22 AM, Kristian Kristensen 
> > > <hoegsb...@google.com> wrote:
> > >> On Wed, Dec 20, 2017 at 12:41 PM, Miguel Angel Vico 
> > >> <mvicom...@nvidia.com> wrote:
> > >>> On Wed, 20 Dec 2017 11:54:10 -0800 Kristian Høgsberg 
> > >>> <hoegsb...@gmail.com> wrote:
> > >>>> I'd like to see concrete examples of actual display controllers
> > >>>> supporting more format layouts than what can be specified with a 64
> > >>>> bit modifier.
> > >>>
> > >>> The main problem is our tiling and other metadata parameters can't
> > >>> generally fit in a modifier, so we find passing a blob of metadata a
> > >>> more suitable mechanism.
> > >>
> > >> I understand that you may have n knobs with a total of more than a total 
> > >> of
> > >> 56 bits that configure your tiling/swizzling for color buffers. What I 
> > >> don't
> > >> buy is that you need all those combinations when passing buffers around
> > >> between codecs, cameras and display controllers. Even if you're sharing
> > >> between the same 3D drivers in different processes, I expect just locking
> > >> down, say, 64 different combinations (you can add more over time) and
> > >> assigning each a modifier would be sufficient. I doubt you'd extract
> > >> meaningful performance gains from going all the way to a blob.
> > 
> > I agree with Kristian above. In my opinion, choosing to encode in
> > modifiers a precise description of every possible tiling/compression
> > layout is not technically incorrect, but I believe it misses the point.
> > The intention behind modifiers is not to exhaustively describe all
> > possibilites.
> > 
> > I summarized this opinion in VK_EXT_image_drm_format_modifier,
> > where I wrote an "introdution to modifiers" section. Here's an excerpt:
> > 
> > One goal of modifiers in the Linux ecosystem is to enumerate for each
> > vendor a reasonably sized set of tiling formats that are appropriate for
> > images shared across processes, APIs, and/or devices, where each
> > participating component may possibly be from different vendors.
> > A non-goal is to enumerate all tiling formats supported by all vendors.
> > Some tiling formats used internally by vendors are inappropriate for
> > sharing; no modifiers should be assigned to such tiling formats.
> 
> fwiw (since the source of truth wrt modifiers is the kernel's uapi
> header):
> 
> Acked-by: Daniel Vetter <daniel.vet...@ffwll.ch>

Linux would eventually encounter big problems if the kernel and Vulkan
disagreed on the fundamental, unspoken Theory of Modifiers. So your
acked-by is definitely worth something here. Thanks for confirming.

> 
> I'm happy to merge modifier #define additions for pretty much anything
> where there's a need for sharing across devices/drivers/apis, explicitly
> including stuff that's only relevant for userspace and which the kernel
> nevers sees (in e.g. a kms addfb2 call). Trying to preemptively enumerate
> everything that's possible doesn't seem like a wise idea. But even then we
> can probably spare the oddball vendor prefix is a driver team really
> insists that this is what they want, best using some code that makes the
> case for them.

Yep. I believe Jason Ekstrand has tentative plans for such a modifier
that improves performance for interop in GL and Vulkan but the kernel
and Intel display hw wouldn't understand: a modifier for CCS_E images
that are fully compressed.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 6/6] intel/isl: Improve the documentation on get_default_aux_state

[Chad Versace]

On Wed 24 Jan 2018, Jason Ekstrand wrote:
> Cc: Chad Versace <chadvers...@chromium.org>
> ---
>  src/intel/isl/isl.h | 24 
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> index e3acb0e..cf53b5a 100644
> --- a/src/intel/isl/isl.h
> +++ b/src/intel/isl/isl.h
> @@ -1565,10 +1565,25 @@ isl_drm_modifier_has_aux(uint64_t modifier)
>  
>  /** Returns the default isl_aux_state for the given modifier.
>   *
> - * All modified images are required to be kept out of the AUX_INVALID state
> - * but they may or may not actually be compressed and may or may not have
> - * clear color.  This function returns the worst case aux_state that we need
  

I was always uncomfortable with the phrase "worst case aux state".
I like the new, precise documentation much better.

Reviewed-by: Chad Versace <chadvers...@chromium.org>

/eom

> - * to assume when getting a surface from another process or API.
> + * If we have a modifier which supports compression, then the auxiliary data
> + * could be in state other than ISL_AUX_STATE_AUX_INVALID.  In particular, it
> + * can be in any of the following:
> + *
> + *  - ISL_AUX_STATE_CLEAR
> + *  - ISL_AUX_STATE_PARTIAL_CLEAR
> + *  - ISL_AUX_STATE_COMPRESSED_CLEAR
> + *  - ISL_AUX_STATE_COMPRESSED_NO_CLEAR
> + *  - ISL_AUX_STATE_RESOLVED
> + *  - ISL_AUX_STATE_PASS_THROUGH
> + *
> + * If the modifier does not support fast-clears, then we are guaranteed
> + * that the surface is at least partially resolved and the first three not
> + * possible.  We return ISL_AUX_STATE_COMPRESSED_CLEAR if the modifier
> + * supports fast clears and ISL_AUX_STATE_COMPRESSED_NO_CLEAR if it does not
> + * because they are the least common denominator of the set of possible aux
> + * states and will yield a valid interpretation of the aux data.
> + *
> + * For modifiers with no aux support, ISL_AUX_STATE_AUX_INVALID is returned.
>   */
>  static inline enum isl_aux_state
>  isl_drm_modifier_get_default_aux_state(uint64_t modifier)
> @@ -1579,6 +1594,7 @@ isl_drm_modifier_get_default_aux_state(uint64_t 
> modifier)
> if (!mod_info || mod_info->aux_usage == ISL_AUX_USAGE_NONE)
>return ISL_AUX_STATE_AUX_INVALID;
>  
> +   assert(mod_info->aux_usage == ISL_AUX_USAGE_CCS_E);
> return mod_info->supports_clear_color ? ISL_AUX_STATE_COMPRESSED_CLEAR :
> ISL_AUX_STATE_COMPRESSED_NO_CLEAR;
>  }
> -- 
> 2.5.0.400.gff86faf
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/6] i965/state: Ignore intel_obj->_Format for depth/stencil and ETC2

[Chad Versace]

On Tue 20 Feb 2018, Jason Ekstrand wrote:
> On Mon, Feb 19, 2018 at 10:01 AM, Chad Versace <[1]chadvers...@chromium.org>
> wrote:
> 
> On Wed 24 Jan 2018, Jason Ekstrand wrote:
> > We're about to start letting the intel_obj->_Format be the "real"
> > texture format.  For depth/stencil textures, this may be a combined
> > depth stencil format.  For ETC2 on gen7 and earlier, this will be the
> > actual ETC2 format.  This makes a bit more GL sense but means we have to
> > be careful in state upload.
> 
> What is the "real" format? It's not a rhetorical question. Throughout
> Mesa, I never know what's real and what's not. By "real", do you mean
> the untranslated user-specified glTextureView(internalformat) and
> glTexImage2D(internalformat)?  Or do you mean simply "more real than
> before" ;)
> 
> 
> By "real" format, I mean the one that the core mesa state tracking code thinks
> it is.  For texture views, that corresponds directly to an actual GL internal
> format.  For textures created through glTexImage2D (not TexStorage) with an
> internal format such as GL_RGB, it's something computed from the internal
> format and the format used for upload.

I understand now. Sounds good to me.

Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] Update: Vulkan modifiers extension VK_EXT_image_drm_format_modifier

[Chad Versace]

As many of you know, I've been writing a Vulkan extension for DRM format
modifiers, named VK_EXT_image_drm_format_modifier.

The extension is very close to completion. I've submitted a branch to
Khronos for review. It's receiving active review inside Khronos from
some non-Mesa closed-source window-system-integration people, and Mesa
people too (namely jekstrand).

You should take a look at the spec if you care about modifiers and Vulkan.
I try to keep up-to-date urls to everything related to this extension at
.

There remain only two unresolved issues from my perspective:

- The exact definition of members of the array
  VkImageDrmFormatModifierExplicitCreateInfoEXT::pPlaneLayouts.
  Should the extension re-use VkSubresourceLayout as the array
  members? Or should it define a new struct with less fields than
  VkSubresourceLayout?

- If an image has a modifier that requires an extra plane (such as
  a color-compression plane), should the extension allow such an
  image to be disjoint? Specifically, if a modifier requires an
  extra plane, should the extension allow the modifier's
  drmFormatModifierTilingFeatures to contain
  VK_FORMAT_FEATURE_DISJOINT_KHR?

  I've tentatively concluded "no": images with extra planes must be
  non-disjoint. Though we could lift this restriction in a future
  extension.

Branches

I maintain a public branch of the Vulkan spec, branch
1.0-VK_EXT_image_drm_format_modifier, which is synchronized with the
Khronos-internal branch of the same name. I like cgit; other people like
Github; so I keep a mirror at both.

cgit: 
http://git.kiwitree.net/cgit/~chadv/vulkan-spec/log?h=1.0-VK_EXT_image_drm_format_modifier
github: 
https://github.com/chadversary/vulkan-spec/commits/1.0-VK_EXT_image_drm_format_modifier
khronos-internal: 
https://gitlab.khronos.org/vulkan/vulkan/merge_requests/2555


Prebuilt Specs
==
I maintain a public build of the branch. The built headers and HTML
specification are synchronized with the git branch thanks to the magic
of shell scripts.

vulkan.h: 
http://git.kiwitree.net/cgit/~chadv/vulkan-spec/tree/src/vulkan/vulkan.h?h=1.0-VK_EXT_image_drm_format_modifier
spec appendix: 
http://kiwitree.net/~chadv/vulkan/1.0-VK_EXT_image_drm_format_modifier/html/vkspec.html#VK_EXT_image_drm_format_modifier
full spec: 
http://kiwitree.net/~chadv/vulkan/1.0-VK_EXT_image_drm_format_modifier/html/vkspec.html


Where to start reading
==
Here's a short reading guide for people unfamiliar with the extension:

- Don't start with the specification. You'll quickly get lost.

- First, read the VK_EXT_external_memory_dma_buf and
  VK_EXT_queue_family_foreign extensions. They're small extensions.
  They're intended to be used with VK_EXT_image_drm_format_modifier.
  Despite that intent, the the three extensions are independent from
  the Vulkan specification's perspective.

- Read the appendix chapter for VK_EXT_image_drm_format_modifier.
  I've written an "introduction to modifiers" section in the
  appendix. If you're already intimately understand modifiers, then
  you can briefly scan this section, skipping over the boring stuff.

- Read the issue section in the appendix.

- Now for the tofu. Study the new structs and functions. Find them
  under `#define VK_EXT_image_drm_format_modifier` in vulkan.h.
  Also, study the new enums values. Find them by grepping 'DRM.*EXT'
  in vulkan.h.

- Finally, go read the specification text for the new structs,
  functions, and enums.


How to send feedback

I honestly don't know. You _could_ comment on the merge request
in the Khronos-internal Gitlab. But you probably (and rightfully so)
want to keep the discussion public.

You could provide general feedback by replying to this thread.

You could leave comments on my Github branch. I don't like Github, but
I can't think of a better solution, other than...

I could send my specification patches to mesa-dev. If people want that,
say so.

So... yeah. I don't know how you should provide feedback. Just do it,
and we'll iron out any problems as they arise.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Allocator Nouveau driver, Mesa EXT_external_objects, and DRM metadata import interfaces

[Chad Versace]

On Thu 21 Dec 2017, Daniel Vetter wrote:
> On Thu, Dec 21, 2017 at 12:22 AM, Kristian Kristensen  
> wrote:
>> On Wed, Dec 20, 2017 at 12:41 PM, Miguel Angel Vico  
>> wrote:
>>> On Wed, 20 Dec 2017 11:54:10 -0800 Kristian Høgsberg  
>>> wrote:
 I'd like to see concrete examples of actual display controllers
 supporting more format layouts than what can be specified with a 64
 bit modifier.
>>>
>>> The main problem is our tiling and other metadata parameters can't
>>> generally fit in a modifier, so we find passing a blob of metadata a
>>> more suitable mechanism.
>>
>> I understand that you may have n knobs with a total of more than a total of
>> 56 bits that configure your tiling/swizzling for color buffers. What I don't
>> buy is that you need all those combinations when passing buffers around
>> between codecs, cameras and display controllers. Even if you're sharing
>> between the same 3D drivers in different processes, I expect just locking
>> down, say, 64 different combinations (you can add more over time) and
>> assigning each a modifier would be sufficient. I doubt you'd extract
>> meaningful performance gains from going all the way to a blob.

I agree with Kristian above. In my opinion, choosing to encode in
modifiers a precise description of every possible tiling/compression
layout is not technically incorrect, but I believe it misses the point.
The intention behind modifiers is not to exhaustively describe all
possibilites.

I summarized this opinion in VK_EXT_image_drm_format_modifier,
where I wrote an "introdution to modifiers" section. Here's an excerpt:

One goal of modifiers in the Linux ecosystem is to enumerate for each
vendor a reasonably sized set of tiling formats that are appropriate for
images shared across processes, APIs, and/or devices, where each
participating component may possibly be from different vendors.
A non-goal is to enumerate all tiling formats supported by all vendors.
Some tiling formats used internally by vendors are inappropriate for
sharing; no modifiers should be assigned to such tiling formats.

> Tegra just redesigned it's modifier space from an ungodly amount of
> bits to just a few layouts. Not even just the ones in used, but simply
> limiting to the ones that make sense (there's dependencies apparently)
> Also note that the modifier alone doesn't need to describe the layout
> precisely, it only makes sense together with a specific pixel format
> and size. E.g. a bunch of the i915 layouts change layout depending
> upon bpp.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/6] i965/tex_image: Reference the renderbuffer miptree in setTexBuffer2

[Chad Versace]

On Wed 24 Jan 2018, Jason Ekstrand wrote:
> The old code made a new miptree that referenced the same BO as the
> renderbuffer and just trusted in the memory aliasing to work.  There are
> only two ways in which the new miptree is liable to differ from the one
> in the renderbuffer and neither of them matter:
> 
>  1) It may have a different target.  The only targets that we can ever
> see in intelSetTexBuffer2 are GL_TEXTURE_2D and GL_TEXTURE_RECTANGLE
> and the difference between the two doesn't matter as far as the
> miptree is concerned; genX(update_sampler_state) only looks at the
> gl_texture_object and not the miptree when determining whether or
> not to use normalized coordinates.
> 
>  2) It may have a very slightly different format.  Again, this doesn't
> matter because we've supported texture views for quite some time so
> we always look at the gl_texture_object format instead of the
> miptree format for hardware setup anyway.
> 
> On the other hand, because we were recreating the miptree, we were using
> intel_miptree_create_for_bo which doesn't understand modifiers.  We
> really want this function to work without doing a resolve so long as you
> have modifiers so we need to fix that.
> 
> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
> Cc: "17.3" <mesa-sta...@lists.freedesktop.org>
> ---
>  src/mesa/drivers/dri/i965/intel_tex_image.c | 19 +--
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c 
> b/src/mesa/drivers/dri/i965/intel_tex_image.c
> index 9d2ede1..a4e7f81 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
> @@ -405,6 +405,7 @@ static void
>  intel_set_texture_image_mt(struct brw_context *brw,
> struct gl_texture_image *image,
> GLenum internal_format,
> +   mesa_format format,
> struct intel_mipmap_tree *mt)
>  
>  {
> @@ -415,7 +416,7 @@ intel_set_texture_image_mt(struct brw_context *brw,
> _mesa_init_teximage_fields(>ctx, image,
>mt->surf.logical_level0_px.width,
>mt->surf.logical_level0_px.height, 1,
> -  0, internal_format, mt->format);
> +  0, internal_format, format);
>  
> brw->ctx.Driver.FreeTextureImageBuffer(>ctx, image);
>  
> @@ -442,7 +443,6 @@ intelSetTexBuffer2(__DRIcontext *pDRICtx, GLint target,
> struct gl_texture_object *texObj;
> struct gl_texture_image *texImage;
> mesa_format texFormat = MESA_FORMAT_NONE;
> -   struct intel_mipmap_tree *mt;
> GLenum internal_format = 0;
>  
> texObj = _mesa_get_current_tex_object(ctx, target);
> @@ -488,20 +488,11 @@ intelSetTexBuffer2(__DRIcontext *pDRICtx, GLint target,
> }
>  
> intel_miptree_make_shareable(brw, rb->mt);
> -   mt = intel_miptree_create_for_bo(brw, rb->mt->bo, texFormat, 0,
> -rb->Base.Base.Width,
> -rb->Base.Base.Height,
> -1, rb->mt->surf.row_pitch,
> -rb->mt->surf.tiling,
> -MIPTREE_CREATE_DEFAULT);
> -   if (mt == NULL)
> -   return;
> -   mt->target = target;
>  
> _mesa_lock_texture(>ctx, texObj);
> texImage = _mesa_get_tex_image(ctx, texObj, target, 0);
> -   intel_set_texture_image_mt(brw, texImage, internal_format, mt);
> -   intel_miptree_release();
> +   intel_set_texture_image_mt(brw, texImage, internal_format,
> +  texFormat, rb->mt);
> _mesa_unlock_texture(>ctx, texObj);
>  }
>  
> @@ -594,7 +585,7 @@ intel_image_target_texture_2d(struct gl_context *ctx, 
> GLenum target,
> const GLenum internal_format =
>image->internal_format != 0 ?
>image->internal_format : _mesa_get_format_base_format(mt->format);
> -   intel_set_texture_image_mt(brw, texImage, internal_format, mt);
> +   intel_set_texture_image_mt(brw, texImage, internal_format, mt->format, 
> mt);

I was unsure whether this hunk should've used mt->format or image->format.
After digging, I discovered that for EGLImages, image->format should == 
mt->format except
(1) for depth-stencil formats, which we don't care about and (2) when
intel_miptree_create_for_dri_image() does a rgbx->rgba fallback, in
which case we definitely want mt->format instead of image->format.

The hunk looks correct to me.

Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/6] i965/state: Ignore intel_obj->_Format for depth/stencil and ETC2

[Chad Versace]

On Wed 24 Jan 2018, Jason Ekstrand wrote:
> We're about to start letting the intel_obj->_Format be the "real"
> texture format.  For depth/stencil textures, this may be a combined
> depth stencil format.  For ETC2 on gen7 and earlier, this will be the
> actual ETC2 format.  This makes a bit more GL sense but means we have to
> be careful in state upload.

What is the "real" format? It's not a rhetorical question. Throughout
Mesa, I never know what's real and what's not. By "real", do you mean
the untranslated user-specified glTextureView(internalformat) and
glTexImage2D(internalformat)?  Or do you mean simply "more real than
before" ;)

> ---
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c 
> b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> index 38af6bc..844c23b 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -507,7 +507,21 @@ brw_update_texture_surface(struct gl_context *ctx,
>const unsigned swizzle = (unlikely(alpha_depth) ? SWIZZLE_XYZW :
>  brw_get_texture_swizzle(>ctx, obj));
>  
> -  mesa_format mesa_fmt = plane == 0 ? intel_obj->_Format : mt->format;
> +  mesa_format mesa_fmt;
> +  if (firstImage->_BaseFormat == GL_DEPTH_STENCIL ||
> +  firstImage->_BaseFormat == GL_DEPTH_COMPONENT) {
> + /* The format from intel_obj may be a combined depth stencil format
> +  * when we just want depth.  Pull it from the miptree instead.  This
> +  * is safe because texture views aren't allowed on depth/stencil.
> +  */
> + mesa_fmt = mt->format;
> +  } else if (mt->etc_format != MESA_FORMAT_NONE) {
> + mesa_fmt = mt->format;

This looks like it would break ETC2 texture views on hw where we decode
the ETC2 on upload (Ivybridge?), if such views worked. I suspect such
texture views never worked.

> +  } else if (plane > 0) {
> + mesa_fmt = mt->format;
> +  } else {
> + mesa_fmt = intel_obj->_Format;
> +  }
>enum isl_format format = translate_tex_format(brw, mesa_fmt,
>  for_txf ? GL_DECODE_EXT :
>  sampler->sRGBDecode);

I want to give a r-b, but want to first hear your reply regarding the
"real" format.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vulkan: Update the XML and headers to 1.0.68

[Chad Versace]

On Wed 24 Jan 2018, Jason Ekstrand wrote:
> ---
>  include/vulkan/vulkan.h| 54 ---
>  src/vulkan/registry/vk.xml | 91 
> +-
>  2 files changed, 130 insertions(+), 15 deletions(-)

Acked-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 09/32] anv: Require a dedicated allocation for modified images

[Chad Versace]

On Wed 29 Nov 2017, Jason Ekstrand wrote:
> On Wed, Nov 29, 2017 at 2:24 PM, Chad Versace <[1]chadvers...@chromium.org>
> wrote:
> 
> On Tue 28 Nov 2017, Jason Ekstrand wrote:
> > This lets us set the BO tiling when we allocate the memory.  This is
> > required for GL to work properly.
> > ---
> >  src/intel/vulkan/anv_device.c | 53 ++
> +
> >  1 file changed, 49 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_device.c 
> b/src/intel/vulkan/anv_device.
> c
> > index df929e4..d82d1f7 100644
> > --- a/src/intel/vulkan/anv_device.c
> > +++ b/src/intel/vulkan/anv_device.c
> > @@ -29,6 +29,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include "anv_private.h"
> >  #include "util/strtod.h"
> > @@ -1612,6 +1613,40 @@ VkResult anv_AllocateMemory(
> >                                    >bo);
> >        if (result != VK_SUCCESS)
> >           goto fail;
> > +
> > +      const VkMemoryDedicatedAllocateInfoKHR *dedicated_info =
> > +         vk_find_struct_const(pAllocateInfo->pNext,
> MEMORY_DEDICATED_ALLOCATE_INFO_KHR);
> > +      if (dedicated_info && dedicated_info->image != VK_NULL_HANDLE) {
> > +         ANV_FROM_HANDLE(anv_image, image, dedicated_info->image);
> > +
> > +         /* For images using modifiers, we require a dedicated
> allocation
> > +          * and we set the BO tiling to match the tiling of the
> underlying
> > +          * modifier.  This is a bit unfortunate as this is completely
> > +          * pointless for Vulkan.  However, GL needs to be able to map
> things
> > +          * so it needs the tiling to be set.  The only way to do this
> in a
> > +          * non-racy way is to set the tiling in the creator of the BO
> so that
> > +          * makes it our job.
> > +          *
> > +          * One of these days, once the GL driver learns to not map
> things
> > +          * through the GTT in random places, we can drop this and 
> start
> > +          * allowing multiple modified images in the same BO.
> > +          */
> > +         if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) {
> > +            assert(isl_drm_modifier_get_info(image->drm_format_mod)->
> tiling ==
> > +                   image->planes[0].surface.isl.tiling);
> > +            const uint32_t i915_tiling =
> > +               isl_tiling_to_i915_tiling(image->planes[0].surface.isl.
> tiling);
> > +            int ret = anv_gem_set_tiling(device, mem->bo->gem_handle,
> > +                                         image->planes[0].surface.isl.
> row_pitch,
> > +                                         i915_tiling);
> > +            if (ret) {
> > +               anv_bo_cache_release(device, >bo_cache, 
> mem->bo);
> > +               return vk_errorf(device->instance, NULL,
> > +                                VK_ERROR_OUT_OF_DEVICE_MEMORY,
> > +                                "failed to set BO tiling: %m");
> > +            }
> > +         }
> > +      }
> >     }
> 
> This hunk of code is run only for non-imported memory. It's in the
> 'else' branch of `if (fd_info && fd_info->handleTypes)`. I believe it
> needs to be run in both branches. Otherwise, if an app creates
> a dma_buf, imports it into Vulkan, binds the image to the dma_buf, and
> afterwards imports the same dma_buf into GL as an EGLImage, then
> intel_create_image_from_fds_common() will fail to discover the bo's
> tiling.
> 
> 
> Yes, that was a bit intentional because setting it any time other than image
> creation is racy.  If we were going to set it on import as well, we may as 
> well
> just do that in the GL driver and call it a day.
>  
> 
> Everything else in this patch looks correct to me, even though it makes
> me sad.

I started writing a mini-essay in reply to this patch. Since that
reply's summary is/was

In general, for the upcoming modifiers extension, this patch is
incorrect. But, for the limited usecase of the pseudo WSI layer,
everything should work correctly.

then I'm ok with landing this patch as-is in this series.

Reviewed-by: Chad Versace <chadvers...@chromium.org>

But we should chat later (not while I'm making breakfast for my kid
(which is right now)) about the best way to fix the general problem,
which becomes real when VK_EXT_image_drm_format_modifier arrives.

I think I've reviewed all the patches now. If I've forgotten any, let me
know... or just push anyway.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 00/32] vulkan/wsi: Rework WSI to look a lot more like a layer

[Chad Versace]

On Wed 29 Nov 2017, Dave Airlie wrote:
> On 29 November 2017 at 10:28, Jason Ekstrand  wrote:
> > This patch series is a v2 of the one I sent out a couple weeks ago to
> > rewrite Vulkan window system integration.  The original patch series can be
> > found on patchwork here:
> >
> > https://patchwork.freedesktop.org/series/33961/
> >
> > This series can be found as a git branch here:
> >
> > https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=review/vulkan-wsi-rewrite-v2
> >
> > Notable changes in v2:
> 
> Thanks for finished this off and making it more presentable,
> 
> You might want to wait for Daniel or Chad to look some of it over, but
> I'm happy to
> give
> 
> Reviewed-by: Dave Airlie 
> 
> for the whole series, it really makes the WSI and prime into a contained 
> space,
> and is what I wished the initial modifier series had looked like :-)

I really like the clean seperation created by this series. I hope the
Android winsys extension (and the upcoming one too) can be rewritten
similarly and placed into src/vulkan/android, so anv and radv can share
it.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 32/32] radv: Implement VK_KHR_get_surface_capabilities2

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> The WSI core code does all the hard work.  Just add the wrappers and
> turn it on.
> ---
>  src/amd/vulkan/radv_extensions.py |  1 +
>  src/amd/vulkan/radv_wsi.c | 26 ++
>  2 files changed, 27 insertions(+)

Reviewed-by: Chad Versace <chadvers...@chromium.org>

This is the end of the series, but I've left a few patches unreviewed.
I'll revisit them now.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 31/32] vulkan/wsi: Initialize individual WSI interfaces in wsi_device_init

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> Now that we have anv_device_init/finish functions, there's no reason to
> have the individual driver do any more work than that.

You mean "wsi_device_init/finish" functions.

> ---
>  src/amd/vulkan/radv_wsi.c   | 36 ++--
>  src/intel/vulkan/anv_wsi.c  | 36 ++--
>  src/vulkan/wsi/wsi_common.c | 37 
> +++--
>  src/vulkan/wsi/wsi_common.h | 19 +++
>  src/vulkan/wsi/wsi_common_private.h | 10 ++
>  5 files changed, 64 insertions(+), 74 deletions(-)


> -void
> +VkResult
>  wsi_device_init(struct wsi_device *wsi,
>  VkPhysicalDevice pdevice,
> -WSI_FN_GetPhysicalDeviceProcAddr proc_addr)
> +WSI_FN_GetPhysicalDeviceProcAddr proc_addr,
> +const VkAllocationCallbacks *alloc)
>  {
> +   VkResult result;
> +
> memset(wsi, 0, sizeof(*wsi));
>  
>  #define WSI_GET_CB(func) \
> @@ -69,6 +72,36 @@ wsi_device_init(struct wsi_device *wsi,
> WSI_GET_CB(QueueSubmit);
> WSI_GET_CB(WaitForFences);
>  #undef WSI_GET_CB
> +
> +#ifdef VK_USE_PLATFORM_XCB_KHR
> +   result = wsi_x11_init_wsi(wsi, alloc);
> +   if (result != VK_SUCCESS)
> +  return result;
> +#endif
> +
> +#ifdef VK_USE_PLATFORM_WAYLAND_KHR
> +   result = wsi_wl_init_wsi(wsi, alloc, pdevice);
> +   if (result != VK_SUCCESS) {
> +#ifdef VK_USE_PLATFORM_XCB_KHR
> +  wsi_x11_finish_wsi(wsi, alloc);
> +#endif
> +  return result;
> +   }
> +#endif

This looks like a bug. wsi_device_init() fails if either of x11 or
wayland initialization fails, even if the user requested only one of
VK_KHR_{xcb,xlib,wayland}_surface.

I filed https://bugs.freedesktop.org/show_bug.cgi?id=104039 .

But this patch is just a refactor patch, and it preserves the existing
(and possibly buggy) behavior. With the commit message fixed,
Reviewed-by: Chad Versace <chadvers...@chromium.org>


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 29/32] vulkan/wsi: Add wrappers for all of the surface queries

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> This lets us move wsi_interface to wsi_common_private.h
> ---
>  src/amd/vulkan/radv_wsi.c   | 41 ++--
>  src/intel/vulkan/anv_wsi.c  | 51 +++-
>  src/vulkan/wsi/wsi_common.c | 77 
> +
>  src/vulkan/wsi/wsi_common.h | 72 +-
>  src/vulkan/wsi/wsi_common_private.h | 34 
>  5 files changed, 192 insertions(+), 83 deletions(-)

Patches 28 & 29 are
Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 27/32] vulkan/wsi: Move wsi_swapchain to wsi_common_private.h

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> The drivers no longer poke at this directly.
> ---
>  src/vulkan/wsi/wsi_common.h | 46 
> +
>  src/vulkan/wsi/wsi_common_private.h | 46 
> +
>  2 files changed, 47 insertions(+), 45 deletions(-)

Yay. It's becoming more layer-like.

Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 26/32] vulkan/wsi: Add a helper for AcquireNextImage

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> Unfortunately, due to the fact that AcquireNextImage does not take a
> queue, the ANV trick for triggering the fence won't work in general.  We
> leave dealing with the fence up to the caller for now.
> ---
>  src/amd/vulkan/radv_wsi.c   | 15 ++-
>  src/intel/vulkan/anv_wsi.c  | 19 +++
>  src/vulkan/wsi/wsi_common.c | 14 ++
>  src/vulkan/wsi/wsi_common.h |  8 
>  4 files changed, 43 insertions(+), 13 deletions(-)

Patch 26 is
Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 25/32] vulkan/wsi: move swapchain create/destroy to common code

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> From: Dave Airlie <airl...@redhat.com>
> 
> v2 (Jason Ekstrand):
>  - Rebase
>  - Alter the names of the helpers to better match the vulkan entrypoints
>  - Use the helpers in anv
> ---
>  src/amd/vulkan/radv_wsi.c   | 42 --
>  src/intel/vulkan/anv_wsi.c  | 35 +--
>  src/vulkan/wsi/wsi_common.c | 38 ++
>  src/vulkan/wsi/wsi_common.h | 12 
>  4 files changed, 63 insertions(+), 64 deletions(-)

> @@ -115,6 +115,9 @@ fail:
>  void
>  wsi_swapchain_finish(struct wsi_swapchain *chain)
>  {
> +   for (unsigned i = 0; i < ARRAY_SIZE(chain->fences); i++)
> +  chain->wsi->DestroyFence(chain->device, chain->fences[i], 
> >alloc);
> +

Yes. I've been waiting to see DestroyFence here since patch 17.

> for (uint32_t i = 0; i < chain->wsi->queue_family_count; i++) {
>chain->wsi->DestroyCommandPool(chain->device, chain->cmd_pools[i],
>   >alloc);
> @@ -485,6 +488,41 @@ wsi_destroy_image(const struct wsi_swapchain *chain,

Patch 25 is
Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 23/32] vulkan/wsi: Move get_images into common code

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> This moves bits out of all four corners (anv, radv, x11, wayland) and
> into the wsi common code.  We also switch to using an outarray to ensure
> we get our return code right.
> ---
>  src/amd/vulkan/radv_wsi.c   |  7 +++
>  src/intel/vulkan/anv_wsi.c  |  7 +++
>  src/vulkan/wsi/wsi_common.c | 17 +
>  src/vulkan/wsi/wsi_common.h |  9 +++--
>  src/vulkan/wsi/wsi_common_wayland.c | 28 +---
>  src/vulkan/wsi/wsi_common_x11.c | 29 +
>  6 files changed, 40 insertions(+), 57 deletions(-)

Patches 22 and 23 are
Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 20/32] vulkan/wsi: Set a proper pWaitDstStageMask on the dummy submit

[Chad Versace]

On Fri 01 Dec 2017, Chad Versace wrote:
> On Tue 28 Nov 2017, Jason Ekstrand wrote:
> > Neither mesa driver really cares, but we should set it none the less for
> > the sake of correctness.
> > ---
> >  src/vulkan/wsi/wsi_common.c | 17 +
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/src/vulkan/wsi/wsi_common.c b/src/vulkan/wsi/wsi_common.c
> > index 4f6648f..e5a9a28 100644
> > --- a/src/vulkan/wsi/wsi_common.c
> > +++ b/src/vulkan/wsi/wsi_common.c
> > @@ -542,14 +542,31 @@ wsi_common_queue_present(const struct wsi_device *wsi,
> >   .sType = VK_STRUCTURE_TYPE_SUBMIT_INFO,
> >   .pNext = NULL,
> >};
> > +  VkPipelineStageFlags *stage_flags = NULL;
> >if (i == 0) {
> >   /* We only need/want to wait on semaphores once.  After that, 
> > we're
> >* guaranteed ordering since it all happens on the same queue.
> >*/
> >   submit_info.waitSemaphoreCount = pPresentInfo->waitSemaphoreCount,
> >   submit_info.pWaitSemaphores = pPresentInfo->pWaitSemaphores,
> > +
> > + /* Set up the pWaitDstStageMasks */
> > + stage_flags = vk_alloc(>alloc,
> > +sizeof(VkPipelineStageFlags) *
> > +pPresentInfo->waitSemaphoreCount,
> > +8,
> > +VK_SYSTEM_ALLOCATION_SCOPE_COMMAND);
> > + if (!stage_flags) {
> > +result = VK_ERROR_OUT_OF_HOST_MEMORY;
> > +goto fail_present;
> > + }
> > + for (uint32_t s = 0; s < pPresentInfo->waitSemaphoreCount; s++)
> > +stage_flags[s] = VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT;
> > +
> > + submit_info.pWaitDstStageMask = stage_flags;
> 
> Since VkSwapchain is required to be externally synchronized, you could
> embed stage_flags directly in struct wsi_swapchain, doubling its size
> when needed. But meh.

Since stage_flags gets freed at the end of the function, you could make
the code simpler by using the stack.

VkPipelineStageFlags stage_flags[submit_info.waitSemaphoreCount];

But some people don't like stack-allocated arrays.

> Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 20/32] vulkan/wsi: Set a proper pWaitDstStageMask on the dummy submit

[Chad Versace]

On Fri 01 Dec 2017, Chad Versace wrote:
> On Tue 28 Nov 2017, Jason Ekstrand wrote:
> > Neither mesa driver really cares, but we should set it none the less for
> > the sake of correctness.
> > ---
> >  src/vulkan/wsi/wsi_common.c | 17 +
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/src/vulkan/wsi/wsi_common.c b/src/vulkan/wsi/wsi_common.c
> > index 4f6648f..e5a9a28 100644
> > --- a/src/vulkan/wsi/wsi_common.c
> > +++ b/src/vulkan/wsi/wsi_common.c
> > @@ -542,14 +542,31 @@ wsi_common_queue_present(const struct wsi_device *wsi,
> >   .sType = VK_STRUCTURE_TYPE_SUBMIT_INFO,
> >   .pNext = NULL,
> >};
> > +  VkPipelineStageFlags *stage_flags = NULL;
> >if (i == 0) {
> >   /* We only need/want to wait on semaphores once.  After that, 
> > we're
> >* guaranteed ordering since it all happens on the same queue.
> >*/
> >   submit_info.waitSemaphoreCount = pPresentInfo->waitSemaphoreCount,
> >   submit_info.pWaitSemaphores = pPresentInfo->pWaitSemaphores,
> > +
> > + /* Set up the pWaitDstStageMasks */
> > + stage_flags = vk_alloc(>alloc,
> > +sizeof(VkPipelineStageFlags) *
> > +pPresentInfo->waitSemaphoreCount,
> > +8,
> > +VK_SYSTEM_ALLOCATION_SCOPE_COMMAND);
> > + if (!stage_flags) {
> > +result = VK_ERROR_OUT_OF_HOST_MEMORY;
> > +goto fail_present;
> > + }
> > + for (uint32_t s = 0; s < pPresentInfo->waitSemaphoreCount; s++)
> > +stage_flags[s] = VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT;
> > +
> > +     submit_info.pWaitDstStageMask = stage_flags;
> 
> Since VkSwapchain is required to be externally synchronized, you could
> embed stage_flags directly in struct wsi_swapchain, doubling its size
> when needed. But meh.
> 
> Reviewed-by: Chad Versace <chadvers...@chromium.org>

I have to quit for the day. I wish I could've completed all the review
by end-of-week, but I've had a busy two days preparing today's Chrome OS
release branchpoint. I'll continue reviewing over breakfast tomorrow,
but don't let me hinder the series. Push it when you're ready; don't let
my slowness become your slowness.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 21/32] anv/wsi: Use the common QueuePresent code

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> ---
>  src/intel/vulkan/anv_wsi.c | 63 
> +-
>  1 file changed, 6 insertions(+), 57 deletions(-)

Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 20/32] vulkan/wsi: Set a proper pWaitDstStageMask on the dummy submit

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> Neither mesa driver really cares, but we should set it none the less for
> the sake of correctness.
> ---
>  src/vulkan/wsi/wsi_common.c | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/src/vulkan/wsi/wsi_common.c b/src/vulkan/wsi/wsi_common.c
> index 4f6648f..e5a9a28 100644
> --- a/src/vulkan/wsi/wsi_common.c
> +++ b/src/vulkan/wsi/wsi_common.c
> @@ -542,14 +542,31 @@ wsi_common_queue_present(const struct wsi_device *wsi,
>   .sType = VK_STRUCTURE_TYPE_SUBMIT_INFO,
>   .pNext = NULL,
>};
> +  VkPipelineStageFlags *stage_flags = NULL;
>if (i == 0) {
>   /* We only need/want to wait on semaphores once.  After that, we're
>* guaranteed ordering since it all happens on the same queue.
>*/
>   submit_info.waitSemaphoreCount = pPresentInfo->waitSemaphoreCount,
>   submit_info.pWaitSemaphores = pPresentInfo->pWaitSemaphores,
> +
> + /* Set up the pWaitDstStageMasks */
> + stage_flags = vk_alloc(>alloc,
> +sizeof(VkPipelineStageFlags) *
> +pPresentInfo->waitSemaphoreCount,
> +8,
> +VK_SYSTEM_ALLOCATION_SCOPE_COMMAND);
> + if (!stage_flags) {
> +result = VK_ERROR_OUT_OF_HOST_MEMORY;
> +goto fail_present;
> + }
> + for (uint32_t s = 0; s < pPresentInfo->waitSemaphoreCount; s++)
> +stage_flags[s] = VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT;
> +
> + submit_info.pWaitDstStageMask = stage_flags;

Since VkSwapchain is required to be externally synchronized, you could
embed stage_flags directly in struct wsi_swapchain, doubling its size
when needed. But meh.

Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 19/32] vulkan/wsi: Only wait on semaphores on the first swapchain

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> ---
>  src/vulkan/wsi/wsi_common.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Smart. I was expecting a patch like this after reading patch 17.
Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 18/32] vulkan/wsi: Refactor result handling in queue_present

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> ---
>  src/vulkan/wsi/wsi_common.c | 54 
> +++--
>  1 file changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/src/vulkan/wsi/wsi_common.c b/src/vulkan/wsi/wsi_common.c
> index 5920359..f149846 100644
> --- a/src/vulkan/wsi/wsi_common.c
> +++ b/src/vulkan/wsi/wsi_common.c
> @@ -514,14 +514,14 @@ wsi_common_queue_present(const struct wsi_device *wsi,
>   int queue_family_index,
>   const VkPresentInfoKHR *pPresentInfo)
>  {
> -   VkResult result = VK_SUCCESS;
> +   VkResult final_result = VK_SUCCESS;

Yes please. The goto improves this code.

Reviewed-by: Chad Versace <chadvers...@chromium.org>



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 17/32] radv/wsi: Move the guts of QueuePresent to wsi common

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> From: Dave Airlie <airl...@redhat.com>
> 
> v2 (Jason Ekstrand):
>  - Better comit message
>  - Rebase
>  - Re-indent to follow wsi_common style
>  - Drop the unneeded _swapchain from the newly added helper
>  - Make the clone more true to the original (as per the rebase)
> ---
>  src/amd/vulkan/radv_wsi.c   | 92 
> +++--
>  src/vulkan/wsi/wsi_common.c | 78 ++
>  src/vulkan/wsi/wsi_common.h | 10 +
>  3 files changed, 93 insertions(+), 87 deletions(-)

> +VkResult
> +wsi_common_queue_present(const struct wsi_device *wsi,
> + VkDevice device,
> + VkQueue queue,
> + int queue_family_index,
> + const VkPresentInfoKHR *pPresentInfo)
> +{
> +   VkResult result = VK_SUCCESS;
> +
> +   const VkPresentRegionsKHR *regions =
> +  vk_find_struct_const(pPresentInfo->pNext, PRESENT_REGIONS_KHR);
> +
> +   for (uint32_t i = 0; i < pPresentInfo->swapchainCount; i++) {
> +  WSI_FROM_HANDLE(wsi_swapchain, swapchain, 
> pPresentInfo->pSwapchains[i]);
> +  VkResult item_result;
> +
> +  if (swapchain->fences[0] == VK_NULL_HANDLE) {
> + const VkFenceCreateInfo fence_info = {
> +.sType = VK_STRUCTURE_TYPE_FENCE_CREATE_INFO,
> +.pNext = NULL,
> +.flags = 0,
> + };
> + item_result = wsi->CreateFence(device, _info,
> +>alloc,
> +>fences[0]);

As part of moving fence creation to the common code, the fence
destruction should also be moved from (anv|radv)_DestroySwapchainKHR to
wsi_swapchain_finish(). But, since that migration doesn't affect the
correctness of the patch, this patch is

Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 16/32] vulkan/wsi: Add a WSI_FROM_HANDLE macro

[Chad Versace]

Patches 15 and 16 are
Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 14/32] vulkan/wsi: Do image creation in common code

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> This uses the mock extension created in a previous commit to tell the
> driver that the image it's just been asked to create is, in fact, a
> window system image with whatever assumptions that implies.  There was a
> lot of redundant code between the two drivers to do basically exactly
> the same thing.
> ---
>  src/amd/vulkan/radv_wsi.c   | 124 
> +---
>  src/intel/vulkan/anv_wsi.c  | 122 +--
>  src/vulkan/wsi/wsi_common.c | 117 +-
>  src/vulkan/wsi/wsi_common.h |  28 +---
>  src/vulkan/wsi/wsi_common_private.h |  25 +++-
>  src/vulkan/wsi/wsi_common_wayland.c |  13 +---
>  src/vulkan/wsi/wsi_common_x11.c |  20 +-
>  7 files changed, 146 insertions(+), 303 deletions(-)

This patch is
Reviewed-by: Chad Versace <chadvers...@chromium.org>

This series is a good improvement over the old code :)

But I found some nits:

- The Vulkan spec, iirc, requires that vkCreateFoo not modify its
  output parameter on failure. The memset in this patch breaks that
  requirement. I'm sure we're breaking that elsewhere too. We should
  probably do a cleanup one day to fix that.

- wsi_destroy_image neither assets that image->fd == -1 nor closes
  it. I believe there is no fd leakage because anv_wsi* does the
  right thing. But that's a small cleanup we could do later.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 12/32] radv: Move wsi initialization later in physical_device_init

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> We need it to happen after memory type setup so that we can query memory
> types in wsi_device_init.
> ---
>  src/amd/vulkan/radv_device.c | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)

Patches 10-12 are
Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 07/32] radv: Implement VK_EXT_external_memory_dma_buf

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> ---
>  src/amd/vulkan/radv_device.c  | 8 ++--
>  src/amd/vulkan/radv_extensions.py | 1 +
>  src/amd/vulkan/radv_formats.c | 8 ++--
>  3 files changed, 13 insertions(+), 4 deletions(-)

With the changes in your wip/vulkan-wsi-prime branch, patches 6 & 7 are
Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 05/32] vulkan/wsi: Add a mock image creation extension

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> ---
>  src/vulkan/wsi/wsi_common.h | 18 ++
>  1 file changed, 18 insertions(+)

Patches 1-5 are
Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 13/32] vulkan/wsi: Implement prime in a completely generic way

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> ---
>  src/amd/vulkan/radv_wsi.c   | 137 +++
>  src/intel/vulkan/anv_wsi.c  |  14 +-
>  src/vulkan/wsi/wsi_common.c | 341 
> +++-
>  src/vulkan/wsi/wsi_common.h |  54 +-
>  src/vulkan/wsi/wsi_common_private.h |  16 ++
>  src/vulkan/wsi/wsi_common_wayland.c |   6 +-
>  src/vulkan/wsi/wsi_common_x11.c |  87 +
>  7 files changed, 475 insertions(+), 180 deletions(-)
> 
> diff --git a/src/amd/vulkan/radv_wsi.c b/src/amd/vulkan/radv_wsi.c
> index 247f7cc..589eb5c 100644
> --- a/src/amd/vulkan/radv_wsi.c
> +++ b/src/amd/vulkan/radv_wsi.c
> @@ -40,6 +40,13 @@ radv_wsi_proc_addr(VkPhysicalDevice physicalDevice, const 
> char *pName)
>   return radv_lookup_entrypoint(pName);
>  }
>  
> +static uint32_t
> +anv_wsi_queue_get_family_index(VkQueue _queue)

This function should have the 'radv' prefix, not 'anv'.

Other than that, this patch is
Reviewed-by: Chad Versace <chadvers...@chromium.org>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 00/12] anv: Add support for the variablePointers feature

[Chad Versace]

On Mon 06 Nov 2017, Jason Ekstrand wrote:
> On Mon, Nov 6, 2017 at 7:26 PM, Jason Ekstrand <[1]ja...@jlekstrand.net> 
> wrote:

> The tests are fixed in CL #1915.  I feel like a dork now...

The CL is still languishing in Gerrit. fyi, I've pushed a branch with
the CL.

http://git.kiwitree.net/cgit/~chadv/vk-gl-cts/log/?h=pu

fwiw, this series is
Tested-by: Chad Versace <chadvers...@chromium.org>

btw, if you'd like to see the branch where I'm integrating this into
ARC++, it's


http://git.kiwitree.net/cgit/~chadv/mesa/log/?h=wip/arc-17.3-anv-variable-pointers
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev