On Tue, Aug 6, 2024 at 11:37 PM Khatri, Sunil <sukha...@amd.com> wrote:
>
>
> On 8/7/2024 3:02 AM, Alex Deucher wrote:
> > On Tue, Aug 6, 2024 at 4:18 AM Sunil Khatri <sunil.kha...@amd.com> wrote:
> >> Add support for logging the registers in devcoredump
> >> buffer for vcn_v4_0_3.
> >>
> >> Signed-off-by: Sunil Khatri <sunil.kha...@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.
Ah, yes, you are right. My brain misread that.
Alex
>
> 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
> >>
