| Message ID | 20210402090933.32276-1-nava.manne@xilinx.com |
|---|---|
| Headers | show |
| Series | Adds support to allow the bitstream configuration from pre-allocated dma-buffer | expand |
Hi Nava, On minor comment below. On 02/04/2021 11.09, Nava kishore Manne wrote: > Some systems are memory constrained but they need to load very > large Configuration files. The FPGA subsystem allows drivers to > request this Configuration image be loaded from the filesystem, > but this requires that the entire configuration data be loaded > into kernel memory first before it's provided to the driver. > This can lead to a situation where we map the configuration > data twice, once to load the configuration data into kernel > memory and once to copy the configuration data into the final > resting place which is nothing but a dma-able continuous buffer. > > This creates needless memory pressure and delays due to multiple > copies. Let's add a dmabuf handling support to the fpga manager > framework that allows drivers to load the Configuration data > directly from a pre-allocated buffer. This skips the intermediate > step of allocating a buffer in kernel memory to hold the > Configuration data. > > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com> > --- > drivers/fpga/fpga-mgr.c | 126 +++++++++++++++++++++++++++++++++- > drivers/fpga/of-fpga-region.c | 3 + > include/linux/fpga/fpga-mgr.h | 6 +- > 3 files changed, 132 insertions(+), 3 deletions(-) > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > index b85bc47c91a9..13faed61af62 100644 > --- a/drivers/fpga/fpga-mgr.c > +++ b/drivers/fpga/fpga-mgr.c > @@ -8,6 +8,8 @@ > * With code from the mailing list: > * Copyright (C) 2013 Xilinx, Inc. > */ > +#include <linux/dma-buf.h> > +#include <linux/dma-map-ops.h> > #include <linux/firmware.h> > #include <linux/fpga/fpga-mgr.h> > #include <linux/idr.h> > @@ -306,6 +308,51 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr, > return rc; > } > > +/** > + * fpga_mgr_buf_load - load fpga from image in dma buffer s/fpga_mgr_buf_load/fpga_dmabuf_load // Martin > + * @mgr: fpga manager > + * @info: fpga image info > + * > + * Step the low level fpga manager through the device-specific steps of getting > + * an FPGA ready to be configured, writing the image to it, then doing whatever > + * post-configuration steps necessary. This code assumes the caller got the > + * mgr pointer from of_fpga_mgr_get() and checked that it is not an error code. > + * > + * Return: 0 on success, negative error code otherwise. > + */ > +static int fpga_dmabuf_load(struct fpga_manager *mgr, > + struct fpga_image_info *info) > +{ > + struct dma_buf_attachment *attach; > + struct sg_table *sgt; > + int ret; > + > + /* create attachment for dmabuf with the user device */ > + attach = dma_buf_attach(mgr->dmabuf, &mgr->dev); > + if (IS_ERR(attach)) { > + pr_err("failed to attach dmabuf\n"); > + ret = PTR_ERR(attach); > + goto fail_put; > + } > + > + sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); > + if (IS_ERR(sgt)) { > + ret = PTR_ERR(sgt); > + goto fail_detach; > + } > + > + info->sgt = sgt; > + ret = fpga_mgr_buf_load_sg(mgr, info, info->sgt); > + dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL); > + > +fail_detach: > + dma_buf_detach(mgr->dmabuf, attach); > +fail_put: > + dma_buf_put(mgr->dmabuf); > + > + return ret; > +} > + > /** > * fpga_mgr_firmware_load - request firmware and load to fpga > * @mgr: fpga manager > @@ -358,6 +405,8 @@ static int fpga_mgr_firmware_load(struct fpga_manager *mgr, > */ > int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info) > { > + if (info->flags & FPGA_MGR_CONFIG_DMA_BUF) > + return fpga_dmabuf_load(mgr, info); > if (info->sgt) > return fpga_mgr_buf_load_sg(mgr, info, info->sgt); > if (info->buf && info->count) > @@ -549,6 +598,62 @@ void fpga_mgr_unlock(struct fpga_manager *mgr) > } > EXPORT_SYMBOL_GPL(fpga_mgr_unlock); > > +static int fpga_dmabuf_fd_get(struct file *file, char __user *argp) > +{ > + struct fpga_manager *mgr = (struct fpga_manager *)(file->private_data); > + int buffd; > + > + if (copy_from_user(&buffd, argp, sizeof(buffd))) > + return -EFAULT; > + > + mgr->dmabuf = dma_buf_get(buffd); > + if (IS_ERR_OR_NULL(mgr->dmabuf)) > + return -EINVAL; > + > + return 0; > +} > + > +static int fpga_device_open(struct inode *inode, struct file *file) > +{ > + struct miscdevice *miscdev = file->private_data; > + struct fpga_manager *mgr = container_of(miscdev, > + struct fpga_manager, miscdev); > + > + file->private_data = mgr; > + > + return 0; > +} > + > +static int fpga_device_release(struct inode *inode, struct file *file) > +{ > + return 0; > +} > + > +static long fpga_device_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + char __user *argp = (char __user *)arg; > + int err; > + > + switch (cmd) { > + case FPGA_IOCTL_LOAD_DMA_BUFF: > + err = fpga_dmabuf_fd_get(file, argp); > + break; > + default: > + err = -ENOTTY; > + } > + > + return err; > +} > + > +static const struct file_operations fpga_fops = { > + .owner = THIS_MODULE, > + .open = fpga_device_open, > + .release = fpga_device_release, > + .unlocked_ioctl = fpga_device_ioctl, > + .compat_ioctl = fpga_device_ioctl, > +}; > + > /** > * fpga_mgr_create - create and initialize a FPGA manager struct > * @dev: fpga manager device from pdev > @@ -569,8 +674,7 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name, > int id, ret; > > if (!mops || !mops->write_complete || !mops->state || > - !mops->write_init || (!mops->write && !mops->write_sg) || > - (mops->write && mops->write_sg)) { > + !mops->write_init || (!mops->write && !mops->write_sg)) { > dev_err(dev, "Attempt to register without fpga_manager_ops\n"); > return NULL; > } > @@ -601,10 +705,28 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name, > mgr->dev.of_node = dev->of_node; > mgr->dev.id = id; > > + /* Make device dma capable by inheriting from parent's */ > + set_dma_ops(&mgr->dev, get_dma_ops(dev)); > + ret = dma_coerce_mask_and_coherent(&mgr->dev, dma_get_mask(dev)); > + if (ret) { > + dev_warn(dev, > + "Failed to set DMA mask %llx.Trying to continue.%x\n", > + dma_get_mask(dev), ret); > + } > + > ret = dev_set_name(&mgr->dev, "fpga%d", id); > if (ret) > goto error_device; > > + mgr->miscdev.minor = MISC_DYNAMIC_MINOR; > + mgr->miscdev.name = kobject_name(&mgr->dev.kobj); > + mgr->miscdev.fops = &fpga_fops; > + ret = misc_register(&mgr->miscdev); > + if (ret) { > + pr_err("fpga: failed to register misc device.\n"); > + goto error_device; > + } > + > return mgr; > > error_device: > diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c > index 35fc2f3d4bd8..698e3e42ccba 100644 > --- a/drivers/fpga/of-fpga-region.c > +++ b/drivers/fpga/of-fpga-region.c > @@ -229,6 +229,9 @@ static struct fpga_image_info *of_fpga_region_parse_ov( > if (of_property_read_bool(overlay, "encrypted-fpga-config")) > info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM; > > + if (of_property_read_bool(overlay, "fpga-config-from-dmabuf")) > + info->flags |= FPGA_MGR_CONFIG_DMA_BUF; > + > if (!of_property_read_string(overlay, "firmware-name", > &firmware_name)) { > info->firmware_name = devm_kstrdup(dev, firmware_name, > diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h > index 2bc3030a69e5..6208c22f7bed 100644 > --- a/include/linux/fpga/fpga-mgr.h > +++ b/include/linux/fpga/fpga-mgr.h > @@ -9,6 +9,7 @@ > #define _LINUX_FPGA_MGR_H > > #include <linux/mutex.h> > +#include <linux/miscdevice.h> > #include <linux/platform_device.h> > > struct fpga_manager; > @@ -73,7 +74,7 @@ enum fpga_mgr_states { > #define FPGA_MGR_ENCRYPTED_BITSTREAM BIT(2) > #define FPGA_MGR_BITSTREAM_LSB_FIRST BIT(3) > #define FPGA_MGR_COMPRESSED_BITSTREAM BIT(4) > - > +#define FPGA_MGR_CONFIG_DMA_BUF BIT(5) > /** > * struct fpga_image_info - information specific to a FPGA image > * @flags: boolean flags as defined above > @@ -167,6 +168,8 @@ struct fpga_compat_id { > struct fpga_manager { > const char *name; > struct device dev; > + struct miscdevice miscdev; > + struct dma_buf *dmabuf; > struct mutex ref_mutex; > enum fpga_mgr_states state; > struct fpga_compat_id *compat_id; > @@ -204,4 +207,5 @@ struct fpga_manager *devm_fpga_mgr_create(struct device *dev, const char *name, > const struct fpga_manager_ops *mops, > void *priv); > > +#define FPGA_IOCTL_LOAD_DMA_BUFF _IOWR('R', 1, __u32) > #endif /*_LINUX_FPGA_MGR_H */ >
On Fri, Apr 02, 2021 at 02:39:31PM +0530, Nava kishore Manne wrote: > Add "fpga-config-from-dmabuf" property to allow the bitstream > configuration from pre-allocated dma-buffer. > > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com> > --- > Documentation/devicetree/bindings/fpga/fpga-region.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt > index 969ca53bb65e..c573cf258d60 100644 > --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt > @@ -177,6 +177,8 @@ Optional properties: > it indicates that the FPGA has already been programmed with this image. > If this property is in an overlay targeting a FPGA region, it is a > request to program the FPGA with that image. > +- fpga-config-from-dmabuf : boolean, set if the FPGA configured done from the > + pre-allocated dma-buffer. Sounds like an implementation detail of the OS. Doesn't belong in DT. Rob > - fpga-bridges : should contain a list of phandles to FPGA Bridges that must be > controlled during FPGA programming along with the parent FPGA bridge. > This property is optional if the FPGA Manager handles the bridges. > -- > 2.18.0 >