On Fri, Jul 11, 2025 at 10:47:39AM +0530, Riana Tauro wrote: > On 7/11/2025 3:16 AM, Raag Jadav wrote: > > On Thu, Jul 10, 2025 at 03:00:06PM -0400, Rodrigo Vivi wrote: > > > On Thu, Jul 10, 2025 at 01:24:52PM +0300, Raag Jadav 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 <andrealm...@igalia.com> > > > > > > > > > > > Cc: Christian König <christian.koe...@amd.com> > > > > > > > > > > > Cc: David Airlie <airl...@gmail.com> > > > > > > > > > > > Cc: <dri-devel@lists.freedesktop.org> > > > > > > > > > > > Suggested-by: Raag Jadav <raag.ja...@intel.com> > > > > > > > > > > > Signed-off-by: Riana Tauro <riana.ta...@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 also don't like the idea or even the thought of scripting something like > > > a firmware-flash. But only to fail with a better pin point to make admin > > > lives easier with a notification. > > > > > > > > > > > > > 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. > > > > > > Exactly, the hardware and firmware are giving the indication of what > > > should be > > > done. It is not 'unknown'. > > > > > > > > > > > Yes, and since the recovery procedure is defined and known to the > > > > consumer, > > > > it can potentially be automated (atleast for non-firmware cases). > > > > > > > > > > I guess if you all want to stick with vendor-specific then I think > > > > > > that's > > > > > > Well, I would honestly prefer a direct firmware-flash, but if that is not > > > usable by other vendors and there's a push back on that, let's go with > > > the vendor-specific then. > > > > I think the procedure for firmware-flash is vendor specific, so the wedged > > event > > alone is not sufficient either way. The consumer will need more guidance > > from > > vendor documentation. > > Procedure of firmware-flash is vendor specific, but the term > 'firmware-flash' is still generic. The patch doesn't mention any vendor > specific firmware or procedure. The push back was for the number of macros > that can be added for other operations.
A common procedure for the methods is what makes them agnostic and usable for all drivers. Otherwise it's pretty much a chaos for the consumer. > > With vendor-specific method, the driver has the opportunity to cover as many > > cases as it wants without having to create a new method everytime, and face > > the > > same dilemma of being vendor agnostic. > > > > > > > > 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. > > > > > > > > The device id along with driver can be identified from uevent (probably > > > > available inside DEVPATH somewhere) to distinguish the vendor. So the > > > > consumer > > > > already knows if the device fits the criteria for recovery. > > > > > > > > > > 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. > > > > > > > > Perhaps the rest of the series didn't make it to dri-devel, which will > > > > answer > > > > most of the above. > > > > > > Riana, could you please try to provide a bit more documentation like Sima > > > asked and re-send the entire series to dri-devel? > > Sure will send the entire series to dri-devel. The documentation is present > in the series. > > > > > With the ideas in this thread also documented so that we don't end up > > repeating > > the same discussion. > It is mentioned in cover letter but i didn't send it to dri-devel. will add > more details Thank you. 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
