Re: [Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround

[Pohjolainen, Topi]

On Mon, Dec 04, 2017 at 09:01:44AM +, Rogovin, Kevin wrote:
> Hi,
> 
> > 1) do extra tex cache flush when needed and specifically only when needed
> > 2) resolve surfaces when undesired combination is about to be sampled
> 
> >Latter case could be addressed also with on-the-fly check in 
> >brw_predraw_resolve_inputs(). There one goes thru all the active textures 
> >for the
> > next draw call and resolves when necessary. One could check for the 
> > undesired combination of textures there. I understand we would need to
> > walk the textures twice, first to check for any occurrences of one type and 
> > then for the other. This, however, would fit to the way we resolve
> > other texture types and prevent from adding more things to check into the 
> > context.
> 
> This patch series does handle the second case, as follows:
>  a) Checking if there are astc5x5 textures and/or textures with auxiliary is 
> done in brw_validate_textures(); this was a really nice place because
>   the function loops over all textures anyways
>  b) the resolve operation is handled by the added function 
> brw_atsc_perform_wa(); this also sets the mode correctly too after the 
> resolve.
> 
> I chose to make a dedicated function that does the mode setting and resolve.  
> Putting the resolve logic into brw_predraw_resolve_inputs()
> is not a bad idea and I am open to it; the interior of the loop would then 
> look ickier as it would had the check on each texture unit of if
> the workaround is necessary and if there is an astc5x5 sampler handing 
> around. It would also kill off a member from the astc5x5_wa
> struct that tracks if there are auxiliary textures.

I wanted to see how it would look using brw_predraw_resolve_inputs(). See the
patch I sent as reply and let me know how you feel about doing something of
that sort.

Writing that reminded me of something I meant to ask: do we hit the sampler
bug with both directions: sampling astc5x5 first and then aux, and vice versa?
Or is only one direction problematic?

> 
> At any rate, I'd appreciate some review for the code so it can land in some 
> form shortly.
> 
> -Kevin
> 
> > 
> > 
> > Kevin Rogovin (5):
> >   i965: define astc5x5 workaround infrastructure
> >   i965: ASTC5x5 workaround logic for blorp
> >   i965: set ASTC5x5 workaround texture type tracking on texture validate
> >   i965: use ASTC5x5 workaround in brw_draw
> >   i965: use ASTC5x5 workaround in brw_compute
> > 
> >  src/mesa/drivers/dri/i965/brw_compute.c  |  6 +++
> >  src/mesa/drivers/dri/i965/brw_context.c  | 63 
> > 
> >  src/mesa/drivers/dri/i965/brw_context.h  | 23 +
> >  src/mesa/drivers/dri/i965/brw_draw.c |  6 +++
> >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  5 ++
> >  src/mesa/drivers/dri/i965/genX_blorp_exec.c  |  5 ++
> >  src/mesa/drivers/dri/i965/intel_batchbuffer.c|  1 +
> >  src/mesa/drivers/dri/i965/intel_tex_image.c  | 16 --
> >  src/mesa/drivers/dri/i965/intel_tex_validate.c   | 13 +
> >  9 files changed, 134 insertions(+), 4 deletions(-)
> > 
> > --
> > 2.14.2
> > 
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround

[Rogovin, Kevin]

Hi,

> 1) do extra tex cache flush when needed and specifically only when needed
> 2) resolve surfaces when undesired combination is about to be sampled

>Latter case could be addressed also with on-the-fly check in 
>brw_predraw_resolve_inputs(). There one goes thru all the active textures for 
>the
> next draw call and resolves when necessary. One could check for the undesired 
> combination of textures there. I understand we would need to
> walk the textures twice, first to check for any occurrences of one type and 
> then for the other. This, however, would fit to the way we resolve
> other texture types and prevent from adding more things to check into the 
> context.

This patch series does handle the second case, as follows:
 a) Checking if there are astc5x5 textures and/or textures with auxiliary is 
done in brw_validate_textures(); this was a really nice place because
  the function loops over all textures anyways
 b) the resolve operation is handled by the added function 
brw_atsc_perform_wa(); this also sets the mode correctly too after the resolve.

I chose to make a dedicated function that does the mode setting and resolve.  
Putting the resolve logic into brw_predraw_resolve_inputs()
is not a bad idea and I am open to it; the interior of the loop would then look 
ickier as it would had the check on each texture unit of if
the workaround is necessary and if there is an astc5x5 sampler handing around. 
It would also kill off a member from the astc5x5_wa
struct that tracks if there are auxiliary textures.

At any rate, I'd appreciate some review for the code so it can land in some 
form shortly.

-Kevin

> 
> 
> Kevin Rogovin (5):
>   i965: define astc5x5 workaround infrastructure
>   i965: ASTC5x5 workaround logic for blorp
>   i965: set ASTC5x5 workaround texture type tracking on texture validate
>   i965: use ASTC5x5 workaround in brw_draw
>   i965: use ASTC5x5 workaround in brw_compute
> 
>  src/mesa/drivers/dri/i965/brw_compute.c  |  6 +++
>  src/mesa/drivers/dri/i965/brw_context.c  | 63 
> 
>  src/mesa/drivers/dri/i965/brw_context.h  | 23 +
>  src/mesa/drivers/dri/i965/brw_draw.c |  6 +++
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  5 ++
>  src/mesa/drivers/dri/i965/genX_blorp_exec.c  |  5 ++
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c|  1 +
>  src/mesa/drivers/dri/i965/intel_tex_image.c  | 16 --
>  src/mesa/drivers/dri/i965/intel_tex_validate.c   | 13 +
>  9 files changed, 134 insertions(+), 4 deletions(-)
> 
> --
> 2.14.2
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround

[Pohjolainen, Topi]

On Fri, Dec 01, 2017 at 07:19:17PM +0200, kevin.rogo...@intel.com wrote:
> From: Kevin Rogovin 
> 
> This patch series implements a needed workaround for Gen9 for ASTC5x5
> sampler reads. The crux of the work around is to make sure that the
> sampler does not read an ASTC5x5 texture and a surface with an auxilary
> buffer without having a texture cache invalidate between such accesses.

The solution here is to store the types of read access (aux, astc5x5) into
context. This information is then used for two purposes: 

   1) do extra tex cache flush when needed and specifically only when needed
   2) resolve surfaces when undesired combination is about to be sampled

Latter case could be addressed also with on-the-fly check in
brw_predraw_resolve_inputs(). There one goes thru all the active textures for
the next draw call and resolves when necessary. One could check for the
undesired combination of textures there. I understand we would need to walk
the textures twice, first to check for any occurences of one type and
then for the other. This, however, would fit to the way we resolve other
texture types and prevent from adding more things to check into the context.

In the first case, if one didn't optimize, i.e., do the extra flush only when
needed, one could instead check if ASTC5x5 texture is going to be sampled and
flush before and after the draw call to be safe. This is not optimal but also
code-wise that simple that I'd be curious to know how much the optimized
flushing helps.

I think the non-optimized cases should be doable also in Vulkan. Jason, what
do you think?

> 
> 
> Kevin Rogovin (5):
>   i965: define astc5x5 workaround infrastructure
>   i965: ASTC5x5 workaround logic for blorp
>   i965: set ASTC5x5 workaround texture type tracking on texture validate
>   i965: use ASTC5x5 workaround in brw_draw
>   i965: use ASTC5x5 workaround in brw_compute
> 
>  src/mesa/drivers/dri/i965/brw_compute.c  |  6 +++
>  src/mesa/drivers/dri/i965/brw_context.c  | 63 
> 
>  src/mesa/drivers/dri/i965/brw_context.h  | 23 +
>  src/mesa/drivers/dri/i965/brw_draw.c |  6 +++
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  5 ++
>  src/mesa/drivers/dri/i965/genX_blorp_exec.c  |  5 ++
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c|  1 +
>  src/mesa/drivers/dri/i965/intel_tex_image.c  | 16 --
>  src/mesa/drivers/dri/i965/intel_tex_validate.c   | 13 +
>  9 files changed, 134 insertions(+), 4 deletions(-)
> 
> -- 
> 2.14.2
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround

[Rogovin, Kevin]

Hi,

 For ANV I do not know as I have not really poked into its code. For i965, this 
patch series handles the situation as to what to do if a draw of dispatch 
compute accesses both an ASTC5x5 texture and a texture with an auxiliary 
buffer. It does this by checking if there are both such textures and ASTC5x5 
textures in the list of currently bound textures. If the answer is yes, then it 
resolves all such auxiliary requiring textures that use an auxiliary buffer so 
that the sampler does not need them when it reads from the surfaces. The 
resolve stuff is handled in the function brw_astc5x5_perform_wa(() in 
brw_context.c of the first patch, the checking is handled in the 3rd patch by 
modifying brw_tex_validate() in intel_tex_validate.c. The 4'th and 5'th patches 
are deceptively small since all they do is add a call to 
brw_astc5x5_perform_wa(() in brw_draw.c and brw_compute.c respectively. The 4th 
patch also has a small addition to prevent surface state for sampler state to 
have the auxiliary surface given in the call.

As to how to do similar auto-resolve and tweak of state on ANV, I need to dive 
quite deep into the code to see how to do it.

-Kevin

-Original Message-
From: Matt Turner [mailto:matts...@gmail.com] 
Sent: Friday, December 1, 2017 8:25 PM
To: Rogovin, Kevin <kevin.rogo...@intel.com>
Cc: Ilia Mirkin <imir...@alum.mit.edu>; mesa-dev@lists.freedesktop.org
Subject: Re: [Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround

On Fri, Dec 1, 2017 at 10:06 AM, Rogovin, Kevin <kevin.rogo...@intel.com> wrote:
> Hi,
>
>  Yes ANV will need something like this as well. If the GPU samples from both 
> an ASTC5x5 texture and one with an aux buffer without a texture cache 
> invalidate between such accesses, then the GPU hangs, which in turn makes the 
> system unresponsive for a few seconds until the kernel resets the GPU; then 
> an ioctl will fail in i965 which means things are very bad usually and (for 
> me atleast on my system with how I build mesa) the application crashes.

I think his question is -- is there anything we can do about the case where a 
single shader program samples ASTC5x5 and a texture with an aux buffer? 
Presumably there's no way to invalidate the texture cache during a shader 
program, so what can you do?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround

[Matt Turner]

On Fri, Dec 1, 2017 at 10:06 AM, Rogovin, Kevin  wrote:
> Hi,
>
>  Yes ANV will need something like this as well. If the GPU samples from both 
> an ASTC5x5 texture and one with an aux buffer without a texture cache 
> invalidate between such accesses, then the GPU hangs, which in turn makes the 
> system unresponsive for a few seconds until the kernel resets the GPU; then 
> an ioctl will fail in i965 which means things are very bad usually and (for 
> me atleast on my system with how I build mesa) the application crashes.

I think his question is -- is there anything we can do about the case
where a single shader program samples ASTC5x5 and a texture with an
aux buffer? Presumably there's no way to invalidate the texture cache
during a shader program, so what can you do?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround

[Rogovin, Kevin]

Hi,

 Yes ANV will need something like this as well. If the GPU samples from both an 
ASTC5x5 texture and one with an aux buffer without a texture cache invalidate 
between such accesses, then the GPU hangs, which in turn makes the system 
unresponsive for a few seconds until the kernel resets the GPU; then an ioctl 
will fail in i965 which means things are very bad usually and (for me atleast 
on my system with how I build mesa) the application crashes.

 -Kevin

-Original Message-
From: ibmir...@gmail.com [mailto:ibmir...@gmail.com] On Behalf Of Ilia Mirkin
Sent: Friday, December 1, 2017 7:38 PM
To: Rogovin, Kevin <kevin.rogo...@intel.com>
Cc: mesa-dev@lists.freedesktop.org
Subject: Re: [Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround

On Fri, Dec 1, 2017 at 12:19 PM,  <kevin.rogo...@intel.com> wrote:
> From: Kevin Rogovin <kevin.rogo...@intel.com>
>
> This patch series implements a needed workaround for Gen9 for ASTC5x5 
> sampler reads. The crux of the work around is to make sure that the 
> sampler does not read an ASTC5x5 texture and a surface with an 
> auxilary buffer without having a texture cache invalidate between such 
> accesses.

Presumably anv needs something like this too? What happens if you have a single 
draw which samples from both an ASTC5x5 texture and one with an aux buffer? 
[Just curious.]
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround

[Ilia Mirkin]

On Fri, Dec 1, 2017 at 12:19 PM,   wrote:
> From: Kevin Rogovin 
>
> This patch series implements a needed workaround for Gen9 for ASTC5x5
> sampler reads. The crux of the work around is to make sure that the
> sampler does not read an ASTC5x5 texture and a surface with an auxilary
> buffer without having a texture cache invalidate between such accesses.

Presumably anv needs something like this too? What happens if you have
a single draw which samples from both an ASTC5x5 texture and one with
an aux buffer? [Just curious.]
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround

[kevin . rogovin]

From: Kevin Rogovin 

This patch series implements a needed workaround for Gen9 for ASTC5x5
sampler reads. The crux of the work around is to make sure that the
sampler does not read an ASTC5x5 texture and a surface with an auxilary
buffer without having a texture cache invalidate between such accesses.


Kevin Rogovin (5):
  i965: define astc5x5 workaround infrastructure
  i965: ASTC5x5 workaround logic for blorp
  i965: set ASTC5x5 workaround texture type tracking on texture validate
  i965: use ASTC5x5 workaround in brw_draw
  i965: use ASTC5x5 workaround in brw_compute

 src/mesa/drivers/dri/i965/brw_compute.c  |  6 +++
 src/mesa/drivers/dri/i965/brw_context.c  | 63 
 src/mesa/drivers/dri/i965/brw_context.h  | 23 +
 src/mesa/drivers/dri/i965/brw_draw.c |  6 +++
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  5 ++
 src/mesa/drivers/dri/i965/genX_blorp_exec.c  |  5 ++
 src/mesa/drivers/dri/i965/intel_batchbuffer.c|  1 +
 src/mesa/drivers/dri/i965/intel_tex_image.c  | 16 --
 src/mesa/drivers/dri/i965/intel_tex_validate.c   | 13 +
 9 files changed, 134 insertions(+), 4 deletions(-)

-- 
2.14.2

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