[Mesa-dev] [PATCH 01/14] anv/cmd_buffer: Advance the address when initializing clear colors

[Nanley Chery]

On Tue, Nov 14, 2017 at 02:30:31PM +0000, Lionel Landwerlin wrote:
> On 13/11/17 22:07, Jason Ekstrand wrote:
> > On Mon, Nov 13, 2017 at 1:30 PM, Nanley Chery <nanleychery at gmail.com
> > <mailto:nanleychery at gmail.com>> wrote:
> > 
> >     On Mon, Nov 13, 2017 at 08:12:41AM -0800, Jason Ekstrand wrote:
> >     > Found by inspection
> >     >
> > 
> >     Good catch.
> > 
> >     > Cc: mesa-stable at lists.freedesktop.org
> >     <mailto:mesa-stable at lists.freedesktop.org>
> >     > ---
> >     >  src/intel/vulkan/genX_cmd_buffer.c | 9 ++++++---
> >     >  1 file changed, 6 insertions(+), 3 deletions(-)
> >     >
> >     > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> >     b/src/intel/vulkan/genX_cmd_buffer.c
> >     > index fbb5706..2564976 100644
> >     > --- a/src/intel/vulkan/genX_cmd_buffer.c
> >     > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> >     > @@ -557,12 +557,13 @@ init_fast_clear_state_entry(struct
> >     anv_cmd_buffer *cmd_buffer,
> >     >     /* Other combinations of auxiliary buffers and platforms
> >     require specific
> >     >      * values in the clear value dword(s).
> >     >      */
> >     > +   struct anv_address addr =
> >     > +      get_fast_clear_state_address(cmd_buffer->device, image,
> >     aspect, level,
> >     > +  FAST_CLEAR_STATE_FIELD_CLEAR_COLOR);
> >     >     unsigned i = 0;
> >     >     for (; i < cmd_buffer->device->isl_dev.ss.clear_value_size;
> >     i += 4) {
> >     >        anv_batch_emit(&cmd_buffer->batch,
> >     GENX(MI_STORE_DATA_IMM), sdi) {
> >     > -         sdi.Address =
> >     > -            get_fast_clear_state_address(cmd_buffer->device,
> >     image, aspect, level,
> >     > -  FAST_CLEAR_STATE_FIELD_CLEAR_COLOR);
> >     > +         sdi.Address = addr;
> > 
> >     The loop increments the variable i by 4 with every iteration. How
> >     about
> >     the following instead:
> >                 sdi.Address = addr + i;
> > 
> > 
> > I really wish we could do that but it's a struct.  I could do
> > 

Whoops.

> > sdi.Address = addr;
> > sdi.Address.offset += i;
> > 

That looks good to me. No worries if you choose to go with the fix you
mentioned below. With one of these fixes, this patch is
Reviewed-by: Nanley Chery <nanley.g.chery at intel.com>

> > --Jason
> > 
> >     -Nanley
> > 
> >     >
> >     >           if (GEN_GEN >= 9) {
> >     >              /* MCS buffers on SKL+ can only have 1/0 clear
> >     colors. */
> >     > @@ -586,6 +587,8 @@ init_fast_clear_state_entry(struct
> >     anv_cmd_buffer *cmd_buffer,
> >     >              sdi.ImmediateData = 0;
> >     >           }
> >     >        }
> >     > +
> >     > +      addr += 4;
> > 
> > 
> > Aparently, I didn't compile-test this because I need a .offset here. :/
> 
> Heh, I was confused too :)
> With that fixed :
> 
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> 
> >     >     }
> >     >  }
> >     >
> >     > --
> >     > 2.5.0.400.gff86faf
> >     >
> >     > _______________________________________________
> >     > mesa-dev mailing list
> >     > mesa-dev at lists.freedesktop.org
> >     <mailto:mesa-dev at lists.freedesktop.org>
> >     > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >     <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
> > 
> > 
> > 
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>