Re: [Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround
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
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
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
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 Cc: Ilia Mirkin ; 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 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
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
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 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, 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
Re: [Mesa-dev] [PATCH 0/5] i965: ASTC5x5 workaround
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
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