Re: [PATCH v5 09/27] dmr/amdgpu: Move some sysfs attrs creation to default_attr

2021-04-29 Thread Christian König 

Am 28.04.21 um 17:11 schrieb Andrey Grodzovsky:

This allows to remove explicit creation and destruction
of those attrs and by this avoids warnings on device
finilizing post physical device extraction.

v5: Use newly added pci_driver.dev_groups directly

Signed-off-by: Andrey Grodzovsky 


Looks like a nice cleanup to me, but this is beyond my understand of sysfs.

Acked-by: Christian König 

Christian.


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 17 ++---
  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 13 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c  | 25 
  drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 14 ---
  4 files changed, 37 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
index 86add0f4ea4d..0346e124ab8c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -1953,6 +1953,15 @@ static ssize_t amdgpu_atombios_get_vbios_version(struct 
device *dev,
  static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
   NULL);
  
+static struct attribute *amdgpu_vbios_version_attrs[] = {

+   &dev_attr_vbios_version.attr,
+   NULL
+};
+
+const struct attribute_group amdgpu_vbios_version_attr_group = {
+   .attrs = amdgpu_vbios_version_attrs
+};
+
  /**
   * amdgpu_atombios_fini - free the driver info and callbacks for atombios
   *
@@ -1972,7 +1981,6 @@ void amdgpu_atombios_fini(struct amdgpu_device *adev)
adev->mode_info.atom_context = NULL;
kfree(adev->mode_info.atom_card_info);
adev->mode_info.atom_card_info = NULL;
-   device_remove_file(adev->dev, &dev_attr_vbios_version);
  }
  
  /**

@@ -1989,7 +1997,6 @@ int amdgpu_atombios_init(struct amdgpu_device *adev)
  {
struct card_info *atom_card_info =
kzalloc(sizeof(struct card_info), GFP_KERNEL);
-   int ret;
  
  	if (!atom_card_info)

return -ENOMEM;
@@ -2027,12 +2034,6 @@ int amdgpu_atombios_init(struct amdgpu_device *adev)
amdgpu_atombios_allocate_fb_scratch(adev);
}
  
-	ret = device_create_file(adev->dev, &dev_attr_vbios_version);

-   if (ret) {
-   DRM_ERROR("Failed to create device file for VBIOS version\n");
-   return ret;
-   }
-
return 0;
  }
  
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

index 54cb5ee2f563..f799c40d7e72 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1605,6 +1605,18 @@ static struct pci_error_handlers amdgpu_pci_err_handler 
= {
.resume = amdgpu_pci_resume,
  };
  
+extern const struct attribute_group amdgpu_vram_mgr_attr_group;

+extern const struct attribute_group amdgpu_gtt_mgr_attr_group;
+extern const struct attribute_group amdgpu_vbios_version_attr_group;
+
+static const struct attribute_group *amdgpu_sysfs_groups[] = {
+   &amdgpu_vram_mgr_attr_group,
+   &amdgpu_gtt_mgr_attr_group,
+   &amdgpu_vbios_version_attr_group,
+   NULL,
+};
+
+
  static struct pci_driver amdgpu_kms_pci_driver = {
.name = DRIVER_NAME,
.id_table = pciidlist,
@@ -1613,6 +1625,7 @@ static struct pci_driver amdgpu_kms_pci_driver = {
.shutdown = amdgpu_pci_shutdown,
.driver.pm = &amdgpu_pm_ops,
.err_handler = &amdgpu_pci_err_handler,
+   .dev_groups = amdgpu_sysfs_groups,
  };
  
  static int __init amdgpu_init(void)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
index 8980329cded0..3b7150e1c5ed 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
@@ -77,6 +77,16 @@ static DEVICE_ATTR(mem_info_gtt_total, S_IRUGO,
  static DEVICE_ATTR(mem_info_gtt_used, S_IRUGO,
   amdgpu_mem_info_gtt_used_show, NULL);
  
+static struct attribute *amdgpu_gtt_mgr_attributes[] = {

+   &dev_attr_mem_info_gtt_total.attr,
+   &dev_attr_mem_info_gtt_used.attr,
+   NULL
+};
+
+const struct attribute_group amdgpu_gtt_mgr_attr_group = {
+   .attrs = amdgpu_gtt_mgr_attributes
+};
+
  static const struct ttm_resource_manager_func amdgpu_gtt_mgr_func;
  /**
   * amdgpu_gtt_mgr_init - init GTT manager and DRM MM
@@ -91,7 +101,6 @@ int amdgpu_gtt_mgr_init(struct amdgpu_device *adev, uint64_t 
gtt_size)
struct amdgpu_gtt_mgr *mgr = &adev->mman.gtt_mgr;
struct ttm_resource_manager *man = &mgr->manager;
uint64_t start, size;
-   int ret;
  
  	man->use_tt = true;

man->func = &amdgpu_gtt_mgr_func;
@@ -104,17 +113,6 @@ int amdgpu_gtt_mgr_init(struct amdgpu_device *adev, 
uint64_t gtt_size)
spin_lock_init(&mgr->lock);
atomic64_set(&mgr->available, gtt_size >> PAGE_SHIFT);
  
-	ret = device_create_file(adev->dev, &dev_attr_mem_info_gtt_

Re: [PATCH v5 09/27] dmr/amdgpu: Move some sysfs attrs creation to default_attr

2021-04-28 Thread Bjorn Helgaas 
In subject,

s/dmr/drm/
s/Move some/Move/ ("some" consumes space without adding meaning)

Or maybe something like: 

  drm/amdgpu: Convert driver sysfs attributes to static attributes

On Wed, Apr 28, 2021 at 11:11:49AM -0400, Andrey Grodzovsky wrote:
> This allows to remove explicit creation and destruction
> of those attrs and by this avoids warnings on device
> finilizing post physical device extraction.

s/finilizing/finalizing/

> v5: Use newly added pci_driver.dev_groups directly

I don't know the DRM convention, but IMO, change notes like "v5: Use
..." can go after "---" so they don't go in the git log.  To me,
they're useful during review, but not after being merged.

I love the patch!  Much cleaner than creating/removing all these
attributes explicitly.

> Signed-off-by: Andrey Grodzovsky 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 17 ++---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 13 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c  | 25 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 14 ---
>  4 files changed, 37 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> index 86add0f4ea4d..0346e124ab8c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> @@ -1953,6 +1953,15 @@ static ssize_t 
> amdgpu_atombios_get_vbios_version(struct device *dev,
>  static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
>  NULL);
>  
> +static struct attribute *amdgpu_vbios_version_attrs[] = {
> + &dev_attr_vbios_version.attr,
> + NULL
> +};
> +
> +const struct attribute_group amdgpu_vbios_version_attr_group = {
> + .attrs = amdgpu_vbios_version_attrs
> +};
> +
>  /**
>   * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>   *
> @@ -1972,7 +1981,6 @@ void amdgpu_atombios_fini(struct amdgpu_device *adev)
>   adev->mode_info.atom_context = NULL;
>   kfree(adev->mode_info.atom_card_info);
>   adev->mode_info.atom_card_info = NULL;
> - device_remove_file(adev->dev, &dev_attr_vbios_version);
>  }
>  
>  /**
> @@ -1989,7 +1997,6 @@ int amdgpu_atombios_init(struct amdgpu_device *adev)
>  {
>   struct card_info *atom_card_info =
>   kzalloc(sizeof(struct card_info), GFP_KERNEL);
> - int ret;
>  
>   if (!atom_card_info)
>   return -ENOMEM;
> @@ -2027,12 +2034,6 @@ int amdgpu_atombios_init(struct amdgpu_device *adev)
>   amdgpu_atombios_allocate_fb_scratch(adev);
>   }
>  
> - ret = device_create_file(adev->dev, &dev_attr_vbios_version);
> - if (ret) {
> - DRM_ERROR("Failed to create device file for VBIOS version\n");
> - return ret;
> - }
> -
>   return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 54cb5ee2f563..f799c40d7e72 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1605,6 +1605,18 @@ static struct pci_error_handlers 
> amdgpu_pci_err_handler = {
>   .resume = amdgpu_pci_resume,
>  };
>  
> +extern const struct attribute_group amdgpu_vram_mgr_attr_group;
> +extern const struct attribute_group amdgpu_gtt_mgr_attr_group;
> +extern const struct attribute_group amdgpu_vbios_version_attr_group;
> +
> +static const struct attribute_group *amdgpu_sysfs_groups[] = {
> + &amdgpu_vram_mgr_attr_group,
> + &amdgpu_gtt_mgr_attr_group,
> + &amdgpu_vbios_version_attr_group,
> + NULL,
> +};
> +
> +
>  static struct pci_driver amdgpu_kms_pci_driver = {
>   .name = DRIVER_NAME,
>   .id_table = pciidlist,
> @@ -1613,6 +1625,7 @@ static struct pci_driver amdgpu_kms_pci_driver = {
>   .shutdown = amdgpu_pci_shutdown,
>   .driver.pm = &amdgpu_pm_ops,
>   .err_handler = &amdgpu_pci_err_handler,
> + .dev_groups = amdgpu_sysfs_groups,
>  };
>  
>  static int __init amdgpu_init(void)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> index 8980329cded0..3b7150e1c5ed 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> @@ -77,6 +77,16 @@ static DEVICE_ATTR(mem_info_gtt_total, S_IRUGO,
>  static DEVICE_ATTR(mem_info_gtt_used, S_IRUGO,
>  amdgpu_mem_info_gtt_used_show, NULL);
>  
> +static struct attribute *amdgpu_gtt_mgr_attributes[] = {
> + &dev_attr_mem_info_gtt_total.attr,
> + &dev_attr_mem_info_gtt_used.attr,
> + NULL
> +};
> +
> +const struct attribute_group amdgpu_gtt_mgr_attr_group = {
> + .attrs = amdgpu_gtt_mgr_attributes
> +};
> +
>  static const struct ttm_resource_manager_func amdgpu_gtt_mgr_func;
>  /**
>   * amdgpu_gtt_mgr_init - init GTT manager and DRM MM
> @@ -91,7 +101,6 @@ int amdgpu_gtt_mgr_init(struct