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
