Re: [PATCH] pmdomain: imx8m-blk-ctrl: Serialize power on/off across sibling domains

[Frank Li]

On Wed, Sep 16, 2026 at 11:06:02AM +0800, Ming Qian(OSS) wrote:
> Hi Frank,
>
> On 9/15/2026 10:13 PM, Frank Li wrote:
> > On Tue, Sep 15, 2026 at 07:09:43PM +0900, Ming Qian wrote:
> > > On i.MX8MP, running the VC8000E encoder and the G1/G2 decoders
> > > concurrently rarely and non-deterministically leaves a VPU stuck in
> > > reset: its block registers read back all zeros. A decoder then times out
> > > (G1/G2 reset failed) or the encoder fails its format check reading a
> > > read-only capability register (VC8000E reset failed). Raising the
> > > runtime-PM autosuspend delay hides it, which points at the blk-ctrl
> > > power on/off path.
> > >
> > > The VPU blk-ctrl exposes G1, G2 and VC8000E as three separate genpds
> > > whose power_on/power_off are serialized only by the per-genpd lock, so on
> > > SMP the callbacks of different domains can run concurrently. Each domain
> > > only manipulates its own bit in the shared BLK_SFT_RSTN/BLK_CLK_EN
> > > registers, so this is not a matter of siblings corrupting each other's
> > > register bits.
> > >
> > > However the sibling power transitions still interact through the shared
> > > VPUMIX resources: the VPUMIX bus power domain, the VPU_NOC and the ADB400
> > > handshake. power_on asserts a domain's reset, enables its clock and
> > > releases the reset after a short udelay, relying on the reset propagating
> > > through that shared path. The GPC does not ack-verify the ADB400
> > > handshake on power-up (it only delays), and per ERR050531 the VPU_NOC
> > > handshake is timing sensitive during VC8000E/VPUMIX power up/down
> > > cycling. So when a sibling's power_on/power_off runs while another domain
> > > is inside its reset window, it disturbs the shared VPU_NOC/ADB/AXI clock
> > > timing and the victim's reset fails to take effect, leaving its block in
> > > reset with registers reading zero.
> > >
> > > This is a blk-ctrl defect: the VPU power domains' power up/down sequences
> > > must not interleave, yet the driver deliberately avoids a genpd
> > > parent/child hierarchy (to meet its sequencing requirements) and so gets
> > > no cross-sibling serialization from the genpd core.
> > >
> > > Add a per-blk-ctrl mutex around the blk-ctrl register and reset sequence
> > > and the synchronous power-up path, so a sibling domain cannot run its
> > > sequence while another domain is inside its reset window. The GPC
> > > power-down is queued by pm_runtime_put() and still completes outside the
> > > lock; serializing the reset sequences is what fixes the observed failure.
> > > The bus domain's GENPD_NOTIFY_ON notifier runs in the same call stack
> > > while the lock is held and must not take it.
> >
> > Thanks you for detail descript problem, basically it is power up/down
> > reset, clock have not serialized.
> >
> > Can you help summery to cut message shorter end emphase most important
> > part?
>
> Thanks for the review. Your summary is right: the power up/down reset
> and clock sequences of the sibling VPU domains are not serialized
> against each other.
>
> Below is the shortened commit message. Does it look acceptable to you?
>
> On i.MX8MP the VPU blk-ctrl exposes G1, G2 and VC8000E as three separate
> genpds, each serialized only by its own genpd lock, so their power_on
> and power_off callbacks can run concurrently on SMP.
>
> The sequences are not independent: they share the VPUMIX bus domain, the
> VPU_NOC and the ADB400 handshake. On power up the GPC cannot ack-verify
> the ADB400 handshake - the ack only completes once blk-ctrl sets the bus
> clk-en bit - so it just waits a fixed delay instead of polling hskack. A
> sibling transition landing inside another domain's reset window disturbs
> that shared clock and handshake timing, the victim's reset does not take
> effect, and its block registers read back all zeros: the decoder times
> out or the encoder fails its format check.
>
> Serialize the blk-ctrl reset sequence with a per-blk-ctrl mutex; the
> driver deliberately avoids a genpd hierarchy, so the genpd core gives no
> cross-sibling serialization.

good

Frank

>
>
> Thanks,
> Ming
>
> >
> > Frank
> >
> > >
> > > Fixes: a1a5f15f7f6c ("soc: imx: imx8m-blk-ctrl: add i.MX8MP VPU blk ctrl")
> > > Signed-off-by: Ming Qian <ming.qian@xxxxxxxxxxx>
> > > ---
> > > Reproduced on i.MX8MP with an Android 6.18 kernel: running an H.264
> > > decode and an H.264 encode concurrently hits the failure within one to
> > > two hours. A VPU comes up stuck in reset, its block registers read back
> > > all zeros, and the decoder times out or the encoder fails its format
> > > check.
> > >
> > > With this patch applied the same test ran overnight, over 14 hours,
> > > without a single occurrence.
> > > ---
> > >   drivers/pmdomain/imx/imx8m-blk-ctrl.c | 15 +++++++++++++++
> > >   1 file changed, 15 insertions(+)
> > >
> > > diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> > > index 479789009c7f..f8105e87ea3c 100644
> > > --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> > > +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> > > @@ -15,6 +15,7 @@
> > >   #include <linux/pm_runtime.h>
> > >   #include <linux/regmap.h>
> > >   #include <linux/clk.h>
> > > +#include <linux/mutex.h>
> > >
> > >   #include <dt-bindings/power/imx8mm-power.h>
> > >   #include <dt-bindings/power/imx8mn-power.h>
> > > @@ -34,6 +35,12 @@ struct imx8m_blk_ctrl {
> > >   	struct regmap *regmap;
> > >   	struct imx8m_blk_ctrl_domain *domains;
> > >   	struct genpd_onecell_data onecell_data;
> > > +	/*
> > > +	 * Serializes the blk-ctrl reset/clock sequence across sibling domains;
> > > +	 * their transitions interact through the shared VPUMIX bus domain,
> > > +	 * VPU_NOC and the not-ack-verified ADB400 handshake (ERR050531).
> > > +	 */
> > > +	struct mutex power_lock;
> > >   };
> > >
> > >   struct imx8m_blk_ctrl_domain_data {
> > > @@ -98,6 +105,8 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > >   	struct imx8m_blk_ctrl *bc = domain->bc;
> > >   	int ret;
> > >
> > > +	guard(mutex)(&bc->power_lock);
> > > +
> > >   	/* make sure bus domain is awake */
> > >   	ret = pm_runtime_get_sync(bc->bus_power_dev);
> > >   	if (ret < 0) {
> > > @@ -164,6 +173,8 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > >   	const struct imx8m_blk_ctrl_domain_data *data = domain->data;
> > >   	struct imx8m_blk_ctrl *bc = domain->bc;
> > >
> > > +	guard(mutex)(&bc->power_lock);
> > > +
> > >   	/* put devices into reset and disable clocks */
> > >   	if (data->mipi_phy_rst_mask)
> > >   		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > @@ -202,6 +213,10 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev)
> > >
> > >   	bc->dev = dev;
> > >
> > > +	ret = devm_mutex_init(dev, &bc->power_lock);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >   	bc_data = of_device_get_match_data(dev);
> > >
> > >   	base = devm_platform_ioremap_resource(pdev, 0);
> > >
> > > ---
> > > base-commit: 6e30287eaf3e41b86dfb86df3b811526693d74b4
> > > change-id: 20260911-imx8mp-blk-ctrl-c46f26783073
> > >
> > >