On Tue, Nov 18, 2025 at 9:22 AM Lin, Leo <[email protected]> wrote: > > [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
Most of the other sysfs files work more like what I suggested. That said, if you'd prefer the other method, can you group then in a directory? E.g., uma/carveout_options uma/carveout Thanks, Alex > > 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 > > >
