[Mesa-dev] [PATCH 04/18] anv: Update the HiZ sampling helper

[Nanley Chery]

On Tue, Feb 28, 2017 at 08:07:35AM -0800, Jason Ekstrand wrote:
> On Tue, Feb 28, 2017 at 8:02 AM, Nanley Chery <nanleychery at gmail.com> wrote:
> 
> > On Mon, Feb 27, 2017 at 08:48:48PM -0800, Jason Ekstrand wrote:
> > > On Feb 27, 2017 5:20 PM, "Nanley Chery" <nanleychery at gmail.com> wrote:
> > >
> > > Validate the inputs and actually verify that this image has a depth
> > > buffer.
> > >
> > > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > > ---
> > >  src/intel/vulkan/anv_blorp.c   | 1 +
> > >  src/intel/vulkan/anv_image.c   | 7 +++----
> > >  src/intel/vulkan/anv_private.h | 7 +++++--
> > >  3 files changed, 9 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> > > index 2cde3b7689..05250de06a 100644
> > > --- a/src/intel/vulkan/anv_blorp.c
> > > +++ b/src/intel/vulkan/anv_blorp.c
> > > @@ -1276,6 +1276,7 @@ anv_cmd_buffer_clear_subpass(struct anv_cmd_buffer
> > > *cmd_buffer)
> > >                 clear_with_hiz = false;
> > >              } else if (gen == 8 &&
> > >                         anv_can_sample_with_hiz(cmd_
> > > buffer->device->info.gen,
> > > +                                               iview->aspect_mask,
> > >                                                 iview->image->samples)) {
> > >                 /* Only gen9+ supports returning ANV_HZ_FC_VAL when
> > > sampling a
> > >                  * fast-cleared portion of a HiZ buffer. Testing has
> > > revealed
> > > diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> > > index 716cdf3a38..f9f264094f 100644
> > > --- a/src/intel/vulkan/anv_image.c
> > > +++ b/src/intel/vulkan/anv_image.c
> > > @@ -502,7 +502,6 @@ anv_layout_to_aux_usage(const uint8_t gen, const
> > struct
> > > anv_image * const image,
> > >         layout == VK_IMAGE_LAYOUT_PREINITIALIZED)
> > >        layout = future_layout;
> > >
> > > -   const bool has_depth = aspects & VK_IMAGE_ASPECT_DEPTH_BIT;
> > >     const bool color_aspect = aspects == VK_IMAGE_ASPECT_COLOR_BIT;
> > >
> > >     /* The following switch currently only handles depth stencil aspects.
> > > @@ -538,7 +537,7 @@ anv_layout_to_aux_usage(const uint8_t gen, const
> > struct
> > > anv_image * const image,
> > >        assert(!color_aspect);
> > >        /* Fall-through */
> > >     case VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL:
> > > -      if (has_depth && anv_can_sample_with_hiz(gen, image->samples))
> > > +      if (anv_can_sample_with_hiz(gen, aspects, image->samples))
> > >           return ISL_AUX_USAGE_HIZ;
> > >        else
> > >           return ISL_AUX_USAGE_NONE;
> > > @@ -699,8 +698,8 @@ anv_CreateImageView(VkDevice _device,
> > >     float red_clear_color = 0.0f;
> > >     enum isl_aux_usage surf_usage = image->aux_usage;
> > >     if (image->aux_usage == ISL_AUX_USAGE_HIZ) {
> > > -      if (iview->aspect_mask & VK_IMAGE_ASPECT_DEPTH_BIT &&
> > > -          anv_can_sample_with_hiz(device->info.gen, image->samples)) {
> > > +      if (anv_can_sample_with_hiz(device->info.gen, iview->aspect_mask,
> > > +                                  image->samples)) {
> > >           /* When a HiZ buffer is sampled on gen9+, ensure that
> > >            * the constant fast clear value is set in the surface state.
> > >            */
> > > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> > private.h
> > > index 5ce9027f88..b21a9fb359 100644
> > > --- a/src/intel/vulkan/anv_private.h
> > > +++ b/src/intel/vulkan/anv_private.h
> > > @@ -1661,9 +1661,12 @@ struct anv_image {
> > >
> > >  /* Returns true if a HiZ-enabled depth buffer can be sampled from. */
> > >  static inline bool
> > > -anv_can_sample_with_hiz(uint8_t gen, uint32_t samples)
> > > +anv_can_sample_with_hiz(uint8_t gen, VkImageAspectFlags aspect_mask,
> > > +                        uint32_t samples)
> > >  {
> > > -   return gen >= 8 && samples == 1;
> > > +   /* Validate the inputs. */
> > > +   assert(gen >= 7 && aspect_mask && samples);
> > > +   return gen >= 8 && aspect_mask & VK_IMAGE_ASPECT_DEPTH_BIT && samples
> > > == 1;
> > >
> > >
> > > I think you want parents around the "aspect & DEPTH"
> > >
> >
> > According to the C11 spec (footnote 85 found under section 6.5
> > Expressions), the parenthesis are not necessary because the bitwise AND
> > operator has higher precedence than the logical AND operator.
> >
> 
> I figured but the compiler likes to warn about those things and I really
> don't like having to look at the spec in order to figure out what a line of
> code means. :-)


Below, is a tip on how to check the operator precedence that may sway
your opinion. Nonetheless, if you'd still like the parenthesis, I can
add it.

Upon reading the aforementioned spec section today, I discovered that
it's actually quite simple to check the precedence. The footnote
explains the following rule and an exception to it: precedence follows
the order of the subsections of section 6.5.

>From the table of contents:
6.5 Expressions
	6.5.1 Primary expressions
	6.5.2 Postfix operators
	6.5.3 Unary operators
	6.5.4 Cast operators
	6.5.5 Multiplicative operators
	6.5.6 Additive operators
	6.5.7 Bitwise shift operators
	6.5.8 Relational operators
	6.5.9 Equality operators
	6.5.10 Bitwise AND operator
	6.5.11 Bitwise exclusive OR operator
	6.5.12 Bitwise inclusive OR operator
	6.5.13 Logical AND operator
	6.5.14 Logical OR operator
	6.5.15 Conditional operator
	6.5.16 Assignment operators
	6.5.17 Comma operator

-Nanley