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

[Nanley Chery]

On Thu, Feb 22, 2018 at 08:23:30AM +0100, Iago Toral wrote:
> On Wed, 2018-02-21 at 09:58 -0800, Nanley Chery wrote:
> > On Wed, Feb 21, 2018 at 09:18:49AM +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.
> > > 
> > > v4:
> > >  - The number of layers to resolve should be taken from the
> > >    framebuffer (Nanley).
> > > 
> > > Fixes new CTS tests for multisampled layered rendering:
> > > dEQP-VK.renderpass.multisample_resolve.layers_*
> > > ---
> > >  src/intel/vulkan/anv_blorp.c | 31 ++++++++++++++++++++-----------
> > >  1 file changed, 20 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/src/intel/vulkan/anv_blorp.c
> > > b/src/intel/vulkan/anv_blorp.c
> > > index bee51e0cdf..efa2ced7f2 100644
> > > --- a/src/intel/vulkan/anv_blorp.c
> > > +++ b/src/intel/vulkan/anv_blorp.c
> > > @@ -1329,25 +1329,34 @@ anv_cmd_buffer_resolve_subpass(struct
> > > anv_cmd_buffer *cmd_buffer)
> > >                                        VK_IMAGE_ASPECT_COLOR_BIT,
> > >                                        ANV_IMAGE_LAYOUT_EXPLICIT_AU
> > > X,
> > >                                        dst_aux_usage, &dst_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;
> > > +
> > 
> > I'm not sure we benefit from storing these fields in new variables,
> > but
> > it's not a big issue. 
> > With or without the new variables, this patch is
> > Reviewed-by: Nanley Chery <nanley.g.chery at intel.com>
> 
> Thanks Nanley.
> 
> I don't have a strong reason to keep the variables but I prefer to keep
> them for two reasons: without them the call to 
> anv_cmd_buffer_mark_image_written was stretching a bit over 80
> characters long and I also find it easier to read something like
> 'base_dst_layer' than something like 'dst_iview-
> >planes[0].isl.base_array_layer' when browsing the code.
> 

Oh, I wasn't aware of the 80-char limit being passed. Thanks for letting
me know.

-Nanley

> > 
> > > +         assert(src_iview->planes[0].isl.array_len >= fb->layers);
> > > +         assert(dst_iview->planes[0].isl.array_len >= fb->layers);
> > > +
> > >           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, fb-
> > > >layers);
> > >  
> > >           assert(!src_iview->image->format->can_ycbcr);
> > >           assert(!dst_iview->image->format->can_ycbcr);
> > >  
> > > -         resolve_surface(&batch,
> > > -                         &src_surf,
> > > -                         src_iview->planes[0].isl.base_level,
> > > -                         src_iview-
> > > >planes[0].isl.base_array_layer,
> > > -                         &dst_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 < fb->layers; i++) {
> > > +            resolve_surface(&batch,
> > > +                            &src_surf,
> > > +                            src_iview->planes[0].isl.base_level,
> > > +                            base_src_layer + i,
> > > +                            &dst_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(&batch);
> > > -- 
> > > 2.14.1
> > > 
> > 
> >