Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR

On Mon, Mar 25, 2019 at 05:58:36PM -0500, Scott Wood wrote:

On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:

quoted

On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:

quoted

In early partial reconfiguration private feature, it only
supports 32bit data width when writing data to hardware for
PR. 512bit data width PR support is an important optimization
for some specific solutions (e.g. XEON with FPGA integrated),
it allows driver to use AVX512 instruction to improve the
performance of partial reconfiguration. e.g. programming one
100MB bitstream image via this 512bit data width PR hardware
only takes ~300ms, but 32bit revision requires ~3s per test
result.

Please note now this optimization is only done on revision 2
of this PR private feature which is only used in integrated
solution that AVX512 is always supported.

Signed-off-by: Ananda Ravuri <redacted>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <redacted>
---
 drivers/fpga/dfl-fme-main.c |  3 ++
 drivers/fpga/dfl-fme-mgr.c  | 75 +++++++++++++++++++++++++++++++++++++-
--
-----
 drivers/fpga/dfl-fme-pr.c   | 45 ++++++++++++++++-----------
 drivers/fpga/dfl-fme.h      |  2 ++
 drivers/fpga/dfl.h          |  5 +++
 5 files changed, 99 insertions(+), 31 deletions(-)

diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
index 086ad24..076d74f 100644
--- a/drivers/fpga/dfl-fme-main.c
+++ b/drivers/fpga/dfl-fme-main.c

@@ -21,6 +21,8 @@
 #include "dfl.h"
 #include "dfl-fme.h"
 
+#define DRV_VERSION	"0.8"

What is this going to be used for?  Under what circumstances will the
driver version be bumped?  What does it have to do with 512-bit writes?

This patchset adds more features to this driver, so i would like to add
a DRV_VERSION there as an initial one. In the future, if some new features
or extensions for existing features (e.g. new revision of a private feature)
are added we need to bump this version.

quoted

quoted

+#if defined(CONFIG_X86) && defined(CONFIG_AS_AVX512)
+
+#include <asm/fpu/api.h>
+
+static inline void copy512(void *src, void __iomem *dst)
+{
+	kernel_fpu_begin();
+
+	asm volatile("vmovdqu64 (%0), %%zmm0;"
+		     "vmovntdq %%zmm0, (%1);"
+		     :
+		     : "r"(src), "r"(dst));
+
+	kernel_fpu_end();
+}

Shouldn't there be some sort of check that AVX512 is actually supported
on the running system?

Also, src should be const, and the asm statement should have a memory
clobber.

quoted

+#else
+static inline void copy512(void *src, void __iomem *dst)
+{
+	WARN_ON_ONCE(1);
+}
+#endif

Likewise, this will be called if a revision 2 device is used on non-x86
(or on x86 with an old binutils).  The driver should fall back to 32-bit
in such cases.

Sorry, I missed the comment about revision 2 only being on integrated
devices -- but will that always be the case?  Seems worthwhile to check for
AVX512 support anyway.  And there's still the possibility of being built
with an old binutils such that CONFIG_AS_AVX512 is not set, or running on a
kernel where avx512 was disabled via a boot option.

What about future revisions >= 2?  Currently the driver will treat them as
if they were revision < 2.  Is that intended?

Yes, it's intended. Currently we don't have any hardware with revisions > 2,
and support new revisions may need new code. :)  e.g. currently revision is
used to tell 32bit vs 512bit PR, but in future revisions, it may have new
capability registers for this purpose.

Thanks
Hao

-Scott