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@xxxxxxx>
---
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)
