Re: [PATCH 1/1] drm/amdgpu: cleanup sysfs file handling

["Das, Nirmoy" <nirmoy.das@xxxxxxx>]

On 5/7/2020 9:21 PM, Alex Deucher wrote:
> On Thu, May 7, 2020 at 4:15 PM Nirmoy Das <nirmoy.das@xxxxxxx> wrote:
> > Create sysfs file using attributes when possible.
> >
> > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  36 +++----
> Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
> for the amdgpu_device.c changes.  For amdgpu_pm.c, I think Kevin has a
> patch set out to clean this up as well that also fixes up the VF
> handling.  May want to check with him on the pm changes.


Thanks Alex, I didn't know. I will check with Kevin.


Regards,

Nirmoy

> Alex
>
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c     | 114 +++++++--------------
> >   2 files changed, 48 insertions(+), 102 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index bf302c799832..cc41e8f5ad14 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -2918,6 +2918,14 @@ static int amdgpu_device_get_job_timeout_settings(struct amdgpu_device *adev)
> >          return ret;
> >   }
> >
> > +static const struct attribute *amdgpu_dev_attributes[] = {
> > +       &dev_attr_product_name.attr,
> > +       &dev_attr_product_number.attr,
> > +       &dev_attr_serial_number.attr,
> > +       &dev_attr_pcie_replay_count.attr,
> > +       NULL
> > +};
> > +
> >   /**
> >    * amdgpu_device_init - initialize the driver
> >    *
> > @@ -3267,27 +3275,9 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >          queue_delayed_work(system_wq, &adev->delayed_init_work,
> >                             msecs_to_jiffies(AMDGPU_RESUME_MS));
> >
> > -       r = device_create_file(adev->dev, &dev_attr_pcie_replay_count);
> > -       if (r) {
> > -               dev_err(adev->dev, "Could not create pcie_replay_count");
> > -               return r;
> > -       }
> > -
> > -       r = device_create_file(adev->dev, &dev_attr_product_name);
> > +       r = sysfs_create_files(&adev->dev->kobj, amdgpu_dev_attributes);
> >          if (r) {
> > -               dev_err(adev->dev, "Could not create product_name");
> > -               return r;
> > -       }
> > -
> > -       r = device_create_file(adev->dev, &dev_attr_product_number);
> > -       if (r) {
> > -               dev_err(adev->dev, "Could not create product_number");
> > -               return r;
> > -       }
> > -
> > -       r = device_create_file(adev->dev, &dev_attr_serial_number);
> > -       if (r) {
> > -               dev_err(adev->dev, "Could not create serial_number");
> > +               dev_err(adev->dev, "Could not create amdgpu device attr\n");
> >                  return r;
> >          }
> >
> > @@ -3370,12 +3360,10 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
> >          adev->rmmio = NULL;
> >          amdgpu_device_doorbell_fini(adev);
> >
> > -       device_remove_file(adev->dev, &dev_attr_pcie_replay_count);
> >          if (adev->ucode_sysfs_en)
> >                  amdgpu_ucode_sysfs_fini(adev);
> > -       device_remove_file(adev->dev, &dev_attr_product_name);
> > -       device_remove_file(adev->dev, &dev_attr_product_number);
> > -       device_remove_file(adev->dev, &dev_attr_serial_number);
> > +
> > +       sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
> >          if (IS_ENABLED(CONFIG_PERF_EVENTS))
> >                  amdgpu_pmu_fini(adev);
> >          if (amdgpu_discovery && adev->asic_type >= CHIP_NAVI10)
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > index c762deb5abc7..f375bc341acc 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > @@ -3239,6 +3239,27 @@ int amdgpu_pm_load_smu_firmware(struct amdgpu_device *adev, uint32_t *smu_versio
> >          return 0;
> >   }
> >
> > +static const struct attribute *amdgpu_pm_attributes[] = {
> > +       &dev_attr_power_dpm_state.attr,
> > +       &dev_attr_power_dpm_force_performance_level.attr,
> > +       &dev_attr_pp_dpm_sclk.attr,
> > +       &dev_attr_pp_dpm_mclk.attr,
> > +
> > +       &dev_attr_pp_sclk_od.attr,
> > +       &dev_attr_pp_mclk_od.attr,
> > +       &dev_attr_pp_power_profile_mode.attr,
> > +       &dev_attr_gpu_busy_percent.attr,
> > +       NULL
> > +};
> > +
> > +static const struct attribute *amdgpu_pm_non_vf_attributes[] = {
> > +       &dev_attr_pp_num_states.attr,
> > +       &dev_attr_pp_cur_state.attr,
> > +       &dev_attr_pp_force_state.attr,
> > +       &dev_attr_pp_table.attr,
> > +       NULL
> > +};
> > +
> >   int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
> >   {
> >          struct pp_hwmgr *hwmgr = adev->powerplay.pp_handle;
> > @@ -3260,45 +3281,6 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
> >                  return ret;
> >          }
> >
> > -       ret = device_create_file(adev->dev, &dev_attr_power_dpm_state);
> > -       if (ret) {
> > -               DRM_ERROR("failed to create device file for dpm state\n");
> > -               return ret;
> > -       }
> > -       ret = device_create_file(adev->dev, &dev_attr_power_dpm_force_performance_level);
> > -       if (ret) {
> > -               DRM_ERROR("failed to create device file for dpm state\n");
> > -               return ret;
> > -       }
> > -
> > -       if (!amdgpu_sriov_vf(adev)) {
> > -               ret = device_create_file(adev->dev, &dev_attr_pp_num_states);
> > -               if (ret) {
> > -                       DRM_ERROR("failed to create device file pp_num_states\n");
> > -                       return ret;
> > -               }
> > -               ret = device_create_file(adev->dev, &dev_attr_pp_cur_state);
> > -               if (ret) {
> > -                       DRM_ERROR("failed to create device file pp_cur_state\n");
> > -                       return ret;
> > -               }
> > -               ret = device_create_file(adev->dev, &dev_attr_pp_force_state);
> > -               if (ret) {
> > -                       DRM_ERROR("failed to create device file pp_force_state\n");
> > -                       return ret;
> > -               }
> > -               ret = device_create_file(adev->dev, &dev_attr_pp_table);
> > -               if (ret) {
> > -                       DRM_ERROR("failed to create device file pp_table\n");
> > -                       return ret;
> > -               }
> > -       }
> > -
> > -       ret = device_create_file(adev->dev, &dev_attr_pp_dpm_sclk);
> > -       if (ret) {
> > -               DRM_ERROR("failed to create device file pp_dpm_sclk\n");
> > -               return ret;
> > -       }
> >
> >          /* Arcturus does not support standalone mclk/socclk/fclk level setting */
> >          if (adev->asic_type == CHIP_ARCTURUS) {
> > @@ -3312,11 +3294,20 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
> >                  dev_attr_pp_dpm_fclk.store = NULL;
> >          }
> >
> > -       ret = device_create_file(adev->dev, &dev_attr_pp_dpm_mclk);
> > +       ret = sysfs_create_files(&adev->dev->kobj, amdgpu_pm_attributes);
> >          if (ret) {
> > -               DRM_ERROR("failed to create device file pp_dpm_mclk\n");
> > +               DRM_ERROR("failed to create pm sysfs files\n");
> >                  return ret;
> >          }
> > +
> > +       if (!amdgpu_sriov_vf(adev)) {
> > +               ret = sysfs_create_files(&adev->dev->kobj, amdgpu_pm_non_vf_attributes);
> > +               if (ret) {
> > +                       DRM_ERROR("failed to create pm sysfs files\n");
> > +                       return ret;
> > +               }
> > +       }
> > +
> >          if (adev->asic_type >= CHIP_VEGA10) {
> >                  ret = device_create_file(adev->dev, &dev_attr_pp_dpm_socclk);
> >                  if (ret) {
> > @@ -3352,23 +3343,7 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
> >                          return ret;
> >                  }
> >          }
> > -       ret = device_create_file(adev->dev, &dev_attr_pp_sclk_od);
> > -       if (ret) {
> > -               DRM_ERROR("failed to create device file pp_sclk_od\n");
> > -               return ret;
> > -       }
> > -       ret = device_create_file(adev->dev, &dev_attr_pp_mclk_od);
> > -       if (ret) {
> > -               DRM_ERROR("failed to create device file pp_mclk_od\n");
> > -               return ret;
> > -       }
> > -       ret = device_create_file(adev->dev,
> > -                       &dev_attr_pp_power_profile_mode);
> > -       if (ret) {
> > -               DRM_ERROR("failed to create device file "
> > -                               "pp_power_profile_mode\n");
> > -               return ret;
> > -       }
> > +
> >          if ((is_support_sw_smu(adev) && adev->smu.od_enabled) ||
> >              (!is_support_sw_smu(adev) && hwmgr->od_enabled)) {
> >                  ret = device_create_file(adev->dev,
> > @@ -3379,13 +3354,6 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
> >                          return ret;
> >                  }
> >          }
> > -       ret = device_create_file(adev->dev,
> > -                       &dev_attr_gpu_busy_percent);
> > -       if (ret) {
> > -               DRM_ERROR("failed to create device file "
> > -                               "gpu_busy_level\n");
> > -               return ret;
> > -       }
> >          /* APU does not have its own dedicated memory */
> >          if (!(adev->flags & AMD_IS_APU) &&
> >               (adev->asic_type != CHIP_VEGA10)) {
> > @@ -3437,16 +3405,11 @@ void amdgpu_pm_sysfs_fini(struct amdgpu_device *adev)
> >
> >          if (adev->pm.int_hwmon_dev)
> >                  hwmon_device_unregister(adev->pm.int_hwmon_dev);
> > -       device_remove_file(adev->dev, &dev_attr_power_dpm_state);
> > -       device_remove_file(adev->dev, &dev_attr_power_dpm_force_performance_level);
> >
> > -       device_remove_file(adev->dev, &dev_attr_pp_num_states);
> > -       device_remove_file(adev->dev, &dev_attr_pp_cur_state);
> > -       device_remove_file(adev->dev, &dev_attr_pp_force_state);
> > -       device_remove_file(adev->dev, &dev_attr_pp_table);
> > +       sysfs_remove_files(&adev->dev->kobj, amdgpu_pm_attributes);
> > +       if (!amdgpu_sriov_vf(adev))
> > +               sysfs_remove_files(&adev->dev->kobj, amdgpu_pm_non_vf_attributes);
> >
> > -       device_remove_file(adev->dev, &dev_attr_pp_dpm_sclk);
> > -       device_remove_file(adev->dev, &dev_attr_pp_dpm_mclk);
> >          if (adev->asic_type >= CHIP_VEGA10) {
> >                  device_remove_file(adev->dev, &dev_attr_pp_dpm_socclk);
> >                  if (adev->asic_type != CHIP_ARCTURUS)
> > @@ -3456,15 +3419,10 @@ void amdgpu_pm_sysfs_fini(struct amdgpu_device *adev)
> >                  device_remove_file(adev->dev, &dev_attr_pp_dpm_pcie);
> >          if (adev->asic_type >= CHIP_VEGA20)
> >                  device_remove_file(adev->dev, &dev_attr_pp_dpm_fclk);
> > -       device_remove_file(adev->dev, &dev_attr_pp_sclk_od);
> > -       device_remove_file(adev->dev, &dev_attr_pp_mclk_od);
> > -       device_remove_file(adev->dev,
> > -                       &dev_attr_pp_power_profile_mode);
> >          if ((is_support_sw_smu(adev) && adev->smu.od_enabled) ||
> >              (!is_support_sw_smu(adev) && hwmgr->od_enabled))
> >                  device_remove_file(adev->dev,
> >                                  &dev_attr_pp_od_clk_voltage);
> > -       device_remove_file(adev->dev, &dev_attr_gpu_busy_percent);
> >          if (!(adev->flags & AMD_IS_APU) &&
> >               (adev->asic_type != CHIP_VEGA10))
> >                  device_remove_file(adev->dev, &dev_attr_mem_busy_percent);
> > --
> > 2.26.2
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cnirmoy.das%40amd.com%7C7347df60ad514491ffb908d7f2c4383b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637244796884676621&amp;sdata=XjTrg30N9ifFJ79Ykl08OqhgIXGfZi6WxmZiFEgebJE%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx