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. > > > 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? Thanks, Rodrigo. > > 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 > > >>>>>> > > >>>>> > > >>>> > > > > >
