[PATCH v1] drm/xe: Rework throttle ABI

[Matt Roper]

On Fri, Oct 25, 2024 at 06:07:40PM -0500, Lucas De Marchi wrote:
> +Sujaritha
> 
> Rodrigo was already Cc'ed, but please take a look below.
> 
> On Fri, Oct 25, 2024 at 01:45:34PM -0700, Matt Roper wrote:
> > On Fri, Oct 25, 2024 at 10:04:14PM +0300, Raag Jadav wrote:
> > > On Fri, Oct 25, 2024 at 10:03:41AM -0700, Matt Roper wrote:
> > > > On Fri, Oct 25, 2024 at 02:52:38PM +0530, Raag Jadav wrote:
> > > > > Current implementation adds multiple sysfs entries for getting GT
> > > > > throttle status and its reasons, which forces the user to read multiple
> > > > > entries for evaluating the result. Since output of each entry is based
> > > > > on the same underlying hardware register and considering the access type
> > > > > of this register is RO/v, the value of this register can change at any
> > > > > given point, even between subsequent sysfs reads. This makes current
> > > > > implementation fundamentally flawed which can produce inconsistent results.
> > > > >
> > > > > Rework throttle ABI and introduce throttle_status attribute which will
> > > > > provide throttle status through a oneshot register read, making it
> > > > > relatively less error prone. The new ABI will provide throttle reasons
> > > > > in a string based list based on the respective bits which are set in the
> > > > > hardware. Empty output means no bits are set and hence no throttling.
> > > >
> > > > But the old ABI is already released, and we have platforms with
> > > > force_probe lifted already.  Presumably userspace software is already
> > > > using the existing interface (otherwise it never would have been allowed
> > > > to land upstream), so that means we can't change it in incompatible ways
> > > > anymore; we're locked into supporting the current interface forever on
> > > > these platforms.
> > > >
> > > > We can change the ABI for _future_ platforms if it makes sense, but it's
> > > > too late to make compatibility-breaking changes for LNL and BMG.
> > > 
> > > It landed upstream pretty recently AFAICT, atleast in xe.
> > 
> > It landed upstream about 10 months ago:
> > 
> >        commit 1c8e9019033728093c04608f44c6e87fec6822e1
> >        Author:     Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
> >        AuthorDate: Fri Dec 8 00:11:52 2023 -0500
> >        Commit:     Rodrigo Vivi <rodrigo.vivi at intel.com>
> >        CommitDate: Thu Dec 21 11:45:28 2023 -0500
> > 
> >            drm/xe: Add frequency throttle reasons sysfs attributes
> > 
> > and has been part of the 6.8 kernel onward.  So this isn't a case where
> > something just recently landed in drm-tip and hasn't made its way into a
> > formal kernel.org release yet.
> > 
> > > So maybe worth reconsidering.
> > 
> > It's not really a decision that we get to make.  One of the absolute
> > rules of Linux kernel development is that we cannot break
> > kernel<->userspace ABI that's being used.  A user must always be able to
> > safely upgrade their kernel without updating any other software on the
> > system without suffering breakage/regressions (with slight exceptions
> > for special security-related cases).  In this case, if you remove the
> > existing sysfs APIs and try to replace them with something different,
> > then it will immediately break existing tools that were using the old
> > interface.  That's simply not allowed by Linux kernel rules.
> > 
> > > 
> > > > >
> > > > > $ cat /sys/devices/.../tile0/gt0/freq0/throttle_status
> > > > > prochot
> > > > > thermal
> > > > > ratl
> > > > > thermalert
> > > > > tdc
> > > > > pl4
> > > > > pl1
> > > > > pl2
> > > > >
> > > > > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2810
> > > >
> > > > I'm not sure what this ticket is about, but it was already closed
> > > > several weeks ago due to no longer reproducing.
> > > 
> > > I'm sure we all have our "it works on my machine" moment :D
> > 
> > The point is that we can't point at a non-active issue as justification
> > here.  If the failure reported in that issue is still relevant, the
> > ticket should be re-opened and a proper analysis should be posted there.
> > But changing the ABI like this wouldn't help anyway since it would just
> > guarantee that the test breaks (because none of the interfaces it tries
> > to check would exist anymore).  This patch wouldn't actually fix or
> > close that ticket.
> > 
> > Besides, the failure in that ticket likely isn't a KMD issue at all,
> > just a bug in the test.  By design, these sysfs interfaces represent the
> > "right now" status of the hardware.  If you read several sysfs entries
> > then the status could change as that's happening.  It's just a flaw in
> > the test that it overlooked this and incorrectly assumed the status
> > would never change while the test is running.
> > 
> > > 
> > > > The ABI change here doesn't seem to be directly related to this ticket.
> > > 
> > > We can always create new tickets, and that's not the point.
> > > The point is we're stuck with unreliable ABI.
> > 
> > Unreliable how?  We don't let new userspace interfaces into the kernel
> > unless there's real (not IGT) userspace software lined up to use them,
> > and the userspace developers have already signed off on the design and
> > semantics of the interfaces.  Presumably the current users are happy
> > with the current semantics and understand that the status provided is
> > live rather than sticky.
> 
> I was looking at that commit and noticed that it actually doesn't point
> to any real userspace user though. Ugh. Kernel doesn't break userspace
> interface. That's true, but it's not the same as "the ABI never
> changes". It's always risky and a decision not taken lightly because
> there's a *potential* for breakage.  However you are not breaking
> userspace if the usersapce doesn't exist.  It seems the uapi was mainly
> a copy & paste from i915.
> 
> So, a couple questions here:
> 
> 1) are there userspace tools consuming this UAPI?

If it turns out that there's nothing actually using this uapi today (and
we can come up with solid evidence for that fact), then the proper
action would probably be to directly revert the existing uapi without
replacement.  Then down the road if/when a real userspace consumer shows
up we can re-add the API (possibly with changes if necessary), following
all the regular rules.

BTW it's a lot harder to guarantee "nothing is using this" today than it
used to be back in the days when all KMD communication typically
funneled through a small number of specific projects like Xorg, mesa,
and libva.  We can figure out if those well-known projects are using an
interface pretty easily, and to some extent we can google through
hosting sites like github looking for lesser-known projects that might
be using an interface, but there's always the chance that
KMD<->userspace interfaces are being used inside proprietary and/or
home-grown, organization-internal tools.  We have requirements for
having an open source consumer to upstream new interfaces, but that rule
doesn't mean that we're allowed to introduce ABI breakage for
non-opensource software.


Matt

> 
> Rodrigo, I've heard you were integrating it in a top-like app. Would
> that be the first user? grepping around I couldn't find anything
> actually using these sysfs files with xe. The only references I found
> were in
> 
> a) https://github.com/intel/compute-runtime
> 
> but that is not in the interface with xe AFAICS... it's in the
> os_frequency_imp_prelim.cpp
> 
> b) https://github.com/geopm/geopm
> 
> not very familiar with that, but it's possible it's actually used,
> although I don't know if it's just an i915 interface or not.
> 
> 
> 2) Unreliable how?  Just as Matt asked, if it's unreliable you need to
> provide a proper justification for it. From the commit message:
> 
> 	Current implementation adds multiple sysfs entries for getting GT
> 	throttle status and its reasons, which forces the user to read multiple
> 	entries for evaluating the result. Since output of each entry is based
> 	on the same underlying hardware register and considering the access type
> 	of this register is RO/v, the value of this register can change at any
> 	given point, even between subsequent sysfs reads. This makes current
> 	implementation fundamentally flawed which can produce inconsistent results.
> 
> so what? the error would be in the user thinking that he could read
> multiple sysfs files atomically. What if user is interested in just 1
> of them?
> 
> It seems like adding a new file that guarantees this atomicity, is
> documented as such and *points to an actual userspace consumer*
> would be much better, with no potential for breakage. If then we find
> out there are no users of the old interface, we may think about
> deprecating/removing it.
> 
> Lucas De Marchi
> 
> > 
> > Sometimes we realize down the road that we did overlook something
> > important in an interface.  It's unfortunate when that happens, but the
> > path forward then is that we create a new, alternate API on the side and
> > let existing userspace migrate over or not as it sees fit (e.g., stuff
> > like the DRM "addfb2" ioctl being added as a more expressive alternative
> > to "addfb" usage).  We still need to support the old interface though;
> > we can generally only sunset it for future platforms where there's no
> > pre-existing software already using the old interface.
> > 
> > 
> > Matt
> > 
> > > 
> > > Side note: The logs in the ticket may help connect the dots with commit message.
> > > 
> > > Raag
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation