Re: [Mesa-dev] [PATCH v2] i965: Disable auxiliary buffers when there are self-dependencies.
On Friday, October 6, 2017 10:25:50 PM PDT Kenneth Graunke wrote:
> On Friday, October 6, 2017 8:09:33 PM PDT Jason Ekstrand wrote:
> > On October 6, 2017 8:00:04 PM Kenneth Graunke wrote:
[snip]
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index 670a92c1168..48392e7494a 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -2678,7 +2676,7 @@ intel_miptree_prepare_fb_fetch(struct brw_context
> > > *brw,
> > > uint32_t start_layer, uint32_t num_layers)
> > > {
> > > intel_miptree_prepare_texture_slices(brw, mt, mt->surf.format, level,
> > > 1,
> > > -start_layer, num_layers, NULL);
> > > +start_layer, num_layers, false);
> >
> > I think we probably want true here. It is for fb_fetch after all. :-)
> > Also, we probably want to do disable_rb_aux_buffer for framebuffer fetch as
> > well.
>
> Sure, I can do that. It won't actually do anything, however, as this is
> the code for non-coherent framebuffer fetch...and on Gen9+, we always do
> a coherent fetch using the Render Target Read messages (even if the app
> says that a non-coherent fetch would be fine). And, Gen9+ is the only
> platform where CCS_E exists. So, the only platforms where this matters
> don't use this code :)
>
> Still, I think your suggestion would make the code clearer, and would
> future-proof it in case we ever decide that non-coherent fetches are
> useful on modern hardware (say, if they're cheaper someday).
Actually, now that I've tried it, this doesn't work.
It causes assertion failures in tests such as:
ES31-CTS.functional.blend_equation_advanced.msaa.lighten
on Broadwell and older. These try to do non-coherent framebuffer fetch
from a multisampled surface. Making intel_miptree_prepare_fb_access
pass true for "disable_aux" causes us to pass ISL_AUX_USAGE_NONE into
intel_miptree_prepare_mcs_access().
That function asserts aux_usage is ISL_AUX_USAGE_MCS, because, as you
said, there's no code for doing full MCS resolves.
Because of that, and this probably not doing anything useful anyway,
I think I'd rather leave v2 as it is. What do you think, Jason?
signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] i965: Disable auxiliary buffers when there are self-dependencies.
On Sat, Oct 7, 2017 at 11:42 AM, Kenneth Graunke
wrote:
> On Saturday, October 7, 2017 8:03:54 AM PDT Jason Ekstrand wrote:
> > On October 6, 2017 10:25:55 PM Kenneth Graunke
> wrote:
> >
> > > On Friday, October 6, 2017 8:09:33 PM PDT Jason Ekstrand wrote:
> > >> On October 6, 2017 8:00:04 PM Kenneth Graunke
> wrote:
> > >> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> > >> > b/src/mesa/drivers/dri/i965/brw_draw.c
> > >> > index c7ed7284501..fcb194dbe86 100644
> > >> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > >> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > >> > @@ -336,21 +336,39 @@ brw_merge_inputs(struct brw_context *brw,
> > >> > }
> > >> > }
> > >> >
> > >> > +/* Disable auxiliary buffers if a renderbuffer is also bound as a
> texture
> > >> > + * or shader image. This causes a self-dependency, where both
> rendering
> > >> > + * and sampling may concurrently read or write the CCS buffer,
> causing
> > >> > + * incorrect pixels.
> > >> > + *
> > >> > + * We don't support sampling from CCS_D, so this only matters for
> CCS_E.
> > >> > + */
> > >> > static bool
> > >> > -intel_disable_rb_aux_buffer(struct brw_context *brw, const struct
> brw_bo
> > >> *bo)
> > >> > +intel_disable_rb_aux_buffer(struct brw_context *brw,
> > >> > +struct intel_mipmap_tree *tex_mt,
> > >> > +const char *usage)
> > >> > {
> > >> > const struct gl_framebuffer *fb = brw->ctx.DrawBuffer;
> > >> > bool found = false;
> > >> >
> > >> > + /* Nothing to disable, don't bother looking */
> > >> > + if (tex_mt->aux_usage != ISL_AUX_USAGE_CCS_E)
> > >> > + return false;
> > >>
> > >> As I mentioned in the office today, I'm not convinced this is actually
> > >> needed. When it isn't CCS_E, we'll resolve anyway so passing
> disable_aux =
> > >> true won't hurt anything.
> > >
> > > Yeah, that's true - but I figured we could avoid the overhead of the
> > > loop in other cases, since it doesn't really matter either way.
> > >
> > > Make sense? Should we leave it? Or would you rather drop it?
> >
> > Leave it in but for a completely different reason than the one you
> stated.
> > :-) I don't care all that much about trying to avoid a loop whose number
> > of iterations was bounded above by 8.
>
> Well, it is nested inside a loop bounded by 32... :)
>
Ok, fair enough. But intel_miptree_prepare can do some nasty looping too.
In any case checking for certain aux_usages is a better plan than I thought.
> > If the have a multisampled self dependency, it should work fine because
> > compression is limited to a single pixel and we dispatch all the samples
> > for a given pixel together.
>
> Probably not for 16x MSAA and SIMD8 shaders, but...it's probably really
> uncommon...
>
And given that we don't have MSAA resolve code, it's probably a good idea
to not ask for a full resolve. That would end badly. :-)
> > We shouldn't force a resolve so we don't want
> > to do this of its MCS. In the case of CCS_D, I'm not sure if it's safe.
> > We will potentially try to sample from a CCS_D image as CCS_E on gen9+ so
> > it's probably best to disable it for that too.
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] i965: Disable auxiliary buffers when there are self-dependencies.
On Saturday, October 7, 2017 8:03:54 AM PDT Jason Ekstrand wrote:
> On October 6, 2017 10:25:55 PM Kenneth Graunke wrote:
>
> > On Friday, October 6, 2017 8:09:33 PM PDT Jason Ekstrand wrote:
> >> On October 6, 2017 8:00:04 PM Kenneth Graunke
> >> wrote:
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> >> > b/src/mesa/drivers/dri/i965/brw_draw.c
> >> > index c7ed7284501..fcb194dbe86 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> >> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> >> > @@ -336,21 +336,39 @@ brw_merge_inputs(struct brw_context *brw,
> >> > }
> >> > }
> >> >
> >> > +/* Disable auxiliary buffers if a renderbuffer is also bound as a
> >> > texture
> >> > + * or shader image. This causes a self-dependency, where both rendering
> >> > + * and sampling may concurrently read or write the CCS buffer, causing
> >> > + * incorrect pixels.
> >> > + *
> >> > + * We don't support sampling from CCS_D, so this only matters for CCS_E.
> >> > + */
> >> > static bool
> >> > -intel_disable_rb_aux_buffer(struct brw_context *brw, const struct
> >> > brw_bo
> >> *bo)
> >> > +intel_disable_rb_aux_buffer(struct brw_context *brw,
> >> > +struct intel_mipmap_tree *tex_mt,
> >> > +const char *usage)
> >> > {
> >> > const struct gl_framebuffer *fb = brw->ctx.DrawBuffer;
> >> > bool found = false;
> >> >
> >> > + /* Nothing to disable, don't bother looking */
> >> > + if (tex_mt->aux_usage != ISL_AUX_USAGE_CCS_E)
> >> > + return false;
> >>
> >> As I mentioned in the office today, I'm not convinced this is actually
> >> needed. When it isn't CCS_E, we'll resolve anyway so passing disable_aux =
> >> true won't hurt anything.
> >
> > Yeah, that's true - but I figured we could avoid the overhead of the
> > loop in other cases, since it doesn't really matter either way.
> >
> > Make sense? Should we leave it? Or would you rather drop it?
>
> Leave it in but for a completely different reason than the one you stated.
> :-) I don't care all that much about trying to avoid a loop whose number
> of iterations was bounded above by 8.
Well, it is nested inside a loop bounded by 32... :)
> If the have a multisampled self dependency, it should work fine because
> compression is limited to a single pixel and we dispatch all the samples
> for a given pixel together.
Probably not for 16x MSAA and SIMD8 shaders, but...it's probably really
uncommon...
> We shouldn't force a resolve so we don't want
> to do this of its MCS. In the case of CCS_D, I'm not sure if it's safe.
> We will potentially try to sample from a CCS_D image as CCS_E on gen9+ so
> it's probably best to disable it for that too.
signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] i965: Disable auxiliary buffers when there are self-dependencies.
On October 6, 2017 10:25:55 PM Kenneth Graunke wrote:
On Friday, October 6, 2017 8:09:33 PM PDT Jason Ekstrand wrote:
On October 6, 2017 8:00:04 PM Kenneth Graunke wrote:
> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> index c7ed7284501..fcb194dbe86 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> @@ -336,21 +336,39 @@ brw_merge_inputs(struct brw_context *brw,
> }
> }
>
> +/* Disable auxiliary buffers if a renderbuffer is also bound as a texture
> + * or shader image. This causes a self-dependency, where both rendering
> + * and sampling may concurrently read or write the CCS buffer, causing
> + * incorrect pixels.
> + *
> + * We don't support sampling from CCS_D, so this only matters for CCS_E.
> + */
> static bool
> -intel_disable_rb_aux_buffer(struct brw_context *brw, const struct brw_bo
*bo)
> +intel_disable_rb_aux_buffer(struct brw_context *brw,
> +struct intel_mipmap_tree *tex_mt,
> +const char *usage)
> {
> const struct gl_framebuffer *fb = brw->ctx.DrawBuffer;
> bool found = false;
>
> + /* Nothing to disable, don't bother looking */
> + if (tex_mt->aux_usage != ISL_AUX_USAGE_CCS_E)
> + return false;
As I mentioned in the office today, I'm not convinced this is actually
needed. When it isn't CCS_E, we'll resolve anyway so passing disable_aux =
true won't hurt anything.
Yeah, that's true - but I figured we could avoid the overhead of the
loop in other cases, since it doesn't really matter either way.
Make sense? Should we leave it? Or would you rather drop it?
Leave it in but for a completely different reason than the one you stated.
:-) I don't care all that much about trying to avoid a loop whose number
of iterations was bounded above by 8.
If the have a multisampled self dependency, it should work fine because
compression is limited to a single pixel and we dispatch all the samples
for a given pixel together. We shouldn't force a resolve so we don't want
to do this of its MCS. In the case of CCS_D, I'm not sure if it's safe.
We will potentially try to sample from a CCS_D image as CCS_E on gen9+ so
it's probably best to disable it for that too.
> +
> for (unsigned i = 0; i < fb->_NumColorDrawBuffers; i++) {
>const struct intel_renderbuffer *irb =
> intel_renderbuffer(fb->_ColorDrawBuffers[i]);
>
> - if (irb && irb->mt->bo == bo) {
> + if (irb && irb->mt->bo == tex_mt->bo) {
> found = brw->draw_aux_buffer_disabled[i] = true;
>}
> }
>
> + if (found) {
> + perf_debug("Disabling CCS because a renderbuffer is also bound %s.\n",
> + usage);
> + }
> +
> return found;
> }
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 670a92c1168..48392e7494a 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -2678,7 +2676,7 @@ intel_miptree_prepare_fb_fetch(struct brw_context *brw,
> uint32_t start_layer, uint32_t num_layers)
> {
> intel_miptree_prepare_texture_slices(brw, mt, mt->surf.format, level, 1,
> -start_layer, num_layers, NULL);
> +start_layer, num_layers, false);
I think we probably want true here. It is for fb_fetch after all. :-)
Also, we probably want to do disable_rb_aux_buffer for framebuffer fetch as
well.
Sure, I can do that. It won't actually do anything, however, as this is
the code for non-coherent framebuffer fetch...and on Gen9+, we always do
a coherent fetch using the Render Target Read messages (even if the app
says that a non-coherent fetch would be fine). And, Gen9+ is the only
platform where CCS_E exists. So, the only platforms where this matters
don't use this code :)
Still, I think your suggestion would make the code clearer, and would
future-proof it in case we ever decide that non-coherent fetches are
useful on modern hardware (say, if they're cheaper someday).
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] i965: Disable auxiliary buffers when there are self-dependencies.
On Friday, October 6, 2017 8:09:33 PM PDT Jason Ekstrand wrote:
> On October 6, 2017 8:00:04 PM Kenneth Graunke wrote:
> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> > b/src/mesa/drivers/dri/i965/brw_draw.c
> > index c7ed7284501..fcb194dbe86 100644
> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > @@ -336,21 +336,39 @@ brw_merge_inputs(struct brw_context *brw,
> > }
> > }
> >
> > +/* Disable auxiliary buffers if a renderbuffer is also bound as a texture
> > + * or shader image. This causes a self-dependency, where both rendering
> > + * and sampling may concurrently read or write the CCS buffer, causing
> > + * incorrect pixels.
> > + *
> > + * We don't support sampling from CCS_D, so this only matters for CCS_E.
> > + */
> > static bool
> > -intel_disable_rb_aux_buffer(struct brw_context *brw, const struct brw_bo
> > *bo)
> > +intel_disable_rb_aux_buffer(struct brw_context *brw,
> > +struct intel_mipmap_tree *tex_mt,
> > +const char *usage)
> > {
> > const struct gl_framebuffer *fb = brw->ctx.DrawBuffer;
> > bool found = false;
> >
> > + /* Nothing to disable, don't bother looking */
> > + if (tex_mt->aux_usage != ISL_AUX_USAGE_CCS_E)
> > + return false;
>
> As I mentioned in the office today, I'm not convinced this is actually
> needed. When it isn't CCS_E, we'll resolve anyway so passing disable_aux =
> true won't hurt anything.
Yeah, that's true - but I figured we could avoid the overhead of the
loop in other cases, since it doesn't really matter either way.
Make sense? Should we leave it? Or would you rather drop it?
> > +
> > for (unsigned i = 0; i < fb->_NumColorDrawBuffers; i++) {
> >const struct intel_renderbuffer *irb =
> > intel_renderbuffer(fb->_ColorDrawBuffers[i]);
> >
> > - if (irb && irb->mt->bo == bo) {
> > + if (irb && irb->mt->bo == tex_mt->bo) {
> > found = brw->draw_aux_buffer_disabled[i] = true;
> >}
> > }
> >
> > + if (found) {
> > + perf_debug("Disabling CCS because a renderbuffer is also bound
> > %s.\n",
> > + usage);
> > + }
> > +
> > return found;
> > }
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 670a92c1168..48392e7494a 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -2678,7 +2676,7 @@ intel_miptree_prepare_fb_fetch(struct brw_context
> > *brw,
> > uint32_t start_layer, uint32_t num_layers)
> > {
> > intel_miptree_prepare_texture_slices(brw, mt, mt->surf.format, level, 1,
> > -start_layer, num_layers, NULL);
> > +start_layer, num_layers, false);
>
> I think we probably want true here. It is for fb_fetch after all. :-)
> Also, we probably want to do disable_rb_aux_buffer for framebuffer fetch as
> well.
Sure, I can do that. It won't actually do anything, however, as this is
the code for non-coherent framebuffer fetch...and on Gen9+, we always do
a coherent fetch using the Render Target Read messages (even if the app
says that a non-coherent fetch would be fine). And, Gen9+ is the only
platform where CCS_E exists. So, the only platforms where this matters
don't use this code :)
Still, I think your suggestion would make the code clearer, and would
future-proof it in case we ever decide that non-coherent fetches are
useful on modern hardware (say, if they're cheaper someday).
signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] i965: Disable auxiliary buffers when there are self-dependencies.
On October 6, 2017 8:00:04 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 | 44 ---
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 12 +++-
src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 2 +-
3 files changed, 33 insertions(+), 25 deletions(-)
Only one patch this time around. This is a lot nicer.
diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
b/src/mesa/drivers/dri/i965/brw_draw.c
index c7ed7284501..fcb194dbe86 100644
--- a/src/mesa/drivers/dri/i965/brw_draw.c
+++ b/src/mesa/drivers/dri/i965/brw_draw.c
@@ -336,21 +336,39 @@ brw_merge_inputs(struct brw_context *brw,
}
}
+/* Disable auxiliary buffers if a renderbuffer is also bound as a texture
+ * or shader image. This causes a self-dependency, where both rendering
+ * and sampling may concurrently read or write the CCS buffer, causing
+ * incorrect pixels.
+ *
+ * We don't support sampling from CCS_D, so this only matters for CCS_E.
+ */
static bool
-intel_disable_rb_aux_buffer(struct brw_context *brw, const struct brw_bo *bo)
+intel_disable_rb_aux_buffer(struct brw_context *brw,
+struct intel_mipmap_tree *tex_mt,
+const char *usage)
{
const struct gl_framebuffer *fb = brw->ctx.DrawBuffer;
bool found = false;
+ /* Nothing to disable, don't bother looking */
+ if (tex_mt->aux_usage != ISL_AUX_USAGE_CCS_E)
+ return false;
As I mentioned in the office today, I'm not convinced this is actually
needed. When it isn't CCS_E, we'll resolve anyway so passing disable_aux =
true won't hurt anything.
+
for (unsigned i = 0; i < fb->_NumColorDrawBuffers; i++) {
const struct intel_renderbuffer *irb =
intel_renderbuffer(fb->_ColorDrawBuffers[i]);
- if (irb && irb->mt->bo == bo) {
+ if (irb && irb->mt->bo == tex_mt->bo) {
found = brw->draw_aux_buffer_disabled[i] = true;
}
}
+ if (found) {
+ perf_debug("Disabling CCS because a renderbuffer is also bound %s.\n",
+ usage);
+ }
+
return found;
}
@@ -363,7 +381,6 @@ intel_disable_rb_aux_buffer(struct brw_context *brw,
const struct brw_bo *bo)
void
brw_predraw_resolve_inputs(struct brw_context *brw)
{
- const struct gen_device_info *devinfo = &brw->screen->devinfo;
struct gl_context *ctx = &brw->ctx;
struct intel_texture_object *tex_obj;
@@ -383,15 +400,11 @@ brw_predraw_resolve_inputs(struct brw_context *brw)
enum isl_format view_format =
translate_tex_format(brw, tex_obj->_Format, sampler->sRGBDecode);
- bool aux_supported;
- intel_miptree_prepare_texture(brw, tex_obj->mt, view_format,
-&aux_supported);
+ const bool disable_aux =
+ intel_disable_rb_aux_buffer(brw, tex_obj->mt, "for sampling");
- if (!aux_supported && devinfo->gen >= 9 &&
- intel_disable_rb_aux_buffer(brw, tex_obj->mt->bo)) {
- perf_debug("Sampling renderbuffer with non-compressible format - "
-"turning off compression\n");
- }
+ intel_miptree_prepare_texture(brw, tex_obj->mt, view_format,
+disable_aux);
brw_render_cache_set_check_flush(brw, tex_obj->mt->bo);
@@ -412,13 +425,10 @@ brw_predraw_resolve_inputs(struct brw_context *brw)
tex_obj = intel_texture_object(u->TexObj);
if (tex_obj && tex_obj->mt) {
- intel_miptree_prepare_image(brw, tex_obj->mt);
+ intel_disable_rb_aux_buffer(brw, tex_obj->mt,
+ "as a shader image");
- if (tex_obj->mt->aux_usage == ISL_AUX_USAGE_CCS_E &&
- intel_disable_rb_aux_buffer(brw, tex_obj->mt->bo)) {
- perf_debug("Using renderbuffer as shader image - turning "
- "off lossless compression\n");
- }
+ intel_miptree_prepare_image(brw, tex_obj->mt);
brw_render_cache_set_check_flush(brw, tex_obj->mt->bo);
}
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 670a92c1168..48392e7494a 100644
--- a/src/mesa/drivers/dri/i965/inte
[Mesa-dev] [PATCH v2] i965: Disable auxiliary buffers when there are self-dependencies.
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 | 44 ---
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 12 +++-
src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 2 +-
3 files changed, 33 insertions(+), 25 deletions(-)
Only one patch this time around. This is a lot nicer.
diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
b/src/mesa/drivers/dri/i965/brw_draw.c
index c7ed7284501..fcb194dbe86 100644
--- a/src/mesa/drivers/dri/i965/brw_draw.c
+++ b/src/mesa/drivers/dri/i965/brw_draw.c
@@ -336,21 +336,39 @@ brw_merge_inputs(struct brw_context *brw,
}
}
+/* Disable auxiliary buffers if a renderbuffer is also bound as a texture
+ * or shader image. This causes a self-dependency, where both rendering
+ * and sampling may concurrently read or write the CCS buffer, causing
+ * incorrect pixels.
+ *
+ * We don't support sampling from CCS_D, so this only matters for CCS_E.
+ */
static bool
-intel_disable_rb_aux_buffer(struct brw_context *brw, const struct brw_bo *bo)
+intel_disable_rb_aux_buffer(struct brw_context *brw,
+struct intel_mipmap_tree *tex_mt,
+const char *usage)
{
const struct gl_framebuffer *fb = brw->ctx.DrawBuffer;
bool found = false;
+ /* Nothing to disable, don't bother looking */
+ if (tex_mt->aux_usage != ISL_AUX_USAGE_CCS_E)
+ return false;
+
for (unsigned i = 0; i < fb->_NumColorDrawBuffers; i++) {
const struct intel_renderbuffer *irb =
intel_renderbuffer(fb->_ColorDrawBuffers[i]);
- if (irb && irb->mt->bo == bo) {
+ if (irb && irb->mt->bo == tex_mt->bo) {
found = brw->draw_aux_buffer_disabled[i] = true;
}
}
+ if (found) {
+ perf_debug("Disabling CCS because a renderbuffer is also bound %s.\n",
+ usage);
+ }
+
return found;
}
@@ -363,7 +381,6 @@ intel_disable_rb_aux_buffer(struct brw_context *brw, const
struct brw_bo *bo)
void
brw_predraw_resolve_inputs(struct brw_context *brw)
{
- const struct gen_device_info *devinfo = &brw->screen->devinfo;
struct gl_context *ctx = &brw->ctx;
struct intel_texture_object *tex_obj;
@@ -383,15 +400,11 @@ brw_predraw_resolve_inputs(struct brw_context *brw)
enum isl_format view_format =
translate_tex_format(brw, tex_obj->_Format, sampler->sRGBDecode);
- bool aux_supported;
- intel_miptree_prepare_texture(brw, tex_obj->mt, view_format,
-&aux_supported);
+ const bool disable_aux =
+ intel_disable_rb_aux_buffer(brw, tex_obj->mt, "for sampling");
- if (!aux_supported && devinfo->gen >= 9 &&
- intel_disable_rb_aux_buffer(brw, tex_obj->mt->bo)) {
- perf_debug("Sampling renderbuffer with non-compressible format - "
-"turning off compression\n");
- }
+ intel_miptree_prepare_texture(brw, tex_obj->mt, view_format,
+disable_aux);
brw_render_cache_set_check_flush(brw, tex_obj->mt->bo);
@@ -412,13 +425,10 @@ brw_predraw_resolve_inputs(struct brw_context *brw)
tex_obj = intel_texture_object(u->TexObj);
if (tex_obj && tex_obj->mt) {
- intel_miptree_prepare_image(brw, tex_obj->mt);
+ intel_disable_rb_aux_buffer(brw, tex_obj->mt,
+ "as a shader image");
- if (tex_obj->mt->aux_usage == ISL_AUX_USAGE_CCS_E &&
- intel_disable_rb_aux_buffer(brw, tex_obj->mt->bo)) {
- perf_debug("Using renderbuffer as shader image - turning "
- "off lossless compression\n");
- }
+ intel_miptree_prepare_image(brw, tex_obj->mt);
brw_render_cache_set_check_flush(brw, tex_obj->mt->bo);
}
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 670a92c1168..48392e7494a 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -2630,9 +2630,9 @@ intel_miptree_prepare_texture_slices(struct brw_context
*brw,
enum isl_format view_format,
