Re: [PATCH v8 2/4] fpga manager: add sysfs interface document

[Jason Gunthorpe <jgunthorpe@obsidianresearch.com>]

On Tue, Jan 13, 2015 at 04:28:47PM +0000, One Thousand Gnomes wrote:

> There is a lot of code overlap in things like loading the bitstreams,
> there is also some overlap because you want to be able to assign FPGA
> resources. For example if you have four FPGAs and you want to use one for
> OS stuff (say video) you want the other three to be open/close
> accessible, but if you've not got a video driver loaded and are running
> the same board headless you'd like all four to be handed out as normal
> resources.
>
> So IMHO it's no different to say kmalloc. We don't pre-empt kernel memory
> and give it to users but we don't reserve memory for kernel and not use
> it.

I do agree with this, and I think this is where this patch set goes so
wrong.

Just exposing all sorts of controls to userspace and having a way for
the kernel to load/unload a bitstream compleletely misses the point that
the two main ways to use FPGAs *require* some kind of kernel side
driver suppport on the FPGA.

Even if it is just a maths FPGA kernel side has to be involved in some
way to restrict DMAs, MMIO, by the user process. Obviously a FPGA that
uses kernel drivers needs kernel side help to bind those drivers.

Plonking /sys/class/fpga_manager/<fpga>/reset for either case is more
likely than not to completely crash the system.

I would look at this problem from a completely different angle - the
90% case is a single FPGA that is loaded to provide HW that Linux
drivers bind to, these days the majority use DT.

So..

soc
{
    zynq_cfg: ps7-dev-cfg@f8007000 { compatible = "xlnx,zynq-devcfg-1.0"; }
    
    fpga@pl {
       compatible = "..";
       fpga,api = "gpio-api1";
       fpga,controller = &zynq_cfg;

       gp0_axi: axi@40000000 {
         gpio3: klina_gpio@80010 {
                 #gpio-cells = <2>;
                 compatible = "linux,basic-mmio-gpio";
                 gpio-controller;
                 reg-names = "dat", "set", "dirin";
                 reg = <0x80010 4>,
                       <0x80014 4>,
                       <0x80018 4>;
         };
       }
    }  
}

Make it so linux can boot, automatically fetch the gpio-api1 bit file
and load it into the chip and then bind the GPIO driver to the FPGA
resource. All without userspace involvment, all potentially done
before calling INIT.

Make it so via sysfs we can reverse this process and reboot the FPGA.

Make it so userspace can't do something to the FPGA without first
unloading the drivers.

Then worry about how to change the FPGA, or providing more user space
control over the process (DT overlays are a natural answer).

Providing a roll-your-own approach via a bunch of sysfs controls
satisfies the 'make the HW work' need without actually providing the
overall architecture someone needs to *make use* of this mechanism.

And no, this isn't just a 'theory', I have 4 shipping products today
that work pretty much exactly like this, including a Zynq
design. Binding kernel drivers to a FPGA once it is loaded is a big
functionality gap, and an annoying problem. Programming the actual
FPGA? That is the *EASIEST* part of this whole thing!

Honestly, I'd much rather see the above use case solved properly
without sysfs support and then go from there to build a replacable
FPGA scheme on the same concepts.

Providing a 'build your own' solution with sysfs controls just seems
to completely miss the mark to me.

> So IMHO it's no different to say kmalloc. We don't pre-empt kernel memory
> and give it to users but we don't reserve memory for kernel and not use
> it.

I think we see a kernel side with more structure, that knows when the
FPGA is in use or not, there is a pretty clear path to later add a
/dev/ scheme for user space use that can properly interlock.

A /dev/ scheme doesn't make much sense to bind kernel drivers...

Jason