Received-SPF: pass (google.com: domain of linux-kernel+bounces-94050-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240306090408.3453152-1-sunil.khatri@amd.com>
 <2f792620-fd8a-412e-9130-e276ba36d5a0@amd.com> <5e2899cd-75b4-4ddd-97ff-4e10a2e67fbb@amd.com>
 <66815303-bd9c-4dfc-ae1a-bbdc5d1bb47c@amd.com> <17e12147-79dd-44ba-b8ae-b96fb72dcfbd@amd.com>
In-Reply-To: <17e12147-79dd-44ba-b8ae-b96fb72dcfbd@amd.com>
From: Alex Deucher <alexdeucher@gmail.com>
Date: Wed, 6 Mar 2024 09:29:00 -0500
Message-ID: <CADnq5_OkeH1x4YgSv6uw0HLb5c-5NOXnzQPJHsDvb=NmEePB-A@mail.gmail.com>
Subject: Re: [PATCH] drm/amdgpu: cache in more vm fault information
To: "Khatri, Sunil" <sukhatri@amd.com>
Cc: =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>, 
	Sunil Khatri <sunil.khatri@amd.com>, Alex Deucher <alexander.deucher@amd.com>, 
	Shashank Sharma <shashank.sharma@amd.com>, amd-gfx@lists.freedesktop.org, 
	Pan@rtg-sunil-navi33.amd.com, Xinhui <Xinhui.Pan@amd.com>, 
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, 
	Mukul Joshi <mukul.joshi@amd.com>, 
	Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 6, 2024 at 8:04=E2=80=AFAM Khatri, Sunil <sukhatri@amd.com> wro=
te:
>
>
> On 3/6/2024 6:12 PM, Christian K=C3=B6nig wrote:
> > Am 06.03.24 um 11:40 schrieb Khatri, Sunil:
> >>
> >> On 3/6/2024 3:37 PM, Christian K=C3=B6nig wrote:
> >>> Am 06.03.24 um 10:04 schrieb Sunil Khatri:
> >>>> When an  page fault interrupt is raised there
> >>>> is a lot more information that is useful for
> >>>> developers to analyse the pagefault.
> >>>
> >>> Well actually those information are not that interesting because
> >>> they are hw generation specific.
> >>>
> >>> You should probably rather use the decoded strings here, e.g. hub,
> >>> client, xcc_id, node_id etc...
> >>>
> >>> See gmc_v9_0_process_interrupt() an example.
> >>> I saw this v9 does provide more information than what v10 and v11
> >>> provide like node_id and fault from which die but thats again very
> >>> specific to IP_VERSION(9, 4, 3)) i dont know why thats information
> >>> is not there in v10 and v11.
> >>
> >> I agree to your point but, as of now during a pagefault we are
> >> dumping this information which is useful like which client
> >> has generated an interrupt and for which src and other information
> >> like address. So i think to provide the similar information in the
> >> devcoredump.
> >>
> >> Currently we do not have all this information from either job or vm
> >> being derived from the job during a reset. We surely could add more
> >> relevant information later on as per request but this information is
> >> useful as
> >> eventually its developers only who would use the dump file provided
> >> by customer to debug.
> >>
> >> Below is the information that i dump in devcore and i feel that is
> >> good information but new information could be added which could be
> >> picked later.
> >>
> >>> Page fault information
> >>> [gfxhub] page fault (src_id:0 ring:24 vmid:3 pasid:32773)
> >>> in page starting at address 0x0000000000000000 from client 0x1b (UTCL=
2)
> >
> > This is a perfect example what I mean. You record in the patch is the
> > client_id, but this is is basically meaningless unless you have access
> > to the AMD internal hw documentation.
> >
> > What you really need is the client in decoded form, in this case
> > UTCL2. You can keep the client_id additionally, but the decoded client
> > string is mandatory to have I think.
> >
> > Sure i am capturing that information as i am trying to minimise the
> > memory interaction to minimum as we are still in interrupt context
> > here that why i recorded the integer information compared to decoding
> and writing strings there itself but to postpone till we dump.
>
> Like decoding to the gfxhub/mmhub based on vmhub/vmid_src and client
> string from client id. So are we good to go with the information with
> the above information of sharing details in devcoredump using the
> additional information from pagefault cached.

I think amdgpu_vm_fault_info() has everything you need already (vmhub,
status, and addr).  client_id and src_id are just tokens in the
interrupt cookie so we know which IP to route the interrupt to.  We
know what they will be because otherwise we'd be in the interrupt
handler for a different IP.  I don't think ring_id has any useful
information in this context and vmid and pasid are probably not too
useful because they are just tokens to associate the fault with a
process.  It would be better to have the process name.

Alex

>
> regards
> sunil
>
> >
> > Regards,
> > Christian.
> >
> >>
> >> Regards
> >> Sunil Khatri
> >>
> >>>
> >>> Regards,
> >>> Christian.
> >>>
> >>>>
> >>>> Add all such information in the last cached
> >>>> pagefault from an interrupt handler.
> >>>>
> >>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> >>>> ---
> >>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++++--
> >>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 7 ++++++-
> >>>>   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 2 +-
> >>>>   drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c | 2 +-
> >>>>   drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c  | 2 +-
> >>>>   drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c  | 2 +-
> >>>>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 2 +-
> >>>>   7 files changed, 18 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>> index 4299ce386322..b77e8e28769d 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>> @@ -2905,7 +2905,7 @@ void amdgpu_debugfs_vm_bo_info(struct
> >>>> amdgpu_vm *vm, struct seq_file *m)
> >>>>    * Cache the fault info for later use by userspace in debugging.
> >>>>    */
> >>>>   void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
> >>>> -                  unsigned int pasid,
> >>>> +                  struct amdgpu_iv_entry *entry,
> >>>>                     uint64_t addr,
> >>>>                     uint32_t status,
> >>>>                     unsigned int vmhub)
> >>>> @@ -2915,7 +2915,7 @@ void amdgpu_vm_update_fault_cache(struct
> >>>> amdgpu_device *adev,
> >>>>         xa_lock_irqsave(&adev->vm_manager.pasids, flags);
> >>>>   -    vm =3D xa_load(&adev->vm_manager.pasids, pasid);
> >>>> +    vm =3D xa_load(&adev->vm_manager.pasids, entry->pasid);
> >>>>       /* Don't update the fault cache if status is 0.  In the multip=
le
> >>>>        * fault case, subsequent faults will return a 0 status which =
is
> >>>>        * useless for userspace and replaces the useful fault
> >>>> status, so
> >>>> @@ -2924,6 +2924,11 @@ void amdgpu_vm_update_fault_cache(struct
> >>>> amdgpu_device *adev,
> >>>>       if (vm && status) {
> >>>>           vm->fault_info.addr =3D addr;
> >>>>           vm->fault_info.status =3D status;
> >>>> +        vm->fault_info.client_id =3D entry->client_id;
> >>>> +        vm->fault_info.src_id =3D entry->src_id;
> >>>> +        vm->fault_info.vmid =3D entry->vmid;
> >>>> +        vm->fault_info.pasid =3D entry->pasid;
> >>>> +        vm->fault_info.ring_id =3D entry->ring_id;
> >>>>           if (AMDGPU_IS_GFXHUB(vmhub)) {
> >>>>               vm->fault_info.vmhub =3D AMDGPU_VMHUB_TYPE_GFX;
> >>>>               vm->fault_info.vmhub |=3D
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> >>>> index 047ec1930d12..c7782a89bdb5 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> >>>> @@ -286,6 +286,11 @@ struct amdgpu_vm_fault_info {
> >>>>       uint32_t    status;
> >>>>       /* which vmhub? gfxhub, mmhub, etc. */
> >>>>       unsigned int    vmhub;
> >>>> +    unsigned int    client_id;
> >>>> +    unsigned int    src_id;
> >>>> +    unsigned int    ring_id;
> >>>> +    unsigned int    pasid;
> >>>> +    unsigned int    vmid;
> >>>>   };
> >>>>     struct amdgpu_vm {
> >>>> @@ -605,7 +610,7 @@ static inline void
> >>>> amdgpu_vm_eviction_unlock(struct amdgpu_vm *vm)
> >>>>   }
> >>>>     void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
> >>>> -                  unsigned int pasid,
> >>>> +                  struct amdgpu_iv_entry *entry,
> >>>>                     uint64_t addr,
> >>>>                     uint32_t status,
> >>>>                     unsigned int vmhub);
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> >>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> >>>> index d933e19e0cf5..6b177ce8db0e 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> >>>> @@ -150,7 +150,7 @@ static int gmc_v10_0_process_interrupt(struct
> >>>> amdgpu_device *adev,
> >>>>           status =3D RREG32(hub->vm_l2_pro_fault_status);
> >>>>           WREG32_P(hub->vm_l2_pro_fault_cntl, 1, ~1);
> >>>>   -        amdgpu_vm_update_fault_cache(adev, entry->pasid, addr,
> >>>> status,
> >>>> +        amdgpu_vm_update_fault_cache(adev, entry, addr, status,
> >>>>                            entry->vmid_src ? AMDGPU_MMHUB0(0) :
> >>>> AMDGPU_GFXHUB(0));
> >>>>       }
> >>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> >>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> >>>> index 527dc917e049..bcf254856a3e 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> >>>> @@ -121,7 +121,7 @@ static int gmc_v11_0_process_interrupt(struct
> >>>> amdgpu_device *adev,
> >>>>           status =3D RREG32(hub->vm_l2_pro_fault_status);
> >>>>           WREG32_P(hub->vm_l2_pro_fault_cntl, 1, ~1);
> >>>>   -        amdgpu_vm_update_fault_cache(adev, entry->pasid, addr,
> >>>> status,
> >>>> +        amdgpu_vm_update_fault_cache(adev, entry, addr, status,
> >>>>                            entry->vmid_src ? AMDGPU_MMHUB0(0) :
> >>>> AMDGPU_GFXHUB(0));
> >>>>       }
> >>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> >>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> >>>> index 3da7b6a2b00d..e9517ebbe1fd 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> >>>> @@ -1270,7 +1270,7 @@ static int gmc_v7_0_process_interrupt(struct
> >>>> amdgpu_device *adev,
> >>>>       if (!addr && !status)
> >>>>           return 0;
> >>>>   -    amdgpu_vm_update_fault_cache(adev, entry->pasid,
> >>>> +    amdgpu_vm_update_fault_cache(adev, entry,
> >>>>                        ((u64)addr) << AMDGPU_GPU_PAGE_SHIFT,
> >>>> status, AMDGPU_GFXHUB(0));
> >>>>         if (amdgpu_vm_fault_stop =3D=3D AMDGPU_VM_FAULT_STOP_FIRST)
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> >>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> >>>> index d20e5f20ee31..a271bf832312 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> >>>> @@ -1438,7 +1438,7 @@ static int gmc_v8_0_process_interrupt(struct
> >>>> amdgpu_device *adev,
> >>>>       if (!addr && !status)
> >>>>           return 0;
> >>>>   -    amdgpu_vm_update_fault_cache(adev, entry->pasid,
> >>>> +    amdgpu_vm_update_fault_cache(adev, entry,
> >>>>                        ((u64)addr) << AMDGPU_GPU_PAGE_SHIFT,
> >>>> status, AMDGPU_GFXHUB(0));
> >>>>         if (amdgpu_vm_fault_stop =3D=3D AMDGPU_VM_FAULT_STOP_FIRST)
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> >>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> >>>> index 47b63a4ce68b..dc9fb1fb9540 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> >>>> @@ -666,7 +666,7 @@ static int gmc_v9_0_process_interrupt(struct
> >>>> amdgpu_device *adev,
> >>>>       rw =3D REG_GET_FIELD(status, VM_L2_PROTECTION_FAULT_STATUS, RW=
);
> >>>>       WREG32_P(hub->vm_l2_pro_fault_cntl, 1, ~1);
> >>>>   -    amdgpu_vm_update_fault_cache(adev, entry->pasid, addr,
> >>>> status, vmhub);
> >>>> +    amdgpu_vm_update_fault_cache(adev, entry, addr, status, vmhub);
> >>>>         dev_err(adev->dev,
> >>>>           "VM_L2_PROTECTION_FAULT_STATUS:0x%08X\n",
> >>>
> >