Message-ID:
 <DS7PR12MB6070A5E6BB63E473C1D7028FCDFD2@DS7PR12MB6070.namprd12.prod.outlook.com>
Date: Tue, 11 Feb 2025 11:50:35 +0000
From: "Manne, Nava kishore" <nava.kishore.manne@....com>
To: Xu Yilun <yilun.xu@...ux.intel.com>
CC: "git (AMD-Xilinx)" <git@....com>, "mdf@...nel.org" <mdf@...nel.org>,
	"hao.wu@...el.com" <hao.wu@...el.com>, "yilun.xu@...el.com"
	<yilun.xu@...el.com>, "trix@...hat.com" <trix@...hat.com>, "robh@...nel.org"
	<robh@...nel.org>, "saravanak@...gle.com" <saravanak@...gle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime
 FPGA programming

Hi Yilun,

> -----Original Message-----
> From: Xu Yilun <yilun.xu@...ux.intel.com>
> Sent: Sunday, March 19, 2023 9:08 PM
> To: Manne, Nava kishore <nava.kishore.manne@....com>
> Cc: git (AMD-Xilinx) <git@....com>; mdf@...nel.org; hao.wu@...el.com;
> yilun.xu@...el.com; trix@...hat.com; robh@...nel.org; saravanak@...gle.com;
> linux-kernel@...r.kernel.org; linux-fpga@...r.kernel.org;
> devicetree@...r.kernel.org
> Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime
> FPGA programming
> 
> On Thu, Dec 19, 2024 at 09:47:12AM +0000, Manne, Nava kishore wrote:
> > Hi Yilun,
> >
> > > -----Original Message-----
> > > From: Xu Yilun <yilun.xu@...ux.intel.com>
> > > Sent: Tuesday, December 10, 2024 2:34 PM
> > > To: Manne, Nava kishore <nava.kishore.manne@....com>
> > > Cc: git (AMD-Xilinx) <git@....com>; mdf@...nel.org;
> > > hao.wu@...el.com; yilun.xu@...el.com; trix@...hat.com;
> > > robh@...nel.org; saravanak@...gle.com; linux-kernel@...r.kernel.org;
> > > linux-fpga@...r.kernel.org; devicetree@...r.kernel.org
> > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface
> > > for runtime FPGA programming
> > >
> > > On Wed, Dec 04, 2024 at 06:40:18AM +0000, Manne, Nava kishore wrote:
> > > > Hi Yilun,
> > > >
> > > > > -----Original Message-----
> > > > > From: Xu Yilun <yilun.xu@...ux.intel.com>
> > > > > Sent: Wednesday, November 27, 2024 7:20 AM
> > > > > To: Manne, Nava kishore <nava.kishore.manne@....com>
> > > > > Cc: git (AMD-Xilinx) <git@....com>; mdf@...nel.org;
> > > > > hao.wu@...el.com; yilun.xu@...el.com; trix@...hat.com;
> > > > > robh@...nel.org; saravanak@...gle.com;
> > > > > linux-kernel@...r.kernel.org; linux-fpga@...r.kernel.org;
> > > > > devicetree@...r.kernel.org
> > > > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL
> > > > > interface for runtime FPGA programming
> > > > >
> > > > > > > > + * struct fpga_region_ops - ops for low level FPGA region
> > > > > > > > +ops for device
> > > > > > > > + * enumeration/removal
> > > > > > > > + * @region_status: returns the FPGA region status
> > > > > > > > + * @region_config_enumeration: Configure and enumerate
> > > > > > > > +the FPGA
> > > region.
> > > > > > > > + * @region_remove: Remove all devices within the FPGA
> > > > > > > > +region
> > > > > > > > + * (which are added as part of the enumeration).
> > > > > > > > + */
> > > > > > > > +struct fpga_region_ops {
> > > > > > > > +	int (*region_status)(struct fpga_region *region);
> > > > > > > > +	int (*region_config_enumeration)(struct fpga_region *region,
> > > > > > > > +					 struct fpga_region_config_info
> > > *config_info);
> > > > > > >
> > > > > > > My current concern is still about this combined API, it just
> > > > > > > offloads all work to low level, but we have some common flows.
> > > > > > > That's why we introduce a common FPGA reprograming API.
> > > > > > >
> > > > > > > I didn't see issue about the vendor specific pre configuration.
> > > > > > > They are generally needed to initialize the struct
> > > > > > > fpga_image_info, which is a common structure for
> > > fpga_region_program_fpga().
> > > > > > >
> > > > > > > For port IDs(AFU) inputs for DFL, I think it could also be
> > > > > > > changed (Don't have to be implemented in this patchset).
> > > > > > > Previously DFL provides an uAPI for the whole device, so it
> > > > > > > needs a port_id input to position which fpga_region within
> > > > > > > the device for programming. But now, we are introducing a
> > > > > > > per fpga_region programming interface, IIUC port_id
> > > > > should not be needed anymore.
> > > > > > >
> > > > > > > The combined API is truly simple for leveraging the existing
> > > > > > > of-fpga-region overlay apply mechanism. But IMHO that flow
> > > > > > > doesn't fit our new uAPI well. That flow is to adapt the
> > > > > > > generic configfs overlay interface, which comes to a dead end as you
> mentioned.
> > > > > > >
> > > > > > > My gut feeling for the generic programing flow should be:
> > > > > > >
> > > > > > >  1. Program the image to HW.
> > > > > > >  2. Enumerate the programmed image (apply the DT overlay)
> > > > > > >
> > > > > > > Why we have to:
> > > > > > >
> > > > > > >  1. Start enumeration.
> > > > > > >  2. On pre enumeration, programe the image.
> > > > > > >  3. Real enumeration.
> > > > > > >
> > > > > >
> > > > > > I agree with the approach of leveraging vendor-specific
> > > > > > callbacks to handle the distinct phases of the FPGA programming process.
> > > > > > Here's the proposed flow.
> > > > > >
> > > > > > Pre-Configuration:
> > > > > > A vendor-specific callback extracts the required
> > > > > > pre-configuration details and initializes struct
> > > > > > fpga_image_info. This ensures that all vendor-specific
> > > > >
> > > > > Since we need to construct the fpga_image_info, initialize
> > > > > multiple field as needed, I'm wondering if configfs could be a solution for the
> uAPI?
> > > > >
> > > >
> > > > A configfs uAPI isn't necessary, we can manage this using the
> > > > proposed IOCTL
> > > flow.
> > > > The POC code looks as follows.
> > >
> > > I prefer more to configfs cause it provides standard FS way to
> > > create the fpga_image_info object, e.g. which attributes are visible
> > > for OF/non-OF region, which attributes come from image blob and can only be
> RO, etc.
> > >
> > > Of couse ioctl() could achieve the same goal but would add much more
> > > specific rules (maybe flags/types) for user to follow.
> > >
> >
> > Agreed. Using ConfigFS is preferable because it provides a
> > standardized filesystem interface for creating and managing the fpga_image_info
> object.
> >
> > The proposed new user interface is outlined as follows:
> >
> > # Mount ConfigFS filesystem
> > mount -t configfs none /sys/kernel/config
> >
> > # Upload Configuration and Load the Bitstream for the Targeted FPGA Region.
> >
> > Configuration File Upload:
> > Upload the configuration file containing the necessary metadata or
> > settings required for configuring the FPGA region. This file may vary
> > based on the vendor and includes important details specific to the vendor's
> requirements.
> >
> > Vendor-Specific Callback:
> > A vendor-specific callback function extracts the relevant configuration data from the
> file.
> > The format and contents of the configuration file can differ between
> > vendors. The callback then initializes the struct fpga_image_info,
> > ensuring all vendor-specific requirements are satisfied.
> >
> > Device-Specific Considerations:
> > For Open Firmware (OF) devices, fpga.dtbo files are used instead of fpga_config
> files.
> > These .dtbo files contain all necessary information to populate the
> fpga_image_info.
> > For non-OF devices, a vendor specific fpga.config files are used to
> > provide the required data for initializing the fpga_image_info.
> 
> non-OF fpga images usually don't contain fpga_image_info data (e.g.
> enable/disable_timeout_us). I think we don't have to force users embed these data in
> fpga image, provide additional configfs attributes to input these data is possible. For
> some FPGA regions (e.g. OF), these attributes could be RO, some could be RW,
> depends on different FPGA region drivers.
> 
> So I think we may have a Configuration File Upload interface, like:
> 
>   echo "config_file" > /sys/kernel/config/fpga/<region>/image
> 
> Some additional parameter interfaces, like:
> 
>   echo 10000 > /sys/kernel/config/fpga/<region>/enable_timeout
>   ...
> 
> And a Configuration interface, like:
> 
>   # programming
>   echo 1 > /sys/kernel/config/fpga/<region>/config
>   # removing
>   echo 0 > /sys/kernel/config/fpga/<region>/config
> 
> How do you think?
> 

I agree and am actively working on the POC changes.
I will submit the RFC patches at the earliest.

Regards,
Navakishore.

Powered by blists - more mailing lists