[PATCH v4 1/9] drm: Add a vendor-specific recovery method to device wedged uevent
Simona Vetter
simona.vetter at ffwll.ch
Mon Jul 14 12:33:53 UTC 2025
On Mon, Jul 14, 2025 at 10:57:48AM +0530, Riana Tauro wrote:
>
>
> On 7/11/2025 2:29 PM, Simona Vetter wrote:
> > On Thu, Jul 10, 2025 at 11:37:14AM +0200, Christian König wrote:
> > > On 10.07.25 11:01, Simona Vetter wrote:
> > > > On Wed, Jul 09, 2025 at 12:52:05PM -0400, Rodrigo Vivi wrote:
> > > > > On Wed, Jul 09, 2025 at 05:18:54PM +0300, Raag Jadav wrote:
> > > > > > On Wed, Jul 09, 2025 at 04:09:20PM +0200, Christian König wrote:
> > > > > > > On 09.07.25 15:41, Simona Vetter wrote:
> > > > > > > > On Wed, Jul 09, 2025 at 04:50:13PM +0530, Riana Tauro wrote:
> > > > > > > > > Certain errors can cause the device to be wedged and may
> > > > > > > > > require a vendor specific recovery method to restore normal
> > > > > > > > > operation.
> > > > > > > > >
> > > > > > > > > Add a recovery method 'WEDGED=vendor-specific' for such errors. Vendors
> > > > > > > > > must provide additional recovery documentation if this method
> > > > > > > > > is used.
> > > > > > > > >
> > > > > > > > > v2: fix documentation (Raag)
> > > > > > > > >
> > > > > > > > > Cc: André Almeida <andrealmeid at igalia.com>
> > > > > > > > > Cc: Christian König <christian.koenig at amd.com>
> > > > > > > > > Cc: David Airlie <airlied at gmail.com>
> > > > > > > > > Cc: <dri-devel at lists.freedesktop.org>
> > > > > > > > > Suggested-by: Raag Jadav <raag.jadav at intel.com>
> > > > > > > > > Signed-off-by: Riana Tauro <riana.tauro at intel.com>
> > > > > > > >
> > > > > > > > I'm not really understanding what this is useful for, maybe concrete
> > > > > > > > example in the form of driver code that uses this, and some tool or
> > > > > > > > documentation steps that should be taken for recovery?
> > > > >
> > > > > The case here is when FW underneath identified something badly corrupted on
> > > > > FW land and decided that only a firmware-flashing could solve the day and
> > > > > raise interrupt to the driver. At that point we want to wedge, but immediately
> > > > > hint the admin the recommended action.
> > > > >
> > > > > > >
> > > > > > > The recovery method for this particular case is to flash in a new firmware.
> > > > > > >
> > > > > > > > The issues I'm seeing here is that eventually we'll get different
> > > > > > > > vendor-specific recovery steps, and maybe even on the same device, and
> > > > > > > > that leads us to an enumeration issue. Since it's just a string and an
> > > > > > > > enum I think it'd be better to just allocate a new one every time there's
> > > > > > > > a new strange recovery method instead of this opaque approach.
> > > > > > >
> > > > > > > That is exactly the opposite of what we discussed so far.
> > > >
> > > > Sorry, I missed that context.
> > > >
> > > > > > > The original idea was to add a firmware-flush recovery method which
> > > > > > > looked a bit wage since it didn't give any information on what to do
> > > > > > > exactly.
> > > > > > >
> > > > > > > That's why I suggested to add a more generic vendor-specific event
> > > > > > > with refers to the documentation and system log to see what actually
> > > > > > > needs to be done.
> > > > > > >
> > > > > > > Otherwise we would end up with events like firmware-flash, update FW
> > > > > > > image A, update FW image B, FW version mismatch etc....
> > > >
> > > > Yeah, that's kinda what I expect to happen, and we have enough numbers for
> > > > this all to not be an issue.
> > > >
> > > > > > Agree. Any newly allocated method that is specific to a vendor is going to
> > > > > > be opaque anyway, since it can't be generic for all drivers. This just helps
> > > > > > reduce the noise in DRM core.
> > > > > >
> > > > > > And yes, there could be different vendor-specific cases for the same driver
> > > > > > and the driver should be able to provide the means to distinguish between
> > > > > > them.
> > > > >
> > > > > Sim, what's your take on this then?
> > > > >
> > > > > Should we get back to the original idea of firmware-flash?
> > > >
> > > > Maybe intel-firmware-flash or something, meaning prefix with the vendor?
> > > >
> > > > The reason I think it should be specific is because I'm assuming you want
> > > > to script this. And if you have a big fleet with different vendors, then
> > > > "vendor-specific" doesn't tell you enough. But if it's something like
> > > > $vendor-$magic_step then it does become scriptable, and we do have have a
> > > > place to put some documentation on what you should do instead.
> > > >
> > > > If the point of this interface isn't that it's scriptable, then I'm not
> > > > sure why it needs to be an uevent?
> > >
> > > You should probably read up on the previous discussion, cause that is
> > > exactly what I asked as well :)
> > >
> > > And no, it should *not* be scripted. That would be a bit brave for a
> > > firmware update where you should absolutely not power down the system
> > > for example.
> >
> > I guess if we clearly state that this is for manual recovery only, or for
> > cases where you exactly know what you're doing (fleet-specific scripts
> > instead of generic distros), I guess this very opaque code makes sense.
> >
> > But we should clearly document then that doing anything scripted here is
> > very much "you get to keep the pieces", and definitely don't try to do
> > something fancy generic.
>
>
> The documentation is part of the series but was sent only to intel-xe
> mailing list. Will re-send the entire series to dri-devel
>
> https://lore.kernel.org/intel-xe/aHH2XGuOvz8bSlax@black.fi.intel.com/T/#m883269cf0b1f6891ecc9c24d3d45325f46d56572
Duh, missed that, but yes definitely send the entire series to all mailing
lists. Especially when adding new drm features like this one does.
> > Which without documentation is just really confusing when some of the
> > other error codes clearly look like they're meant to facilitate scripted
> > recovery.
> >
>
> To get consensus on the patch, is 'vendor-specific' okay or is it better to
> have 'firmware-flash' with additional event parameter 'vendor' if number of
> macros is not a concern?
I'll refrain from a vote.
-Sima
>
> Thanks
> Riana
> > > In my understanding the new value "vendor-specific" basically means it
> > > is a known issue with a documented solution, while "unknown" means the
> > > driver has no idea how to solve it.
> >
> > I think that's another detail which should be documented clearly.
> > -Sima
> > >
> > > Regards,
> > > Christian.
> > >
> > > > I guess if you all want to stick with vendor-specific then I think that's
> > > > ok with me too, but the docs should at least explain how to figure out
> > > > from the uevent which vendor you're on with a small example. What I'm
> > > > worried is that if we have this on multiple drivers userspace will
> > > > otherwise make a complete mess and might want to run the wrong recovery
> > > > steps.
> > > >
> > > > I think ideally, no matter what, we'd have a concrete driver patch which
> > > > then also comes with the documentation for what exactly you're supposed to
> > > > do as something you can script. And not just this stand-alone patch here.
> > > >
> > > > Cheers, Sima
> > > > >
> > > > > >
> > > > > > Raag
> > > > > >
> > > > > > > > > ---
> > > > > > > > > Documentation/gpu/drm-uapi.rst | 9 +++++----
> > > > > > > > > drivers/gpu/drm/drm_drv.c | 2 ++
> > > > > > > > > include/drm/drm_device.h | 4 ++++
> > > > > > > > > 3 files changed, 11 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > > > > > > > > index 263e5a97c080..c33070bdb347 100644
> > > > > > > > > --- a/Documentation/gpu/drm-uapi.rst
> > > > > > > > > +++ b/Documentation/gpu/drm-uapi.rst
> > > > > > > > > @@ -421,10 +421,10 @@ Recovery
> > > > > > > > > Current implementation defines three recovery methods, out of which, drivers
> > > > > > > > > can use any one, multiple or none. Method(s) of choice will be sent in the
> > > > > > > > > uevent environment as ``WEDGED=<method1>[,..,<methodN>]`` in order of less to
> > > > > > > > > -more side-effects. If driver is unsure about recovery or method is unknown
> > > > > > > > > -(like soft/hard system reboot, firmware flashing, physical device replacement
> > > > > > > > > -or any other procedure which can't be attempted on the fly), ``WEDGED=unknown``
> > > > > > > > > -will be sent instead.
> > > > > > > > > +more side-effects. If recovery method is specific to vendor
> > > > > > > > > +``WEDGED=vendor-specific`` will be sent and userspace should refer to vendor
> > > > > > > > > +specific documentation for further recovery steps. If driver is unsure about
> > > > > > > > > +recovery or method is unknown, ``WEDGED=unknown`` will be sent instead
> > > > > > > > > Userspace consumers can parse this event and attempt recovery as per the
> > > > > > > > > following expectations.
> > > > > > > > > @@ -435,6 +435,7 @@ following expectations.
> > > > > > > > > none optional telemetry collection
> > > > > > > > > rebind unbind + bind driver
> > > > > > > > > bus-reset unbind + bus reset/re-enumeration + bind
> > > > > > > > > + vendor-specific vendor specific recovery method
> > > > > > > > > unknown consumer policy
> > > > > > > > > =============== ========================================
> > > > > > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > > > > > > > index cdd591b11488..0ac723a46a91 100644
> > > > > > > > > --- a/drivers/gpu/drm/drm_drv.c
> > > > > > > > > +++ b/drivers/gpu/drm/drm_drv.c
> > > > > > > > > @@ -532,6 +532,8 @@ static const char *drm_get_wedge_recovery(unsigned int opt)
> > > > > > > > > return "rebind";
> > > > > > > > > case DRM_WEDGE_RECOVERY_BUS_RESET:
> > > > > > > > > return "bus-reset";
> > > > > > > > > + case DRM_WEDGE_RECOVERY_VENDOR:
> > > > > > > > > + return "vendor-specific";
> > > > > > > > > default:
> > > > > > > > > return NULL;
> > > > > > > > > }
> > > > > > > > > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> > > > > > > > > index 08b3b2467c4c..08a087f149ff 100644
> > > > > > > > > --- a/include/drm/drm_device.h
> > > > > > > > > +++ b/include/drm/drm_device.h
> > > > > > > > > @@ -26,10 +26,14 @@ struct pci_controller;
> > > > > > > > > * Recovery methods for wedged device in order of less to more side-effects.
> > > > > > > > > * To be used with drm_dev_wedged_event() as recovery @method. Callers can
> > > > > > > > > * use any one, multiple (or'd) or none depending on their needs.
> > > > > > > > > + *
> > > > > > > > > + * Refer to "Device Wedging" chapter in Documentation/gpu/drm-uapi.rst for more
> > > > > > > > > + * details.
> > > > > > > > > */
> > > > > > > > > #define DRM_WEDGE_RECOVERY_NONE BIT(0) /* optional telemetry collection */
> > > > > > > > > #define DRM_WEDGE_RECOVERY_REBIND BIT(1) /* unbind + bind driver */
> > > > > > > > > #define DRM_WEDGE_RECOVERY_BUS_RESET BIT(2) /* unbind + reset bus device + bind */
> > > > > > > > > +#define DRM_WEDGE_RECOVERY_VENDOR BIT(3) /* vendor specific recovery method */
> > > > > > > > > /**
> > > > > > > > > * struct drm_wedge_task_info - information about the guilty task of a wedge dev
> > > > > > > > > --
> > > > > > > > > 2.47.1
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > >
> > >
> >
>
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-xe
mailing list