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

[Jordan Justen]

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?

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.

-Jordan