[Mesa-dev] [PATCH 1/2] anv/cmd_buffer: Don't temporarily enable CCS_E within a render pass

[Nanley Chery]

On Mon, Jan 23, 2017 at 05:42:44PM -0800, Jason Ekstrand wrote:
> On Mon, Jan 23, 2017 at 5:40 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 
> > On Mon, Jan 23, 2017 at 05:30:22PM -0800, Jason Ekstrand wrote:
> > > On Mon, Jan 23, 2017 at 4:55 PM, Nanley Chery <nanleychery at gmail.com>
> > wrote:
> > >
> > > > Compressing a render target and decompressing it in the same
> > > > single-subpass render pass may waste bandwidth. While this may be
> > > > beneficial in some circumstances, it does not help in all.
> > > >
> > > > Cc: "13.0 17.0" <mesa-stable at lists.freedesktop.org>
> > > >
> > >
> > > This doesn't really fix a bug...  I guess you can consider it a perf bug
> > > but it's not going to cause apps to fail.  I think I'm ok with pulling it
> > > back to 17.0 but let's leave 13.0 alone.
> > >
> > >
> >
> > Sounds good to me. I retract the "13.0" from this patch and the
> > following (I plan to officially do so in future patch revisions). As a
> > disclaimer, I personally don't mind retracting the nomination for "17.0"
> > if it's an issue.
> >
> 
> I don't know how I feel for 17.0.  Obviously, making 17.0 faster is a good
> thing.  On the other hand, users will get a 17.1 soon enough and this can
> probably wait for that.  I think my preference (mild though it is) would be
> to not bother with stable at all
> 
> 

Great. I'm leaning the same way, so lets go with that. Such patches don't
fit within the description of acceptable performance patches for stable
branches, so there was a chance they'd get rejected anyhow.

-Nanley

> > -Nanley
> >
> > > > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > > > ---
> > > >  src/intel/vulkan/genX_cmd_buffer.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > > > b/src/intel/vulkan/genX_cmd_buffer.c
> > > > index a22fb2b6fb..9cde6896bb 100644
> > > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > > @@ -265,8 +265,10 @@ color_attachment_compute_aux_usage(struct
> > anv_device
> > > > *device,
> > > >        att_state->fast_clear = false;
> > > >     }
> > > >
> > > > -   if (isl_format_supports_lossless_compression(&device->info,
> > > > -                                                iview->isl.format)) {
> > > > +   /* TODO: Consider using a heuristic to determine if temporarily
> > > > enabling
> > > > +    * CCS_E for this image view would be beneficial.
> > > > +    */
> > > >
> > >
> > > Maybe we should update this comment a bit:
> > >
> > > 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.
> > >
> > > Also... This doesn't do quite what you think it does.  It shuts off fast
> > > clears entirely on Sky Lake if we can't texture from CCS_E.  We need to
> > add
> > > some code above to do
> > >
> > > if (GEN_GEN >= 9 && !isl_format_supports_lossless_compression(...))
> > >    att_state->fast_clear = false;
> > >
> > > And then remove the GEN_GEN >= 9 case below.  Maybe we want to do the
> > code
> > > shuffling as a refactor patch and put this patch on top of it?
> > >
> > >
> > > > +   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 if (att_state->fast_clear) {
> > > > --
> > > > 2.11.0
> > > >
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > >
> >