Re: [Mesa-dev] [PATCH 2/2] i965: Disable auxiliary buffers when there are self-dependencies.

2017-10-06 Thread Jason Ekstrand 
On Fri, Oct 6, 2017 at 11:13 AM, Nanley Chery  wrote:

> On Thu, Oct 05, 2017 at 10:02:40PM -0700, Kenneth Graunke wrote:
> > Jason and I investigated several OpenGL CTS failures where the tests
> > bind the same texture for rendering and texturing, at the same time.
> > This has defined results as long as the reads happen before writes,
> > or the regions are non-overlapping.  Normally, this just works out.
> >
> > However, CCS can cause problems.  If the shader is reading one set of
> > pixels, and writing to different pixels that are adjacent, they may end
> > up being covered by the same CCS block.  So rendering may be writing a
> > CCS block, while the sampler is trying to read it.  Corruption ensues.
> >
> > Disabling CCS is unfortunate, but safe.
> >
> > Fixes several KHR-GL45.texture_barrier.* subtests.
> >
> > Cc: nanleych...@gmail.com
> > Cc: ja...@jlekstrand.net
> > ---
> >  src/mesa/drivers/dri/i965/brw_draw.c  |  8 +++-
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 13 +++--
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  2 +-
> >  3 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> > index cab3758d7b5..c13fa8c367a 100644
> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > @@ -383,7 +383,13 @@ brw_predraw_resolve_inputs(struct brw_context *brw)
> >   translate_tex_format(brw, tex_obj->_Format,
> sampler->sRGBDecode);
> >int drawbuf_idx = get_drawbuffer_index(brw, tex_obj->mt->bo);
> >
> > -  bool aux_supported;
> > +  /* Disable auxiliary buffers if there's a self-dependency, where
> > +   * we're both texturing from and rendering to the same buffer.
> > +   * It's not necessarily safe - concurrent reads and writes to the
> > +   * CCS buffer can result in incorrect pixels.
> > +   */
> > +  bool aux_supported = drawbuf_idx == -1;
> > +
> >intel_miptree_prepare_texture(brw, tex_obj->mt, view_format,
> >  _supported);
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 5b7cde82f65..29b93dd656c 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -2651,9 +2651,10 @@ intel_miptree_prepare_texture_slices(struct
> brw_context *brw,
> >   enum isl_format view_format,
> >   uint32_t start_level, uint32_t
> num_levels,
> >   uint32_t start_layer, uint32_t
> num_layers,
> > - bool *aux_supported_out)
> > + bool *aux_supported)
>
> I find the name of this new parameter a little misleading. It suggests
> that the caller can determine that the aux surface is usable during the
> texture operation, when in fact it is the
> intel_miptree_texture_aux_usage function in the callee which does this
> determination. Since the caller can determine that the aux surface
> cannot be used, perhaps something like disable_aux would be a better
> fit?
>

I was thinking the same thing.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] i965: Disable auxiliary buffers when there are self-dependencies.

2017-10-06 Thread Nanley Chery 
On Thu, Oct 05, 2017 at 10:02:40PM -0700, Kenneth Graunke wrote:
> Jason and I investigated several OpenGL CTS failures where the tests
> bind the same texture for rendering and texturing, at the same time.
> This has defined results as long as the reads happen before writes,
> or the regions are non-overlapping.  Normally, this just works out.
> 
> However, CCS can cause problems.  If the shader is reading one set of
> pixels, and writing to different pixels that are adjacent, they may end
> up being covered by the same CCS block.  So rendering may be writing a
> CCS block, while the sampler is trying to read it.  Corruption ensues.
> 
> Disabling CCS is unfortunate, but safe.
> 
> Fixes several KHR-GL45.texture_barrier.* subtests.
> 
> Cc: nanleych...@gmail.com
> Cc: ja...@jlekstrand.net
> ---
>  src/mesa/drivers/dri/i965/brw_draw.c  |  8 +++-
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 13 +++--
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  2 +-
>  3 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c 
> b/src/mesa/drivers/dri/i965/brw_draw.c
> index cab3758d7b5..c13fa8c367a 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> @@ -383,7 +383,13 @@ brw_predraw_resolve_inputs(struct brw_context *brw)
>   translate_tex_format(brw, tex_obj->_Format, sampler->sRGBDecode);
>int drawbuf_idx = get_drawbuffer_index(brw, tex_obj->mt->bo);
>  
> -  bool aux_supported;
> +  /* Disable auxiliary buffers if there's a self-dependency, where
> +   * we're both texturing from and rendering to the same buffer.
> +   * It's not necessarily safe - concurrent reads and writes to the
> +   * CCS buffer can result in incorrect pixels.
> +   */
> +  bool aux_supported = drawbuf_idx == -1;
> +
>intel_miptree_prepare_texture(brw, tex_obj->mt, view_format,
>  _supported);
>  
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 5b7cde82f65..29b93dd656c 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -2651,9 +2651,10 @@ intel_miptree_prepare_texture_slices(struct 
> brw_context *brw,
>   enum isl_format view_format,
>   uint32_t start_level, uint32_t 
> num_levels,
>   uint32_t start_layer, uint32_t 
> num_layers,
> - bool *aux_supported_out)
> + bool *aux_supported)

I find the name of this new parameter a little misleading. It suggests
that the caller can determine that the aux surface is usable during the
texture operation, when in fact it is the
intel_miptree_texture_aux_usage function in the callee which does this
determination. Since the caller can determine that the aux surface
cannot be used, perhaps something like disable_aux would be a better
fit?

>  {
> -   enum isl_aux_usage aux_usage =
> +   enum isl_aux_usage aux_usage = aux_supported && !*aux_supported ?
> +  ISL_AUX_USAGE_NONE :
>intel_miptree_texture_aux_usage(brw, mt, view_format);
> bool clear_supported = aux_usage != ISL_AUX_USAGE_NONE;
>  
> @@ -2667,20 +2668,20 @@ intel_miptree_prepare_texture_slices(struct 
> brw_context *brw,
> intel_miptree_prepare_access(brw, mt, start_level, num_levels,
>  start_layer, num_layers,
>  aux_usage, clear_supported);
> -   if (aux_supported_out)
> -  *aux_supported_out = aux_usage != ISL_AUX_USAGE_NONE;
> +   if (aux_supported)
> +  *aux_supported = aux_usage != ISL_AUX_USAGE_NONE;
>  }
>  
>  void
>  intel_miptree_prepare_texture(struct brw_context *brw,
>struct intel_mipmap_tree *mt,
>enum isl_format view_format,
> -  bool *aux_supported_out)
> +  bool *aux_supported)
>  {
> intel_miptree_prepare_texture_slices(brw, mt, view_format,
>  0, INTEL_REMAINING_LEVELS,
>  0, INTEL_REMAINING_LAYERS,
> -aux_supported_out);
> +aux_supported);
>  }
>  
>  void
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h 
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> index 2fce28c524b..0da7fafe601 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -640,7 +640,7 @@ void
>  intel_miptree_prepare_texture(struct brw_context *brw,
>struct intel_mipmap_tree *mt,
>enum isl_format view_format,
> -

Re: [Mesa-dev] [PATCH 2/2] i965: Disable auxiliary buffers when there are self-dependencies.

2017-10-05 Thread Jason Ekstrand 

On October 5, 2017 10:02:58 PM Kenneth Graunke  wrote:


Jason and I investigated several OpenGL CTS failures where the tests
bind the same texture for rendering and texturing, at the same time.
This has defined results as long as the reads happen before writes,
or the regions are non-overlapping.  Normally, this just works out.

However, CCS can cause problems.  If the shader is reading one set of
pixels, and writing to different pixels that are adjacent, they may end
up being covered by the same CCS block.  So rendering may be writing a
CCS block, while the sampler is trying to read it.  Corruption ensues.

Disabling CCS is unfortunate, but safe.

Fixes several KHR-GL45.texture_barrier.* subtests.

Cc: nanleych...@gmail.com
Cc: ja...@jlekstrand.net
---
 src/mesa/drivers/dri/i965/brw_draw.c  |  8 +++-
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 13 +++--
 src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  2 +-
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_draw.c 
b/src/mesa/drivers/dri/i965/brw_draw.c

index cab3758d7b5..c13fa8c367a 100644
--- a/src/mesa/drivers/dri/i965/brw_draw.c
+++ b/src/mesa/drivers/dri/i965/brw_draw.c
@@ -383,7 +383,13 @@ brw_predraw_resolve_inputs(struct brw_context *brw)
  translate_tex_format(brw, tex_obj->_Format, sampler->sRGBDecode);
   int drawbuf_idx = get_drawbuffer_index(brw, tex_obj->mt->bo);

-  bool aux_supported;
+  /* Disable auxiliary buffers if there's a self-dependency, where
+   * we're both texturing from and rendering to the same buffer.
+   * It's not necessarily safe - concurrent reads and writes to the
+   * CCS buffer can result in incorrect pixels.
+   */
+  bool aux_supported = drawbuf_idx == -1;
+
   intel_miptree_prepare_texture(brw, tex_obj->mt, view_format,
 _supported);

diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c

index 5b7cde82f65..29b93dd656c 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -2651,9 +2651,10 @@ intel_miptree_prepare_texture_slices(struct 
brw_context *brw,

  enum isl_format view_format,
  uint32_t start_level, uint32_t num_levels,
  uint32_t start_layer, uint32_t num_layers,
- bool *aux_supported_out)
+ bool *aux_supported)


Now that this is an input parameter, there's no reason for it to be a pointer.


 {
-   enum isl_aux_usage aux_usage =
+   enum isl_aux_usage aux_usage = aux_supported && !*aux_supported ?
+  ISL_AUX_USAGE_NONE :
   intel_miptree_texture_aux_usage(brw, mt, view_format);
bool clear_supported = aux_usage != ISL_AUX_USAGE_NONE;

@@ -2667,20 +2668,20 @@ intel_miptree_prepare_texture_slices(struct 
brw_context *brw,

intel_miptree_prepare_access(brw, mt, start_level, num_levels,
 start_layer, num_layers,
 aux_usage, clear_supported);
-   if (aux_supported_out)
-  *aux_supported_out = aux_usage != ISL_AUX_USAGE_NONE;
+   if (aux_supported)
+  *aux_supported = aux_usage != ISL_AUX_USAGE_NONE;
 }

 void
 intel_miptree_prepare_texture(struct brw_context *brw,
   struct intel_mipmap_tree *mt,
   enum isl_format view_format,
-  bool *aux_supported_out)
+  bool *aux_supported)
 {
intel_miptree_prepare_texture_slices(brw, mt, view_format,
 0, INTEL_REMAINING_LEVELS,
 0, INTEL_REMAINING_LAYERS,
-aux_supported_out);
+aux_supported);
 }

 void
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h 
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h

index 2fce28c524b..0da7fafe601 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
@@ -640,7 +640,7 @@ void
 intel_miptree_prepare_texture(struct brw_context *brw,
   struct intel_mipmap_tree *mt,
   enum isl_format view_format,
-  bool *aux_supported_out);
+  bool *aux_supported);
 void
 intel_miptree_prepare_image(struct brw_context *brw,
 struct intel_mipmap_tree *mt);
--
2.14.2




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

[Mesa-dev] [PATCH 2/2] i965: Disable auxiliary buffers when there are self-dependencies.

2017-10-05 Thread Kenneth Graunke 
Jason and I investigated several OpenGL CTS failures where the tests
bind the same texture for rendering and texturing, at the same time.
This has defined results as long as the reads happen before writes,
or the regions are non-overlapping.  Normally, this just works out.

However, CCS can cause problems.  If the shader is reading one set of
pixels, and writing to different pixels that are adjacent, they may end
up being covered by the same CCS block.  So rendering may be writing a
CCS block, while the sampler is trying to read it.  Corruption ensues.

Disabling CCS is unfortunate, but safe.

Fixes several KHR-GL45.texture_barrier.* subtests.

Cc: nanleych...@gmail.com
Cc: ja...@jlekstrand.net
---
 src/mesa/drivers/dri/i965/brw_draw.c  |  8 +++-
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 13 +++--
 src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  2 +-
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_draw.c 
b/src/mesa/drivers/dri/i965/brw_draw.c
index cab3758d7b5..c13fa8c367a 100644
--- a/src/mesa/drivers/dri/i965/brw_draw.c
+++ b/src/mesa/drivers/dri/i965/brw_draw.c
@@ -383,7 +383,13 @@ brw_predraw_resolve_inputs(struct brw_context *brw)
  translate_tex_format(brw, tex_obj->_Format, sampler->sRGBDecode);
   int drawbuf_idx = get_drawbuffer_index(brw, tex_obj->mt->bo);
 
-  bool aux_supported;
+  /* Disable auxiliary buffers if there's a self-dependency, where
+   * we're both texturing from and rendering to the same buffer.
+   * It's not necessarily safe - concurrent reads and writes to the
+   * CCS buffer can result in incorrect pixels.
+   */
+  bool aux_supported = drawbuf_idx == -1;
+
   intel_miptree_prepare_texture(brw, tex_obj->mt, view_format,
 _supported);
 
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 5b7cde82f65..29b93dd656c 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -2651,9 +2651,10 @@ intel_miptree_prepare_texture_slices(struct brw_context 
*brw,
  enum isl_format view_format,
  uint32_t start_level, uint32_t num_levels,
  uint32_t start_layer, uint32_t num_layers,
- bool *aux_supported_out)
+ bool *aux_supported)
 {
-   enum isl_aux_usage aux_usage =
+   enum isl_aux_usage aux_usage = aux_supported && !*aux_supported ?
+  ISL_AUX_USAGE_NONE :
   intel_miptree_texture_aux_usage(brw, mt, view_format);
bool clear_supported = aux_usage != ISL_AUX_USAGE_NONE;
 
@@ -2667,20 +2668,20 @@ intel_miptree_prepare_texture_slices(struct brw_context 
*brw,
intel_miptree_prepare_access(brw, mt, start_level, num_levels,
 start_layer, num_layers,
 aux_usage, clear_supported);
-   if (aux_supported_out)
-  *aux_supported_out = aux_usage != ISL_AUX_USAGE_NONE;
+   if (aux_supported)
+  *aux_supported = aux_usage != ISL_AUX_USAGE_NONE;
 }
 
 void
 intel_miptree_prepare_texture(struct brw_context *brw,
   struct intel_mipmap_tree *mt,
   enum isl_format view_format,
-  bool *aux_supported_out)
+  bool *aux_supported)
 {
intel_miptree_prepare_texture_slices(brw, mt, view_format,
 0, INTEL_REMAINING_LEVELS,
 0, INTEL_REMAINING_LAYERS,
-aux_supported_out);
+aux_supported);
 }
 
 void
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h 
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
index 2fce28c524b..0da7fafe601 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
@@ -640,7 +640,7 @@ void
 intel_miptree_prepare_texture(struct brw_context *brw,
   struct intel_mipmap_tree *mt,
   enum isl_format view_format,
-  bool *aux_supported_out);
+  bool *aux_supported);
 void
 intel_miptree_prepare_image(struct brw_context *brw,
 struct intel_mipmap_tree *mt);
-- 
2.14.2

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