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

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

[Timur Kristóf]

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>
---
 drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
index b72a0c34425a..ff7128a11cad 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
@@ -288,7 +288,7 @@ static int uvd_v3_1_fw_validate(struct amdgpu_device *adev)
 	int i;
 	uint32_t keysel = adev->uvd.keyselect;
 
-	if (RREG32(mmUVD_FW_START) & UVD_FW_STATUS__PASS_MASK) {
+	if (RREG32(mmUVD_FW_START)) {
 		dev_dbg(adev->dev, "UVD keyselect already set: 0x%x (on CPU: 0x%x)\n",
 			RREG32(mmUVD_FW_START), adev->uvd.keyselect);
 		return 0;
-- 
2.55.0

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

[Mario Limonciello]

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>
> ---
>   drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
> index b72a0c34425a..ff7128a11cad 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
> @@ -288,7 +288,7 @@ static int uvd_v3_1_fw_validate(struct amdgpu_device *adev)
>   	int i;
>   	uint32_t keysel = adev->uvd.keyselect;
>   
> -	if (RREG32(mmUVD_FW_START) & UVD_FW_STATUS__PASS_MASK) {
> +	if (RREG32(mmUVD_FW_START)) {
>   		dev_dbg(adev->dev, "UVD keyselect already set: 0x%x (on CPU: 0x%x)\n",
>   			RREG32(mmUVD_FW_START), adev->uvd.keyselect);
>   		return 0;

Can you have a look at the Sashiko feedback:

https://sashiko.dev/#/patchset/20261006211607.75407-1-timur.kristof%40gmail.com

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

[Timur Kristóf]

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. 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.

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

[Mario Limonciello]

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.