Re: [PATCH] drm/amdgpu/uvd3.1: Fix checking whether UVD firmware keyselect was already set

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/8/26 15:21, Timur Kristóf wrote:
> On 2026. október 7., szerda 7:50:29 középső államokbeli zónaidő Mario
> Limonciello wrote:
>> On 10/6/26 16:16, Timur Kristóf wrote:
>>> The PASS_MASK is for a different register not for UVD_FW_START.
>>> It was a mistake to check this flag. It should be enough to check
>>> that the register is non-zero.
>>>
>>> Fixes: 13e4cf116dbf ("drm/amdgpu/uvd3.1: Don't validate the firmware when
>>> already validated") Signed-off-by: Timur Kristóf
>>> <timur.kristof@gmail.com>
>>
>> Can you have a look at the Sashiko feedback:
>>
>> https://sashiko.dev/#/patchset/20261006211607.75407-1-timur.kristof%40gmail.
>> com
> 
> Hi Mario,
> 
> This patch simply fixes a typo in my previous patch that I referenced in the
> Fixes tag above. The PASS_MASK field doesn't belong to the UVD_FW_START
> register, it was simply a mistake on my part.
> 
> All in all, I'm not sure what to do with this feedback, as the patch is just
> fixing a typo and doesn't introduce any of the issues that the bot is
> complaining about. 

Right; understood.  I personally don't think that we *need* to fix any 
of the feedback in this patch.

I mostly wanted to make sure you got a chance to look it over and agree 
to that.

I'll queue the patch up to ASDN.

> In general, it might be a good idea to do something when
> the firmware validation fails (though it's out of scope for this patch). For
> example we could ignore the UVD block and move forward without it. We
> discussed this briefly some time ago with Alex, and the conclusion was the we
> prefer not to support "degraded" functionality in amdgpu. We could try to
> reset the GPU and try the firmware validation again, but then there is no
> reason to assume it has any chance of success if it had already failed before.
> 
> I'll try to respond to some points from the bot's feedback below:
> 
>> Does this check falsely succeed if a previous validation attempt
>> timed out or failed?
> 
> According to my current understanding, the FW validator always fails on the
> second attempt (even if the first attempt was successful).
> If firmware validation fails or times out, then the driver simply fails to
> initialize and won't retry again AFAIK. The purpose of checking the register
> is to prevent trying to validate again because the second attempt always
> fails, which can leave the GPU in a bad state after a suspend/resume cycle (on
> systems where suspend doesn't completely power off the GPU).
> 
>> If uvd_v3_1_fw_validate() times out or fails after writing
>> to mmUVD_FW_START, the register remains non-zero.
> 
> Yes. If uvd_v3_1_fw_validate() fails, then amdgpu just fails to initialize (or
> fails to resume from suspend) and will never recover. This is how it always
> was and my patch doesn't change that.
> 
>> Upon initialization failure, amdgpu_device_ip_fini()
>> skips hw_fini for the UVD block. [...]
> 
> As far as I see amdgpu_device_ip_fini() doesn't call hw_fini() at all for any
> block, so this argument doesn't make sense to me.
> 
>> Furthermore, for SI ASICs (the only users of UVD 3.1),
>> si_need_reset_on_init() unconditionally returns false,
>> meaning the ASIC is not automatically reset on a subsequent driver reload.
>> [...]
>> Consequently, upon driver reload, this check will see the leftover non-zero
>> value and immediately return 0. The driver will then incorrectly assume
>> validation succeeded and use the uninitialized UVD block, which can lead to
>> ring test failures or GPU hangs.
> 
> This is true, though isn't changed or affected in any way by this patch.
> 
>> If validation fails later in this function, does this leak the
>> UVD DPM clocks? [...]
>> As a result, the UVD clocks enabled before calling uvd_v3_1_fw_validate()
>> are never disabled on failure.
> 
> Again, if uvd_v3_1_fw_validate() fails then the driver just fails to
> initialize and the GPU isn't recovered. This isn't changed or affected in any
> way by this patch (or the previous one). So it doesn't matter what we do with
> the clocks.