Re: [PATCH v3 14/21] fpga: dfl: add fpga manager platform driver for FME

[Alan Tull]

On Mon, Feb 5, 2018 at 8:17 PM, Wu Hao <hao.wu@xxxxxxxxx> wrote:
> On Mon, Feb 05, 2018 at 11:21:52AM -0600, Alan Tull wrote:
>> On Sun, Feb 4, 2018 at 4:05 AM, Wu Hao <hao.wu@xxxxxxxxx> wrote:
>> > On Sat, Feb 03, 2018 at 11:41:24AM +0100, Moritz Fischer wrote:
>> >> Hi Hao,
>> >>
>> >> On Fri, Feb 02, 2018 at 04:26:26PM -0800, Luebbers, Enno wrote:
>> >> > Hi Hao, Alan,
>> >> >
>> >> > On Fri, Feb 02, 2018 at 05:42:13PM +0800, Wu Hao wrote:
>> >> > > On Thu, Feb 01, 2018 at 04:00:36PM -0600, Alan Tull wrote:
>> >> > > > On Mon, Nov 27, 2017 at 12:42 AM, Wu Hao <hao.wu@xxxxxxxxx> wrote:
>> >> > > >
>> >> > > > Hi Hao,
>> >> > > >
>> >> > > > A few comments below.   Besides that, looks good.
>> >> > > >
>> >> > > > > This patch adds fpga manager driver for FPGA Management Engine (FME). It
>> >> > > > > implements fpga_manager_ops for FPGA Partial Reconfiguration function.
>> >> > > > >
>> >> > > > > Signed-off-by: Tim Whisonant <tim.whisonant@xxxxxxxxx>
>> >> > > > > Signed-off-by: Enno Luebbers <enno.luebbers@xxxxxxxxx>
>> >> > > > > Signed-off-by: Shiva Rao <shiva.rao@xxxxxxxxx>
>> >> > > > > Signed-off-by: Christopher Rauer <christopher.rauer@xxxxxxxxx>
>> >> > > > > Signed-off-by: Kang Luwei <luwei.kang@xxxxxxxxx>
>> >> > > > > Signed-off-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
>> >> > > > > Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
>> >> > > > > ----
>> >> > > > > v3: rename driver to dfl-fpga-fme-mgr
>> >> > > > >     implemented status callback for fpga manager
>> >> > > > >     rebased due to fpga api changes
>> >> > > > > ---
>> >> > > > >  .../ABI/testing/sysfs-platform-fpga-dfl-fme-mgr    |   8 +
>> >> > > > >  drivers/fpga/Kconfig                               |   6 +
>> >> > > > >  drivers/fpga/Makefile                              |   1 +
>> >> > > > >  drivers/fpga/fpga-dfl-fme-mgr.c                    | 318 +++++++++++++++++++++
>> >> > > > >  drivers/fpga/fpga-dfl.h                            |  39 ++-
>> >> > > > >  5 files changed, 371 insertions(+), 1 deletion(-)
>> >> > > > >  create mode 100644 Documentation/ABI/testing/sysfs-platform-fpga-dfl-fme-mgr
>> >> > > > >  create mode 100644 drivers/fpga/fpga-dfl-fme-mgr.c
>> >> > > > >
>> >> > > > > diff --git a/Documentation/ABI/testing/sysfs-platform-fpga-dfl-fme-mgr b/Documentation/ABI/testing/sysfs-platform-fpga-dfl-fme-mgr
>> >> > > > > new file mode 100644
>> >> > > > > index 0000000..2d4f917
>> >> > > > > --- /dev/null
>> >> > > > > +++ b/Documentation/ABI/testing/sysfs-platform-fpga-dfl-fme-mgr
>> >> > > > > @@ -0,0 +1,8 @@
>> >> > > > > +What:          /sys/bus/platform/devices/fpga-dfl-fme-mgr.0/interface_id
>> >> > > > > +Date:          November 2017
>> >> > > > > +KernelVersion:  4.15
>> >> > > > > +Contact:       Wu Hao <hao.wu@xxxxxxxxx>
>> >> > > > > +Description:   Read-only. It returns interface id of partial reconfiguration
>> >> > > > > +               hardware. Userspace could use this information to check if
>> >> > > > > +               current hardware is compatible with given image before FPGA
>> >> > > > > +               programming.
>> >> > > >
>> >> > > > I'm a little confused by this.  I can understand that the PR bitstream
>> >> > > > has a dependency on the FPGA's static image, but I don't understand
>> >> > > > the dependency of the bistream on the hardware that is used to program
>> >> > > > the bitstream to the FPGA.
>> >> > >
>> >> > > Sorry for the confusion, the interface_id is used to indicate the version of
>> >> > > the hardware for partial reconfiguration (it's part of the static image of
>> >> > > the FPGA device). Will improve the description on this.
>> >>
>> >> I'm not sure userland should be making the call on whether what you're
>> >> trying to load is compatible or not.
>>
>> Could you explain more about what your concern was about this (unless
>> Hao has covered it below)?
>>
>> It makes sense to me in this use case at least since userspace has a
>> pile of images and is choosing which one to load.
>>
>> >> Isn't there a way to check this in
>> >> your PR reconfiguration handler in-kernel?
>> >
>> > Hi Moritz
>> >
>> > Actually with current driver interface, doing a partial reconfiguration with an
>> > incompatible image, then driver will report PR failure with error code
>> > FPGA_MGR_STATUS_INCOMPATIBLE_IMAGE_ERR as hardware checks it, but anyway user
>> > needs to know hardware interface_id information to find or re-generated correct
>> > images. I think it's more flexible to leave it to userspace on using this
>> > information exposed by driver. : )
>> >
>> > Thanks
>> > Hao
>> >
>> >>
>> >> > >
>> >> >
>> >> > The interface_id expresses the compatibility of the static region with PR
>> >> > bitstreams generated for it. It changes every time a new static region is
>> >> > generated.
>>
>> In the near future the DFL framework will be used with SoC's that have
>> a hard FPGA PR manager (that's not part of the static region).  The
>> hard FPGA manager driver won't know anything about the static region.
>>
>> >> >
>> >> > Would it make more sense to have the interface_id exposed as part of the FME
>> >> > device (which represents the static region)? I'm not sure - it kind of also
>> >> > makes sense here, where you would have all the information in one place (if the
>> >> > interface_id matches, I can use this component to program a bitstream).
>>
>> According to the intel-fpga.txt document, the identifier for the
>> static image is at
>>
>> /sys/class/fpga_region/regionX/fpga-dfl-fme.n/bitstream_id
>
> Hi Alan

Hi Hao,

>
> This bitstream_id refects the full static region version. As you know, PR is
> only a sub feature of the FME functional unit, it's possible that we have
> different static region (different bitstream_id) but it has the exact same
> PR sub feature under the FME, only changes on other sub features or function
> units.

OK, thanks for that explanation.  That makes sense but could have
easily been different.  Please document this somewhere.

Alan