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

[Jordan Justen]

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.)

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

-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>