On 10/9/26 08:19, Liang, Prike wrote:
> AMD General
>
> Regards,
> Prike
>
>> -----Original Message-----
>> From: Lazar, Lijo <[email protected]>
>> Sent: Friday, October 9, 2026 12:50 PM
>> To: Liang, Prike <[email protected]>; Koenig, Christian
>> <[email protected]>; [email protected]
>> Cc: Deucher, Alexander <[email protected]>; Prosyak, Vitaly
>> <[email protected]>
>> Subject: Re: [PATCH v2 09/14] drm/amdgpu: defer JPEG and VCN idle work across
>> GPU reset
>>
>>
>>
>> On 09-Oct-26 9:13 AM, Liang, Prike wrote:
>>> AMD General
>>>
>>> Regards,
>>> Prike
>>>
>>>> -----Original Message-----
>>>> From: Liang, Prike
>>>> Sent: Thursday, October 8, 2026 8:42 PM
>>>> To: Koenig, Christian <[email protected]>; Lazar, Lijo
>>>> <[email protected]>; [email protected]
>>>> Cc: Deucher, Alexander <[email protected]>; Prosyak, Vitaly
>>>> <[email protected]>
>>>> Subject: RE: [PATCH v2 09/14] drm/amdgpu: defer JPEG and VCN idle
>>>> work across GPU reset
>>>>
>>>>
>>>>
>>>> Regards,
>>>> Prike
>>>>
>>>>> -----Original Message-----
>>>>> From: Koenig, Christian <[email protected]>
>>>>> Sent: Thursday, October 8, 2026 8:34 PM
>>>>> To: Lazar, Lijo <[email protected]>; Liang, Prike
>>>>> <[email protected]>; amd- [email protected]
>>>>> Cc: Deucher, Alexander <[email protected]>; Prosyak, Vitaly
>>>>> <[email protected]>
>>>>> Subject: Re: [PATCH v2 09/14] drm/amdgpu: defer JPEG and VCN idle
>>>>> work across GPU reset
>>>>>
>>>>> On 10/8/26 14:17, Lazar, Lijo wrote:
>>>>>>
>>>>>>
>>>>>> On 29-Sep-26 9:16 AM, Prike Liang wrote:
>>>>>>> Protect media idle workers with a reset-domain read trylock. On
>>>>>>> contention, requeue the idle timeout and return without hardware
>>>>>>> access. Keep the ordinary end-use scheduling event, including from
>>>>>>> reset-time ring tests, so otherwise idle engines eventually power
>>>>>>> down and VCN
>>>>> drops its profile.
>>>>>>>
>>>>>>> A blocking read acquisition would deadlock with reset-time
>>>>>>> synchronous cancellation of these workers, so the workers must
>>>>>>> only try the
>>>> lock.
>>>>>>>
>>>>>>> Signed-off-by: Prike Liang <[email protected]>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c | 7 +++++++
>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 7 +++++++
>>>>>>> 2 files changed, 14 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>>>>>>> index 9887a4f99aed..7b10260ad0d4 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>>>>>>> @@ -114,6 +114,12 @@ static void
>>>>>>> amdgpu_jpeg_idle_work_handler(struct
>>>>>>> work_struct *work)
>>>>>>> unsigned int fences = 0;
>>>>>>> unsigned int i, j;
>>>>>>> + /* Reset may cancel this worker synchronously while holding
>>>>>>> the writer. */
>>>>>>> + if (!down_read_trylock(&adev->reset_domain->sem)) {
>>>>>>> + schedule_delayed_work(&adev->jpeg.idle_work,
>>>>>>> +JPEG_IDLE_TIMEOUT);
>>>>>>
>>>>>> Is there any need to schedule this work here? Being not able to get
>>>>>> the lock
>>>>> indicates that a reset is in progress already.
>>>>>
>>>>> Yeah agree. We should just block for the GPU reset to finish.
>>>>>
>>>>> So a simple down_read(&adev->reset_domain->sem) should do it.
>>>>
>>>> This patch wants to schedule a delay idle work during processing a
>>>> gpu reset process, and then retry to run the VCN idle at the next
>>>> scheduled loop.
>>>
>>> A potential deadlock can occur if the VCN/JPEG idle worker runs during a GPU
>> reset while performing a blocking wait for the reset to complete. This patch
>> resolves
>> the issue by using a retry-based wait on the GPU reset semaphore within the
>> idle
>> worker callback.
>>>
>>
>> VCN is going to be idle after a reset. The next idle worker will be
>> scheduled after a
>> fresh job submission is complete. Why we need to reschedule idle work here?
>
> If the VCN idle worker runs while GPU reset holds the semaphore, returning
> without rescheduling could leave VCN ungated if no new jobs arrive. However,
> blocking on the semaphore in VCN idle worker could deadlock because the reset
> path may synchronously cancel the worker while holding the semaphore. Using a
> trylock and rescheduling on contention avoids this deadlock while ensuring
> VCN can be power gated once reset completes.
In this case absolutely clear NAK to that! Approaches like that are completely
illegal upstream.
The reset work handler *CAN'T* depend on the idle work and so also can't
synchronously cancel it. That is an absolutely clear NO-GO.
Just use down_read() to make sure that lockdep starts complaining when the
reset handler does such nasty things.
Regards,
Christian.
>
> Thanks,
> Prike
>
>> Thanks,
>> Lijo
>>
>>> Thanks,
>>> Prike
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Lijo
>>>>>>
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> for (i = 0; i < adev->jpeg.num_jpeg_inst; ++i) {
>>>>>>> if (adev->jpeg.harvest_config & (1U << i))
>>>>>>> continue;
>>>>>>> @@ -129,6 +135,7 @@ static void
>>>>>>> amdgpu_jpeg_idle_work_handler(struct
>>>>>>> work_struct *work)
>>>>>>> mutex_unlock(&adev->jpeg.jpeg_pg_lock);
>>>>>>> } else
>>>>>>> schedule_delayed_work(&adev->jpeg.idle_work,
>>>>>>> JPEG_IDLE_TIMEOUT);
>>>>>>> + up_read(&adev->reset_domain->sem);
>>>>>>> }
>>>>>>> void amdgpu_jpeg_ring_begin_use(struct amdgpu_ring *ring)
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>>>>>> index 17db7264269e..f851a6ecea1b 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>>>>>>> @@ -470,6 +470,12 @@ static void
>>>>>>> amdgpu_vcn_idle_work_handler(struct
>>>>>>> work_struct *work)
>>>>>>> if (adev->vcn.harvest_config & (1 << i))
>>>>>>> return;
>>>>>>> + /* Reset may cancel this worker synchronously while holding
>>>>>>> the writer. */
>>>>>>> + if (!down_read_trylock(&adev->reset_domain->sem)) {
>>>>>>> + schedule_delayed_work(&vcn_inst->idle_work,
>>>>>>> +VCN_IDLE_TIMEOUT);
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> for (j = 0; j < adev->vcn.inst[i].num_enc_rings; ++j)
>>>>>>> fence[i] +=
>>>>>>> amdgpu_fence_count_emitted(&vcn_inst->ring_enc[j]);
>>>>>>> @@ -499,6 +505,7 @@ static void
>>>>>>> amdgpu_vcn_idle_work_handler(struct
>>>>>>> work_struct *work)
>>>>>>> } else {
>>>>>>> schedule_delayed_work(&vcn_inst->idle_work,
>>>>>>> VCN_IDLE_TIMEOUT);
>>>>>>> }
>>>>>>> + up_read(&adev->reset_domain->sem);
>>>>>>> }
>>>>>>> void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
>>>>>>
>>>
>
