[AMD Official Use Only - AMD Internal Distribution Only] > -----Original Message----- > From: Alex Deucher <[email protected]> > Sent: Tuesday, November 18, 2025 3:49 AM > To: Lin, Leo <[email protected]> > Cc: Deucher, Alexander <[email protected]>; Koenig, Christian > <[email protected]>; David Airlie <[email protected]>; Simona > Vetter <[email protected]>; Maarten Lankhorst > <[email protected]>; Maxime Ripard > <[email protected]>; Thomas Zimmermann <[email protected]>; > Jonathan Corbet <[email protected]>; [email protected]; dri- > [email protected]; [email protected]; linux- > [email protected]; Tsao, Anson <[email protected]>; Mario > Limonciello (AMD) (kernel.org) <[email protected]> > Subject: Re: [PATCH v2 4/5] drm/amdgpu: add UMA allocation interfaces to > sysfs > > On Fri, Nov 14, 2025 at 3:52 AM Yo-Jung Leo Lin (AMD) <[email protected]> > wrote: > > > > Add 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. > > Would it be cleaner to combine these? Reading the file would show the > indexed list of options with a * next to the currently selected one and then > writing the index to the file would cause the change to take effect on the > next > reboot. > > Alex
My thought is that it'll make it less friendly for processing output with awk/cut/other text processing tools. Also, I feel that the format will be trickier to maintain in the future, considering that it has to stay consistent for potentially new carveout option naming scheme. (Plus, the general guideline of making a sysfs file content array-ish in the kernel doc.[1]) That being said, it's my personal thought. If you feel cleaner to merge those two interfaces into one, it's definitely possible to do this in the next version and see what people's feedback would be. Let me know if that's what you'd like to see. [1] https://docs.kernel.org/filesystems/sysfs.html#attributes BR, Leo > > > > > Co-developed-by: Mario Limonciello (AMD) <[email protected]> > > Signed-off-by: Mario Limonciello (AMD) <[email protected]> > > Signed-off-by: Yo-Jung Leo Lin (AMD) <[email protected]> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 127 > > +++++++++++++++++++++++++++++++ > > 1 file changed, 127 insertions(+) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c > > index 444ab4102168..b9378f34eb79 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c > > @@ -30,6 +30,7 @@ > > #include <linux/power_supply.h> > > #include <linux/pm_runtime.h> > > #include <linux/suspend.h> > > +#include <linux/device.h> > > #include <acpi/video.h> > > #include <acpi/actbl.h> > > > > @@ -1247,6 +1248,125 @@ int amdgpu_acpi_get_mem_info(struct > amdgpu_device *adev, int xcc_id, > > return -ENOENT; > > } > > > > +static ssize_t uma_carveout_options_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) { > > + struct amdgpu_uma_carveout_info *uma_info = > &amdgpu_acpi_priv.atcs.uma_info; > > + ssize_t size = 0; > > + > > + if (!uma_info || !uma_info->num_entries) > > + return -ENODEV; > > + > > + for (int i = 0; i < uma_info->num_entries; i++) { > > + size += sysfs_emit_at(buf, size, "%d: %s (%u GB)\n", > > + i, > > + uma_info->entries[i].name, > > + uma_info->entries[i].memory_carved); > > + } > > + > > + return size; > > +} > > +static DEVICE_ATTR_RO(uma_carveout_options); > > + > > +static ssize_t uma_carveout_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) { > > + return sysfs_emit(buf, "%u\n", > > +amdgpu_acpi_priv.atcs.uma_info.uma_option_index); > > +} > > + > > +static ssize_t uma_carveout_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) { > > + struct amdgpu_uma_carveout_info *uma_info = > &amdgpu_acpi_priv.atcs.uma_info; > > + struct drm_device *ddev = dev_get_drvdata(dev); > > + struct amdgpu_device *adev = drm_to_adev(ddev); > > + 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; > > + > > + opt = &uma_info->entries[val]; > > + > > + if (!(opt->flags & AMDGPU_UMA_FLAG_AUTO) && > > + !(opt->flags & AMDGPU_UMA_FLAG_CUSTOM)) { > > + drm_err_once(ddev, "Option %ul not supported due to lack of > Custom/Auto flag", r); > > + 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(uma_carveout); > > + > > +static struct attribute *amdgpu_uma_attrs[] = { > > + &dev_attr_uma_carveout.attr, > > + &dev_attr_uma_carveout_options.attr, > > + NULL > > +}; > > + > > +const struct attribute_group amdgpu_uma_attr_group = { > > + .attrs = amdgpu_uma_attrs > > +}; > > + > > +static int amdgpu_acpi_uma_option_init(struct amdgpu_device *adev) { > > + struct amdgpu_atcs *atcs = &amdgpu_acpi_priv.atcs; > > + int rc; > > + > > + if (!atcs->functions.set_uma_allocation_size) > > + return -ENODEV; > > + > > + rc = amdgpu_atomfirmware_get_uma_carveout_info(adev, &atcs- > >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(&atcs->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 0; > > + > > +out_attr: > > + mutex_destroy(&atcs->uma_info.update_lock); > > +out_info: > > + return rc; > > +} > > + > > +static void amdgpu_acpi_uma_option_fini(void) { > > + struct amdgpu_uma_carveout_info *uma_info = > > +&amdgpu_acpi_priv.atcs.uma_info; > > + > > + mutex_destroy(&uma_info->update_lock); > > + uma_info->num_entries = 0; > > +} > > + > > /** > > * amdgpu_acpi_event - handle notify events > > * > > @@ -1291,6 +1411,12 @@ static int amdgpu_acpi_event(struct > > notifier_block *nb, int amdgpu_acpi_init(struct amdgpu_device *adev) > > { > > struct amdgpu_atif *atif = &amdgpu_acpi_priv.atif; > > + int rc; > > + > > + rc = amdgpu_acpi_uma_option_init(adev); > > + > > + if (rc) > > + drm_dbg(adev_to_drm(adev), "Not creating uma carveout > > + interfaces: %d", rc); > > > > if (atif->notifications.brightness_change) { > > if (adev->dc_enabled) { @@ -1343,6 +1469,7 @@ void > > amdgpu_acpi_get_backlight_caps(struct amdgpu_dm_backlight_caps *caps) > > void amdgpu_acpi_fini(struct amdgpu_device *adev) { > > unregister_acpi_notifier(&adev->acpi_nb); > > + amdgpu_acpi_uma_option_fini(); > > } > > > > /** > > > > -- > > 2.43.0 > >
