(resending for the 5.13 cycle)
This is a stack to add an Adreno GPU specific handler for pagefaults. The first
patch starts by wiring up report_iommu_fault for arm-smmu. The next patch adds
a adreno-smmu-priv function hook to capture a handful of important debugging
registers such as TTBR0, CONTEXTIDR, FSYNR0 and others. This is used by the
third patch to print more detailed information on page fault such as the TTBR0
for the pagetable that caused the fault and the source of the fault as
determined by a combination of the FSYNR1 register and an internal GPU
register.
This code provides a solid base that we can expand on later for even more
extensive GPU side page fault debugging capabilities.
v3: Always clear FSR even if the target driver is going to handle resume
v2: Fix comment wording and function pointer check per Rob Clark
Jordan Crouse (3):
iommu/arm-smmu: Add support for driver IOMMU fault handlers
drm/msm: Add an adreno-smmu-priv callback to get pagefault info
drm/msm: Improve the a6xx page fault handler
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +-
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 76 +++++++++++++++++++++-
drivers/gpu/drm/msm/msm_iommu.c | 11 +++-
drivers/gpu/drm/msm/msm_mmu.h | 4 +-
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 19 ++++++
drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 ++-
drivers/iommu/arm/arm-smmu/arm-smmu.h | 2 +
include/linux/adreno-smmu-priv.h | 31 ++++++++-
8 files changed, 145 insertions(+), 11 deletions(-)
--
2.25.1
Use the new adreno-smmu-priv fault info function to get more SMMU
debug registers and print the current TTBR0 to debug per-instance
pagetables and figure out which GPU block generated the request.
Signed-off-by: Jordan Crouse <[email protected]>
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +-
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 76 +++++++++++++++++++++++++--
drivers/gpu/drm/msm/msm_iommu.c | 11 +++-
drivers/gpu/drm/msm/msm_mmu.h | 4 +-
4 files changed, 87 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 7e553d3efeb2..56b548921c4e 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1075,7 +1075,7 @@ bool a5xx_idle(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
return true;
}
-static int a5xx_fault_handler(void *arg, unsigned long iova, int flags)
+static int a5xx_fault_handler(void *arg, unsigned long iova, int flags, void *data)
{
struct msm_gpu *gpu = arg;
pr_warn_ratelimited("*** gpu fault: iova=%08lx, flags=%d (%u,%u,%u,%u)\n",
@@ -1085,7 +1085,7 @@ static int a5xx_fault_handler(void *arg, unsigned long iova, int flags)
gpu_read(gpu, REG_A5XX_CP_SCRATCH_REG(6)),
gpu_read(gpu, REG_A5XX_CP_SCRATCH_REG(7)));
- return -EFAULT;
+ return 0;
}
static void a5xx_cp_err_irq(struct msm_gpu *gpu)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 064b7face504..97eabd87740c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -959,18 +959,88 @@ static void a6xx_recover(struct msm_gpu *gpu)
msm_gpu_hw_init(gpu);
}
-static int a6xx_fault_handler(void *arg, unsigned long iova, int flags)
+static const char *a6xx_uche_fault_block(struct msm_gpu *gpu, u32 mid)
+{
+ static const char *uche_clients[7] = {
+ "VFD", "SP", "VSC", "VPC", "HLSQ", "PC", "LRZ",
+ };
+ u32 val;
+
+ if (mid < 1 || mid > 3)
+ return "UNKNOWN";
+
+ /*
+ * The source of the data depends on the mid ID read from FSYNR1.
+ * and the client ID read from the UCHE block
+ */
+ val = gpu_read(gpu, REG_A6XX_UCHE_CLIENT_PF);
+
+ /* mid = 3 is most precise and refers to only one block per client */
+ if (mid == 3)
+ return uche_clients[val & 7];
+
+ /* For mid=2 the source is TP or VFD except when the client id is 0 */
+ if (mid == 2)
+ return ((val & 7) == 0) ? "TP" : "TP|VFD";
+
+ /* For mid=1 just return "UCHE" as a catchall for everything else */
+ return "UCHE";
+}
+
+static const char *a6xx_fault_block(struct msm_gpu *gpu, u32 id)
+{
+ if (id == 0)
+ return "CP";
+ else if (id == 4)
+ return "CCU";
+ else if (id == 6)
+ return "CDP Prefetch";
+
+ return a6xx_uche_fault_block(gpu, id);
+}
+
+#define ARM_SMMU_FSR_TF BIT(1)
+#define ARM_SMMU_FSR_PF BIT(3)
+#define ARM_SMMU_FSR_EF BIT(4)
+
+static int a6xx_fault_handler(void *arg, unsigned long iova, int flags, void *data)
{
struct msm_gpu *gpu = arg;
+ struct adreno_smmu_fault_info *info = data;
+ const char *type = "UNKNOWN";
- pr_warn_ratelimited("*** gpu fault: iova=%08lx, flags=%d (%u,%u,%u,%u)\n",
+ /*
+ * Print a default message if we couldn't get the data from the
+ * adreno-smmu-priv
+ */
+ if (!info) {
+ pr_warn_ratelimited("*** gpu fault: iova=%.16lx flags=%d (%u,%u,%u,%u)\n",
iova, flags,
gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(4)),
gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(5)),
gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(6)),
gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(7)));
- return -EFAULT;
+ return 0;
+ }
+
+ if (info->fsr & ARM_SMMU_FSR_TF)
+ type = "TRANSLATION";
+ else if (info->fsr & ARM_SMMU_FSR_PF)
+ type = "PERMISSION";
+ else if (info->fsr & ARM_SMMU_FSR_EF)
+ type = "EXTERNAL";
+
+ pr_warn_ratelimited("*** gpu fault: ttbr0=%.16llx iova=%.16lx dir=%s type=%s source=%s (%u,%u,%u,%u)\n",
+ info->ttbr0, iova,
+ flags & IOMMU_FAULT_WRITE ? "WRITE" : "READ", type,
+ a6xx_fault_block(gpu, info->fsynr1 & 0xff),
+ gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(4)),
+ gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(5)),
+ gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(6)),
+ gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(7)));
+
+ return 0;
}
static void a6xx_cp_hw_err_irq(struct msm_gpu *gpu)
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index 50d881794758..6975b95c3c29 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -211,8 +211,17 @@ static int msm_fault_handler(struct iommu_domain *domain, struct device *dev,
unsigned long iova, int flags, void *arg)
{
struct msm_iommu *iommu = arg;
+ struct adreno_smmu_priv *adreno_smmu = dev_get_drvdata(iommu->base.dev);
+ struct adreno_smmu_fault_info info, *ptr = NULL;
+
+ if (adreno_smmu->get_fault_info) {
+ adreno_smmu->get_fault_info(adreno_smmu->cookie, &info);
+ ptr = &info;
+ }
+
if (iommu->base.handler)
- return iommu->base.handler(iommu->base.arg, iova, flags);
+ return iommu->base.handler(iommu->base.arg, iova, flags, ptr);
+
pr_warn_ratelimited("*** fault: iova=%16lx, flags=%d\n", iova, flags);
return 0;
}
diff --git a/drivers/gpu/drm/msm/msm_mmu.h b/drivers/gpu/drm/msm/msm_mmu.h
index 61ade89d9e48..a88f44c3268d 100644
--- a/drivers/gpu/drm/msm/msm_mmu.h
+++ b/drivers/gpu/drm/msm/msm_mmu.h
@@ -26,7 +26,7 @@ enum msm_mmu_type {
struct msm_mmu {
const struct msm_mmu_funcs *funcs;
struct device *dev;
- int (*handler)(void *arg, unsigned long iova, int flags);
+ int (*handler)(void *arg, unsigned long iova, int flags, void *data);
void *arg;
enum msm_mmu_type type;
};
@@ -43,7 +43,7 @@ struct msm_mmu *msm_iommu_new(struct device *dev, struct iommu_domain *domain);
struct msm_mmu *msm_gpummu_new(struct device *dev, struct msm_gpu *gpu);
static inline void msm_mmu_set_fault_handler(struct msm_mmu *mmu, void *arg,
- int (*handler)(void *arg, unsigned long iova, int flags))
+ int (*handler)(void *arg, unsigned long iova, int flags, void *data))
{
mmu->arg = arg;
mmu->handler = handler;
--
2.25.1
Call report_iommu_fault() to allow upper-level drivers to register their
own fault handlers.
Signed-off-by: Jordan Crouse <[email protected]>
---
drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index d8c6bfde6a61..0f3a9b5f3284 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -408,6 +408,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct arm_smmu_device *smmu = smmu_domain->smmu;
int idx = smmu_domain->cfg.cbndx;
+ int ret;
fsr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSR);
if (!(fsr & ARM_SMMU_FSR_FAULT))
@@ -417,8 +418,12 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR);
cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
- dev_err_ratelimited(smmu->dev,
- "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
+ ret = report_iommu_fault(domain, dev, iova,
+ fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);
+
+ if (ret == -ENOSYS)
+ dev_err_ratelimited(smmu->dev,
+ "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
fsr, iova, fsynr, cbfrsynra, idx);
arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_FSR, fsr);
--
2.25.1
On 2021-02-25 17:51, Jordan Crouse wrote:
> Call report_iommu_fault() to allow upper-level drivers to register their
> own fault handlers.
>
> Signed-off-by: Jordan Crouse <[email protected]>
> ---
>
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index d8c6bfde6a61..0f3a9b5f3284 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -408,6 +408,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_device *smmu = smmu_domain->smmu;
> int idx = smmu_domain->cfg.cbndx;
> + int ret;
>
> fsr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSR);
> if (!(fsr & ARM_SMMU_FSR_FAULT))
> @@ -417,8 +418,12 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR);
> cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
>
> - dev_err_ratelimited(smmu->dev,
> - "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
> + ret = report_iommu_fault(domain, dev, iova,
Beware that "dev" here is not a struct device, so this isn't right. I'm
not entirely sure what we *should* be passing here, since we can't
easily attribute a context fault to a specific client device, and
passing the IOMMU device seems a bit dubious too, so maybe just NULL?
Robin.
> + fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);
> +
> + if (ret == -ENOSYS)
> + dev_err_ratelimited(smmu->dev,
> + "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
> fsr, iova, fsynr, cbfrsynra, idx);
>
> arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_FSR, fsr);
>
On Tue, Mar 02, 2021 at 12:17:24PM +0000, Robin Murphy wrote:
> On 2021-02-25 17:51, Jordan Crouse wrote:
> > Call report_iommu_fault() to allow upper-level drivers to register their
> > own fault handlers.
> >
> > Signed-off-by: Jordan Crouse <[email protected]>
> > ---
> >
> > drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index d8c6bfde6a61..0f3a9b5f3284 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -408,6 +408,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > struct arm_smmu_device *smmu = smmu_domain->smmu;
> > int idx = smmu_domain->cfg.cbndx;
> > + int ret;
> > fsr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSR);
> > if (!(fsr & ARM_SMMU_FSR_FAULT))
> > @@ -417,8 +418,12 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> > iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR);
> > cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
> > - dev_err_ratelimited(smmu->dev,
> > - "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
> > + ret = report_iommu_fault(domain, dev, iova,
>
> Beware that "dev" here is not a struct device, so this isn't right. I'm not
> entirely sure what we *should* be passing here, since we can't easily
> attribute a context fault to a specific client device, and passing the IOMMU
> device seems a bit dubious too, so maybe just NULL?
Agreed. The GPU doesn't use it and I doubt anything else would either since the
SMMU device is opaque to the leaf driver.
Jordan
> Robin.
>
> > + fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);
> > +
> > + if (ret == -ENOSYS)
> > + dev_err_ratelimited(smmu->dev,
> > + "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
> > fsr, iova, fsynr, cbfrsynra, idx);
> > arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_FSR, fsr);
> >
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Tue, Mar 2, 2021 at 7:54 AM Jordan Crouse <[email protected]> wrote:
>
> On Tue, Mar 02, 2021 at 12:17:24PM +0000, Robin Murphy wrote:
> > On 2021-02-25 17:51, Jordan Crouse wrote:
> > > Call report_iommu_fault() to allow upper-level drivers to register their
> > > own fault handlers.
> > >
> > > Signed-off-by: Jordan Crouse <[email protected]>
> > > ---
> > >
> > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > index d8c6bfde6a61..0f3a9b5f3284 100644
> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > @@ -408,6 +408,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> > > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > > struct arm_smmu_device *smmu = smmu_domain->smmu;
> > > int idx = smmu_domain->cfg.cbndx;
> > > + int ret;
> > > fsr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSR);
> > > if (!(fsr & ARM_SMMU_FSR_FAULT))
> > > @@ -417,8 +418,12 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> > > iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR);
> > > cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
> > > - dev_err_ratelimited(smmu->dev,
> > > - "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
> > > + ret = report_iommu_fault(domain, dev, iova,
> >
> > Beware that "dev" here is not a struct device, so this isn't right. I'm not
> > entirely sure what we *should* be passing here, since we can't easily
> > attribute a context fault to a specific client device, and passing the IOMMU
> > device seems a bit dubious too, so maybe just NULL?
>
> Agreed. The GPU doesn't use it and I doubt anything else would either since the
> SMMU device is opaque to the leaf driver.
Looks like other iommu drivers use a fun mix of attached device (for
ones that can make assumptions about one device per domain) and the
iommu dev ptr.. probably NULL is the right answer..
BR,
-R
> Jordan
>
> > Robin.
> >
> > > + fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);
> > > +
> > > + if (ret == -ENOSYS)
> > > + dev_err_ratelimited(smmu->dev,
> > > + "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
> > > fsr, iova, fsynr, cbfrsynra, idx);
> > > arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_FSR, fsr);
> > >
> > _______________________________________________
> > iommu mailing list
> > [email protected]
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu