| Message ID | 20200212202236.13095-1-robh@kernel.org |
|---|---|
| State | Accepted |
| Commit | eb9d8ddbc107d02e489681f9dcbf93949e1a99a4 |
| Headers | show |
| Series | [v2] drm/panfrost: Don't try to map on error faults | expand |
On 12/02/2020 20:22, Rob Herring wrote: > From: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > If the exception type isn't a translation fault, don't try to map and > instead go straight to a terminal fault. > > Otherwise, we can get flooded by kernel warnings and further faults. > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > I rewrote this some simplifying the code and somewhat following Steven's > suggested. Still not using defines though. No defines here was good > enough before IMO. Heresy! It's a good thing you're not writing kbase code - there you (seemingly) need to pick a #define which is as long as possible, but then still wrap the code to avoid the 80 character limit... For example[1]. Although shifting the code right might get you extra bonus points, deep nesting seems to be encouraged! ;) [1] https://gitlab.freedesktop.org/panfrost/mali_kbase/blob/master/driver/product/kernel/drivers/gpu/arm/midgard/backend/gpu/mali_kbase_js_backend.c#L156 > > Only compile tested. I've done some quick testing on a Firefly RK3288. But I don't have any (handy) tests to trigger non-TRANSLATION_FAULT MMU faults. Reviewed-by: Steven Price <steven.price@arm.com> Thanks, Steve > > drivers/gpu/drm/panfrost/panfrost_mmu.c | 44 +++++++++++-------------- > 1 file changed, 19 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c > index 763cfca886a7..4f2836bd9215 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c > @@ -596,33 +596,27 @@ static irqreturn_t panfrost_mmu_irq_handler_thread(int irq, void *data) > source_id = (fault_status >> 16); > > /* Page fault only */ > - if ((status & mask) == BIT(i)) { > - WARN_ON(exception_type < 0xC1 || exception_type > 0xC4); > - > + ret = -1; > + if ((status & mask) == BIT(i) && (exception_type & 0xF8) == 0xC0) > ret = panfrost_mmu_map_fault_addr(pfdev, i, addr); > - if (!ret) { > - mmu_write(pfdev, MMU_INT_CLEAR, BIT(i)); > - status &= ~mask; > - continue; > - } > - } > > - /* terminal fault, print info about the fault */ > - dev_err(pfdev->dev, > - "Unhandled Page fault in AS%d at VA 0x%016llX\n" > - "Reason: %s\n" > - "raw fault status: 0x%X\n" > - "decoded fault status: %s\n" > - "exception type 0x%X: %s\n" > - "access type 0x%X: %s\n" > - "source id 0x%X\n", > - i, addr, > - "TODO", > - fault_status, > - (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"), > - exception_type, panfrost_exception_name(pfdev, exception_type), > - access_type, access_type_name(pfdev, fault_status), > - source_id); > + if (ret) > + /* terminal fault, print info about the fault */ > + dev_err(pfdev->dev, > + "Unhandled Page fault in AS%d at VA 0x%016llX\n" > + "Reason: %s\n" > + "raw fault status: 0x%X\n" > + "decoded fault status: %s\n" > + "exception type 0x%X: %s\n" > + "access type 0x%X: %s\n" > + "source id 0x%X\n", > + i, addr, > + "TODO", > + fault_status, > + (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"), > + exception_type, panfrost_exception_name(pfdev, exception_type), > + access_type, access_type_name(pfdev, fault_status), > + source_id); > > mmu_write(pfdev, MMU_INT_CLEAR, mask); > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, 12 Feb 2020 at 21:22, Rob Herring <robh@kernel.org> wrote: > > From: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > If the exception type isn't a translation fault, don't try to map and > instead go straight to a terminal fault. > > Otherwise, we can get flooded by kernel warnings and further faults. > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > I rewrote this some simplifying the code and somewhat following Steven's > suggested. Still not using defines though. No defines here was good > enough before IMO. > > Only compile tested. Looks good, I also tested it on RK3399. Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> Thanks, Tomeu > drivers/gpu/drm/panfrost/panfrost_mmu.c | 44 +++++++++++-------------- > 1 file changed, 19 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c > index 763cfca886a7..4f2836bd9215 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c > @@ -596,33 +596,27 @@ static irqreturn_t panfrost_mmu_irq_handler_thread(int irq, void *data) > source_id = (fault_status >> 16); > > /* Page fault only */ > - if ((status & mask) == BIT(i)) { > - WARN_ON(exception_type < 0xC1 || exception_type > 0xC4); > - > + ret = -1; > + if ((status & mask) == BIT(i) && (exception_type & 0xF8) == 0xC0) > ret = panfrost_mmu_map_fault_addr(pfdev, i, addr); > - if (!ret) { > - mmu_write(pfdev, MMU_INT_CLEAR, BIT(i)); > - status &= ~mask; > - continue; > - } > - } > > - /* terminal fault, print info about the fault */ > - dev_err(pfdev->dev, > - "Unhandled Page fault in AS%d at VA 0x%016llX\n" > - "Reason: %s\n" > - "raw fault status: 0x%X\n" > - "decoded fault status: %s\n" > - "exception type 0x%X: %s\n" > - "access type 0x%X: %s\n" > - "source id 0x%X\n", > - i, addr, > - "TODO", > - fault_status, > - (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"), > - exception_type, panfrost_exception_name(pfdev, exception_type), > - access_type, access_type_name(pfdev, fault_status), > - source_id); > + if (ret) > + /* terminal fault, print info about the fault */ > + dev_err(pfdev->dev, > + "Unhandled Page fault in AS%d at VA 0x%016llX\n" > + "Reason: %s\n" > + "raw fault status: 0x%X\n" > + "decoded fault status: %s\n" > + "exception type 0x%X: %s\n" > + "access type 0x%X: %s\n" > + "source id 0x%X\n", > + i, addr, > + "TODO", > + fault_status, > + (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"), > + exception_type, panfrost_exception_name(pfdev, exception_type), > + access_type, access_type_name(pfdev, fault_status), > + source_id); > > mmu_write(pfdev, MMU_INT_CLEAR, mask); > > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> On Wed, Feb 12, 2020 at 02:22:36PM -0600, Rob Herring wrote: > From: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > If the exception type isn't a translation fault, don't try to map and > instead go straight to a terminal fault. > > Otherwise, we can get flooded by kernel warnings and further faults. > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > I rewrote this some simplifying the code and somewhat following Steven's > suggested. Still not using defines though. No defines here was good > enough before IMO. > > Only compile tested. > > drivers/gpu/drm/panfrost/panfrost_mmu.c | 44 +++++++++++-------------- > 1 file changed, 19 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c > index 763cfca886a7..4f2836bd9215 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c > @@ -596,33 +596,27 @@ static irqreturn_t panfrost_mmu_irq_handler_thread(int irq, void *data) > source_id = (fault_status >> 16); > > /* Page fault only */ > - if ((status & mask) == BIT(i)) { > - WARN_ON(exception_type < 0xC1 || exception_type > 0xC4); > - > + ret = -1; > + if ((status & mask) == BIT(i) && (exception_type & 0xF8) == 0xC0) > ret = panfrost_mmu_map_fault_addr(pfdev, i, addr); > - if (!ret) { > - mmu_write(pfdev, MMU_INT_CLEAR, BIT(i)); > - status &= ~mask; > - continue; > - } > - } > > - /* terminal fault, print info about the fault */ > - dev_err(pfdev->dev, > - "Unhandled Page fault in AS%d at VA 0x%016llX\n" > - "Reason: %s\n" > - "raw fault status: 0x%X\n" > - "decoded fault status: %s\n" > - "exception type 0x%X: %s\n" > - "access type 0x%X: %s\n" > - "source id 0x%X\n", > - i, addr, > - "TODO", > - fault_status, > - (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"), > - exception_type, panfrost_exception_name(pfdev, exception_type), > - access_type, access_type_name(pfdev, fault_status), > - source_id); > + if (ret) > + /* terminal fault, print info about the fault */ > + dev_err(pfdev->dev, > + "Unhandled Page fault in AS%d at VA 0x%016llX\n" > + "Reason: %s\n" > + "raw fault status: 0x%X\n" > + "decoded fault status: %s\n" > + "exception type 0x%X: %s\n" > + "access type 0x%X: %s\n" > + "source id 0x%X\n", > + i, addr, > + "TODO", > + fault_status, > + (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"), > + exception_type, panfrost_exception_name(pfdev, exception_type), > + access_type, access_type_name(pfdev, fault_status), > + source_id); > > mmu_write(pfdev, MMU_INT_CLEAR, mask); > > -- > 2.20.1 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 763cfca886a7..4f2836bd9215 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -596,33 +596,27 @@ static irqreturn_t panfrost_mmu_irq_handler_thread(int irq, void *data) source_id = (fault_status >> 16); /* Page fault only */ - if ((status & mask) == BIT(i)) { - WARN_ON(exception_type < 0xC1 || exception_type > 0xC4); - + ret = -1; + if ((status & mask) == BIT(i) && (exception_type & 0xF8) == 0xC0) ret = panfrost_mmu_map_fault_addr(pfdev, i, addr); - if (!ret) { - mmu_write(pfdev, MMU_INT_CLEAR, BIT(i)); - status &= ~mask; - continue; - } - } - /* terminal fault, print info about the fault */ - dev_err(pfdev->dev, - "Unhandled Page fault in AS%d at VA 0x%016llX\n" - "Reason: %s\n" - "raw fault status: 0x%X\n" - "decoded fault status: %s\n" - "exception type 0x%X: %s\n" - "access type 0x%X: %s\n" - "source id 0x%X\n", - i, addr, - "TODO", - fault_status, - (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"), - exception_type, panfrost_exception_name(pfdev, exception_type), - access_type, access_type_name(pfdev, fault_status), - source_id); + if (ret) + /* terminal fault, print info about the fault */ + dev_err(pfdev->dev, + "Unhandled Page fault in AS%d at VA 0x%016llX\n" + "Reason: %s\n" + "raw fault status: 0x%X\n" + "decoded fault status: %s\n" + "exception type 0x%X: %s\n" + "access type 0x%X: %s\n" + "source id 0x%X\n", + i, addr, + "TODO", + fault_status, + (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"), + exception_type, panfrost_exception_name(pfdev, exception_type), + access_type, access_type_name(pfdev, fault_status), + source_id); mmu_write(pfdev, MMU_INT_CLEAR, mask);