[Mesa-dev] [PATCH 1/5] egl/android: Declare loop vars inside their loops

[Chad Versace]

That is, consistently do this:

for (int i = 0; ...)

No behavioral change.
---
 src/egl/drivers/dri2/platform_android.c | 32 ++--
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index 5550f580a80..a5f45a0bfbe 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -64,9 +64,7 @@ static const struct droid_yuv_format droid_yuv_formats[] = {
 static int
 get_fourcc_yuv(int native, int is_ycrcb, int chroma_step)
 {
-   int i;
-
-   for (i = 0; i < ARRAY_SIZE(droid_yuv_formats); ++i)
+   for (int i = 0; i < ARRAY_SIZE(droid_yuv_formats); ++i)
   if (droid_yuv_formats[i].native == native &&
   droid_yuv_formats[i].is_ycrcb == is_ycrcb &&
   droid_yuv_formats[i].chroma_step == chroma_step)
@@ -78,9 +76,7 @@ get_fourcc_yuv(int native, int is_ycrcb, int chroma_step)
 static bool
 is_yuv(int native)
 {
-   int i;
-
-   for (i = 0; i < ARRAY_SIZE(droid_yuv_formats); ++i)
+   for (int i = 0; i < ARRAY_SIZE(droid_yuv_formats); ++i)
   if (droid_yuv_formats[i].native == native)
  return true;
 
@@ -299,9 +295,8 @@ droid_free_local_buffers(struct dri2_egl_surface *dri2_surf)
 {
struct dri2_egl_display *dri2_dpy =
   dri2_egl_display(dri2_surf->base.Resource.Display);
-   int i;
 
-   for (i = 0; i < ARRAY_SIZE(dri2_surf->local_buffers); i++) {
+   for (int i = 0; i < ARRAY_SIZE(dri2_surf->local_buffers); i++) {
   if (dri2_surf->local_buffers[i]) {
  dri2_dpy->dri2->releaseBuffer(dri2_dpy->dri_screen,
dri2_surf->local_buffers[i]);
@@ -951,10 +946,10 @@ static int
 droid_get_buffers_parse_attachments(struct dri2_egl_surface *dri2_surf,
 unsigned int *attachments, int count)
 {
-   int num_buffers = 0, i;
+   int num_buffers = 0;
 
/* fill dri2_surf->buffers */
-   for (i = 0; i < count * 2; i += 2) {
+   for (int i = 0; i < count * 2; i += 2) {
   __DRIbuffer *buf, *local;
 
   assert(num_buffers < ARRAY_SIZE(dri2_surf->buffers));
@@ -1046,20 +1041,21 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
  EGL_RECORDABLE_ANDROID, EGL_TRUE,
  EGL_NONE
};
+
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
-   int count, i, j;
+   int count = 0;
 
-   count = 0;
-   for (i = 0; dri2_dpy->driver_configs[i]; i++) {
+   for (int i = 0; dri2_dpy->driver_configs[i]; i++) {
   const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
-  struct dri2_egl_config *dri2_conf;
 
-  for (j = 0; j < ARRAY_SIZE(visuals); j++) {
+  for (int j = 0; j < ARRAY_SIZE(visuals); j++) {
  config_attrs[1] = visuals[j].format;
  config_attrs[3] = visuals[j].format;
 
- dri2_conf = dri2_add_config(dpy, dri2_dpy->driver_configs[i],
-   count + 1, surface_type, config_attrs, visuals[j].rgba_masks);
+ struct dri2_egl_config *dri2_conf =
+ dri2_add_config(dpy, dri2_dpy->driver_configs[i], count + 1,
+ surface_type, config_attrs,
+ visuals[j].rgba_masks);
  if (dri2_conf) {
 count++;
 format_count[j]++;
@@ -1067,7 +1063,7 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
   }
}
 
-   for (i = 0; i < ARRAY_SIZE(format_count); i++) {
+   for (int i = 0; i < ARRAY_SIZE(format_count); i++) {
   if (!format_count[i]) {
  _eglLog(_EGL_DEBUG, "No DRI config supports native format 0x%x",
  visuals[i].format);
-- 
2.13.0

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

[Mesa-dev] [PATCH 4/5] egl/android: Pull invariant var outside of loop

[Chad Versace]

This makes the nested loops in droid_add_configs_for_visuals() easier to
read.

No behavioral change.
---
 src/egl/drivers/dri2/platform_android.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index 9dc2d831b49..fcf29bce713 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1035,12 +1035,11 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
   { HAL_PIXEL_FORMAT_BGRA_, { 0x00ff, 0xff00, 0x00ff, 
0xff00 } },
};
 
+   const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
int config_count = 0;
 
for (int i = 0; dri2_dpy->driver_configs[i]; i++) {
-  const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
-
   for (int j = 0; j < ARRAY_SIZE(visuals); j++) {
  const EGLint config_attrs[] = {
EGL_NATIVE_VISUAL_ID,   visuals[j].format,
-- 
2.13.0

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

[Mesa-dev] [PATCH 5/5] egl/android: Change order of EGLConfig generation

[Chad Versace]

Many Android apps (such as Google's official NDK GLES2 example app), and
even portions the core framework code (such as SystemServiceManager in
Nougat), incorrectly choose their EGLConfig.  They neglect to match the
EGLConfig's EGL_NATIVE_VISUAL_ID against the window's native format, and
instead choose the first EGLConfig whose channel sizes match those of
the native window format while ignoring the channel *ordering*.

We can detect such buggy clients in logcat when they call
eglCreateSurface, by detecting the mismatch between the EGLConfig's
format and the window's format.

As a workaround, this patch changes the order of EGLConfig generation
such that all EGLConfigs for HAL pixel format i precede those for HAL
pixel format i+1. In my (chadversary) testing on Android Nougat, this
was good enough to pacify the buggy clients.
---
 src/egl/drivers/dri2/platform_android.c | 32 +---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index fcf29bce713..c294691f291 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1039,23 +1039,41 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
int config_count = 0;
 
-   for (int i = 0; dri2_dpy->driver_configs[i]; i++) {
-  for (int j = 0; j < ARRAY_SIZE(visuals); j++) {
+   /* The nesting of loops is significant here. Also significant is the order
+* of the HAL pixel formats. Many Android apps (such as Google's official
+* NDK GLES2 example app), and even portions the core framework code (such
+* as SystemServiceManager in Nougat), incorrectly choose their EGLConfig.
+* They neglect to match the EGLConfig's EGL_NATIVE_VISUAL_ID against the
+* window's native format, and instead choose the first EGLConfig whose
+* channel sizes match those of the native window format while ignoring the
+* channel *ordering*.
+*
+* We can detect such buggy clients in logcat when they call
+* eglCreateSurface, by detecting the mismatch between the EGLConfig's
+* format and the window's format.
+*
+* As a workaround, we generate EGLConfigs such that all EGLConfigs for HAL
+* pixel format i precede those for HAL pixel format i+1. In my
+* (chadversary) testing on Android Nougat, this was good enough to pacify
+* the buggy clients.
+*/
+   for (int i = 0; i < ARRAY_SIZE(visuals); i++) {
+  for (int j = 0; dri2_dpy->driver_configs[j]; j++) {
  const EGLint config_attrs[] = {
-   EGL_NATIVE_VISUAL_ID,   visuals[j].format,
-   EGL_NATIVE_VISUAL_TYPE, visuals[j].format,
+   EGL_NATIVE_VISUAL_ID,   visuals[i].format,
+   EGL_NATIVE_VISUAL_TYPE, visuals[i].format,
EGL_FRAMEBUFFER_TARGET_ANDROID, EGL_TRUE,
EGL_RECORDABLE_ANDROID, EGL_TRUE,
EGL_NONE
  };
 
  struct dri2_egl_config *dri2_conf =
- dri2_add_config(dpy, dri2_dpy->driver_configs[i],
+ dri2_add_config(dpy, dri2_dpy->driver_configs[j],
  config_count + 1, surface_type, config_attrs,
- visuals[j].rgba_masks);
+ visuals[i].rgba_masks);
  if (dri2_conf) {
 config_count++;
-format_count[j]++;
+format_count[i]++;
  }
   }
}
-- 
2.13.0

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

[Mesa-dev] [PATCH 3/5] egl/android: Rename var in droid_add_configs_for_visuals()

[Chad Versace]

Rename 'config' to 'config_count'. I didn't understand what the variable
did until I untangled the for-loops. Now the next person won't have that
problem.
---
 src/egl/drivers/dri2/platform_android.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index f309fcea11f..9dc2d831b49 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1036,7 +1036,7 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
};
 
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
-   int count = 0;
+   int config_count = 0;
 
for (int i = 0; dri2_dpy->driver_configs[i]; i++) {
   const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
@@ -1051,11 +1051,11 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
  };
 
  struct dri2_egl_config *dri2_conf =
- dri2_add_config(dpy, dri2_dpy->driver_configs[i], count + 1,
- surface_type, config_attrs,
+ dri2_add_config(dpy, dri2_dpy->driver_configs[i],
+ config_count + 1, surface_type, config_attrs,
  visuals[j].rgba_masks);
  if (dri2_conf) {
-count++;
+config_count++;
 format_count[j]++;
  }
   }
@@ -1068,7 +1068,7 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
   }
}
 
-   return (count != 0);
+   return (config_count != 0);
 }
 
 static int
-- 
2.13.0

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

[Mesa-dev] [PATCH 2/5] egl/android: Declare 'const' the EGLConfig attribs template array

[Chad Versace]

No behavioral change. Just a cleanup.

Post-patch, we no longer modify the same array on each iteration of the
inner loop of droid_add_configs_for_visuals(). Instead, we just declare
the array as const inside the inner loop.
---
 src/egl/drivers/dri2/platform_android.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index a5f45a0bfbe..f309fcea11f 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1034,13 +1034,6 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
   { HAL_PIXEL_FORMAT_RGB_565,   { 0xf800, 0x07e0, 0x001f, 
0x } },
   { HAL_PIXEL_FORMAT_BGRA_, { 0x00ff, 0xff00, 0x00ff, 
0xff00 } },
};
-   EGLint config_attrs[] = {
- EGL_NATIVE_VISUAL_ID,   0,
- EGL_NATIVE_VISUAL_TYPE, 0,
- EGL_FRAMEBUFFER_TARGET_ANDROID, EGL_TRUE,
- EGL_RECORDABLE_ANDROID, EGL_TRUE,
- EGL_NONE
-   };
 
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
int count = 0;
@@ -1049,8 +1042,13 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
   const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
 
   for (int j = 0; j < ARRAY_SIZE(visuals); j++) {
- config_attrs[1] = visuals[j].format;
- config_attrs[3] = visuals[j].format;
+ const EGLint config_attrs[] = {
+   EGL_NATIVE_VISUAL_ID,   visuals[j].format,
+   EGL_NATIVE_VISUAL_TYPE, visuals[j].format,
+   EGL_FRAMEBUFFER_TARGET_ANDROID, EGL_TRUE,
+   EGL_RECORDABLE_ANDROID, EGL_TRUE,
+   EGL_NONE
+ };
 
  struct dri2_egl_config *dri2_conf =
  dri2_add_config(dpy, dri2_dpy->driver_configs[i], count + 1,
-- 
2.13.0

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

[Mesa-dev] [PATCH 0/5] egl/android: Change order of EGLConfig generation

[Chad Versace]

Patches 1-4 are little cleanups. The real change is in patch 5.

I wrote this series while debugging issues that Rob Herring found [1]
while testing my i965 RGBX patch series [2].  *This* patch series
fixes those errors, and is also independent of my RGBX series.

[1]: https://lists.freedesktop.org/archives/mesa-dev/2017-June/158400.html
[2]: https://lists.freedesktop.org/archives/mesa-dev/2017-June/158142.html

Many Android apps (such as Google's official NDK GLES2 example app), and
even portions the core framework code (such as SystemServiceManager in
Nougat), incorrectly choose their EGLConfig.  They neglect to match the
EGLConfig's EGL_NATIVE_VISUAL_ID against the window's native format, and
instead choose the first EGLConfig whose channel sizes match those of
the native window format while ignoring the channel *ordering*.

We can detect such buggy clients in logcat when they call
eglCreateSurface, by detecting the mismatch between the EGLConfig's
format and the window's format.

As a workaround, this patch series changes the order of EGLConfig
generation such that all EGLConfigs for HAL pixel format i precede those
for HAL pixel format i+1. In my testing on Android Nougat, this was good
enough to pacify the buggy clients.

This patch series lives on a git tag:
   
http://git.kiwitree.net/cgit/~chadv/mesa/tag/?h=chadv/review/2017-06-16/egl-android-config-order-v01

Chad Versace (5):
  egl/android: Declare loop vars inside their loops
  egl/android: Declare 'const' the EGLConfig attribs template array
  egl/android: Rename var in droid_add_configs_for_visuals()
  egl/android: Pull invariant var outside of loop
  egl/android: Change order of EGLConfig generation

 src/egl/drivers/dri2/platform_android.c | 79 +++--
 1 file changed, 45 insertions(+), 34 deletions(-)

-- 
2.13.0

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

Re: [Mesa-dev] [RFC 13/22] RFC: vulkan: Update registry for MESAX dma_buf extensions

[Chad Versace]

On Fri 16 Jun 2017, Emil Velikov wrote:
> Hi gents,
> 
> On 8 June 2017 at 19:44, Daniel Stone  wrote:
> 
> >   - VK_MESAX_external_memory_dma_buf
> >   - VK_MESAX_external_image_dma_buf
> Perhaps not so crazy idea:
> 
> Considering a handful of the people involved (Collabora, Google,
> Intel) are Khronos members, it should be possible to have the
> extensions as EXT as opposed to MESA. Just something to consider as
> the extensions are going through the process.

I raised the topic in Khronos when I began the work. The consensus was
that the initial extensions, which are dependent on KHX, should be
prefixed with MESAX. When the final extensions arrive, which would be
based on VK_KHR_external_* instead of VK_KHX_external_*, it would be
good to rename them at that time with s/MESAX/EXT/.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/6] i965: Add RGBX, RGBA configs, even on gen9

[Chad Versace]

On Thu 15 Jun 2017, Rob Herring wrote:
> On Tue, Jun 13, 2017 at 1:55 PM, Chad Versace <chadvers...@chromium.org> 
> wrote:
> > On Fri 09 Jun 2017, Tapani Pälli wrote:
> >>
> >>
> >> On 06/08/2017 09:27 PM, Chad Versace wrote:
> >> > On Thu 08 Jun 2017, Tomasz Figa wrote:
> >> > > On Thu, Jun 8, 2017 at 4:08 PM, Tapani Pälli <tapani.pa...@intel.com> 
> >> > > wrote:
> >> > > >
> >> > > > On 06/08/2017 09:36 AM, Tapani Pälli wrote:
> >> > > > >
> >> > > > >
> >> > > > >
> >> > > > > On 06/08/2017 06:05 AM, Tomasz Figa wrote:
> >> > > > > >
> >> > > > > > On Wed, Jun 7, 2017 at 5:36 AM, Chad Versace 
> >> > > > > > <chadvers...@chromium.org>
> >> > > > > > wrote:
> >> > > > > > >
> >> > > > > > > More patches to break your formats... again ;)
> >> > > > > > >
> >> > > > > > > The Android framework requires support for EGLConfigs with
> >> > > > > > > HAL_PIXEL_FORMAT_RGBX_ and HAL_PIXEL_FORMAT_RGBA_. 
> >> > > > > > > This prevents
> >> > > > > > > Chrome OS from updating its Android drivers, because earlier 
> >> > > > > > > this year
> >> > > > > > > Intel disabled all rgbx formats for gen >=9 in 
> >> > > > > > > brw_surface_formats.c.
> >> > > > > > > This patch series safely (hopefully?) fixes that problem.
> >> > > > > > >
> >> > > > > > > If you want the meat, read patches 2 and 6.
> >> > > > > > >
> >> > > > > > > Chad Versace (6):
> >> > > > > > > mesa: Add _mesa_format_fallback_rgba_to_rgbx()
> >> > > > > > > i965: Add a RGBX->RGBA fallback for 
> >> > > > > > > glEGLImageTextureTarget2D()
> >> > > > > > > i965: Rename some vague format members of brw_context
> >> > > > > > > i965/dri: Add intel_screen param to 
> >> > > > > > > intel_create_winsys_renderbuffer
> >> > > > > > > i965: Move brw_context format arrays to intel_screen
> >> > > > > > > i965/dri: Support R8G8B8A8 and R8G8B8X8 configs
> >> > > > > >
> >> > > > > >
> >> > > > > > Thanks a lot Chad!
> >> > > > > >
> >> > > > > > Just to make sure, did you have a chance to test it with X11 
> >> > > > > > apps,
> >> > > > > > that were reported to have incorrect colors last time we tried
> >> > > > > > enabling these formats? I.e.
> >> > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=95071
> >> >
> >> > tfiga, I only tested with glxgears. To be safe, I'll also test the KDE
> >> > apps in that bug report and reply back with the results.
> >> >
> >> > > > > I just tested this set on Android and I'm getting wrong colors 
> >> > > > > with this,
> >> > > > > red and blue swapped.
> >> >
> >> > Uh oh.
> >> >
> >> > I did something dumb. When testing this on ARC++, I chose
> >> > a BLACK-AND-WHITE game! I'll retest with a color game.
> >>
> >> It was pretty hard to keep coffee in mouth when reading this comment.
> >>
> >> > https://play.google.com/store/apps/details?id=com.ZephirothGames.AsteroidStormFREE=en
> >> >
> >> > > > I can 'fix' this by reordering the configs in 
> >> > > > intel_screen_make_configs so
> >> > > > that the new configs (RGBA, RGBX) required by Android are before the 
> >> > > > old
> >> > > > ones (BGRA, BGRX).
> >> > >
> >> > > I think that would have exactly the opposite effect on the X11 apps I
> >> > > mentioned before. In my testing, they were sensitive to the order of
> >> > > configs, due to component bit masks not being considered in selection,
> >> > > only bit depths.
> >> >
> >> > Yes.
> >> >
> >> > > H

Re: [Mesa-dev] [RFC 00/22] DRI3 v1.1, ANV dmabuf

[Chad Versace]

On Thu 08 Jun 2017, Jason Ekstrand wrote:
> On Thu, Jun 8, 2017 at 11:43 AM, Daniel Stone  wrote:
> 
> > Hi,
> > With full support for modifiers in DRIimage, this patch series adds
> > support for fully plumbing them through X11. A patchset proposing
> > an extension to DRI3 to support multiple planes and modifiers can
> > be found here:
> > https://lists.x.org/archives/xorg-devel/2017-June/053854.html
> >
> > The Git trees, all of which are using the wip/2017-05/dri3-v1.1 branch,
> > can be found here:
> >
> > git://git.collabora.com/git/user/daniels/xcb-proto
> >   - contains support for lists of FDs, as well as the protocol itself
> > git://git.collabora.com/git/user/daniels/libxcb
> >   - support for lists of FDs
> > git://git.collabora.com/git/user/daniels/dri3proto
> >   - what it says on the box
> > git://git.collabora.com/git/user/daniels/mesa
> >   - this patchset, as well as the previous to enable modifier
> > import/queries on i965:
> > https://lists.freedesktop.org/archives/mesa-dev/2017-June/158092.html
> > git://git.collabora.com/git/user/daniels/xserver
> >   - X server support (note: has rough edges)
> >
> > This has been tested on Skylake, where it gained Y-tiling support when
> > running EGL, GLX, and Vulkan clients under a composited X11 environment
> > (and for the output of the compositor itself). CCS support also worked
> > as the multi-plane testbed, but I've dropped that part of the patchset
> > as Jason has taken it over.
> >
> > Note that this currently breaks RADV: I'm not entirely sure what the
> > best way to get a clean transition path there is.
> >
> > Most of the DRI3 work was done by Louis-Francis Ratté-Boulianne; the
> > ANV work was taken from Chad Versace's tree, rebased and bulked up
> > slightly to support DRI3 v1.1. Thanks also to Intel for sponsoring this
> > work.
> >
> 
> I'm just getting started looking at these.  The first thing to say is that
> I'm pretty sure I made a bunch of comments the first time they were sent
> out and I don't see anything different.  It's been a while though.

I found some small differences in the first few patches. I haven't
progressed past patch 13 yet, though.

Jason, patches 5 through 12 are general cleanups in preparation for
vk-dma-buf support. Those cleanups, in my opinion, can and should land
before the actual vk-dma-buf patches, and could land now (after review
and testing). What do you think? I don't expect those patches to change,
even as the rest of the patch series evolves.


> ___
> 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] [RFC 11/22] anv: Handle failure in make_hiz_surface_maybe()

[Chad Versace]

Jason, I recall that you disliked this patch. Is that correct? If so,
why did you dislike it?

On Thu 08 Jun 2017, Daniel Stone wrote:
> From: Chad Versace <chadvers...@chromium.org>
> 
> make_ccs_surface_maybe() correctly handles failure
> isl_surf_get_ccs_surf(). When it fails, the resultant VkImage is still
> valid, just without a ccs surface.
> 
> Same of make_mcs_surface_maybe() and isl_surf_get_mcs_surf().
> 
> Fix make_hiz_surface_maybe() to do the same.
> 
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> ---
>  src/intel/vulkan/anv_image.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> index 6e7d943014..dae7ed9311 100644
> --- a/src/intel/vulkan/anv_image.c
> +++ b/src/intel/vulkan/anv_image.c
> @@ -162,7 +162,9 @@ make_hiz_surface_maybe(const struct anv_device *dev,
> } else {
>ok = isl_surf_get_hiz_surf(>isl_dev, >depth_surface.isl,
>   >aux_surface.isl);
> -  assert(ok);
> +  if (!ok)
> + return;
> +
>add_surface(image, >aux_surface);
>image->aux_usage = ISL_AUX_USAGE_HIZ;
> }
> -- 
> 2.13.0
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC 02/22] intel/isl: Add ISL <-> DRM modifier conversion

[Chad Versace]

On Thu 08 Jun 2017, Daniel Stone wrote:
> From: Chad Versace <chadvers...@chromium.org>
> 
> It converts a DRM format modifier to and from enum isl_tiling and
> aux_usage. That's all.
> 
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> ---
>  src/intel/Makefile.isl.am |  1 +
>  src/intel/isl/isl.c   | 59 
> +++
>  src/intel/isl/isl.h   | 16 +
>  3 files changed, 76 insertions(+)
> 
> diff --git a/src/intel/Makefile.isl.am b/src/intel/Makefile.isl.am
> index ee2215df1d..f8bc142942 100644
> --- a/src/intel/Makefile.isl.am
> +++ b/src/intel/Makefile.isl.am
> @@ -33,6 +33,7 @@ noinst_LTLIBRARIES += $(ISL_GEN_LIBS) isl/libisl.la
>  
>  isl_libisl_la_LIBADD = $(ISL_GEN_LIBS)
>  isl_libisl_la_SOURCES = $(ISL_FILES) $(ISL_GENERATED_FILES)
> +isl_libisl_la_CFLAGS = $(AM_CFLAGS) $(LIBDRM_CFLAGS)
>  
>  isl_libisl_gen4_la_SOURCES = $(ISL_GEN4_FILES)
>  isl_libisl_gen4_la_CFLAGS = $(AM_CFLAGS) -DGEN_VERSIONx10=40
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index 60a594394b..2512d75447 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -26,6 +26,7 @@
>  #include 
>  
>  #include "genxml/genX_bits.h"
> +#include 
>  
>  #include "isl.h"
>  #include "isl_gen4.h"
> @@ -35,6 +36,10 @@
>  #include "isl_gen9.h"
>  #include "isl_priv.h"
>  
> +#ifndef DRM_FORMAT_MOD_LINEAR
> +#define DRM_FORMAT_MOD_LINEAR 0 /* Linux 4.10 */
> +#endif
> +
>  void PRINTFLIKE(3, 4) UNUSED
>  __isl_finishme(const char *file, int line, const char *fmt, ...)
>  {
> @@ -48,6 +53,60 @@ __isl_finishme(const char *file, int line, const char 
> *fmt, ...)
> fprintf(stderr, "%s:%d: FINISHME: %s\n", file, line, buf);
>  }
>  
> +bool
> +isl_tiling_from_drm_format_mod(uint64_t mod, enum isl_tiling *tiling,
> +   enum isl_aux_usage *aux_usage)
> +{

This function looks good to me.

Alternatively, we could define two orthogonal functions:

bool
isl_tiling_from_drm_format_mod(uint64_t mod, enum isl_tiling *tiling);

bool
isl_aux_usage_from_drm_format_mod(uint64_t mod, enum isl_aux_usage 
*aux_usage);

I'm just thinking out loud, though. Don't interpret that as a suggesting.

> +   switch (mod) {
> +   case DRM_FORMAT_MOD_LINEAR:
> +  *tiling = ISL_TILING_LINEAR;
> +  *aux_usage = ISL_AUX_USAGE_NONE;
> +  return true;
> +   case I915_FORMAT_MOD_X_TILED:
> +  *tiling = ISL_TILING_X;
> +  *aux_usage = ISL_AUX_USAGE_NONE;
> +  return true;
> +   case I915_FORMAT_MOD_Y_TILED:
> +  *tiling = ISL_TILING_Y0;
> +  *aux_usage = ISL_AUX_USAGE_NONE;
> +  return true;
> +   case I915_FORMAT_MOD_Yf_TILED:
> +  *tiling = ISL_TILING_Yf;
> +  *aux_usage = ISL_AUX_USAGE_NONE;
> +  return true;
> +   }
> +
> +   return false;
> +}
> +
> +uint64_t
> +isl_tiling_to_drm_format_mod(enum isl_tiling tiling,
> + enum isl_aux_usage aux_usage)
> +{
> +   /* XXX: Need to disambiguate aux surface usage; we can validly have a CCS
> +*  aux surface attached (e.g. Y_CCS modifier), but always resolve it
> +*  before usage such that sampling with no aux plane (e.g. plain Y 
> mod)
> +*  is valid. Punt for now, and revisit when we expose aux surfaces to
> +*  external consumers. */

Hmm... this bothers me, obviously. I'll keep reading the patch series,
and return here after getting a better overview of how the patches work
together.

A small style nit... the terminal */ goes on its own line in Intel code (i965 
too).

Regardless of the XXX comment, the function looks good to me. The
function does exactly what it claims to do.

> +
> +   switch (tiling) {
> +   case ISL_TILING_LINEAR:
> +  return DRM_FORMAT_MOD_LINEAR;
> +   case ISL_TILING_X:
> +  return I915_FORMAT_MOD_X_TILED;
> +   case ISL_TILING_Y0:
> +  return I915_FORMAT_MOD_Y_TILED;
> +   case ISL_TILING_Yf:
> +  return I915_FORMAT_MOD_Yf_TILED;
> +   case ISL_TILING_W:
> +   case ISL_TILING_HIZ:
> +   case ISL_TILING_CCS:
> +  unreachable("non-color-buffer mode in isl_tiling_to_drm_format_mod");
> +   }
> +
> +   unreachable("unknown tiling in isl_tiling_to_drm_format_mod");
> +}
> +
>  void
>  isl_device_init(struct isl_device *dev,
>  const struct gen_device_info *info,
> diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> index 95ecaf90d8..30ed078ffd 100644
> --- a/src/intel/isl/isl.h
> +++ b/src/intel/isl/isl.h
> @@ -1482,6 +1482,22 @@ bool
>  isl_has_matching

Re: [Mesa-dev] [PATCH 0/6] i965: Add RGBX, RGBA configs, even on gen9

[Chad Versace]

On Fri 09 Jun 2017, Tapani Pälli wrote:
> 
> 
> On 06/08/2017 09:27 PM, Chad Versace wrote:
> > On Thu 08 Jun 2017, Tomasz Figa wrote:
> > > On Thu, Jun 8, 2017 at 4:08 PM, Tapani Pälli <tapani.pa...@intel.com> 
> > > wrote:
> > > > 
> > > > On 06/08/2017 09:36 AM, Tapani Pälli wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 06/08/2017 06:05 AM, Tomasz Figa wrote:
> > > > > > 
> > > > > > On Wed, Jun 7, 2017 at 5:36 AM, Chad Versace 
> > > > > > <chadvers...@chromium.org>
> > > > > > wrote:
> > > > > > > 
> > > > > > > More patches to break your formats... again ;)
> > > > > > > 
> > > > > > > The Android framework requires support for EGLConfigs with
> > > > > > > HAL_PIXEL_FORMAT_RGBX_ and HAL_PIXEL_FORMAT_RGBA_. This 
> > > > > > > prevents
> > > > > > > Chrome OS from updating its Android drivers, because earlier this 
> > > > > > > year
> > > > > > > Intel disabled all rgbx formats for gen >=9 in 
> > > > > > > brw_surface_formats.c.
> > > > > > > This patch series safely (hopefully?) fixes that problem.
> > > > > > > 
> > > > > > > If you want the meat, read patches 2 and 6.
> > > > > > > 
> > > > > > > Chad Versace (6):
> > > > > > > mesa: Add _mesa_format_fallback_rgba_to_rgbx()
> > > > > > > i965: Add a RGBX->RGBA fallback for 
> > > > > > > glEGLImageTextureTarget2D()
> > > > > > > i965: Rename some vague format members of brw_context
> > > > > > > i965/dri: Add intel_screen param to 
> > > > > > > intel_create_winsys_renderbuffer
> > > > > > > i965: Move brw_context format arrays to intel_screen
> > > > > > > i965/dri: Support R8G8B8A8 and R8G8B8X8 configs
> > > > > > 
> > > > > > 
> > > > > > Thanks a lot Chad!
> > > > > > 
> > > > > > Just to make sure, did you have a chance to test it with X11 apps,
> > > > > > that were reported to have incorrect colors last time we tried
> > > > > > enabling these formats? I.e.
> > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=95071
> > 
> > tfiga, I only tested with glxgears. To be safe, I'll also test the KDE
> > apps in that bug report and reply back with the results.
> > 
> > > > > I just tested this set on Android and I'm getting wrong colors with 
> > > > > this,
> > > > > red and blue swapped.
> > 
> > Uh oh.
> > 
> > I did something dumb. When testing this on ARC++, I chose
> > a BLACK-AND-WHITE game! I'll retest with a color game.
> 
> It was pretty hard to keep coffee in mouth when reading this comment.
> 
> > https://play.google.com/store/apps/details?id=com.ZephirothGames.AsteroidStormFREE=en
> > 
> > > > I can 'fix' this by reordering the configs in intel_screen_make_configs 
> > > > so
> > > > that the new configs (RGBA, RGBX) required by Android are before the old
> > > > ones (BGRA, BGRX).
> > > 
> > > I think that would have exactly the opposite effect on the X11 apps I
> > > mentioned before. In my testing, they were sensitive to the order of
> > > configs, due to component bit masks not being considered in selection,
> > > only bit depths.
> > 
> > Yes.
> > 
> > > However, for Android, I thought EGL already used bit masks, so
> > > possibly there is still an unrelated bug somewhere.
> > 
> > Android certainly inspects the channel masks, using dri2_add_config().
> > So I'm also curious why Tapani sees swapped channels. The
> > platform_android.c is patched with format hacks, and I assume the same
> > is true for android-ia. Maybe the hacks are intefering somehow. I'll
> > investigate.
> 
> We shouldn't have related 'hacks', but here are some changes that might be
> interesting or somehow related:
> 
> add EGL_ALPHA_SIZE attrib:
> 
> https://github.com/android-ia/frameworks-native/commit/8237c9f8eb36d4f9ead40c8cb4a68ae9518d3c9f
> 
> sorting display configs:
> 
> https://github.com/android-ia/frameworks-native/pull/2/commits/bb29af0777e747effacd239565f52aad96c45558
> 
> visuals being added:
> 
> https://github.com/android-ia/external-mesa/commit/ceac31ff7e53ec5034bc379d37ce365c000e9e4a

I confirmed that this series causes no regressions with Gnome and KDE
apps. For KDE, I used Amarok as the test app. To prove to myself
I *really* was testing this series, I tested the series with the new
formats placed at the end of the array, and then I tested again with the new
formats placed at the start of the array. As expected, Amarok's colors
were correct in the first test, and were wrong in the second test (blue
and red were swapped.

I still haven't succeeded in fully testing these patches against
Android, for a combination of technical and non-technical reasons. I'm
travelling this week, so I will probably be able to resume testing
Android apps on Thursday.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [RFC 00/22] DRI3 v1.1, ANV dmabuf

[Chad Versace]

On Thu 08 Jun 2017, Daniel Stone wrote:
> Hi,
> With full support for modifiers in DRIimage, this patch series adds
> support for fully plumbing them through X11. A patchset proposing
> an extension to DRI3 to support multiple planes and modifiers can
> be found here:
> https://lists.x.org/archives/xorg-devel/2017-June/053854.html
> 
> The Git trees, all of which are using the wip/2017-05/dri3-v1.1 branch,
> can be found here:
> 
> git://git.collabora.com/git/user/daniels/xcb-proto
>   - contains support for lists of FDs, as well as the protocol itself
> git://git.collabora.com/git/user/daniels/libxcb
>   - support for lists of FDs
> git://git.collabora.com/git/user/daniels/dri3proto
>   - what it says on the box
> git://git.collabora.com/git/user/daniels/mesa
>   - this patchset, as well as the previous to enable modifier
> import/queries on i965:
> https://lists.freedesktop.org/archives/mesa-dev/2017-June/158092.html
> git://git.collabora.com/git/user/daniels/xserver
>   - X server support (note: has rough edges)
> 
> This has been tested on Skylake, where it gained Y-tiling support when
> running EGL, GLX, and Vulkan clients under a composited X11 environment
> (and for the output of the compositor itself). CCS support also worked
> as the multi-plane testbed, but I've dropped that part of the patchset
> as Jason has taken it over.
> 
> Note that this currently breaks RADV: I'm not entirely sure what the
> best way to get a clean transition path there is.
> 
> Most of the DRI3 work was done by Louis-Francis Ratté-Boulianne; the
> ANV work was taken from Chad Versace's tree, rebased and bulked up
> slightly to support DRI3 v1.1. Thanks also to Intel for sponsoring this
> work.

Hi Daniel, I resumed work on the VK_MESAX_external extensions earlier
today, before noticing you had sent out this patch series. (My baby has
had fever since Thursday, so I haven't exactly been attentive to
mesa-dev).  I pushed an updated branch [1] of the draft extension spec
that resolves some major issues with its API; namely, how the API
manages planar images and auxiliary surfaces.

Thanks for pushing the dma_buf work forward for Mesa, Anvil, and EGL.
I'll take a closer look at the patches later today, after getting some
sleep (it's 3:30am here).

[1]: https://github.com/chadversary/vulkan-spec/tree/wip/1.0-VK_MESAX_external
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] egl: fix _eglQuerySurface in EGL_BUFFER_AGE_EXT case

[Chad Versace]

On Thu 08 Jun 2017, Tapani Pälli wrote:
> Specification states that in case of error, value should not be
> written, patch changes buffer age queries to return -1 in case of
> error so that we can skip changing the value.
> 
> In addition, small change to droid_query_buffer_age to return 0
> in case buffer does not have a back buffer available.
> 
> Fixes:
>dEQP-EGL.functional.negative_partial_update.not_postable_surface
> 
> Signed-off-by: Tapani Pälli <tapani.pa...@intel.com>
> ---
>  src/egl/drivers/dri2/platform_android.c | 4 ++--
>  src/egl/drivers/dri2/platform_drm.c | 2 +-
>  src/egl/main/eglsurface.c   | 6 +-
>  3 files changed, 8 insertions(+), 4 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 0/6] i965: Add RGBX, RGBA configs, even on gen9

[Chad Versace]

On Thu 08 Jun 2017, Tomasz Figa wrote:
> On Thu, Jun 8, 2017 at 4:08 PM, Tapani Pälli <tapani.pa...@intel.com> wrote:
> >
> > On 06/08/2017 09:36 AM, Tapani Pälli wrote:
> >>
> >>
> >>
> >> On 06/08/2017 06:05 AM, Tomasz Figa wrote:
> >>>
> >>> On Wed, Jun 7, 2017 at 5:36 AM, Chad Versace <chadvers...@chromium.org>
> >>> wrote:
> >>>>
> >>>> More patches to break your formats... again ;)
> >>>>
> >>>> The Android framework requires support for EGLConfigs with
> >>>> HAL_PIXEL_FORMAT_RGBX_ and HAL_PIXEL_FORMAT_RGBA_. This prevents
> >>>> Chrome OS from updating its Android drivers, because earlier this year
> >>>> Intel disabled all rgbx formats for gen >=9 in brw_surface_formats.c.
> >>>> This patch series safely (hopefully?) fixes that problem.
> >>>>
> >>>> If you want the meat, read patches 2 and 6.
> >>>>
> >>>> Chad Versace (6):
> >>>>mesa: Add _mesa_format_fallback_rgba_to_rgbx()
> >>>>i965: Add a RGBX->RGBA fallback for glEGLImageTextureTarget2D()
> >>>>i965: Rename some vague format members of brw_context
> >>>>i965/dri: Add intel_screen param to intel_create_winsys_renderbuffer
> >>>>i965: Move brw_context format arrays to intel_screen
> >>>>i965/dri: Support R8G8B8A8 and R8G8B8X8 configs
> >>>
> >>>
> >>> Thanks a lot Chad!
> >>>
> >>> Just to make sure, did you have a chance to test it with X11 apps,
> >>> that were reported to have incorrect colors last time we tried
> >>> enabling these formats? I.e.
> >>> https://bugs.freedesktop.org/show_bug.cgi?id=95071

tfiga, I only tested with glxgears. To be safe, I'll also test the KDE
apps in that bug report and reply back with the results.

> >> I just tested this set on Android and I'm getting wrong colors with this,
> >> red and blue swapped.

Uh oh.

I did something dumb. When testing this on ARC++, I chose
a BLACK-AND-WHITE game! I'll retest with a color game.

https://play.google.com/store/apps/details?id=com.ZephirothGames.AsteroidStormFREE=en

> > I can 'fix' this by reordering the configs in intel_screen_make_configs so
> > that the new configs (RGBA, RGBX) required by Android are before the old
> > ones (BGRA, BGRX).
> 
> I think that would have exactly the opposite effect on the X11 apps I
> mentioned before. In my testing, they were sensitive to the order of
> configs, due to component bit masks not being considered in selection,
> only bit depths.

Yes.

> However, for Android, I thought EGL already used bit masks, so
> possibly there is still an unrelated bug somewhere.

Android certainly inspects the channel masks, using dri2_add_config().
So I'm also curious why Tapani sees swapped channels. The
platform_android.c is patched with format hacks, and I assume the same
is true for android-ia. Maybe the hacks are intefering somehow. I'll
investigate.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 6/6] i965/dri: Support R8G8B8A8 and R8G8B8X8 configs

[Chad Versace]

On Thu 08 Jun 2017, Rob Herring wrote:
> On Tue, Jun 6, 2017 at 3:37 PM, Chad Versace <chadvers...@chromium.org> wrote:
> > The Android framework requires support for EGLConfigs with
> > HAL_PIXEL_FORMAT_RGBX_ and HAL_PIXEL_FORMAT_RGBA_.
> >
> > Even though all RGBX formats are disabled on gen9 by
> > brw_surface_formats.c, the new configs work correctly on Broxton thanks
> > to _mesa_format_fallback_rgbx_to_rgba().
> >
> > On GLX, this creates no new configs, and therefore breaks no existing
> > apps. See in-patch comments for explanation. I tested with glxinfo and
> > glxgears on Skylake.
> >
> > On Wayland, this also creates no new configs, and therfore breaks no
> > existing apps. (I tested with mesa-demos' eglinfo and es2gears_wayland
> > on Skylake). The reason differs from GLX, though. In
> > dri2_wl_add_configs_for_visual(), the format table contains only
> > B8G8R8X8, B8G8R8A8, and B5G6B5; and dri2_add_config() correctly matches
> > EGLConfig to format by inspecting channel masks.
> >
> > On Android, in Chrome OS, I tested this on a Broxton device. I confirmed
> > that the Google Play Store's EGLSurface used HAL_PIXEL_FORMAT_RGBA_,
> > and that an Asteroid game's EGLSurface used HAL_PIXEL_FORMAT_RGBX_.
> > Both apps worked well. (Disclaimer: I didn't test this patch on Android
> > with Mesa master. I backported this patch series to an older Android
> > branch).
> > ---
> >  src/mesa/drivers/dri/i965/intel_fbo.c| 24 ++--
> >  src/mesa/drivers/dri/i965/intel_screen.c | 23 ++-
> >  2 files changed, 44 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c 
> > b/src/mesa/drivers/dri/i965/intel_fbo.c
> > index 16d1325736..e56d30a2c0 100644
> > --- a/src/mesa/drivers/dri/i965/intel_fbo.c
> > +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
> > @@ -34,6 +34,7 @@
> >  #include "main/teximage.h"
> >  #include "main/image.h"
> >  #include "main/condrender.h"
> > +#include "main/format_fallback.h"
> >  #include "util/hash_table.h"
> >  #include "util/set.h"
> >
> > @@ -450,10 +451,29 @@ intel_create_winsys_renderbuffer(struct intel_screen 
> > *screen,
> >
> > _mesa_init_renderbuffer(rb, 0);
> > rb->ClassID = INTEL_RB_CLASS;
> > +   rb->NumSamples = num_samples;
> > +
> > +   /* The base format and internal format must be derived from the 
> > user-visible
> > +* format (that is, the gl_config's format), even if we internally use
> > +* choose a different format for the renderbuffer. Otherwise, rendering 
> > may
> > +* use incorrect channel write masks.
> > +*/
> > rb->_BaseFormat = _mesa_get_format_base_format(format);
> > -   rb->Format = format;
> > rb->InternalFormat = rb->_BaseFormat;
> > -   rb->NumSamples = num_samples;
> > +
> > +   rb->Format = format;
> > +   if (!screen->mesa_format_supports_render[rb->Format]) {
> > +  /* The glRenderbufferStorage paths in core Mesa detect if the driver
> > +   * does not support the user-requested format, and then searches for
> > +   * a falback format. The DRI code bypasses core Mesa, though. So we 
> > do
> > +   * the fallbacks here.
> > +   *
> > +   * We must support MESA_FORMAT_R8G8B8X8 on Android because the 
> > Android
> > +   * framework requires HAL_PIXEL_FORMAT_RGBX winsys surfaces.
> > +   */
> > +  rb->Format = _mesa_format_fallback_rgbx_to_rgba(rb->Format);
> > +  assert(screen->mesa_format_supports_render[rb->Format]);
> > +   }
> >
> > /* intel-specific methods */
> > rb->Delete = intel_delete_renderbuffer;
> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
> > b/src/mesa/drivers/dri/i965/intel_screen.c
> > index 563065b91f..a9d132f868 100644
> > --- a/src/mesa/drivers/dri/i965/intel_screen.c
> > +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> > @@ -1547,7 +1547,28 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
> > static const mesa_format formats[] = {
> >MESA_FORMAT_B5G6R5_UNORM,
> >MESA_FORMAT_B8G8R8A8_UNORM,
> > -  MESA_FORMAT_B8G8R8X8_UNORM
> > +  MESA_FORMAT_B8G8R8X8_UNORM,
> > +
> > +  /* The 32-bit RGBA format must not precede the 32-bit BGRA format.
> > +   * Likewise for RGBX and BGRX.  Otherwise, the GLX client and the GLX
> > +   * s

Re: [Mesa-dev] [PATCH 2/6] i965: Add a RGBX->RGBA fallback for glEGLImageTextureTarget2D()

[Chad Versace]

On Tue 06 Jun 2017, Daniel Stone wrote:
> Hi Chad,
> 
> On 6 June 2017 at 21:36, Chad Versace <chadvers...@chromium.org> wrote:
> > @@ -254,8 +255,22 @@ create_mt_for_dri_image(struct brw_context *brw,
> > struct gl_context *ctx = >ctx;
> > struct intel_mipmap_tree *mt;
> > uint32_t draw_x, draw_y;
> > +   mesa_format format = image->format;
> > +
> > +   if (!ctx->TextureFormatSupported[format]) {
> > +  /* The texture storage paths in core Mesa detect if the driver does 
> > not
> > +   * support the user-requested format, and then searches for a
> > +   * fallback format. The DRIimage code bypasses core Mesa, though. So 
> > we
> > +   * do the fallbacks here for important formats.
> > +   *
> > +   * We must support DRM_FOURCC_XBGR textures because the Android
> > +   * framework produces HAL_PIXEL_FORMAT_RGBX winsys surfaces, 
> > which
> > +   * the Chrome OS compositor consumes as dma_buf EGLImages.
> > +   */
> > +  format = _mesa_format_fallback_rgbx_to_rgba(format);
> > +   }
> >
> > -   if (!ctx->TextureFormatSupported[image->format])
> > +   if (!ctx->TextureFormatSupported[format])
> >return NULL;

I dislike what I wrote above. There's a much better way to do the
fallback, a way that handles more types of fallback than rgbx->rgba and
that's the same as the fallback used by glTexStorage2D(). The better way
is to re-use the core Mesa code that the comment refers to, like this:

mesa_format format = ctx->Driver.ChooseTextureFormat(ctx, GL_TEXTURE_2D,
 internalFormat, 
GL_NONE, GL_NONE);

As precedent, that's exactly what intel_renderbuffer_format() does.

> >
> > /* Disable creation of the texture's aux buffers because the driver 
> > exposes
> > @@ -263,7 +278,7 @@ create_mt_for_dri_image(struct brw_context *brw,
> >  * buffer's content to the main buffer nor for invalidating the aux 
> > buffer's
> >  * content.
> >  */
> > -   mt = intel_miptree_create_for_bo(brw, image->bo, image->format,
> > +   mt = intel_miptree_create_for_bo(brw, image->bo, format,
> >  0, image->width, image->height, 1,
> >  image->pitch,
> >  MIPTREE_LAYOUT_DISABLE_AUX);
> 
> I wonder if it wouldn't be better to do this in
> intel_create_image_from_name. That way it would be more obvious
> up-front what's happening,

I agree that the intent would become more obvious if the format fallback
were done at time of import instead of gl*Storage. But I see two
arguments against it:

1. First, the weaker argument.

   The chosen fallback format,
   and even the choice to do a fallback at all, is a property of the
   image's usage and not a property of the image itself. A single
   image can have multiple uses during its lifetime, and the driver
   may need a different fallback or no fallback for each. I'm
   defining "image usage" here in terms of
   glEGLImageTargetTexture2DOES, glEGLImageTargetRenderbufferStorageOES, and
   GL_TEXURE_EXTERNAL_OES vs GL_TEXTURE_2D.

   Which reminds me... I should have submitted an analgous patch for
   glEGLImageTargetRenderbufferStorageOES().

   Since the driver may support a given format for texturing but not
   rendering, or for rendering but not texturing, we would need to do at
   least two format fallbacks during image import, and cache the fallback
   results in the image struct. This approach is possible, but...
   onto the next bullet.

2. A more practical argument.

   If possible, it's better to do the fallback for
   glEGLImageTextureTarget2DOES() in the same way as for
   glTexStorage2D(), as I explained above. But that requires access
   to a GL context; eglCreateImage may be called without
   a context. [EGL_EXT_image_dma_buf_import explicitly requires that
   eglCreateImage(context=EGL_CONTEXT_NONE)]; and therefore
   intel_create_image_name() and friends also have no context.

> and also it'd be easy to add the analogue
> to intel_create_image_from_fds for the explicit dmabuf (as opposed to
> ANativeBuffer; your description of 'from dmabufs' threw me because the
> dmabuf fd import path would fail immediately on FourCC lookup) import
> path.

I did mean dma_buf, not AHardwareBuffer or ANativeBuffer, in this patch.

The patch series is secretly crazy. It's implicitly partitioned into
3 sets of patches: patches that touch code that will run only on
Android; that will run only on Chrome OS; and that will run on both.

 

Re: [Mesa-dev] [PATCH 1/6] mesa: Add _mesa_format_fallback_rgba_to_rgbx()

[Chad Versace]

On Tue 06 Jun 2017, Dylan Baker wrote:
> Quoting Chad Versace (2017-06-06 15:11:18)
> > On Tue 06 Jun 2017, Dylan Baker wrote:
> > > Quoting Chad Versace (2017-06-06 13:36:55)


> > > > +write_preamble(stdout)
> > > > +write_func_mesa_format_fallback_rgbx_to_rgba(stdout, formats)
> > > 
> > > We really shouldn't write to stdout like this, it can cause all kinds of
> > > breakages if there's ever a UTF-8 character (say ©) and the terminal 
> > > doesn't
> > > have a unicode locale it'll fail,
> > 
> > Ugh. I wasn't aware that Python's stdout was broken. Is Python's
> > sys.stdout opened in "text" mode, and is that the cause of the
> > brokenness?
> > 
> > Does it still fail if stdout is redirected to a file? Because that's the
> > only case that matters here.
> 
> It's not python, it's the shell (I think). In this case it won't be a problem
> since you don't have any non-ascii characters, but we've run into cases where
> someone (like me) adds "Copyright © 3001 Mystery Science Theatre" and then
> breaks some (but not many) systems. I know this because I added such a 
> copyright
> and broke someone's system, and eventually we narrowed it down to the fact 
> that
> this person didn't have a UTF-8 locale but I did, we ended up just removing 
> the
> © character from the output to fix it.

Ok, then it's not really a problem for build-system scripts.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 00/30] i965: Overhaul resolves

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> This patch series does a complete overhaul of the current resolve handling
> framework inside the i916 OpenGL driver.

I've ran out of time to review the remainder of the series. I believe
I skipped patches 24-30. You can add my acked-by to those patches if you
wish.

Yay for partial resolves!
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 23/30] i965: Move depth to the new resolve functions

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_clear.c | 12 ++--
>  src/mesa/drivers/dri/i965/brw_context.c   |  7 ---
>  src/mesa/drivers/dri/i965/brw_draw.c  | 17 +
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 23 ++-
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  9 +
>  5 files changed, 50 insertions(+), 18 deletions(-)

Yes! For me, in this patch, the series finally clicked for me.

Up until this patch, I've been cheerfully reviewing nice cleanups, nice
refactors, nice simplifications. All those changes, however, felt like
incremental improvements, each localized in concept.

In this patch, I finally see how big of a global difference the
series makes. Compared to the existing code (which was hairy
spaghetti), reasoning about aux state and writing correct code to handle
aux state transitions is now relatively effortless.

Patch 23 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 22/30] i965: Move images to the new resolve functions

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_context.c   | 9 +
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 9 +
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 3 +++
>  3 files changed, 13 insertions(+), 8 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 21/30] i965: Move framebuffer fetch to the new resolve functions

[Chad Versace]

Patches 20 and 21 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 19/30] i965: Move color rendering to the new resolve functions

[Chad Versace]

On Wed 31 May 2017, Pohjolainen, Topi wrote:
> On Tue, May 30, 2017 at 05:59:51PM -0700, Jason Ekstrand wrote:
> > On Fri, May 26, 2017 at 4:30 PM, Jason Ekstrand <ja...@jlekstrand.net>
> > wrote:
> > 
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_context.c   | 35 +++
> > >  src/mesa/drivers/dri/i965/brw_draw.c  |  4 +--
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 48
> > > +++
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  9 +
> > >  4 files changed, 63 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> > > b/src/mesa/drivers/dri/i965/brw_context.c
> > > index 07ddaf0..48e8b6c 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > > @@ -301,38 +301,11 @@ intel_update_state(struct gl_context * ctx, GLuint
> > > new_state)
> > >if (irb == NULL || irb->mt == NULL)
> > >   continue;
> > >
> > > -  struct intel_mipmap_tree *mt = irb->mt;
> > > +  intel_miptree_prepare_render(brw, irb->mt, irb->mt_level,
> > > +   irb->mt_layer, irb->layer_count,
> > > +   ctx->Color.sRGBEnabled);
> > >
> > > -  /* If FRAMEBUFFER_SRGB is used on Gen9+ then we need to resolve any
> > > of
> > > -   * the single-sampled color renderbuffers because the CCS buffer
> > > isn't
> > > -   * supported for SRGB formats. This only matters if
> > > FRAMEBUFFER_SRGB is
> > > -   * enabled because otherwise the surface state will be programmed
> > > with
> > > -   * the linear equivalent format anyway.
> > > -   */
> > > -  if (brw->gen >= 9 && ctx->Color.sRGBEnabled && mt->num_samples <= 1
> > > &&
> > > -  _mesa_get_srgb_format_linear(mt->format) != mt->format) {
> > > -
> > > - /* Lossless compression is not supported for SRGB formats, it
> > > -  * should be impossible to get here with such surfaces.
> > > -  */
> > > - assert(!intel_miptree_is_lossless_compressed(brw, mt));
> > > - intel_miptree_all_slices_resolve_color(brw, mt, 0);
> > > - brw_render_cache_set_check_flush(brw, mt->bo);
> > > -  }
> > > -
> > > -  /* For layered rendering non-compressed fast cleared buffers need
> > > to be
> > > -   * resolved. Surface state can carry only one fast color clear 
> > > value
> > > -   * while each layer may have its own fast clear color value. For
> > > -   * compressed buffers color value is available in the color buffer.
> > > -   */
> > > -  if (irb->layer_count > 1 &&
> > > -  !(irb->mt->aux_disable & INTEL_AUX_DISABLE_CCS) &&
> > > -  !intel_miptree_is_lossless_compressed(brw, mt)) {
> > > - assert(brw->gen >= 8);
> > > -
> > > - intel_miptree_resolve_color(brw, mt, irb->mt_level, 1,
> > > - irb->mt_layer, irb->layer_count, 0);
> > > -  }
> > > +  brw_render_cache_set_check_flush(brw, irb->mt->bo);
> > >
> > 
> > This render_cache_set_check_flush is unneeded and is actually the cause of
> > most of the performance regressions in this series.  Making it
> > unconditional meant we flushed the render cache on every draw call.  It's a
> > bit surprising that doing so didn't hurt things any worse than it did.  It
> > was originally put in to satisfy the requirements about flushing around
> > resolves.  Now that we do that directly in brw_blorp_resolve_color, we
> > don't need it at all much less unconditionally.  I've removed this line
> > locally.
> 
> Makes sense, I remember fighting against unconditional flushing as well. This
> series makes a big difference in how easy is to keep track of aux state and
> flushing more precisely.

Please say in the commit message that the patch removes an instance of
brw_render_cache_set_check_flush(). The commit message in your
i965-resolve-rework-v3 branch implies that it's merely a refactoring
patch.

With the expanded commit message,
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 18/30] i965: Move texturing to the new resolve functions

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_context.c   | 55 
> +--
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 55 
> +++
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  6 +++
>  3 files changed, 71 insertions(+), 45 deletions(-)

>  static void
>  intel_update_state(struct gl_context * ctx, GLuint new_state)
>  {
> @@ -259,16 +224,16 @@ intel_update_state(struct gl_context * ctx, GLuint 
> new_state)
>/* We need inte_texture_object::_Format to be valid */
>intel_finalize_mipmap_tree(brw, i);
>  
> -  if (intel_miptree_sample_with_hiz(brw, tex_obj->mt))
> - intel_miptree_all_slices_resolve_hiz(brw, tex_obj->mt);
> -  else
> - intel_miptree_all_slices_resolve_depth(brw, tex_obj->mt);
> -  /* Sampling engine understands lossless compression and resolving
> -   * those surfaces should be skipped for performance reasons.
> -   */
> -  const int flags = intel_texture_view_requires_resolve(brw, tex_obj) ?
> -   0 : INTEL_MIPTREE_IGNORE_CCS_E;
> -  intel_miptree_all_slices_resolve_color(brw, tex_obj->mt, flags);
> +  bool aux_supported;
> +  intel_miptree_prepare_texture(brw, tex_obj->mt, tex_obj->_Format,
> +_supported);
> +
> +  if (!aux_supported && brw->gen >= 9 &&
> +  intel_disable_rb_aux_buffer(brw, tex_obj->mt->bo)) {
> + perf_debug("Sampling renderbuffer with non-compressible format - "
> +"turning off compression");
> +  }
> +

I don't understand why intel_disable_rb_aux_buffer() is called here.
Specifically, I don't understand why the code disables aux for
a *framebuffer* attachment in response to the state of a *texture* view
in the *same* draw call. There is hidden subtlety here.

[time passes... tick tick tick]

In general, the GL spec claims undefined behavior when simultaneuosly
sampling and rendering to the same buffer. Self-blits are legal, though.
So, it seems to me that intel_disable_rb_aux_buffer() is applicable here
only during a glBlit* when src and dest are the same. Is that correct?
Do there exist other applicable scenarios that I'm failing to see?

Anyway, this is just a refactoring patch, so
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 15/30] i965/miptree: Add new entrypoints for resolve management

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> This commit adds a new unified interface for doing resolves.  The basic
> format is that, prior to any surface access such as texturing or
> rendering, you call intel_miptree_prepare_access.  If the surface was
> written, you call intel_miptree_finish_write.  These two functions take
> parameters which tell them whether or not auxiliary compression and fast
> clears are supported on the surface.  Later commits will add wrappers
> around these two functions for texturing, rendering, etc.
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 156 
> +-
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  80 +
>  2 files changed, 232 insertions(+), 4 deletions(-)


> +void
> +intel_miptree_prepare_access(struct brw_context *brw,
> + struct intel_mipmap_tree *mt,
> + uint32_t start_level, uint32_t num_levels,
> + uint32_t start_layer, uint32_t num_layers,
> + bool aux_supported, bool fast_clear_supported)

This parameter list seems a good place to pass in a bool that indicates
the miptree data can be invalidated before access. For example, before
a full, non-scissored clear or before mapping with
GL_MAP_INVALIDATE_RANGE_BIT. That info would let us avoid unneeded aux
ops.

But, of course, such a change doesn't belong in this patch, or perhaps
even in this patch series. I just wanted to suggest a future
improvement.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 16/30] i965: Use the new resolve function for several simple cases

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_context.c|  2 +-
>  src/mesa/drivers/dri/i965/intel_blit.c | 14 --
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c  | 12 +---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h  | 18 ++
>  src/mesa/drivers/dri/i965/intel_pixel_bitmap.c |  2 +-
>  src/mesa/drivers/dri/i965/intel_pixel_read.c   |  2 +-
>  src/mesa/drivers/dri/i965/intel_tex_image.c|  7 ---
>  src/mesa/drivers/dri/i965/intel_tex_subimage.c |  7 ---
>  8 files changed, 38 insertions(+), 26 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 15/30] i965/miptree: Add new entrypoints for resolve management

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> This commit adds a new unified interface for doing resolves.  The basic
> format is that, prior to any surface access such as texturing or
> rendering, you call intel_miptree_prepare_access.  If the surface was
> written, you call intel_miptree_finish_write.  These two functions take
> parameters which tell them whether or not auxiliary compression and fast
> clears are supported on the surface.  Later commits will add wrappers
> around these two functions for texturing, rendering, etc.
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 156 
> +-
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  80 +
>  2 files changed, 232 insertions(+), 4 deletions(-)

Modulo the errant whitespace changes Topi pointed out, which should be
squashed into an earlier 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 14/30] intel/isl: Add an enum for describing auxiliary compression state

[Chad Versace]

On Tue 06 Jun 2017, Jason Ekstrand wrote:
> On Tue, Jun 6, 2017 at 6:00 PM, Chad Versace <c...@kiwitree.net> wrote:
> 
> > On Tue 06 Jun 2017, Jason Ekstrand wrote:
> > > On Tue, Jun 6, 2017 at 1:32 PM, Jason Ekstrand <ja...@jlekstrand.net>
> > wrote:
> > >
> > > > On Tue, Jun 6, 2017 at 1:22 PM, Chad Versace <chadvers...@chromium.org
> > >
> > > > wrote:
> > > >
> > > >> On Fri 26 May 2017, Jason Ekstrand wrote:
> >
> > > > How about a section after the auxiliary compression ops section which
> > goes
> > > > into detail on each of the compression types and discusses which
> > states are
> > > > valid etc.
> > > >
> > >
> > > How does this look:
> > >
> > > https://cgit.freedesktop.org/~jekstrand/mesa/commit/?h=wip/
> > i965-resolve-rework-v3=8478b102c99e3ec43ec687b3f4e52acb9acbd5ba
> > >
> > > I'll squash it in if you like it.
> >
> > Please squash that in, with fixes :)
> >
> > I don't believe the pass-through state is impossible with MCS, because
> > there is no single surface for write to "pass through" to. The aux
> > surface can never be ignored with MCS. Another bit of evidence for this
> > is that there exists no MCS ambiguate op, and therefore no arrow can
> > exist in the diagram that carries the "resolved" box to the
> > pass-through" box.
> >
> 
> Too many negatives going on...  I think you meant "I belive the
> pass-through state is impossible" :-)

Right... sloppy me.

> I do not agree.  While no resolve has been written, one could easily do
> so.  It would be implemented most likely as a blit from the surface to
> itself with MCS enabled on the source and disabled for the destination
> followed by blasting the appropriate value into the MCS.  Modulo issues
> with the order in which pixels are dispatched (which I don't think is an
> actual issue so long as SIMD > num_samples), the result should be a surface
> in the pass-through state which can safely be rendered to with MCS
> disabled.  Now, why anyone would ever want to do that is beyond me.  The
> state which doesn't exist is the regular resolved state because, as with
> CCS and MCS, the aux surface stores so little data, that there isn't really
> any room for compression when the main surface contains valid data.

Thanks for taking the time to explain that corner case to stubborn me.
Such a state is so far outside of realistic usage that I failed to see
it earlier.

This patch, with extras squashed in, 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 14/30] intel/isl: Add an enum for describing auxiliary compression state

[Chad Versace]

On Tue 06 Jun 2017, Jason Ekstrand wrote:
> On Tue, Jun 6, 2017 at 1:32 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> 
> > On Tue, Jun 6, 2017 at 1:22 PM, Chad Versace <chadvers...@chromium.org>
> > wrote:
> >
> >> On Fri 26 May 2017, Jason Ekstrand wrote:

> > How about a section after the auxiliary compression ops section which goes
> > into detail on each of the compression types and discusses which states are
> > valid etc.
> >
> 
> How does this look:
> 
> https://cgit.freedesktop.org/~jekstrand/mesa/commit/?h=wip/i965-resolve-rework-v3=8478b102c99e3ec43ec687b3f4e52acb9acbd5ba
> 
> I'll squash it in if you like it.

Please squash that in, with fixes :)

I don't believe the pass-through state is impossible with MCS, because
there is no single surface for write to "pass through" to. The aux
surface can never be ignored with MCS. Another bit of evidence for this
is that there exists no MCS ambiguate op, and therefore no arrow can
exist in the diagram that carries the "resolved" box to the
pass-through" box.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 14/30] intel/isl: Add an enum for describing auxiliary compression state

[Chad Versace]

On Tue 06 Jun 2017, Jason Ekstrand wrote:
> On Tue, Jun 6, 2017 at 1:22 PM, Chad Versace <chadvers...@chromium.org>
> wrote:
> 
> > On Fri 26 May 2017, Jason Ekstrand wrote:
> > > This enum describes all of the states that a auxiliary compressed
> > > surface can have.  All of the states as well as normative language for
> > > referring to each of the compression operations is provided in the
> > > truly colossal comment for the new isl_aux_state enum.  There is also
> > > a diagram showing how surfaces move between the different states.
> > > ---
> > >  src/intel/isl/isl.h | 142 ++
> > ++
> > >  1 file changed, 142 insertions(+)
> > >
> > > diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> > > index b9d8fa8..df6d3e3 100644
> > > --- a/src/intel/isl/isl.h
> > > +++ b/src/intel/isl/isl.h
> > > @@ -560,6 +560,148 @@ enum isl_aux_usage {
> > > ISL_AUX_USAGE_CCS_E,
> > >  };
> > >
> > > +/**
> > > + * Enum for keeping track of the state an auxiliary compressed surface.
> >
> > This is really nice and helpful for everyone.
> >
> > I also learned something new from it: that a resolve on CCS_E also
> > ambiguates the aux surface. Do you have any insight on why the hardware
> > does that?
> >
> > > + *
> > > + * For any given auxiliary surface compression format (HiZ, CCS, or
> > MCS), any
> > > + * given slice (lod + array layer) can be in one of the six states
> > described
> > > + * by this enum.  Draw and resolve operations may cause the slice to
> > change
> > > + * from one state to another.  The six valid states are:
> >
> > I have one suggestion: please carefully distinguish between CCS_D and
> > CCS_E in the documentation. In my experience, muddy thinking where the
> > two are not cleanly distinguished leads to confused minds and confusing
> > code.
> >
> > For someone who already has a firm grasp on aux state, the ambiguous
> > term "CCS" poses no problem. That wise person automatically infers from
> > context if "CCS" applies to CCS_D, to CCS_E, or to both. But for someone
> > who's understanding of aux isn't as solid, the term "CCS" can lead to
> > incorrect inferences.
> >
> > For example, below you say that the partial resolve "operation is only
> > available for CCS". That's misleading. It should say "only available for
> > CCS_E".
> >
> > Another benefit: It becomes possible to document that
> > ISL_AUX_STATE_COMPRESSED_NO_CLEAR is valid only for CCS_E and HIZ, but
> > not valid for CCS_D and MCS.
> >
> 
> It is valid for MCS.  If you don't fast-clear but only render, then you're
> in that state.  It's only invalid for CCS_D.

Oops. You're right. compressed-no-clear is the "normal" state for MCS
compression blocks.

> 
> 
> > Other than the CCS_D/CCS_E distinction, the patch looks good to me. This
> > is a really nice addition to the driver.
> >
> 
> How about a section after the auxiliary compression ops section which goes
> into detail on each of the compression types and discusses which states are
> valid etc.

That sounds good, as long as there's not too much duplication between
the two sections.

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

Re: [Mesa-dev] [PATCH 3/4] configure.ac: remove unused Android specifics

[Chad Versace]

On Mon 05 Jun 2017, Emil Velikov wrote:
> From: Emil Velikov <emil.veli...@collabora.com>
> 
> The HAVE_ANDROID conditional has been unused as of commit 51accecce77
> ("mesa/dri: always link against shared glapi") and with that one gone we
> no longer need the host detection.
> 
> Cc: Chad Versace <chadvers...@chromium.org>
> Cc: Nicolas Boichat <drink...@chromium.org>
> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
> ---
>  configure.ac | 6 --
>  1 file changed, 6 deletions(-)

Since it's not used, then kill it. We can always revive it later if we
need it. (And I do suspect we may need the CHOST detection again).

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] mesa: Add _mesa_format_fallback_rgba_to_rgbx()

[Chad Versace]

On Tue 06 Jun 2017, Dylan Baker wrote:
> Quoting Chad Versace (2017-06-06 13:36:55)
> > The new function takes a mesa_format and, if the format is an alpha
> > format with a non-alpha variant, returns the non-alpha format.
> > Otherwise, it returns the original format.
> > 
> > Example:
> >   input -> output
> > 
> >   // Fallback exists
> >   MESA_FORMAT_R8G8B8X8_UNORM -> MESA_FORMAT_R8G8B8A8_UNORM
> >   MESA_FORMAT_RGBX_UNORM16 -> MESA_FORMAT_RGBA_UNORM16
> > 
> >   // No fallback
> >   MESA_FORMAT_R8G8B8A8_UNORM -> MESA_FORMAT_R8G8B8A8_UNORM
> >   MESA_FORMAT_Z_FLOAT32 -> MESA_FORMAT_Z_FLOAT32
> > 
> > i965 will use this for EGLImages and DRIimages.
> > ---
> >  src/mesa/Android.gen.mk  |  12 +++
> >  src/mesa/Makefile.am |   7 ++
> >  src/mesa/Makefile.sources|   2 +
> >  src/mesa/main/.gitignore |   1 +
> >  src/mesa/main/format_fallback.h  |  31 +++
> >  src/mesa/main/format_fallback.py | 180 
> > +++
> >  6 files changed, 233 insertions(+)
> >  create mode 100644 src/mesa/main/format_fallback.h
> >  create mode 100644 src/mesa/main/format_fallback.py

[snip]

> > +def main():
> > +pargs = parse_args()
> > +
> > +formats = {}
> > +for fmt in format_parser.parse(pargs.csv):
> > +formats[fmt.name] = fmt
> 
> You could simplify this as:
> formats = {f.name: f for f in format_parser.parse(pargs.csv)}

Thanks. I'll do that.

> 
> > +
> > +write_preamble(stdout)
> > +write_func_mesa_format_fallback_rgbx_to_rgba(stdout, formats)
> 
> We really shouldn't write to stdout like this, it can cause all kinds of
> breakages if there's ever a UTF-8 character (say ©) and the terminal doesn't
> have a unicode locale it'll fail,

Ugh. I wasn't aware that Python's stdout was broken. Is Python's
sys.stdout opened in "text" mode, and is that the cause of the
brokenness?

Does it still fail if stdout is redirected to a file? Because that's the
only case that matters here.

> if you just open the file you want (say one
> passed as an argument) then it doesn't matter what the console supports. We do
> this all over the place so it's not a blocker for me, but I still think it's a
> bad idea to write to stdout.

> If you decide not to change this you at the very least need to call
> stdout.flush() after write_func_mesa_format_fallback_Rgbx_to_rgba.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 0/6] i965: Add RGBX, RGBA configs, even on gen9

[Chad Versace]

More patches to break your formats... again ;)

The Android framework requires support for EGLConfigs with
HAL_PIXEL_FORMAT_RGBX_ and HAL_PIXEL_FORMAT_RGBA_. This prevents
Chrome OS from updating its Android drivers, because earlier this year
Intel disabled all rgbx formats for gen >=9 in brw_surface_formats.c.
This patch series safely (hopefully?) fixes that problem.

If you want the meat, read patches 2 and 6.

Chad Versace (6):
  mesa: Add _mesa_format_fallback_rgba_to_rgbx()
  i965: Add a RGBX->RGBA fallback for glEGLImageTextureTarget2D()
  i965: Rename some vague format members of brw_context
  i965/dri: Add intel_screen param to intel_create_winsys_renderbuffer
  i965: Move brw_context format arrays to intel_screen
  i965/dri: Support R8G8B8A8 and R8G8B8X8 configs

 src/mesa/Android.gen.mk  |  12 ++
 src/mesa/Makefile.am |   7 +
 src/mesa/Makefile.sources|   2 +
 src/mesa/drivers/dri/i965/brw_blorp.c|  10 +-
 src/mesa/drivers/dri/i965/brw_context.h  |   5 +-
 src/mesa/drivers/dri/i965/brw_meta_util.c|   2 +-
 src/mesa/drivers/dri/i965/brw_surface_formats.c  |  94 +++-
 src/mesa/drivers/dri/i965/brw_tex_layout.c   |   2 +-
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  10 +-
 src/mesa/drivers/dri/i965/intel_fbo.c|  34 -
 src/mesa/drivers/dri/i965/intel_fbo.h|   6 +-
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c|   4 +-
 src/mesa/drivers/dri/i965/intel_screen.c |  39 -
 src/mesa/drivers/dri/i965/intel_screen.h |   5 +
 src/mesa/drivers/dri/i965/intel_tex.c|   2 +-
 src/mesa/drivers/dri/i965/intel_tex_image.c  |  19 ++-
 src/mesa/main/.gitignore |   1 +
 src/mesa/main/format_fallback.h  |  31 
 src/mesa/main/format_fallback.py | 180 +++
 19 files changed, 392 insertions(+), 73 deletions(-)
 create mode 100644 src/mesa/main/format_fallback.h
 create mode 100644 src/mesa/main/format_fallback.py

-- 
2.13.0

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

[Mesa-dev] [PATCH 3/6] i965: Rename some vague format members of brw_context

[Chad Versace]

I'm swimming in a vortex of formats. Mesa formats, isl formats, DRI
formats, GL formats, etc.

It's easy to misinterpret the following brw_context members unless
you've recently read their definition.  In upcoming patches, I change
them from embedded arrays to simple pointers; after that, even their
definition doesn't help, because the MESA_FORMAT_COUNT hint will no
longer be present.

Rename them to prevent further confusion. While we're renaming, choose
shorter names too.

-format_supported_as_render_target
+mesa_format_supports_render

-render_target_format
+mesa_to_isl_render_format
---
 src/mesa/drivers/dri/i965/brw_blorp.c| 10 +-
 src/mesa/drivers/dri/i965/brw_context.h  |  4 ++--
 src/mesa/drivers/dri/i965/brw_meta_util.c|  2 +-
 src/mesa/drivers/dri/i965/brw_surface_formats.c  | 20 ++--
 src/mesa/drivers/dri/i965/brw_tex_layout.c   |  2 +-
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 10 +-
 src/mesa/drivers/dri/i965/intel_fbo.c|  2 +-
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c|  4 ++--
 src/mesa/drivers/dri/i965/intel_tex.c|  2 +-
 9 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c 
b/src/mesa/drivers/dri/i965/brw_blorp.c
index 28be620429..34f6bc4c84 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp.c
+++ b/src/mesa/drivers/dri/i965/brw_blorp.c
@@ -243,8 +243,8 @@ brw_blorp_to_isl_format(struct brw_context *brw, 
mesa_format format,
   return ISL_FORMAT_R16_UNORM;
default: {
   if (is_render_target) {
- assert(brw->format_supported_as_render_target[format]);
- return brw->render_target_format[format];
+ assert(brw->mesa_format_supports_render[format]);
+ return brw->mesa_to_isl_render_format[format];
   } else {
  return brw_isl_format_for_mesa_format(format);
   }
@@ -607,7 +607,7 @@ brw_blorp_copytexsubimage(struct brw_context *brw,
_mesa_get_format_base_format(dst_mt->format) == GL_DEPTH_STENCIL)
   return false;
 
-   if (!brw->format_supported_as_render_target[dst_image->TexFormat])
+   if (!brw->mesa_format_supports_render[dst_image->TexFormat])
   return false;
 
/* Source clipping shouldn't be necessary, since copytexsubimage (in
@@ -858,7 +858,7 @@ do_single_blorp_clear(struct brw_context *brw, struct 
gl_framebuffer *fb,
   struct blorp_batch batch;
   blorp_batch_init(>blorp, , brw, 0);
   blorp_fast_clear(, ,
-   brw->render_target_format[format],
+   brw->mesa_to_isl_render_format[format],
level, logical_layer, num_layers,
x0, y0, x1, y1);
   blorp_batch_finish();
@@ -884,7 +884,7 @@ do_single_blorp_clear(struct brw_context *brw, struct 
gl_framebuffer *fb,
   struct blorp_batch batch;
   blorp_batch_init(>blorp, , brw, 0);
   blorp_clear(, ,
-  brw->render_target_format[format],
+  brw->mesa_to_isl_render_format[format],
   ISL_SWIZZLE_IDENTITY,
   level, irb_logical_mt_layer(irb), num_layers,
   x0, y0, x1, y1,
diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
b/src/mesa/drivers/dri/i965/brw_context.h
index c15abe1d48..17a76f0808 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -1161,8 +1161,8 @@ struct brw_context
const struct brw_tracked_state render_atoms[76];
const struct brw_tracked_state compute_atoms[11];
 
-   enum isl_format render_target_format[MESA_FORMAT_COUNT];
-   bool format_supported_as_render_target[MESA_FORMAT_COUNT];
+   enum isl_format mesa_to_isl_render_format[MESA_FORMAT_COUNT];
+   bool mesa_format_supports_render[MESA_FORMAT_COUNT];
 
/* PrimitiveRestart */
struct {
diff --git a/src/mesa/drivers/dri/i965/brw_meta_util.c 
b/src/mesa/drivers/dri/i965/brw_meta_util.c
index cbc2dedde8..0342a527d7 100644
--- a/src/mesa/drivers/dri/i965/brw_meta_util.c
+++ b/src/mesa/drivers/dri/i965/brw_meta_util.c
@@ -289,7 +289,7 @@ brw_is_color_fast_clear_compatible(struct brw_context *brw,
 */
if (brw->gen >= 9 &&
brw_isl_format_for_mesa_format(mt->format) !=
-   brw->render_target_format[mt->format])
+   brw->mesa_to_isl_render_format[mt->format])
   return false;
 
/* Gen9 doesn't support fast clear on single-sampled SRGB buffers. When
diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c 
b/src/mesa/drivers/dri/i965/brw_surface_formats.c
index f878317e92..c33cafa836 100644
--- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
+++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
@@ -301,21 +301,21 @@ brw_init_surface_formats(struct brw_context *brw)
*/
   if (isl_format_supports_rendering(devinfo, render) &&
   (isl_format_supports_alpha_blending(devinfo, render) || is_integer)) 
{
-

[Mesa-dev] [PATCH 2/6] i965: Add a RGBX->RGBA fallback for glEGLImageTextureTarget2D()

[Chad Versace]

This enables support for importing RGBX EGLImage textures on
Skylake.

Chrome OS needs support for RGBX EGLImage textures because because
the Android framework produces HAL_PIXEL_FORMAT_RGBX winsys
surfaces, which the Chrome OS compositor consumes as dma_bufs.  On
hardware for which RGBX is unsupported or disabled, normally core Mesa
provides the RGBX->RGBA fallback during glTexStorage.  But the DRIimage
code bypasses core Mesa, so we must do the fallback in i965.
---
 src/mesa/drivers/dri/i965/intel_tex_image.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c 
b/src/mesa/drivers/dri/i965/intel_tex_image.c
index 649b3907d1..92c6c15c72 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_image.c
+++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
@@ -5,6 +5,7 @@
 #include "main/bufferobj.h"
 #include "main/context.h"
 #include "main/formats.h"
+#include "main/format_fallback.h"
 #include "main/glformats.h"
 #include "main/image.h"
 #include "main/pbo.h"
@@ -254,8 +255,22 @@ create_mt_for_dri_image(struct brw_context *brw,
struct gl_context *ctx = >ctx;
struct intel_mipmap_tree *mt;
uint32_t draw_x, draw_y;
+   mesa_format format = image->format;
+
+   if (!ctx->TextureFormatSupported[format]) {
+  /* The texture storage paths in core Mesa detect if the driver does not
+   * support the user-requested format, and then searches for a
+   * fallback format. The DRIimage code bypasses core Mesa, though. So we
+   * do the fallbacks here for important formats.
+   *
+   * We must support DRM_FOURCC_XBGR textures because the Android
+   * framework produces HAL_PIXEL_FORMAT_RGBX winsys surfaces, which
+   * the Chrome OS compositor consumes as dma_buf EGLImages.
+   */
+  format = _mesa_format_fallback_rgbx_to_rgba(format);
+   }
 
-   if (!ctx->TextureFormatSupported[image->format])
+   if (!ctx->TextureFormatSupported[format])
   return NULL;
 
/* Disable creation of the texture's aux buffers because the driver exposes
@@ -263,7 +278,7 @@ create_mt_for_dri_image(struct brw_context *brw,
 * buffer's content to the main buffer nor for invalidating the aux buffer's
 * content.
 */
-   mt = intel_miptree_create_for_bo(brw, image->bo, image->format,
+   mt = intel_miptree_create_for_bo(brw, image->bo, format,
 0, image->width, image->height, 1,
 image->pitch,
 MIPTREE_LAYOUT_DISABLE_AUX);
-- 
2.13.0

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

[Mesa-dev] [PATCH 5/6] i965: Move brw_context format arrays to intel_screen

[Chad Versace]

This allows us to query the driver's supported formats in i965's DRI code,
where often there is available a DRIscreen but no GL context.

To reduce diff noise, this patch does not completely remove
brw_context's format arrays. It just redeclares them as pointers which
point to the arrays in intel_screen.

Specifically, move these two arrays from brw_context to intel_screen:
mesa_to_isl_render_format[]
mesa_format_supports_render[]

And add a new array to intel_screen,
mesa_format_supportex_texture[]
which brw_init_surface_formats() copies to ctx->TextureFormatSupported.
---
 src/mesa/drivers/dri/i965/brw_context.h |  5 +-
 src/mesa/drivers/dri/i965/brw_surface_formats.c | 92 +++--
 src/mesa/drivers/dri/i965/intel_screen.c|  2 +
 src/mesa/drivers/dri/i965/intel_screen.h|  5 ++
 4 files changed, 64 insertions(+), 40 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
b/src/mesa/drivers/dri/i965/brw_context.h
index 17a76f0808..476981bfad 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -1161,8 +1161,8 @@ struct brw_context
const struct brw_tracked_state render_atoms[76];
const struct brw_tracked_state compute_atoms[11];
 
-   enum isl_format mesa_to_isl_render_format[MESA_FORMAT_COUNT];
-   bool mesa_format_supports_render[MESA_FORMAT_COUNT];
+   const enum isl_format *mesa_to_isl_render_format;
+   const bool *mesa_format_supports_render;
 
/* PrimitiveRestart */
struct {
@@ -1419,6 +1419,7 @@ void brw_upload_image_surfaces(struct brw_context *brw,
struct brw_stage_prog_data *prog_data);
 
 /* brw_surface_formats.c */
+void intel_screen_init_surface_formats(struct intel_screen *screen);
 void brw_init_surface_formats(struct brw_context *brw);
 bool brw_render_target_supported(struct brw_context *brw,
  struct gl_renderbuffer *rb);
diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c 
b/src/mesa/drivers/dri/i965/brw_surface_formats.c
index c33cafa836..a2bc1ded6d 100644
--- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
+++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
@@ -203,17 +203,16 @@ brw_isl_format_for_mesa_format(mesa_format mesa_format)
 }
 
 void
-brw_init_surface_formats(struct brw_context *brw)
+intel_screen_init_surface_formats(struct intel_screen *screen)
 {
-   const struct gen_device_info *devinfo = >screen->devinfo;
-   struct gl_context *ctx = >ctx;
-   int gen;
+   const struct gen_device_info *devinfo = >devinfo;
mesa_format format;
 
-   memset(>TextureFormatSupported, 0, 
sizeof(ctx->TextureFormatSupported));
+   memset(>mesa_format_supports_texture, 0,
+  sizeof(screen->mesa_format_supports_texture));
 
-   gen = brw->gen * 10;
-   if (brw->is_g4x || brw->is_haswell)
+   int gen = devinfo->gen * 10;
+   if (devinfo->is_g4x || devinfo->is_haswell)
   gen += 5;
 
for (format = MESA_FORMAT_NONE + 1; format < MESA_FORMAT_COUNT; format++) {
@@ -237,7 +236,7 @@ brw_init_surface_formats(struct brw_context *brw)
 
   if (isl_format_supports_sampling(devinfo, texture) &&
   (isl_format_supports_filtering(devinfo, texture) || is_integer))
-ctx->TextureFormatSupported[format] = true;
+screen->mesa_format_supports_texture[format] = true;
 
   /* Re-map some render target formats to make them supported when they
* wouldn't be using their format for texturing.
@@ -301,30 +300,30 @@ brw_init_surface_formats(struct brw_context *brw)
*/
   if (isl_format_supports_rendering(devinfo, render) &&
   (isl_format_supports_alpha_blending(devinfo, render) || is_integer)) 
{
-brw->mesa_to_isl_render_format[format] = render;
-brw->mesa_format_supports_render[format] = true;
+screen->mesa_to_isl_render_format[format] = render;
+screen->mesa_format_supports_render[format] = true;
   }
}
 
/* We will check this table for FBO completeness, but the surface format
 * table above only covered color rendering.
 */
-   brw->mesa_format_supports_render[MESA_FORMAT_Z24_UNORM_S8_UINT] = true;
-   brw->mesa_format_supports_render[MESA_FORMAT_Z24_UNORM_X8_UINT] = true;
-   brw->mesa_format_supports_render[MESA_FORMAT_S_UINT8] = true;
-   brw->mesa_format_supports_render[MESA_FORMAT_Z_FLOAT32] = true;
-   brw->mesa_format_supports_render[MESA_FORMAT_Z32_FLOAT_S8X24_UINT] = true;
-   if (brw->gen >= 8)
-  brw->mesa_format_supports_render[MESA_FORMAT_Z_UNORM16] = true;
+   screen->mesa_format_supports_render[MESA_FORMAT_Z24_UNORM_S8_UINT] = true;
+   screen->mesa_format_supports_render[MESA_FORMAT_Z24_UNORM_X8_UINT] = true;
+   screen->mesa_format_supports_render[MESA_FORMAT_S_UINT8] = true;
+   screen->mesa_format_supports_render[MESA_FORMAT_Z_FLOAT32] = true;
+   screen->mesa_format_supports_render[MESA_FORMAT_Z32_FLOAT_S8X24_UINT] = 
true;
+   if (gen >= 80)
+  

[Mesa-dev] [PATCH 6/6] i965/dri: Support R8G8B8A8 and R8G8B8X8 configs

[Chad Versace]

The Android framework requires support for EGLConfigs with
HAL_PIXEL_FORMAT_RGBX_ and HAL_PIXEL_FORMAT_RGBA_.

Even though all RGBX formats are disabled on gen9 by
brw_surface_formats.c, the new configs work correctly on Broxton thanks
to _mesa_format_fallback_rgbx_to_rgba().

On GLX, this creates no new configs, and therefore breaks no existing
apps. See in-patch comments for explanation. I tested with glxinfo and
glxgears on Skylake.

On Wayland, this also creates no new configs, and therfore breaks no
existing apps. (I tested with mesa-demos' eglinfo and es2gears_wayland
on Skylake). The reason differs from GLX, though. In
dri2_wl_add_configs_for_visual(), the format table contains only
B8G8R8X8, B8G8R8A8, and B5G6B5; and dri2_add_config() correctly matches
EGLConfig to format by inspecting channel masks.

On Android, in Chrome OS, I tested this on a Broxton device. I confirmed
that the Google Play Store's EGLSurface used HAL_PIXEL_FORMAT_RGBA_,
and that an Asteroid game's EGLSurface used HAL_PIXEL_FORMAT_RGBX_.
Both apps worked well. (Disclaimer: I didn't test this patch on Android
with Mesa master. I backported this patch series to an older Android
branch).
---
 src/mesa/drivers/dri/i965/intel_fbo.c| 24 ++--
 src/mesa/drivers/dri/i965/intel_screen.c | 23 ++-
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c 
b/src/mesa/drivers/dri/i965/intel_fbo.c
index 16d1325736..e56d30a2c0 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.c
+++ b/src/mesa/drivers/dri/i965/intel_fbo.c
@@ -34,6 +34,7 @@
 #include "main/teximage.h"
 #include "main/image.h"
 #include "main/condrender.h"
+#include "main/format_fallback.h"
 #include "util/hash_table.h"
 #include "util/set.h"
 
@@ -450,10 +451,29 @@ intel_create_winsys_renderbuffer(struct intel_screen 
*screen,
 
_mesa_init_renderbuffer(rb, 0);
rb->ClassID = INTEL_RB_CLASS;
+   rb->NumSamples = num_samples;
+
+   /* The base format and internal format must be derived from the user-visible
+* format (that is, the gl_config's format), even if we internally use
+* choose a different format for the renderbuffer. Otherwise, rendering may
+* use incorrect channel write masks.
+*/
rb->_BaseFormat = _mesa_get_format_base_format(format);
-   rb->Format = format;
rb->InternalFormat = rb->_BaseFormat;
-   rb->NumSamples = num_samples;
+
+   rb->Format = format;
+   if (!screen->mesa_format_supports_render[rb->Format]) {
+  /* The glRenderbufferStorage paths in core Mesa detect if the driver
+   * does not support the user-requested format, and then searches for
+   * a falback format. The DRI code bypasses core Mesa, though. So we do
+   * the fallbacks here.
+   *
+   * We must support MESA_FORMAT_R8G8B8X8 on Android because the Android
+   * framework requires HAL_PIXEL_FORMAT_RGBX winsys surfaces.
+   */
+  rb->Format = _mesa_format_fallback_rgbx_to_rgba(rb->Format);
+  assert(screen->mesa_format_supports_render[rb->Format]);
+   }
 
/* intel-specific methods */
rb->Delete = intel_delete_renderbuffer;
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index 563065b91f..a9d132f868 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -1547,7 +1547,28 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
static const mesa_format formats[] = {
   MESA_FORMAT_B5G6R5_UNORM,
   MESA_FORMAT_B8G8R8A8_UNORM,
-  MESA_FORMAT_B8G8R8X8_UNORM
+  MESA_FORMAT_B8G8R8X8_UNORM,
+
+  /* The 32-bit RGBA format must not precede the 32-bit BGRA format.
+   * Likewise for RGBX and BGRX.  Otherwise, the GLX client and the GLX
+   * server may disagree on which format the GLXFBConfig represents,
+   * resulting in swapped color channels.
+   *
+   * The problem, as of 2017-05-30:
+   * When matching a GLXFBConfig to a __DRIconfig, GLX ignores the channel
+   * order and chooses the first __DRIconfig with the expected channel
+   * sizes. Specifically, GLX compares the GLXFBConfig's and __DRIconfig's
+   * __DRI_ATTRIB_{CHANNEL}_SIZE but ignores __DRI_ATTRIB_{CHANNEL}_MASK.
+   *
+   * EGL does not suffer from this problem. It correctly compares the
+   * channel masks when matching EGLConfig to __DRIconfig.
+   */
+
+  /* Required by Android, for HAL_PIXEL_FORMAT_RGBA_. */
+  MESA_FORMAT_R8G8B8A8_UNORM,
+
+  /* Required by Android, for HAL_PIXEL_FORMAT_RGBX_. */
+  MESA_FORMAT_R8G8B8X8_UNORM,
};
 
/* GLX_SWAP_COPY_OML is not supported due to page flipping. */
-- 
2.13.0

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

[Mesa-dev] [PATCH 4/6] i965/dri: Add intel_screen param to intel_create_winsys_renderbuffer

[Chad Versace]

The param is currently unused. It will later be used it to support
R8G8B8X8 EGLConfigs on Skylake.
---
 src/mesa/drivers/dri/i965/intel_fbo.c|  8 +---
 src/mesa/drivers/dri/i965/intel_fbo.h|  6 --
 src/mesa/drivers/dri/i965/intel_screen.c | 14 --
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c 
b/src/mesa/drivers/dri/i965/intel_fbo.c
index 88e2fc7bf1..16d1325736 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.c
+++ b/src/mesa/drivers/dri/i965/intel_fbo.c
@@ -438,7 +438,8 @@ intel_nop_alloc_storage(struct gl_context * ctx, struct 
gl_renderbuffer *rb,
  * \param num_samples must be quantized.
  */
 struct intel_renderbuffer *
-intel_create_winsys_renderbuffer(mesa_format format, unsigned num_samples)
+intel_create_winsys_renderbuffer(struct intel_screen *screen,
+ mesa_format format, unsigned num_samples)
 {
struct intel_renderbuffer *irb = CALLOC_STRUCT(intel_renderbuffer);
if (!irb)
@@ -470,11 +471,12 @@ intel_create_winsys_renderbuffer(mesa_format format, 
unsigned num_samples)
  * \param num_samples must be quantized.
  */
 struct intel_renderbuffer *
-intel_create_private_renderbuffer(mesa_format format, unsigned num_samples)
+intel_create_private_renderbuffer(struct intel_screen *screen,
+  mesa_format format, unsigned num_samples)
 {
struct intel_renderbuffer *irb;
 
-   irb = intel_create_winsys_renderbuffer(format, num_samples);
+   irb = intel_create_winsys_renderbuffer(screen, format, num_samples);
irb->Base.Base.AllocStorage = intel_alloc_private_renderbuffer_storage;
 
return irb;
diff --git a/src/mesa/drivers/dri/i965/intel_fbo.h 
b/src/mesa/drivers/dri/i965/intel_fbo.h
index 2d2ef1ebc6..752e4f36a8 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.h
+++ b/src/mesa/drivers/dri/i965/intel_fbo.h
@@ -167,10 +167,12 @@ intel_rb_format(const struct intel_renderbuffer *rb)
 }
 
 extern struct intel_renderbuffer *
-intel_create_winsys_renderbuffer(mesa_format format, unsigned num_samples);
+intel_create_winsys_renderbuffer(struct intel_screen *screen,
+ mesa_format format, unsigned num_samples);
 
 struct intel_renderbuffer *
-intel_create_private_renderbuffer(mesa_format format, unsigned num_samples);
+intel_create_private_renderbuffer(struct intel_screen *screen,
+  mesa_format format, unsigned num_samples);
 
 struct gl_renderbuffer*
 intel_create_wrapped_renderbuffer(struct gl_context * ctx,
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index 22f6d9af03..7733d91fea 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -1200,11 +1200,11 @@ intelCreateBuffer(__DRIscreen *dri_screen,
}
 
/* setup the hardware-based renderbuffers */
-   rb = intel_create_winsys_renderbuffer(rgbFormat, num_samples);
+   rb = intel_create_winsys_renderbuffer(screen, rgbFormat, num_samples);
_mesa_attach_and_own_rb(fb, BUFFER_FRONT_LEFT, >Base.Base);
 
if (mesaVis->doubleBufferMode) {
-  rb = intel_create_winsys_renderbuffer(rgbFormat, num_samples);
+  rb = intel_create_winsys_renderbuffer(screen, rgbFormat, num_samples);
   _mesa_attach_and_own_rb(fb, BUFFER_BACK_LEFT, >Base.Base);
}
 
@@ -1217,10 +1217,11 @@ intelCreateBuffer(__DRIscreen *dri_screen,
   assert(mesaVis->stencilBits == 8);
 
   if (screen->devinfo.has_hiz_and_separate_stencil) {
- rb = intel_create_private_renderbuffer(MESA_FORMAT_Z24_UNORM_X8_UINT,
+ rb = intel_create_private_renderbuffer(screen,
+MESA_FORMAT_Z24_UNORM_X8_UINT,
 num_samples);
  _mesa_attach_and_own_rb(fb, BUFFER_DEPTH, >Base.Base);
- rb = intel_create_private_renderbuffer(MESA_FORMAT_S_UINT8,
+ rb = intel_create_private_renderbuffer(screen, MESA_FORMAT_S_UINT8,
 num_samples);
  _mesa_attach_and_own_rb(fb, BUFFER_STENCIL, >Base.Base);
   } else {
@@ -1228,7 +1229,8 @@ intelCreateBuffer(__DRIscreen *dri_screen,
   * Use combined depth/stencil. Note that the renderbuffer is
   * attached to two attachment points.
   */
- rb = intel_create_private_renderbuffer(MESA_FORMAT_Z24_UNORM_S8_UINT,
+ rb = intel_create_private_renderbuffer(screen,
+MESA_FORMAT_Z24_UNORM_S8_UINT,
 num_samples);
  _mesa_attach_and_own_rb(fb, BUFFER_DEPTH, >Base.Base);
  _mesa_attach_and_reference_rb(fb, BUFFER_STENCIL, >Base.Base);
@@ -1236,7 +1238,7 @@ intelCreateBuffer(__DRIscreen *dri_screen,
}
else if (mesaVis->depthBits == 16) {
   assert(mesaVis->stencilBits == 0);
-  rb = 

[Mesa-dev] [PATCH 1/6] mesa: Add _mesa_format_fallback_rgba_to_rgbx()

[Chad Versace]

The new function takes a mesa_format and, if the format is an alpha
format with a non-alpha variant, returns the non-alpha format.
Otherwise, it returns the original format.

Example:
  input -> output

  // Fallback exists
  MESA_FORMAT_R8G8B8X8_UNORM -> MESA_FORMAT_R8G8B8A8_UNORM
  MESA_FORMAT_RGBX_UNORM16 -> MESA_FORMAT_RGBA_UNORM16

  // No fallback
  MESA_FORMAT_R8G8B8A8_UNORM -> MESA_FORMAT_R8G8B8A8_UNORM
  MESA_FORMAT_Z_FLOAT32 -> MESA_FORMAT_Z_FLOAT32

i965 will use this for EGLImages and DRIimages.
---
 src/mesa/Android.gen.mk  |  12 +++
 src/mesa/Makefile.am |   7 ++
 src/mesa/Makefile.sources|   2 +
 src/mesa/main/.gitignore |   1 +
 src/mesa/main/format_fallback.h  |  31 +++
 src/mesa/main/format_fallback.py | 180 +++
 6 files changed, 233 insertions(+)
 create mode 100644 src/mesa/main/format_fallback.h
 create mode 100644 src/mesa/main/format_fallback.py

diff --git a/src/mesa/Android.gen.mk b/src/mesa/Android.gen.mk
index 42d4ba1969..8dcfb460e9 100644
--- a/src/mesa/Android.gen.mk
+++ b/src/mesa/Android.gen.mk
@@ -34,6 +34,7 @@ sources := \
main/enums.c \
main/api_exec.c \
main/dispatch.h \
+   main/format_fallback.c \
main/format_pack.c \
main/format_unpack.c \
main/format_info.h \
@@ -135,6 +136,17 @@ $(intermediates)/main/get_hash.h: 
$(glapi)/gl_and_es_API.xml \
$(LOCAL_PATH)/main/get_hash_params.py $(GET_HASH_GEN)
$(call es-gen)
 
+FORMAT_FALLBACK := $(LOCAL_PATH)/main/format_fallback.py
+format_fallback_deps := \
+   $(LOCAL_PATH)/main/formats.csv \
+   $(LOCAL_PATH)/main/format_parser.py \
+   $(FORMAT_FALLBACK)
+
+$(intermediates)/main/format_fallback.c: PRIVATE_SCRIPT := $(MESA_PYTHON2) 
$(FORMAT_FALLBACK)
+$(intermediates)/main/format_fallback.c: PRIVATE_XML :=
+$(intermediates)/main/format_fallback.c: $(format_fallback_deps)
+   $(call es-gen, $<)
+
 FORMAT_INFO := $(LOCAL_PATH)/main/format_info.py
 format_info_deps := \
$(LOCAL_PATH)/main/formats.csv \
diff --git a/src/mesa/Makefile.am b/src/mesa/Makefile.am
index 53f311d2a9..2c39374aef 100644
--- a/src/mesa/Makefile.am
+++ b/src/mesa/Makefile.am
@@ -37,6 +37,7 @@ include Makefile.sources
 
 EXTRA_DIST = \
drivers/SConscript \
+   main/format_fallback.py \
main/format_info.py \
main/format_pack.py \
main/format_parser.py \
@@ -54,6 +55,7 @@ EXTRA_DIST = \
 
 BUILT_SOURCES = \
main/get_hash.h \
+   main/format_fallback.c \
main/format_info.h \
main/format_pack.c \
main/format_unpack.c \
@@ -70,6 +72,11 @@ main/get_hash.h: ../mapi/glapi/gen/gl_and_es_API.xml 
main/get_hash_params.py \
$(PYTHON_GEN) $(srcdir)/main/get_hash_generator.py \
-f $(srcdir)/../mapi/glapi/gen/gl_and_es_API.xml > $@
 
+main/format_fallback.c: main/format_fallback.py \
+main/format_parser.py \
+   main/formats.csv
+   $(PYTHON_GEN) $(srcdir)/main/format_fallback.py 
$(srcdir)/main/formats.csv > $@
+
 main/format_info.h: main/formats.csv \
 main/format_parser.py main/format_info.py
$(PYTHON_GEN) $(srcdir)/main/format_info.py $(srcdir)/main/formats.csv 
> $@
diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
index 8a65fbe663..642100b956 100644
--- a/src/mesa/Makefile.sources
+++ b/src/mesa/Makefile.sources
@@ -94,6 +94,8 @@ MAIN_FILES = \
main/ffvertex_prog.h \
main/fog.c \
main/fog.h \
+   main/format_fallback.h \
+   main/format_fallback.c \
main/format_info.h \
main/format_pack.h \
main/format_pack.c \
diff --git a/src/mesa/main/.gitignore b/src/mesa/main/.gitignore
index 836d8f104a..8cc33cfee6 100644
--- a/src/mesa/main/.gitignore
+++ b/src/mesa/main/.gitignore
@@ -4,6 +4,7 @@ enums.c
 remap_helper.h
 get_hash.h
 get_hash.h.tmp
+format_fallback.c
 format_info.h
 format_info.c
 format_pack.c
diff --git a/src/mesa/main/format_fallback.h b/src/mesa/main/format_fallback.h
new file mode 100644
index 00..5ca8269b2f
--- /dev/null
+++ b/src/mesa/main/format_fallback.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright 2017 Google
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included
+ * in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 

Re: [Mesa-dev] [PATCH 14/30] intel/isl: Add an enum for describing auxiliary compression state

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> This enum describes all of the states that a auxiliary compressed
> surface can have.  All of the states as well as normative language for
> referring to each of the compression operations is provided in the
> truly colossal comment for the new isl_aux_state enum.  There is also
> a diagram showing how surfaces move between the different states.
> ---
>  src/intel/isl/isl.h | 142 
> 
>  1 file changed, 142 insertions(+)
> 
> diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> index b9d8fa8..df6d3e3 100644
> --- a/src/intel/isl/isl.h
> +++ b/src/intel/isl/isl.h
> @@ -560,6 +560,148 @@ enum isl_aux_usage {
> ISL_AUX_USAGE_CCS_E,
>  };
>  
> +/**
> + * Enum for keeping track of the state an auxiliary compressed surface.

This is really nice and helpful for everyone.

I also learned something new from it: that a resolve on CCS_E also
ambiguates the aux surface. Do you have any insight on why the hardware
does that?

> + *
> + * For any given auxiliary surface compression format (HiZ, CCS, or MCS), any
> + * given slice (lod + array layer) can be in one of the six states described
> + * by this enum.  Draw and resolve operations may cause the slice to change
> + * from one state to another.  The six valid states are:

I have one suggestion: please carefully distinguish between CCS_D and
CCS_E in the documentation. In my experience, muddy thinking where the
two are not cleanly distinguished leads to confused minds and confusing
code.

For someone who already has a firm grasp on aux state, the ambiguous
term "CCS" poses no problem. That wise person automatically infers from
context if "CCS" applies to CCS_D, to CCS_E, or to both. But for someone
who's understanding of aux isn't as solid, the term "CCS" can lead to
incorrect inferences.

For example, below you say that the partial resolve "operation is only
available for CCS". That's misleading. It should say "only available for
CCS_E".

Another benefit: It becomes possible to document that
ISL_AUX_STATE_COMPRESSED_NO_CLEAR is valid only for CCS_E and HIZ, but
not valid for CCS_D and MCS.

Other than the CCS_D/CCS_E distinction, the patch looks good to me. This
is a really nice addition to the driver.

One more comment at the end...

> + *
> + *1) Clear:  In this state, each block in the auxiliary surface contains 
> a
> + *   magic value that indicates that the block is in the clear state.  If
> + *   a block is in the clear state, it's values in the primary surface 
> are
> + *   ignored and the color of the samples in the block is taken either 
> the
> + *   RENDER_SURFACE_STATE packet for color or 3DSTATE_CLEAR_PARAMS for
> + *   depth.  Since neither the primary surface nor the auxiliary surface
> + *   contains the clear value, the surface can be cleared to a different
> + *   color by simply changing the clear color without modifying either
> + *   surface.
> + *
> + *2) Compressed w/ Clear:  In this state, neither the auxiliary surface
> + *   nor the primary surface has a complete representation of the data.
> + *   Instead, both surfaces must be used together or else rendering
> + *   corruption may occur.  Depending on the auxiliary compression format
> + *   and the data, any given block in the primary surface may contain 
> all,
> + *   some, or none of the data required to reconstruct the actual sample
> + *   values.  Blocks may also be in the clear state (see Clear) and have
> + *   their value taken from outside the surface.
> + *
> + *3) Compressed w/o Clear:  This state is identical to the state above
> + *   except that no blocks are in the clear state.  In this state, all of
> + *   the data required to reconstruct the final sample values is 
> contained
> + *   in the auxiliary and primary surface and the clear value is not
> + *   considered.
> + *
> + *4) Resolved:  In this state, the primary surface contains 100% of the
> + *   data.  The auxiliary surface is also valid so the surface can be
> + *   validly used with or without aux enabled.  The auxiliary surface 
> may,
> + *   however, contain non-trivial data and any update to the primary
> + *   surface with aux disabled will cause the two to get out of sync.
> + *
> + *5) Pass-through:  In this state, the primary surface contains 100% of 
> the
> + *   data and every block in the auxiliary surface contains a magic value
> + *   which indicates that the auxiliary surface should be ignored and the
> + *   only the primary surface should be considered.  Updating the primary
> + *   surface without aux works fine and can be done repeatedly in this
> + *   mode.  Writing to a surface in pass-through mode with aux enabled 
> may
> + *   cause the auxiliary buffer to contain non-trivial data and no longer
> + *   be in the pass-through state.
> + *
> 

Re: [Mesa-dev] [PATCH] i965: Don't try to resolve CCS with MESA_FORMAT_NONE.

[Chad Versace]

On Sun 04 Jun 2017, Jason Ekstrand wrote:
> On June 4, 2017 5:36:57 PM Kenneth Graunke  wrote:
> 
> > On Sunday, June 4, 2017 3:27:04 PM PDT Jason Ekstrand wrote:
> > > How does the texture even have a format of MESA_FORMAT_NONE?  That seems
> > > like the first question to ask.
> > 
> > It's the window system buffer, and it actually has a format of
> > B8G8R8A8_UNORM_SRGB...my guess is just that _Format isn't set yet.
> 
> That seems like the bigger problem :-)

I was afraid that my patch would uncover long-hidden bugs like this.
Thanks for fixing it quickly.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 13/30] i965: Combine render target resolve code

[Chad Versace]

There's a patch on your branch I didn't see on mesa-dev.
   Subject: i965: Be a bit more conservative about certain resolves 
It has my r-b.

I have comments on this patch...

On Fri 26 May 2017, Jason Ekstrand wrote:
> We have two different bits of resolve code for render targets: one in
> brw_draw where it's always been and one in brw_context to deal with sRGB
> on gen9.  Let's pull them together.
> ---
>  src/mesa/drivers/dri/i965/brw_context.c | 47 
> -
>  src/mesa/drivers/dri/i965/brw_draw.c| 34 
>  2 files changed, 29 insertions(+), 52 deletions(-)



> +  /* For layered rendering non-compressed fast cleared buffers need to be
> +   * resolved. Surface state can carry only one fast color clear value
> +   * while each layer may have its own fast clear color value. For
> +   * compressed buffers color value is available in the color buffer.
> +   */
> +  if (irb->layer_count > 1 &&
> +  !(irb->mt->aux_disable & INTEL_AUX_DISABLE_CCS) &&
> +  !intel_miptree_is_lossless_compressed(brw, mt)) {

This condition smells bad. It smells like a shot in the dark. It smells
like a haphazard guess. "We haven't permanently disabled CCS for this
miptree. And it lacks CCS_E. So, well, it probably has CCS_D, I guess.".

I would much rather see the condition with something more certain.
Something like:

if (irb->layer_count > 1 &&
intel_miptree_has_css_d_in_layer_range(brw, mt, irb->mt_level, 
irb->mt_layer, irb->layer_count))

Anway, this patch is a good cleanup, and functional changes like I'm
requesting don't belong in a refactoring patch like this one.

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

> + assert(brw->gen >= 8);
> +
> + intel_miptree_resolve_color(brw, mt, irb->mt_level, 1,
> + irb->mt_layer, irb->layer_count, 0);
> +  }
> }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 12/30] i965/blorp: Move MCS allocation earlier for clears

[Chad Versace]

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 08/30] i965: Inline renderbuffer_att_set_needs_depth_resolve

[Chad Versace]

On Fri 02 Jun 2017, Jason Ekstrand wrote:
> On Fri, Jun 2, 2017 at 2:41 PM, Chad Versace <[1]chadvers...@chromium.org>
> wrote:
> 
> I believe you could simplify this by eliminating the 'else' branch. As
> long as depth_irb->layer_count == 1 for non-layered renderbuffers (and
> I really hope it is), then the 'for' loop does the right thing.
> 
> 
> Sure.  I was sort-of trying to avoid functional changes.  That said... I'm
> happy to make the change.  Lots of stuff would suddenly get cleaner.  Mind if
> that's a follow-on patch to the series?

It was just a suggestion, a nice-to-have.

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 10/30] i965/blorp: Take an explicit fast clear op in resolve_color

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_blorp.c | 15 ++-
>  src/mesa/drivers/dri/i965/brw_blorp.h |  3 ++-
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 15 ++-
>  3 files changed, 18 insertions(+), 15 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 09/30] i965/miptree: Move color resolve on map to intel_miptree_map

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> None of the other methods such as blit work with CCS either so we need
> to do the resolve for all maps.

Not exactly. No down-resolve is needed for maps with
GL_MAP_INVALIDATE_RANGE_BIT. But that's a separate problem for
a separate patch.

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

> This change also makes us only resolve
> the one slice we're mapping and not the entire image.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 08/30] i965: Inline renderbuffer_att_set_needs_depth_resolve

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_clear.c | 12 ++--
>  src/mesa/drivers/dri/i965/brw_draw.c  | 12 +++-
>  src/mesa/drivers/dri/i965/intel_fbo.c | 15 ---
>  src/mesa/drivers/dri/i965/intel_fbo.h |  3 ---
>  4 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_clear.c 
> b/src/mesa/drivers/dri/i965/brw_clear.c
> index adaf250..9f4a7e6 100644
> --- a/src/mesa/drivers/dri/i965/brw_clear.c
> +++ b/src/mesa/drivers/dri/i965/brw_clear.c
> @@ -207,7 +207,7 @@ brw_fast_clear_depth(struct gl_context *ctx)
> brw_emit_pipe_control_flush(brw, PIPE_CONTROL_DEPTH_STALL);
> }
>  
> -   if (fb->MaxNumLayers > 0) {
> +   if (depth_att->Layered) {
>for (unsigned layer = 0; layer < depth_irb->layer_count; layer++) {
>   intel_hiz_exec(brw, mt, depth_irb->mt_level,
>  depth_irb->mt_layer + layer,

There's a hidden 'else' branch here. My comment below applies to it too.

> @@ -236,7 +236,15 @@ brw_fast_clear_depth(struct gl_context *ctx)
> /* Now, the HiZ buffer contains data that needs to be resolved to the 
> depth
>  * buffer.
>  */
> -   intel_renderbuffer_att_set_needs_depth_resolve(depth_att);
> +   if (fb->MaxNumLayers > 0) {
> +  for (unsigned layer = 0; layer < depth_irb->layer_count; layer++) {
> + intel_miptree_slice_set_needs_depth_resolve(mt, depth_irb->mt_level,
> + depth_irb->mt_layer + 
> layer);
> +  }
> +   } else {
> +  intel_miptree_slice_set_needs_depth_resolve(mt, depth_irb->mt_level,
> +  depth_irb->mt_layer);
> +   }

I believe you could simplify this by eliminating the 'else' branch. As
long as depth_irb->layer_count == 1 for non-layered renderbuffers (and
I really hope it is), then the 'for' loop does the right thing.

>  
> return true;
>  }
> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c 
> b/src/mesa/drivers/dri/i965/brw_draw.c
> index 23a3c6c..f728731 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> @@ -373,7 +373,17 @@ brw_postdraw_set_buffers_need_resolve(struct brw_context 
> *brw)
> if (back_irb)
>back_irb->need_downsample = true;
> if (depth_irb && brw_depth_writes_enabled(brw)) {
> -  intel_renderbuffer_att_set_needs_depth_resolve(depth_att);
> +  if (depth_att->Layered) {
> + for (unsigned layer = 0; layer < depth_irb->layer_count; layer++) {
> +intel_miptree_slice_set_needs_depth_resolve(depth_irb->mt,
> +depth_irb->mt_level,
> +depth_irb->mt_layer 
> + layer);
> + }
> +  } else {

And this branch too.

> + intel_miptree_slice_set_needs_depth_resolve(depth_irb->mt,
> + depth_irb->mt_level,
> + depth_irb->mt_layer);
> +  }
>brw_render_cache_set_add_bo(brw, depth_irb->mt->bo);
> }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 07/30] i965: Get rid of intel_renderbuffer_resolve_*

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> There is exactly one caller so it's a bit pointless to have all of this
> plumbing.  Just inline it at the one place it's used.
> ---
>  src/mesa/drivers/dri/i965/brw_context.c |  7 +--
>  src/mesa/drivers/dri/i965/intel_fbo.c   | 26 --
>  src/mesa/drivers/dri/i965/intel_fbo.h   | 24 
>  3 files changed, 5 insertions(+), 52 deletions(-)

This patch brought back old memories. You're ripping up the code from my
first major project at Intel. And it feels good to see it cleaned up.

Patch 7 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 06/30] i965/miptree: Refactor intel_miptree_resolve_color

[Chad Versace]

On Wed 31 May 2017, Jason Ekstrand wrote:
> On Tue, May 30, 2017 at 12:40 AM, Pohjolainen, Topi <[1]
> topi.pohjolai...@gmail.com> wrote:
> 
> On Fri, May 26, 2017 at 04:30:10PM -0700, Jason Ekstrand wrote:
> > The new version now takes a range of levels as well as a range of
> > layers.  It should also be a tiny bit faster because it only walks the
> > resolve_map list once instead of once per layer.
> > ---
> >  src/mesa/drivers/dri/i965/brw_blorp.c         |  3 +-
> >  src/mesa/drivers/dri/i965/brw_context.c       |  9 +++---
> >  src/mesa/drivers/dri/i965/brw_draw.c          |  2 +-
> >  src/mesa/drivers/dri/i965/intel_blit.c        |  8 ++---
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 46


> Original (a dozen lines earlier) checked for num_layers. Maybe:
> 
>          /* Mipmapped fast clear is only supported for gen8+. */
> 
> > +      assert(brw->gen >= 8 || map->level == 0);
> 
> 
> Actually, I think I want "assert(brw->gen >= 8 || (map->level == 0 && map->
> layer == 0))"

Which is what appears in wip/i965-resolve-rework-v3. The patch in that
branch 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 05/30] i965/miptree: Clean up the depth resolve helpers a little

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 70 
> ---
>  1 file changed, 30 insertions(+), 40 deletions(-)

Patch 5 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 04/30] i965/miptree: Store fast clear colors in an isl_color_value

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> This commit, out of necessity, makes a number of changes at once:
> 
>  1) Changes intel_mipmap_tree to store the clear color for both color
> and depth as an isl_color_value.
> 
>  2) Changes the depth/stencil emit code to do the format conversion of
> the depth clear value on Haswell and earlier instead of pulling a
> uint32_t directly from the miptree.
> 
>  3) Changes ISL's depth/stencil emit code to perform the format
> conversion of the depth clear value on Haswell and earlier instead
> of assuming that the depth value in the float is pre-converted.
> 
>  4) Changes blorp to pass the depth value through as a float.
> ---
>  src/intel/blorp/blorp_genX_exec.h|  2 +-
>  src/intel/isl/isl_emit_depth_stencil.c   | 19 
>  src/mesa/drivers/dri/i965/brw_blorp.c| 18 +++-
>  src/mesa/drivers/dri/i965/brw_clear.c| 15 ++-
>  src/mesa/drivers/dri/i965/brw_meta_util.c| 56 
> +++-
>  src/mesa/drivers/dri/i965/brw_meta_util.h|  7 +--
>  src/mesa/drivers/dri/i965/brw_misc_state.c   | 23 +-
>  src/mesa/drivers/dri/i965/brw_state.h|  3 ++
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  2 +-
>  src/mesa/drivers/dri/i965/gen6_depth_state.c |  7 ++-
>  src/mesa/drivers/dri/i965/gen7_misc_state.c  |  7 ++-
>  src/mesa/drivers/dri/i965/gen8_depth_state.c |  2 +-
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c| 31 -
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h| 34 ++----
>  14 files changed, 90 insertions(+), 136 deletions(-)

Patch 4 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 01/30] i965: Mark depth surfaces as needing a HiZ resolve after blitting

[Chad Versace]

On Fri 02 Jun 2017, Jason Ekstrand wrote:
> On Fri, Jun 2, 2017 at 1:01 PM, Chad Versace <[1]chadvers...@chromium.org>
> wrote:
> 
> On Wed 31 May 2017, Pohjolainen, Topi wrote:
> > On Wed, May 31, 2017 at 10:22:09AM -0700, Jason Ekstrand wrote:
> > > On Tue, May 30, 2017 at 7:29 AM, Pohjolainen, Topi <
> > > [2]topi.pohjolai...@gmail.com> wrote:
> 
> > > > > On Fri, May 26, 2017 at 04:30:05PM -0700, Jason Ekstrand wrote:
> > > > > > Cc: "17.0 17.1" <[3]mesa-sta...@lists.freedesktop.org>
> > > > > > ---
> > > > > >  src/mesa/drivers/dri/i965/intel_blit.c | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/src/mesa/drivers/dri/i965/intel_blit.c
> > > > b/src/mesa/drivers/dri/i965/intel_blit.c
> > > > > > index 2925fc2..b1e1eaa 100644
> > > > > > --- a/src/mesa/drivers/dri/i965/intel_blit.c
> > > > > > +++ b/src/mesa/drivers/dri/i965/intel_blit.c
> > > > > > @@ -329,6 +329,7 @@ intel_miptree_blit(struct brw_context *brw,
> > > > > >     intel_miptree_slice_resolve_depth(brw, dst_mt, dst_level,
> > > > dst_slice);
> > > > > >     intel_miptree_resolve_color(brw, src_mt, src_level,
> src_slice, 1,
> > > > 0);
> > > > > >     intel_miptree_resolve_color(brw, dst_mt, dst_level,
> dst_slice, 1,
> > > > 0);
> > > > > > +   intel_miptree_slice_set_needs_hiz_resolve(dst_mt, dst_level,
> > > > dst_slice);
> 
> Something feels wrong. Suppose that, before the blit, the HiZ buffer
> contains significant data. Here, we schedule an op to invalidate the HiZ
> data. Also, suppose the the below call to emit_miptree_blit() rejects
> the blit (maybe because the pitch is too big) and returns false. The
> failed blit will likely emit a GL error or, in some cases, i965 will
> fallback to to doing the blit with the 3D engine. In either case, there
> is a likely bug waiting because the HiZ buffer is scheduled to have its
> HiZ data invalidated before its next use, but the HiZ data is still
> significant.
> 
> 
> I agree that you are technically mostly correct.  However, I don't think it's
> actually going to matter in practice for 3 reasons:
> 
>  1) The GL error case is bogus.  We aren't allowed to start modifying things
> and then throw a GL error unless that error is OUT_OF_MEMORY.  All of the
> error-checking should have happened up-front and it is our job to succeed
> somehow.
> 
>  2) By the time we get to a blit path, we've given up on HiZ.  We never fall
> back from BLIT to render.  If BLIT fails, we fall back to software at which
> point we're going to write without HiZ anyway so setting needs_hiz_resolve is
> fine.
> 
>  3) There is no bug waiting.  Since we do a depth resolve first, the HiZ and
> depth will be consistent and anything that anything that would use it with HiZ
> will see the fact that it needs a HiZ resolve and do one.  You can't get into 
> a
> case where we say that it needs a hiz resolve and then edit HiZ without
> resolving.  The worst that happens if I'm completely wrong about (1) and (2) 
> is
> that you get more HiZ resolves than needed.

You convinced with me point 3. I failed to take into account the depth
resolve a few lines above.

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 03/30] intel/isl: Add a helper for determining if a color is 0/1

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> ---
>  src/intel/isl/isl.c | 19 +++
>  src/intel/isl/isl.h |  4 
>  2 files changed, 23 insertions(+)
> 
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index 321850e..4eec2fd 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -267,6 +267,25 @@ isl_tiling_get_info(const struct isl_device *dev,
> return true;
>  }
>  
> +bool
> +isl_color_value_is_zero_one(union isl_color_value value,
> +enum isl_format format)
> +{
> +   if (isl_format_has_int_channel(format)) {
> +  for (unsigned i = 0; i < 4; i++) {

This patch assumes the all four channels of isl_color_value are
initialized, even if the isl_format has fewer than four channels.
This smells like the seed of a hard-to-reproduce bug.

Since it's not possible today to query the number of channels in an
isl_format, please at least add a scary comment on the function about
this.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/30] i965/surface_state: Images can't handle CCS at all

[Chad Versace]

On Fri 26 May 2017, Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)

Patch 2 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 01/30] i965: Mark depth surfaces as needing a HiZ resolve after blitting

[Chad Versace]

On Wed 31 May 2017, Pohjolainen, Topi wrote:
> On Wed, May 31, 2017 at 10:22:09AM -0700, Jason Ekstrand wrote:
> > On Tue, May 30, 2017 at 7:29 AM, Pohjolainen, Topi <
> > topi.pohjolai...@gmail.com> wrote:

> > > > On Fri, May 26, 2017 at 04:30:05PM -0700, Jason Ekstrand wrote:
> > > > > Cc: "17.0 17.1" 
> > > > > ---
> > > > >  src/mesa/drivers/dri/i965/intel_blit.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/src/mesa/drivers/dri/i965/intel_blit.c
> > > b/src/mesa/drivers/dri/i965/intel_blit.c
> > > > > index 2925fc2..b1e1eaa 100644
> > > > > --- a/src/mesa/drivers/dri/i965/intel_blit.c
> > > > > +++ b/src/mesa/drivers/dri/i965/intel_blit.c
> > > > > @@ -329,6 +329,7 @@ intel_miptree_blit(struct brw_context *brw,
> > > > > intel_miptree_slice_resolve_depth(brw, dst_mt, dst_level,
> > > dst_slice);
> > > > > intel_miptree_resolve_color(brw, src_mt, src_level, src_slice, 1,
> > > 0);
> > > > > intel_miptree_resolve_color(brw, dst_mt, dst_level, dst_slice, 1,
> > > 0);
> > > > > +   intel_miptree_slice_set_needs_hiz_resolve(dst_mt, dst_level,
> > > dst_slice);

Something feels wrong. Suppose that, before the blit, the HiZ buffer
contains significant data. Here, we schedule an op to invalidate the HiZ
data. Also, suppose the the below call to emit_miptree_blit() rejects
the blit (maybe because the pitch is too big) and returns false. The
failed blit will likely emit a GL error or, in some cases, i965 will
fallback to to doing the blit with the 3D engine. In either case, there
is a likely bug waiting because the HiZ buffer is scheduled to have its
HiZ data invalidated before its next use, but the HiZ data is still
significant.

The intel_miptree_slice_set_needs_hiz_resolve() needs to happen *after*
the successfull call to emit_miptree_blit().

> > > > >
> > > > > if (src_flip)
> > > > >src_y = minify(src_mt->physical_height0, src_level -
> > > src_mt->first_level) - src_y - height;
> > > > > @@ -387,6 +388,7 @@ intel_miptree_copy(struct brw_context *brw,
> > > > > intel_miptree_slice_resolve_depth(brw, dst_mt, dst_level,
> > > dst_slice);
> > > > > intel_miptree_resolve_color(brw, src_mt, src_level, src_slice, 1,
> > > 0);
> > > > > intel_miptree_resolve_color(brw, dst_mt, dst_level, dst_slice, 1,
> > > 0);
> > > > > +   intel_miptree_slice_set_needs_hiz_resolve(dst_mt, dst_level,
> > > dst_slice);

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

[Mesa-dev] [PATCH] i965: Replace 0 with ISL_FORMAT_UNSUPPORTED in format table (v2)

[Chad Versace]

From: Chad Versace <chadvers...@chromium.org>

When given an *unsupported* mesa_format,
brw_isl_format_for_mesa_format() returned 0, a *valid* isl_format,
ISL_FORMAT_R32G32B32A32_FLOAT.  The problem is that
brw_isl_format_for_mesa_format's inner table used 0 instead of
ISL_FORMAT_UNSUPPORTED to indicate unsupported mesa formats.

Some callers of brw_isl_format_for_mesa_format() were aware of this
weirdness, and worked around it. This patch removes those workarounds.

Tested on Broadwell as below, no regressions:

> deqp-egl --deqp-case='dEQP-EGL.functional.image.modify.*'
Test run totals:
  Passed:24/37 (64.9%)
  Failed:0/37 (0.0%)
  Not supported: 13/37 (35.1%)
  Warnings:  0/37 (0.0%)

> deqp-gles3 --deqp-case='dEQP-GLES3.functional.texture.format.*'
Test run totals:
  Passed:530/530 (100.0%)
  Failed:0/530 (0.0%)
  Not supported: 0/530 (0.0%)
  Warnings:  0/530 (0.0%)


v2: Ensure that all array elements are initialized to
  ISL_FORMAT_UNSUPPORTED, even when new formats are added to enum
  mesa_format, by using an designated range initializer.
---

 src/mesa/drivers/dri/i965/brw_surface_formats.c  | 96 ++--
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  2 +-
 2 files changed, 6 insertions(+), 92 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c 
b/src/mesa/drivers/dri/i965/brw_surface_formats.c
index 52d3acb..f878317 100644
--- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
+++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
@@ -36,31 +36,19 @@ brw_isl_format_for_mesa_format(mesa_format mesa_format)
 * staying in sync, so we initialize to 0 even though
 * ISL_FORMAT_R32G32B32A32_FLOAT happens to also be 0.
 */
-   static const uint32_t table[MESA_FORMAT_COUNT] =
-   {
-  [MESA_FORMAT_A8B8G8R8_UNORM] = 0,
+   static const enum isl_format table[MESA_FORMAT_COUNT] = {
+  [0 ... MESA_FORMAT_COUNT-1] = ISL_FORMAT_UNSUPPORTED,
+
   [MESA_FORMAT_R8G8B8A8_UNORM] = ISL_FORMAT_R8G8B8A8_UNORM,
   [MESA_FORMAT_B8G8R8A8_UNORM] = ISL_FORMAT_B8G8R8A8_UNORM,
-  [MESA_FORMAT_A8R8G8B8_UNORM] = 0,
-  [MESA_FORMAT_X8B8G8R8_UNORM] = 0,
   [MESA_FORMAT_R8G8B8X8_UNORM] = ISL_FORMAT_R8G8B8X8_UNORM,
   [MESA_FORMAT_B8G8R8X8_UNORM] = ISL_FORMAT_B8G8R8X8_UNORM,
-  [MESA_FORMAT_X8R8G8B8_UNORM] = 0,
-  [MESA_FORMAT_BGR_UNORM8] = 0,
   [MESA_FORMAT_RGB_UNORM8] = ISL_FORMAT_R8G8B8_UNORM,
   [MESA_FORMAT_B5G6R5_UNORM] = ISL_FORMAT_B5G6R5_UNORM,
-  [MESA_FORMAT_R5G6B5_UNORM] = 0,
   [MESA_FORMAT_B4G4R4A4_UNORM] = ISL_FORMAT_B4G4R4A4_UNORM,
-  [MESA_FORMAT_A4R4G4B4_UNORM] = 0,
-  [MESA_FORMAT_A1B5G5R5_UNORM] = 0,
   [MESA_FORMAT_B5G5R5A1_UNORM] = ISL_FORMAT_B5G5R5A1_UNORM,
-  [MESA_FORMAT_A1R5G5B5_UNORM] = 0,
-  [MESA_FORMAT_L4A4_UNORM] = 0,
   [MESA_FORMAT_L8A8_UNORM] = ISL_FORMAT_L8A8_UNORM,
-  [MESA_FORMAT_A8L8_UNORM] = 0,
   [MESA_FORMAT_L16A16_UNORM] = ISL_FORMAT_L16A16_UNORM,
-  [MESA_FORMAT_A16L16_UNORM] = 0,
-  [MESA_FORMAT_B2G3R3_UNORM] = 0,
   [MESA_FORMAT_A_UNORM8] = ISL_FORMAT_A8_UNORM,
   [MESA_FORMAT_A_UNORM16] = ISL_FORMAT_A16_UNORM,
   [MESA_FORMAT_L_UNORM8] = ISL_FORMAT_L8_UNORM,
@@ -71,29 +59,16 @@ brw_isl_format_for_mesa_format(mesa_format mesa_format)
   [MESA_FORMAT_YCBCR] = ISL_FORMAT_YCRCB_SWAPUVY,
   [MESA_FORMAT_R_UNORM8] = ISL_FORMAT_R8_UNORM,
   [MESA_FORMAT_R8G8_UNORM] = ISL_FORMAT_R8G8_UNORM,
-  [MESA_FORMAT_G8R8_UNORM] = 0,
   [MESA_FORMAT_R_UNORM16] = ISL_FORMAT_R16_UNORM,
   [MESA_FORMAT_R16G16_UNORM] = ISL_FORMAT_R16G16_UNORM,
-  [MESA_FORMAT_G16R16_UNORM] = 0,
   [MESA_FORMAT_B10G10R10A2_UNORM] = ISL_FORMAT_B10G10R10A2_UNORM,
-  [MESA_FORMAT_S8_UINT_Z24_UNORM] = 0,
-  [MESA_FORMAT_Z24_UNORM_S8_UINT] = 0,
-  [MESA_FORMAT_Z_UNORM16] = 0,
-  [MESA_FORMAT_Z24_UNORM_X8_UINT] = 0,
-  [MESA_FORMAT_X8_UINT_Z24_UNORM] = 0,
-  [MESA_FORMAT_Z_UNORM32] = 0,
   [MESA_FORMAT_S_UINT8] = ISL_FORMAT_R8_UINT,
 
-  [MESA_FORMAT_BGR_SRGB8] = 0,
-  [MESA_FORMAT_A8B8G8R8_SRGB] = 0,
   [MESA_FORMAT_B8G8R8A8_SRGB] = ISL_FORMAT_B8G8R8A8_UNORM_SRGB,
-  [MESA_FORMAT_A8R8G8B8_SRGB] = 0,
   [MESA_FORMAT_R8G8B8A8_SRGB] = ISL_FORMAT_R8G8B8A8_UNORM_SRGB,
-  [MESA_FORMAT_X8R8G8B8_SRGB] = 0,
   [MESA_FORMAT_B8G8R8X8_SRGB] = ISL_FORMAT_B8G8R8X8_UNORM_SRGB,
   [MESA_FORMAT_L_SRGB8] = ISL_FORMAT_L8_UNORM_SRGB,
   [MESA_FORMAT_L8A8_SRGB] = ISL_FORMAT_L8A8_UNORM_SRGB,
-  [MESA_FORMAT_A8L8_SRGB] = 0,
   [MESA_FORMAT_SRGB_DXT1] = ISL_FORMAT_BC1_UNORM_SRGB,
   [MESA_FORMAT_SRGBA_DXT1] = ISL_FORMAT_BC1_UNORM_SRGB,
   [MESA_FORMAT_SRGBA_DXT3] = ISL_FORMAT_BC2_UNORM_SRGB,
@@ -109,7 +84,6 @@ brw_isl_format_for_mesa_format(mesa_format mesa_format)
   [MESA_FORMAT_RGBA_FLOAT32] = ISL_FORMAT_R32G32B32A32_FLOAT,
   [MESA_FORMAT_RGBA_FLOAT16] = ISL_FORMAT_R16G16

Re: [Mesa-dev] [PATCH 4/6] i965: Replace 0 with ISL_FORMAT_UNSUPPORTED in format table

[Chad Versace]

On Wed 31 May 2017, Jason Ekstrand wrote:
> On May 31, 2017 9:32:23 PM Ian Romanick  wrote:
> 
> > Having the unsupported format value not be zero isn't very safe.  The
> > C99 rules say that any field missing an initializer is implicitly
> > initialized to zero.  If a MESA_FORMAT_ value is added but is not added
> > to the array initializer, we'll have this same problem... but in a way
> > that is much harder to detect.
> 
> Good point.  Chad, how would you feel about leaving things zero and then
> having a loop that walks the array, knows about the one special case, and
> sets all unsupported things to ISL_FORMAT_UNSUPPORTED?  Then we get the best
> of both worlds.

My patch is garbage. We all agree.

I prefer Matt's approach that preserves the array's static constness. If
that doesn't work, then I'll write a loop. I'll reply back after testing
it.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH V2] i965: Rename brw_format variable names to isl_format

[Chad Versace]

On Thu 01 Jun 2017, Jason Ekstrand wrote:
> On Thu, Jun 1, 2017 at 9:41 AM, Anuj Phogat <anuj.pho...@gmail.com> wrote:
> 
> > On Tue, May 23, 2017 at 2:35 PM, Anuj Phogat <anuj.pho...@gmail.com>
> > wrote:
> > > This patch makes non functional changes. Renaming is just to
> > > make the code more readable.
> > >
> > > V2: update the types to "enum isl_format"
> > Jason, do you have any other questions ? r-b ?
> >
> 
> Patch 1/6 of the series Chad sent out yesterday also does this and a bit
> more.  I don't think he saw yours.

Right. I didn't see Anuj's patch.

Anuj, your patch looks good to me, and it's
Reviewed-by: Chad Versace <chadvers...@chromium.org>

If your patch lands before mine, it's no trouble to rebase my patches.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH 6/6] i965/dri: Fix bad GL error in intel_create_winsys_renderbuffer()

[Chad Versace]

On Wed 31 May 2017, Ian Romanick wrote:
> On 05/31/2017 04:43 PM, Chad Versace wrote:
> > This function never occurs in the callchain of a GL function. It occurs
> > only in the callchain of eglCreate*Surface and the analogous paths for
> > GLX.  Therefore, even if a  thread does have a bound GL context,
> > emitting a GL error here is wrong. A misplaced GL error, when no GL
> > call is made, can confuse clients.
> 
> This seems right.  What seems wrong, however, is that the callers ignore
> the potentially NULL return.  intelCreateBuffer (intel_screen.c) could
> easily return false when it gets NULL, but it merrily plugs along.

Yes, that seems to be a problem throughout the DRI layer. iirc, some DRI
vfunc signatures even lack return codes. I'll explore the
intelCreateBuffer callgraph and reply with some follow-up patches.

> This patch clearly makes things better, but I'd love to see a follow up
> that fixes the other pre-existing problems.
> 
> Reviewed-by: Ian Romanick <ian.d.roman...@intel.com>

Thanks.

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

[Mesa-dev] [PATCH] i965: Reject unsupported formats in glEGLImageTargetTexture2D()

[Chad Versace]

If the EGLImage's format is not a supported texture format according to
brw_surface_formats.c, then refuse to create the miptree. This follows
the precedent in glEGLImageRenderbufferStorage (implemented by
intel_image_target_renderbuffer_storage), which rejects the EGLImage's
format if is not renderable.

Tested on Broadwell with deqp@1ee59ff9 on X11/EGL:

> ./deqp-egl --deqp-case='dEQP-EGL.functional.image.*'
Test run totals:
  Passed:131/169 (77.5%)
  Failed:0/169 (0.0%)
  Not supported: 38/169 (22.5%)
  Warnings:  0/169 (0.0%)
---
 src/mesa/drivers/dri/i965/intel_tex_image.c | 4 
 1 file changed, 4 insertions(+)

Tomasz, you're CC'd because this is related to R8G8B8X8 EGLConfigs on
Skylake.

diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c 
b/src/mesa/drivers/dri/i965/intel_tex_image.c
index 7208d8e..649b390 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_image.c
+++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
@@ -251,9 +251,13 @@ static struct intel_mipmap_tree *
 create_mt_for_dri_image(struct brw_context *brw,
 GLenum target, __DRIimage *image)
 {
+   struct gl_context *ctx = >ctx;
struct intel_mipmap_tree *mt;
uint32_t draw_x, draw_y;
 
+   if (!ctx->TextureFormatSupported[image->format])
+  return NULL;
+
/* Disable creation of the texture's aux buffers because the driver exposes
 * no EGL API to manage them. That is, there is no API for resolving the aux
 * buffer's content to the main buffer nor for invalidating the aux buffer's
-- 
2.9.4

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

[Mesa-dev] [PATCH 0/6] i965: Little fixes & cleanups

[Chad Versace]

I found these little fixes and cleanups while hacking the i965 DRI layer
to support R8G8B8X8 EGLConfigs on Skylake (those patches aren't ready
yet, though).

Patches 1, 2, 4, 5 should change behavior. They're pure cleanups.

Patches 3, 6 are actual fixes.

I tested this series on Broadwell with:
> deqp-egl --deqp-case='dEQP-EGL.functional.image.modify.*'
Test run totals:
  Passed:24/37 (64.9%)
  Failed:0/37 (0.0%)
  Not supported: 13/37 (35.1%)
  Warnings:  0/37 (0.0%)

> deqp-gles3 --deqp-case='dEQP-GLES3.functional.texture.format.*'
Test run totals:
  Passed:530/530 (100.0%)
  Failed:0/530 (0.0%)
  Not supported: 0/530 (0.0%)
  Warnings:  0/530 (0.0%)

This patch series lives on the tag

http://git.kiwitree.net/cgit/~chadv/mesa/tag/?h=chadv/review/2017-05-31/i965-cleanups-v01

Chad Versace (6):
  i965: Fix return type of brw_isl_format_for_mesa_format()
  i965: Fix return type of translate_tex_format()
  i965: Remove bad assert on isl_format
  i965: Replace 0 with ISL_FORMAT_UNSUPPORTED in format table
  i965: Cleanup in intel_create_winsys_renderbuffer()
  i965/dri: Fix bad GL error in intel_create_winsys_renderbuffer()

 src/mesa/drivers/dri/i965/brw_context.c  |   5 +-
 src/mesa/drivers/dri/i965/brw_state.h|   9 +-
 src/mesa/drivers/dri/i965/brw_surface_formats.c  | 184 +++
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  12 +-
 src/mesa/drivers/dri/i965/intel_fbo.c|  13 +-
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c|   4 +-
 6 files changed, 110 insertions(+), 117 deletions(-)

-- 
2.9.3

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

[Mesa-dev] [PATCH 6/6] i965/dri: Fix bad GL error in intel_create_winsys_renderbuffer()

[Chad Versace]

This function never occurs in the callchain of a GL function. It occurs
only in the callchain of eglCreate*Surface and the analogous paths for
GLX.  Therefore, even if a  thread does have a bound GL context,
emitting a GL error here is wrong. A misplaced GL error, when no GL
call is made, can confuse clients.

Cc: mesa-sta...@lists.freedesktop.org
---
 src/mesa/drivers/dri/i965/intel_fbo.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c 
b/src/mesa/drivers/dri/i965/intel_fbo.c
index d69b921..a24ddd0 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.c
+++ b/src/mesa/drivers/dri/i965/intel_fbo.c
@@ -440,13 +440,9 @@ intel_nop_alloc_storage(struct gl_context * ctx, struct 
gl_renderbuffer *rb,
 struct intel_renderbuffer *
 intel_create_winsys_renderbuffer(mesa_format format, unsigned num_samples)
 {
-   GET_CURRENT_CONTEXT(ctx);
-
struct intel_renderbuffer *irb = CALLOC_STRUCT(intel_renderbuffer);
-   if (!irb) {
-  _mesa_error(ctx, GL_OUT_OF_MEMORY, "creating renderbuffer");
+   if (!irb)
   return NULL;
-   }
 
struct gl_renderbuffer *rb = >Base.Base;
irb->layer_count = 1;
-- 
2.9.3

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

[Mesa-dev] [PATCH 2/6] i965: Fix return type of translate_tex_format()

[Chad Versace]

It returns an isl_format, not GLuint BRW_FORMAT.  I updated every
translate_tex_format() found by git-grep.

No change in behavior.
---
 src/mesa/drivers/dri/i965/brw_state.h| 6 +++---
 src/mesa/drivers/dri/i965/brw_surface_formats.c  | 2 +-
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_state.h 
b/src/mesa/drivers/dri/i965/brw_state.h
index cb341f0..cbfb8e7 100644
--- a/src/mesa/drivers/dri/i965/brw_state.h
+++ b/src/mesa/drivers/dri/i965/brw_state.h
@@ -209,9 +209,9 @@ enum isl_format brw_isl_format_for_mesa_format(mesa_format 
mesa_format);
 
 GLuint translate_tex_target(GLenum target);
 
-GLuint translate_tex_format(struct brw_context *brw,
-mesa_format mesa_format,
-GLenum srgb_decode);
+enum isl_format translate_tex_format(struct brw_context *brw,
+ mesa_format mesa_format,
+ GLenum srgb_decode);
 
 int brw_get_texture_swizzle(const struct gl_context *ctx,
 const struct gl_texture_object *t);
diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c 
b/src/mesa/drivers/dri/i965/brw_surface_formats.c
index f482f22..155146c 100644
--- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
+++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
@@ -513,7 +513,7 @@ brw_render_target_supported(struct brw_context *brw,
return brw->format_supported_as_render_target[format];
 }
 
-GLuint
+enum isl_format
 translate_tex_format(struct brw_context *brw,
  mesa_format mesa_format,
 GLenum srgb_decode)
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 f041d9a..de6fd8d 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
@@ -557,8 +557,8 @@ brw_update_texture_surface(struct gl_context *ctx,
 brw_get_texture_swizzle(>ctx, obj));
 
   mesa_format mesa_fmt = plane == 0 ? intel_obj->_Format : mt->format;
-  unsigned format = translate_tex_format(brw, mesa_fmt,
- sampler->sRGBDecode);
+  enum isl_format format = translate_tex_format(brw, mesa_fmt,
+sampler->sRGBDecode);
 
   /* Implement gen6 and gen7 gather work-around */
   bool need_green_to_blue = false;
-- 
2.9.3

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

[Mesa-dev] [PATCH 4/6] i965: Replace 0 with ISL_FORMAT_UNSUPPORTED in format table

[Chad Versace]

When given an *unsupported* mesa_format,
brw_isl_format_for_mesa_format() returned 0, a *valid* isl_format,
ISL_FORMAT_R32G32B32A32_FLOAT.  The problem is that
brw_isl_format_for_mesa_format's inner table used 0 instead of
ISL_FORMAT_UNSUPPORTED to indicate unsupported mesa formats.

Some callers of brw_isl_format_for_mesa_format() were aware of this
weirdness, and worked around it. This patch removes those workarounds.
---
 src/mesa/drivers/dri/i965/brw_surface_formats.c  | 168 +++
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c |   2 +-
 2 files changed, 84 insertions(+), 86 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c 
b/src/mesa/drivers/dri/i965/brw_surface_formats.c
index 52d3acb..8fd9ae5 100644
--- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
+++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
@@ -38,29 +38,29 @@ brw_isl_format_for_mesa_format(mesa_format mesa_format)
 */
static const uint32_t table[MESA_FORMAT_COUNT] =
{
-  [MESA_FORMAT_A8B8G8R8_UNORM] = 0,
+  [MESA_FORMAT_A8B8G8R8_UNORM] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_R8G8B8A8_UNORM] = ISL_FORMAT_R8G8B8A8_UNORM,
   [MESA_FORMAT_B8G8R8A8_UNORM] = ISL_FORMAT_B8G8R8A8_UNORM,
-  [MESA_FORMAT_A8R8G8B8_UNORM] = 0,
-  [MESA_FORMAT_X8B8G8R8_UNORM] = 0,
+  [MESA_FORMAT_A8R8G8B8_UNORM] = ISL_FORMAT_UNSUPPORTED,
+  [MESA_FORMAT_X8B8G8R8_UNORM] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_R8G8B8X8_UNORM] = ISL_FORMAT_R8G8B8X8_UNORM,
   [MESA_FORMAT_B8G8R8X8_UNORM] = ISL_FORMAT_B8G8R8X8_UNORM,
-  [MESA_FORMAT_X8R8G8B8_UNORM] = 0,
-  [MESA_FORMAT_BGR_UNORM8] = 0,
+  [MESA_FORMAT_X8R8G8B8_UNORM] = ISL_FORMAT_UNSUPPORTED,
+  [MESA_FORMAT_BGR_UNORM8] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_RGB_UNORM8] = ISL_FORMAT_R8G8B8_UNORM,
   [MESA_FORMAT_B5G6R5_UNORM] = ISL_FORMAT_B5G6R5_UNORM,
-  [MESA_FORMAT_R5G6B5_UNORM] = 0,
+  [MESA_FORMAT_R5G6B5_UNORM] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_B4G4R4A4_UNORM] = ISL_FORMAT_B4G4R4A4_UNORM,
-  [MESA_FORMAT_A4R4G4B4_UNORM] = 0,
-  [MESA_FORMAT_A1B5G5R5_UNORM] = 0,
+  [MESA_FORMAT_A4R4G4B4_UNORM] = ISL_FORMAT_UNSUPPORTED,
+  [MESA_FORMAT_A1B5G5R5_UNORM] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_B5G5R5A1_UNORM] = ISL_FORMAT_B5G5R5A1_UNORM,
-  [MESA_FORMAT_A1R5G5B5_UNORM] = 0,
-  [MESA_FORMAT_L4A4_UNORM] = 0,
+  [MESA_FORMAT_A1R5G5B5_UNORM] = ISL_FORMAT_UNSUPPORTED,
+  [MESA_FORMAT_L4A4_UNORM] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_L8A8_UNORM] = ISL_FORMAT_L8A8_UNORM,
-  [MESA_FORMAT_A8L8_UNORM] = 0,
+  [MESA_FORMAT_A8L8_UNORM] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_L16A16_UNORM] = ISL_FORMAT_L16A16_UNORM,
-  [MESA_FORMAT_A16L16_UNORM] = 0,
-  [MESA_FORMAT_B2G3R3_UNORM] = 0,
+  [MESA_FORMAT_A16L16_UNORM] = ISL_FORMAT_UNSUPPORTED,
+  [MESA_FORMAT_B2G3R3_UNORM] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_A_UNORM8] = ISL_FORMAT_A8_UNORM,
   [MESA_FORMAT_A_UNORM16] = ISL_FORMAT_A16_UNORM,
   [MESA_FORMAT_L_UNORM8] = ISL_FORMAT_L8_UNORM,
@@ -71,29 +71,29 @@ brw_isl_format_for_mesa_format(mesa_format mesa_format)
   [MESA_FORMAT_YCBCR] = ISL_FORMAT_YCRCB_SWAPUVY,
   [MESA_FORMAT_R_UNORM8] = ISL_FORMAT_R8_UNORM,
   [MESA_FORMAT_R8G8_UNORM] = ISL_FORMAT_R8G8_UNORM,
-  [MESA_FORMAT_G8R8_UNORM] = 0,
+  [MESA_FORMAT_G8R8_UNORM] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_R_UNORM16] = ISL_FORMAT_R16_UNORM,
   [MESA_FORMAT_R16G16_UNORM] = ISL_FORMAT_R16G16_UNORM,
-  [MESA_FORMAT_G16R16_UNORM] = 0,
+  [MESA_FORMAT_G16R16_UNORM] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_B10G10R10A2_UNORM] = ISL_FORMAT_B10G10R10A2_UNORM,
-  [MESA_FORMAT_S8_UINT_Z24_UNORM] = 0,
-  [MESA_FORMAT_Z24_UNORM_S8_UINT] = 0,
-  [MESA_FORMAT_Z_UNORM16] = 0,
-  [MESA_FORMAT_Z24_UNORM_X8_UINT] = 0,
-  [MESA_FORMAT_X8_UINT_Z24_UNORM] = 0,
-  [MESA_FORMAT_Z_UNORM32] = 0,
+  [MESA_FORMAT_S8_UINT_Z24_UNORM] = ISL_FORMAT_UNSUPPORTED,
+  [MESA_FORMAT_Z24_UNORM_S8_UINT] = ISL_FORMAT_UNSUPPORTED,
+  [MESA_FORMAT_Z_UNORM16] = ISL_FORMAT_UNSUPPORTED,
+  [MESA_FORMAT_Z24_UNORM_X8_UINT] = ISL_FORMAT_UNSUPPORTED,
+  [MESA_FORMAT_X8_UINT_Z24_UNORM] = ISL_FORMAT_UNSUPPORTED,
+  [MESA_FORMAT_Z_UNORM32] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_S_UINT8] = ISL_FORMAT_R8_UINT,
 
-  [MESA_FORMAT_BGR_SRGB8] = 0,
-  [MESA_FORMAT_A8B8G8R8_SRGB] = 0,
+  [MESA_FORMAT_BGR_SRGB8] = ISL_FORMAT_UNSUPPORTED,
+  [MESA_FORMAT_A8B8G8R8_SRGB] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_B8G8R8A8_SRGB] = ISL_FORMAT_B8G8R8A8_UNORM_SRGB,
-  [MESA_FORMAT_A8R8G8B8_SRGB] = 0,
+  [MESA_FORMAT_A8R8G8B8_SRGB] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_R8G8B8A8_SRGB] = ISL_FORMAT_R8G8B8A8_UNORM_SRGB,
-  [MESA_FORMAT_X8R8G8B8_SRGB] = 0,
+  [MESA_FORMAT_X8R8G8B8_SRGB] = ISL_FORMAT_UNSUPPORTED,
   [MESA_FORMAT_B8G8R8X8_SRGB] = 

[Mesa-dev] [PATCH 3/6] i965: Remove bad assert on isl_format

[Chad Versace]

translate_tex_format() asserted that isl_format != 0. But 0 is a valid
format, ISL_FORMAT_R32G32B32A32_FLOAT.
---
 src/mesa/drivers/dri/i965/brw_surface_formats.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c 
b/src/mesa/drivers/dri/i965/brw_surface_formats.c
index 155146c..52d3acb 100644
--- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
+++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
@@ -575,7 +575,6 @@ translate_tex_format(struct brw_context *brw,
}
 
default:
-  assert(brw_isl_format_for_mesa_format(mesa_format) != 0);
   return brw_isl_format_for_mesa_format(mesa_format);
}
 }
-- 
2.9.3

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

[Mesa-dev] [PATCH 5/6] i965: Cleanup in intel_create_winsys_renderbuffer()

[Chad Versace]

Combine variable declarations and assignments.
Trivial cleanup.
---
 src/mesa/drivers/dri/i965/intel_fbo.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c 
b/src/mesa/drivers/dri/i965/intel_fbo.c
index d3905a5..d69b921 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.c
+++ b/src/mesa/drivers/dri/i965/intel_fbo.c
@@ -440,18 +440,15 @@ intel_nop_alloc_storage(struct gl_context * ctx, struct 
gl_renderbuffer *rb,
 struct intel_renderbuffer *
 intel_create_winsys_renderbuffer(mesa_format format, unsigned num_samples)
 {
-   struct intel_renderbuffer *irb;
-   struct gl_renderbuffer *rb;
-
GET_CURRENT_CONTEXT(ctx);
 
-   irb = CALLOC_STRUCT(intel_renderbuffer);
+   struct intel_renderbuffer *irb = CALLOC_STRUCT(intel_renderbuffer);
if (!irb) {
   _mesa_error(ctx, GL_OUT_OF_MEMORY, "creating renderbuffer");
   return NULL;
}
 
-   rb = >Base.Base;
+   struct gl_renderbuffer *rb = >Base.Base;
irb->layer_count = 1;
 
_mesa_init_renderbuffer(rb, 0);
-- 
2.9.3

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

[Mesa-dev] [PATCH 1/6] i965: Fix return type of brw_isl_format_for_mesa_format()

[Chad Versace]

It returns an isl_format, not uint32_t BRW_FORMAT.
I updated every brw_isl_format_for_mesa_format() found by git-grep.

No change in behavior.
---
 src/mesa/drivers/dri/i965/brw_context.c  |  5 +++--
 src/mesa/drivers/dri/i965/brw_state.h|  3 +--
 src/mesa/drivers/dri/i965/brw_surface_formats.c  | 13 -
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  8 
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c|  4 ++--
 5 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index 3e3fe2d..f2a7f61 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -205,9 +205,10 @@ intel_texture_view_requires_resolve(struct brw_context 
*brw,
!intel_miptree_is_lossless_compressed(brw, intel_tex->mt))
  return false;
 
-   const uint32_t brw_format = 
brw_isl_format_for_mesa_format(intel_tex->_Format);
+   const enum isl_format format =
+  brw_isl_format_for_mesa_format(intel_tex->_Format);
 
-   if (isl_format_supports_ccs_e(>screen->devinfo, brw_format))
+   if (isl_format_supports_ccs_e(>screen->devinfo, format))
   return false;
 
perf_debug("Incompatible sampling format (%s) for rbc (%s)\n",
diff --git a/src/mesa/drivers/dri/i965/brw_state.h 
b/src/mesa/drivers/dri/i965/brw_state.h
index bec7b03..cb341f0 100644
--- a/src/mesa/drivers/dri/i965/brw_state.h
+++ b/src/mesa/drivers/dri/i965/brw_state.h
@@ -205,8 +205,7 @@ uint32_t brw_state_batch_size(struct brw_context *brw, 
uint32_t offset);
 void gen4_init_vtable_surface_functions(struct brw_context *brw);
 uint32_t brw_get_surface_tiling_bits(uint32_t tiling);
 uint32_t brw_get_surface_num_multisamples(unsigned num_samples);
-
-uint32_t brw_isl_format_for_mesa_format(mesa_format mesa_format);
+enum isl_format brw_isl_format_for_mesa_format(mesa_format mesa_format);
 
 GLuint translate_tex_target(GLenum target);
 
diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c 
b/src/mesa/drivers/dri/i965/brw_surface_formats.c
index b176a21..f482f22 100644
--- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
+++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
@@ -28,7 +28,7 @@
 #include "brw_state.h"
 #include "brw_defines.h"
 
-uint32_t
+enum isl_format
 brw_isl_format_for_mesa_format(mesa_format mesa_format)
 {
/* This table is ordered according to the enum ordering in formats.h.  We do
@@ -300,7 +300,7 @@ brw_init_surface_formats(struct brw_context *brw)
   gen += 5;
 
for (format = MESA_FORMAT_NONE + 1; format < MESA_FORMAT_COUNT; format++) {
-  uint32_t texture, render;
+  enum isl_format texture, render;
   bool is_integer = _mesa_is_format_integer_color(format);
 
   render = texture = brw_isl_format_for_mesa_format(format);
@@ -376,6 +376,8 @@ brw_init_surface_formats(struct brw_context *brw)
   case ISL_FORMAT_R8G8B8X8_UNORM_SRGB:
  render = ISL_FORMAT_R8G8B8A8_UNORM_SRGB;
  break;
+  default:
+ break;
   }
 
   /* Note that GL_EXT_texture_integer says that blending doesn't occur for
@@ -555,7 +557,8 @@ translate_tex_format(struct brw_context *brw,
case MESA_FORMAT_RGBA_ASTC_10x10:
case MESA_FORMAT_RGBA_ASTC_12x10:
case MESA_FORMAT_RGBA_ASTC_12x12: {
-  GLuint brw_fmt = brw_isl_format_for_mesa_format(mesa_format);
+  enum isl_format isl_fmt =
+ brw_isl_format_for_mesa_format(mesa_format);
 
   /**
* It is possible to process these formats using the LDR Profile
@@ -566,9 +569,9 @@ translate_tex_format(struct brw_context *brw,
* processing sRGBs, which are incompatible with this mode.
*/
   if (ctx->Extensions.KHR_texture_compression_astc_hdr)
- brw_fmt |= GEN9_SURFACE_ASTC_HDR_FORMAT_BIT;
+ isl_fmt |= GEN9_SURFACE_ASTC_HDR_FORMAT_BIT;
 
-  return brw_fmt;
+  return isl_fmt;
}
 
default:
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 e019adc..f041d9a 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
@@ -686,7 +686,7 @@ brw_update_buffer_texture_surface(struct gl_context *ctx,
uint32_t size = tObj->BufferSize;
struct brw_bo *bo = NULL;
mesa_format format = tObj->_BufferObjectFormat;
-   uint32_t brw_format = brw_isl_format_for_mesa_format(format);
+   enum isl_format isl_format = brw_isl_format_for_mesa_format(format);
int texel_size = _mesa_get_format_bytes(format);
 
if (intel_obj) {
@@ -712,14 +712,14 @@ brw_update_buffer_texture_surface(struct gl_context *ctx,
 */
size = MIN2(size, ctx->Const.MaxTextureBufferSize * (unsigned) texel_size);
 
-   if (brw_format == 0 && format != MESA_FORMAT_RGBA_FLOAT32) {
+   if (isl_format == 0 && format != MESA_FORMAT_RGBA_FLOAT32) {
   _mesa_problem(NULL, "bad format %s for texture buffer\n",
  

[Mesa-dev] [PATCH 1/9] i965/dri: Rewrite comment for intelCreateBuffer

[Chad Versace]

The old comment pinned this function to X11 windows. In reality, this
function serves more than X11 and more than just windows.
---
 src/mesa/drivers/dri/i965/intel_screen.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index 9842de6742..5de6f18b84 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -1152,7 +1152,11 @@ intelDestroyScreen(__DRIscreen * sPriv)
 
 
 /**
- * This is called when we need to set up GL rendering to a new X window.
+ * Create a gl_framebuffer and attach it to __DRIdrawable::driverPrivate.
+ *
+ *_This implements driDriverAPI::createNewDrawable, which the DRI layer calls
+ * when creating a EGLSurface, GLXDrawable, or GLXPixmap. Despite the name,
+ * this does not allocate GPU memory.
  */
 static GLboolean
 intelCreateBuffer(__DRIscreen *dri_screen,
-- 
2.13.0

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

[Mesa-dev] [PATCH 6/9] i965: Add whitespace in intel_update_image_buffers()

[Chad Versace]

Improve readability.  Add an empty line between two large 'if' blocks.
---
 src/mesa/drivers/dri/i965/brw_context.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index 825912b7b5..3e3fe2d0ed 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -1779,6 +1779,7 @@ intel_update_image_buffers(struct brw_context *brw, 
__DRIdrawable *drawable)
 images.front,
 __DRI_IMAGE_BUFFER_FRONT);
}
+
if (images.image_mask & __DRI_IMAGE_BUFFER_BACK) {
   drawable->w = images.back->width;
   drawable->h = images.back->height;
-- 
2.13.0

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

[Mesa-dev] [PATCH 9/9] i965: Fix type of brw_context::render_target_format[]

[Chad Versace]

It's an array of isl_format, not uint32_t. This patch updates every
reference to render_target_format[] git-grep.

Trivial cleanup. No change in behavior.
---
 src/mesa/drivers/dri/i965/brw_blorp.c| 4 ++--
 src/mesa/drivers/dri/i965/brw_context.h  | 2 +-
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c 
b/src/mesa/drivers/dri/i965/brw_blorp.c
index 7ffe8b885a..9030fe7e76 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp.c
+++ b/src/mesa/drivers/dri/i965/brw_blorp.c
@@ -920,7 +920,7 @@ do_single_blorp_clear(struct brw_context *brw, struct 
gl_framebuffer *fb,
   struct blorp_batch batch;
   blorp_batch_init(>blorp, , brw, 0);
   blorp_fast_clear(, ,
-   (enum isl_format)brw->render_target_format[format],
+   brw->render_target_format[format],
level, logical_layer, num_layers,
x0, y0, x1, y1);
   blorp_batch_finish();
@@ -946,7 +946,7 @@ do_single_blorp_clear(struct brw_context *brw, struct 
gl_framebuffer *fb,
   struct blorp_batch batch;
   blorp_batch_init(>blorp, , brw, 0);
   blorp_clear(, ,
-  (enum isl_format)brw->render_target_format[format],
+  brw->render_target_format[format],
   ISL_SWIZZLE_IDENTITY,
   level, irb_logical_mt_layer(irb), num_layers,
   x0, y0, x1, y1,
diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
b/src/mesa/drivers/dri/i965/brw_context.h
index f45fc2479c..c15abe1d48 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -1161,7 +1161,7 @@ struct brw_context
const struct brw_tracked_state render_atoms[76];
const struct brw_tracked_state compute_atoms[11];
 
-   uint32_t render_target_format[MESA_FORMAT_COUNT];
+   enum isl_format render_target_format[MESA_FORMAT_COUNT];
bool format_supported_as_render_target[MESA_FORMAT_COUNT];
 
/* PrimitiveRestart */
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 a0fed6096d..e019adcf2d 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
@@ -986,7 +986,7 @@ gen4_update_renderbuffer_surface(struct brw_context *brw,
struct intel_mipmap_tree *mt = irb->mt;
uint32_t *surf;
uint32_t tile_x, tile_y;
-   uint32_t format = 0;
+   enum isl_format format;
uint32_t offset;
/* _NEW_BUFFERS */
mesa_format rb_format = _mesa_get_render_format(ctx, intel_rb_format(irb));
@@ -1172,7 +1172,7 @@ update_renderbuffer_read_surfaces(struct brw_context *brw)
  uint32_t *surf_offset = >wm.base.surf_offset[surf_index];
 
  if (irb) {
-const unsigned format = brw->render_target_format[
+const enum isl_format format = brw->render_target_format[
_mesa_get_render_format(ctx, intel_rb_format(irb))];
 assert(isl_format_supports_sampling(>screen->devinfo,
 format));
-- 
2.13.0

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

[Mesa-dev] [PATCH 0/9] i965: A few tiny trivial cleanups

[Chad Versace]

I was hacking on the i965/DRI glue, and on brw_surface_formats.c, and
I found several little cleanups to make the code nicer. All these
patches are super small.

Chad Versace (9):
  i965/dri: Rewrite comment for intelCreateBuffer
  i965/dri: Combine declaration and assignment in intelCreateBuffer
  i965: Rename intel_create_renderbuffer
  i965: Fix type of intel_update_image_buffers::format
  i965: Move an 'i' declaration into its 'for' loop
  i965: Add whitespace in intel_update_image_buffers()
  i965: Document type of GLuint __DRIimage::format
  i965: Move func to right comment block in brw_context.h
  i965: Fix type of brw_context::render_target_format[]

 src/mesa/drivers/dri/i965/brw_blorp.c|  4 ++--
 src/mesa/drivers/dri/i965/brw_context.c  |  7 ---
 src/mesa/drivers/dri/i965/brw_context.h  |  4 ++--
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  4 ++--
 src/mesa/drivers/dri/i965/intel_fbo.c| 11 ++-
 src/mesa/drivers/dri/i965/intel_fbo.h|  2 +-
 src/mesa/drivers/dri/i965/intel_image.h  |  2 +-
 src/mesa/drivers/dri/i965/intel_screen.c | 13 -
 8 files changed, 26 insertions(+), 21 deletions(-)

-- 
2.13.0

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

[Mesa-dev] [PATCH 3/9] i965: Rename intel_create_renderbuffer

[Chad Versace]

The name is misleading because the function is unrelated to GL
renderbuffers. Rename it to intel_create_winsys_renderbuffer.
---
 src/mesa/drivers/dri/i965/intel_fbo.c| 11 ++-
 src/mesa/drivers/dri/i965/intel_fbo.h|  2 +-
 src/mesa/drivers/dri/i965/intel_screen.c |  4 ++--
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c 
b/src/mesa/drivers/dri/i965/intel_fbo.c
index 60a6d0f455..d3905a5f7f 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.c
+++ b/src/mesa/drivers/dri/i965/intel_fbo.c
@@ -431,13 +431,14 @@ intel_nop_alloc_storage(struct gl_context * ctx, struct 
gl_renderbuffer *rb,
 }
 
 /**
- * Create a new intel_renderbuffer which corresponds to an on-screen window,
- * not a user-created renderbuffer.
+ * Create an intel_renderbuffer for a __DRIdrawable. This function is
+ * unrelated to GL renderbuffers (that is, those created by
+ * glGenRenderbuffers).
  *
  * \param num_samples must be quantized.
  */
 struct intel_renderbuffer *
-intel_create_renderbuffer(mesa_format format, unsigned num_samples)
+intel_create_winsys_renderbuffer(mesa_format format, unsigned num_samples)
 {
struct intel_renderbuffer *irb;
struct gl_renderbuffer *rb;
@@ -469,7 +470,7 @@ intel_create_renderbuffer(mesa_format format, unsigned 
num_samples)
 
 /**
  * Private window-system buffers (as opposed to ones shared with the display
- * server created with intel_create_renderbuffer()) are most similar in their
+ * server created with intel_create_winsys_renderbuffer()) are most similar in 
their
  * handling to user-created renderbuffers, but they have a resize handler that
  * may be called at intel_update_renderbuffers() time.
  *
@@ -480,7 +481,7 @@ intel_create_private_renderbuffer(mesa_format format, 
unsigned num_samples)
 {
struct intel_renderbuffer *irb;
 
-   irb = intel_create_renderbuffer(format, num_samples);
+   irb = intel_create_winsys_renderbuffer(format, num_samples);
irb->Base.Base.AllocStorage = intel_alloc_private_renderbuffer_storage;
 
return irb;
diff --git a/src/mesa/drivers/dri/i965/intel_fbo.h 
b/src/mesa/drivers/dri/i965/intel_fbo.h
index 08b82e8934..2d2ef1ebc6 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.h
+++ b/src/mesa/drivers/dri/i965/intel_fbo.h
@@ -167,7 +167,7 @@ intel_rb_format(const struct intel_renderbuffer *rb)
 }
 
 extern struct intel_renderbuffer *
-intel_create_renderbuffer(mesa_format format, unsigned num_samples);
+intel_create_winsys_renderbuffer(mesa_format format, unsigned num_samples);
 
 struct intel_renderbuffer *
 intel_create_private_renderbuffer(mesa_format format, unsigned num_samples);
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index 24123e7986..22f6d9af03 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -1200,11 +1200,11 @@ intelCreateBuffer(__DRIscreen *dri_screen,
}
 
/* setup the hardware-based renderbuffers */
-   rb = intel_create_renderbuffer(rgbFormat, num_samples);
+   rb = intel_create_winsys_renderbuffer(rgbFormat, num_samples);
_mesa_attach_and_own_rb(fb, BUFFER_FRONT_LEFT, >Base.Base);
 
if (mesaVis->doubleBufferMode) {
-  rb = intel_create_renderbuffer(rgbFormat, num_samples);
+  rb = intel_create_winsys_renderbuffer(rgbFormat, num_samples);
   _mesa_attach_and_own_rb(fb, BUFFER_BACK_LEFT, >Base.Base);
}
 
-- 
2.13.0

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

[Mesa-dev] [PATCH 7/9] i965: Document type of GLuint __DRIimage::format

[Chad Versace]

It's either a mesa_format or mesa_array_format.
---
 src/mesa/drivers/dri/i965/intel_image.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/intel_image.h 
b/src/mesa/drivers/dri/i965/intel_image.h
index ad426910e4..cf0610540f 100644
--- a/src/mesa/drivers/dri/i965/intel_image.h
+++ b/src/mesa/drivers/dri/i965/intel_image.h
@@ -70,7 +70,7 @@ struct __DRIimageRec {
uint32_t pitch; /**< in bytes */
GLenum internal_format;
uint32_t dri_format;
-   GLuint format;
+   GLuint format; /**< mesa_format or mesa_array_format */
uint64_t modifier; /**< fb modifier (fourcc) */
uint32_t offset;
 
-- 
2.13.0

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

[Mesa-dev] [PATCH 2/9] i965/dri: Combine declaration and assignment in intelCreateBuffer

[Chad Versace]

Trivial cleanup.
---
 src/mesa/drivers/dri/i965/intel_screen.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index 5de6f18b84..24123e7986 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -1169,12 +1169,11 @@ intelCreateBuffer(__DRIscreen *dri_screen,
mesa_format rgbFormat;
unsigned num_samples =
   intel_quantize_num_samples(screen, mesaVis->samples);
-   struct gl_framebuffer *fb;
 
if (isPixmap)
   return false;
 
-   fb = CALLOC_STRUCT(gl_framebuffer);
+   struct gl_framebuffer *fb = CALLOC_STRUCT(gl_framebuffer);
if (!fb)
   return false;
 
-- 
2.13.0

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

[Mesa-dev] [PATCH 8/9] i965: Move func to right comment block in brw_context.h

[Chad Versace]

brw_init_surface_formats() is defined in brw_surface_formats.c, not
brw_wm_surface_state.c.
---
 src/mesa/drivers/dri/i965/brw_context.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
b/src/mesa/drivers/dri/i965/brw_context.h
index ccedeccf30..f45fc2479c 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -1388,7 +1388,6 @@ brw_get_index_type(unsigned index_size)
 void brw_prepare_vertices(struct brw_context *brw);
 
 /* brw_wm_surface_state.c */
-void brw_init_surface_formats(struct brw_context *brw);
 void brw_create_constant_surface(struct brw_context *brw,
  struct brw_bo *bo,
  uint32_t offset,
@@ -1420,6 +1419,7 @@ void brw_upload_image_surfaces(struct brw_context *brw,
struct brw_stage_prog_data *prog_data);
 
 /* brw_surface_formats.c */
+void brw_init_surface_formats(struct brw_context *brw);
 bool brw_render_target_supported(struct brw_context *brw,
  struct gl_renderbuffer *rb);
 uint32_t brw_depth_format(struct brw_context *brw, mesa_format format);
-- 
2.13.0

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

[Mesa-dev] [PATCH 4/9] i965: Fix type of intel_update_image_buffers::format

[Chad Versace]

It's a mesa_format, not an unsigned int.
---
 src/mesa/drivers/dri/i965/brw_context.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index c815a0454d..adae921e57 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -1739,7 +1739,7 @@ intel_update_image_buffers(struct brw_context *brw, 
__DRIdrawable *drawable)
struct intel_renderbuffer *front_rb;
struct intel_renderbuffer *back_rb;
struct __DRIimageList images;
-   unsigned int format;
+   mesa_format format;
uint32_t buffer_mask = 0;
int ret;
 
-- 
2.13.0

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

[Mesa-dev] [PATCH 5/9] i965: Move an 'i' declaration into its 'for' loop

[Chad Versace]

In intel_update_dri2_buffers().
Trivial cleanup.
---
 src/mesa/drivers/dri/i965/brw_context.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index adae921e57..825912b7b5 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -1410,7 +1410,7 @@ intel_update_dri2_buffers(struct brw_context *brw, 
__DRIdrawable *drawable)
struct gl_framebuffer *fb = drawable->driverPrivate;
struct intel_renderbuffer *rb;
__DRIbuffer *buffers = NULL;
-   int i, count;
+   int count;
const char *region_name;
 
/* Set this up front, so that in case our buffers get invalidated
@@ -1426,7 +1426,7 @@ intel_update_dri2_buffers(struct brw_context *brw, 
__DRIdrawable *drawable)
if (buffers == NULL)
   return;
 
-   for (i = 0; i < count; i++) {
+   for (int i = 0; i < count; i++) {
switch (buffers[i].attachment) {
case __DRI_BUFFER_FRONT_LEFT:
rb = intel_get_renderbuffer(fb, BUFFER_FRONT_LEFT);
-- 
2.13.0

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

[Mesa-dev] [PATCH 0/2] egl/android: A few trivial cleanups

[Chad Versace]

Chad Versace (2):
  egl/android: Align channel masks in HAL_PIXEL_FORMAT table
  egl/android: Drop unused 'format' param in get_back_bo()

 src/egl/drivers/dri2/platform_android.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.13.0

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

[Mesa-dev] [PATCH 2/2] egl/android: Drop unused 'format' param in get_back_bo()

[Chad Versace]

---
 src/egl/drivers/dri2/platform_android.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index e76e1678a8..4fed751eae 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -489,7 +489,7 @@ get_front_bo(struct dri2_egl_surface *dri2_surf, unsigned 
int format)
 }
 
 static int
-get_back_bo(struct dri2_egl_surface *dri2_surf, unsigned int format)
+get_back_bo(struct dri2_egl_surface *dri2_surf)
 {
struct dri2_egl_display *dri2_dpy =
   dri2_egl_display(dri2_surf->base.Resource.Display);
@@ -589,7 +589,7 @@ droid_image_get_buffers(__DRIdrawable *driDrawable,
}
 
if (buffer_mask & __DRI_IMAGE_BUFFER_BACK) {
-  if (get_back_bo(dri2_surf, format) < 0)
+  if (get_back_bo(dri2_surf) < 0)
  return 0;
 
   if (dri2_surf->dri_image_back) {
-- 
2.13.0

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

[Mesa-dev] [PATCH 1/2] egl/android: Align channel masks in HAL_PIXEL_FORMAT table

[Chad Versace]

Improves readability. No change in behavior.
---
 src/egl/drivers/dri2/platform_android.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index de24a8f5c4..e76e1678a8 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -990,10 +990,10 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
   int format;
   unsigned int rgba_masks[4];
} visuals[] = {
-  { HAL_PIXEL_FORMAT_RGBA_, { 0xff, 0xff00, 0xff, 0xff00 } },
-  { HAL_PIXEL_FORMAT_RGBX_, { 0xff, 0xff00, 0xff, 0x0 } },
-  { HAL_PIXEL_FORMAT_RGB_565,   { 0xf800, 0x7e0, 0x1f, 0x0 } },
-  { HAL_PIXEL_FORMAT_BGRA_, { 0xff, 0xff00, 0xff, 0xff00 } },
+  { HAL_PIXEL_FORMAT_RGBA_, { 0x00ff, 0xff00, 0x00ff, 
0xff00 } },
+  { HAL_PIXEL_FORMAT_RGBX_, { 0x00ff, 0xff00, 0x00ff, 
0x } },
+  { HAL_PIXEL_FORMAT_RGB_565,   { 0xf800, 0x07e0, 0x001f, 
0x } },
+  { HAL_PIXEL_FORMAT_BGRA_, { 0x00ff, 0xff00, 0x00ff, 
0xff00 } },
};
EGLint config_attrs[] = {
  EGL_NATIVE_VISUAL_ID,   0,
-- 
2.13.0

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

Re: [Mesa-dev] [PATCH] st/dri: Add fence extension to SW path

[Chad Versace]

When you resubmit the patch again, please add (v3) to the subject.

On Mon 08 May 2017, Gurchetan Singh wrote:
> Use the same fence implementation for drisw.c as dri2.c by making
> dri2FenceExtension an external variable. Since the fence implementation
> is not dri2.c specific, put it in a separate file. This is desirable for
> synchronization in virtual machines.
> 
> v2: Don't depend on dri2.c for extensions (Emil)
> ---
>  src/gallium/state_trackers/dri/Makefile.sources |   2 +
>  src/gallium/state_trackers/dri/dri2.c   | 203 +
>  src/gallium/state_trackers/dri/dri_extensions.c | 229 
> 
>  src/gallium/state_trackers/dri/dri_extensions.h |  30 
>  src/gallium/state_trackers/dri/drisw.c  |   2 +
>  5 files changed, 264 insertions(+), 202 deletions(-)
>  create mode 100644 src/gallium/state_trackers/dri/dri_extensions.c
>  create mode 100644 src/gallium/state_trackers/dri/dri_extensions.h


Usually, to make review easier, and to make the git log easier to
understand, it's better to break patches like this into two: patch
1 moves the code, and patch 2 makes the actual changes.

> diff --git a/src/gallium/state_trackers/dri/dri2.c 
> b/src/gallium/state_trackers/dri/dri2.c
> index ed6004f836..2d95e668f8 100644
> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c


> -static bool
> -dri2_is_opencl_interop_loaded_locked(struct dri_screen *screen)
> -{
> -   return screen->opencl_dri_event_add_ref &&
> -  screen->opencl_dri_event_release &&
> -  screen->opencl_dri_event_wait &&
> -  screen->opencl_dri_event_get_fence;
> -}
> -
> -static bool
> -dri2_load_opencl_interop(struct dri_screen *screen)
> -{
> -#if defined(RTLD_DEFAULT)
> -   bool success;

There's a bug here.  Pre-patch, RTLD_DEFAULT is defined here and this
code gets built.  I verified this with #error. Post-patch, in the
equivalent code in the new file, RTLD_DEFAULT is undefined, and this
code never gets built.

> -
> -   mtx_lock(>opencl_func_mutex);
> -
> -   if (dri2_is_opencl_interop_loaded_locked(screen)) {
> -  mtx_unlock(>opencl_func_mutex);
> -  return true;
> -   }
> -
> -   screen->opencl_dri_event_add_ref =
> -  dlsym(RTLD_DEFAULT, "opencl_dri_event_add_ref");
> -   screen->opencl_dri_event_release =
> -  dlsym(RTLD_DEFAULT, "opencl_dri_event_release");
> -   screen->opencl_dri_event_wait =
> -  dlsym(RTLD_DEFAULT, "opencl_dri_event_wait");
> -   screen->opencl_dri_event_get_fence =
> -  dlsym(RTLD_DEFAULT, "opencl_dri_event_get_fence");
> -
> -   success = dri2_is_opencl_interop_loaded_locked(screen);
> -   mtx_unlock(>opencl_func_mutex);
> -   return success;
> -#else
> -   return false;
> -#endif
> -}


> diff --git a/src/gallium/state_trackers/dri/dri_extensions.c 
> b/src/gallium/state_trackers/dri/dri_extensions.c
> new file mode 100644
> index 00..2fa7233aab
> --- /dev/null
> +++ b/src/gallium/state_trackers/dri/dri_extensions.c


> +static bool
> +dri2_is_opencl_interop_loaded_locked(struct dri_screen *screen)
> +{
> +   return screen->opencl_dri_event_add_ref &&
> +  screen->opencl_dri_event_release &&
> +  screen->opencl_dri_event_wait &&
> +  screen->opencl_dri_event_get_fence;
> +}
> +
> +static bool
> +dri2_load_opencl_interop(struct dri_screen *screen)
> +{
> +#if defined(RTLD_DEFAULT)
> +   bool success;
> +
> +   pipe_mutex_lock(screen->opencl_func_mutex);

pipe_mutex_lock/unlock need to be replaced with mtx_lock/unlock, because
the pipe_mutex wrappers no longer exist. See this commit.

commit e000b17f87bd960c4ce1c0892017023d4dc59609
Author: Brian Paul 
Date:   Wed Apr 5 15:15:27 2017 -0600

winsys/svga: use c11 thread types/functions

Gallium no longer has wrappers for mutexes and conditi

> +
> +   if (dri2_is_opencl_interop_loaded_locked(screen)) {
> +  pipe_mutex_unlock(screen->opencl_func_mutex);
> +  return true;
> +   }
> +
> +   screen->opencl_dri_event_add_ref =
> +  dlsym(RTLD_DEFAULT, "opencl_dri_event_add_ref");
> +   screen->opencl_dri_event_release =
> +  dlsym(RTLD_DEFAULT, "opencl_dri_event_release");
> +   screen->opencl_dri_event_wait =
> +  dlsym(RTLD_DEFAULT, "opencl_dri_event_wait");
> +   screen->opencl_dri_event_get_fence =
> +  dlsym(RTLD_DEFAULT, "opencl_dri_event_get_fence");
> +
> +   success = dri2_is_opencl_interop_loaded_locked(screen);
> +   pipe_mutex_unlock(screen->opencl_func_mutex);
> +   return success;
> +#else
> +   return false;
> +#endif
> +}

Those are the only issues I found. But, I don't know this code well, so
you should probably seek the review of someone who touched it recently,
like Rob Herring, Rob Clark, or Tim Arceri.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH] egl: Partially revert 23c86c74, fix eglMakeCurrent

[Chad Versace]

On Thu 18 May 2017, Tapani Pälli wrote:
> On 05/18/2017 12:16 AM, Tomasz Figa wrote:
> > Hi,
> > 
> > On Tue, May 16, 2017 at 11:35 PM, Tapani Pälli <tapani.pa...@intel.com> 
> > wrote:
> > > 
> > > On 05/16/2017 08:10 PM, Chad Versace wrote:
> > > > On Tue 16 May 2017, Tapani Pälli wrote:
> > > > > 
> > > > > 
> > > > > On 05/16/2017 02:04 AM, Chad Versace wrote:
> > > > > > Fixes regressions in Android CtsVerifier.apk on Intel Chrome OS 
> > > > > > devices
> > > > > > due to incorrect error handling in eglMakeCurrent. See below on how 
> > > > > > to
> > > > > > confirm the regression is fixed.
> > > > > > 
> > > > > > This partially reverts
> > > > > > 
> > > > > >commit 23c86c74cc450a23848b85cfe914376caede1cdf
> > > > > >Author:  Chad Versace <chadvers...@chromium.org>
> > > > > >Subject: egl: Emit error when EGLSurface is lost
> > > > > > 
> > > > > > The bad commit added the error handling below. #2 and #3 were right,
> > > > > > but #1 was wrong.
> > > > > > 
> > > > > >1. eglMakeCurrent emits EGL_BAD_CURRENT_SURFACE if the 
> > > > > > calling
> > > > > >   thread has unflushed commands and either previous surface 
> > > > > > is no
> > > > > >   longer valid.
> > > > > > 
> > > > > >2. eglMakeCurrent emits EGL_BAD_NATIVE_WINDOW if either new
> > > > > > surface
> > > > > >   is no longer valid.
> > > > > > 
> > > > > >3. eglSwapBuffers emits EGL_BAD_NATIVE_WINDOW if the swapped
> > > > > > surface
> > > > > >   is no longer valid.
> > > > > > 
> > > > > > Whe I wrote the bad commit, I misunderstood the EGL spec language
> > > > > > for #1. The correct behavior is, if I understand correctly now:
> > > > > > 
> > > > > >- Assume a bound EGLSurface is no longer valid.
> > > > > >- Assume the bound EGLContext has unflushed commands.
> > > > > >- The app calls eglMakeCurrent. The spec requires 
> > > > > > eglMakeCurrent
> > > > > > to
> > > > > >  implicitly flush. After flushing, eglMakeCurrent emits
> > > > > >  EGL_BAD_CURRENT_SURFACE and does *not* alter the thread's
> > > > > >  current bindings.
> > > > > >- If the app calls eglMakeCurrent again, and the app inserts 
> > > > > > no
> > > > > >  commands into the GL command stream between the two
> > > > > > eglMakeCurrent
> > > > > >  calls, then this second eglMakeCurrent succeeds without 
> > > > > > emitting
> > > > > > an
> > > > > >  error.
> > > > > > 
> > > > > > How to confirm this fixes the regression:
> > > > > > 
> > > > > >Download android-cts-verifier-7.1_r5-linux_x86-x86.zip from
> > > > > >source.android.com, unpack, and `adb install 
> > > > > > CtsVerifier.apk`.
> > > > > >Run test "Projection Cube". Click the Pass button (a
> > > > > >green checkmark). Then run test "Projection Widget". Confirm 
> > > > > > that
> > > > > >widgets are visible and that logcat does not complain about
> > > > > >eglMakeCurrent failure.
> > > > > > 
> > > > > >Then confirm there are no regressions in the cts-traded 
> > > > > > module
> > > > > > that
> > > > > >commit 263243b1 fixed:
> > > > > > 
> > > > > >cts-tf > run cts --skip-preconditions --skip-device-info 
> > > > > > \
> > > > > > -m CtsCameraTestCases \
> > > > > > -t android.hardware.camera2.cts.RobustnessTest
> > > > > > 
> > > > > >Tested with Chrome OS board "reef".
> > > > > 
> > 

Re: [Mesa-dev] [PATCH 2/7] egl/x11: check for dri2_dpy->flush before using the flush extension

[Chad Versace]

On Mon 15 May 2017, Emil Velikov wrote:
> From: Emil Velikov 
> 
> Analogous to previous commit.
> 
> Signed-off-by: Emil Velikov 
> ---

> If people prefer I can split the whitespace/indent changes in 
> this/previous path from the rest. Don't feel too strongly either way.

I do think the patches would be easier to review if you did that. As
written, it's difficult to see what's behavioral change vs cosmetic
cleanups.

> 
>  src/egl/drivers/dri2/platform_x11.c | 22 +-
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_x11.c 
> b/src/egl/drivers/dri2/platform_x11.c
> index 3bce0bb3f21..becf00547e6 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -817,8 +817,7 @@ dri2_copy_region(_EGLDriver *drv, _EGLDisplay *disp,
> if (draw->Type == EGL_PIXMAP_BIT || draw->Type == EGL_PBUFFER_BIT)
>return EGL_TRUE;
>  
> -   if (dri2_dpy->flush)
> -  dri2_dpy->flush->flush(dri2_surf->dri_drawable);
> +   dri2_dpy->flush->flush(dri2_surf->dri_drawable);
>  
> if (dri2_surf->have_fake_front)
>render_attachment = XCB_DRI2_ATTACHMENT_BUFFER_FAKE_FRONT_LEFT;
> @@ -880,8 +879,7 @@ dri2_x11_swap_buffers_msc(_EGLDriver *drv, _EGLDisplay 
> *disp, _EGLSurface *draw,
>  * happened.  The driver should still be using the viewport hack to catch
>  * window resizes.
>  */
> -   if (dri2_dpy->flush &&
> -   dri2_dpy->flush->base.version >= 3 && dri2_dpy->flush->invalidate)
> +   if (dri2_dpy->flush->base.version >= 3 && dri2_dpy->flush->invalidate)
>dri2_dpy->flush->invalidate(dri2_surf->dri_drawable);
>  
> return swap_count;
> @@ -893,19 +891,17 @@ dri2_x11_swap_buffers(_EGLDriver *drv, _EGLDisplay 
> *disp, _EGLSurface *draw)
> struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> struct dri2_egl_surface *dri2_surf = dri2_egl_surface(draw);
>  
> -   if (dri2_dpy->dri2) {
> -  if (dri2_x11_swap_buffers_msc(drv, disp, draw, 0, 0, 0) != -1) {
> -  return EGL_TRUE;
> -  }
> +   if (!dri2_dpy->flush) {
> +  dri2_dpy->core->swapBuffers(dri2_surf->dri_drawable);
> +  return EGL_TRUE;

Yes, thank you.

I'll say it again to anyone who's reading. The Go style guide insists on
this, for good reason.

https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

Try to keep the normal code path at a minimal indentation, and indent
the error handling, dealing with it first. This improves the readability
of the code by permitting visually scanning the normal path quickly.

> +   }
> +
> +   if (dri2_x11_swap_buffers_msc(drv, disp, draw, 0, 0, 0) == -1) {
>/* Swap failed with a window drawable. */
>_eglError(EGL_BAD_NATIVE_WINDOW, __func__);
>return EGL_FALSE;
> -   } else {
> -  assert(dri2_dpy->swrast);
> -
> -  dri2_dpy->core->swapBuffers(dri2_surf->dri_drawable);
> -  return EGL_TRUE;
> }
> +   return EGL_TRUE;
>  }

There's a lot of hidden complexity in platform_x11.c, and I'm unsure if
this patch preserves proper extension checks. I defer to someone who
knows this code better.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/7] egl/drm: check for dri2_dpy->flush before using the flush extension

[Chad Versace]

On Mon 15 May 2017, Emil Velikov wrote:
> From: Emil Velikov <emil.veli...@collabora.com>
> 
> The current __DRI_DRI2 imples __DRI2_FLUSH. At the same time, one can
> use __DRI_IMAGE_DRIVER alongside the latter, so the current check is
> confusing at best.
> 
> Check for what we use.

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

> 
> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
> ---
>  src/egl/drivers/dri2/platform_drm.c | 41 
> +++--
>  1 file changed, 21 insertions(+), 20 deletions(-)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] intel/isl/gen6: Fix combined depth stencil alignment

[Chad Versace]

On Mon 15 May 2017, Jason Ekstrand wrote:
> All combined depth stencil buffers (even those with just stencil)
> require a 4x4 alignment on Sandy Bridge.  The only depth/stencil buffer
> type that requires 4x2 is separate stencil.
> ---
>  src/intel/isl/isl_gen6.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)

Thanks.
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] egl: Partially revert 23c86c74, fix eglMakeCurrent

[Chad Versace]

On Tue 16 May 2017, Tapani Pälli wrote:
> 
> 
> On 05/16/2017 02:04 AM, Chad Versace wrote:
> > Fixes regressions in Android CtsVerifier.apk on Intel Chrome OS devices
> > due to incorrect error handling in eglMakeCurrent. See below on how to
> > confirm the regression is fixed.
> > 
> > This partially reverts
> > 
> >  commit 23c86c74cc450a23848b85cfe914376caede1cdf
> >  Author:  Chad Versace <chadvers...@chromium.org>
> >  Subject: egl: Emit error when EGLSurface is lost
> > 
> > The bad commit added the error handling below. #2 and #3 were right,
> > but #1 was wrong.
> > 
> >  1. eglMakeCurrent emits EGL_BAD_CURRENT_SURFACE if the calling
> > thread has unflushed commands and either previous surface is no
> > longer valid.
> > 
> >  2. eglMakeCurrent emits EGL_BAD_NATIVE_WINDOW if either new surface
> > is no longer valid.
> > 
> >  3. eglSwapBuffers emits EGL_BAD_NATIVE_WINDOW if the swapped surface
> > is no longer valid.
> > 
> > Whe I wrote the bad commit, I misunderstood the EGL spec language
> > for #1. The correct behavior is, if I understand correctly now:
> > 
> >  - Assume a bound EGLSurface is no longer valid.
> >  - Assume the bound EGLContext has unflushed commands.
> >  - The app calls eglMakeCurrent. The spec requires eglMakeCurrent to
> >implicitly flush. After flushing, eglMakeCurrent emits
> >EGL_BAD_CURRENT_SURFACE and does *not* alter the thread's
> >current bindings.
> >  - If the app calls eglMakeCurrent again, and the app inserts no
> >commands into the GL command stream between the two eglMakeCurrent
> >calls, then this second eglMakeCurrent succeeds without emitting an
> >error.
> > 
> > How to confirm this fixes the regression:
> > 
> >  Download android-cts-verifier-7.1_r5-linux_x86-x86.zip from
> >  source.android.com, unpack, and `adb install CtsVerifier.apk`.
> >  Run test "Projection Cube". Click the Pass button (a
> >  green checkmark). Then run test "Projection Widget". Confirm that
> >  widgets are visible and that logcat does not complain about
> >  eglMakeCurrent failure.
> > 
> >  Then confirm there are no regressions in the cts-traded module that
> >  commit 263243b1 fixed:
> > 
> >  cts-tf > run cts --skip-preconditions --skip-device-info \
> >   -m CtsCameraTestCases \
> >   -t android.hardware.camera2.cts.RobustnessTest
> > 
> >  Tested with Chrome OS board "reef".
> 
> both tests passed on Android-IA with this patch ... but if I minimize
> "Projection Widget" test it starts to bang EGL_BAD_NATIVE_WINDOW heavily. Is
> this expected behavior?

I'm unsure. I haven't yet tried that experiment. I'll give it a try when
I'm back at my desk.

Which EGL function is emitting EGL_BAD_NATIVE_WINDOW in logcat?
eglMakeCurrent or eglSwapBuffers, or something else?

What was the behavior before this patch? And before
commit 23c86c74 (egl: Emit error when EGLSurface is lost)?

> > Cc: "17.1" <mesa-sta...@lists.freedesktop.org>
> > Cc: Tomasz Figa <tf...@chromium.org>
> > Cc: Nicolas Boichat <drink...@chromium.org>
> > Cc: Emil Velikov <emil.veli...@collabora.com>
> > Fixes: 23c86c74 (egl: Emit error when EGLSurface is lost)
> > ---
> >   src/egl/main/eglapi.c | 19 ---
> >   1 file changed, 19 deletions(-)
> > 
> > diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> > index aa0eb94666..9cea2f41ff 100644
> > --- a/src/egl/main/eglapi.c
> > +++ b/src/egl/main/eglapi.c
> > @@ -828,25 +828,6 @@ eglMakeCurrent(EGLDisplay dpy, EGLSurface draw, 
> > EGLSurface read,
> >RETURN_EGL_ERROR(disp, EGL_BAD_MATCH, EGL_FALSE);
> >  }
> > -   _EGLThreadInfo *t =_eglGetCurrentThread();
> > -   _EGLContext *old_ctx = t->CurrentContext;
> > -   _EGLSurface *old_draw_surf = old_ctx ? old_ctx->DrawSurface : NULL;
> > -   _EGLSurface *old_read_surf = old_ctx ? old_ctx->ReadSurface : NULL;
> > -
> > -   /* From the EGL 1.5 spec, Section 3.7.3 Binding Context and Drawables:
> > -*
> > -*If the previous context of the calling thread has unflushed 
> > commands,
> > -*and the previous surface is no longer valid, an
> > -*EGL_BAD_CURRENT_SURFACE error is generated.
> > -*
> > -* It's difficult to check if the context has unflushed c

[Mesa-dev] [PATCH] egl: Partially revert 23c86c74, fix eglMakeCurrent

[Chad Versace]

Fixes regressions in Android CtsVerifier.apk on Intel Chrome OS devices
due to incorrect error handling in eglMakeCurrent. See below on how to
confirm the regression is fixed.

This partially reverts

commit 23c86c74cc450a23848b85cfe914376caede1cdf
Author:  Chad Versace <chadvers...@chromium.org>
Subject: egl: Emit error when EGLSurface is lost

The bad commit added the error handling below. #2 and #3 were right,
but #1 was wrong.

1. eglMakeCurrent emits EGL_BAD_CURRENT_SURFACE if the calling
   thread has unflushed commands and either previous surface is no
   longer valid.

2. eglMakeCurrent emits EGL_BAD_NATIVE_WINDOW if either new surface
   is no longer valid.

3. eglSwapBuffers emits EGL_BAD_NATIVE_WINDOW if the swapped surface
   is no longer valid.

Whe I wrote the bad commit, I misunderstood the EGL spec language
for #1. The correct behavior is, if I understand correctly now:

- Assume a bound EGLSurface is no longer valid.
- Assume the bound EGLContext has unflushed commands.
- The app calls eglMakeCurrent. The spec requires eglMakeCurrent to
  implicitly flush. After flushing, eglMakeCurrent emits
  EGL_BAD_CURRENT_SURFACE and does *not* alter the thread's
  current bindings.
- If the app calls eglMakeCurrent again, and the app inserts no
  commands into the GL command stream between the two eglMakeCurrent
  calls, then this second eglMakeCurrent succeeds without emitting an
  error.

How to confirm this fixes the regression:

Download android-cts-verifier-7.1_r5-linux_x86-x86.zip from
source.android.com, unpack, and `adb install CtsVerifier.apk`.
Run test "Projection Cube". Click the Pass button (a
green checkmark). Then run test "Projection Widget". Confirm that
widgets are visible and that logcat does not complain about
eglMakeCurrent failure.

Then confirm there are no regressions in the cts-traded module that
commit 263243b1 fixed:

cts-tf > run cts --skip-preconditions --skip-device-info \
 -m CtsCameraTestCases \
 -t android.hardware.camera2.cts.RobustnessTest

Tested with Chrome OS board "reef".

Cc: "17.1" <mesa-sta...@lists.freedesktop.org>
Cc: Tomasz Figa <tf...@chromium.org>
Cc: Nicolas Boichat <drink...@chromium.org>
Cc: Emil Velikov <emil.veli...@collabora.com>
Fixes: 23c86c74 (egl: Emit error when EGLSurface is lost)
---
 src/egl/main/eglapi.c | 19 ---
 1 file changed, 19 deletions(-)

diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index aa0eb94666..9cea2f41ff 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -828,25 +828,6 @@ eglMakeCurrent(EGLDisplay dpy, EGLSurface draw, EGLSurface 
read,
  RETURN_EGL_ERROR(disp, EGL_BAD_MATCH, EGL_FALSE);
}
 
-   _EGLThreadInfo *t =_eglGetCurrentThread();
-   _EGLContext *old_ctx = t->CurrentContext;
-   _EGLSurface *old_draw_surf = old_ctx ? old_ctx->DrawSurface : NULL;
-   _EGLSurface *old_read_surf = old_ctx ? old_ctx->ReadSurface : NULL;
-
-   /* From the EGL 1.5 spec, Section 3.7.3 Binding Context and Drawables:
-*
-*If the previous context of the calling thread has unflushed commands,
-*and the previous surface is no longer valid, an
-*EGL_BAD_CURRENT_SURFACE error is generated.
-*
-* It's difficult to check if the context has unflushed commands, but it's
-* easy to check if the surface is no longer valid.
-*/
-   if (old_draw_surf && old_draw_surf->Lost)
-  RETURN_EGL_ERROR(disp, EGL_BAD_CURRENT_SURFACE, EGL_FALSE);
-   if (old_read_surf && old_read_surf->Lost)
-  RETURN_EGL_ERROR(disp, EGL_BAD_CURRENT_SURFACE, EGL_FALSE);
-
/*If a native window underlying either draw or read is no longer valid,
 *an EGL_BAD_NATIVE_WINDOW error is generated.
 */
-- 
2.12.0

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

Re: [Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

[Chad Versace]

On Sat 13 May 2017, Jason Ekstrand wrote:
> On Fri, May 12, 2017 at 3:35 PM, Chad Versace <chadvers...@chromium.org>
> wrote:
> 
> > On Thu 11 May 2017, Jason Ekstrand wrote:
> > > On Thu, May 11, 2017 at 9:08 PM, Chad Versace <chadvers...@chromium.org>
> > > wrote:
> > >
> > > > On Thu 11 May 2017, Jason Ekstrand wrote:
> > > > > On Thu, May 11, 2017 at 7:03 AM, Pohjolainen, Topi <
> > > > > topi.pohjolai...@gmail.com> wrote:
> > > > >
> > > > > > On Wed, May 10, 2017 at 02:30:31PM -0700, Jason Ekstrand wrote:
> > > > > > > ---
> > > > > > >  src/intel/isl/isl_gen6.c | 30 --
> > > > > > >  1 file changed, 12 insertions(+), 18 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/intel/isl/isl_gen6.c b/src/intel/isl/isl_gen6.c
> > > > > > > index b746903..0de9620 100644
> > > > > > > --- a/src/intel/isl/isl_gen6.c
> > > > > > > +++ b/src/intel/isl/isl_gen6.c
> > > > > > > @@ -88,6 +88,8 @@ isl_gen6_choose_image_alignment_el(const
> > struct
> > > > > > isl_device *dev,
> > > > > > >  *| format | halign | valign |
> > > > > > >  *++++
> > > > > > >  *| YUV 4:2:2 formats  |  4 |  * |
> > > > > > > +*| BC1-5  |  4 |  4 |
> > > > > > > +*| FXT1   |  8 |  4 |
> > > > > > >  *| uncompressed formats   |  4 |  * |
> > > > > > >  *++++
> > > > > > >  *
> > > > > > > @@ -110,29 +112,13 @@ isl_gen6_choose_image_alignment_el(const
> > > > struct
> > > > > > isl_device *dev,
> > > > > > >  */
> > > > > > >
> > > > > > > if (isl_format_is_compressed(info->format)) {
> > > > > > > +  /* Compressed formats have an alignment equal to their
> > block
> > > > size
> > > > > > */
> > > > > > >*image_align_el = isl_extent3d(1, 1, 1);
> > > > > > >return;
> > > > > > > }
> > > > > > >
> > > > > > > -   if (isl_format_is_yuv(info->format)) {
> > > > > > > -  *image_align_el = isl_extent3d(4, 2, 1);
> > > > > > > -  return;
> > > > > > > -   }
> > > > > > > -
> > > > > > > -   if (info->samples > 1) {
> > > > > > > -  *image_align_el = isl_extent3d(4, 4, 1);
> > > > > > > -  return;
> > > > > > > -   }
> > > > > > > -
> > > > > > > -   if (isl_surf_usage_is_depth_or_stencil(info->usage) &&
> > > > > > > -   !ISL_DEV_USE_SEPARATE_STENCIL(dev)) {
> > > > > >
> > > > > > Maybe mention in the commit that we drop this as it is always
> > false on
> > > > > > gen6+?
> > > > > > In isl.c: "dev->use_separate_stencil = ISL_DEV_GEN(dev) >= 6;"
> > > > > >
> > > > >
> > > > > No, I dropped it not because we always use separate stencil but
> > because
> > > > > it's redundant with the regular depth case.  The PRM says to use a
> > 4x4
> > > > > alignment for all depth buffers but 4x2 for separate stencil.
> > Combined
> > > > > depth-stencil falls under the "depth" case so I didn't think we
> > needed to
> > > > > call it out explicitly.
> > > >
> > > > I admit that the condition I wrote
> > > >
> > > > if (isl_surf_usage_is_depth_or_stencil(info->usage) &&
> > > > !ISL_DEV_USE_SEPARATE_STENCIL(dev))
> > > >
> > > > was poorly chosen. I should've written it without relying on
> > > > ISL_DEV_USE_SEPARATE_STENCIL.
> > > >
> > > > Topi has a point because a surface with
> > > > format=ISL_FORMAT_X24_TYPELESS_G8_UINT will necessarily have
> > > > usage=ISL_SURF_USAGE_STENCIL_BIT (no depth bit). The

Re: [Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

[Chad Versace]

On Thu 11 May 2017, Jason Ekstrand wrote:
> On Thu, May 11, 2017 at 9:08 PM, Chad Versace <chadvers...@chromium.org>
> wrote:
> 
> > On Thu 11 May 2017, Jason Ekstrand wrote:
> > > On Thu, May 11, 2017 at 7:03 AM, Pohjolainen, Topi <
> > > topi.pohjolai...@gmail.com> wrote:
> > >
> > > > On Wed, May 10, 2017 at 02:30:31PM -0700, Jason Ekstrand wrote:
> > > > > ---
> > > > >  src/intel/isl/isl_gen6.c | 30 --
> > > > >  1 file changed, 12 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/src/intel/isl/isl_gen6.c b/src/intel/isl/isl_gen6.c
> > > > > index b746903..0de9620 100644
> > > > > --- a/src/intel/isl/isl_gen6.c
> > > > > +++ b/src/intel/isl/isl_gen6.c
> > > > > @@ -88,6 +88,8 @@ isl_gen6_choose_image_alignment_el(const struct
> > > > isl_device *dev,
> > > > >  *| format | halign | valign |
> > > > >  *++++
> > > > >  *| YUV 4:2:2 formats  |  4 |  * |
> > > > > +*| BC1-5  |  4 |  4 |
> > > > > +*| FXT1   |  8 |  4 |
> > > > >  *| uncompressed formats   |  4 |  * |
> > > > >  *++++
> > > > >  *
> > > > > @@ -110,29 +112,13 @@ isl_gen6_choose_image_alignment_el(const
> > struct
> > > > isl_device *dev,
> > > > >  */
> > > > >
> > > > > if (isl_format_is_compressed(info->format)) {
> > > > > +  /* Compressed formats have an alignment equal to their block
> > size
> > > > */
> > > > >*image_align_el = isl_extent3d(1, 1, 1);
> > > > >return;
> > > > > }
> > > > >
> > > > > -   if (isl_format_is_yuv(info->format)) {
> > > > > -  *image_align_el = isl_extent3d(4, 2, 1);
> > > > > -  return;
> > > > > -   }
> > > > > -
> > > > > -   if (info->samples > 1) {
> > > > > -  *image_align_el = isl_extent3d(4, 4, 1);
> > > > > -  return;
> > > > > -   }
> > > > > -
> > > > > -   if (isl_surf_usage_is_depth_or_stencil(info->usage) &&
> > > > > -   !ISL_DEV_USE_SEPARATE_STENCIL(dev)) {
> > > >
> > > > Maybe mention in the commit that we drop this as it is always false on
> > > > gen6+?
> > > > In isl.c: "dev->use_separate_stencil = ISL_DEV_GEN(dev) >= 6;"
> > > >
> > >
> > > No, I dropped it not because we always use separate stencil but because
> > > it's redundant with the regular depth case.  The PRM says to use a 4x4
> > > alignment for all depth buffers but 4x2 for separate stencil.  Combined
> > > depth-stencil falls under the "depth" case so I didn't think we needed to
> > > call it out explicitly.
> >
> > I admit that the condition I wrote
> >
> > if (isl_surf_usage_is_depth_or_stencil(info->usage) &&
> > !ISL_DEV_USE_SEPARATE_STENCIL(dev))
> >
> > was poorly chosen. I should've written it without relying on
> > ISL_DEV_USE_SEPARATE_STENCIL.
> >
> > Topi has a point because a surface with
> > format=ISL_FORMAT_X24_TYPELESS_G8_UINT will necessarily have
> > usage=ISL_SURF_USAGE_STENCIL_BIT (no depth bit). The hardware requires
> > the surface to have valign=4, and my badly written (but correct)
> > condition ensured valign=4 in this case. After this patch, the function
> > wrongly chooses valign=2.
> >
> 
> No it won't.  It's hard to see in patch form, but after this patch is
> applied we check for depth first and then stencil.  If it has ANY depth
> component, then it will get 4x4 aligned.  Only separate stencil gets 4x2.

This function, pre-patch and post-patch, does not inspect the format for
a depth component.  It checks if the depth usage flag is set. That's the
key difference.

On gen6, it's possible to create a usable surface with format
X24_TYPELESS_G8_UINT with ISL_SURF_USAGE_STENCIL_BIT and no depth bit.
And, if I understand this patch correctly, that surface does not satisfy
the depth check in this patch:

if (isl_surf_usage_is_depth(info->usage)) {
   /* depth buffer (possibly interleaved with stencil) */

Re: [Mesa-dev] [PATCH 4/4] intel/isl: Refactor gen8_choose_image_alignment_el

[Chad Versace]

On Wed 10 May 2017, Jason Ekstrand wrote:
> ---
>  src/intel/isl/isl_gen8.c | 193 
> +--
>  1 file changed, 53 insertions(+), 140 deletions(-)


> +   /* For all other formats, the alignment is determined by the horizontal 
> and
> +* vertical alignment fields of RENDER_SURFACE_STATE.  There are a few
> +* restrictions, but we generally have a choice.
> +*/
>  
[...]
> +   /* Vertical alignment is unrestricted so we choose the smallest allowed
> +* alignment because that will use the least memory
>  */
> +   const uint32_t valign = 4;
>  
> +   bool needs_halign16 = false;
> if (!(info->usage & ISL_SURF_USAGE_DISABLE_AUX_BIT)) {
[...]
> +  needs_halign16 = true;
> }

> +   const uint32_t halign = needs_halign16 ? 16 : 4;
[...]
> +   *image_align_el = isl_extent3d(halign, valign, 1);
>  }

Small suggestion. The code would be more straightforward without the
temporary valign, needs_halign16 vars. Something like this:

...

if (!(info->usage & ISL_SURF_USAGE_DISABLE_AUX_BIT)) {
*image_align_el = isl_extent3d(16, 4, 1);
return;
}

*image_align_el = isl_extent3d(4, 4, 1);
    }

With or without that change, this patch is a good cleanup.
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 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

[Chad Versace]

On Thu 11 May 2017, Jason Ekstrand wrote:
> On Thu, May 11, 2017 at 7:03 AM, Pohjolainen, Topi <
> topi.pohjolai...@gmail.com> wrote:
> 
> > On Wed, May 10, 2017 at 02:30:31PM -0700, Jason Ekstrand wrote:
> > > ---
> > >  src/intel/isl/isl_gen6.c | 30 --
> > >  1 file changed, 12 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/src/intel/isl/isl_gen6.c b/src/intel/isl/isl_gen6.c
> > > index b746903..0de9620 100644
> > > --- a/src/intel/isl/isl_gen6.c
> > > +++ b/src/intel/isl/isl_gen6.c
> > > @@ -88,6 +88,8 @@ isl_gen6_choose_image_alignment_el(const struct
> > isl_device *dev,
> > >  *| format | halign | valign |
> > >  *++++
> > >  *| YUV 4:2:2 formats  |  4 |  * |
> > > +*| BC1-5  |  4 |  4 |
> > > +*| FXT1   |  8 |  4 |
> > >  *| uncompressed formats   |  4 |  * |
> > >  *++++
> > >  *
> > > @@ -110,29 +112,13 @@ isl_gen6_choose_image_alignment_el(const struct
> > isl_device *dev,
> > >  */
> > >
> > > if (isl_format_is_compressed(info->format)) {
> > > +  /* Compressed formats have an alignment equal to their block size
> > */
> > >*image_align_el = isl_extent3d(1, 1, 1);
> > >return;
> > > }
> > >
> > > -   if (isl_format_is_yuv(info->format)) {
> > > -  *image_align_el = isl_extent3d(4, 2, 1);
> > > -  return;
> > > -   }
> > > -
> > > -   if (info->samples > 1) {
> > > -  *image_align_el = isl_extent3d(4, 4, 1);
> > > -  return;
> > > -   }
> > > -
> > > -   if (isl_surf_usage_is_depth_or_stencil(info->usage) &&
> > > -   !ISL_DEV_USE_SEPARATE_STENCIL(dev)) {
> >
> > Maybe mention in the commit that we drop this as it is always false on
> > gen6+?
> > In isl.c: "dev->use_separate_stencil = ISL_DEV_GEN(dev) >= 6;"
> >
> 
> No, I dropped it not because we always use separate stencil but because
> it's redundant with the regular depth case.  The PRM says to use a 4x4
> alignment for all depth buffers but 4x2 for separate stencil.  Combined
> depth-stencil falls under the "depth" case so I didn't think we needed to
> call it out explicitly.

I admit that the condition I wrote

if (isl_surf_usage_is_depth_or_stencil(info->usage) &&
!ISL_DEV_USE_SEPARATE_STENCIL(dev))

was poorly chosen. I should've written it without relying on
ISL_DEV_USE_SEPARATE_STENCIL.

Topi has a point because a surface with
format=ISL_FORMAT_X24_TYPELESS_G8_UINT will necessarily have
usage=ISL_SURF_USAGE_STENCIL_BIT (no depth bit). The hardware requires
the surface to have valign=4, and my badly written (but correct)
condition ensured valign=4 in this case. After this patch, the function
wrongly chooses valign=2.

I suggest either

a. Asserting that separate stencil is true in the neighborhood of

if (isl_surf_usage_is_depth(info->usage)) {
/* depth buffer (possibly interleaved with stencil) */
*image_align_el = isl_extent3d(4, 4, 1);
return;
}

  and dropping the reference to possible interleaved stencil.

b. Or, better, rewrite the original logic to be clearer.

if ((info->usage & ISL_SURF_USAGE_STENCIL_BIT)
&& info->format == ISL_FORMAT_R8_UINT) {
/* separate stencil surface */
*image_align_el = isl_extent3d(4, 2, 1);
return;
}

if (info->usage & (ISL_SURF_USAGE_DEPTH_BIT | 
ISL_SURF_USAGE_STENCIL_BIT))
/* separate depth surface or interleaved depthstencil */
*image_align_el = isl_extent3d(4, 4, 1);
return;
}

> > Other than that:
> >
> > Reviewed-by: Topi Pohjolainen 
> >
> > > -  /* interleaved depthstencil buffer */
> > > -  *image_align_el = isl_extent3d(4, 4, 1);
> > > -  return;
> > > -   }
> > > -
> > > if (isl_surf_usage_is_depth(info->usage)) {
> > > -  /* separate depth buffer */
> > > +  /* depth buffer (possibly interleaved with stencil) */
> > >*image_align_el = isl_extent3d(4, 4, 1);
> > >return;
> > > }
> > > @@ -143,5 +129,13 @@ isl_gen6_choose_image_alignment_el(const struct
> > isl_device *dev,
> > >return;
> > > }
> > >
> > > +   if (info->samples > 1) {
> > > +  *image_align_el = isl_extent3d(4, 4, 1);
> > > +  return;
> > > +   }
> > > +
> > > +   /* For everything else, we can chose any vertical alignment we
> > want.  We
> > > +* choose an alignment of 2 because it uses the least memory.
> > > +*/
> > > *image_align_el = isl_extent3d(4, 2, 1);

The code is correct, but the comment is wrong. We cannot choose any
valign we wish for the remaining cases. It is true that 2 is *valid* for
all remaining cases, but it is also *required* for some of those cases,
as explained in the big comment block above. 

Re: [Mesa-dev] [PATCH 2/4] intel/isl: Refactor gen7_choose_image_alignment_el

[Chad Versace]

turn;
> > +   } else if (isl_format_is_compressed(info->format)) {
> > +  /* Compressed formats all have alignment equal to block size. */
> > +  *image_align_el = isl_extent3d(1, 1, 1);
> > +  return;
> > +   }
> >  
> > -/**
> > - * Choose vertical subimage alignment, in units of surface elements.
> > - */
> > -static uint32_t
> > -gen7_choose_valign_el(const struct isl_device *dev,
> > -  const struct isl_surf_init_info *restrict info,
> > -  enum isl_tiling tiling)
> > -{
> > -   MAYBE_UNUSED bool require_valign2 = false;
> > -   bool require_valign4 = false;
> > +   /* Everything after this point is in the "set by Surface Horizontal or
> > +* Vertical Alignment" case.  Now it's just a matter of applying
> > +* restrictions.
> > +*/
> >  
> > -   if (isl_format_is_compressed(info->format))
> > -  return 1;
> > +   /* There are no restrictions on halign beyond what's given in the table
> > +* above.  We set it to the minimum value of 4 because that uses the 
> > least
> > +* memory.
> > +*/
> > +   const uint32_t halign = 4;
> >  
> > -   if (gen7_format_needs_valign2(dev, info->format))
> > -  require_valign2 = true;
> > +   bool require_valign4 = false;
> >  
> > /* From the Ivybridge PRM, Volume 4, Part 1, Section 2.12.1:
> >  * RENDER_SURFACE_STATE Surface Vertical Alignment:
> >  *
> > -*- This field is intended to be set to VALIGN_4 if the surface was
> > -*  rendered as a depth buffer, for a multisampled (4x) render 
> > target,
> > -*  or for a multisampled (8x) render target, since these surfaces
> > -*  support only alignment of 4.  Use of VALIGN_4 for other 
> > surfaces is
> > -*  supported, but uses more memory.  This field must be set to
> > -*  VALIGN_4 for all tiled Y Render Target surfaces.
> > +** This field is intended to be set to VALIGN_4 if the surface was
> > +*  rendered as a depth buffer,
> >  *
> > +** for a multisampled (4x) render target, or for a multisampled 
> > (8x)
> > +*  render target, since these surfaces support only alignment of 4.
> > +*
> > +** This field must be set to VALIGN_4 for all tiled Y Render Target
> > +*  surfaces
> > +*
> > +** Value of 1 is not supported for format YCRCB_NORMAL (0x182),
> > +*  YCRCB_SWAPUVY (0x183), YCRCB_SWAPUV (0x18f), YCRCB_SWAPY (0x190)
> > +*
> > +** If Number of Multisamples is not MULTISAMPLECOUNT_1, this field
> > +*  must be set to VALIGN_4."
> > +*
> > +* The first restriction is already handled by the table above and the
> > +* second restriction is redundant with the fifth.
> >  */
> > -   if (isl_surf_usage_is_depth(info->usage) ||
> > -   info->samples > 1 ||
> > -   ((info->usage & ISL_SURF_USAGE_RENDER_TARGET_BIT) &&
> > -tiling == ISL_TILING_Y0)) {
> > +   if (info->samples > 1)
> >require_valign4 = true;
> > -   }
> > -
> > -   if (isl_surf_usage_is_stencil(info->usage)) {
> > -  /* The Ivybridge PRM states that the stencil buffer's vertical 
> > alignment
> > -   * is 8 [Ivybridge PRM, Volume 1, Part 1, Section 6.18.4.4 Alignment
> > -   * Unit Size]. valign=8 is outside the set of valid values of
> > -   * RENDER_SURFACE_STATE.SurfaceVerticalAlignment, but that's ok 
> > because
> > -   * a stencil buffer will never be used directly for texturing or
> > -   * rendering on gen7.
> > -   */
> > -  return 8;
> > -   }
> > -
> > -   assert(!require_valign2 || !require_valign4);
> > -
> > -   if (require_valign4)
> > -  return 4;
> >  
> > -   /* Prefer VALIGN_2 because it conserves memory. */
> > -   return 2;
> > -}
> > -
> > -void
> > -isl_gen7_choose_image_alignment_el(const struct isl_device *dev,
> > -   const struct isl_surf_init_info 
> > *restrict info,
> > -   enum isl_tiling tiling,
> > -   enum isl_dim_layout dim_layout,
> > -   enum isl_msaa_layout msaa_layout,
> > -   struct isl_extent3d *image_align_el)
> > -{
> > -   assert(ISL_DEV_GEN(dev) == 7);
> > +   if (tiling == ISL_TILING_Y0 &&
> > +   (info->usage & ISL_SURF_USAGE_RENDER_TARGET_BIT))
> > +  require_valign4 = true;
> >  
> > -   /* Handled by isl_choose_image_alignment_el */
> > -   assert(info->format != ISL_FORMAT_HIZ);
> > +   assert(!(require_valign4 && gen7_format_needs_valign2(dev, 
> > info->format)));
> >  
> > -   /* IVB+ does not support combined depthstencil. */
> > -   assert(!isl_surf_usage_is_depth_and_stencil(info->usage));
> > +   /* We default to HALIGN_2 because it uses the least memory. */
> 
>VALIGN_2
> 
> Otherwise looks good to me:
> 
> Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>

Me too. Combining the two functions really simplifies things.
Reviewed-by: Chad Versace <chadvers...@chromium.org>

Please fix the hiccup in the commit message ("we then to").
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/4] intel/isl/gen7: Use stencil vertical alignment of 8 instead of 4

[Chad Versace]

On Wed 10 May 2017, Jason Ekstrand wrote:
> From: "Pohjolainen, Topi" <topi.pohjolai...@gmail.com>
> 
> The reasoning Chad gave in the comment for choosing a valign of 4 is
> entirely bunk.  The fact that you have to multiply pitch by 2 is
> completely unrelated to the halign/valign parameters used for texture
> layout.  (Not completely unrelated.  W-tiling is just Y-tiling with a
> bit of extra swizzling which turns 8x8 W-tiled chunks into 16x4 y-tiled
> chunks so it makes everything easier if miplevels are always aligned to
> 8x8.)  The fact that RENDER_SURFACE_STATE::SurfaceVerticalAlignmet
> doesn't have a VALIGN_8 option doesn't matter since this is gen7 and you
> can't do stencil texturing anyway.
> 
> v2 (Jason Ekstrand):
>  - Delete most of Chad's comment and add a more descriptive commit
>message.
> 
> Signed-off-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
> Cc: "17.0 17.1" <mesa-sta...@lists.freedesktop.org>
> Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>


> -   * FINISHME(chadv): Decide to set valign=4 or valign=8 after isl's API
> -   * is more polished.

Thanks for finishing that :)
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] i965: Use isl for hiz and stencil

[Chad Versace]

On Tue 09 May 2017, Jason Ekstrand wrote:
> All of the below being said, I'm fine with landing things with a
> BACK_TO_BACK layout and then making the switch to one isl_surf per miplevel
> later if that's an easier path.

I haven't read the series in full, but I agree with Jason here that an
array of isl_surf, one per miplevel, makes more sense than a new isl
layout. One reason is that's a more accurate description of the
hardware's behavior.

FWIW, before I left Intel, I had planned to implement gen6 hiz with an
array of isl_surf.

Don't let my comments block the series, though. I doubt I'll find the
time to review it all.

> On Tue, May 9, 2017 at 8:45 AM, Jason Ekstrand  wrote:
> 
> > On Mon, May 8, 2017 at 11:10 PM, Pohjolainen, Topi <
> > topi.pohjolai...@gmail.com> wrote:
> >
> >> On Mon, May 08, 2017 at 04:51:35PM -0700, Jason Ekstrand wrote:
> >> > On Wed, May 3, 2017 at 2:22 AM, Topi Pohjolainen <
> >> topi.pohjolai...@gmail.com
> >> > > wrote:
> >> >
> >> > > Patches 1-17 are revision that
> >> > >
> >> > >   - rework hiz on gen6 to use on-demand offset calculator allowing
> >> > > one to drop dependency to miptree structure and
> >> > >   - rework all auxiliary surfaces to be created against isl directly.
> >> > >
> >> > > Patches 18 and 19 introduce new surface layout in ISL. This is called
> >> > > back-to-back and similar to layout ALL_SLICES_AT_EACH_LOD found in
> >> > > i965 for gen6 hiz and stencil. This layout stacks slices for each
> >> level
> >> > > after one and other, or back to back. All slices ate each lod is
> >> almost
> >> > > the same except that it places levels one and two side-by-side trying
> >> > > to preserve space. Back-to-back wastes a little more memory but aligns
> >> > > each level on page boundary simplifying driver logic.
> >> > >
> >> >
> >> > My primary gripe here is that you seem to have half-added back-to-back
> >> to
> >> > ISL.  If this layout is a long-term thing, then we should add a new
> >> > ISL_DIM_LAYOUT_GEN6_BACK_TO_BACK layout and plumb your offset function
> >> > through isl_surf_get_image_offset_sa.  Is this intended to be a
> >> permanent
> >> > solution?  I think eventually, I'd like us to go with one surf per
> >> miplevel
> >> > (which is almost the same) but I can see how this is easier at the
> >> moment.
> >> > However, I think this works sufficiently well that I'm ok with doing the
> >> > back-to-back thing for a while.
> >>
> >> I thought about adding new layout type but couldn't decide which way is
> >> better. It is easy to buy your arguments in favor, and I'm happy to give
> >> it
> >> a go.
> >> If miptree per level is your number one choice, then lets go with that.
> >
> >
> > I said "one surf per miplevel".  I see no reason why we need N miptrees.
> >
> >
> >> I just
> >> need to check a few things first about the actual solution. I would see
> >> something in these lines:
> >>
> >> 1) Add a dynamically allocated array of miptrees into miptree. This would
> >>contain miptree instance per level.
> >>
> >> 2) Still uses one buffer object containing space for all levels. The
> >> instances
> >>in the array would either have their ::bo pointer zero or pointing to
> >> the
> >>parent ::bo. In both cases ::offset would point the start of the level.
> >>
> >
> > Yes
> >
> >
> >> 3) Instances in the array are not reference counted and therefore deleted
> >>simply by deallocating the malloced chunk underneath.
> >>
> >
> > If we have one isl_surf per miplevel and not a miptree per level, then I
> > don't think this is an issue.
> >
> >
> >> 4) Add similar dynamically allocated array of intel_miptree_aux_buffer
> >>instances for hiz. Here also use one ::bo which would need to added to
> >>miptree I think cause there ins't one in miptree. Or perhaps add the
> >>array of aux buffers to aux buffer?
> >>
> >
> > Looking at intel_miptree_aux_buffer, I think what we would end up with is
> > an array of aux_buffers
> >
> >
> >> 5) ISL doesn't need to know about this and hence we would add the total
> >> space
> >>calculator along with ::offset initialization in i965 (brw_tex_layout,
> >>I think).
> >>
> >
> > That's fine.  We already do that in Vulkan with anv_surface.  ::offset
> > calculation can be done easily enough by just adding sizes.
> >
> >
> >> 6) In Vulkan <-> GL interop, we'd pass single level arrays only as ISL
> >> didn't
> >>know about back-2-back. Or we simply don't care about gen6 as Vulkan
> >>doesn't support it anyhow?
> >>
> >
> > Yeah, we don't care about gen6.
> >
> > --Jason
> >

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

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

Re: [Mesa-dev] [RFC] intel/isl: Rewrite gen7_choose_image_alignment_el

[Chad Versace]

 {
do_stuff;
} else if (oh_what_did_you_say) {
return big_thing;
}

do_more_stuff();
return ok_now;


> But it's largely a matter of taste, so...*shrug*.

Not always. The Go code guidelines strongly recommend the "early
return" style for error handling. The guide provides a good explanation.
<https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow>

> If I were writing this, I would probably handle the R16 case as:
> 
>if (info->format == ISL_FORMAT_R16_UNORM) {
>   return isl_extent3d(8, 4, 1);
> 
>return isl_extent3d(4, 4, 1);
> 
> but leave the rest as is.  Totally up to you though, Jason.

Anyway, I've stated my preference. Either way, this patch is a large
improvement over my original isl 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 18/21] anv: Implement support for exporting semaphores as FENCE_FD

[Chad Versace]

On Tue 09 May 2017, Jason Ekstrand wrote:
> On Tue, May 9, 2017 at 4:49 PM, Chad Versace <chadvers...@chromium.org>
> wrote:
> 
> > I learned a lot by reviewing this patch. Before reviewing it, some parts
> > of the external_semaphore spec still confused me. But reviewing this
> > forced me to really learn the behavior of external semaphores.
> >
> > Other than some small security nits, the only real issue I found was
> > the error behavior of vkQueueSubmit. Maybe I'm wrong and your error
> > behavior is fine, though. Let's discuss.
> >
> > A little complaint against the current spec... The patch was hard to
> > review because the 1.0.49 has incorrect language regarding external
> > semaphores. I had to go review the unreleased spec to get to the truth.
> > Anyway, on with review...
> >
> > On Fri 14 Apr 2017, Jason Ekstrand wrote:
> > > ---
> > >  src/intel/vulkan/anv_batch_chain.c | 96 ++
> > ++--
> > >  src/intel/vulkan/anv_device.c  | 25 ++
> > >  src/intel/vulkan/anv_gem.c | 36 ++
> > >  src/intel/vulkan/anv_private.h | 24 +++---
> > >  src/intel/vulkan/anv_queue.c   | 73 +++--
> > >  5 files changed, 240 insertions(+), 14 deletions(-)
> >
> > > diff --git a/src/intel/vulkan/anv_batch_chain.c
> > b/src/intel/vulkan/anv_batch_chain.c
> > > index 0529f22..ec37c81 100644
> > > --- a/src/intel/vulkan/anv_batch_chain.c
> > > +++ b/src/intel/vulkan/anv_batch_chain.c
> > > @@ -1387,6 +1387,23 @@ setup_execbuf_for_cmd_buffer(struct anv_execbuf
> > *execbuf,
> > > return VK_SUCCESS;
> > >  }
> > >
> > > +static void
> > > +setup_empty_execbuf(struct anv_execbuf *execbuf, struct anv_device
> > *device)
> > > +{
> > > +   anv_execbuf_add_bo(execbuf, >trivial_batch_bo, NULL, 0,
> > > +  >alloc);
> > > +
> > > +   execbuf->execbuf = (struct drm_i915_gem_execbuffer2) {
> > > +  .buffers_ptr = (uintptr_t) execbuf->objects,
> > > +  .buffer_count = execbuf->bo_count,
> > > +  .batch_start_offset = 0,
> > > +  .batch_len = 8, /* GEN8_MI_BATCH_BUFFER_END and NOOP */
> >
> > Instead of hard-coding a magic number here, I think
> > .batch_len = device->trivial_batch_bo.next - device->trivial_batch_bo.
> > start,
> > is better. With that, the comment never becomes stale.
> >
> > > +  .flags = I915_EXEC_HANDLE_LUT | I915_EXEC_RENDER,
> > > +  .rsvd1 = device->context_id,
> > > +  .rsvd2 = 0,
> > > +   };
> > > +}
> > > +
> > >  VkResult
> > >  anv_cmd_buffer_execbuf(struct anv_device *device,
> > > struct anv_cmd_buffer *cmd_buffer,
> > > @@ -1398,11 +1415,13 @@ anv_cmd_buffer_execbuf(struct anv_device *device,
> > > struct anv_execbuf execbuf;
> > > anv_execbuf_init();
> > >
> > > +   int in_fence = -1;
> > > VkResult result = VK_SUCCESS;
> > > for (uint32_t i = 0; i < num_in_semaphores; i++) {
> > >ANV_FROM_HANDLE(anv_semaphore, semaphore, in_semaphores[i]);
> > > -  assert(semaphore->temporary.type == ANV_SEMAPHORE_TYPE_NONE);
> > > -  struct anv_semaphore_impl *impl = >permanent;
> > > +  struct anv_semaphore_impl *impl =
> > > + semaphore->temporary.type != ANV_SEMAPHORE_TYPE_NONE ?
> > > + >temporary : >permanent;
> > >
> > >switch (impl->type) {
> > >case ANV_SEMAPHORE_TYPE_BO:
> > > @@ -1411,13 +1430,42 @@ anv_cmd_buffer_execbuf(struct anv_device *device,
> > >   if (result != VK_SUCCESS)
> > >  return result;
> > >   break;
> > > +
> > > +  case ANV_SEMAPHORE_TYPE_SYNC_FILE:
> > > + if (in_fence == -1) {
> > > +in_fence = impl->fd;
> > > + } else {
> > > +int merge = anv_gem_sync_file_merge(device, in_fence,
> > impl->fd);
> > > +if (merge == -1)
> > > +   return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX);
> >
> > Because we immediately close the semaphore's fd, the spec does not allow
> > us to return VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX here. We must either
> > defer closing the fd until immediately before VK_SUCCESS, or return
> > VK_ERROR_DEVICE_LOST.
> >
&

Re: [Mesa-dev] [PATCH 18/21] anv: Implement support for exporting semaphores as FENCE_FD

[Chad Versace]

I learned a lot by reviewing this patch. Before reviewing it, some parts
of the external_semaphore spec still confused me. But reviewing this
forced me to really learn the behavior of external semaphores.

Other than some small security nits, the only real issue I found was
the error behavior of vkQueueSubmit. Maybe I'm wrong and your error
behavior is fine, though. Let's discuss.

A little complaint against the current spec... The patch was hard to
review because the 1.0.49 has incorrect language regarding external
semaphores. I had to go review the unreleased spec to get to the truth.
Anyway, on with review...

On Fri 14 Apr 2017, Jason Ekstrand wrote:
> ---
>  src/intel/vulkan/anv_batch_chain.c | 96 
> --
>  src/intel/vulkan/anv_device.c  | 25 ++
>  src/intel/vulkan/anv_gem.c | 36 ++
>  src/intel/vulkan/anv_private.h | 24 +++---
>  src/intel/vulkan/anv_queue.c   | 73 +++--
>  5 files changed, 240 insertions(+), 14 deletions(-)

> diff --git a/src/intel/vulkan/anv_batch_chain.c 
> b/src/intel/vulkan/anv_batch_chain.c
> index 0529f22..ec37c81 100644
> --- a/src/intel/vulkan/anv_batch_chain.c
> +++ b/src/intel/vulkan/anv_batch_chain.c
> @@ -1387,6 +1387,23 @@ setup_execbuf_for_cmd_buffer(struct anv_execbuf 
> *execbuf,
> return VK_SUCCESS;
>  }
>  
> +static void
> +setup_empty_execbuf(struct anv_execbuf *execbuf, struct anv_device *device)
> +{
> +   anv_execbuf_add_bo(execbuf, >trivial_batch_bo, NULL, 0,
> +  >alloc);
> +
> +   execbuf->execbuf = (struct drm_i915_gem_execbuffer2) {
> +  .buffers_ptr = (uintptr_t) execbuf->objects,
> +  .buffer_count = execbuf->bo_count,
> +  .batch_start_offset = 0,
> +  .batch_len = 8, /* GEN8_MI_BATCH_BUFFER_END and NOOP */

Instead of hard-coding a magic number here, I think
.batch_len = device->trivial_batch_bo.next - device->trivial_batch_bo.start,
is better. With that, the comment never becomes stale.

> +  .flags = I915_EXEC_HANDLE_LUT | I915_EXEC_RENDER,
> +  .rsvd1 = device->context_id,
> +  .rsvd2 = 0,
> +   };
> +}
> +
>  VkResult
>  anv_cmd_buffer_execbuf(struct anv_device *device,
> struct anv_cmd_buffer *cmd_buffer,
> @@ -1398,11 +1415,13 @@ anv_cmd_buffer_execbuf(struct anv_device *device,
> struct anv_execbuf execbuf;
> anv_execbuf_init();
>  
> +   int in_fence = -1;
> VkResult result = VK_SUCCESS;
> for (uint32_t i = 0; i < num_in_semaphores; i++) {
>ANV_FROM_HANDLE(anv_semaphore, semaphore, in_semaphores[i]);
> -  assert(semaphore->temporary.type == ANV_SEMAPHORE_TYPE_NONE);
> -  struct anv_semaphore_impl *impl = >permanent;
> +  struct anv_semaphore_impl *impl =
> + semaphore->temporary.type != ANV_SEMAPHORE_TYPE_NONE ?
> + >temporary : >permanent;
>  
>switch (impl->type) {
>case ANV_SEMAPHORE_TYPE_BO:
> @@ -1411,13 +1430,42 @@ anv_cmd_buffer_execbuf(struct anv_device *device,
>   if (result != VK_SUCCESS)
>  return result;
>   break;
> +
> +  case ANV_SEMAPHORE_TYPE_SYNC_FILE:
> + if (in_fence == -1) {
> +in_fence = impl->fd;
> + } else {
> +int merge = anv_gem_sync_file_merge(device, in_fence, impl->fd);
> +if (merge == -1)
> +   return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX);

Because we immediately close the semaphore's fd, the spec does not allow
us to return VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX here. We must either
defer closing the fd until immediately before VK_SUCCESS, or return
VK_ERROR_DEVICE_LOST.

From the 1.0.49 spec:

If vkQueueSubmit fails, it may return VK_ERROR_OUT_OF_HOST_MEMORY or
VK_ERROR_OUT_OF_DEVICE_MEMORY.  If it does, the implementation must
ensure that the state and contents of any resources or
synchronization primitives referenced by the submitted command
buffers and any semaphores referenced by pSubmits is unaffected by
the call or its failure. If vkQueueSubmit fails in such a way that
the implementation can not make that guarantee, the implementation
must return VK_ERROR_DEVICE_LOST

> +
> +close(impl->fd);
> +close(in_fence);
> +in_fence = merge;
> + }
> +
> + impl->fd = -1;
> + break;
> +
>default:
>   break;
>}
> +
> +  /* Waiting on a semaphore with temporary state implicitly resets it 
> back
> +   * to the permanent state.
> +   */
> +  if (semaphore->temporary.type != ANV_SEMAPHORE_TYPE_NONE) {
> + assert(semaphore->temporary.type == ANV_SEMAPHORE_TYPE_SYNC_FILE);
> + semaphore->temporary.type = ANV_SEMAPHORE_TYPE_NONE;
> +  }
> }
>  
> +   bool need_out_fence = false;
> for (uint32_t i = 0; i < num_out_semaphores; i++) {
>ANV_FROM_HANDLE(anv_semaphore, semaphore, out_semaphores[i]);
> +  /* Out 

Re: [Mesa-dev] [PATCH] configure.ac: Also match -androideabi tuple

[Chad Versace]

On Fri 05 May 2017, Nicolas Boichat wrote:
> From: Nicolas Boichat <drink...@chromium.org>
> 
> On ARM Android platforms, the host_os tuple should be linux-androideabi,
> so let's match both -android and -androideabi (or any other
> -android* tuple) to determine if we should do an Android build.
> ---
>  configure.ac | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index a614458ab7c..df3eb6b29ac 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -269,7 +269,7 @@ DEFINES="-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS 
> -D__STDC_LIMIT_MACROS"
>  AC_SUBST([DEFINES])
>  android=no
>  case "$host_os" in
> -*-android)
> +*-android*)
>  android=yes
>  ;;
>  linux*|*-gnu*|gnu*|cygwin*)

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

Re: [Mesa-dev] [PATCH 7/7] egl: use designated initializers

[Chad Versace]

I was going to write a similar patch series today, until I found
yours :) Thanks for cleaning this up.

By the way, I have some i965 Android logging fixups I need to send to
the list.

With the for-loop conditional fixed, the series 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] anv/allocator: Only write to _vg_ptr if we have valgrind

[Chad Versace]

On Fri 05 May 2017, Jason Ekstrand wrote:
> This fixes the build when not building against valgrind headers.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100945
> ---
>  src/intel/vulkan/anv_allocator.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

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