From b5fab345654c603c07525100d744498f28786929 Mon Sep 17 00:00:00 2001 From: Alyssa Rosenzweig Date: Tue, 24 Aug 2021 13:30:25 -0400 Subject: drm/panfrost: Simplify lock_region calculation In lock_region, simplify the calculation of the region_width parameter. This field is the size, but encoded as ceil(log2(size)) - 1. ceil(log2(size)) may be computed directly as fls(size - 1). However, we want to use the 64-bit versions as the amount to lock can exceed 32-bits. This avoids undefined (and completely wrong) behaviour when locking all memory (size ~0). In this case, the old code would "round up" ~0 to the nearest page, overflowing to 0. Since fls(0) == 0, this would calculate a region width of 10 + 0 = 10. But then the code would shift by (region_width - 11) = -1. As shifting by a negative number is undefined, UBSAN flags the bug. Of course, even if it were defined the behaviour is wrong, instead of locking all memory almost none would get locked. The new form of the calculation corrects this special case and avoids the undefined behaviour. Signed-off-by: Alyssa Rosenzweig Reported-and-tested-by: Chris Morgan Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver") Cc: Reviewed-by: Steven Price Reviewed-by: Rob Herring Signed-off-by: Steven Price Link: https://patchwork.freedesktop.org/patch/msgid/20210824173028.7528-2-alyssa.rosenzweig@collabora.com --- drivers/gpu/drm/panfrost/panfrost_mmu.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) (limited to 'drivers/gpu/drm/panfrost/panfrost_mmu.c') diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 0da5b3100ab1..f6e02d0392f4 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -62,21 +62,12 @@ static void lock_region(struct panfrost_device *pfdev, u32 as_nr, { u8 region_width; u64 region = iova & PAGE_MASK; - /* - * fls returns: - * 1 .. 32 - * - * 10 + fls(num_pages) - * results in the range (11 .. 42) - */ - - size = round_up(size, PAGE_SIZE); - region_width = 10 + fls(size >> PAGE_SHIFT); - if ((size >> PAGE_SHIFT) != (1ul << (region_width - 11))) { - /* not pow2, so must go up to the next pow2 */ - region_width += 1; - } + /* 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); + region_width = fls64(size - 1) - 1; region |= region_width; /* Lock the region that needs to be updated */ -- cgit From a77b58825d7221d4a45c47881c35a47ba003aa73 Mon Sep 17 00:00:00 2001 From: Alyssa Rosenzweig Date: Tue, 24 Aug 2021 13:30:26 -0400 Subject: drm/panfrost: Use u64 for size in lock_region Mali virtual addresses are 48-bit. Use a u64 instead of size_t to ensure we can express the "lock everything" condition as ~0ULL without overflow. This code was silently broken on any platform where a size_t is less than 48-bits; in particular, it was broken on 32-bit armv7 platforms which remain in use with panfrost. (Mainly RK3288) Signed-off-by: Alyssa Rosenzweig Suggested-by: Rob Herring Tested-by: Chris Morgan Reviewed-by: Steven Price Reviewed-by: Rob Herring Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver") Cc: Signed-off-by: Steven Price Link: https://patchwork.freedesktop.org/patch/msgid/20210824173028.7528-3-alyssa.rosenzweig@collabora.com --- drivers/gpu/drm/panfrost/panfrost_mmu.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'drivers/gpu/drm/panfrost/panfrost_mmu.c') diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index f6e02d0392f4..3a795273e505 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -58,7 +58,7 @@ static int write_cmd(struct panfrost_device *pfdev, u32 as_nr, u32 cmd) } static void lock_region(struct panfrost_device *pfdev, u32 as_nr, - u64 iova, size_t size) + u64 iova, u64 size) { u8 region_width; u64 region = iova & PAGE_MASK; @@ -78,7 +78,7 @@ static void lock_region(struct panfrost_device *pfdev, u32 as_nr, static int mmu_hw_do_operation_locked(struct panfrost_device *pfdev, int as_nr, - u64 iova, size_t size, u32 op) + u64 iova, u64 size, u32 op) { if (as_nr < 0) return 0; @@ -95,7 +95,7 @@ static int mmu_hw_do_operation_locked(struct panfrost_device *pfdev, int as_nr, static int mmu_hw_do_operation(struct panfrost_device *pfdev, struct panfrost_mmu *mmu, - u64 iova, size_t size, u32 op) + u64 iova, u64 size, u32 op) { int ret; @@ -112,7 +112,7 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m u64 transtab = cfg->arm_mali_lpae_cfg.transtab; u64 memattr = cfg->arm_mali_lpae_cfg.memattr; - mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM); + mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM); mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), transtab & 0xffffffffUL); mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), transtab >> 32); @@ -128,7 +128,7 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr) { - mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM); + mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM); mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0); mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0); @@ -242,7 +242,7 @@ static size_t get_pgsize(u64 addr, size_t size) static void panfrost_mmu_flush_range(struct panfrost_device *pfdev, struct panfrost_mmu *mmu, - u64 iova, size_t size) + u64 iova, u64 size) { if (mmu->as < 0) return; -- cgit From bd7ffbc3ca12629aeb66fb9e28cf42b7f37e3e3b Mon Sep 17 00:00:00 2001 From: Alyssa Rosenzweig Date: Tue, 24 Aug 2021 13:30:27 -0400 Subject: drm/panfrost: Clamp lock region to Bifrost minimum 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, 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 Tested-by: Chris Morgan Reviewed-by: Steven Price Reviewed-by: Rob Herring Cc: Signed-off-by: Steven Price Link: https://patchwork.freedesktop.org/patch/msgid/20210824173028.7528-4-alyssa.rosenzweig@collabora.com --- drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/gpu/drm/panfrost/panfrost_mmu.c') 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; -- cgit