Re: [Mesa-dev] [PATCH v3] anv/blorp: multisample resolve all attachment layers

2018-02-20 Thread Iago Toral 
Hi Nanley,

thanks for having a look at this, you're right that we should use the
framebuffer dimensions to decide on the number of layers to resolve. 

I'll send a new version with the fix.

Iago

On Tue, 2018-02-20 at 15:18 -0800, Nanley Chery wrote:
> On Thu, Feb 15, 2018 at 09:40:16AM +0100, Iago Toral Quiroga wrote:
> > We were only resolving the first.
> > 
> > v2:
> >   - Do not require that the number of layers on dst and src are an
> > exact match, it is okay if the dst has more layers so long as
> > it has at least the same that we are going to resolve.
> >   - Do not always resolve array_len layers, we should resolve
> > only from base_array_layer to array_len.
> > 
> > v3:
> >   - v2 was assuming that array_len represented the total number of
> > layers in the image, but it represents the number of layers
> > starting at the base array ayer.
> > 
> > Fixes new CTS tests for multisampled layered rendering:
> > dEQP-VK.renderpass.multisample_resolve.layers_*
> > ---
> >  src/intel/vulkan/anv_blorp.c | 30 +++---
> >  1 file changed, 19 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/intel/vulkan/anv_blorp.c
> > b/src/intel/vulkan/anv_blorp.c
> > index d38b343671..df566773a4 100644
> > --- a/src/intel/vulkan/anv_blorp.c
> > +++ b/src/intel/vulkan/anv_blorp.c
> > @@ -1543,25 +1543,33 @@ anv_cmd_buffer_resolve_subpass(struct
> > anv_cmd_buffer *cmd_buffer)
> >   get_blorp_surf_for_anv_image(cmd_buffer->device,
> > dst_iview->image,
> >VK_IMAGE_ASPECT_COLOR_BIT,
> >dst_aux_usage, _surf);
> > +
> > + uint32_t base_src_layer = src_iview-
> > >planes[0].isl.base_array_layer;
> > + uint32_t base_dst_layer = dst_iview-
> > >planes[0].isl.base_array_layer;
> > + uint32_t num_layers = src_iview->planes[0].isl.array_len;
> 
> num_layers should be equal to fb->layers. As seen in the definition
> of
> renderArea in the Vulkan spec, resolve operations are limited to the
> renderArea, which extends to all layers of the framebuffer.
> 
>renderArea is the render area that is affected by the render pass
>instance. The effects of attachment load, store and multisample
> resolve
>operations are restricted to the pixels whose x and y coordinates
> fall
>within the render area on all attachments. The render area extends
> to
>all layers of framebuffer.
> 
> > + assert(num_layers <= dst_iview->planes[0].isl.array_len);
> > +
> 
> This assertion is false. The spec allows having an arrayed
> multisample
> source image view and a non-arrayed single-sampled destination image
> view as long as the framebuffer is non-arrayed.
> 
>Each element of pAttachments must have dimensions at least as
> large as
>the corresponding framebuffer dimension
> 
> -Nanley
> 
> >   anv_cmd_buffer_mark_image_written(cmd_buffer, dst_iview-
> > >image,
> > VK_IMAGE_ASPECT_COLOR_B
> > IT,
> > dst_surf.aux_usage,
> > dst_iview-
> > >planes[0].isl.base_level,
> > -   dst_iview-
> > >planes[0].isl.base_array_layer, 1);
> > +   base_dst_layer,
> > num_layers);
> >  
> >   assert(!src_iview->image->format->can_ycbcr);
> >   assert(!dst_iview->image->format->can_ycbcr);
> >  
> > - resolve_surface(,
> > - _surf,
> > - src_iview->planes[0].isl.base_level,
> > - src_iview-
> > >planes[0].isl.base_array_layer,
> > - _surf,
> > - dst_iview->planes[0].isl.base_level,
> > - dst_iview-
> > >planes[0].isl.base_array_layer,
> > - render_area.offset.x,
> > render_area.offset.y,
> > - render_area.offset.x,
> > render_area.offset.y,
> > - render_area.extent.width,
> > render_area.extent.height);
> > + for (uint32_t i = 0; i < num_layers; i++) {
> > +resolve_surface(,
> > +_surf,
> > +src_iview->planes[0].isl.base_level,
> > +base_src_layer + i,
> > +_surf,
> > +dst_iview->planes[0].isl.base_level,
> > +base_dst_layer + i,
> > +render_area.offset.x,
> > render_area.offset.y,
> > +render_area.offset.x,
> > render_area.offset.y,
> > +render_area.extent.width,
> > render_area.extent.height);
> > + }
> >}
> >  
> >blorp_batch_finish();
> > -- 
> > 2.14.1
> > 
> > ___
> > mesa-dev mailing

Re: [Mesa-dev] [PATCH v3] anv/blorp: multisample resolve all attachment layers

2018-02-20 Thread Nanley Chery 
On Thu, Feb 15, 2018 at 09:40:16AM +0100, Iago Toral Quiroga wrote:
> We were only resolving the first.
> 
> v2:
>   - Do not require that the number of layers on dst and src are an
> exact match, it is okay if the dst has more layers so long as
> it has at least the same that we are going to resolve.
>   - Do not always resolve array_len layers, we should resolve
> only from base_array_layer to array_len.
> 
> v3:
>   - v2 was assuming that array_len represented the total number of
> layers in the image, but it represents the number of layers
> starting at the base array ayer.
> 
> Fixes new CTS tests for multisampled layered rendering:
> dEQP-VK.renderpass.multisample_resolve.layers_*
> ---
>  src/intel/vulkan/anv_blorp.c | 30 +++---
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> index d38b343671..df566773a4 100644
> --- a/src/intel/vulkan/anv_blorp.c
> +++ b/src/intel/vulkan/anv_blorp.c
> @@ -1543,25 +1543,33 @@ anv_cmd_buffer_resolve_subpass(struct anv_cmd_buffer 
> *cmd_buffer)
>   get_blorp_surf_for_anv_image(cmd_buffer->device, dst_iview->image,
>VK_IMAGE_ASPECT_COLOR_BIT,
>dst_aux_usage, _surf);
> +
> + uint32_t base_src_layer = src_iview->planes[0].isl.base_array_layer;
> + uint32_t base_dst_layer = dst_iview->planes[0].isl.base_array_layer;
> + uint32_t num_layers = src_iview->planes[0].isl.array_len;

num_layers should be equal to fb->layers. As seen in the definition of
renderArea in the Vulkan spec, resolve operations are limited to the
renderArea, which extends to all layers of the framebuffer.

   renderArea is the render area that is affected by the render pass
   instance. The effects of attachment load, store and multisample resolve
   operations are restricted to the pixels whose x and y coordinates fall
   within the render area on all attachments. The render area extends to
   all layers of framebuffer.

> + assert(num_layers <= dst_iview->planes[0].isl.array_len);
> +

This assertion is false. The spec allows having an arrayed multisample
source image view and a non-arrayed single-sampled destination image
view as long as the framebuffer is non-arrayed.

   Each element of pAttachments must have dimensions at least as large as
   the corresponding framebuffer dimension

-Nanley

>   anv_cmd_buffer_mark_image_written(cmd_buffer, dst_iview->image,
> VK_IMAGE_ASPECT_COLOR_BIT,
> dst_surf.aux_usage,
> 
> dst_iview->planes[0].isl.base_level,
> -   
> dst_iview->planes[0].isl.base_array_layer, 1);
> +   base_dst_layer, num_layers);
>  
>   assert(!src_iview->image->format->can_ycbcr);
>   assert(!dst_iview->image->format->can_ycbcr);
>  
> - resolve_surface(,
> - _surf,
> - src_iview->planes[0].isl.base_level,
> - src_iview->planes[0].isl.base_array_layer,
> - _surf,
> - dst_iview->planes[0].isl.base_level,
> - dst_iview->planes[0].isl.base_array_layer,
> - render_area.offset.x, render_area.offset.y,
> - render_area.offset.x, render_area.offset.y,
> - render_area.extent.width, 
> render_area.extent.height);
> + for (uint32_t i = 0; i < num_layers; i++) {
> +resolve_surface(,
> +_surf,
> +src_iview->planes[0].isl.base_level,
> +base_src_layer + i,
> +_surf,
> +dst_iview->planes[0].isl.base_level,
> +base_dst_layer + i,
> +render_area.offset.x, render_area.offset.y,
> +render_area.offset.x, render_area.offset.y,
> +render_area.extent.width, 
> render_area.extent.height);
> + }
>}
>  
>blorp_batch_finish();
> -- 
> 2.14.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 v3] anv/blorp: multisample resolve all attachment layers

2018-02-15 Thread Iago Toral Quiroga 
We were only resolving the first.

v2:
  - Do not require that the number of layers on dst and src are an
exact match, it is okay if the dst has more layers so long as
it has at least the same that we are going to resolve.
  - Do not always resolve array_len layers, we should resolve
only from base_array_layer to array_len.

v3:
  - v2 was assuming that array_len represented the total number of
layers in the image, but it represents the number of layers
starting at the base array ayer.

Fixes new CTS tests for multisampled layered rendering:
dEQP-VK.renderpass.multisample_resolve.layers_*
---
 src/intel/vulkan/anv_blorp.c | 30 +++---
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
index d38b343671..df566773a4 100644
--- a/src/intel/vulkan/anv_blorp.c
+++ b/src/intel/vulkan/anv_blorp.c
@@ -1543,25 +1543,33 @@ anv_cmd_buffer_resolve_subpass(struct anv_cmd_buffer 
*cmd_buffer)
  get_blorp_surf_for_anv_image(cmd_buffer->device, dst_iview->image,
   VK_IMAGE_ASPECT_COLOR_BIT,
   dst_aux_usage, _surf);
+
+ uint32_t base_src_layer = src_iview->planes[0].isl.base_array_layer;
+ uint32_t base_dst_layer = dst_iview->planes[0].isl.base_array_layer;
+ uint32_t num_layers = src_iview->planes[0].isl.array_len;
+ assert(num_layers <= dst_iview->planes[0].isl.array_len);
+
  anv_cmd_buffer_mark_image_written(cmd_buffer, dst_iview->image,
VK_IMAGE_ASPECT_COLOR_BIT,
dst_surf.aux_usage,
dst_iview->planes[0].isl.base_level,
-   
dst_iview->planes[0].isl.base_array_layer, 1);
+   base_dst_layer, num_layers);
 
  assert(!src_iview->image->format->can_ycbcr);
  assert(!dst_iview->image->format->can_ycbcr);
 
- resolve_surface(,
- _surf,
- src_iview->planes[0].isl.base_level,
- src_iview->planes[0].isl.base_array_layer,
- _surf,
- dst_iview->planes[0].isl.base_level,
- dst_iview->planes[0].isl.base_array_layer,
- render_area.offset.x, render_area.offset.y,
- render_area.offset.x, render_area.offset.y,
- render_area.extent.width, render_area.extent.height);
+ for (uint32_t i = 0; i < num_layers; i++) {
+resolve_surface(,
+_surf,
+src_iview->planes[0].isl.base_level,
+base_src_layer + i,
+_surf,
+dst_iview->planes[0].isl.base_level,
+base_dst_layer + i,
+render_area.offset.x, render_area.offset.y,
+render_area.offset.x, render_area.offset.y,
+render_area.extent.width, 
render_area.extent.height);
+ }
   }
 
   blorp_batch_finish();
-- 
2.14.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev