IOMMU Archive on lore.kernel.org
 help / color / Atom feed
From: Robin Murphy <robin.murphy@arm.com>
To: Rob Clark <robdclark@gmail.com>, iommu@lists.linux-foundation.org
Cc: Rob Clark <robdclark@chromium.org>, Will Deacon <will@kernel.org>,
	linux-arm-msm@vger.kernel.org,
	open list <linux-kernel@vger.kernel.org>,
	freedreno@lists.freedesktop.org,
	"moderated list:ARM SMMU DRIVERS"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] iommu/arm-smmu: fix "hang" when games exit
Date: Tue, 10 Sep 2019 16:01:37 +0100
Message-ID: <418d8426-f299-1269-2b2e-f86677cf22c2@arm.com> (raw)
In-Reply-To: <20190907175013.24246-1-robdclark@gmail.com>

On 07/09/2019 18:50, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> When games, browser, or anything using a lot of GPU buffers exits, there
> can be many hundreds or thousands of buffers to unmap and free.  If the
> GPU is otherwise suspended, this can cause arm-smmu to resume/suspend
> for each buffer, resulting 5-10 seconds worth of reprogramming the
> context bank (arm_smmu_write_context_bank()/arm_smmu_write_s2cr()/etc).
> To the user it would appear that the system is locked up.
> 
> A simple solution is to use pm_runtime_put_autosuspend() instead, so we
> don't immediately suspend the SMMU device.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
> Note: I've tied the autosuspend enable/delay to the consumer device,
> based on the reasoning that if the consumer device benefits from using
> an autosuspend delay, then it's corresponding SMMU probably does too.
> Maybe that is overkill and we should just unconditionally enable
> autosuspend.

I'm not sure there's really any reason to expect that a supplier's usage 
model when doing things for itself bears any relation to that of its 
consumer(s), so I'd certainly lean towards the "unconditional" argument 
myself.

Of course ideally we'd skip resuming altogether in the map/unmap paths 
(since resume implies a full TLB reset anyway), but IIRC that approach 
started to get messy in the context of the initial RPM patchset. I'm 
planning to fiddle around a bit more to clean up the implementation of 
the new iommu_flush_ops stuff, so I've made a note to myself to revisit 
RPM to see if there's a sufficiently clean way to do better. In the 
meantime, though, I don't have any real objection to using some 
reasonable autosuspend delay on the principle that if we've been woken 
up to map/unmap one page, there's a high likelihood that more will 
follow in short order (and in the configuration slow-paths it won't have 
much impact either way).

Robin.

>   drivers/iommu/arm-smmu.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index c2733b447d9c..73a0dd53c8a3 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -289,7 +289,7 @@ static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
>   static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
>   {
>   	if (pm_runtime_enabled(smmu->dev))
> -		pm_runtime_put(smmu->dev);
> +		pm_runtime_put_autosuspend(smmu->dev);
>   }
>   
>   static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> @@ -1445,6 +1445,15 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   	/* Looks ok, so add the device to the domain */
>   	ret = arm_smmu_domain_add_master(smmu_domain, fwspec);
>   
> +#ifdef CONFIG_PM
> +	/* TODO maybe device_link_add() should do this for us? */
> +	if (dev->power.use_autosuspend) {
> +		pm_runtime_set_autosuspend_delay(smmu->dev,
> +			dev->power.autosuspend_delay);
> +		pm_runtime_use_autosuspend(smmu->dev);
> +	}
> +#endif
> +
>   rpm_put:
>   	arm_smmu_rpm_put(smmu);
>   	return ret;
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-07 17:50 Rob Clark
2019-09-10 15:01 ` Robin Murphy [this message]
2019-09-10 15:45   ` Rob Clark
2019-10-07 20:49   ` [PATCH v2] " Rob Clark
2019-10-09 10:10     ` Robin Murphy
2019-10-28 22:20     ` Will Deacon
2019-10-28 22:38       ` Rob Clark
2019-10-28 22:51         ` Robin Murphy
2019-10-29 11:15           ` Will Deacon

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=418d8426-f299-1269-2b2e-f86677cf22c2@arm.com \
    --to=robin.murphy@arm.com \
    --cc=freedreno@lists.freedesktop.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robdclark@chromium.org \
    --cc=robdclark@gmail.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git