linux-kernel - Re: [PATCH] drm/panthor: attach the driver's multiple power domains

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20251009172320.6a2cbf55@fedora>
Date: Thu, 9 Oct 2025 17:23:20 +0200
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Rain Yang <jiyu.yang@....nxp.com>
Cc: airlied@...il.com, dri-devel@...ts.freedesktop.org, imx@...ts.linux.dev,
 linux-kernel@...r.kernel.org, liviu.dudau@....com,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
 prabhu.sundararaj@....com, simona@...ll.ch, steven.price@....com,
 tzimmermann@...e.de, xianzhong.li@....com
Subject: Re: [PATCH] drm/panthor: attach the driver's multiple power domains

On Thu, 9 Oct 2025 23:06:17 +0800
Rain Yang <jiyu.yang@....nxp.com> wrote:

> On Thu, Oct 09, 2025 at 04:09:29PM +0200, Boris Brezillon wrote:
> >On Thu, 9 Oct 2025 16:08:20 +0200
> >Boris Brezillon <boris.brezillon@...labora.com> wrote:
> >  
> >> On Thu,  9 Oct 2025 22:00:39 +0800
> >> Rain Yang <jiyu.yang@....nxp.com> wrote:
> >>   
> >> > From: Rain Yang <jiyu.yang@....com>
> >> > 
> >> > Some platforms, such as i.MX95, utilize multiple power domains that need
> >> > to be attached explicitly. This patch ensures that the driver properly
> >> > attaches all available power domains using devm_pm_domain_attach_list().
> >> > 
> >> > Signed-off-by: Prabhu Sundararaj <prabhu.sundararaj@....com>
> >> > Signed-off-by: Rain Yang <jiyu.yang@....com>
> >> > ---
> >> >  drivers/gpu/drm/panthor/panthor_device.c | 6 ++++++
> >> >  drivers/gpu/drm/panthor/panthor_device.h | 2 ++
> >> >  2 files changed, 8 insertions(+)
> >> > 
> >> > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> >> > index f0b2da5b2b96..6f40d053b16c 100644
> >> > --- a/drivers/gpu/drm/panthor/panthor_device.c
> >> > +++ b/drivers/gpu/drm/panthor/panthor_device.c
> >> > @@ -218,6 +218,12 @@ int panthor_device_init(struct panthor_device *ptdev)
> >> >  	if (ret)
> >> >  		return ret;
> >> >  
> >> > +	ret = devm_pm_domain_attach_list(ptdev->base.dev, NULL, &ptdev->pd_list);
> >> > +	if (ret < 0) {
> >> > +		drm_err(&ptdev->base, "attach power domains failed, ret=%d", ret);
> >> > +		return ret;
> >> > +	}
> >> > +
> >> >  	ret = panthor_devfreq_init(ptdev);
> >> >  	if (ret)
> >> >  		return ret;
> >> > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> >> > index 4fc7cf2aeed5..5ecb541ec67b 100644
> >> > --- a/drivers/gpu/drm/panthor/panthor_device.h
> >> > +++ b/drivers/gpu/drm/panthor/panthor_device.h
> >> > @@ -196,6 +196,8 @@ struct panthor_device {
> >> >  		/** @recovery_needed: True when a resume attempt failed. */
> >> >  		atomic_t recovery_needed;
> >> >  	} pm;    
> >> 
> >> Add a blank line here.
> >>   
> >> > +	/** @pm: Power management related data. */  
> >
> >Also, the comment is wrong, and it would probably make sense to move
> >that to the pm sub-struct since this is PM related.  
> thanks, will fix it next version.
> >  
> >> > +	struct dev_pm_domain_list  *pd_list;
> >> >      
> >> 
> >> Do we even need to keep the pd_list in panthor_device if we don't do
> >> anything with it?  
> The second power domain is typically used for frequency scaling. The driver
> works fine when there's only one single power domain. That said, I will update
> the implementation to attach multiple power domains only when the domain count
> is greater than one.

That's not what I meant. What I mean is that the
panthor_device::pd_list is never used, because the extra power domains
get attached to the struct device directly, and the PM core
automatically enables all PDs on resume and disable them on suspend.
Because it's a devm_ function, you don't need it to detach the pd_list
at ::remove() time either. TLDR; that means you can pass
devm_pm_domain_attach_list() a local pd_list instead of ptdev->pd_list
and you can thus drop panthor_device::pd_list altogether, I think.

If you intend to use the pd_list for manual PD control in panthor in a
follow-up patchset, this should be mentioned in the commit message.

> >>   
> >> >  	/** @profile_mask: User-set profiling flags for job accounting. */
> >> >  	u32 profile_mask;    
> >>   
> >