[PATCH v4 1/9] drm: Add a vendor-specific recovery method to device wedged uevent
Christian König
christian.koenig at amd.com
Thu Jul 10 09:37:14 UTC 2025
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.
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.
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
>>>>>>
>>>>>
>>>>
>
More information about the dri-devel
mailing list