Framebuffer mmap on PowerPC

5 messages, 3 authors, 2023-09-04 · open the first message on its own page

Framebuffer mmap on PowerPC

From: Thomas Zimmermann <hidden>
Date: 2023-09-04 07:24:52

Hi,

there's a per-architecture function called fb_pgprotect() that sets 
VMA's vm_page_prot for mmaped framebuffers. Most architectures use a 
simple implementation based on pgprot_writecomine() [1] or 
pgprot_noncached(). [2]

On PPC this function uses phys_mem_access_prot() and therefore requires 
the mmap call's file struct. [3] Removing the file argument would help 
with simplifying the caller of fb_pgprotect(). [4]

Why is the file even required on PPC?

Is it possible to replace phys_mem_access_prot() with something simpler 
that does not use the file struct?

Best regards
Thomas


[1] 
https://elixir.bootlin.com/linux/v6.5/source/include/asm-generic/fb.h#L19
[2] 
https://elixir.bootlin.com/linux/v6.5/source/arch/mips/include/asm/fb.h#L11
[3] 
https://elixir.bootlin.com/linux/v6.5/source/arch/powerpc/include/asm/fb.h#L12
[4] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/video/fbdev/core/fbmem.c#L1299


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: Framebuffer mmap on PowerPC

From: Christophe Leroy <hidden>
Date: 2023-08-31 17:38:43


Le 31/08/2023 à 16:41, Thomas Zimmermann a écrit :
Hi,

there's a per-architecture function called fb_pgprotect() that sets 
VMA's vm_page_prot for mmaped framebuffers. Most architectures use a 
simple implementation based on pgprot_writecomine() [1] or 
pgprot_noncached(). [2]

On PPC this function uses phys_mem_access_prot() and therefore requires 
the mmap call's file struct. [3] Removing the file argument would help 
with simplifying the caller of fb_pgprotect(). [4]

Why is the file even required on PPC?

Is it possible to replace phys_mem_access_prot() with something simpler 
that does not use the file struct?
Looks like phys_mem_access_prot() defaults to calling pgprot_noncached() 
see 
https://elixir.bootlin.com/linux/v6.5/source/arch/powerpc/mm/mem.c#L37 
but for a few platforms that's superseeded by 
pci_phys_mem_access_prot(), see 
https://elixir.bootlin.com/linux/v6.5/source/arch/powerpc/kernel/pci-common.c#L524

However, as far as I can see pci_phys_mem_access_prot() doesn't use file 
so you could likely drop that argument on phys_mem_access_prot() on 
powerpc. But when I for instance look at arm, I see that the file 
argument is used, see 
https://elixir.bootlin.com/linux/v6.5/source/arch/arm/mm/mmu.c#L713

So, the simplest is maybe the following, allthough that's probably worth 
a comment:
diff --git a/arch/powerpc/include/asm/fb.h b/arch/powerpc/include/asm/fb.h
index 5f1a2e5f7654..8b9b856f476e 100644
--- a/arch/powerpc/include/asm/fb.h
+++ b/arch/powerpc/include/asm/fb.h
@@ -6,10 +6,9 @@

  #include <asm/page.h>

-static inline void fb_pgprotect(struct file *file, struct 
vm_area_struct *vma,
-				unsigned long off)
+static inline void fb_pgprotect(struct vm_area_struct *vma, unsigned 
long off)
  {
-	vma->vm_page_prot = phys_mem_access_prot(file, off >> PAGE_SHIFT,
+	vma->vm_page_prot = phys_mem_access_prot(NULL, off >> PAGE_SHIFT,
  						 vma->vm_end - vma->vm_start,
  						 vma->vm_page_prot);
  }


Christophe

Best regards
Thomas


[1] 
https://elixir.bootlin.com/linux/v6.5/source/include/asm-generic/fb.h#L19
[2] 
https://elixir.bootlin.com/linux/v6.5/source/arch/mips/include/asm/fb.h#L11
[3] 
https://elixir.bootlin.com/linux/v6.5/source/arch/powerpc/include/asm/fb.h#L12
[4] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/video/fbdev/core/fbmem.c#L1299

Re: Framebuffer mmap on PowerPC

From: Christophe Leroy <hidden>
Date: 2023-08-31 17:41:41


Le 31/08/2023 à 19:38, Christophe Leroy a écrit :
quoted hunk

Le 31/08/2023 à 16:41, Thomas Zimmermann a écrit :
quoted
Hi,

there's a per-architecture function called fb_pgprotect() that sets 
VMA's vm_page_prot for mmaped framebuffers. Most architectures use a 
simple implementation based on pgprot_writecomine() [1] or 
pgprot_noncached(). [2]

On PPC this function uses phys_mem_access_prot() and therefore 
requires the mmap call's file struct. [3] Removing the file argument 
would help with simplifying the caller of fb_pgprotect(). [4]

Why is the file even required on PPC?

Is it possible to replace phys_mem_access_prot() with something 
simpler that does not use the file struct?
Looks like phys_mem_access_prot() defaults to calling pgprot_noncached() 
see 
https://elixir.bootlin.com/linux/v6.5/source/arch/powerpc/mm/mem.c#L37 
but for a few platforms that's superseeded by 
pci_phys_mem_access_prot(), see 
https://elixir.bootlin.com/linux/v6.5/source/arch/powerpc/kernel/pci-common.c#L524

However, as far as I can see pci_phys_mem_access_prot() doesn't use file 
so you could likely drop that argument on phys_mem_access_prot() on 
powerpc. But when I for instance look at arm, I see that the file 
argument is used, see 
https://elixir.bootlin.com/linux/v6.5/source/arch/arm/mm/mmu.c#L713

So, the simplest is maybe the following, allthough that's probably worth 
a comment:
diff --git a/arch/powerpc/include/asm/fb.h b/arch/powerpc/include/asm/fb.h
index 5f1a2e5f7654..8b9b856f476e 100644
--- a/arch/powerpc/include/asm/fb.h
+++ b/arch/powerpc/include/asm/fb.h
@@ -6,10 +6,9 @@

  #include <asm/page.h>

-static inline void fb_pgprotect(struct file *file, struct 
vm_area_struct *vma,
-                unsigned long off)
+static inline void fb_pgprotect(struct vm_area_struct *vma, unsigned 
long off)
  {
-    vma->vm_page_prot = phys_mem_access_prot(file, off >> PAGE_SHIFT,
+    vma->vm_page_prot = phys_mem_access_prot(NULL, off >> PAGE_SHIFT,
                           vma->vm_end - vma->vm_start,
                           vma->vm_page_prot);
  }
And while at it, maybe also replace off >> PAGE_SHIFT by PHYS_PFN(off)

Christophe

Christophe

quoted
Best regards
Thomas


[1] 
https://elixir.bootlin.com/linux/v6.5/source/include/asm-generic/fb.h#L19
[2] 
https://elixir.bootlin.com/linux/v6.5/source/arch/mips/include/asm/fb.h#L11
[3] 
https://elixir.bootlin.com/linux/v6.5/source/arch/powerpc/include/asm/fb.h#L12
[4] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/video/fbdev/core/fbmem.c#L1299

Re: Framebuffer mmap on PowerPC

From: "Arnd Bergmann" <arnd@arndb.de>
Date: 2023-09-01 01:46:05

On Thu, Aug 31, 2023, at 10:41, Thomas Zimmermann wrote:
Hi,

there's a per-architecture function called fb_pgprotect() that sets 
VMA's vm_page_prot for mmaped framebuffers. Most architectures use a 
simple implementation based on pgprot_writecomine() [1] or 
pgprot_noncached(). [2]

On PPC this function uses phys_mem_access_prot() and therefore requires 
the mmap call's file struct. [3] Removing the file argument would help 
with simplifying the caller of fb_pgprotect(). [4]

Why is the file even required on PPC?

Is it possible to replace phys_mem_access_prot() with something simpler 
that does not use the file struct?
What what I can tell, the structure of the code is a result of
these constraints:

- some powerpc platforms use different page table flags for
  prefetchable vs nonprefetchable BARs on PCI memory.

- page table flags must match between all mappings, in particular
  here between /dev/fb0 and /dev/mem, as mismatched attributes
  cause a checkstop. On other architectures this may cause
  undefined behavior instead of a checkstop

It's unfortunate that we have multiple incompatible ways
to determine the page flags based on firmware (ia64),
pci (powerpc) or file->f_flags (arm, csky), when they all
try to solve the same problem here.

Christophe's suggested approach to simplify it is probably
fine, another way would be to pass the f_flags value instead
of the file pointer.

      Arnd

Re: Framebuffer mmap on PowerPC

From: Thomas Zimmermann <hidden>
Date: 2023-09-04 07:24:55

Hi

Am 31.08.23 um 19:38 schrieb Christophe Leroy:

Le 31/08/2023 à 16:41, Thomas Zimmermann a écrit :
quoted
Hi,

there's a per-architecture function called fb_pgprotect() that sets
VMA's vm_page_prot for mmaped framebuffers. Most architectures use a
simple implementation based on pgprot_writecomine() [1] or
pgprot_noncached(). [2]

On PPC this function uses phys_mem_access_prot() and therefore requires
the mmap call's file struct. [3] Removing the file argument would help
with simplifying the caller of fb_pgprotect(). [4]

Why is the file even required on PPC?

Is it possible to replace phys_mem_access_prot() with something simpler
that does not use the file struct?
Looks like phys_mem_access_prot() defaults to calling pgprot_noncached()
see
https://elixir.bootlin.com/linux/v6.5/source/arch/powerpc/mm/mem.c#L37
but for a few platforms that's superseeded by
pci_phys_mem_access_prot(), see
https://elixir.bootlin.com/linux/v6.5/source/arch/powerpc/kernel/pci-common.c#L524

However, as far as I can see pci_phys_mem_access_prot() doesn't use file
so you could likely drop that argument on phys_mem_access_prot() on
powerpc. But when I for instance look at arm, I see that the file
argument is used, see
https://elixir.bootlin.com/linux/v6.5/source/arch/arm/mm/mmu.c#L713
Right, I've seen these various implementations. Luckily, the ARM 
framebuffers use a plain pgprot_writecombine() without any references on 
to file.
So, the simplest is maybe the following, allthough that's probably worth
a comment:
Could we drop the file argument from PPC's internal functions and 
provide this interface to fb_pgprotect()? phys_mem_access_prot() would 
be a trivial wrapper around that internal API. I'd provide a patch to do 
that.

Best regards
Thomas
quoted hunk
diff --git a/arch/powerpc/include/asm/fb.h b/arch/powerpc/include/asm/fb.h
index 5f1a2e5f7654..8b9b856f476e 100644
--- a/arch/powerpc/include/asm/fb.h
+++ b/arch/powerpc/include/asm/fb.h
@@ -6,10 +6,9 @@

   #include <asm/page.h>

-static inline void fb_pgprotect(struct file *file, struct
vm_area_struct *vma,
-				unsigned long off)
+static inline void fb_pgprotect(struct vm_area_struct *vma, unsigned
long off)
   {
-	vma->vm_page_prot = phys_mem_access_prot(file, off >> PAGE_SHIFT,
+	vma->vm_page_prot = phys_mem_access_prot(NULL, off >> PAGE_SHIFT,
   						 vma->vm_end - vma->vm_start,
   						 vma->vm_page_prot);
   }


Christophe

quoted
Best regards
Thomas


[1]
https://elixir.bootlin.com/linux/v6.5/source/include/asm-generic/fb.h#L19
[2]
https://elixir.bootlin.com/linux/v6.5/source/arch/mips/include/asm/fb.h#L11
[3]
https://elixir.bootlin.com/linux/v6.5/source/arch/powerpc/include/asm/fb.h#L12
[4]
https://elixir.bootlin.com/linux/v6.5/source/drivers/video/fbdev/core/fbmem.c#L1299
-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help