Re: [PATCH] drm/amdgpu: Add sysfs entries for xgmi hive.
On 2019-03-05 2:16 p.m., Grodzovsky, Andrey wrote: > In each /sys/class/drm/cardX/device/ device you will see the following > > xgmi_device_id /* contains the device id within the hive */ > xgmi_hive_info -> > ../../../../:00:01.1/:02:00.0/:03:00.0/:04:00.0/xgmi_hive_info/ > /* hive info folder */ > > inside xgmi_hive_info are back pointers to /sys/class/drm/cardX/device/ > for each hive member - > > lrwxrwxrwx 1 root root 0 Mar 5 12:27 node1 -> ../../:04:00.0/ > lrwxrwxrwx 1 root root 0 Mar 5 12:27 node2 -> > ../../../../../:00:1b.0/:05:00.0/:06:00.0/:07:00.0/ > > -r--r--r-- 1 root root 4096 Mar 5 12:27 xgmi_hive_id /* contains the > hive id */ Could I get an ssh on such a box to test out the umr code (which I haven't written yet)? Thanks, Tom ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: Add sysfs entries for xgmi hive.
In each /sys/class/drm/cardX/device/ device you will see the following
xgmi_device_id /* contains the device id within the hive */
xgmi_hive_info ->
../../../../:00:01.1/:02:00.0/:03:00.0/:04:00.0/xgmi_hive_info/
/* hive info folder */
inside xgmi_hive_info are back pointers to /sys/class/drm/cardX/device/
for each hive member -
lrwxrwxrwx 1 root root 0 Mar 5 12:27 node1 -> ../../:04:00.0/
lrwxrwxrwx 1 root root 0 Mar 5 12:27 node2 ->
../../../../../:00:1b.0/:05:00.0/:06:00.0/:07:00.0/
-r--r--r-- 1 root root 4096 Mar 5 12:27 xgmi_hive_id /* contains the
hive id */
Andrey
On 3/5/19 1:18 PM, StDenis, Tom wrote:
> Couple of comments inline.
>
> Since I don't have any XGMI gear it would probably help to see what the
> final directory layout/contents look like so I can update umr to
> automagically scan all of this.
>
> Tom
>
> On 2019-03-05 12:47 p.m., Andrey Grodzovsky wrote:
>> For each device a file xgmi_device_id is created.
>> On the first device a subdirectory named xgmi_hive_info is created,
>> It contains a file named hive_id and symlinks named node 1-4 linking
>> to each device in the hive.
>>
>> Signed-off-by: Andrey Grodzovsky
>> ---
>>drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 145
>> ++-
>>drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 6 +-
>>2 files changed, 146 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> index 407dd16c..394be10 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> @@ -34,12 +34,132 @@ static DEFINE_MUTEX(xgmi_mutex);
>>static struct amdgpu_hive_info xgmi_hives[AMDGPU_MAX_XGMI_HIVE];
>>static unsigned hive_count = 0;
>>
>> -
>>void *amdgpu_xgmi_hive_try_lock(struct amdgpu_hive_info *hive)
>>{
>> return >device_list;
>>}
>>
>> +static ssize_t amdgpu_xgmi_show_hive_id(struct device *dev,
>> +struct device_attribute *attr, char *buf)
>> +{
>> +struct amdgpu_hive_info *hive =
>> +container_of(attr, struct amdgpu_hive_info, dev_attr);
>> +
>> +return snprintf(buf, PAGE_SIZE, "%llu\n", hive->hive_id);
>> +}
>> +
>> +static int amdgpu_xgmi_sysfs_create(struct amdgpu_device *adev,
>> +struct amdgpu_hive_info *hive)
>> +{
>> +int ret = 0;
>> +
>> +if (WARN_ON(hive->kobj))
>> +return -1;
>> +
>> +hive->kobj = kobject_create_and_add("xgmi_hive_info", >dev->kobj);
>> +if (!hive->kobj) {
>> +dev_err(adev->dev, "XGMI: Failed to allocate sysfs entry!\n");
>> +return -1;
>> +}
>> +
>> +hive->dev_attr = (struct device_attribute) {
>> +.attr = {
>> +.name = "xgmi_hive_id",
>> +.mode = S_IRUGO,
>> +
>> +},
>> +.show = amdgpu_xgmi_show_hive_id,
>> +};
>> +
>> +ret = sysfs_create_file(hive->kobj, >dev_attr.attr);
>> +if (ret) {
>> +dev_err(adev->dev, "XGMI: Failed to create device file
>> xgmi_hive_id\n");
>> +kobject_del(hive->kobj);
>> +kobject_put(hive->kobj);
>> +hive->kobj = NULL;
>> +}
>> +
>> +return ret;
>> +}
>> +
>> +static void amdgpu_xgmi_sysfs_destroy(struct amdgpu_device *adev,
>> +struct amdgpu_hive_info *hive)
>> +{
>> +sysfs_remove_file(hive->kobj, >dev_attr.attr);
>> +kobject_del(hive->kobj);
>> +kobject_put(hive->kobj);
>> +hive->kobj = NULL;
>> +}
>> +
>> +static ssize_t amdgpu_xgmi_show_device_id(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> +struct drm_device *ddev = dev_get_drvdata(dev);
>> +struct amdgpu_device *adev = ddev->dev_private;
>> +
>> +return snprintf(buf, PAGE_SIZE, "%llu\n", adev->gmc.xgmi.node_id);
>> +
>> +}
>> +
>> +
>> +static DEVICE_ATTR(xgmi_device_id, S_IRUGO, amdgpu_xgmi_show_device_id,
>> NULL);
>> +
>> +
>> +static int amdgpu_xgmi_sysf_add_dev_info(struct amdgpu_device *adev,
>> + struct amdgpu_hive_info *hive)
> Probably meant "sysfs" here right?
>
>> +{
>> +int ret = 0;
>> +char node[10] = { 0 };
>> +
>> +/* Create xgmi device id file */
>> +ret = device_create_file(adev->dev, _attr_xgmi_device_id);
>> +if (ret) {
>> +dev_err(adev->dev, "XGMI: Failed to create device file
>> xgmi_device_id\n");
>> +return ret;
>> +}
>> +
>> +/* Create sysfs link to hive info folder on the first device */
>> +if (adev != hive->adev) {
>> +ret = sysfs_create_link(>dev->kobj, hive->kobj,
>> +"xgmi_hive_info");
>> +if (ret) {
>> +dev_err(adev->dev, "XGMI: Failed to create link to hive
>>
Re: [PATCH] drm/amdgpu: Add sysfs entries for xgmi hive.
Am 05.03.19 um 19:36 schrieb Grodzovsky, Andrey:
> On 3/5/19 1:26 PM, Koenig, Christian wrote:
>> Am 05.03.19 um 19:24 schrieb Grodzovsky, Andrey:
>>> On 3/5/19 1:18 PM, Koenig, Christian wrote:
Am 05.03.19 um 18:47 schrieb Andrey Grodzovsky:
> For each device a file xgmi_device_id is created.
> On the first device a subdirectory named xgmi_hive_info is created,
> It contains a file named hive_id and symlinks named node 1-4 linking
> to each device in the hive.
>
> Signed-off-by: Andrey Grodzovsky
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 145
> ++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 6 +-
> 2 files changed, 146 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> index 407dd16c..394be10 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> @@ -34,12 +34,132 @@ static DEFINE_MUTEX(xgmi_mutex);
> static struct amdgpu_hive_info xgmi_hives[AMDGPU_MAX_XGMI_HIVE];
> static unsigned hive_count = 0;
>
> -
> void *amdgpu_xgmi_hive_try_lock(struct amdgpu_hive_info *hive)
> {
> return >device_list;
> }
>
> +static ssize_t amdgpu_xgmi_show_hive_id(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct amdgpu_hive_info *hive =
> + container_of(attr, struct amdgpu_hive_info, dev_attr);
> +
> + return snprintf(buf, PAGE_SIZE, "%llu\n", hive->hive_id);
> +}
> +
> +static int amdgpu_xgmi_sysfs_create(struct amdgpu_device *adev,
> + struct amdgpu_hive_info *hive)
> +{
> + int ret = 0;
> +
> + if (WARN_ON(hive->kobj))
> + return -1;
Please return proper error codes here.
> +
> + hive->kobj = kobject_create_and_add("xgmi_hive_info", >dev->kobj);
> + if (!hive->kobj) {
> + dev_err(adev->dev, "XGMI: Failed to allocate sysfs entry!\n");
> + return -1;
> + }
> +
> + hive->dev_attr = (struct device_attribute) {
> + .attr = {
> + .name = "xgmi_hive_id",
> + .mode = S_IRUGO,
> +
> + },
> + .show = amdgpu_xgmi_show_hive_id,
> + };
Why do you put the attribute into the hive? Usually those are statically
declared somewhere.
>>> To access the hive from amdgpu_xgmi_show_hive_id using container_of
>> Ok that is rather unusual.
>>
>> Can't we use device_create_file() and a proper device attribute here?
>>
>> See amdgpu_get_dpm_forced_performance_level for an example how to use it.
> device_create_file will attach the file to the device's kobj which will place
> it in sys/class/drm/cardX/device while we want it to reside inside the
> xgmi_hive_info folder, for this i added
> kobj to the hive and use sysfs_create_file directly (device_create_file is
> just a wrapper around this) to specify i want to create the file (device
> property) for hive's kobj.
Gotcha, now I understand. Not sure if that is the right approach, but it
will certainly work.
Alternative would be to embed the kobject for the hive directory in the
hive structure, that's how it is done with the devices as well.
Anyway feel free to go ahead with the current approach.
Christian.
>
> Andrey
>
>> Christian.
>>
>>> Andrey
>>>
>>>
Apart from that looks good to me,
Christian.
> +
> + ret = sysfs_create_file(hive->kobj, >dev_attr.attr);
> + if (ret) {
> + dev_err(adev->dev, "XGMI: Failed to create device file
> xgmi_hive_id\n");
> + kobject_del(hive->kobj);
> + kobject_put(hive->kobj);
> + hive->kobj = NULL;
> + }
> +
> + return ret;
> +}
> +
> +static void amdgpu_xgmi_sysfs_destroy(struct amdgpu_device *adev,
> + struct amdgpu_hive_info *hive)
> +{
> + sysfs_remove_file(hive->kobj, >dev_attr.attr);
> + kobject_del(hive->kobj);
> + kobject_put(hive->kobj);
> + hive->kobj = NULL;
> +}
> +
> +static ssize_t amdgpu_xgmi_show_device_id(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = ddev->dev_private;
> +
> + return snprintf(buf, PAGE_SIZE, "%llu\n", adev->gmc.xgmi.node_id);
> +
> +}
> +
> +
> +static DEVICE_ATTR(xgmi_device_id, S_IRUGO, amdgpu_xgmi_show_device_id,
> NULL);
> +
> +
> +static int amdgpu_xgmi_sysf_add_dev_info(struct amdgpu_device *adev,
> + struct amdgpu_hive_info
Re: [PATCH] drm/amdgpu: Add sysfs entries for xgmi hive.
On 3/5/19 1:26 PM, Koenig, Christian wrote:
> Am 05.03.19 um 19:24 schrieb Grodzovsky, Andrey:
>> On 3/5/19 1:18 PM, Koenig, Christian wrote:
>>> Am 05.03.19 um 18:47 schrieb Andrey Grodzovsky:
For each device a file xgmi_device_id is created.
On the first device a subdirectory named xgmi_hive_info is created,
It contains a file named hive_id and symlinks named node 1-4 linking
to each device in the hive.
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 145
++-
drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 6 +-
2 files changed, 146 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
index 407dd16c..394be10 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
@@ -34,12 +34,132 @@ static DEFINE_MUTEX(xgmi_mutex);
static struct amdgpu_hive_info xgmi_hives[AMDGPU_MAX_XGMI_HIVE];
static unsigned hive_count = 0;
-
void *amdgpu_xgmi_hive_try_lock(struct amdgpu_hive_info *hive)
{
return >device_list;
}
+static ssize_t amdgpu_xgmi_show_hive_id(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct amdgpu_hive_info *hive =
+ container_of(attr, struct amdgpu_hive_info, dev_attr);
+
+ return snprintf(buf, PAGE_SIZE, "%llu\n", hive->hive_id);
+}
+
+static int amdgpu_xgmi_sysfs_create(struct amdgpu_device *adev,
+ struct amdgpu_hive_info *hive)
+{
+ int ret = 0;
+
+ if (WARN_ON(hive->kobj))
+ return -1;
>>> Please return proper error codes here.
>>>
+
+ hive->kobj = kobject_create_and_add("xgmi_hive_info", >dev->kobj);
+ if (!hive->kobj) {
+ dev_err(adev->dev, "XGMI: Failed to allocate sysfs entry!\n");
+ return -1;
+ }
+
+ hive->dev_attr = (struct device_attribute) {
+ .attr = {
+ .name = "xgmi_hive_id",
+ .mode = S_IRUGO,
+
+ },
+ .show = amdgpu_xgmi_show_hive_id,
+ };
>>> Why do you put the attribute into the hive? Usually those are statically
>>> declared somewhere.
>> To access the hive from amdgpu_xgmi_show_hive_id using container_of
> Ok that is rather unusual.
>
> Can't we use device_create_file() and a proper device attribute here?
>
> See amdgpu_get_dpm_forced_performance_level for an example how to use it.
device_create_file will attach the file to the device's kobj which will place
it in sys/class/drm/cardX/device while we want it to reside inside the
xgmi_hive_info folder, for this i added
kobj to the hive and use sysfs_create_file directly (device_create_file is just
a wrapper around this) to specify i want to create the file (device property)
for hive's kobj.
Andrey
>
> Christian.
>
>> Andrey
>>
>>
>>> Apart from that looks good to me,
>>> Christian.
>>>
+
+ ret = sysfs_create_file(hive->kobj, >dev_attr.attr);
+ if (ret) {
+ dev_err(adev->dev, "XGMI: Failed to create device file
xgmi_hive_id\n");
+ kobject_del(hive->kobj);
+ kobject_put(hive->kobj);
+ hive->kobj = NULL;
+ }
+
+ return ret;
+}
+
+static void amdgpu_xgmi_sysfs_destroy(struct amdgpu_device *adev,
+ struct amdgpu_hive_info *hive)
+{
+ sysfs_remove_file(hive->kobj, >dev_attr.attr);
+ kobject_del(hive->kobj);
+ kobject_put(hive->kobj);
+ hive->kobj = NULL;
+}
+
+static ssize_t amdgpu_xgmi_show_device_id(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct drm_device *ddev = dev_get_drvdata(dev);
+ struct amdgpu_device *adev = ddev->dev_private;
+
+ return snprintf(buf, PAGE_SIZE, "%llu\n", adev->gmc.xgmi.node_id);
+
+}
+
+
+static DEVICE_ATTR(xgmi_device_id, S_IRUGO, amdgpu_xgmi_show_device_id,
NULL);
+
+
+static int amdgpu_xgmi_sysf_add_dev_info(struct amdgpu_device *adev,
+ struct amdgpu_hive_info *hive)
+{
+ int ret = 0;
+ char node[10] = { 0 };
+
+ /* Create xgmi device id file */
+ ret = device_create_file(adev->dev, _attr_xgmi_device_id);
+ if (ret) {
+ dev_err(adev->dev, "XGMI: Failed to create device file
xgmi_device_id\n");
+ return ret;
+ }
+
+ /* Create sysfs link to hive info folder on the first device */
+ if (adev != hive->adev) {
+
Re: [PATCH] drm/amdgpu: Add sysfs entries for xgmi hive.
Am 05.03.19 um 19:24 schrieb Grodzovsky, Andrey:
> On 3/5/19 1:18 PM, Koenig, Christian wrote:
>> Am 05.03.19 um 18:47 schrieb Andrey Grodzovsky:
>>> For each device a file xgmi_device_id is created.
>>> On the first device a subdirectory named xgmi_hive_info is created,
>>> It contains a file named hive_id and symlinks named node 1-4 linking
>>> to each device in the hive.
>>>
>>> Signed-off-by: Andrey Grodzovsky
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 145
>>> ++-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 6 +-
>>> 2 files changed, 146 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>>> index 407dd16c..394be10 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>>> @@ -34,12 +34,132 @@ static DEFINE_MUTEX(xgmi_mutex);
>>> static struct amdgpu_hive_info xgmi_hives[AMDGPU_MAX_XGMI_HIVE];
>>> static unsigned hive_count = 0;
>>>
>>> -
>>> void *amdgpu_xgmi_hive_try_lock(struct amdgpu_hive_info *hive)
>>> {
>>> return >device_list;
>>> }
>>>
>>> +static ssize_t amdgpu_xgmi_show_hive_id(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct amdgpu_hive_info *hive =
>>> + container_of(attr, struct amdgpu_hive_info, dev_attr);
>>> +
>>> + return snprintf(buf, PAGE_SIZE, "%llu\n", hive->hive_id);
>>> +}
>>> +
>>> +static int amdgpu_xgmi_sysfs_create(struct amdgpu_device *adev,
>>> + struct amdgpu_hive_info *hive)
>>> +{
>>> + int ret = 0;
>>> +
>>> + if (WARN_ON(hive->kobj))
>>> + return -1;
>> Please return proper error codes here.
>>
>>> +
>>> + hive->kobj = kobject_create_and_add("xgmi_hive_info", >dev->kobj);
>>> + if (!hive->kobj) {
>>> + dev_err(adev->dev, "XGMI: Failed to allocate sysfs entry!\n");
>>> + return -1;
>>> + }
>>> +
>>> + hive->dev_attr = (struct device_attribute) {
>>> + .attr = {
>>> + .name = "xgmi_hive_id",
>>> + .mode = S_IRUGO,
>>> +
>>> + },
>>> + .show = amdgpu_xgmi_show_hive_id,
>>> + };
>> Why do you put the attribute into the hive? Usually those are statically
>> declared somewhere.
> To access the hive from amdgpu_xgmi_show_hive_id using container_of
Ok that is rather unusual.
Can't we use device_create_file() and a proper device attribute here?
See amdgpu_get_dpm_forced_performance_level for an example how to use it.
Christian.
>
> Andrey
>
>
>> Apart from that looks good to me,
>> Christian.
>>
>>> +
>>> + ret = sysfs_create_file(hive->kobj, >dev_attr.attr);
>>> + if (ret) {
>>> + dev_err(adev->dev, "XGMI: Failed to create device file
>>> xgmi_hive_id\n");
>>> + kobject_del(hive->kobj);
>>> + kobject_put(hive->kobj);
>>> + hive->kobj = NULL;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static void amdgpu_xgmi_sysfs_destroy(struct amdgpu_device *adev,
>>> + struct amdgpu_hive_info *hive)
>>> +{
>>> + sysfs_remove_file(hive->kobj, >dev_attr.attr);
>>> + kobject_del(hive->kobj);
>>> + kobject_put(hive->kobj);
>>> + hive->kobj = NULL;
>>> +}
>>> +
>>> +static ssize_t amdgpu_xgmi_show_device_id(struct device *dev,
>>> +struct device_attribute *attr,
>>> +char *buf)
>>> +{
>>> + struct drm_device *ddev = dev_get_drvdata(dev);
>>> + struct amdgpu_device *adev = ddev->dev_private;
>>> +
>>> + return snprintf(buf, PAGE_SIZE, "%llu\n", adev->gmc.xgmi.node_id);
>>> +
>>> +}
>>> +
>>> +
>>> +static DEVICE_ATTR(xgmi_device_id, S_IRUGO, amdgpu_xgmi_show_device_id,
>>> NULL);
>>> +
>>> +
>>> +static int amdgpu_xgmi_sysf_add_dev_info(struct amdgpu_device *adev,
>>> +struct amdgpu_hive_info *hive)
>>> +{
>>> + int ret = 0;
>>> + char node[10] = { 0 };
>>> +
>>> + /* Create xgmi device id file */
>>> + ret = device_create_file(adev->dev, _attr_xgmi_device_id);
>>> + if (ret) {
>>> + dev_err(adev->dev, "XGMI: Failed to create device file
>>> xgmi_device_id\n");
>>> + return ret;
>>> + }
>>> +
>>> + /* Create sysfs link to hive info folder on the first device */
>>> + if (adev != hive->adev) {
>>> + ret = sysfs_create_link(>dev->kobj, hive->kobj,
>>> + "xgmi_hive_info");
>>> + if (ret) {
>>> + dev_err(adev->dev, "XGMI: Failed to create link to hive
>>> info");
>>> + goto remove_file;
>>> + }
>>> + }
>>> +
>>> + sprintf(node, "node%d", hive->number_devices);
>>> + /* Create sysfs link form the hive folder to yorself */
>>> + ret = sysfs_create_link(hive->kobj, >dev->kobj, node);
>>> + if (ret) {
>>> +
Re: [PATCH] drm/amdgpu: Add sysfs entries for xgmi hive.
On 3/5/19 1:18 PM, Koenig, Christian wrote:
> Am 05.03.19 um 18:47 schrieb Andrey Grodzovsky:
>> For each device a file xgmi_device_id is created.
>> On the first device a subdirectory named xgmi_hive_info is created,
>> It contains a file named hive_id and symlinks named node 1-4 linking
>> to each device in the hive.
>>
>> Signed-off-by: Andrey Grodzovsky
>> ---
>>drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 145
>> ++-
>>drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 6 +-
>>2 files changed, 146 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> index 407dd16c..394be10 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> @@ -34,12 +34,132 @@ static DEFINE_MUTEX(xgmi_mutex);
>>static struct amdgpu_hive_info xgmi_hives[AMDGPU_MAX_XGMI_HIVE];
>>static unsigned hive_count = 0;
>>
>> -
>>void *amdgpu_xgmi_hive_try_lock(struct amdgpu_hive_info *hive)
>>{
>> return >device_list;
>>}
>>
>> +static ssize_t amdgpu_xgmi_show_hive_id(struct device *dev,
>> +struct device_attribute *attr, char *buf)
>> +{
>> +struct amdgpu_hive_info *hive =
>> +container_of(attr, struct amdgpu_hive_info, dev_attr);
>> +
>> +return snprintf(buf, PAGE_SIZE, "%llu\n", hive->hive_id);
>> +}
>> +
>> +static int amdgpu_xgmi_sysfs_create(struct amdgpu_device *adev,
>> +struct amdgpu_hive_info *hive)
>> +{
>> +int ret = 0;
>> +
>> +if (WARN_ON(hive->kobj))
>> +return -1;
> Please return proper error codes here.
>
>> +
>> +hive->kobj = kobject_create_and_add("xgmi_hive_info", >dev->kobj);
>> +if (!hive->kobj) {
>> +dev_err(adev->dev, "XGMI: Failed to allocate sysfs entry!\n");
>> +return -1;
>> +}
>> +
>> +hive->dev_attr = (struct device_attribute) {
>> +.attr = {
>> +.name = "xgmi_hive_id",
>> +.mode = S_IRUGO,
>> +
>> +},
>> +.show = amdgpu_xgmi_show_hive_id,
>> +};
> Why do you put the attribute into the hive? Usually those are statically
> declared somewhere.
To access the hive from amdgpu_xgmi_show_hive_id using container_of
Andrey
>
> Apart from that looks good to me,
> Christian.
>
>> +
>> +ret = sysfs_create_file(hive->kobj, >dev_attr.attr);
>> +if (ret) {
>> +dev_err(adev->dev, "XGMI: Failed to create device file
>> xgmi_hive_id\n");
>> +kobject_del(hive->kobj);
>> +kobject_put(hive->kobj);
>> +hive->kobj = NULL;
>> +}
>> +
>> +return ret;
>> +}
>> +
>> +static void amdgpu_xgmi_sysfs_destroy(struct amdgpu_device *adev,
>> +struct amdgpu_hive_info *hive)
>> +{
>> +sysfs_remove_file(hive->kobj, >dev_attr.attr);
>> +kobject_del(hive->kobj);
>> +kobject_put(hive->kobj);
>> +hive->kobj = NULL;
>> +}
>> +
>> +static ssize_t amdgpu_xgmi_show_device_id(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> +struct drm_device *ddev = dev_get_drvdata(dev);
>> +struct amdgpu_device *adev = ddev->dev_private;
>> +
>> +return snprintf(buf, PAGE_SIZE, "%llu\n", adev->gmc.xgmi.node_id);
>> +
>> +}
>> +
>> +
>> +static DEVICE_ATTR(xgmi_device_id, S_IRUGO, amdgpu_xgmi_show_device_id,
>> NULL);
>> +
>> +
>> +static int amdgpu_xgmi_sysf_add_dev_info(struct amdgpu_device *adev,
>> + struct amdgpu_hive_info *hive)
>> +{
>> +int ret = 0;
>> +char node[10] = { 0 };
>> +
>> +/* Create xgmi device id file */
>> +ret = device_create_file(adev->dev, _attr_xgmi_device_id);
>> +if (ret) {
>> +dev_err(adev->dev, "XGMI: Failed to create device file
>> xgmi_device_id\n");
>> +return ret;
>> +}
>> +
>> +/* Create sysfs link to hive info folder on the first device */
>> +if (adev != hive->adev) {
>> +ret = sysfs_create_link(>dev->kobj, hive->kobj,
>> +"xgmi_hive_info");
>> +if (ret) {
>> +dev_err(adev->dev, "XGMI: Failed to create link to hive
>> info");
>> +goto remove_file;
>> +}
>> +}
>> +
>> +sprintf(node, "node%d", hive->number_devices);
>> +/* Create sysfs link form the hive folder to yorself */
>> +ret = sysfs_create_link(hive->kobj, >dev->kobj, node);
>> +if (ret) {
>> +dev_err(adev->dev, "XGMI: Failed to create link from hive
>> info");
>> +goto remove_link;
>> +}
>> +
>> +goto success;
>> +
>> +
>> +remove_link:
>> +sysfs_remove_link(>dev->kobj, adev->ddev->unique);
>> +
>> +remove_file:
>> +device_remove_file(adev->dev, _attr_xgmi_device_id);
>> +
>>
Re: [PATCH] drm/amdgpu: Add sysfs entries for xgmi hive.
Couple of comments inline.
Since I don't have any XGMI gear it would probably help to see what the
final directory layout/contents look like so I can update umr to
automagically scan all of this.
Tom
On 2019-03-05 12:47 p.m., Andrey Grodzovsky wrote:
> For each device a file xgmi_device_id is created.
> On the first device a subdirectory named xgmi_hive_info is created,
> It contains a file named hive_id and symlinks named node 1-4 linking
> to each device in the hive.
>
> Signed-off-by: Andrey Grodzovsky
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 145
> ++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 6 +-
> 2 files changed, 146 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> index 407dd16c..394be10 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> @@ -34,12 +34,132 @@ static DEFINE_MUTEX(xgmi_mutex);
> static struct amdgpu_hive_info xgmi_hives[AMDGPU_MAX_XGMI_HIVE];
> static unsigned hive_count = 0;
>
> -
> void *amdgpu_xgmi_hive_try_lock(struct amdgpu_hive_info *hive)
> {
> return >device_list;
> }
>
> +static ssize_t amdgpu_xgmi_show_hive_id(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct amdgpu_hive_info *hive =
> + container_of(attr, struct amdgpu_hive_info, dev_attr);
> +
> + return snprintf(buf, PAGE_SIZE, "%llu\n", hive->hive_id);
> +}
> +
> +static int amdgpu_xgmi_sysfs_create(struct amdgpu_device *adev,
> + struct amdgpu_hive_info *hive)
> +{
> + int ret = 0;
> +
> + if (WARN_ON(hive->kobj))
> + return -1;
> +
> + hive->kobj = kobject_create_and_add("xgmi_hive_info", >dev->kobj);
> + if (!hive->kobj) {
> + dev_err(adev->dev, "XGMI: Failed to allocate sysfs entry!\n");
> + return -1;
> + }
> +
> + hive->dev_attr = (struct device_attribute) {
> + .attr = {
> + .name = "xgmi_hive_id",
> + .mode = S_IRUGO,
> +
> + },
> + .show = amdgpu_xgmi_show_hive_id,
> + };
> +
> + ret = sysfs_create_file(hive->kobj, >dev_attr.attr);
> + if (ret) {
> + dev_err(adev->dev, "XGMI: Failed to create device file
> xgmi_hive_id\n");
> + kobject_del(hive->kobj);
> + kobject_put(hive->kobj);
> + hive->kobj = NULL;
> + }
> +
> + return ret;
> +}
> +
> +static void amdgpu_xgmi_sysfs_destroy(struct amdgpu_device *adev,
> + struct amdgpu_hive_info *hive)
> +{
> + sysfs_remove_file(hive->kobj, >dev_attr.attr);
> + kobject_del(hive->kobj);
> + kobject_put(hive->kobj);
> + hive->kobj = NULL;
> +}
> +
> +static ssize_t amdgpu_xgmi_show_device_id(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = ddev->dev_private;
> +
> + return snprintf(buf, PAGE_SIZE, "%llu\n", adev->gmc.xgmi.node_id);
> +
> +}
> +
> +
> +static DEVICE_ATTR(xgmi_device_id, S_IRUGO, amdgpu_xgmi_show_device_id,
> NULL);
> +
> +
> +static int amdgpu_xgmi_sysf_add_dev_info(struct amdgpu_device *adev,
> + struct amdgpu_hive_info *hive)
Probably meant "sysfs" here right?
> +{
> + int ret = 0;
> + char node[10] = { 0 };
> +
> + /* Create xgmi device id file */
> + ret = device_create_file(adev->dev, _attr_xgmi_device_id);
> + if (ret) {
> + dev_err(adev->dev, "XGMI: Failed to create device file
> xgmi_device_id\n");
> + return ret;
> + }
> +
> + /* Create sysfs link to hive info folder on the first device */
> + if (adev != hive->adev) {
> + ret = sysfs_create_link(>dev->kobj, hive->kobj,
> + "xgmi_hive_info");
> + if (ret) {
> + dev_err(adev->dev, "XGMI: Failed to create link to hive
> info");
> + goto remove_file;
> + }
> + }
> +
> + sprintf(node, "node%d", hive->number_devices);
> + /* Create sysfs link form the hive folder to yorself */
"yourself"
> + ret = sysfs_create_link(hive->kobj, >dev->kobj, node);
> + if (ret) {
> + dev_err(adev->dev, "XGMI: Failed to create link from hive
> info");
> + goto remove_link;
> + }
> +
> + goto success;
> +
> +
> +remove_link:
> + sysfs_remove_link(>dev->kobj, adev->ddev->unique);
> +
> +remove_file:
> + device_remove_file(adev->dev, _attr_xgmi_device_id);
> +
> +success:
> + return ret;
> +}
> +
> +static void amdgpu_xgmi_sysf_rem_dev_info(struct amdgpu_device *adev,
> +
Re: [PATCH] drm/amdgpu: Add sysfs entries for xgmi hive.
Am 05.03.19 um 18:47 schrieb Andrey Grodzovsky:
> For each device a file xgmi_device_id is created.
> On the first device a subdirectory named xgmi_hive_info is created,
> It contains a file named hive_id and symlinks named node 1-4 linking
> to each device in the hive.
>
> Signed-off-by: Andrey Grodzovsky
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 145
> ++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 6 +-
> 2 files changed, 146 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> index 407dd16c..394be10 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
> @@ -34,12 +34,132 @@ static DEFINE_MUTEX(xgmi_mutex);
> static struct amdgpu_hive_info xgmi_hives[AMDGPU_MAX_XGMI_HIVE];
> static unsigned hive_count = 0;
>
> -
> void *amdgpu_xgmi_hive_try_lock(struct amdgpu_hive_info *hive)
> {
> return >device_list;
> }
>
> +static ssize_t amdgpu_xgmi_show_hive_id(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct amdgpu_hive_info *hive =
> + container_of(attr, struct amdgpu_hive_info, dev_attr);
> +
> + return snprintf(buf, PAGE_SIZE, "%llu\n", hive->hive_id);
> +}
> +
> +static int amdgpu_xgmi_sysfs_create(struct amdgpu_device *adev,
> + struct amdgpu_hive_info *hive)
> +{
> + int ret = 0;
> +
> + if (WARN_ON(hive->kobj))
> + return -1;
Please return proper error codes here.
> +
> + hive->kobj = kobject_create_and_add("xgmi_hive_info", >dev->kobj);
> + if (!hive->kobj) {
> + dev_err(adev->dev, "XGMI: Failed to allocate sysfs entry!\n");
> + return -1;
> + }
> +
> + hive->dev_attr = (struct device_attribute) {
> + .attr = {
> + .name = "xgmi_hive_id",
> + .mode = S_IRUGO,
> +
> + },
> + .show = amdgpu_xgmi_show_hive_id,
> + };
Why do you put the attribute into the hive? Usually those are statically
declared somewhere.
Apart from that looks good to me,
Christian.
> +
> + ret = sysfs_create_file(hive->kobj, >dev_attr.attr);
> + if (ret) {
> + dev_err(adev->dev, "XGMI: Failed to create device file
> xgmi_hive_id\n");
> + kobject_del(hive->kobj);
> + kobject_put(hive->kobj);
> + hive->kobj = NULL;
> + }
> +
> + return ret;
> +}
> +
> +static void amdgpu_xgmi_sysfs_destroy(struct amdgpu_device *adev,
> + struct amdgpu_hive_info *hive)
> +{
> + sysfs_remove_file(hive->kobj, >dev_attr.attr);
> + kobject_del(hive->kobj);
> + kobject_put(hive->kobj);
> + hive->kobj = NULL;
> +}
> +
> +static ssize_t amdgpu_xgmi_show_device_id(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = ddev->dev_private;
> +
> + return snprintf(buf, PAGE_SIZE, "%llu\n", adev->gmc.xgmi.node_id);
> +
> +}
> +
> +
> +static DEVICE_ATTR(xgmi_device_id, S_IRUGO, amdgpu_xgmi_show_device_id,
> NULL);
> +
> +
> +static int amdgpu_xgmi_sysf_add_dev_info(struct amdgpu_device *adev,
> + struct amdgpu_hive_info *hive)
> +{
> + int ret = 0;
> + char node[10] = { 0 };
> +
> + /* Create xgmi device id file */
> + ret = device_create_file(adev->dev, _attr_xgmi_device_id);
> + if (ret) {
> + dev_err(adev->dev, "XGMI: Failed to create device file
> xgmi_device_id\n");
> + return ret;
> + }
> +
> + /* Create sysfs link to hive info folder on the first device */
> + if (adev != hive->adev) {
> + ret = sysfs_create_link(>dev->kobj, hive->kobj,
> + "xgmi_hive_info");
> + if (ret) {
> + dev_err(adev->dev, "XGMI: Failed to create link to hive
> info");
> + goto remove_file;
> + }
> + }
> +
> + sprintf(node, "node%d", hive->number_devices);
> + /* Create sysfs link form the hive folder to yorself */
> + ret = sysfs_create_link(hive->kobj, >dev->kobj, node);
> + if (ret) {
> + dev_err(adev->dev, "XGMI: Failed to create link from hive
> info");
> + goto remove_link;
> + }
> +
> + goto success;
> +
> +
> +remove_link:
> + sysfs_remove_link(>dev->kobj, adev->ddev->unique);
> +
> +remove_file:
> + device_remove_file(adev->dev, _attr_xgmi_device_id);
> +
> +success:
> + return ret;
> +}
> +
> +static void amdgpu_xgmi_sysf_rem_dev_info(struct amdgpu_device *adev,
> + struct amdgpu_hive_info *hive)
> +{
> +
