[Mesa-dev] [PATCH 2/2] i965: Fix intel_miptree_is_fast_clear_capable()

[Ben Widawsky]

On Thu, Oct 01, 2015 at 08:20:07AM -0700, Chad Versace wrote:
> There are three types of fast clears:
>   a. fast depth clears
>   b. fast singlesample color clears
>   c. fast multisample color clears
> Function intel_miptree_is_fast_clear_capable() checks if a miptree
> supports fast clears of type (b).
> 
> Rename the function to disambiguate what it does:
>   old: intel_miptree_is_fast_clear_capable
>   new: intel_miptree_supports_non_msrt_fast_clear
> 
> The functionally *accidentally* rejected multisampled color surfaces
> because it thought they were singlesample array surfaces. Fix that by
> explicitly rejecting surfaces with samples > 1.
> 

I wasn't going to say anything except you put "accidentally" in bold. I
don't think you can determine what Paul was thinking when he originally
implemented that code. Calling it accidental is unnecessarily judgmental (as is
my response here). The code was correct, and unless you want to ask him, we
should assume it was intentional.

FWIW: I didn't spot anything wrong in this patch, but I don't know the code well
enough to call that a reviewed-by in the time I spent looking at it.

> This fix would have been needed before we enabled layered fast
> singlesample color clears (introduced in gen8), which we want to do
> eventually. For now, though, this patch changes no behavior; it just
> fixes how the driver chooses its behavior.
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 05dc291..a169c41 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -194,8 +194,8 @@ intel_tiling_supports_non_msrt_mcs(struct brw_context *brw, unsigned tiling)
>   *       64bpp, and 128bpp.
>   */
>  static bool
> -intel_miptree_is_fast_clear_capable(struct brw_context *brw,
> -                                    struct intel_mipmap_tree *mt)
> +intel_miptree_supports_non_msrt_fast_clear(struct brw_context *brw,
> +                                           struct intel_mipmap_tree *mt)
>  {
>     /* MCS support does not exist prior to Gen7 */
>     if (brw->gen < 7)
> @@ -204,6 +204,10 @@ intel_miptree_is_fast_clear_capable(struct brw_context *brw,
>     if (mt->disable_aux_buffers)
>        return false;
>  
> +   /* This function applies only to non-multisampled render targets. */
> +   if (mt->num_samples > 1)
> +      return false;
> +
>     /* MCS is only supported for color buffers */
>     switch (_mesa_get_format_base_format(mt->format)) {
>     case GL_DEPTH_COMPONENT:
> @@ -222,7 +226,16 @@ intel_miptree_is_fast_clear_capable(struct brw_context *brw,
>  
>        return false;
>     }
> +
> +   /* Check for layered surfaces. */
>     if (mt->physical_depth0 != 1) {
> +       /* Multisample surfaces with the CMS layout are not layered surfaces,
> +        * yet still have physical_depth0 > 1. Assert that we don't
> +        * accidentally reject a multisampled surface here. We should have
> +        * rejected it earlier by explicitly checking the sample count.
> +        */
> +      assert(mt->num_samples <= 1);
> +
>        if (brw->gen >= 8) {
>           perf_debug("Layered fast clear - giving up. (%dx%d%d)\n",
>                      mt->logical_width0, mt->logical_height0,
> @@ -494,7 +507,7 @@ intel_miptree_create_layout(struct brw_context *brw,
>      *  7   |      ?         |        ?
>      *  6   |      ?         |        ?
>      */
> -   if (intel_miptree_is_fast_clear_capable(brw, mt)) {
> +   if (intel_miptree_supports_non_msrt_fast_clear(brw, mt)) {
>        if (brw->gen >= 9 || (brw->gen == 8 && num_samples <= 1))
>           layout_flags |= MIPTREE_LAYOUT_FORCE_HALIGN16;
>     } else if (brw->gen >= 9 && num_samples > 1) {
> @@ -692,7 +705,7 @@ intel_miptree_create(struct brw_context *brw,
>      * clear actually occurs.
>      */
>     if (intel_tiling_supports_non_msrt_mcs(brw, mt->tiling) &&
> -       intel_miptree_is_fast_clear_capable(brw, mt)) {
> +       intel_miptree_supports_non_msrt_fast_clear(brw, mt)) {
>        mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_RESOLVED;
>        assert(brw->gen < 8 || mt->halign == 16 || num_samples <= 1);
>     }
> @@ -800,8 +813,9 @@ intel_update_winsys_renderbuffer_miptree(struct brw_context *intel,
>      * clear actually occurs.
>      */
>     if (intel_tiling_supports_non_msrt_mcs(intel, singlesample_mt->tiling) &&
> -       intel_miptree_is_fast_clear_capable(intel, singlesample_mt))
> +       intel_miptree_supports_non_msrt_fast_clear(intel, singlesample_mt)) {
>        singlesample_mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_RESOLVED;
> +   }
>  
>     if (num_samples == 0) {
>        intel_miptree_release(&irb->mt);
> -- 
> 2.5.0.342.g44e0223
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev