Re: [Intel-gfx] [PATCH V3 2/8] drm/i915/gt: Add support of mocs auxiliary registers programming

["Siddiqui, Ayaz A" <ayaz.siddiqui@intel.com>]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@intel.com>
> Sent: Thursday, September 2, 2021 9:36 PM
> To: Siddiqui, Ayaz A <ayaz.siddiqui@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; S, Srinivasan <srinivasan.s@intel.com>;
> Wilson, Chris P <chris.p.wilson@intel.com>
> Subject: Re: [PATCH V3 2/8] drm/i915/gt: Add support of mocs auxiliary
> registers programming
> 
> On Thu, Sep 02, 2021 at 04:56:18AM -0700, Siddiqui, Ayaz A wrote:
> ...
> > > > +static int check_aux_regs(struct intel_engine_cs *engine,
> > > > +                     const struct drm_i915_aux_table *r,
> > > > +                     u32 **vaddr)
> > >
> > > One other concern (which is part of why I didn't really want to see
> > > this framework handled separately from workarounds) is that the aux
> > > table might tell us to program a register with a specific value, but
> > > we may also have a hardware workaround for a platform/stepping that
> > > overrides that with an alternate value.  Our workaround framework is
> > > smart enough to combine multiple entries for the same register into
> > > a single operation (if the set of bits being updated are different),
> > > or will warn if there's two conflicting sets of programming
> > > requested for certain bits. Right now it's not clear who wins if the
> > > aux table wants to program a register to value 'X' but the
> > > workaround lists want to program the same register to value 'Y.'  In
> > > theory the workaround should overrule the regular programming, but
> > > at the moment these selftests aren't checking to see if that's the
> > > case.  We may not have any such conflicts today (especially since we have
> so few registers that are going to be on the aux table initially), but it may
> come up eventually.
> > Yes its valid point, I did not thought about it. Do you think that
> > moving to workaround will be better option here?
> 
> I think there's a short-term and a long-term aspect here.  My opinion is that
> in the immediate short term we should add these two MOCS-related
> registers (one of which is a context register, one of which is an engine
> register) as additional fake workarounds.  Despite calling them
> "workarounds" that part of the code is already more of a generic "GT register
> override" framework, and we already have a number of things programmed
> there that aren't actually workarounds.  Trying to spin up a completely new
> framework ("aux table") for GT register overrides is going to take a bit more
> time to get right, and I'm not sure we want to hold up the proper MOCS
> programming while that happens (especially since ADL is about to leave
> "force probe required" state and we really don't want to miss the boat on
> getting MOCS programmed correctly before that happens).
> 
> Longer term I do think we want to rework how we handle both formal
> workarounds and non-workaround register overrides in the driver.  That's
> been something I've been meaning to work on for quite a while now, but it
> just keeps getting preempted by higher priority tasks that show up;
> hopefully I can get back to it soon.  But such rework is going to take a bit of
> time, both to get widespread agreement on the redesign, and to do some
> extensive testing to make sure we don't mishandle any corner cases around
> reset handling, execlist vs GuC, etc.  It will also probably happen in multiple
> steps rather than jumping from our current design straight to the final form; I
> don't think it makes sense to make the MOCS programming dependent on
> completion of that long, multi-step process.
> 
> I think one of Chris' concerns about re-using the workaround framework for
> setting these two MOCS-related registers is that the programming would
> wind up getting verified by the workarounds selftest rather than the mocs
> selftest (and thus failures on these specific registers may not get the
> attention they need).  That's true, but if the concern is great enough, I think
> we could make the gt_mocs selftest:
>  - scan the workaround lists and ensure that the two MOCS-related
>    registers truly are present on the appropriate list (if not, error)
>  - check that the register programming still matches the value defined
>    in the workaround (if not, error); this would duplicate the check
>    also done in the workaround selftest, but that's probably fine to
>    have both tests fail if there's a programming problem
>  - lookup the programmed MOCS values in the platform's MOCS table and
>    make sure that they really have the expected characteristics (L3 on
>    platforms going forward, UC on the older platforms that we can't
>    change now for abi compat reasons)
> 
> 
> Matt
Thanks Matt, I have modified that register programming using workaround framework.
I'll share the new series soon.
Since we have already planned to rework on framework so let add category specific
verification in scope of that planned activity instead of adding a temporary
verification in mocs selftest.
Meanwhile programming of values are already verified in workaround.

Regards
-Ayaz


> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795