Re: [PATCH v2 1/2] drm/amdgpu/mes: Support 64-bit register reads

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 28-Sep-26 10:13 AM, Lazar, Lijo wrote:
> 
> 
> On 24-Sep-26 12:05 PM, Victor Zhao wrote:
>> [why]
>> Some register values span two consecutive dwords and must be sampled
>> with one MES operation.
>>
>> [how]
>> Add amdgpu_mes_rreg64(), set READ_REG read64Bits for MES 11 and
>> MES 12, and combine the two returned dwords into a 64-bit value.
>>
>> v2: add MES 11 support with the same read64Bits option
>>
>> Signed-off-by: Victor Zhao <Victor.Zhao@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c       | 42 +++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h       |  3 ++
>>   drivers/gpu/drm/amd/amdgpu/mes_v11_0.c        |  2 +
>>   drivers/gpu/drm/amd/amdgpu/mes_v12_0.c        |  2 +
>>   drivers/gpu/drm/amd/amdgpu/mes_v12_1.c        |  2 +
>>   drivers/gpu/drm/amd/include/mes_v11_api_def.h |  7 ++++
>>   6 files changed, 58 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c b/drivers/gpu/ 
>> drm/amd/amdgpu/amdgpu_mes.c
>> index 5bebe74de01ad..52b05dc7b74e2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
>> @@ -582,6 +582,7 @@ uint32_t amdgpu_mes_rreg(struct amdgpu_device 
>> *adev, uint32_t reg,
>>       op_input.op = MES_MISC_OP_READ_REG;
>>       op_input.read_reg.reg_offset = reg;
>>       op_input.read_reg.buffer_addr = read_val_gpu_addr;
>> +    op_input.read_reg.read64_bits = false;
>>       if (!adev->mes.funcs->misc_op) {
>>           dev_err(adev->dev, "mes rreg is not supported!\n");
>> @@ -602,6 +603,47 @@ uint32_t amdgpu_mes_rreg(struct amdgpu_device 
>> *adev, uint32_t reg,
>>       return val;
>>   }
>> +uint64_t amdgpu_mes_rreg64(struct amdgpu_device *adev, uint32_t reg,
>> +               uint32_t xcc_id)
>> +{
>> +    struct mes_misc_op_input op_input = { 0 };
>> +    uint64_t val = 0;
>> +    uint32_t addr_offset;
>> +    uint32_t *read_val_ptr;
>> +    int r;
>> +
>> +    if (amdgpu_wb_get(adev, &addr_offset)) {
>> +        dev_err(adev->dev, "critical bug! too many mes readers\n");
>> +        return 0;

For the comment about patch 2, this API should return an error code. 0 
could be misinterpreted as a valid register value.

Thanks,
Lijo
>> +    }
>> +
>> +    read_val_ptr = (uint32_t *)&adev->wb.wb[addr_offset];
>> +    op_input.xcc_id = xcc_id;
>> +    op_input.op = MES_MISC_OP_READ_REG;
>> +    op_input.read_reg.reg_offset = reg;
>> +    op_input.read_reg.buffer_addr = adev->wb.gpu_addr + addr_offset * 4;
>> +    op_input.read_reg.read64_bits = true;
>> +
>> +    if (!adev->mes.funcs->misc_op) {
>> +        dev_err(adev->dev, "mes rreg is not supported!\n");
>> +        goto error;
>> +    }
>> +
>> +    amdgpu_mes_lock(&adev->mes);
>> +    r = adev->mes.funcs->misc_op(&adev->mes, &op_input);
>> +    amdgpu_mes_unlock(&adev->mes);
>> +    if (r) {
>> +        dev_err(adev->dev, "failed to read reg (0x%x)\n", reg);
>> +    } else {
>> +        val = READ_ONCE(read_val_ptr[0]);
>> +        val |= (uint64_t)READ_ONCE(read_val_ptr[1]) << 32;
>> +    }
>> +
>> +error:
>> +    amdgpu_wb_free(adev, addr_offset);
>> +    return val;
>> +}
>> +
>>   int amdgpu_mes_wreg(struct amdgpu_device *adev, uint32_t reg,
>>               uint32_t val, uint32_t xcc_id)
>>   {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h b/drivers/gpu/ 
>> drm/amd/amdgpu/amdgpu_mes.h
>> index 33a426f58e723..d67814124f9e1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
>> @@ -373,6 +373,7 @@ struct mes_misc_op_input {
>>           struct {
>>               uint32_t                  reg_offset;
>>               uint64_t                  buffer_addr;
>> +            bool                      read64_bits;
>>           } read_reg;
>>           struct {
>> @@ -545,6 +546,8 @@ int amdgpu_mes_detect_and_reset_hung_queues(struct 
>> amdgpu_device *adev,
>>   uint32_t amdgpu_mes_rreg(struct amdgpu_device *adev, uint32_t reg,
>>                uint32_t xcc_id);
>> +uint64_t amdgpu_mes_rreg64(struct amdgpu_device *adev, uint32_t reg,
>> +               uint32_t xcc_id);
>>   int amdgpu_mes_wreg(struct amdgpu_device *adev,
>>               uint32_t reg, uint32_t val, uint32_t xcc_id);
>>   int amdgpu_mes_reg_write_reg_wait(struct amdgpu_device *adev,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c b/drivers/gpu/drm/ 
>> amd/amdgpu/mes_v11_0.c
>> index d8b4d3deff605..304087b851644 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
>> @@ -905,6 +905,8 @@ static int mes_v11_0_misc_op(struct amdgpu_mes *mes,
>>           misc_pkt.opcode = MESAPI_MISC__READ_REG;
>>           misc_pkt.read_reg.reg_offset = input->read_reg.reg_offset;
>>           misc_pkt.read_reg.buffer_addr = input->read_reg.buffer_addr;
>> +        misc_pkt.read_reg.option.bits.read64Bits =
>> +            input->read_reg.read64_bits;
> 
> 
> Is this only a missing interface definition or does this need supported 
> FW version check?
> 
> Thanks,
> Lijo
> 
>>           break;
>>       case MES_MISC_OP_WRITE_REG:
>>           misc_pkt.opcode = MESAPI_MISC__WRITE_REG;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v12_0.c b/drivers/gpu/drm/ 
>> amd/amdgpu/mes_v12_0.c
>> index 04465804c2544..0d2a60528802e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/mes_v12_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/mes_v12_0.c
>> @@ -858,6 +858,8 @@ static int mes_v12_0_misc_op(struct amdgpu_mes *mes,
>>           misc_pkt.opcode = MESAPI_MISC__READ_REG;
>>           misc_pkt.read_reg.reg_offset = input->read_reg.reg_offset;
>>           misc_pkt.read_reg.buffer_addr = input->read_reg.buffer_addr;
>> +        misc_pkt.read_reg.option.bits.read64Bits =
>> +            input->read_reg.read64_bits;
>>           break;
>>       case MES_MISC_OP_WRITE_REG:
>>           misc_pkt.opcode = MESAPI_MISC__WRITE_REG;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c b/drivers/gpu/drm/ 
>> amd/amdgpu/mes_v12_1.c
>> index d17ced46d5f73..e0a488e516c09 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
>> @@ -581,6 +581,8 @@ static int mes_v12_1_misc_op(struct amdgpu_mes *mes,
>>       case MES_MISC_OP_READ_REG:
>>           misc_pkt.opcode = MESAPI_MISC__READ_REG;
>>           misc_pkt.read_reg.buffer_addr = input->read_reg.buffer_addr;
>> +        misc_pkt.read_reg.option.bits.read64Bits =
>> +            input->read_reg.read64_bits;
>>           mes_v12_1_get_rrmt(input->read_reg.reg_offset,
>>                      GET_INST(GC, input->xcc_id),
>>                      &misc_pkt.read_reg.rrmt_opt,
>> diff --git a/drivers/gpu/drm/amd/include/mes_v11_api_def.h b/drivers/ 
>> gpu/drm/amd/include/mes_v11_api_def.h
>> index 1fe824b41f2a5..b27ec78da3b08 100644
>> --- a/drivers/gpu/drm/amd/include/mes_v11_api_def.h
>> +++ b/drivers/gpu/drm/amd/include/mes_v11_api_def.h
>> @@ -635,6 +635,13 @@ struct WRITE_REG {
>>   struct READ_REG {
>>       uint32_t                  reg_offset;
>>       uint64_t                  buffer_addr;
>> +    union {
>> +        struct {
>> +            uint32_t read64Bits : 1;
>> +            uint32_t reserved : 31;
>> +        } bits;
>> +        uint32_t all;
>> +    } option;
>>   };
>>   enum WRM_OPERATION {
>