[Mesa-dev] [PATCH 04/18] anv: Update the HiZ sampling helper
Nanley Chery
nanleychery at gmail.com
Tue Feb 28 18:19:40 UTC 2017
On Tue, Feb 28, 2017 at 09:33:26AM -0800, Jason Ekstrand wrote:
> On Tue, Feb 28, 2017 at 9:29 AM, Nanley Chery <nanleychery at 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 <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.
> >
>
> 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.
By this, I take it that you personally find it more obvious with
parentheses. I'll have it in the v2.
> 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. :-)
>
I don't get any warnings, but I can get GCC to warn if I add an equality
after the "&". It seems selectively warn depending on the set of
operators in a grouping.
-Nanley
>
> > 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
> >
More information about the mesa-dev
mailing list