[PATCH 2/7] drm/amdgpu: Add ring reset callback for JPEG4_0_3
Sundararaju, Sathishkumar
sasundar at amd.com
Fri Jan 31 17:40:32 UTC 2025
On 1/31/2025 10:35 PM, Christian König wrote:
> Am 31.01.25 um 17:55 schrieb Sundararaju, Sathishkumar:
>> On 1/31/2025 10:19 PM, Christian König wrote:
>>> Am 31.01.25 um 17:40 schrieb Sundararaju, Sathishkumar:
>>>> Hi Christian,
>>>>
>>>>
>>>> On 1/31/2025 9:56 PM, Christian König wrote:
>>>>> Am 31.01.25 um 17:23 schrieb Sathishkumar S:
>>>>>> Add ring reset function callback for JPEG4_0_3 to
>>>>>> recover from job timeouts without a full gpu reset.
>>>>>>
>>>>>> Signed-off-by: Sathishkumar S <sathishkumar.sundararaju at amd.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/amd/amdgpu/jpeg_v4_0_3.c | 60
>>>>>> ++++++++++++++++++++++--
>>>>>> 1 file changed, 57 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v4_0_3.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/jpeg_v4_0_3.c
>>>>>> index be0b3b4c8690..62d8628dccc5 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/jpeg_v4_0_3.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v4_0_3.c
>>>>>> @@ -204,9 +204,7 @@ static int jpeg_v4_0_3_sw_init(struct
>>>>>> amdgpu_ip_block *ip_block)
>>>>>> if (r)
>>>>>> return r;
>>>>>> - /* TODO: Add queue reset mask when FW fully supports it */
>>>>>> - adev->jpeg.supported_reset =
>>>>>> - amdgpu_get_soft_full_reset_mask(&adev->jpeg.inst[0].ring_dec[0]);
>>>>>> + adev->jpeg.supported_reset = AMDGPU_RESET_TYPE_PER_PIPE;
>>>>>> r = amdgpu_jpeg_sysfs_reset_mask_init(adev);
>>>>>> if (r)
>>>>>> return r;
>>>>>> @@ -231,6 +229,7 @@ static int jpeg_v4_0_3_sw_fini(struct
>>>>>> amdgpu_ip_block *ip_block)
>>>>>> return r;
>>>>>> amdgpu_jpeg_sysfs_reset_mask_fini(adev);
>>>>>> +
>>>>>> r = amdgpu_jpeg_sw_fini(adev);
>>>>>> return r;
>>>>>> @@ -1099,6 +1098,60 @@ static int
>>>>>> jpeg_v4_0_3_process_interrupt(struct amdgpu_device *adev,
>>>>>> return 0;
>>>>>> }
>>>>>> +static int jpeg_v4_0_3_wait_for_idle_on_inst(struct
>>>>>> amdgpu_ring *ring)
>>>>>> +{
>>>>>> + struct amdgpu_device *adev = ring->adev;
>>>>>> + struct amdgpu_ring *r;
>>>>>> + int ret, j;
>>>>>> +
>>>>>> + for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
>>>>>> + r = &adev->jpeg.inst[ring->me].ring_dec[j];
>>>>>> + r->sched.ready = false;
>>>>>> + }
>>>>>> + /* publish update */
>>>>>> + smp_rmb();
>>>>>
>>>>> Using smp_rmb() to publish the update is 100% incorrect.
>>>>>
>>>>> What exactly are you trying to do?
>>>> On JPEG4_0_3, there are possibly multiple contexts submitting jobs
>>>> to all cores on the affected instance,
>>>> so I am changing sched_ready to false and using smp_rmb() to
>>>> publish to other cores which are trying to
>>>> submit on this instance , for them to read the updated change
>>>> immediately and stop submission before
>>>> I wait for idle on this instance, which makes sure other good
>>>> contexts of jpeg get a chance to be migrated
>>>> out to any available instance before reset starts, that way good
>>>> jpeg contexts get a chance to continue
>>>> without issues while this instance is being reset and made ready to
>>>> continue decoding after reset.
>>>
>>> Ok, completely NAK to this approach!
>>>
>>> First of all setting r->sched.ready to false is a completely no-go.
>>> I can't remember how often I had to remove that nonsense.
>>>
>>> With the exception of the KIQ the back-ends should *never* touch
>>> that stuff!
>>>
>>> Then the Linux kernel requires that ever use of smp_rmb() requires
>>> to document the matching write memory barrier.
>>>
>>> Over all please completely remove that code all together. The wait
>>> for idle function is *only* supposed to wait for the HW to become
>>> idle and nothing else.
>>
>> Okay Christian, Thank you for explaining , I will remove this.
>
> Thanks! I just have one question: Why do you got the impression that
> you need to do this?
It was to migrate the good jpeg contexts out to other instances
unaffected while the reset happens on the current, probably because of
amdgpu_fence_driver_force_completion()
all contexts on this instance are lost (there can be 7 other functional
rings on any instance on JPEG4_0_3) , it was to protect the good
vaapi_ctx workloads like training
data or even another ffmpeg mjpeg decode running in parallel to be
unaffected and continue running on other available instances while reset
is in progress, affecting only the bad job.
without this wait_for_idle on the instance can timeout eventually, that
was the reason for me to go this way.
>
> The problem here is that the higher level takes care of stopping
> submissions and when the backends starts to mess with that state
> (sched.ready for example) it causes tons of problems at other places.
>
> So I had to remove code like this literally between twenty or thirty
> times already and I really need to find a way to better document that
> this is the wrong approach to stop people from coding that in the
> first place.
Oh, my naive understanding around smp_rmb() usage! my apology for that,
will avoid this and also clean up another instance of this usage that I
know off, I used it in a debugfs entry, will clean it up as well.
Thank you again for explaining.
Regards,
Sathish
>
> Thanks,
> Christian.
>
>>
>> Regards,
>> Sathish
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Regards,
>>>> Sathish
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>
>>>>>> + for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
>>>>>> + r = &adev->jpeg.inst[ring->me].ring_dec[j];
>>>>>> + if (r->pipe == j)
>>>>>> + continue;
>>>>>> + ret = SOC15_WAIT_ON_RREG_OFFSET(JPEG, GET_INST(JPEG,
>>>>>> ring->me),
>>>>>> + regUVD_JRBC0_UVD_JRBC_STATUS,
>>>>>> + jpeg_v4_0_3_core_reg_offset(j),
>>>>>> + UVD_JRBC0_UVD_JRBC_STATUS__RB_JOB_DONE_MASK,
>>>>>> + UVD_JRBC0_UVD_JRBC_STATUS__RB_JOB_DONE_MASK);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int jpeg_v4_0_3_ring_reset(struct amdgpu_ring *ring,
>>>>>> unsigned int vmid)
>>>>>> +{
>>>>>> + struct amdgpu_device *adev = ring->adev;
>>>>>> + struct amdgpu_ring *r;
>>>>>> + int ret, j;
>>>>>> +
>>>>>> + if (amdgpu_sriov_vf(adev))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + jpeg_v4_0_3_wait_for_idle_on_inst(ring);
>>>>>> + jpeg_v4_0_3_stop_inst(ring->adev, ring->me);
>>>>>> + jpeg_v4_0_3_start_inst(ring->adev, ring->me);
>>>>>> + for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
>>>>>> + r = &adev->jpeg.inst[ring->me].ring_dec[j];
>>>>>> + jpeg_v4_0_3_start_jrbc(r);
>>>>>> + ret = amdgpu_ring_test_helper(r);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + r->sched.ready = true;
>>>>>> + }
>>>>>> + /* publish update */
>>>>>> + smp_rmb();
>>>>>> + dev_info(adev->dev, "Reset on %s succeeded\n",
>>>>>> ring->sched.name);
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> static const struct amd_ip_funcs jpeg_v4_0_3_ip_funcs = {
>>>>>> .name = "jpeg_v4_0_3",
>>>>>> .early_init = jpeg_v4_0_3_early_init,
>>>>>> @@ -1145,6 +1198,7 @@ static const struct amdgpu_ring_funcs
>>>>>> jpeg_v4_0_3_dec_ring_vm_funcs = {
>>>>>> .emit_wreg = jpeg_v4_0_3_dec_ring_emit_wreg,
>>>>>> .emit_reg_wait = jpeg_v4_0_3_dec_ring_emit_reg_wait,
>>>>>> .emit_reg_write_reg_wait =
>>>>>> amdgpu_ring_emit_reg_write_reg_wait_helper,
>>>>>> + .reset = jpeg_v4_0_3_ring_reset,
>>>>>> };
>>>>>> static void jpeg_v4_0_3_set_dec_ring_funcs(struct
>>>>>> amdgpu_device *adev)
>>>>>
>>>>
>>>
>>
>
More information about the amd-gfx
mailing list