Am 12.10.2017 um 05:48 schrieb Alex Deucher:
> 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.
We also already have writeable debugfs files. Just take a look at how
the register or VRAM accessors work.
Should be trivial to move over if you have sysfs already working.
Regards,
Christian.
>
>
>> ---
>> 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
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
