On Tue, Feb 28, 2017 at 9:29 AM, Nanley Chery <nanleych...@gmail.com> wrote:
> On Tue, Feb 28, 2017 at 08:07:35AM -0800, Jason Ekstrand wrote:
> > On Tue, Feb 28, 2017 at 8:02 AM, Nanley Chery <nanleych...@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" <nanleych...@gmail.com>
> wrote:
> > > >
> > > > Validate the inputs and actually verify that this image has a depth
> > > > buffer.
> > > >
> > > > Signed-off-by: Nanley Chery <nanley.g.ch...@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.
>
Yes, I know how to check and I know that logical comes after bitwise. It's
not a matter of how easy it is to check so much as whether or not I have
to. A pair of parentheses is a small cost to pay for code obviousness.
I'm very surprised you didn't see a GCC warning about this. It always
warns on me whenever I mix "&" with "&&" or "||". Please add them. :-)
> 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
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
