Re: [PATCH v6 4/5] drm/amdgpu: add UMA allocation interfaces to sysfs

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 16-Dec-25 7:38 PM, Mario Limonciello wrote:
> On 12/16/25 3:43 AM, Lazar, Lijo wrote:
>>
>>
>> On 12-Dec-25 1:29 PM, Yo-Jung Leo Lin (AMD) wrote:
>>> Add a uma/ directory containing two sysfs files as interfaces to
>>> inspect or change UMA carveout size. These files are:
>>>
>>> - uma/carveout_options: a read-only file listing all the available
>>>    UMA allocation options and their index.
>>>
>>> - uma/carveout: a file that is both readable and writable. On read,
>>>    it shows the index of the current setting. Writing a valid index
>>>    into this file allows users to change the UMA carveout size to that
>>>    option on the next boot.
>>>
>>> Co-developed-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>>> Signed-off-by: Yo-Jung Leo Lin (AMD) <Leo.Lin@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143 ++++++++++++++++++ 
>>> + ++++++++++
>>>   1 file changed, 143 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/ 
>>> gpu/ drm/amd/amdgpu/amdgpu_device.c
>>> index 903c4706040d..e78e6982312c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -36,6 +36,7 @@
>>>   #include <linux/pci.h>
>>>   #include <linux/pci-p2pdma.h>
>>>   #include <linux/apple-gmux.h>
>>> +#include <linux/nospec.h>
>>>   #include <drm/drm_atomic_helper.h>
>>>   #include <drm/drm_client_event.h>
>>> @@ -417,6 +418,146 @@ static const struct attribute_group 
>>> amdgpu_board_attrs_group = {
>>>       .is_visible = amdgpu_board_attrs_is_visible
>>>   };
>>> +static ssize_t carveout_options_show(struct device *dev,
>>> +                     struct device_attribute *attr,
>>> +                     char *buf)
>>> +{
>>> +    struct drm_device *ddev = dev_get_drvdata(dev);
>>> +    struct amdgpu_device *adev = drm_to_adev(ddev);
>>> +    struct amdgpu_uma_carveout_info *uma_info = &adev->uma_info;
>>> +    uint32_t memory_carved;
>>> +    ssize_t size = 0;
>>> +
>>> +    if (!uma_info || !uma_info->num_entries)
>>> +        return -ENODEV;
>>> +
>>> +    for (int i = 0; i < uma_info->num_entries; i++) {
>>> +        memory_carved = uma_info->entries[i].memory_carved_mb;
>>> +        if (memory_carved >= SZ_1G/SZ_1M) {
>>> +            size += sysfs_emit_at(buf, size, "%d: %s (%u GB)\n",
>>> +                          i,
>>> +                          uma_info->entries[i].name,
>>> +                          memory_carved >> 10);
>>> +        } else {
>>> +            size += sysfs_emit_at(buf, size, "%d: %s (%u MB)\n",
>>> +                          i,
>>> +                          uma_info->entries[i].name,
>>> +                          memory_carved);
>>> +        }
>>> +    }
>>> +
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_RO(carveout_options);
>>> +
>>> +static ssize_t carveout_show(struct device *dev,
>>> +                 struct device_attribute *attr,
>>> +                 char *buf)
>>> +{
>>> +    struct drm_device *ddev = dev_get_drvdata(dev);
>>> +    struct amdgpu_device *adev = drm_to_adev(ddev);
>>> +
>>> +    return sysfs_emit(buf, "%u\n", adev->uma_info.uma_option_index);
>>
>> It would be better to show the size along with the index.
> 
> This idea complicates userspace parsing because rather than a atoi() 
> parser you now need to do sscanf().
> 
> IE - I like it the way it is in the series at the moment.
> 

Userspace already has to go through this for carveout_options parsing. 
It does not make any difference :)

I was thinking of an easier way without cross reference. It would be 
better even if you put the actual carve out size instead of an index 
(assuming carve out size is deterministic).

Thanks,
Lijo


>>
>> Regardless, series is -
>>
>>      Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>
>>
>> Thanks,
>> Lijo
>>
>>> +}
>>> +
>>> +static ssize_t carveout_store(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 = drm_to_adev(ddev);
>>> +    struct amdgpu_uma_carveout_info *uma_info = &adev->uma_info;
>>> +    struct amdgpu_uma_carveout_option *opt;
>>> +    unsigned long val;
>>> +    uint8_t flags;
>>> +    int r;
>>> +
>>> +    r = kstrtoul(buf, 10, &val);
>>> +    if (r)
>>> +        return r;
>>> +
>>> +    if (val >= uma_info->num_entries)
>>> +        return -EINVAL;
>>> +
>>> +    val = array_index_nospec(val, uma_info->num_entries);
>>> +    opt = &uma_info->entries[val];
>>> +
>>> +    if (!(opt->flags & AMDGPU_UMA_FLAG_AUTO) &&
>>> +        !(opt->flags & AMDGPU_UMA_FLAG_CUSTOM)) {
>>> +        drm_err_once(ddev, "Option %lu not supported due to lack of 
>>> Custom/Auto flag", val);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    flags = opt->flags;
>>> +    flags &= ~((flags & AMDGPU_UMA_FLAG_AUTO) >> 1);
>>> +
>>> +    guard(mutex)(&uma_info->update_lock);
>>> +
>>> +    r = amdgpu_acpi_set_uma_allocation_size(adev, val, flags);
>>> +    if (r)
>>> +        return r;
>>> +
>>> +    uma_info->uma_option_index = val;
>>> +
>>> +    return count;
>>> +}
>>> +static DEVICE_ATTR_RW(carveout);
>>> +
>>> +static struct attribute *amdgpu_uma_attrs[] = {
>>> +    &dev_attr_carveout.attr,
>>> +    &dev_attr_carveout_options.attr,
>>> +    NULL
>>> +};
>>> +
>>> +const struct attribute_group amdgpu_uma_attr_group = {
>>> +    .name = "uma",
>>> +    .attrs = amdgpu_uma_attrs
>>> +};
>>> +
>>> +static void amdgpu_uma_sysfs_init(struct amdgpu_device *adev)
>>> +{
>>> +    int rc;
>>> +
>>> +    if (!(adev->flags & AMD_IS_APU))
>>> +        return;
>>> +
>>> +    if (!amdgpu_acpi_is_set_uma_allocation_size_supported())
>>> +        return;
>>> +
>>> +    rc = amdgpu_atomfirmware_get_uma_carveout_info(adev, &adev- 
>>> >uma_info);
>>> +    if (rc) {
>>> +        drm_dbg(adev_to_drm(adev),
>>> +            "Failed to parse UMA carveout info from VBIOS: %d\n", rc);
>>> +        goto out_info;
>>> +    }
>>> +
>>> +    mutex_init(&adev->uma_info.update_lock);
>>> +
>>> +    rc = devm_device_add_group(adev->dev, &amdgpu_uma_attr_group);
>>> +    if (rc) {
>>> +        drm_dbg(adev_to_drm(adev), "Failed to add UMA carveout sysfs 
>>> interfaces %d\n", rc);
>>> +        goto out_attr;
>>> +    }
>>> +
>>> +    return;
>>> +
>>> +out_attr:
>>> +    mutex_destroy(&adev->uma_info.update_lock);
>>> +out_info:
>>> +    return;
>>> +}
>>> +
>>> +static void amdgpu_uma_sysfs_fini(struct amdgpu_device *adev)
>>> +{
>>> +    struct amdgpu_uma_carveout_info *uma_info = &adev->uma_info;
>>> +
>>> +    if (!amdgpu_acpi_is_set_uma_allocation_size_supported())
>>> +        return;
>>> +
>>> +    mutex_destroy(&uma_info->update_lock);
>>> +    uma_info->num_entries = 0;
>>> +}
>>> +
>>>   static void amdgpu_device_get_pcie_info(struct amdgpu_device *adev);
>>>   /**
>>> @@ -4492,6 +4633,7 @@ static int 
>>> amdgpu_device_sys_interface_init(struct amdgpu_device *adev)
>>>       amdgpu_fru_sysfs_init(adev);
>>>       amdgpu_reg_state_sysfs_init(adev);
>>>       amdgpu_xcp_sysfs_init(adev);
>>> +    amdgpu_uma_sysfs_init(adev);
>>>       return r;
>>>   }
>>> @@ -4507,6 +4649,7 @@ static void 
>>> amdgpu_device_sys_interface_fini(struct amdgpu_device *adev)
>>>       amdgpu_reg_state_sysfs_fini(adev);
>>>       amdgpu_xcp_sysfs_fini(adev);
>>> +    amdgpu_uma_sysfs_fini(adev);
>>>   }
>>>   /**
>>>
>>
>