[Mesa-dev] [PATCH] RFC: Workaround for gen9 hw astc5x5 sampler bug

[Rogovin, Kevin]

> If you take a look at brw_update_texture_surface(), just in the end before
> brw_emit_surface_state() the logic explictly consults for intel_miptree_texture_aux_usage(). 
> This in turn tells if the auxiliary buffer is resolved and it doesn't need to be programmed.

The full stack trace is this:

 brw_update_texture_surface() calls intel_miptree_texture_usage() which for HiZ calls the function intel_miptree_sample_with_hiz() which, as the name suggest, returns true if and only if it is ok to use the HiZ to speed up depth texture reads. Indeed, the function calls intel_miptree_texture_usage() switches on intel_mipmap_tree::aux_usage which the documentation states is about the intended usage of the auxiliary buffer. Indeed, if the value is ISL_AUX_USAGE_NONE that means, quoting the comment tag above the field, "that auxiliary compression is permanently disabled". The conclusion then is that even if a buffer is fully resolved, the value of aux_usage is not ISL_AUX_USAGE_NONE and that the return value of calls intel_miptree_texture_usage() gives the return value assuming that the auxiliary buffer can be used with respect to that it holds good values enough for the sampler.

>This path goes thru brw_blorp_download_miptree() which in turn takes either
> brw_blorp_copy_miptrees() or brw_blorp_blit_miptrees(). Both in turn consult blorp_surf_for_miptree().

If you are 100% sure (because I am not) that ALL blorp paths walk through that, then I have no real objection except it needs to do the magic of checking if it is reading from an ASTC5x5 or a surface with an auxiliary and update the enumeration appropriately.

> This would be a nice a piece of documentation to add into the code. Thanks for explaining.

I can add this, though I do freely admit I take this for granted and an axiom of HW accelerated graphics.

>I don't see how the bit mask prevents one from forcing anything.

By not being able to encode such a state (both are present) such a state is impossible to store and cannot be reached. From the point of view of code, an enum is slightly more pleasant to read than bitmaks IMO.

> I now do see a flaw in the RFC related to this. In brw_predraw_resolve_inputs() one would 
> actually need to smash down the recorded AUX mask bit when it reacts to ASTC5x5 being present.

Indeed, really two passes are needed over the textures. The first pass to detect if ASTC5x5 is present and a second to resolve auxiliary using textures if they are present.

>Well, if nothing else, I would really like to see you used brw_predraw_resolve_inputs() for the resolves.

I am happy with that as that walks through the textures anyways BUT it is called before brw_predraw_resolve_framebuffer() which might do some resolving too. The easy way out is to permute their calling order unless there is some other dangling assumption preventing permuting the call order of those two.

> 
> -Kevin
>