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

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

[Anson Jacob]

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

[Anson Jacob]

- 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 <Anson.Jacob@amd.com>
CC: Christoph Hellwig <hch@infradead.org>
CC: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
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

[Christoph Hellwig]

> +
> +/*
> + * 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

[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 <Anson.Jacob@amd.com>
CC: Christoph Hellwig <hch@infradead.org>
CC: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
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

[Christian König]

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 <Anson.Jacob@amd.com>
> CC: Christoph Hellwig <hch@infradead.org>
> CC: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> 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.

> ---
>   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

[Christoph Hellwig]

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

[Christian König]

Am 21.07.21 um 08:51 schrieb Christoph Hellwig:
> 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.

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.