Re: [Mesa-dev] [PATCH 05/13] anv: Add and use color auxiliary buffer helpers
On Mon, Jun 19, 2017 at 05:03:51PM -0700, Jason Ekstrand wrote:
> On Mon, Jun 19, 2017 at 4:46 PM, Nanley Chery 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
> > > 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
> > > > ---
> > > > 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 ui
Re: [Mesa-dev] [PATCH 05/13] anv: Add and use color auxiliary buffer helpers
On Mon, Jun 19, 2017 at 4:46 PM, Nanley Chery 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
> > 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
> > > ---
> > > 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.
> > > +}
> > > +
> > > +/* 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.
> > > +}
> > > +
> > > +/* 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)) {
> > > +
Re: [Mesa-dev] [PATCH 05/13] anv: Add and use color auxiliary buffer helpers
On Mon, Jun 19, 2017 at 04:16:32PM -0700, Jason Ekstrand wrote:
> On Tue, Jun 13, 2017 at 11:41 AM, Nanley Chery
> 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
> > ---
> > 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
)
> > +}
> > +
> > +/* 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.
> > +}
> > +
> > +/* 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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
___
mesa-dev mailing l
Re: [Mesa-dev] [PATCH 05/13] anv: Add and use color auxiliary buffer helpers
On Tue, Jun 13, 2017 at 11:41 AM, Nanley Chery
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
> ---
> 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.
> +}
> +
> +/* 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.
> +}
> +
> +/* 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()?
> +
> + 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 05/13] anv: Add and use color auxiliary buffer helpers
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
---
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;
+}
+
+/* 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;
+}
+
+/* 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);
+
+ 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
