[Mesa-dev] [PATCH v3 06/13] intel/isl: Add support to emit clear value address.

[Rafael Antognolli]

On Tue, Feb 27, 2018 at 05:00:24PM -0800, Jordan Justen wrote:
> On 2018-02-26 21:35:42, Jason Ekstrand wrote:
> > On Mon, Feb 26, 2018 at 8:12 PM, Jordan Justen <jordan.l.justen at intel.com>
> > wrote:
> > 
> > > On 2018-02-26 17:08:12, Jason Ekstrand wrote:
> > > > On Mon, Feb 26, 2018 at 1:14 PM, Jordan Justen <
> > > jordan.l.justen at intel.com>
> > > > wrote:
> > > >
> > > > > On 2018-02-21 13:45:15, Rafael Antognolli wrote:
> > > > > > +   bool use_clear_address;
> > > > >
> > > > > I'm still wondering about this field. I think at the end we can just a
> > > > > assume that if gen >= 10 and aux_usage != ISL_AUX_USAGE_NONE, then
> > > > > we'll use the address.
> > > > >
> > > >
> > > > That's not going to work if we want to turn this on for blorp, anv, and
> > > > i965 separately.
> > >
> > > I guess this goes to the point I mentioned below. Maybe it make it
> > > easier to break it up for enabling it. (I'm not certain that we
> > > couldn't slice it up another way, but the argument seems fine.)
> > >
> > > But, after that, is it needed? If it's alway enabled when gen >= 10
> > > and aux_usage != ISL_AUX_USAGE_NONE, then once everything is in place,
> > > then isl can easily check for that condition, and there's no purpose
> > > for use_clear_address. Correct?
> > >
> > 
> > I suppose.  Once everything's moved over, there's really no reason to keep
> > it around on gen10.
> > 
> > > I also wonder if clear_address is needed in the info struct. It did
> > > not look like blorp set it by the end of the series, yet blorp was
> > > enabling the feature. (I'm guessing that the reloc must be handing the
> > > aux buf offset for blorp.)
> > >
> > 
> > Yes and no.  It's not really used today but it is needed the moment we get
> > rid of relocations.
> 
> Ah. Good point.
> 
> I guess the pinned address could still be written by the 'reloc'
> function, without emitting the reloc. That would also mean
> clear_address wouldn't be needed in the isl info struct. The name
> 'reloc' starts adding confusion in this case, and maybe something like
> emit_bo_address might make more sense at that point.
> 
> Or, we could make the reloc functions total no-ops with pinned
> addresses, in which case clear_address would be needed.
> 
> Based on this, we might want to decide if patch 7 should be doing
> anything with clear_address... (Right now it doesn't set it.)

If that's the case, then we not only need to set clear_address, but also
address and aux_address (they are also not being set there).

> > >
> > > > > I think you mentioned that it could be tough implement the support in
> > > > > steps if we had an all or nothing enaling of the address usage. But,
> > > > > does that mean that at the end of your series you could add a patch to
> > > > > remove this `use_clear_address` field?
> > > > >
> > > > > Maybe as a test in jenkins, you could add a patch that asserts that if
> > > > > gen >= 10 and there is an aux_buffer, then use_clear_address==true in
> > > > > your current series.
> > > > >
> > > > > -Jordan
> > >