Message-ID:
 <DS7PR12MB6070AAA0C413DBF26F685207CD222@DS7PR12MB6070.namprd12.prod.outlook.com>
Date: Thu, 21 Nov 2024 10:07:00 +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: Tuesday, November 19, 2024 9:45 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
> 
> On Tue, Oct 29, 2024 at 02:47:34PM +0530, Nava kishore Manne wrote:
> > Introduces an IOCTL interface within the fpga-region subsystem,
> > providing a generic and standardized mechanism for configuring (or)
> > reprogramming FPGAs during runtime. The newly added interface supports
> > both OF (Open Firmware) and non-OF devices, leveraging vendor-specific
> > callbacks (e.g., configuration + enumeration, removal, and status) to
> > accommodate a wide range of device specific configurations.
> >
> > The IOCTL interface ensures compatibility with both OF and non-OF
> > devices, allowing for seamless FPGA reprogramming across diverse
> > platforms.
> >
> > Vendor-specific callbacks are integrated into the interface, enabling
> > custom FPGA configuration + enumeration, removal, and status reporting
> > mechanisms, ensuring flexibility for vendor implementations.
> >
> > This solution enhances FPGA runtime management, supporting various
> > device types and vendors, while ensuring compatibility with the
> > current FPGA configuration flow.
> >
> > Signed-off-by: Nava kishore Manne <nava.kishore.manne@....com>
> > ---
> > Changes for v2:
> >  - As discussed with Yilun, the implementation has been modified to
> > utilize a  callback approach, enabling seamless handling of both OF and non-OF
> devices.
> >
> >  - As suggested by Yilun in the POC code, we have moved away from
> > using  void *args  as a parameter for ICOTL inputs to obtain the
> > required user inputs. Instead, we are  utilizing the
> > fpga_region_config_info structure to gather user inputs. Currently,
> > this structure is implemented to support only OF devices, but we intend to extend
> it by incorporating new members to accommodate non-OF devices in the future.
> >
> >  drivers/fpga/fpga-region.c       | 110 +++++++++++++++++++++++++++++++
> >  drivers/fpga/of-fpga-region.c    |  91 ++++++++++++++++++++++++-
> >  include/linux/fpga/fpga-region.h |  32 +++++++++
> > include/uapi/linux/fpga-region.h |  51 ++++++++++++++
> >  4 files changed, 283 insertions(+), 1 deletion(-)  create mode 100644
> > include/uapi/linux/fpga-region.h
> >
> > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> > index 753cd142503e..c6bea3c99a69 100644
> > --- a/drivers/fpga/fpga-region.c
> > +++ b/drivers/fpga/fpga-region.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/fpga/fpga-bridge.h>
> >  #include <linux/fpga/fpga-mgr.h>
> >  #include <linux/fpga/fpga-region.h>
> > +#include <linux/fpga-region.h>
> >  #include <linux/idr.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> > @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = {
> > };  ATTRIBUTE_GROUPS(fpga_region);
> >
> > +static int fpga_region_device_open(struct inode *inode, struct file
> > +*file) {
> > +	struct miscdevice *miscdev = file->private_data;
> > +	struct fpga_region *region = container_of(miscdev, struct
> > +fpga_region, miscdev);
> > +
> > +	file->private_data = region;
> > +
> > +	return 0;
> > +}
> > +
> > +static int fpga_region_device_release(struct inode *inode, struct
> > +file *file) {
> > +	return 0;
> > +}
> > +
> > +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd,
> > +				     unsigned long arg)
> > +{
> > +	int err;
> > +	void __user *argp = (void __user *)arg;
> > +	struct fpga_region_config_info config_info;
> > +	struct fpga_region *region =  (struct fpga_region
> > +*)(file->private_data);
> > +
> > +	switch (cmd) {
> > +	case FPGA_REGION_IOCTL_LOAD:
> > +		if (copy_from_user(&config_info, argp, sizeof(struct
> fpga_region_config_info)))
> > +			return -EFAULT;
> > +
> > +		err = region->region_ops->region_config_enumeration(region,
> > +&config_info);
> > +
> > +		break;
> > +	case FPGA_REGION_IOCTL_REMOVE:
> > +		if (copy_from_user(&config_info, argp, sizeof(struct
> fpga_region_config_info)))
> > +			return -EFAULT;
> > +
> > +		err = region->region_ops->region_remove(region, &config_info);
> > +
> > +		break;
> > +	case FPGA_REGION_IOCTL_STATUS:
> > +		unsigned int status;
> > +
> > +		status = region->region_ops->region_status(region);
> > +
> > +		if (copy_to_user((void __user *)arg, &status, sizeof(status)))
> > +			err = -EFAULT;
> > +		break;
> > +	default:
> > +		err = -ENOTTY;
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +static const struct file_operations fpga_region_fops = {
> > +	.owner		= THIS_MODULE,
> > +	.open		= fpga_region_device_open,
> > +	.release	= fpga_region_device_release,
> > +	.unlocked_ioctl	= fpga_region_device_ioctl,
> > +	.compat_ioctl	= fpga_region_device_ioctl,
> > +};
> > +
> >  /**
> >   * __fpga_region_register_full - create and register an FPGA Region device
> >   * @parent: device parent
> > @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const
> struct fpga_region_info
> >  	if (ret)
> >  		goto err_remove;
> >
> > +	if (info->region_ops) {
> > +		region->region_ops = info->region_ops;
> > +		region->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +		region->miscdev.name = kobject_name(&region->dev.kobj);
> > +		region->miscdev.fops = &fpga_region_fops;
> > +		ret = misc_register(&region->miscdev);
> > +		if (ret) {
> > +			pr_err("fpga-region: failed to register misc device.\n");
> > +			goto err_remove;
> > +		}
> > +	}
> > +
> >  	ret = device_register(&region->dev);
> >  	if (ret) {
> > +		misc_deregister(&region->miscdev);
> >  		put_device(&region->dev);
> >  		return ERR_PTR(ret);
> >  	}
> > @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent,
> > struct fpga_manager *mgr,  }
> > EXPORT_SYMBOL_GPL(__fpga_region_register);
> >
> > +/**
> > + * __fpga_region_register_with_ops - create and register an FPGA
> > +Region device
> > + * with user interface call-backs.
> > + * @parent: device parent
> > + * @mgr: manager that programs this region
> > + * @region_ops: ops for low level FPGA region for device
> > +enumeration/removal
> > + * @priv: of-fpga-region private data
> > + * @get_bridges: optional function to get bridges to a list
> > + * @owner: module containing the get_bridges function
> > + *
> > + * This simple version of the register function should be sufficient for most users.
> > + * The fpga_region_register_full() function is available for users
> > +that need to
> > + * pass additional, optional parameters.
> > + *
> > + * Return: struct fpga_region or ERR_PTR()  */ struct fpga_region *
> > +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager
> *mgr,
> > +				const struct fpga_region_ops *region_ops,
> > +				void *priv,
> > +				int (*get_bridges)(struct fpga_region *),
> > +				struct module *owner)
> > +{
> > +	struct fpga_region_info info = { 0 };
> > +
> > +	info.mgr = mgr;
> > +	info.priv = priv;
> > +	info.get_bridges = get_bridges;
> > +	info.region_ops = region_ops;
> > +
> > +	return __fpga_region_register_full(parent, &info, owner); }
> > +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops);
> > +
> >  /**
> >   * fpga_region_unregister - unregister an FPGA region
> >   * @region: FPGA region
> > @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register);
> >   */
> >  void fpga_region_unregister(struct fpga_region *region)  {
> > +	misc_deregister(&region->miscdev);
> >  	device_unregister(&region->dev);
> >  }
> >  EXPORT_SYMBOL_GPL(fpga_region_unregister);
> > diff --git a/drivers/fpga/of-fpga-region.c
> > b/drivers/fpga/of-fpga-region.c index 8526a5a86f0c..63fe56e0466f
> > 100644
> > --- a/drivers/fpga/of-fpga-region.c
> > +++ b/drivers/fpga/of-fpga-region.c
> > @@ -8,6 +8,8 @@
> >  #include <linux/fpga/fpga-bridge.h>
> >  #include <linux/fpga/fpga-mgr.h>
> >  #include <linux/fpga/fpga-region.h>
> > +#include <linux/firmware.h>
> > +#include <linux/fpga-region.h>
> >  #include <linux/idr.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> > @@ -18,6 +20,20 @@
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >
> > +/**
> > + * struct of_fpga_region_priv - Private data structure
> > + * image.
> > + * @dev:	Device data structure
> > + * @fw:		firmware of coeff table.
> > + * @path:	path of FPGA overlay image firmware file.
> > + * @ovcs_id:	overlay changeset id.
> > + */
> > +struct of_fpga_region_priv {
> > +	struct device *dev;
> > +	const struct firmware *fw;
> > +	int ovcs_id;
> > +};
> > +
> >  static const struct of_device_id fpga_region_of_match[] = {
> >  	{ .compatible = "fpga-region", },
> >  	{},
> > @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = {
> >  	.notifier_call = of_fpga_region_notify,  };
> >
> > +static int of_fpga_region_status(struct fpga_region *region) {
> > +	struct of_fpga_region_priv *ovcs = region->priv;
> > +
> > +	if (ovcs->ovcs_id)
> > +		return FPGA_REGION_HAS_PL;
> 
> Could you help specify what is PL?

We will replace "PL" (Programmable Logic) with FPGA_REGION_HAS_CONFIGURED
for better clarity.

> > +
> > +	return FPGA_REGION_EMPTY;
> > +}
> > +
> > +static int of_fpga_region_config_enumeration(struct fpga_region *region,
> > +					     struct fpga_region_config_info *config_info)
> {
> > +	struct of_fpga_region_priv *ovcs = region->priv;
> > +	int err;
> > +
> > +	/* if it's set do not allow changes */
> > +	if (ovcs->ovcs_id)
> > +		return -EPERM;
> > +
> > +	err = request_firmware(&ovcs->fw, config_info->firmware_name, NULL);
> > +	if (err != 0)
> > +		goto out_err;
> > +
> > +	err = of_overlay_fdt_apply((void *)ovcs->fw->data, ovcs->fw->size,
> > +				   &ovcs->ovcs_id, NULL);
> > +	if (err < 0) {
> > +		pr_err("%s: Failed to create overlay (err=%d)\n",
> > +		       __func__, err);
> > +		release_firmware(ovcs->fw);
> > +		goto out_err;
> > +	}
> > +
> > +	return 0;
> > +
> > +out_err:
> > +	ovcs->ovcs_id = 0;
> > +	ovcs->fw = NULL;
> > +
> > +	return err;
> > +}
> > +
> > +static int of_fpga_region_config_remove(struct fpga_region *region,
> > +					struct fpga_region_config_info *config_info) {
> > +	struct of_fpga_region_priv *ovcs = region->priv;
> > +
> > +	if (!ovcs->ovcs_id)
> > +		return -EPERM;
> > +
> > +	of_overlay_remove(&ovcs->ovcs_id);
> > +	release_firmware(ovcs->fw);
> > +
> > +	ovcs->ovcs_id = 0;
> > +	ovcs->fw = NULL;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct fpga_region_ops region_ops = {
> > +	.region_status = of_fpga_region_status,
> > +	.region_config_enumeration = of_fpga_region_config_enumeration,
> > +	.region_remove = of_fpga_region_config_remove, };
> > +
> >  static int of_fpga_region_probe(struct platform_device *pdev)  {
> >  	struct device *dev = &pdev->dev;
> >  	struct device_node *np = dev->of_node;
> > +	struct of_fpga_region_priv *priv;
> >  	struct fpga_region *region;
> >  	struct fpga_manager *mgr;
> >  	int ret;
> >
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->dev = dev;
> > +
> >  	/* Find the FPGA mgr specified by region or parent region. */
> >  	mgr = of_fpga_region_get_mgr(np);
> >  	if (IS_ERR(mgr))
> >  		return -EPROBE_DEFER;
> >
> > -	region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges);
> > +	region = fpga_region_register_with_ops(dev, mgr, &region_ops, priv,
> > +					       of_fpga_region_get_bridges);
> >  	if (IS_ERR(region)) {
> >  		ret = PTR_ERR(region);
> >  		goto eprobe_mgr_put;
> > diff --git a/include/linux/fpga/fpga-region.h
> > b/include/linux/fpga/fpga-region.h
> > index 5fbc05fe70a6..3a3ba6dbb5e1 100644
> > --- a/include/linux/fpga/fpga-region.h
> > +++ b/include/linux/fpga/fpga-region.h
> > @@ -6,15 +6,35 @@
> >  #include <linux/device.h>
> >  #include <linux/fpga/fpga-mgr.h>
> >  #include <linux/fpga/fpga-bridge.h>
> > +#include <linux/fpga-region.h>
> > +#include <linux/miscdevice.h>
> >
> >  struct fpga_region;
> >
> > +/**
> > + * 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
requirements are handled before proceeding to the programming phase.
 
Programming:
The common API fpga_region_program_fpga() is used to program the image
to hardware. This standardizes the programming logic, minimizing duplication
and ensuring consistency across implementations.
 
Enumeration:
A vendor-specific callback is used for real enumeration, enabling hardware
specific customization while keeping the flow flexible and adaptable

This approach provides a clean separation of responsibilities, with
vendor-specific logic confined to the pre-configuration and enumeration
phases, while the programming phase leverages a common API.
It simplifies maintenance and aligns well with the Program -> Enumerate flow.
 
Looking forward to your feedback.


Regards,
Navakishore.

Powered by blists - more mailing lists