[RFC 0/2]fpga: Add fpga configuration support from a pre-allocated dma-able buffer

[RFC 0/2]fpga: Add fpga configuration support from a pre-allocated dma-able buffer

[Nava kishore Manne]

Lots of embedded systems have memory constraints 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.

This implementation allows the lower-level drivers to request the FPGA
Configuration image be loaded from pre-allocated dma-able continuous
buffer and also it avoid needless memory pressure and delays due to
multiple copies.

Please take a look at the changes and let us know if any improvements
are required.

Nava kishore Manne (2):
  fpga: support loading from a pre-allocated buffer
  fpga: versal: Use the scatterlist interface

 drivers/fpga/fpga-mgr.c       | 113 ++++++++++++++++++++++++++++++++++
 drivers/fpga/versal-fpga.c    |  13 ++++
 include/linux/fpga/fpga-mgr.h |  10 +++
 3 files changed, 136 insertions(+)

-- 
2.25.1

[RFC 1/2] fpga: support loading from a pre-allocated buffer

[Nava kishore Manne]

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.kishore.manne@amd.com>
---
 drivers/fpga/fpga-mgr.c       | 113 ++++++++++++++++++++++++++++++++++
 include/linux/fpga/fpga-mgr.h |  10 +++
 2 files changed, 123 insertions(+)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 06651389c592..23d2b4d45827 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>
@@ -519,6 +521,39 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr,
 	return rc;
 }
 
+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
@@ -573,6 +608,8 @@ int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
 {
 	info->header_size = mgr->mops->initial_header_size;
 
+	if (mgr->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)
@@ -732,6 +769,64 @@ void fpga_mgr_put(struct fpga_manager *mgr)
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
+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;
+
+	mgr->flags = FPGA_MGR_CONFIG_DMA_BUF;
+
+	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_lock - Lock FPGA manager for exclusive use
  * @mgr:	fpga manager
@@ -815,10 +910,28 @@ fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *in
 	mgr->dev.of_node = parent->of_node;
 	mgr->dev.id = id;
 
+	/* Make device dma capable by inheriting from parent's */
+	set_dma_ops(&mgr->dev, get_dma_ops(parent));
+	ret = dma_coerce_mask_and_coherent(&mgr->dev, dma_get_mask(parent));
+	if (ret) {
+		dev_warn(parent,
+			 "Failed to set DMA mask %llx. Trying to continue... %x\n",
+			 dma_get_mask(parent), 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;
+	}
+
 	/*
 	 * Initialize framework state by requesting low level driver read state
 	 * from device.  FPGA may be in reset mode or may have been programmed
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 54f63459efd6..c5de19a9b4ed 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;
@@ -77,6 +78,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 an FPGA image
@@ -197,7 +199,10 @@ struct fpga_manager_ops {
  * struct fpga_manager - fpga manager structure
  * @name: name of low level fpga manager
  * @dev: fpga manager device
+ * @flags: flags determines the type of Bitstream
+ * @dmabuf: shared dma buffer
  * @ref_mutex: only allows one reference to fpga manager
+ * @miscdev: information about character device node
  * @state: state of fpga manager
  * @compat_id: FPGA manager id for compatibility check.
  * @mops: pointer to struct of fpga manager ops
@@ -206,7 +211,10 @@ struct fpga_manager_ops {
 struct fpga_manager {
 	const char *name;
 	struct device dev;
+	unsigned long flags;
+	struct dma_buf *dmabuf;
 	struct mutex ref_mutex;
+	struct miscdevice miscdev;
 	enum fpga_mgr_states state;
 	struct fpga_compat_id *compat_id;
 	const struct fpga_manager_ops *mops;
@@ -244,4 +252,6 @@ struct fpga_manager *
 devm_fpga_mgr_register(struct device *parent, 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 */
-- 
2.25.1

Re: [RFC 1/2] fpga: support loading from a pre-allocated buffer

[Marco Pagani]

On 2023-11-22 06:30, 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.

Sharing images/bitstreams using dma-buf to avoid multiple copies
make sense to me to have a fast path for partial reconfiguration.
However, implementing the userspace interface for importing the
buffer at the manager level seems questionable, considering that
the manager should be responsible only for writing images.

Wouldn't it be conceptually cleaner to implement the interface for
importing dma-buf as a separate layer on top of the manager? Such a
layer could then program the FPGA using the standard write_sg 
interface exported by the manager. In this way, each component would
have its own responsibility.

> 
> Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> ---
>  drivers/fpga/fpga-mgr.c       | 113 ++++++++++++++++++++++++++++++++++
>  include/linux/fpga/fpga-mgr.h |  10 +++
>  2 files changed, 123 insertions(+)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 06651389c592..23d2b4d45827 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>
> @@ -519,6 +521,39 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr,
>  	return rc;
>  }
>  
> +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
> @@ -573,6 +608,8 @@ int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
>  {
>  	info->header_size = mgr->mops->initial_header_size;
>  
> +	if (mgr->flags & FPGA_MGR_CONFIG_DMA_BUF)
> +		return fpga_dmabuf_load(mgr, info);

I'm not understanding the whole picture. After the dma-buf has been
imported from userspace, who is supposed to call fpga_mgr_load() or
fpga_region_program_fpga()? And who should load and export the dma-buf
containing the image in the first place?

I think it would be interesting to have a system that buffers a set of
alternative configurations for each (reconfigurable) region. Alternative
configurations could be represented and activated through a sysfs
interface. The user could request a specific configuration by writing in
the corresponding sysfs file, and the system would use the preloaded
image and optionally the overlay to configure the region. What do you
think?

> [...]

Thanks,
Marco

[RFC 2/2] fpga: versal: Use the scatterlist interface

[Nava kishore Manne]

Allows drivers to request the Configuration image
be loaded from dma-able continuous buffer to avoid
needless memory pressure and delays due to multiple
copies.

Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
---
 drivers/fpga/versal-fpga.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/fpga/versal-fpga.c b/drivers/fpga/versal-fpga.c
index 3710e8f01be2..bf4fb4426417 100644
--- a/drivers/fpga/versal-fpga.c
+++ b/drivers/fpga/versal-fpga.c
@@ -19,6 +19,18 @@ static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
 	return 0;
 }
 
+static int versal_fpga_ops_write_sg(struct fpga_manager *mgr,
+				    struct sg_table *sgt)
+{
+	dma_addr_t dma_addr;
+	int ret;
+
+	dma_addr = sg_dma_address(sgt->sgl);
+	ret = zynqmp_pm_load_pdi(PDI_SRC_DDR, dma_addr);
+
+	return ret;
+}
+
 static int versal_fpga_ops_write(struct fpga_manager *mgr,
 				 const char *buf, size_t size)
 {
@@ -40,6 +52,7 @@ static int versal_fpga_ops_write(struct fpga_manager *mgr,
 static const struct fpga_manager_ops versal_fpga_ops = {
 	.write_init = versal_fpga_ops_write_init,
 	.write = versal_fpga_ops_write,
+	.write_sg = versal_fpga_ops_write_sg,
 };
 
 static int versal_fpga_probe(struct platform_device *pdev)
-- 
2.25.1