Re: [Intel-xe] [RFC] drm/xe: A minimal assert for some forcewake domain on xe_mmio.

[Matthew Brost <matthew.brost@intel.com>]

On Mon, May 22, 2023 at 08:28:05PM -0700, Matt Roper wrote:
> On Mon, May 22, 2023 at 06:15:27PM -0400, Rodrigo Vivi wrote:
> > It is the maximum protection we can do with the current infrastructure.
> > 
> > Cc: John Harrison <John.C.Harrison@Intel.com>
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > Cc: Francois Dugast <francois.dugast@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > 
> > 
> > RFC
> > ===
> > 
> > Okay, so, this is more an RFC to brainstorm the future of the force_wake in
> > Xe than anything else.
> > 
> > On i915 the force_wake is built-in the mmio functions at uncore component.
> > 
> > With that approach we had few historical issues iirc:
> > 
> > 1. Display performance with vblank evasion that requested uncore to provide
> > the 'fw' variantes that are actually the ones that avoid fw (contrary to what
> > the name suggests).
> 
> In i915 there were more differences between fw and non-fw variants
> of register functions than just forcewake handling:
> 
>  * _fw() functions assumed that the caller was already holding
>    forcewake, whereas the non-fw functions would obtain it themselves
>  * _fw() functions also assumed that the caller was already holding
>    uncore->lock, whereas the non-fw functions would obtain and release
>    the lock around each register access
>  * _fw() functions do no tracing and no debug assertions
>  * _fw() functions do not check for unclaimed MMIO
> 
> I don't think the first bullet there (forcewake) really mattered much
> from a performance perspective.  For display registers, a quick binary
> search of the FW table (just a handful of CPU cycles) would quickly
> determine that no forcewake domains were needed for those register
> offsets, so we wouldn't be doing anything with forcewake at the hardware
> level at all.  For display registers, the more performance-relevant
> aspects of using the _fw() functions was doing all your register
> accesses together without contention with other MMIO work.  I.e.,
> holding the uncore lock over an entire set of registers rather than
> grabbing/releasing it for each one, and (for vblank evasion
> specifically) doing it all while interrupts were disabled.  For debug
> drivers, there was also a bunch of other stuff that the _fw() functions
> bypassed (e.g., tripling of the number of register accesses due to
> reading FPGA_DBG before/after each register to look for unclaimed
> accesses).
> 
> > 
> > 2. Missed ranges updates. Sometimes we messed up with the ranges, there were
> > other times that the spec was updated and we didn't get the notification, and
> > there were cases that the BSpec had bugs.
> > 
> > For these reasons in Xe we have decided to let to the caller the
> > responsibility to set the force_wake bits for their domains before doing
> > the MMIO.
> 
> I don't think #2 was a reason to skip forcewake in Xe.  Not noticing a
> bspec update (or the bspec itself having incorrect information) would
> lead to even more mistakes if the caller has to explicitly grab the
> appropriate domains than if the driver does it implicitly.  The explicit
> handling only helps in the subset of cases where we blindly grab all of
> the domains (as we do during part of init).
> 
> 

MMIO access that requires forcewakes really only should be in a few
places, off the top of my head:

1. Init
2. Reset
3. Sysfs / Debugfs

In all of these cases I think it fine to blindly grab all forcewakes.

Please chime in if I'm missing something.

> > 
> > However I'm seeing many questions and doubts popping up:
> > 
> > 1. Are we confident that we are not missing any wake-up?
> > 2. Are we confident that the domains are set correctly?
> > 3. Are we not wasting power if we are waking up ALL the domains instead
> >    of some specific one?
> 
> I think we're only holding ALL domains during init; for most of the
> runtime MMIO accesses, I think the code is currently attempting to only
> grab the domain(s) that it thinks the registers it's accessing will
> need.  Although that might be working right now, I'm a little bit
> worried about how that will scale in the long term when a single
> register might be in several different domains depending on what
> platform you're running on.  There have definitely been cases in the
> past where groups of registers migrated from RENDER to GT or vice versa
> between platforms, so the exact domain you needed for an operation
> varied by platform.  And things are even more complicated if you're
> doing any MMIO against the media, since the hardware seems to change
> exactly how it splits up the media power wells somewhat often.
>
> > 4. What about the display disconnection now because i915 and xe have different
> >    mmio approaches but reusing i915-display?
> > 
> > It looks to me that the cons of the current approach are superseding the
> > cons of the i915 approach. But I want to hear more thoughts here before
> > we decide which route to take.
> > 
> > Maybe we have that domain as part of the mmio calls themselves? Maybe
> > a double approach where the caller is responsible but mmio has the range
> > information and double check it?
> 
> As noted above, part of the challenge with forcewake is that even if the
> caller knows it wants to access register FOO, and even if it know FOO is
> a GT register that likely needs forcewake, it's sometimes challenging to
> make sure it's grabbing the correct domain(s) for every single platform
> the Xe driver will eventually support if the power management handling
> changes.  I think that was part of the motivation for encoding the
> tables into the driver in i915.  It seems like GT power wells don't
> change as much these days as they used to, but it's hard to say whether
> that will continue in the future or not.  Who knows...maybe they'll
> eventually start creating dedicated domains for stuff like blitters,
> GuC, etc.  rather than lumping all of those into the "GT" catchall
> domain.
> 
> If we do decide to go back to implicit forcewake handling with the table
> encoded into the driver, it might be worth doing something sort of like

Let's not do this intel_uncore.c is an unreadable mess, I don't want
anything like this in Xe.

> what Lucas is doing in the OOB workaround series --- drop the
> per-platform tables into a human-readable text file that's more similar
> to the format used by the bspec (exact ranges, forcewake domain, MCR
> replication type, etc.) and then provide a small parser program that
> will convert that into actual code (and do things like consolidating
> adjacent ranges).
> 
> > 
> > Any other idea? Thoughts?
> 
> Once the big GT vs tile refactor stuff I have in flight gets finalized,
> I plan to follow up with another series that creates a more appropriate
> MMIO target for register operations rather than using "xe_gt" as the
> target (even for things completely unrelated to the small GT subset of
> hardware).  My idea is that you'd grab an MMIO target structure for MMIO
> operations against a specific hardware unit, and then the info inside
> the MMIO structure would be able to figure out if there are additional
> checks and/or operations it should perform.  E.g.,
> 
>  * mmio = xe_mmio_for_device(xe_device *xe);
>     - Used to submit MMIO operations against the root tile of the PCI
>       device.  Only used during init (e.g., to read the registers that
>       tell us which tiles exist on the platform) and during top-level
>       interrupt enable/disable (since all interrupts are routed through
>       the root tile).
>     - No forcewake needed for register accesses through a handle of this
>       type since you'd only ever be accessing sgunit registers for these
>       types of things.
>     - Register accesses through mmio can warn on debug builds if the
>       register appears to be in a GT-related MMIO range.
> 
>  * mmio = xe_mmio_for_display(xe_device *xe);
>     - Pretty much the same as handles returned by xe_mmio_for_device(),
>       but if a handle of this type is used to try to read/write
>       registers outside the display range, we could have debug builds
>       throw some extra warnings.
>     - Unclaimed register detection could be confined to accesses through
>       these handles.
> 
>  * mmio = xe_mmio_for_tile(xe_tile *tile);
>     - Used to access non-GT registers that reside in a specific tile.
>       I.e., sgunit/soc registers.
>     - As above, no forcewake needed, can make MMIO operations warn
>       if used to access a GT range.
> 
>  * mmio = xe_mmio_for_gt(xe_gt *gt);
>     - Used to access GT registers in a specific GT
>     - Does automatic GSI offset translation for media GTs
>     - Can either do automatic forcewake like i915 does, or can do debug
>       check+warn like you have here.
>     - Can make MMIO operations warn if MMIO offset is outside GT range
>     - Can also trigger warnings if a GT non-GSI, non-media engine
>       register is accessed from an MMIO obtained from a media GT.
>

Ack on this if we keep in managable, worried code / feature bloat those
that will result in a similar file to intel_uncore.c in Xe.

Matt

> At some point we'll probably want to add some debug tracepoints for MMIO
> access to assist with debugging; the mmio structure can also help
> provide meaningful output for each type of MMIO handle as well.
> 
> 
> One other MMIO-related thing:
> At the moment Xe does no locking of MMIO access.  I know there were
> hardware lockups on old platforms if simultaneous MMIO accesses
> happened, but I've never seen that tied to a specific HSD ticket that
> would let us know whether modern platforms are still susceptible or not.
> If it turns out they are and that we need to bring in the equivalent of
> i915's uncore lock (and figure out if any register access is
> problematic, or only certain types/ranges).
> 
> 
> Matt
> 
> > 
> > Thanks,
> > Rodrigo.
> > 
> >  drivers/gpu/drm/xe/xe_mmio.h | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_mmio.h b/drivers/gpu/drm/xe/xe_mmio.h
> > index 1407f1189b0d..6302ca85e3f4 100644
> > --- a/drivers/gpu/drm/xe/xe_mmio.h
> > +++ b/drivers/gpu/drm/xe/xe_mmio.h
> > @@ -18,8 +18,26 @@ struct xe_device;
> >  
> >  int xe_mmio_init(struct xe_device *xe);
> >  
> > +static inline void xe_mmio_assert_forcewake(struct xe_gt *gt,
> > +					    struct xe_reg reg)
> > +{
> > +	struct xe_force_wake *fw = &gt->mmio.fw;
> > +
> > +	/* No need for forcewake in this range */
> > +	if (reg.addr >= 0x40000 && reg.addr < 0x116000)
> > +		return;
> > +
> > +	/*
> > +	 * XXX: Weak. It checks for some domain, but impossible to determine
> > +	 * if the right one is selected, or if it was set by the same caller
> > +	 */
> > +	WARN_ON(!fw->awake_domains);
> > +}
> > +
> >  static inline u8 xe_mmio_read8(struct xe_gt *gt, struct xe_reg reg)
> >  {
> > +	xe_mmio_assert_forcewake(gt, reg);
> > +
> >  	if (reg.addr < gt->mmio.adj_limit)
> >  		reg.addr += gt->mmio.adj_offset;
> >  
> > @@ -29,6 +47,8 @@ static inline u8 xe_mmio_read8(struct xe_gt *gt, struct xe_reg reg)
> >  static inline void xe_mmio_write32(struct xe_gt *gt,
> >  				   struct xe_reg reg, u32 val)
> >  {
> > +	xe_mmio_assert_forcewake(gt, reg);
> > +
> >  	if (reg.addr < gt->mmio.adj_limit)
> >  		reg.addr += gt->mmio.adj_offset;
> >  
> > @@ -37,6 +57,8 @@ static inline void xe_mmio_write32(struct xe_gt *gt,
> >  
> >  static inline u32 xe_mmio_read32(struct xe_gt *gt, struct xe_reg reg)
> >  {
> > +	xe_mmio_assert_forcewake(gt, reg);
> > +
> >  	if (reg.addr < gt->mmio.adj_limit)
> >  		reg.addr += gt->mmio.adj_offset;
> >  
> > @@ -58,6 +80,8 @@ static inline u32 xe_mmio_rmw32(struct xe_gt *gt, struct xe_reg reg, u32 clr,
> >  static inline void xe_mmio_write64(struct xe_gt *gt,
> >  				   struct xe_reg reg, u64 val)
> >  {
> > +	xe_mmio_assert_forcewake(gt, reg);
> > +
> >  	if (reg.addr < gt->mmio.adj_limit)
> >  		reg.addr += gt->mmio.adj_offset;
> >  
> > @@ -66,6 +90,8 @@ static inline void xe_mmio_write64(struct xe_gt *gt,
> >  
> >  static inline u64 xe_mmio_read64(struct xe_gt *gt, struct xe_reg reg)
> >  {
> > +	xe_mmio_assert_forcewake(gt, reg);
> > +
> >  	if (reg.addr < gt->mmio.adj_limit)
> >  		reg.addr += gt->mmio.adj_offset;
> >  
> > -- 
> > 2.39.2
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation