On Wed, Oct 11, 2017 at 7:28 AM, Rex Zhu <Rex.Zhu at amd.com> wrote:
> Change-Id: Ie06f87445e7d6945472d88ac976693c98d96cd43
> Signed-off-by: Rex Zhu <Rex.Zhu at amd.com>
Please add a better patch description. Something like:
Add a sysfs interface to allocate a smu logging buffer on the fly.
Additionally, wouldn't this be better in debugfs rather than sysfs?
It's not really a user configuration option per se. It's really only
there for debugging power stuff. That said, I'm not sure how tricky
it would be to add it to the existing amdgpu drm debugfs files since
those are read only at the moment and that would be the most logical
place for it. I guess calling debugfs_create_file and using
adev->ddev->primary->debugfs_root should work.
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 134 +++++++++++++++++++++++++++++
> 3 files changed, 141 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index da48f97..9d2f095 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1636,6 +1636,9 @@ struct amdgpu_device {
> bool has_hw_reset;
> u8 reset_magic[AMDGPU_RESET_MAGIC_NUM];
>
> + struct amdgpu_bo *smu_prv_buffer;
> + u32 smu_prv_buffer_size;
Please put these in the amdgpu_pm or amd_powerplay structures instead
since they are power related.
> +
> /* record last mm index being written through WREG32*/
> unsigned long last_mm_index;
> bool in_sriov_reset;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index fdfe418..c9e3019 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2430,6 +2430,10 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
> release_firmware(adev->firmware.gpu_info_fw);
> adev->firmware.gpu_info_fw = NULL;
> }
> + if (adev->smu_prv_buffer) {
> + amdgpu_bo_free_kernel(&adev->smu_prv_buffer, NULL, NULL);
> + adev->smu_prv_buffer = NULL;
> + }
> adev->accel_working = false;
> cancel_delayed_work_sync(&adev->late_init_work);
> /* free i2c buses */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index f3afa66..84e67fb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -737,6 +737,129 @@ static ssize_t amdgpu_set_pp_compute_power_profile(struct device *dev,
> return amdgpu_set_pp_power_profile(dev, buf, count, &request);
> }
>
> +static ssize_t amdgpu_get_pp_alloc_mem_for_smu(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = ddev->dev_private;
> + struct sysinfo si;
> + bool is_os_64 = (sizeof(void *) == 8) ? true : false;
> + uint64_t total_memory;
> + uint64_t dram_size_seven_GB = 0x1B8000000;
> + uint64_t dram_size_three_GB = 0xB8000000;
> + u32 buf_len = 0;
> + u32 gart_size;
> +
> + if (!is_os_64) {
> + adev->smu_prv_buffer_size = 0;
> + buf_len += snprintf(buf + buf_len, PAGE_SIZE,
> + "Feature only supported on 64-bit OS\n");
> + return buf_len;
> + }
> +
> + si_meminfo(&si);
> + total_memory = (uint64_t)si.totalram * si.mem_unit;
> +
> + if (total_memory < dram_size_three_GB) {
> + adev->smu_prv_buffer_size = 0;
> + buf_len += snprintf(buf + buf_len, PAGE_SIZE,
> + "System memory not enough, Feature not enabled\n");
> + return buf_len;
> + }
> +
> + if (total_memory < dram_size_seven_GB)
> + adev->smu_prv_buffer_size = 512;
> + else
> + adev->smu_prv_buffer_size = 2048;
> +
> + buf_len += snprintf(buf + buf_len, PAGE_SIZE,
> + "Max support: %d Mb",
> + adev->smu_prv_buffer_size);
> +
> + gart_size = adev->mc.gart_size >> 20;
> + if (amdgpu_gart_size == -1)
> + buf_len += snprintf(buf + buf_len, PAGE_SIZE,
> + " (Need to set gartsize more than %d Mb)\n",
> + adev->smu_prv_buffer_size + gart_size);
> + else
> + buf_len += snprintf(buf + buf_len, PAGE_SIZE,
> + "\n");
> +
Why do we need all this logic? just return the current value of
smu_priv_buffer_size. This interface is weird; reading it shouldn't
set the smu_prv_buffer_size.
> + return buf_len;
> +}
> +
> +static int amdgpu_alloc_mem_for_smu(struct amdgpu_device *adev, uint32_t size)
> +{
> + int r = -EINVAL;
> + void *cpu_ptr = NULL;
> + uint64_t gpu_addr;
> +
> + if (amdgpu_bo_create_kernel(adev, size,
> + PAGE_SIZE, AMDGPU_GEM_DOMAIN_GTT,
> + &adev->smu_prv_buffer,
> + &gpu_addr,
> + &cpu_ptr)) {
> + DRM_ERROR("amdgpu: failed to create smu prv buffer\n");
> + return r;
> + }
> +
> + if (adev->powerplay.pp_funcs->notify_smu_memory_info)
> + r = amdgpu_dpm_notify_smu_memory_info(adev,
> + lower_32_bits((unsigned long)cpu_ptr),
> + upper_32_bits((unsigned long)cpu_ptr),
> + lower_32_bits(gpu_addr),
> + upper_32_bits(gpu_addr),
> + size);
> +
> + if (r) {
> + amdgpu_bo_free_kernel(&adev->smu_prv_buffer, NULL, NULL);
> + adev->smu_prv_buffer = NULL;
> + DRM_ERROR("amdgpu: failed to notify SMU buffer address.\n");
> + }
> +
> + return r;
> +}
> +
> +static ssize_t amdgpu_set_pp_alloc_mem_for_smu(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = ddev->dev_private;
> + u32 size;
> + int err;
> +
> + if (adev->smu_prv_buffer_size == 0) {
> + pr_info("Please check amdgpu_gtt_mm value first\n");
> + return -EINVAL;
> + }
> +
> + err = kstrtou32(buf, 10, &size);
> + if (err)
> + return err;
> +
> + if (size == 0) {
> + if (adev->smu_prv_buffer) {
> + amdgpu_bo_free_kernel(&adev->smu_prv_buffer, NULL, NULL);
> + adev->smu_prv_buffer = NULL;
> + return count;
> + }
> + return -EINVAL;
> + }
> +
> + if (size > adev->smu_prv_buffer_size) {
> + pr_info("Smu memory size not supported\n");
> + return -EINVAL;
> + }
Same thing here. Just call amdgpu_bo_create_kernel() with the
requested size. If it fails, just return the error message. Apps
will know if they get -ENOMEM that they need to reduce their requested
size or adjust the gart_size parameter.
Alex
> +
> + if (amdgpu_alloc_mem_for_smu(adev, size << 20))
> + count = -EINVAL;
> +
> + return count;
> +}
> +
> static DEVICE_ATTR(power_dpm_state, S_IRUGO | S_IWUSR, amdgpu_get_dpm_state, amdgpu_set_dpm_state);
> static DEVICE_ATTR(power_dpm_force_performance_level, S_IRUGO | S_IWUSR,
> amdgpu_get_dpm_forced_performance_level,
> @@ -770,6 +893,10 @@ static DEVICE_ATTR(pp_gfx_power_profile, S_IRUGO | S_IWUSR,
> static DEVICE_ATTR(pp_compute_power_profile, S_IRUGO | S_IWUSR,
> amdgpu_get_pp_compute_power_profile,
> amdgpu_set_pp_compute_power_profile);
> +static DEVICE_ATTR(pp_alloc_mem_for_smu, S_IRUGO | S_IWUSR,
> + amdgpu_get_pp_alloc_mem_for_smu,
> + amdgpu_set_pp_alloc_mem_for_smu);
> +
>
> static ssize_t amdgpu_hwmon_show_temp(struct device *dev,
> struct device_attribute *attr,
> @@ -1398,6 +1525,13 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
> return ret;
> }
>
> + ret = device_create_file(adev->dev,
> + &dev_attr_pp_alloc_mem_for_smu);
> + if (ret) {
> + DRM_ERROR("failed to create device file pp_alloc_mem_for_smu\n");
> + return ret;
> + }
> +
> ret = amdgpu_debugfs_pm_init(adev);
> if (ret) {
> DRM_ERROR("Failed to register debugfs file for dpm!\n");
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
