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

On Wed, 2019-03-27 at 13:10 +0800, Wu Hao wrote:

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

quoted

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.

This doesn't seem like a good way of advertising API availability... Besides
being awkward to query, what happens if a distro kernel has backported some
features but not others that came before?  What does it advertise?

I'd suggest some sort of feature flag mechanism that can be queried via
ioctl (e.g. along the lines of KVM capabilities), if "try the API and fall
back if it fails" is unsatisfactory.

Plus, if it's about new APIs being exposed, this doesn't seem like the right
patch for it to be in...

quoted

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.

The driver should refuse to bind to unrecognized revisions, if they're not
expected to be compatible.

-Scott