[PATCH] drm/amd/pp: add new sysfs pp_alloc_mem_for_smu

Christian K├Ânig ckoenig.leichtzumerken at gmail.com
Thu Oct 12 08:10:54 UTC 2017


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




More information about the amd-gfx mailing list