[Mesa-dev] [PATCH v3 11/16] anv/cmd_buffer: Move aux_usage assignment up
Jason Ekstrand
jason at jlekstrand.net
Wed Jul 12 00:09:54 UTC 2017
On Tue, Jul 11, 2017 at 4:31 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 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.
I'm not entirely convinced either. :-)
> 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.
>
I don't really care that much. I'd go for either 2. or just leave it as
is. Either is fine with me. I think it'll be much more clear what to do
once we have MCS fast clears in there. What I wrote above was me trying to
imagine what it would look like with MCS fast clears and then delete the
extra MCS bits. It'll work better once someone is writing code in a
computer rather than my brain. :-)
--Jason
> 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
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170711/c3b35268/attachment.html>
More information about the mesa-dev
mailing list