<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 28, 2017 at 9:29 AM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Feb 28, 2017 at 08:07:35AM -0800, Jason Ekstrand wrote: > On Tue, Feb 28, 2017 at 8:02 AM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote: > > > On Mon, Feb 27, 2017 at 08:48:48PM -0800, Jason Ekstrand wrote: > > > On Feb 27, 2017 5:20 PM, "Nanley Chery" <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote: > > > > > > Validate the inputs and actually verify that this image has a depth > > > buffer. > > > > > > Signed-off-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> > > > --- > > > 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. :-) </div></div>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. </blockquote><div> </div><div>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. :-) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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 </blockquote></div> </div></div>