[PATCH 33/36] drm/amdgpu/vcn: add a helper framework for engine resets

[Sundararaju, Sathishkumar]

Please ignore my previous comments here, the new helper additions for 
vcn non unified queues are good.

But one concern, the vinst->reset(vinst) callback must take in ring 
pointer to handle guilty/non-guilty
for appropriate re-emit part, else the guilty ring has to be tracked 
within the ring structure or identified
by some query with in reset.

Regards,
Sathish


On 6/17/2025 10:00 AM, Sundararaju, Sathishkumar wrote:
> Hi Alex,
>
> Would it be good to have this logic in the reset call back itself ?
>
> Adding common vinst->reset stops the flexibility of having separate 
> reset functionality for enc rings and decode rings,
> can selectively handle the drm_sched_wqueue_start/stop and re-emit of 
> guilty/non-guilty for enc and dec separately.
>
> And the usual vcn_stop() followed by vcn_start() isn't helping in 
> reset of the engine for vcn3.
>
> I tried a workaround to pause_dpg and enable static clockgate and 
> powergate, and then stop()/start() the engine
> which is working consistently so far.
>
> Regards,
> Sathish
>
> On 6/17/2025 8:38 AM, Alex Deucher wrote:
>> With engine resets we reset all queues on the engine rather
>> than just a single queue.  Add a framework to handle this
>> similar to SDMA.
>>
>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 64 +++++++++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  6 ++-
>>   2 files changed, 69 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> index c8885c3d54b33..075740ed275eb 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> @@ -134,6 +134,7 @@ int amdgpu_vcn_sw_init(struct amdgpu_device 
>> *adev, int i)
>> mutex_init(&adev->vcn.inst[i].vcn1_jpeg1_workaround);
>>       mutex_init(&adev->vcn.inst[i].vcn_pg_lock);
>> +    mutex_init(&adev->vcn.inst[i].engine_reset_mutex);
>>       atomic_set(&adev->vcn.inst[i].total_submission_cnt, 0);
>>       INIT_DELAYED_WORK(&adev->vcn.inst[i].idle_work, 
>> amdgpu_vcn_idle_work_handler);
>> atomic_set(&adev->vcn.inst[i].dpg_enc_submission_cnt, 0);
>> @@ -1451,3 +1452,66 @@ int vcn_set_powergating_state(struct 
>> amdgpu_ip_block *ip_block,
>>         return ret;
>>   }
>> +
>> +/**
>> + * amdgpu_vcn_reset_engine - Reset a specific VCN engine
>> + * @adev: Pointer to the AMDGPU device
>> + * @instance_id: VCN engine instance to reset
>> + *
>> + * Returns: 0 on success, or a negative error code on failure.
>> + */
>> +static int amdgpu_vcn_reset_engine(struct amdgpu_device *adev,
>> +                   uint32_t instance_id)
>> +{
>> +    struct amdgpu_vcn_inst *vinst = &adev->vcn.inst[instance_id];
>> +    int r, i;
>> +
>> +    mutex_lock(&vinst->engine_reset_mutex);
>> +    /* Stop the scheduler's work queue for the dec and enc rings if 
>> they are running.
>> +     * This ensures that no new tasks are submitted to the queues while
>> +     * the reset is in progress.
>> +     */
>> +    drm_sched_wqueue_stop(&vinst->ring_dec.sched);
>> +    for (i = 0; i < vinst->num_enc_rings; i++)
>> + drm_sched_wqueue_stop(&vinst->ring_enc[i].sched);
>> +
>> +    /* Perform the VCN reset for the specified instance */
>> +    r = vinst->reset(vinst);
>> +    if (r) {
>> +        dev_err(adev->dev, "Failed to reset VCN instance %u\n", 
>> instance_id);
>> +    } else {
>> +        /* Restart the scheduler's work queue for the dec and enc rings
>> +         * if they were stopped by this function. This allows new tasks
>> +         * to be submitted to the queues after the reset is complete.
>> +         */
>> +        drm_sched_wqueue_start(&vinst->ring_dec.sched);
>> +        for (i = 0; i < vinst->num_enc_rings; i++)
>> + drm_sched_wqueue_start(&vinst->ring_enc[i].sched);
>> +    }
>> +    mutex_unlock(&vinst->engine_reset_mutex);
>> +
>> +    return r;
>> +}
>> +
>> +/**
>> + * amdgpu_vcn_ring_reset - Reset a VCN ring
>> + * @ring: ring to reset
>> + * @vmid: vmid of guilty job
>> + * @guilty_fence: guilty fence
>> + *
>> + * This helper is for VCN blocks without unified queues because
>> + * resetting the engine resets all queues in that case.  With
>> + * unified queues we have one queue per engine.
>> + * Returns: 0 on success, or a negative error code on failure.
>> + */
>> +int amdgpu_vcn_ring_reset(struct amdgpu_ring *ring,
>> +              unsigned int vmid,
>> +              struct amdgpu_fence *guilty_fence)
>> +{
>> +    struct amdgpu_device *adev = ring->adev;
>> +
>> +    if (adev->vcn.inst[ring->me].using_unified_queue)
>> +        return -EINVAL;
>> +
>> +    return amdgpu_vcn_reset_engine(adev, ring->me);
>> +}
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>> index 83adf81defc71..0bc0a94d7cf0f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>> @@ -330,7 +330,9 @@ struct amdgpu_vcn_inst {
>>                     struct dpg_pause_state *new_state);
>>       int (*set_pg_state)(struct amdgpu_vcn_inst *vinst,
>>                   enum amd_powergating_state state);
>> +    int (*reset)(struct amdgpu_vcn_inst *vinst);
>>       bool using_unified_queue;
>> +    struct mutex        engine_reset_mutex;
>>   };
>>     struct amdgpu_vcn_ras {
>> @@ -552,5 +554,7 @@ void amdgpu_debugfs_vcn_sched_mask_init(struct 
>> amdgpu_device *adev);
>>     int vcn_set_powergating_state(struct amdgpu_ip_block *ip_block,
>>                     enum amd_powergating_state state);
>> -
>> +int amdgpu_vcn_ring_reset(struct amdgpu_ring *ring,
>> +              unsigned int vmid,
>> +              struct amdgpu_fence *guilty_fence);
>>   #endif
>