Re: [Mesa-dev] [PATCH 1/9] i965/gen6+: Invalidate constant cache on brw_emit_mi_flush().

[Kenneth Graunke]

On Tuesday, December 13, 2016 1:10:22 PM PST Francisco Jerez wrote:
> Kenneth Graunke  writes:
> 
> > On Friday, December 9, 2016 11:03:24 AM PST Francisco Jerez wrote:
> >> In order to make sure that the constant cache is coherent with
> >> previous rendering when we start using it for pull constant loads.
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_pipe_control.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.c 
> >> b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> >> index dd426bf..b8f7406 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_pipe_control.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> >> @@ -351,6 +351,7 @@ brw_emit_mi_flush(struct brw_context *brw)
> >>int flags = PIPE_CONTROL_NO_WRITE | 
> >> PIPE_CONTROL_RENDER_TARGET_FLUSH;
> >>if (brw->gen >= 6) {
> >>   flags |= PIPE_CONTROL_INSTRUCTION_INVALIDATE |
> >> +  PIPE_CONTROL_CONST_CACHE_INVALIDATE |
> >>PIPE_CONTROL_DEPTH_CACHE_FLUSH |
> >>PIPE_CONTROL_VF_CACHE_INVALIDATE |
> >>PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
> >> 
> >
> > As we've talked about before...brw_emit_mi_flush() basically means
> > "I didn't actually think about what kind of flushing I need, so I threw
> > random bits at it, and hoped the problems would somehow go away."
> >
> > I guess we may as well add this to the hodgepodge, since we haven't
> > replaced all uses of brw_emit_mi_flush() yet...but I sort of wonder
> > whether it does anything useful.
> >
> > Does anything actually call brw_emit_mi_flush between draws?  (It did
> > two years ago, but you and I changed a number of flushes since then...)
> >
> > The use case I can think of is:
> >
> > 1. App binds a buffer object as an SSBO, atomic counter buffer,
> 
> These (and image stores and atomics) will be handled by
> brw_memory_barrier(), which already takes care of invalidating the
> constant cache.

Oh, right, MemoryBarrier() handles this.  Makes sense.  Sounds like we
don't need any additional flushing then.

> >transform feedback output, pipeline statistics buffer, or whatever
> 
> I'm not convinced that coherency is currently managed correctly for
> these by the driver (e.g. brw_end_transform_feedback() calls
> brw_emit_mi_flush(), and so does hsw_end_transform_feedback(), but only
> *before* writing its result into the XFBO, so it won't necessarily be
> visible to shaders if the XFBO is immediately reused as e.g. UBO).
> Either way the situation should be no worse than using the sampler.

I can totally believe this is broken.  There are some test failures on
Haswell that I can fix with additional flushing.

> >(but not rendering - you can't render to a buffer object).
> 
> You can indirectly :), by e.g. binding the buffer as PBO and calling
> glReadPixels -- Which will call brw_emit_mi_flush() after rendering,
> eventually hitting the constant cache invalidation introduced above.

Bah...can't bind it via a framebuffer, but...yes, there is that :)
Good catch.

I'm fine with this patch.  We should just kill off mi_flush in the
not-too-distant future.


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 1/9] i965/gen6+: Invalidate constant cache on brw_emit_mi_flush().

[Francisco Jerez]

Kenneth Graunke  writes:

> On Friday, December 9, 2016 11:03:24 AM PST Francisco Jerez wrote:
>> In order to make sure that the constant cache is coherent with
>> previous rendering when we start using it for pull constant loads.
>> ---
>>  src/mesa/drivers/dri/i965/brw_pipe_control.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.c 
>> b/src/mesa/drivers/dri/i965/brw_pipe_control.c
>> index dd426bf..b8f7406 100644
>> --- a/src/mesa/drivers/dri/i965/brw_pipe_control.c
>> +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c
>> @@ -351,6 +351,7 @@ brw_emit_mi_flush(struct brw_context *brw)
>>int flags = PIPE_CONTROL_NO_WRITE | PIPE_CONTROL_RENDER_TARGET_FLUSH;
>>if (brw->gen >= 6) {
>>   flags |= PIPE_CONTROL_INSTRUCTION_INVALIDATE |
>> +  PIPE_CONTROL_CONST_CACHE_INVALIDATE |
>>PIPE_CONTROL_DEPTH_CACHE_FLUSH |
>>PIPE_CONTROL_VF_CACHE_INVALIDATE |
>>PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
>> 
>
> As we've talked about before...brw_emit_mi_flush() basically means
> "I didn't actually think about what kind of flushing I need, so I threw
> random bits at it, and hoped the problems would somehow go away."
>
> I guess we may as well add this to the hodgepodge, since we haven't
> replaced all uses of brw_emit_mi_flush() yet...but I sort of wonder
> whether it does anything useful.
>
> Does anything actually call brw_emit_mi_flush between draws?  (It did
> two years ago, but you and I changed a number of flushes since then...)
>
> The use case I can think of is:
>
> 1. App binds a buffer object as an SSBO, atomic counter buffer,

These (and image stores and atomics) will be handled by
brw_memory_barrier(), which already takes care of invalidating the
constant cache.

>transform feedback output, pipeline statistics buffer, or whatever

I'm not convinced that coherency is currently managed correctly for
these by the driver (e.g. brw_end_transform_feedback() calls
brw_emit_mi_flush(), and so does hsw_end_transform_feedback(), but only
*before* writing its result into the XFBO, so it won't necessarily be
visible to shaders if the XFBO is immediately reused as e.g. UBO).
Either way the situation should be no worse than using the sampler.

>(but not rendering - you can't render to a buffer object).

You can indirectly :), by e.g. binding the buffer as PBO and calling
glReadPixels -- Which will call brw_emit_mi_flush() after rendering,
eventually hitting the constant cache invalidation introduced above.

> 2. Drawing or compute dispatch
> 3. App binds that buffer as a UBO.
> 4. More drawing or compute dispatch
>
> It seems like the real solution is to do a CONST_CACHE_INVALIDATE
> when changing UBO bindings.  (That is, assuming the same BO can't
> be bound as a UBO and something else at the same time...)
>
> This isn't a NAK, I'm just wondering if you have any thoughts.


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

Re: [Mesa-dev] [PATCH 1/9] i965/gen6+: Invalidate constant cache on brw_emit_mi_flush().

[Kenneth Graunke]

On Friday, December 9, 2016 11:03:24 AM PST Francisco Jerez wrote:
> In order to make sure that the constant cache is coherent with
> previous rendering when we start using it for pull constant loads.
> ---
>  src/mesa/drivers/dri/i965/brw_pipe_control.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.c 
> b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> index dd426bf..b8f7406 100644
> --- a/src/mesa/drivers/dri/i965/brw_pipe_control.c
> +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> @@ -351,6 +351,7 @@ brw_emit_mi_flush(struct brw_context *brw)
>int flags = PIPE_CONTROL_NO_WRITE | PIPE_CONTROL_RENDER_TARGET_FLUSH;
>if (brw->gen >= 6) {
>   flags |= PIPE_CONTROL_INSTRUCTION_INVALIDATE |
> +  PIPE_CONTROL_CONST_CACHE_INVALIDATE |
>PIPE_CONTROL_DEPTH_CACHE_FLUSH |
>PIPE_CONTROL_VF_CACHE_INVALIDATE |
>PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
> 

As we've talked about before...brw_emit_mi_flush() basically means
"I didn't actually think about what kind of flushing I need, so I threw
random bits at it, and hoped the problems would somehow go away."

I guess we may as well add this to the hodgepodge, since we haven't
replaced all uses of brw_emit_mi_flush() yet...but I sort of wonder
whether it does anything useful.

Does anything actually call brw_emit_mi_flush between draws?  (It did
two years ago, but you and I changed a number of flushes since then...)

The use case I can think of is:

1. App binds a buffer object as an SSBO, atomic counter buffer,
   transform feedback output, pipeline statistics buffer, or whatever
   (but not rendering - you can't render to a buffer object).
2. Drawing or compute dispatch
3. App binds that buffer as a UBO.
4. More drawing or compute dispatch

It seems like the real solution is to do a CONST_CACHE_INVALIDATE
when changing UBO bindings.  (That is, assuming the same BO can't
be bound as a UBO and something else at the same time...)

This isn't a NAK, I'm just wondering if you have any thoughts.


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

[Mesa-dev] [PATCH 1/9] i965/gen6+: Invalidate constant cache on brw_emit_mi_flush().

[Francisco Jerez]

In order to make sure that the constant cache is coherent with
previous rendering when we start using it for pull constant loads.
---
 src/mesa/drivers/dri/i965/brw_pipe_control.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.c 
b/src/mesa/drivers/dri/i965/brw_pipe_control.c
index dd426bf..b8f7406 100644
--- a/src/mesa/drivers/dri/i965/brw_pipe_control.c
+++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c
@@ -351,6 +351,7 @@ brw_emit_mi_flush(struct brw_context *brw)
   int flags = PIPE_CONTROL_NO_WRITE | PIPE_CONTROL_RENDER_TARGET_FLUSH;
   if (brw->gen >= 6) {
  flags |= PIPE_CONTROL_INSTRUCTION_INVALIDATE |
+  PIPE_CONTROL_CONST_CACHE_INVALIDATE |
   PIPE_CONTROL_DEPTH_CACHE_FLUSH |
   PIPE_CONTROL_VF_CACHE_INVALIDATE |
   PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
-- 
2.10.2

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