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.jus...@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.antogno...@intel.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
