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

[Wu Hao]

On Wed, Mar 27, 2019 at 01:19:29AM -0500, Scott Wood wrote:
> 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:
> > > On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:
> > > > On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> > > > > 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 <ananda.ravuri@xxxxxxxxx>
> > > > > Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxx>
> > > > > Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
> > > > > ---
> > > > >  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?

DRV_VERSION here is not used for API availablity. :)

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

Actually this patch doesn't introduce new APIs, I am trying to make this
transparent to endusers. That means users don't need to know it's a 32bit
PR or a faster 512bit one, they still use the same IOCTL interface for PR.

the API_VERSION and CHECK_EXTENSION ioctls have been defined, but I think
at least we don't need to bump them for this change. How do you think?

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

Yes, agree.

Thanks
Hao