Re: [Mesa-dev] [PATCH 2/5] anv/cmd_buffer: Delay variable assignment in HZ function

[Nanley Chery]

On Mon, Oct 10, 2016 at 04:38:12PM -0700, Nanley Chery wrote:
> On Mon, Oct 10, 2016 at 04:01:29PM -0700, Jason Ekstrand wrote:
> > My patch to fix partial draws makes us use full_surface_op for HiZ resolves
> > as well so this patch (and maybe the next one?) are probably moot.
> > 
> 
> I realize that this patch conflicts with yours, but in my reply to that
> patch, I stated that I don't think the right fix is within this emit
> function.
> 

I've reviewed the patch in the aforementioned thread, so I'll send out a
v2 that takes this into account.

> -Nanley
> 
> > On Oct 10, 2016 3:33 PM, "Nanley Chery"  wrote:
> > 
> > > Delay the assignment of some variables until we know they will be used.
> > >
> > > Signed-off-by: Nanley Chery 
> > > ---
> > >  src/intel/vulkan/gen8_cmd_buffer.c | 32 +---
> > >  1 file changed, 17 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/gen8_cmd_buffer.c
> > > b/src/intel/vulkan/gen8_cmd_buffer.c
> > > index 02c2d7c..e88ab02 100644
> > > --- a/src/intel/vulkan/gen8_cmd_buffer.c
> > > +++ b/src/intel/vulkan/gen8_cmd_buffer.c
> > > @@ -410,7 +410,6 @@ void
> > >  genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer *cmd_buffer,
> > >enum blorp_hiz_op op)
> > >  {
> > > -   struct anv_cmd_state *cmd_state = _buffer->state;
> > > const struct anv_image_view *iview =
> > >anv_cmd_buffer_get_depth_stencil_view(cmd_buffer);
> > >
> > > @@ -421,21 +420,9 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> > > *cmd_buffer,
> > > if (cmd_buffer->state.pass->subpass_count > 1)
> > >return;
> > >
> > > +   struct anv_cmd_state *cmd_state = _buffer->state;
> > > const uint32_t ds = cmd_state->subpass->depth_stencil_attachment;
> > > -
> > > -   /* Section 7.4. of the Vulkan 1.0.27 spec states:
> > > -*
> > > -*   "The render area must be contained within the framebuffer
> > > dimensions."
> > > -*
> > > -* Therefore, the only way the extent of the render area can match
> > > that of
> > > -* the image view is if the render area offset equals (0, 0).
> > > -*/
> > > -   const bool full_surface_op =
> > > - cmd_state->render_area.extent.width == iview->extent.width
> > > &&
> > > - cmd_state->render_area.extent.height ==
> > > iview->extent.height;
> > > -   if (full_surface_op)
> > > -  assert(cmd_state->render_area.offset.x == 0 &&
> > > - cmd_state->render_area.offset.y == 0);
> > > +   bool full_surface_op;
> > >
> > > /* This variable corresponds to the Pixel Dim column in the table
> > > below */
> > > struct isl_extent2d px_dim;
> > > @@ -447,6 +434,21 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> > > *cmd_buffer,
> > >VK_ATTACHMENT_LOAD_OP_CLEAR)
> > >   return;
> > >
> > > +  /* Section 7.4. of the Vulkan 1.0.27 spec states:
> > > +   *
> > > +   *   "The render area must be contained within the framebuffer
> > > dimensions."
> > > +   *
> > > +   * Therefore, the only way the extent of the render area can match
> > > that of
> > > +   * the image view is if the render area offset equals (0, 0).
> > > +   */
> > > +  full_surface_op =
> > > + cmd_state->render_area.extent.width == iview->extent.width
> > > &&
> > > + cmd_state->render_area.extent.height ==
> > > iview->extent.height;
> > > +  if (full_surface_op)
> > > + assert(cmd_state->render_area.offset.x == 0 &&
> > > +cmd_state->render_area.offset.y == 0);
> > > +
> > > +
> > >/* Apply alignment restrictions. Despite the BDW PRM mentioning
> > > this is
> > > * only needed for a depth buffer surface type of D16_UNORM, 
> > > testing
> > > * showed it to be necessary for other depth formats as well
> > > --
> > > 2.10.0
> > >
> > > ___
> > > 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

Re: [Mesa-dev] [PATCH 2/5] anv/cmd_buffer: Delay variable assignment in HZ function

[Nanley Chery]

On Mon, Oct 10, 2016 at 04:01:29PM -0700, Jason Ekstrand wrote:
> My patch to fix partial draws makes us use full_surface_op for HiZ resolves
> as well so this patch (and maybe the next one?) are probably moot.
> 

I realize that this patch conflicts with yours, but in my reply to that
patch, I stated that I don't think the right fix is within this emit
function.

-Nanley

> On Oct 10, 2016 3:33 PM, "Nanley Chery"  wrote:
> 
> > Delay the assignment of some variables until we know they will be used.
> >
> > Signed-off-by: Nanley Chery 
> > ---
> >  src/intel/vulkan/gen8_cmd_buffer.c | 32 +---
> >  1 file changed, 17 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/intel/vulkan/gen8_cmd_buffer.c
> > b/src/intel/vulkan/gen8_cmd_buffer.c
> > index 02c2d7c..e88ab02 100644
> > --- a/src/intel/vulkan/gen8_cmd_buffer.c
> > +++ b/src/intel/vulkan/gen8_cmd_buffer.c
> > @@ -410,7 +410,6 @@ void
> >  genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer *cmd_buffer,
> >enum blorp_hiz_op op)
> >  {
> > -   struct anv_cmd_state *cmd_state = _buffer->state;
> > const struct anv_image_view *iview =
> >anv_cmd_buffer_get_depth_stencil_view(cmd_buffer);
> >
> > @@ -421,21 +420,9 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> > *cmd_buffer,
> > if (cmd_buffer->state.pass->subpass_count > 1)
> >return;
> >
> > +   struct anv_cmd_state *cmd_state = _buffer->state;
> > const uint32_t ds = cmd_state->subpass->depth_stencil_attachment;
> > -
> > -   /* Section 7.4. of the Vulkan 1.0.27 spec states:
> > -*
> > -*   "The render area must be contained within the framebuffer
> > dimensions."
> > -*
> > -* Therefore, the only way the extent of the render area can match
> > that of
> > -* the image view is if the render area offset equals (0, 0).
> > -*/
> > -   const bool full_surface_op =
> > - cmd_state->render_area.extent.width == iview->extent.width
> > &&
> > - cmd_state->render_area.extent.height ==
> > iview->extent.height;
> > -   if (full_surface_op)
> > -  assert(cmd_state->render_area.offset.x == 0 &&
> > - cmd_state->render_area.offset.y == 0);
> > +   bool full_surface_op;
> >
> > /* This variable corresponds to the Pixel Dim column in the table
> > below */
> > struct isl_extent2d px_dim;
> > @@ -447,6 +434,21 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> > *cmd_buffer,
> >VK_ATTACHMENT_LOAD_OP_CLEAR)
> >   return;
> >
> > +  /* Section 7.4. of the Vulkan 1.0.27 spec states:
> > +   *
> > +   *   "The render area must be contained within the framebuffer
> > dimensions."
> > +   *
> > +   * Therefore, the only way the extent of the render area can match
> > that of
> > +   * the image view is if the render area offset equals (0, 0).
> > +   */
> > +  full_surface_op =
> > + cmd_state->render_area.extent.width == iview->extent.width
> > &&
> > + cmd_state->render_area.extent.height ==
> > iview->extent.height;
> > +  if (full_surface_op)
> > + assert(cmd_state->render_area.offset.x == 0 &&
> > +cmd_state->render_area.offset.y == 0);
> > +
> > +
> >/* Apply alignment restrictions. Despite the BDW PRM mentioning
> > this is
> > * only needed for a depth buffer surface type of D16_UNORM, testing
> > * showed it to be necessary for other depth formats as well
> > --
> > 2.10.0
> >
> > ___
> > 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

Re: [Mesa-dev] [PATCH 2/5] anv/cmd_buffer: Delay variable assignment in HZ function

[Jason Ekstrand]

My patch to fix partial draws makes us use full_surface_op for HiZ resolves
as well so this patch (and maybe the next one?) are probably moot.

On Oct 10, 2016 3:33 PM, "Nanley Chery"  wrote:

> Delay the assignment of some variables until we know they will be used.
>
> Signed-off-by: Nanley Chery 
> ---
>  src/intel/vulkan/gen8_cmd_buffer.c | 32 +---
>  1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/src/intel/vulkan/gen8_cmd_buffer.c
> b/src/intel/vulkan/gen8_cmd_buffer.c
> index 02c2d7c..e88ab02 100644
> --- a/src/intel/vulkan/gen8_cmd_buffer.c
> +++ b/src/intel/vulkan/gen8_cmd_buffer.c
> @@ -410,7 +410,6 @@ void
>  genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer *cmd_buffer,
>enum blorp_hiz_op op)
>  {
> -   struct anv_cmd_state *cmd_state = _buffer->state;
> const struct anv_image_view *iview =
>anv_cmd_buffer_get_depth_stencil_view(cmd_buffer);
>
> @@ -421,21 +420,9 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> *cmd_buffer,
> if (cmd_buffer->state.pass->subpass_count > 1)
>return;
>
> +   struct anv_cmd_state *cmd_state = _buffer->state;
> const uint32_t ds = cmd_state->subpass->depth_stencil_attachment;
> -
> -   /* Section 7.4. of the Vulkan 1.0.27 spec states:
> -*
> -*   "The render area must be contained within the framebuffer
> dimensions."
> -*
> -* Therefore, the only way the extent of the render area can match
> that of
> -* the image view is if the render area offset equals (0, 0).
> -*/
> -   const bool full_surface_op =
> - cmd_state->render_area.extent.width == iview->extent.width
> &&
> - cmd_state->render_area.extent.height ==
> iview->extent.height;
> -   if (full_surface_op)
> -  assert(cmd_state->render_area.offset.x == 0 &&
> - cmd_state->render_area.offset.y == 0);
> +   bool full_surface_op;
>
> /* This variable corresponds to the Pixel Dim column in the table
> below */
> struct isl_extent2d px_dim;
> @@ -447,6 +434,21 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> *cmd_buffer,
>VK_ATTACHMENT_LOAD_OP_CLEAR)
>   return;
>
> +  /* Section 7.4. of the Vulkan 1.0.27 spec states:
> +   *
> +   *   "The render area must be contained within the framebuffer
> dimensions."
> +   *
> +   * Therefore, the only way the extent of the render area can match
> that of
> +   * the image view is if the render area offset equals (0, 0).
> +   */
> +  full_surface_op =
> + cmd_state->render_area.extent.width == iview->extent.width
> &&
> + cmd_state->render_area.extent.height ==
> iview->extent.height;
> +  if (full_surface_op)
> + assert(cmd_state->render_area.offset.x == 0 &&
> +cmd_state->render_area.offset.y == 0);
> +
> +
>/* Apply alignment restrictions. Despite the BDW PRM mentioning
> this is
> * only needed for a depth buffer surface type of D16_UNORM, testing
> * showed it to be necessary for other depth formats as well
> --
> 2.10.0
>
> ___
> 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