[RFC v2 0/2] PPC: Add generic FPU api similar to x86

7 messages, 3 authors, 2021-07-21 · open the first message on its own page

[RFC v2 0/2] PPC: Add generic FPU api similar to x86

From: Anson Jacob <hidden>
Date: 2021-07-21 04:48:35

This is an attempt to have generic FPU enable/disable
calls similar to x86. 
So that we can simplify gpu/drm/amd/display/dc/os_types.h

Also adds FPU correctness logic seen in x86.

v2:
- Added asm/fpu/api.h powerpc variant with kernel_fpu_begin/end()
  and kernel_fpu_enabled() declarations
- Updated kernel_fpu_enabled as EXPORT_SYMBOL_GPL
- Got rid of macro switch for PPC in dc/os_types.h as header file
  with same name as x86 is added by previous patch in the series

Anson Jacob (2):
  ppc/fpu: Add generic FPU api similar to x86
  drm/amd/display: Use PPC FPU functions

 arch/powerpc/include/asm/fpu/api.h        |  18 +++
 arch/powerpc/include/asm/switch_to.h      |  25 +----
 arch/powerpc/kernel/process.c             | 130 ++++++++++++++++++++++
 drivers/gpu/drm/amd/display/dc/os_types.h |  29 -----
 4 files changed, 151 insertions(+), 51 deletions(-)
 create mode 100644 arch/powerpc/include/asm/fpu/api.h

-- 
2.25.1

[RFC v2 1/2] ppc/fpu: Add generic FPU api similar to x86

From: Anson Jacob <hidden>
Date: 2021-07-21 04:48:37

- Add kernel_fpu_begin & kernel_fpu_end API as x86
- Add logic similar to x86 to ensure fpu
  begin/end call correctness
- Add kernel_fpu_enabled to know if FPU is enabled

v2:
- Added asm/fpu/api.h powerpc variant with kernel_fpu_begin/end()
  and kernel_fpu_enabled() declarations
- Updated kernel_fpu_enabled as EXPORT_SYMBOL_GPL

Signed-off-by: Anson Jacob <redacted>
CC: Christoph Hellwig <hch@infradead.org>
CC: Rodrigo Siqueira <redacted>
CC: Harry Wentland <harry.wentland@amd.com>
CC: Christian König <christian.koenig@amd.com>
---
 arch/powerpc/include/asm/fpu/api.h   |  18 ++++
 arch/powerpc/include/asm/switch_to.h |  25 +-----
 arch/powerpc/kernel/process.c        | 130 +++++++++++++++++++++++++++
 3 files changed, 151 insertions(+), 22 deletions(-)
 create mode 100644 arch/powerpc/include/asm/fpu/api.h
diff --git a/arch/powerpc/include/asm/fpu/api.h b/arch/powerpc/include/asm/fpu/api.h
new file mode 100644
index 000000000000..57308cdc65c9
--- /dev/null
+++ b/arch/powerpc/include/asm/fpu/api.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _ASM_POWERPC_FPU_API_H
+#define _ASM_POWERPC_FPU_API_H
+
+/*
+ * Use kernel_fpu_begin/end() if you intend to use FPU in kernel context. It
+ * disables preemption so be careful if you intend to use it for long periods
+ * of time.
+ * TODO: If you intend to use the FPU in irq/softirq you need to check first with
+ * irq_fpu_usable() if it is possible.
+ */
+
+extern bool kernel_fpu_enabled(void);
+extern void kernel_fpu_begin(void);
+extern void kernel_fpu_end(void);
+
+#endif /* _ASM_POWERPC_FPU_API_H */
diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
index 9d1fbd8be1c7..a9a919279f48 100644
--- a/arch/powerpc/include/asm/switch_to.h
+++ b/arch/powerpc/include/asm/switch_to.h
@@ -41,10 +41,7 @@ extern void enable_kernel_fp(void);
 extern void flush_fp_to_thread(struct task_struct *);
 extern void giveup_fpu(struct task_struct *);
 extern void save_fpu(struct task_struct *);
-static inline void disable_kernel_fp(void)
-{
-	msr_check_and_clear(MSR_FP);
-}
+extern void disable_kernel_fp(void);
 #else
 static inline void save_fpu(struct task_struct *t) { }
 static inline void flush_fp_to_thread(struct task_struct *t) { }
@@ -55,10 +52,7 @@ extern void enable_kernel_altivec(void);
 extern void flush_altivec_to_thread(struct task_struct *);
 extern void giveup_altivec(struct task_struct *);
 extern void save_altivec(struct task_struct *);
-static inline void disable_kernel_altivec(void)
-{
-	msr_check_and_clear(MSR_VEC);
-}
+extern void disable_kernel_altivec(void);
 #else
 static inline void save_altivec(struct task_struct *t) { }
 static inline void __giveup_altivec(struct task_struct *t) { }
@@ -67,20 +61,7 @@ static inline void __giveup_altivec(struct task_struct *t) { }
 #ifdef CONFIG_VSX
 extern void enable_kernel_vsx(void);
 extern void flush_vsx_to_thread(struct task_struct *);
-static inline void disable_kernel_vsx(void)
-{
-	msr_check_and_clear(MSR_FP|MSR_VEC|MSR_VSX);
-}
-#else
-static inline void enable_kernel_vsx(void)
-{
-	BUILD_BUG();
-}
-
-static inline void disable_kernel_vsx(void)
-{
-	BUILD_BUG();
-}
+extern void disable_kernel_vsx(void);
 #endif
 
 #ifdef CONFIG_SPE
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 185beb290580..969096c0123c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -75,6 +75,17 @@
 #define TM_DEBUG(x...) do { } while(0)
 #endif
 
+/*
+ * Track whether the kernel is using the FPU state
+ * currently.
+ *
+ * This flag is used:
+ *
+ *   - kernel_fpu_begin()/end() correctness
+ *   - kernel_fpu_enabled info
+ */
+static DEFINE_PER_CPU(bool, in_kernel_fpu);
+
 extern unsigned long _get_SP(void);
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
@@ -212,6 +223,9 @@ void enable_kernel_fp(void)
 	unsigned long cpumsr;
 
 	WARN_ON(preemptible());
+	WARN_ON_ONCE(this_cpu_read(in_kernel_fpu));
+
+	this_cpu_write(in_kernel_fpu, true);
 
 	cpumsr = msr_check_and_set(MSR_FP);
 
@@ -231,6 +245,15 @@ void enable_kernel_fp(void)
 	}
 }
 EXPORT_SYMBOL(enable_kernel_fp);
+
+void disable_kernel_fp(void)
+{
+	WARN_ON_ONCE(!this_cpu_read(in_kernel_fpu));
+
+	this_cpu_write(in_kernel_fpu, false);
+	msr_check_and_clear(MSR_FP);
+}
+EXPORT_SYMBOL(disable_kernel_fp);
 #else
 static inline void __giveup_fpu(struct task_struct *tsk) { }
 #endif /* CONFIG_PPC_FPU */
@@ -263,6 +286,9 @@ void enable_kernel_altivec(void)
 	unsigned long cpumsr;
 
 	WARN_ON(preemptible());
+	WARN_ON_ONCE(this_cpu_read(in_kernel_fpu));
+
+	this_cpu_write(in_kernel_fpu, true);
 
 	cpumsr = msr_check_and_set(MSR_VEC);
 
@@ -283,6 +309,14 @@ void enable_kernel_altivec(void)
 }
 EXPORT_SYMBOL(enable_kernel_altivec);
 
+void disable_kernel_altivec(void)
+{
+	WARN_ON_ONCE(!this_cpu_read(in_kernel_fpu));
+
+	this_cpu_write(in_kernel_fpu, false);
+	msr_check_and_clear(MSR_VEC);
+}
+EXPORT_SYMBOL(disable_kernel_altivec);
 /*
  * Make sure the VMX/Altivec register state in the
  * the thread_struct is up to date for task tsk.
@@ -333,6 +367,9 @@ void enable_kernel_vsx(void)
 	unsigned long cpumsr;
 
 	WARN_ON(preemptible());
+	WARN_ON_ONCE(this_cpu_read(in_kernel_fpu));
+
+	this_cpu_write(in_kernel_fpu, true);
 
 	cpumsr = msr_check_and_set(MSR_FP|MSR_VEC|MSR_VSX);
 
@@ -354,6 +391,15 @@ void enable_kernel_vsx(void)
 }
 EXPORT_SYMBOL(enable_kernel_vsx);
 
+void disable_kernel_vsx(void)
+{
+	WARN_ON_ONCE(!this_cpu_read(in_kernel_fpu));
+
+	this_cpu_write(in_kernel_fpu, false);
+	msr_check_and_clear(MSR_FP|MSR_VEC|MSR_VSX);
+}
+EXPORT_SYMBOL(disable_kernel_vsx);
+
 void flush_vsx_to_thread(struct task_struct *tsk)
 {
 	if (tsk->thread.regs) {
@@ -406,6 +452,90 @@ void flush_spe_to_thread(struct task_struct *tsk)
 }
 #endif /* CONFIG_SPE */
 
+static bool fpu_support(void)
+{
+	if (cpu_has_feature(CPU_FTR_VSX_COMP)) {
+		return true;
+	} else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) {
+		return true;
+	} else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) {
+		return true;
+	}
+
+	return false;
+}
+
+bool kernel_fpu_enabled(void)
+{
+	return this_cpu_read(in_kernel_fpu);
+}
+EXPORT_SYMBOL_GPL(kernel_fpu_enabled);
+
+void kernel_fpu_begin(void)
+{
+	if (!fpu_support()) {
+		WARN_ON_ONCE(1);
+		return;
+	}
+
+	preempt_disable();
+
+#ifdef CONFIG_VSX
+	if (cpu_has_feature(CPU_FTR_VSX_COMP)) {
+		enable_kernel_vsx();
+		return;
+	}
+#endif
+
+#ifdef CONFIG_ALTIVEC
+	if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) {
+		enable_kernel_altivec();
+		return;
+	}
+#endif
+
+#ifdef CONFIG_PPC_FPU
+	if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) {
+		enable_kernel_fp();
+		return;
+	}
+#endif
+}
+EXPORT_SYMBOL_GPL(kernel_fpu_begin);
+
+void kernel_fpu_end(void)
+{
+	if (!fpu_support()) {
+		WARN_ON_ONCE(1);
+		return;
+	}
+
+#ifdef CONFIG_VSX
+	if (cpu_has_feature(CPU_FTR_VSX_COMP)) {
+		disable_kernel_vsx();
+		goto done;
+	}
+#endif
+
+#ifdef CONFIG_ALTIVEC
+	if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) {
+		disable_kernel_altivec();
+		goto done;
+	}
+#endif
+
+#ifdef CONFIG_PPC_FPU
+	if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) {
+		disable_kernel_fp();
+		goto done;
+	}
+#endif
+
+done:
+	preempt_enable();
+}
+EXPORT_SYMBOL_GPL(kernel_fpu_end);
+
 static unsigned long msr_all_available;
 
 static int __init init_msr_all_available(void)
-- 
2.25.1

Re: [RFC v2 1/2] ppc/fpu: Add generic FPU api similar to x86

From: Christoph Hellwig <hch@infradead.org>
Date: 2021-07-21 07:01:46

+
+/*
+ * Use kernel_fpu_begin/end() if you intend to use FPU in kernel context. It
+ * disables preemption so be careful if you intend to use it for long periods
+ * of time.
+ * TODO: If you intend to use the FPU in irq/softirq you need to check first with
+ * irq_fpu_usable() if it is possible.
Please avoid the overly lone lines comments.
+extern bool kernel_fpu_enabled(void);
+extern void kernel_fpu_begin(void);
+extern void kernel_fpu_end(void);
No need for the externs.
+/*
+ * Track whether the kernel is using the FPU state
+ * currently.
This all fits on a single line.
+static bool fpu_support(void)
+{
+	if (cpu_has_feature(CPU_FTR_VSX_COMP)) {
+		return true;
+	} else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) {
+		return true;
+	} else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) {
+		return true;
+	}
No need for the braces, or else after a return.  In fact this could
be simplified down to:

	return cpu_has_feature(CPU_FTR_VSX_COMP) ||
		cpu_has_feature(CPU_FTR_ALTIVEC_COMP) ||
		cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE));
+	preempt_disable();
+
+#ifdef CONFIG_VSX
+	if (cpu_has_feature(CPU_FTR_VSX_COMP)) {
+		enable_kernel_vsx();
+		return;
+	}
+#endif
+
+#ifdef CONFIG_ALTIVEC
+	if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) {
+		enable_kernel_altivec();
+		return;
+	}
+#endif
+
+#ifdef CONFIG_PPC_FPU
+	if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) {
+		enable_kernel_fp();
+		return;
+	}
+#endif
All the features are defined away if not supported (and we already rely
on that in fpu_support()).  So this could become:

	if (cpu_has_feature(CPU_FTR_VSX_COMP))
		enable_kernel_vsx();
	else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP))
		enable_kernel_altivec();
	else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE))
		enable_kernel_fp();

Same for the disable path.

[RFC v2 2/2] drm/amd/display: Use PPC FPU functions

From: Anson Jacob <hidden>
Date: 2021-07-21 04:49:22

Use kernel_fpu_begin & kernel_fpu_end for PPC

Depends on "ppc/fpu: Add generic FPU api similar to x86"

v2:
- Got rid of macro switch for PPC as header file with same
  name as x86 is added by previous patch in the series

Signed-off-by: Anson Jacob <redacted>
CC: Christoph Hellwig <hch@infradead.org>
CC: Rodrigo Siqueira <redacted>
CC: Harry Wentland <harry.wentland@amd.com>
CC: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/display/dc/os_types.h | 29 -----------------------
 1 file changed, 29 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
index 126c2f3a4dd3..47ef434f93d8 100644
--- a/drivers/gpu/drm/amd/display/dc/os_types.h
+++ b/drivers/gpu/drm/amd/display/dc/os_types.h
@@ -51,38 +51,9 @@
 #define dm_error(fmt, ...) DRM_ERROR(fmt, ##__VA_ARGS__)
 
 #if defined(CONFIG_DRM_AMD_DC_DCN)
-#if defined(CONFIG_X86)
 #include <asm/fpu/api.h>
 #define DC_FP_START() kernel_fpu_begin()
 #define DC_FP_END() kernel_fpu_end()
-#elif defined(CONFIG_PPC64)
-#include <asm/switch_to.h>
-#include <asm/cputable.h>
-#define DC_FP_START() { \
-	if (cpu_has_feature(CPU_FTR_VSX_COMP)) { \
-		preempt_disable(); \
-		enable_kernel_vsx(); \
-	} else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) { \
-		preempt_disable(); \
-		enable_kernel_altivec(); \
-	} else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) { \
-		preempt_disable(); \
-		enable_kernel_fp(); \
-	} \
-}
-#define DC_FP_END() { \
-	if (cpu_has_feature(CPU_FTR_VSX_COMP)) { \
-		disable_kernel_vsx(); \
-		preempt_enable(); \
-	} else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) { \
-		disable_kernel_altivec(); \
-		preempt_enable(); \
-	} else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) { \
-		disable_kernel_fp(); \
-		preempt_enable(); \
-	} \
-}
-#endif
 #endif
 
 /*
-- 
2.25.1

Re: [RFC v2 2/2] drm/amd/display: Use PPC FPU functions

From: Christian König <christian.koenig@amd.com>
Date: 2021-07-21 06:30:30

Am 21.07.21 um 06:48 schrieb Anson Jacob:
Use kernel_fpu_begin & kernel_fpu_end for PPC

Depends on "ppc/fpu: Add generic FPU api similar to x86"

v2:
- Got rid of macro switch for PPC as header file with same
   name as x86 is added by previous patch in the series

Signed-off-by: Anson Jacob <redacted>
CC: Christoph Hellwig <hch@infradead.org>
CC: Rodrigo Siqueira <redacted>
CC: Harry Wentland <harry.wentland@amd.com>
CC: Christian König <christian.koenig@amd.com>
Looks good in general, but question is what about other architectures 
like ARM?

Regards,
Christian.
quoted hunk
---
  drivers/gpu/drm/amd/display/dc/os_types.h | 29 -----------------------
  1 file changed, 29 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
index 126c2f3a4dd3..47ef434f93d8 100644
--- a/drivers/gpu/drm/amd/display/dc/os_types.h
+++ b/drivers/gpu/drm/amd/display/dc/os_types.h
@@ -51,38 +51,9 @@
  #define dm_error(fmt, ...) DRM_ERROR(fmt, ##__VA_ARGS__)
  
  #if defined(CONFIG_DRM_AMD_DC_DCN)
-#if defined(CONFIG_X86)
  #include <asm/fpu/api.h>
  #define DC_FP_START() kernel_fpu_begin()
  #define DC_FP_END() kernel_fpu_end()
-#elif defined(CONFIG_PPC64)
-#include <asm/switch_to.h>
-#include <asm/cputable.h>
-#define DC_FP_START() { \
-	if (cpu_has_feature(CPU_FTR_VSX_COMP)) { \
-		preempt_disable(); \
-		enable_kernel_vsx(); \
-	} else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) { \
-		preempt_disable(); \
-		enable_kernel_altivec(); \
-	} else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) { \
-		preempt_disable(); \
-		enable_kernel_fp(); \
-	} \
-}
-#define DC_FP_END() { \
-	if (cpu_has_feature(CPU_FTR_VSX_COMP)) { \
-		disable_kernel_vsx(); \
-		preempt_enable(); \
-	} else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) { \
-		disable_kernel_altivec(); \
-		preempt_enable(); \
-	} else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) { \
-		disable_kernel_fp(); \
-		preempt_enable(); \
-	} \
-}
-#endif
  #endif
  
  /*

Re: [RFC v2 2/2] drm/amd/display: Use PPC FPU functions

From: Christoph Hellwig <hch@infradead.org>
Date: 2021-07-21 06:53:31

On Wed, Jul 21, 2021 at 08:29:43AM +0200, Christian K??nig wrote:
Looks good in general, but question is what about other architectures like
ARM?
DRM_AMD_DC_DCN currently requires X86 || PPC64.

Maybe a good think would be to add a new KERNEL_FPU_API Kconfig symbol,
selected by x86 and powerpc (I think ppc32 should be fine too now) so
that we get these arch dependencies out of the driver.

Re: [RFC v2 2/2] drm/amd/display: Use PPC FPU functions

From: Christian König <christian.koenig@amd.com>
Date: 2021-07-21 09:33:40

Am 21.07.21 um 08:51 schrieb Christoph Hellwig:
On Wed, Jul 21, 2021 at 08:29:43AM +0200, Christian K??nig wrote:
quoted
Looks good in general, but question is what about other architectures like
ARM?
DRM_AMD_DC_DCN currently requires X86 || PPC64.
And exactly that's the problem I'm noting here. At least officially AMD 
claims that we support ARM and some very brave still use the hardware 
together with MIPS as well.
Maybe a good think would be to add a new KERNEL_FPU_API Kconfig symbol,
selected by x86 and powerpc (I think ppc32 should be fine too now) so
that we get these arch dependencies out of the driver.
Good idea.

Christian.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help