linux-kernel - Re: [PATCH v6 05/18] drm/imagination: Add power domain control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPDyKFpKkSb=kGn4y18uFy9uhxFkjt7Gi2bUAzzqGnyUtTAoMg@mail.gmail.com>
Date: Wed, 4 Jun 2025 16:08:21 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Matt Coster <matt.coster@...tec.com>
Cc: Frank Binns <frank.binns@...tec.com>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Nishanth Menon <nm@...com>, 
	Vignesh Raghavendra <vigneshr@...com>, Tero Kristo <kristo@...nel.org>, dri-devel@...ts.freedesktop.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, Randolph Sapp <rs@...com>, 
	Darren Etheridge <detheridge@...com>, Michal Wilczynski <m.wilczynski@...sung.com>, 
	Alessio Belle <alessio.belle@...tec.com>, Alexandru Dadu <alexandru.dadu@...tec.com>
Subject: Re: [PATCH v6 05/18] drm/imagination: Add power domain control

On Thu, 10 Apr 2025 at 12:12, Matt Coster <matt.coster@...tec.com> wrote:
>
> The first supported GPU only used a single power domain so this was
> automatically handled by the device runtime.
>
> In order to support multiple power domains, they must be enumerated from
> devicetree and linked to both the GPU device and each other to ensure
> correct power sequencing at start time.
>
> For all Imagination Rogue GPUs, power domains are named "a", "b", etc. and
> the sequence A->B->... is always valid for startup with the reverse true
> for shutdown. Note this is not always the *only* valid sequence, but it's
> simple and does not require special-casing for different GPU power
> topologies.
>
> Signed-off-by: Matt Coster <matt.coster@...tec.com>
> Reviewed-by: Frank Binns <frank.binns@...tec.com>

Realize that I am a bit late to this, but I was recently pointed to
this commit [1] by Michal.

[...]

> +
> +int pvr_power_domains_init(struct pvr_device *pvr_dev)
> +{
> +       struct device *dev = from_pvr_device(pvr_dev)->dev;
> +
> +       struct device_link **domain_links __free(kfree) = NULL;
> +       struct device **domain_devs __free(kfree) = NULL;
> +       int domain_count;
> +       int link_count;
> +
> +       char dev_name[2] = "a";

It's certainly good to use a name to be future proof, but we should at
least try to pick a somewhat descriptive name for what this PM domain
is used for.

Moreover, this requires a corresponding update to the DT docs.

> +       int err;
> +       int i;
> +
> +       domain_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> +                                                 "#power-domain-cells");
> +       if (domain_count < 0)
> +               return domain_count;
> +
> +       if (domain_count <= 1)
> +               return 0;
> +
> +       link_count = domain_count + (domain_count - 1);
> +
> +       domain_devs = kcalloc(domain_count, sizeof(*domain_devs), GFP_KERNEL);
> +       if (!domain_devs)
> +               return -ENOMEM;
> +
> +       domain_links = kcalloc(link_count, sizeof(*domain_links), GFP_KERNEL);
> +       if (!domain_links)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < domain_count; i++) {
> +               struct device *domain_dev;
> +
> +               dev_name[0] = 'a' + i;
> +               domain_dev = dev_pm_domain_attach_by_name(dev, dev_name);
> +               if (IS_ERR_OR_NULL(domain_dev)) {
> +                       err = domain_dev ? PTR_ERR(domain_dev) : -ENODEV;
> +                       goto err_detach;
> +               }
> +
> +               domain_devs[i] = domain_dev;
> +       }
> +
> +       for (i = 0; i < domain_count; i++) {
> +               struct device_link *link;
> +
> +               link = device_link_add(dev, domain_devs[i], DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
> +               if (!link) {
> +                       err = -ENODEV;
> +                       goto err_unlink;
> +               }
> +
> +               domain_links[i] = link;
> +       }
> +
> +       for (i = domain_count; i < link_count; i++) {
> +               struct device_link *link;
> +
> +               link = device_link_add(domain_devs[i - domain_count + 1],
> +                                      domain_devs[i - domain_count],
> +                                      DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
> +               if (!link) {
> +                       err = -ENODEV;
> +                       goto err_unlink;
> +               }
> +
> +               domain_links[i] = link;
> +       }

Most of the code above (and the error/remove-path) can be considered
as boiler-plate code and can be replaced by using
devm_pm_domain_attach_list() instead.

[...]

Kind regards
Uffe

[1]
https://lore.kernel.org/all/a68e3bee-f4ad-4d73-a5a8-e39772c41711@samsung.com/