Am 15.08.2017 um 13:33 schrieb Tom St Denis:
> On 15/08/17 07:22 AM, Christian König wrote:
>> Am 14.08.2017 um 13:37 schrieb Tom St Denis:
>>> This patch adds trace points to the amdgpu bo_kmap/bo/kunmap functions
>>> which capture internal allocations.
>>>
>>> (v2): Simply add a sleep to the init. Users can use this script to
>>> load with map debugging enabled:
>>>
>>> #!/bin/bash
>>> modprobe amdgpu map_debug=1 &
>>> sleep 1
>>> echo 1 >
>>> /sys/kernel/debug/tracing/events/amdgpu/amdgpu_ttm_tt_unpopulate/enable
>>> echo 1 >
>>> /sys/kernel/debug/tracing/events/amdgpu/amdgpu_ttm_tt_populate/enable
>>
>> I've just realized that there is a far simpler method than that or
>> enabling trace points using the module command line.
>>
>> Assuming your GPU is PCI device 01:00.0 connected through bridge
>> 00:02.1 (use lspci -t to figure the connection out):
>>
>> # Temporary remove the PCIe device from the bus
>> echo 1 > /sys/bus/pci/devices/0000\:01\:00.0/remove
>> # Load amdgpu, this allows you to enable the trace points you want
>> and also sets probes etc..
>> modprobe amdpgu
>> # Rescan the bus, the device subsystem will automatically probe
>> amdgpu with this device
>> echo 1 > /sys/bus/pci/devices/0000\:00\:02.1/rescan
>
> That would definitely be a bunch trickier to script up. Nobody will
> run this by hand. So we'd need to remove all devices that are AMD
> vendor ID but GPU only (e.g. not other controllers) which means we
> need to parse the lspci output for VGA controller related text.
>
> Definitely doable and probably "cleaner" from a race point of view.
>
>> Apart from that I would really like to see those trace points in TTM
>> instead. I also don't mind adding a dev or pdev pointer to the
>> ttm_bo_device structure for this.
>
> Not against this in principle. There's a bit of confusion... our ttm
> amdgpu functions are passed
>
> struct ttm_tt *ttm
>
> which we cast to
>
> struct amdgpu_ttm_tt {
> struct ttm_dma_tt ttm;
> struct amdgpu_device *adev;
BTW: Isn't this adev pointer here superflous? I mean the ttm_tt has a
bdev pointer to use, don't they?
> u64 offset;
> ... <snip>
>
> Then we use the "->ttm" of that to get dma_address[] but pages[] from
> the original ttm_tt structure ... from what I recall (when I looked
> into this last week) ttm_dma_tt and ttm_tt aren't the same structure
> right? (and looking at it right now ttm_dma_tt begins with ttm_tt....).
>
> So wherever I put the trace in ttm I need access to dma_address[]
> which doesn't seem so clear cut. Can I just cast a ttm_tt pointer to
> ttm_dma_tt?
Ah, ok now I understand your problem. Well, printing the dma addresses
in this situation is probably not a good idea to start with.
When a buffer object is mapped to kernel space there is no guarantee
that it already has it's DMA mapping. For amdgpu we probably never run
into this situation, but for other drivers that might now be the case.
Do we really need that info?
Christian.
>
> Tom
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Signed-off-by: Tom St Denis <tom.stdenis at amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 ++++++++++-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 ++--
>>> 4 files changed, 25 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index d2aaad77c353..2f5781df88c5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -121,6 +121,7 @@ extern int amdgpu_cntl_sb_buf_per_se;
>>> extern int amdgpu_param_buf_per_se;
>>> extern int amdgpu_job_hang_limit;
>>> extern int amdgpu_lbpw;
>>> +extern int amdgpu_enable_map_debugging;
>>> #ifdef CONFIG_DRM_AMDGPU_SI
>>> extern int amdgpu_si_support;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index 2cdf8443e7d3..0ed777cc2f63 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -120,6 +120,7 @@ int amdgpu_cntl_sb_buf_per_se = 0;
>>> int amdgpu_param_buf_per_se = 0;
>>> int amdgpu_job_hang_limit = 0;
>>> int amdgpu_lbpw = -1;
>>> +int amdgpu_enable_map_debugging = 0;
>>> MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in
>>> megabytes");
>>> module_param_named(vramlimit, amdgpu_vram_limit, int, 0600);
>>> @@ -263,6 +264,9 @@ module_param_named(job_hang_limit,
>>> amdgpu_job_hang_limit, int ,0444);
>>> MODULE_PARM_DESC(lbpw, "Load Balancing Per Watt (LBPW) support (1
>>> = enable, 0 = disable, -1 = auto)");
>>> module_param_named(lbpw, amdgpu_lbpw, int, 0444);
>>> +MODULE_PARM_DESC(map_debug, "Enable map debugging in kernel (1 =
>>> on, 0 = off)");
>>> +module_param_named(map_debug, amdgpu_enable_map_debugging, int, 0444);
>>> +
>>> #ifdef CONFIG_DRM_AMDGPU_SI
>>> #if defined(CONFIG_DRM_RADEON) || defined(CONFIG_DRM_RADEON_MODULE)
>>> @@ -865,11 +869,19 @@ static struct pci_driver amdgpu_kms_pci_driver
>>> = {
>>> };
>>> +// enable trace events during init
>>> +static void amdgpu_enable_dma_tracers(void)
>>> +{
>>> + msleep(3000);
>>> +}
>>> static int __init amdgpu_init(void)
>>> {
>>> int r;
>>> + if (amdgpu_enable_map_debugging)
>>> + amdgpu_enable_dma_tracers();
>>> +
>>> r = amdgpu_sync_init();
>>> if (r)
>>> goto error_sync;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index e7e899190bef..a292e86fbaa7 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -37,6 +37,9 @@
>>> #include "amdgpu.h"
>>> #include "amdgpu_trace.h"
>>> +void amdgpu_trace_dma_map(struct ttm_tt *ttm);
>>> +void amdgpu_trace_dma_unmap(struct ttm_tt *ttm);
>>> +
>>> static void amdgpu_ttm_bo_destroy(struct ttm_buffer_object *tbo)
>>> {
>>> struct amdgpu_device *adev = amdgpu_ttm_adev(tbo->bdev);
>>> @@ -625,6 +628,9 @@ int amdgpu_bo_kmap(struct amdgpu_bo *bo, void
>>> **ptr)
>>> if (r)
>>> return r;
>>> + if (unlikely(trace_amdgpu_ttm_tt_populate_enabled()) &&
>>> bo->tbo.mem.mem_type != TTM_PL_VRAM)
>>> + amdgpu_trace_dma_map(bo->tbo.ttm);
>>> +
>>> if (ptr)
>>> *ptr = amdgpu_bo_kptr(bo);
>>> @@ -640,8 +646,11 @@ void *amdgpu_bo_kptr(struct amdgpu_bo *bo)
>>> void amdgpu_bo_kunmap(struct amdgpu_bo *bo)
>>> {
>>> - if (bo->kmap.bo)
>>> + if (bo->kmap.bo) {
>>> + if (unlikely(trace_amdgpu_ttm_tt_unpopulate_enabled()) &&
>>> bo->tbo.mem.mem_type != TTM_PL_VRAM)
>>> + amdgpu_trace_dma_unmap(bo->tbo.ttm);
>>> ttm_bo_kunmap(&bo->kmap);
>>> + }
>>> }
>>> struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo)
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 26665b4baf36..85727be3fbb7 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -663,7 +663,7 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt
>>> *ttm, struct page **pages)
>>> return r;
>>> }
>>> -static void amdgpu_trace_dma_map(struct ttm_tt *ttm)
>>> +void amdgpu_trace_dma_map(struct ttm_tt *ttm)
>>> {
>>> struct amdgpu_device *adev = amdgpu_ttm_adev(ttm->bdev);
>>> struct amdgpu_ttm_tt *gtt = (void *)ttm;
>>> @@ -679,7 +679,7 @@ static void amdgpu_trace_dma_map(struct ttm_tt
>>> *ttm)
>>> }
>>> }
>>> -static void amdgpu_trace_dma_unmap(struct ttm_tt *ttm)
>>> +void amdgpu_trace_dma_unmap(struct ttm_tt *ttm)
>>> {
>>> struct amdgpu_device *adev = amdgpu_ttm_adev(ttm->bdev);
>>> struct amdgpu_ttm_tt *gtt = (void *)ttm;
>>
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
