[PATCH 3/3] drm/amdkfd: gfx12 context save/restore trap handler fixes

[Lancelot SIX]

On 23/05/2024 20:31, Jay Cornwall wrote:
> On 5/23/2024 13:37, Lancelot SIX wrote:
> 
>>> @@ -622,8 +638,15 @@ L_SAVE_HWREG:
>>>   #if ASIC_FAMILY >= CHIP_GFX12
>>>       // Ensure no further changes to barrier or LDS state.
>>> +    // STATE_PRIV.BARRIER_COMPLETE may change up to this point.
>>>       s_barrier_signal    -2
>>>       s_barrier_wait    -2
>>> +
>>> +    // Re-read final state of BARRIER_COMPLETE field for save.
>>> +    s_getreg_b32    s_save_tmp, hwreg(S_STATUS_HWREG)
>>> +    s_and_b32    s_save_tmp, s_save_tmp, 
>>> SQ_WAVE_STATE_PRIV_BARRIER_COMPLETE_MASK
>>> +    s_andn2_b32    s_save_status, s_save_status, 
>>> SQ_WAVE_STATE_PRIV_BARRIER_COMPLETE_MASK
>>
>> Even if BARRIER_COMPLETE can be asserted while we are in the trap 
>> hadler, I do not think it can be cleared.  That being said, it might 
>> be easier to just replace the bit, making it clearer.
> 
> Yes, I chose to structure it this way to make the intent clearer. We 
> don't gain much from dropping the s_andn2. Most of the time spent in the 
> save handler is stalled on memory instructions.
> 
>>> @@ -1351,7 +1369,17 @@ L_SKIP_BARRIER_RESTORE:
>>>       s_setreg_b32    hwreg(HW_REG_SHADER_XNACK_MASK), 
>>> s_restore_xnack_mask
>>>   #endif
>>> +#if ASIC_FAMILY < CHIP_GFX12
>>>       s_setreg_b32    hwreg(S_TRAPSTS_HWREG), s_restore_trapsts
>>
>> Wouldn't other gfx1x architectures have a similar issue when writing 
>> TRAPSTS here?  That is if TRAPSTS.SAVECTX is set while we are 
>> restoring, wouldn't we loose it?
>>
>> And for gfx11, there is TRAPSTS.HOST_TRAP that could have the same 
>> issue to some degree (not sure if we would loose the host trap 
>> completly, or re-enter with trap ID + HT bit set in ttmp1).
> 
> Prior to gfx12 context save and host trap exceptions are not delivered 
> to a wave until STATUS.PRIV=0, i.e. it leaves the trap handler.
> 
> The changes needed for gfx12 are due to a design change in this area. 
> Exceptions are now flagged immediately and cause re-entry to the trap if 
> any are non-zero.

Thanks for the clarifications.  The patch looks good to me.

Reviewed-by: Lancelot Six <lancelot.six at amd.com>

Best,
Lancelot.