[Mesa-dev] [PATCH 05/13] anv: Add and use color auxiliary buffer helpers
Nanley Chery
nanleychery at gmail.com
Tue Jun 20 21:09:34 UTC 2017
On Mon, Jun 19, 2017 at 05:03:51PM -0700, Jason Ekstrand wrote:
> On Mon, Jun 19, 2017 at 4:46 PM, Nanley Chery <nanleychery at gmail.com> wrote:
>
> > On Mon, Jun 19, 2017 at 04:16:32PM -0700, Jason Ekstrand wrote:
> > > On Tue, Jun 13, 2017 at 11:41 AM, Nanley Chery <nanleychery at gmail.com>
> > > wrote:
> > >
> > > > v2:
> > > > - Check for aux levels in layer helper (Jason Ekstrand)
> > > > - Don't assert aux is present, return 0 if it isn't.
> > > > - Use the helpers.
> > > >
> > > > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > > > ---
> > > > src/intel/vulkan/anv_blorp.c | 4 ++++
> > > > src/intel/vulkan/anv_private.h | 39 ++++++++++++++++++++++++++++++
> > > > +++++++++
> > > > 2 files changed, 43 insertions(+)
> > > >
> > > > diff --git a/src/intel/vulkan/anv_blorp.c
> > b/src/intel/vulkan/anv_blorp.c
> > > > index a869eebc24..421f860428 100644
> > > > --- a/src/intel/vulkan/anv_blorp.c
> > > > +++ b/src/intel/vulkan/anv_blorp.c
> > > > @@ -1436,6 +1436,7 @@ anv_image_ccs_clear(struct anv_cmd_buffer
> > > > *cmd_buffer,
> > > > const struct isl_view *view,
> > > > const VkImageSubresourceRange *subresourceRange)
> > > > {
> > > > + assert(anv_image_has_color_aux(image) && image->samples == 1);
> > > > assert(image->type == VK_IMAGE_TYPE_3D || image->extent.depth ==
> > 1);
> > > >
> > > > struct blorp_batch batch;
> > > > @@ -1488,6 +1489,9 @@ anv_image_ccs_clear(struct anv_cmd_buffer
> > > > *cmd_buffer,
> > > > blorp_layer_count = anv_get_layerCount(image,
> > subresourceRange);
> > > > }
> > > >
> > > > + assert(level < anv_color_aux_levels(image));
> > > > + assert(blorp_base_layer + blorp_layer_count <=
> > > > + anv_color_aux_layers(image, level));
> > > > blorp_fast_clear(&batch, &surf, surf.surf->format,
> > > > level, blorp_base_layer, blorp_layer_count,
> > > > 0, 0, extent.width, extent.height);
> > > > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> > > > private.h
> > > > index fe6ac3bc1b..32aec7782b 100644
> > > > --- a/src/intel/vulkan/anv_private.h
> > > > +++ b/src/intel/vulkan/anv_private.h
> > > > @@ -2071,6 +2071,45 @@ struct anv_image {
> > > > struct anv_surface aux_surface;
> > > > };
> > > >
> > > > +/* Return whether or not the image has a color auxiliary buffer. */
> > > > +static inline bool
> > > > +anv_image_has_color_aux(const struct anv_image * const image)
> > > > +{
> > > > + assert(image);
> > > > +
> > > > + return image->aspects == VK_IMAGE_ASPECT_COLOR_BIT &&
> > > > + image->aux_surface.isl.size > 0;
> > > >
> > >
> > > I'm not a big fan of conflating CCS and MCS. We do that in i965 today
> > and
> > > it's no end of pain. How about anv_image_has_ccs and restrict on sample
> > > count as well? Same comment applies to the two below.
> > >
> > >
> >
> > I'm not sure I understand your concern here, but I can create two
> > functions instead. I think most of the callers of this function only
> > care about whether or not there's a color buffer with CCS or MCS.
> > (You can find more of the callers here:
> > https://cgit.freedesktop.org/~nchery/mesa/log/?h=vk/perf/
> > layout-ccsd-resolves-v3 )
> >
>
> My concern is that "color aux" isn't really a useful thing. You could call
> it anv_image_has_aux and make it apply to everything, or we can make it
> specific in which case it should be has_ccs. Separating it into depth and
> color without splitting out MSAA is deceptive.
>
>
Looking at all the uses of the helper, it looks like you're right - it
isn't really useful. Due to nearby assertions, most usages could be
replaced with a simple aspect check. I'm thinking I'll just delete it.
> > > > +}
> > > > +
> > > > +/* Returns the number of auxiliary buffer levels attached to a color
> > > > image. */
> > > > +static inline uint8_t
> > > > +anv_color_aux_levels(const struct anv_image * const image)
> > > > +{
> > > > + assert(image && image->aspects == VK_IMAGE_ASPECT_COLOR_BIT);
> > > > + return anv_image_has_color_aux(image) ?
> > image->aux_surface.isl.levels
> > > > : 0;
> > > >
> > >
> > > Is there some reason why we can't or shouldn't assert
> > anv_image_has_ccs? I
> > > haven't read all of the users of these helpers so maybe there's something
> > > really useful about having them silently return 0.
> > >
> > >
> >
> > We did previously. With your suggestion of returning 0 in the below
> > function, I thought that it would make sense to give this function the
> > same behavior.
> >
>
> If I've managed go suggest that we do it both ways, then I clearly don't
> care. Let's just go with what you have here. As stated above, making it
> anv_image_aux_levels instead of mentioning color would be good though.
>
>
Sounds good. I like the idea of making it aspect independent.
-Nanley
> > > > +}
> > > > +
> > > > +/* Returns the number of auxiliary buffer layers attached to a color
> > > > image. */
> > > > +static inline uint32_t
> > > > +anv_color_aux_layers(const struct anv_image * const image,
> > > > + const uint8_t miplevel)
> > > > +{
> > > > + assert(image && image->aspects == VK_IMAGE_ASPECT_COLOR_BIT);
> > > > +
> > > > + /* The miplevel must exist in the main buffer. */
> > > > + assert(miplevel < image->levels);
> > > >
> > >
> > > Similarly can we or should we assert that miplevel <
> > anv_color_aux_levels()?
> > >
> > >
> >
> > I'm confused, you suggested that we return 0 in your previous review of
> > this patch.
> >
> > > > +
> > > > + if (miplevel >= anv_color_aux_levels(image)) {
> > > > + /* There are no layers with auxiliary data because the miplevel
> > has
> > > > no
> > > > + * auxiliary data.
> > > > + */
> > > > + return 0;
> > > > + } else {
> > > > + return MAX2(image->aux_surface.isl.logical_level0_px.array_len,
> > > > + image->aux_surface.isl.logical_level0_px.depth >>
> > > > miplevel);
> > > > + }
> > > > +}
> > > > +
> > > > /* Returns true if a HiZ-enabled depth buffer can be sampled from. */
> > > > static inline bool
> > > > anv_can_sample_with_hiz(const struct gen_device_info * const devinfo,
> > > > --
> > > > 2.13.1
> > > >
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > >
> >
More information about the mesa-dev
mailing list