[Mesa-dev] [PATCH 1/2] anv/gen9+: Initialize new fields in STATE_BASE_ADDRESS

[Rafael Antognolli]

On Wed, Oct 10, 2018 at 05:00:33PM -0700, Jordan Justen wrote:
> On 2018-10-10 14:38:23, Rafael Antognolli wrote:
> > On Wed, Oct 10, 2018 at 02:04:11PM -0700, Jordan Justen wrote:
> > > On 2018-10-10 13:45:13, Rafael Antognolli wrote:
> > > > On Wed, Oct 10, 2018 at 01:39:25PM -0700, Jordan Justen wrote:
> > > > > Ref: 263b584d5e4 "i965/skl: Emit extra zeros in STATE_BASE_ADDRESS on Skylake."
> > > > > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > > > > ---
> > > > >  src/intel/vulkan/genX_cmd_buffer.c | 12 ++++++++++++
> > > > >  1 file changed, 12 insertions(+)
> > > > > 
> > > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c
> > > > > index c3a7e5c83c3..43a02f22567 100644
> > > > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > > > @@ -121,6 +121,18 @@ genX(cmd_buffer_emit_state_base_address)(struct anv_cmd_buffer *cmd_buffer)
> > > > >        sba.IndirectObjectBufferSizeModifyEnable  = true;
> > > > >        sba.InstructionBufferSize                 = 0xfffff;
> > > > >        sba.InstructionBuffersizeModifyEnable     = true;
> > > > > +#  endif
> > > > > +#  if (GEN_GEN >= 9)
> > > > > +      sba.BindlessSurfaceStateBaseAddress = (struct anv_address) { NULL, 0 };
> > > > > +      sba.BindlessSurfaceStateMemoryObjectControlState = GENX(MOCS);
> > > > > +      sba.BindlessSurfaceStateBaseAddressModifyEnable = true;
> > > > > +      sba.BindlessSurfaceStateSize = 0;
> > > > > +#  endif
> > > > > +#  if (GEN_GEN >= 10)
> > > > > +      sba.BindlessSamplerStateBaseAddress = (struct anv_address) { NULL, 0 };
> > > > > +      sba.BindlessSamplerStateMemoryObjectControlState = GENX(MOCS);
> > > > > +      sba.BindlessSamplerStateBaseAddressModifyEnable = true;
> > > > > +      sba.BindlessSamplerStateBufferSize = 0;
> > > > 
> > > > Do we really need to set all of these fields? AFAIK the ones we don't
> > > > set should be left as 0's anyway, so at least the Address and BufferSize
> > > > should be fine to be left out. I think the MOCS field should be fine
> > > > too, since we are not setting any pointer here. Unless you want to
> > > > be really explicit...
> > > 
> > > Yeah. I don't know that it is helpful since the genxml already sets
> > > the packet length, and I guess things should be zero by default. Maybe
> > > it will make it a little easier to find for bindless in the future?
> > > 
> > > Regarding Jason's comment about the enable bit, I was following Ken's
> > > referenced commit (263b584d5e4) for the similar field in gen9+ on
> > > i965. Maybe it is good to actually force the write to explicitly set
> > > the size to 0?
> > 
> > Yeah, my understanding is that we should set the "modify" bit, so it
> > will actually set the address and size to 0.
> > 
> > > I guess setting MOCS does not follow what Ken did in i965.
> > > 
> > > If we actually do want to set the enable bit, then it might be good to
> > > also leave the fields being explicitly set to zero.
> > > 
> > > My preference would be to just set the fields explicitly. Since we
> > > only specify this packet in one place, it doesn't seem like it adds
> > > too much verbosity.
> > 
> > On most of the genxml code I've seen, we only set the fields that are
> > not zeroed by default.
> 
> I think there are exceptions:
> 
> $ git grep -Ee "\..* = 0;" src/intel/vulkan/genX_*
> 
> I think the rule is more like: if setting the field to 0 is notable,
> then it's better to explicitly set it for informational purposes.
> 
> If the 'enable' bit is set, then I think think the fields that will be
> updated are notable. If the 'enable' bit was not set, then maybe the
> fields are not important. (In that case, perhaps the patch should be
> dropped entirely.)

OK, that's indeed a good explanation.

> Anyway, if Jason doesn't have any further input, I'll go with your
> suggestion of dropping the zeroed fields.

I'm fine with either way, as long as we set the modify enable bit.

> -Jordan
> 
> > And if the name of these fields change or
> > something in a future generation, assuming we are still not using them,
> > it's easier to just change the xml for that gen.
> > 
> > So to keep the code consistent with the rest, I would leave it out, but
> > regardless of what you choose,
> > 
> > Reviewed-by: Rafael Antognolli <rafael.antognolli at intel.com>