| Message ID | 20210820213117.13050-4-alyssa.rosenzweig@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Bug fixes for lock_region | expand |
On 20/08/2021 22:31, Alyssa Rosenzweig wrote: > When locking a region, we currently clamp to a PAGE_SIZE as the minimum > lock region. While this is valid for Midgard, it is invalid for Bifrost, While the spec does seem to state it's invalid for Bifrost - kbase didn't bother with a lower clamp for a long time. I actually think this is in many ways more of a spec bug: i.e. implementation details of the round-up that the hardware does. But it's much safer following the spec ;) And it seems like kbase eventually caught up too. > where the minimum locking size is 8x larger than the 4k page size. Add a > hardware definition for the minimum lock region size (corresponding to > KBASE_LOCK_REGION_MIN_SIZE_LOG2 in kbase) and respect it. > > Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> > Tested-by: Chris Morgan <macromorgan@hotmail.com> > Cc: <stable@vger.kernel.org> Reviewed-by: Steven Price <steven.price@arm.com> > --- > drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 +- > drivers/gpu/drm/panfrost/panfrost_regs.h | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c > index 3a795273e505..dfe5f1d29763 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c > @@ -66,7 +66,7 @@ static void lock_region(struct panfrost_device *pfdev, u32 as_nr, > /* The size is encoded as ceil(log2) minus(1), which may be calculated > * with fls. The size must be clamped to hardware bounds. > */ > - size = max_t(u64, size, PAGE_SIZE); > + size = max_t(u64, size, AS_LOCK_REGION_MIN_SIZE); > region_width = fls64(size - 1) - 1; > region |= region_width; > > diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h > index 1940ff86e49a..6c5a11ef1ee8 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_regs.h > +++ b/drivers/gpu/drm/panfrost/panfrost_regs.h > @@ -316,6 +316,8 @@ > #define AS_FAULTSTATUS_ACCESS_TYPE_READ (0x2 << 8) > #define AS_FAULTSTATUS_ACCESS_TYPE_WRITE (0x3 << 8) > > +#define AS_LOCK_REGION_MIN_SIZE (1ULL << 15) > + > #define gpu_write(dev, reg, data) writel(data, dev->iomem + reg) > #define gpu_read(dev, reg) readl(dev->iomem + reg) > >
> > When locking a region, we currently clamp to a PAGE_SIZE as the minimum > > lock region. While this is valid for Midgard, it is invalid for Bifrost, > > While the spec does seem to state it's invalid for Bifrost - kbase > didn't bother with a lower clamp for a long time. I actually think this > is in many ways more of a spec bug: i.e. implementation details of the > round-up that the hardware does. But it's much safer following the spec > ;) And it seems like kbase eventually caught up too. Yeah, makes sense. Should I drop the Cc: stable in that case? If the issue is purely theoretical.
On 23 August 2021 22:13:45 BST, Alyssa Rosenzweig <alyssa@collabora.com> wrote: >> > When locking a region, we currently clamp to a PAGE_SIZE as the minimum >> > lock region. While this is valid for Midgard, it is invalid for Bifrost, >> >> While the spec does seem to state it's invalid for Bifrost - kbase >> didn't bother with a lower clamp for a long time. I actually think this >> is in many ways more of a spec bug: i.e. implementation details of the >> round-up that the hardware does. But it's much safer following the spec >> ;) And it seems like kbase eventually caught up too. > >Yeah, makes sense. Should I drop the Cc: stable in that case? If the >issue is purely theoretical. I think it might still be worth fixing. Early Bifrost should be fine, but something triggered a bug report that caused kbase to be fixed, so I'm less confident that there's nothing out there that cares. Following both kbase and the spec seems the safest approach. Thanks, Steve
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 3a795273e505..dfe5f1d29763 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -66,7 +66,7 @@ static void lock_region(struct panfrost_device *pfdev, u32 as_nr, /* The size is encoded as ceil(log2) minus(1), which may be calculated * with fls. The size must be clamped to hardware bounds. */ - size = max_t(u64, size, PAGE_SIZE); + size = max_t(u64, size, AS_LOCK_REGION_MIN_SIZE); region_width = fls64(size - 1) - 1; region |= region_width; diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h index 1940ff86e49a..6c5a11ef1ee8 100644 --- a/drivers/gpu/drm/panfrost/panfrost_regs.h +++ b/drivers/gpu/drm/panfrost/panfrost_regs.h @@ -316,6 +316,8 @@ #define AS_FAULTSTATUS_ACCESS_TYPE_READ (0x2 << 8) #define AS_FAULTSTATUS_ACCESS_TYPE_WRITE (0x3 << 8) +#define AS_LOCK_REGION_MIN_SIZE (1ULL << 15) + #define gpu_write(dev, reg, data) writel(data, dev->iomem + reg) #define gpu_read(dev, reg) readl(dev->iomem + reg)