Re: [PATCH v1 06/15] drm/amdgpu: add print support for vcn_v4_0_3 ip dump

["Khatri, Sunil" <sukhatri@amd.com>]

On 8/7/2024 3:02 AM, Alex Deucher wrote:
> On Tue, Aug 6, 2024 at 4:18 AM Sunil Khatri <sunil.khatri@amd.com> wrote:
>> Add support for logging the registers in devcoredump
>> buffer for vcn_v4_0_3.
>>
>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c | 34 ++++++++++++++++++++++++-
>>   1 file changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
>> index dd3baccb2904..033e5c88527c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
>> @@ -1823,6 +1823,38 @@ static void vcn_v4_0_3_set_irq_funcs(struct amdgpu_device *adev)
>>          adev->vcn.inst->irq.funcs = &vcn_v4_0_3_irq_funcs;
>>   }
>>
>> +static void vcn_v4_0_3_print_ip_state(void *handle, struct drm_printer *p)
>> +{
>> +       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>> +       int i, j;
>> +       uint32_t reg_count = ARRAY_SIZE(vcn_reg_list_4_0_3);
>> +       uint32_t inst_off, is_powered;
>> +
>> +       if (!adev->vcn.ip_dump)
>> +               return;
>> +
>> +       drm_printf(p, "num_instances:%d\n", adev->vcn.num_vcn_inst);
>> +       for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
>> +               if (adev->vcn.harvest_config & (1 << i)) {
>> +                       drm_printf(p, "\nHarvested Instance:VCN%d Skipping dump\n", i);
>> +                       continue;
>> +               }
>> +
>> +               inst_off = i * reg_count;
>> +               is_powered = (adev->vcn.ip_dump[inst_off] &
>> +                               UVD_POWER_STATUS__UVD_POWER_STATUS_MASK) != 1;
> Actually, we shouldn't be checking whether or not VCN is powered up
> when we print the results.  We've already stored the registers so we
> don't care if VCN is powered at this point or not.  VCN could be
> powered down by the time we print this.  It would be better to just
> store a flag to determine whether or not we logged the registers in
> the first place, then use that to determine whether or not we print
> anything.  Same comment for the other VCN print_ip_state callbacks.
This is exactly the same being done here.

is_powered = (adev->vcn.ip_dump[inst_off] &
+                               UVD_POWER_STATUS__UVD_POWER_STATUS_MASK) != 1;
The above is already stored at the time of capturing the dump, i am just checking the value to make sure if it was
powered up at the time of dump and if yes then add logs to devcore dump else skip. Its done this way rather than using a variable as there could
be multiple instances of VCN and one may be powered or not so power state is captured for each instance and based on that value only its we are printing or logging in devcoredump.

Regards
Sunil khatri

>
> Alex
>
>> +
>> +               if (is_powered) {
>> +                       drm_printf(p, "\nActive Instance:VCN%d\n", i);
>> +                       for (j = 0; j < reg_count; j++)
>> +                               drm_printf(p, "%-50s \t 0x%08x\n", vcn_reg_list_4_0_3[j].reg_name,
>> +                                          adev->vcn.ip_dump[inst_off + j]);
>> +               } else {
>> +                       drm_printf(p, "\nInactive Instance:VCN%d\n", i);
>> +               }
>> +       }
>> +}
>> +
>>   static void vcn_v4_0_3_dump_ip_state(void *handle)
>>   {
>>          struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>> @@ -1871,7 +1903,7 @@ static const struct amd_ip_funcs vcn_v4_0_3_ip_funcs = {
>>          .set_clockgating_state = vcn_v4_0_3_set_clockgating_state,
>>          .set_powergating_state = vcn_v4_0_3_set_powergating_state,
>>          .dump_ip_state = vcn_v4_0_3_dump_ip_state,
>> -       .print_ip_state = NULL,
>> +       .print_ip_state = vcn_v4_0_3_print_ip_state,
>>   };
>>
>>   const struct amdgpu_ip_block_version vcn_v4_0_3_ip_block = {
>> --
>> 2.34.1
>>