The issue with the pcie_bw file is that the sent/received information needs to be obtained simultaneously, so those calculations need to be together and thus can't be split over two files (aside: I did find an old article about the whole one-value-per-file thing, where it's described as being a rule that ~10% of sysfs files don't follow; https://lwn.net/Articles/378884/) . I have no issue splitting this memory usage apart. The values don't NEED to be together, so I can definitely split them up to try to keep with the spirit of sysfs. This also means that if the hammer drops and all sysfs files must conform to that standard at some point in the future, I'll have less work to do. Kent > -----Original Message----- > From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of > Deucher, Alexander > Sent: Thursday, February 28, 2019 11:13 AM > To: Kuehling, Felix <Felix.Kuehling@xxxxxxx>; Koenig, Christian > <Christian.Koenig@xxxxxxx>; Russell, Kent <Kent.Russell@xxxxxxx>; > amd-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: RE: [PATCH] drm/amdgpu: Add sysfs files for returning VRAM/GTT > info > > > -----Original Message----- > > From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of > > Kuehling, Felix > > Sent: Thursday, February 28, 2019 11:09 AM > > To: Koenig, Christian <Christian.Koenig@xxxxxxx>; Russell, Kent > > <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx > > Subject: Re: [PATCH] drm/amdgpu: Add sysfs files for returning > > VRAM/GTT info > > > > On 2/28/2019 9:56 AM, Christian König wrote: > > > Am 28.02.19 um 16:32 schrieb Russell, Kent: > > >> Add 3 files that return: > > >> The total amount of VRAM and the current total used VRAM The total > > >> amount of VRAM and the current total used visible VRAM The total > > >> GTT size and the current total of used GTT > > >> > > >> Each returns 2 integers, total and used, in bytes > > > > > > Well that is a good start, but unfortunately violates the rules for > > > sysfs. You need to return one value per file. > > > > Is this rule written down anywhere. I see that space-separated lists > > of things are common. E.g. scaling_available_governors in the cpufreq > directories. > > > > In Documentation/admin-guide/sysfs-rules.rst I don't see any rule > > about single value per file. Maybe that's because these rules are more > > from user mode usage of sysfs rather than for kernel implementations. > > This (two values) is also more consistent with the pcie bw file IIRC. > > Alex > > > > > Regards, > > Felix > > > > > > > > > > So you should create 6 files in total. > > > > > > Regards, > > > Christian. > > > > > >> > > >> Change-Id: I0bd702b166b4253887ef76fb1bba8b9aadc7e2c5 > > >> Signed-off-by: Kent Russell <kent.russell@xxxxxxx> > > >> --- > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 36 > +++++++++++ > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 67 > > >> ++++++++++++++++++++ > > >> 2 files changed, 103 insertions(+) > > >> > > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c > > >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c > > >> index da7b1b92d9cf..adfa211c5152 100644 > > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c > > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c > > >> @@ -36,6 +36,30 @@ struct amdgpu_gtt_node { > > >> struct ttm_buffer_object *tbo; > > >> }; > > >> +/** > > >> + * DOC: mem_info_gtt > > >> + * > > >> + * The amdgpu driver provides a sysfs API for reporting current > > >> + GTT > > >> information > > >> + * The file mem_info_gtt is used for this. > > >> + * The file returns the total size of the GTT block and the > > >> + current > > >> amount of > > >> + * used GTT as 2 separate integers, in bytes */ static ssize_t > > >> +amdgpu_mem_info_gtt_show(struct device *dev, > > >> + struct device_attribute *attr, char *buf) { > > >> + struct drm_device *ddev = dev_get_drvdata(dev); > > >> + struct amdgpu_device *adev = ddev->dev_private; > > >> + uint64_t used_gtt, total_gtt; > > >> + > > >> + used_gtt = > > >> +amdgpu_gtt_mgr_usage(&adev->mman.bdev.man[TTM_PL_TT]); > > >> + total_gtt = (adev->mman.bdev.man[TTM_PL_TT].size) * PAGE_SIZE; > > >> + > > >> + return snprintf(buf, PAGE_SIZE, "%llu %llu\n", > > >> + total_gtt, used_gtt); } > > >> + > > >> +static DEVICE_ATTR(mem_info_gtt, S_IRUGO, > > amdgpu_mem_info_gtt_show, > > >> NULL); > > >> + > > >> /** > > >> * amdgpu_gtt_mgr_init - init GTT manager and DRM MM > > >> * > > >> @@ -50,6 +74,7 @@ static int amdgpu_gtt_mgr_init(struct > > >> ttm_mem_type_manager *man, > > >> struct amdgpu_device *adev = amdgpu_ttm_adev(man->bdev); > > >> struct amdgpu_gtt_mgr *mgr; > > >> uint64_t start, size; > > >> + int ret; > > >> mgr = kzalloc(sizeof(*mgr), GFP_KERNEL); > > >> if (!mgr) > > >> @@ -61,6 +86,13 @@ static int amdgpu_gtt_mgr_init(struct > > >> ttm_mem_type_manager *man, > > >> spin_lock_init(&mgr->lock); > > >> atomic64_set(&mgr->available, p_size); > > >> man->priv = mgr; > > >> + > > >> + ret = device_create_file(adev->dev, &dev_attr_mem_info_gtt); > > >> + if (ret) { > > >> + DRM_ERROR("Failed to create device file mem_info_gtt\n"); > > >> + return ret; > > >> + } > > >> + > > >> return 0; > > >> } > > >> @@ -74,12 +106,16 @@ static int amdgpu_gtt_mgr_init(struct > > >> ttm_mem_type_manager *man, > > >> */ > > >> static int amdgpu_gtt_mgr_fini(struct ttm_mem_type_manager *man) > > >> { > > >> + struct amdgpu_device *adev = amdgpu_ttm_adev(man->bdev); > > >> struct amdgpu_gtt_mgr *mgr = man->priv; > > >> spin_lock(&mgr->lock); > > >> drm_mm_takedown(&mgr->mm); > > >> spin_unlock(&mgr->lock); > > >> kfree(mgr); > > >> man->priv = NULL; > > >> + > > >> + device_remove_file(adev->dev, &dev_attr_mem_info_gtt); > > >> + > > >> return 0; > > >> } > > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > > >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > > >> index 3f9d5d00c9b3..d0bada997cba 100644 > > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > > >> @@ -32,6 +32,55 @@ struct amdgpu_vram_mgr { > > >> atomic64_t vis_usage; > > >> }; > > >> +/** > > >> + * DOC: mem_info_vram > > >> + * > > >> + * The amdgpu driver provides a sysfs API for reporting current > > >> + VRAM > > >> information > > >> + * The file mem_info_vram is used for this. > > >> + * The file returns the total amount of VRAM and the current total > > >> amount of > > >> + * used VRAM as 2 separate integers, in bytes */ static ssize_t > > >> +amdgpu_mem_info_vram_show(struct device *dev, > > >> + struct device_attribute *attr, char *buf) { > > >> + struct drm_device *ddev = dev_get_drvdata(dev); > > >> + struct amdgpu_device *adev = ddev->dev_private; > > >> + uint64_t used_vram, total_vram; > > >> + > > >> + used_vram = > > >> amdgpu_vram_mgr_usage(&adev- > >mman.bdev.man[TTM_PL_VRAM]); > > >> + total_vram = adev->gmc.real_vram_size; > > >> + > > >> + return snprintf(buf, PAGE_SIZE, "%llu %llu\n", total_vram, > > >> used_vram); > > >> +} > > >> + > > >> +/** > > >> + * DOC: mem_info_vis_vram > > >> + * > > >> + * The amdgpu driver provides a sysfs API for reporting current > > >> visible VRAM > > >> + * information > > >> + * The file mem_info_vis_vram is used for this. > > >> + * The file returns the total amount of VRAM and the current total > > >> amount of > > >> + * used visible VRAM as 2 separate integers, in bytes */ static > > >> +ssize_t amdgpu_mem_info_vis_vram_show(struct device *dev, > > >> + struct device_attribute *attr, char *buf) { > > >> + struct drm_device *ddev = dev_get_drvdata(dev); > > >> + struct amdgpu_device *adev = ddev->dev_private; > > >> + uint64_t used_vis_vram, total_vram; > > >> + > > >> + used_vis_vram = > > >> + amdgpu_vram_mgr_vis_usage(&adev- > > >mman.bdev.man[TTM_PL_VRAM]); > > >> + total_vram = adev->gmc.real_vram_size; > > >> + > > >> + return snprintf(buf, PAGE_SIZE, "%llu %llu\n", > > >> + total_vram, used_vis_vram); } > > >> + > > >> +static DEVICE_ATTR(mem_info_vram, S_IRUGO, > > >> amdgpu_mem_info_vram_show, NULL); > > >> +static DEVICE_ATTR(mem_info_vis_vram, S_IRUGO, > > >> amdgpu_mem_info_vis_vram_show, > > >> + NULL); > > >> + > > >> /** > > >> * amdgpu_vram_mgr_init - init VRAM manager and DRM MM > > >> * > > >> @@ -43,7 +92,9 @@ struct amdgpu_vram_mgr { > > >> static int amdgpu_vram_mgr_init(struct ttm_mem_type_manager > > *man, > > >> unsigned long p_size) > > >> { > > >> + struct amdgpu_device *adev = amdgpu_ttm_adev(man->bdev); > > >> struct amdgpu_vram_mgr *mgr; > > >> + int ret; > > >> mgr = kzalloc(sizeof(*mgr), GFP_KERNEL); > > >> if (!mgr) > > >> @@ -52,6 +103,19 @@ static int amdgpu_vram_mgr_init(struct > > >> ttm_mem_type_manager *man, > > >> drm_mm_init(&mgr->mm, 0, p_size); > > >> spin_lock_init(&mgr->lock); > > >> man->priv = mgr; > > >> + > > >> + /* Add the two VRAM-related sysfs files */ > > >> + ret = device_create_file(adev->dev, &dev_attr_mem_info_vram); > > >> + if (ret) { > > >> + DRM_ERROR("Failed to create device file mem_info_vram\n"); > > >> + return ret; > > >> + } > > >> + ret = device_create_file(adev->dev, > > >> +&dev_attr_mem_info_vis_vram); > > >> + if (ret) { > > >> + DRM_ERROR("Failed to create device file > > >> +mem_info_vis_vram\n"); > > >> + return ret; > > >> + } > > >> + > > >> return 0; > > >> } > > >> @@ -65,6 +129,7 @@ static int amdgpu_vram_mgr_init(struct > > >> ttm_mem_type_manager *man, > > >> */ > > >> static int amdgpu_vram_mgr_fini(struct ttm_mem_type_manager > > *man) > > >> { > > >> + struct amdgpu_device *adev = amdgpu_ttm_adev(man->bdev); > > >> struct amdgpu_vram_mgr *mgr = man->priv; > > >> spin_lock(&mgr->lock); > > >> @@ -72,6 +137,8 @@ static int amdgpu_vram_mgr_fini(struct > > >> ttm_mem_type_manager *man) > > >> spin_unlock(&mgr->lock); > > >> kfree(mgr); > > >> man->priv = NULL; > > >> + device_remove_file(adev->dev, &dev_attr_mem_info_vram); > > >> + device_remove_file(adev->dev, &dev_attr_mem_info_vis_vram); > > >> return 0; > > >> } > > > > > > _______________________________________________ > > > amd-gfx mailing list > > > amd-gfx@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ > amd-gfx mailing list > amd-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
