[Mesa-dev] [PATCH v3 11/16] anv/cmd_buffer: Move aux_usage assignment up

[Nanley Chery]

On Mon, Jul 10, 2017 at 02:36:16PM -0700, Jason Ekstrand wrote:
> On Wed, Jun 28, 2017 at 2:14 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 
> > For readability, bring the assignment of CCS closer to the assignment of
> > NONE and MCS.
> >
> > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > ---
> >  src/intel/vulkan/genX_cmd_buffer.c | 62 ++++++++++++++++++------------
> > --------
> >  1 file changed, 30 insertions(+), 32 deletions(-)
> >
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > index 49ad41edbd..1aa79c8e7b 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -253,6 +253,36 @@ color_attachment_compute_aux_usage(struct anv_device
> > * device,
> >        att_state->input_aux_usage = ISL_AUX_USAGE_MCS;
> >        att_state->fast_clear = false;
> >        return;
> > +   } else if (iview->image->aux_usage == ISL_AUX_USAGE_CCS_E) {
> > +      att_state->aux_usage = ISL_AUX_USAGE_CCS_E;
> > +      att_state->input_aux_usage = ISL_AUX_USAGE_CCS_E;
> >
> 
> I'm not sure if this actually improves readability as-is.  The no aux case
> and MCS cases are early returns.  Maybe what we want is something like this:
> 
> if (aux_surface.isl.size == 0) {
>    /* set to none */
>    return;
> }
> 
> switch (aux_usage) {
> case MCS:
> case CCS_E:
>    aux_state->aux_usage = iview->image->aux_usage;
>    aux_state->input_aux_usage = iview->image->aux_usage;
>    break;
> 
> case NONE:
>    assert(samples == 1);
>    /* stuff below */
>    break;
> 
> default:
>    unreachable();
> }
> 
> /* Now we determine whether or not we want to fast-clear */
> 
> if (samples > 1) {
>    perf_debug();
>    fast_clear = false;
>    return;
> }
> 
> /* Other fast clear determination. */
> 
> Incidentally, it may be cleaner in the long run if we split this into two
> functions: compute_ccs_usage and compute_mcs_usage.
> 
> Just thoughts BTW.  I'm not 100% sure how to make this the most readable.
> 
> 

I'm personally not finding the alternative block above more readable. To
overcome this disagreement, I don't mind doing any of the following:

1. Omit the rationale in the commit message.
2. Drop the patch from this series.

Thoughts?

-Nanley

> > +   } else {
> > +      att_state->aux_usage = ISL_AUX_USAGE_CCS_D;
> > +      /* From the Sky Lake PRM, RENDER_SURFACE_STATE::
> > AuxiliarySurfaceMode:
> > +       *
> > +       *    "If Number of Multisamples is MULTISAMPLECOUNT_1, AUX_CCS_D
> > +       *    setting is only allowed if Surface Format supported for Fast
> > +       *    Clear. In addition, if the surface is bound to the sampling
> > +       *    engine, Surface Format must be supported for Render Target
> > +       *    Compression for surfaces bound to the sampling engine."
> > +       *
> > +       * In other words, we can only sample from a fast-cleared image if
> > it
> > +       * also supports color compression.
> > +       */
> > +      if (isl_format_supports_ccs_e(&device->info, iview->isl.format)) {
> > +         /* TODO: Consider using a heuristic to determine if temporarily
> > enabling
> > +          * CCS_E for this image view would be beneficial.
> > +          *
> > +          * While fast-clear resolves and partial resolves are fairly
> > cheap in the
> > +          * case where you render to most of the pixels, full resolves
> > are not
> > +          * because they potentially involve reading and writing the
> > entire
> > +          * framebuffer.  If we can't texture with CCS_E, we should leave
> > it off and
> > +          * limit ourselves to fast clears.
> > +          */
> > +         att_state->input_aux_usage = ISL_AUX_USAGE_CCS_D;
> > +      } else {
> > +         att_state->input_aux_usage = ISL_AUX_USAGE_NONE;
> > +      }
> >     }
> >
> >     assert(iview->image->aux_surface.isl.usage & ISL_SURF_USAGE_CCS_BIT);
> > @@ -315,38 +345,6 @@ color_attachment_compute_aux_usage(struct anv_device
> > * device,
> >     } else {
> >        att_state->fast_clear = false;
> >     }
> > -
> > -   /**
> > -    * TODO: Consider using a heuristic to determine if temporarily
> > enabling
> > -    * CCS_E for this image view would be beneficial.
> > -    *
> > -    * While fast-clear resolves and partial resolves are fairly cheap in
> > the
> > -    * case where you render to most of the pixels, full resolves are not
> > -    * because they potentially involve reading and writing the entire
> > -    * framebuffer.  If we can't texture with CCS_E, we should leave it
> > off and
> > -    * limit ourselves to fast clears.
> > -    */
> > -   if (iview->image->aux_usage == ISL_AUX_USAGE_CCS_E) {
> > -      att_state->aux_usage = ISL_AUX_USAGE_CCS_E;
> > -      att_state->input_aux_usage = ISL_AUX_USAGE_CCS_E;
> > -   } else {
> > -      att_state->aux_usage = ISL_AUX_USAGE_CCS_D;
> > -      /* From the Sky Lake PRM, RENDER_SURFACE_STATE::
> > AuxiliarySurfaceMode:
> > -       *
> > -       *    "If Number of Multisamples is MULTISAMPLECOUNT_1, AUX_CCS_D
> > -       *    setting is only allowed if Surface Format supported for Fast
> > -       *    Clear. In addition, if the surface is bound to the sampling
> > -       *    engine, Surface Format must be supported for Render Target
> > -       *    Compression for surfaces bound to the sampling engine."
> > -       *
> > -       * In other words, we can only sample from a fast-cleared image if
> > it
> > -       * also supports color compression.
> > -       */
> > -      if (isl_format_supports_ccs_e(&device->info, iview->isl.format))
> > -         att_state->input_aux_usage = ISL_AUX_USAGE_CCS_D;
> > -      else
> > -         att_state->input_aux_usage = ISL_AUX_USAGE_NONE;
> > -   }
> >  }
> >
> >  static bool
> > --
> > 2.13.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >