[Public]
> -----Original Message-----
> From: Alex Deucher <[email protected]>
> Sent: Wednesday, October 8, 2025 1:46 PM
> To: Kim, Jonathan <[email protected]>
> Cc: [email protected]; Liu, Shaoyun <[email protected]>
> Subject: Re: [PATCH] drm/amdgpu: fix gfx12 mes packet submission status check
>
> On Wed, Oct 8, 2025 at 1:27 PM Kim, Jonathan <[email protected]>
> wrote:
> >
> > [Public]
> >
> > > -----Original Message-----
> > > From: Alex Deucher <[email protected]>
> > > Sent: Wednesday, October 8, 2025 1:12 PM
> > > To: Kim, Jonathan <[email protected]>
> > > Cc: [email protected]; Liu, Shaoyun <[email protected]>
> > > Subject: Re: [PATCH] drm/amdgpu: fix gfx12 mes packet submission status
> check
> > >
> > > On Wed, Oct 8, 2025 at 12:51 PM Jonathan Kim <[email protected]>
> > > wrote:
> > > >
> > > > The driver currently only checks that the MES packet submission fence
> > > > did not timeout but does not actually check if the fence return status
> > > > matches the expected completion value it passed to MES prior to
> > > > submission.
> > > >
> > > > For example, this can result in REMOVE_QUEUE requests returning success
> > > > to the driver when the queue actually failed to preempt.
> > > >
> > > > Fix this by having the driver actually compare the completion status
> > > > value to the expected success value.
> > >
> > > This should be correct as is:
> > >
> > > *status_ptr = 0;
> >
> > That's not what I'm observing at the moment.
> > amdgpu_fence_wait_polling can still return fine where status_ptr != 0 nor 1.
> > From what I've been told, only 0x1 == success (the completion fence value
> passed to MES).
> > Shaoyun can probably elaborate or correct me if I'm wrong.
> > It's pretty easy for me to put the HW in a bad state (dangling queues post
> remove) without the driver knowing in its current state.
>
> 1 is just what the driver puts as the fence value to write:
> api_status->api_completion_fence_value = 1;
> and the memory location is initialized to 0:
> *status_ptr = 0;
> The firmware should either write 1 or nothing, writing a random value
> in there looks like memory corruption or a firmware bug. If it
> doesn't write the fence, the driver can assume the operation failed.
> If it writes random values, then we have no idea what's going on.
FW writes high 32 bits to status_ptr for debug codes if low bits is 0.
So we should probably only check the 32 lower bits then.
Jon
>
> Alex
>
> >
> > Jon
> >
> > > ...
> > > api_status->api_completion_fence_value = 1;
> > > ...
> > > if (r < 1 || !*status_ptr) {
> > >
> > > Alex
> > >
> > > >
> > > > Signed-off-by: Jonathan Kim <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/amd/amdgpu/mes_v12_0.c | 3 +--
> > > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v12_0.c
> > > b/drivers/gpu/drm/amd/amdgpu/mes_v12_0.c
> > > > index aff06f06aeee..58f61170cf85 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/mes_v12_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/mes_v12_0.c
> > > > @@ -228,8 +228,7 @@ static int
> > > mes_v12_0_submit_pkt_and_poll_completion(struct amdgpu_mes *mes,
> > > > pipe, x_pkt->header.opcode);
> > > >
> > > > r = amdgpu_fence_wait_polling(ring, seq, timeout);
> > > > - if (r < 1 || !*status_ptr) {
> > > > -
> > > > + if (r < 1 || *status_ptr !=
> > > > api_status->api_completion_fence_value) {
> > > > if (misc_op_str)
> > > > dev_err(adev->dev, "MES(%d) failed to respond
> > > > to msg=%s
> > > (%s)\n",
> > > > pipe, op_str, misc_op_str);
> > > > --
> > > > 2.34.1
> > > >
