On Mon, Jul 10, 2017 at 02:36:16PM -0700, Jason Ekstrand wrote:
> On Wed, Jun 28, 2017 at 2:14 PM, Nanley Chery <nanleych...@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.ch...@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@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
