| Message ID | 20210402090933.32276-3-nava.manne@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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 */ >
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 + * @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 */
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(-)