Hi Yilun,

-----Original Message-----
From: Xu Yilun <redacted>
Sent: Wednesday, November 27, 2024 7:20 AM
To: Manne, Nava kishore <redacted>
Cc: git (AMD-Xilinx) <redacted>; mdf@kernel.org; hao.wu@intel.com;
yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; saravanak@google.com;
linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org;
devicetree@vger.kernel.org
Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime
FPGA programming

quoted

quoted

quoted

+ * 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.

quoted

quoted

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.

static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
	struct fpga_region *region =  (struct fpga_region *)(file->private_data);
	struct fpga_region_config_info config_info;
	void __user *argp = (void __user *)arg;
	struct device *dev = &region->dev;
	struct fpga_image_info *info;
	int err;

	switch (cmd) {
	case FPGA_REGION_IOCTL_LOAD:
		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
		return -EFAULT;

		info = fpga_image_info_alloc(dev);
		if (!info)
			return ERR_PTR(-ENOMEM);

		/* 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.
		 */
		err = region->region_ops->region_preconfig(region, &config_info, info);
		if (err)
			return err;

		/* The common API fpga_region_program_fpga() is used to program
		 * the image to hardware.
		 */
		region->info = info;
		err = fpga_region_program_fpga(region);
		if (err) {
			fpga_image_info_free(info);
			region->info = NULL;
		}

		/* A vendor-specific callback is used for real enumeration, enabling
		 * hardware specific customization.
		 */
		err = region->region_ops->region_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);
		if (err)
			return err;

		fpga_image_info_free(region->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;
}

Regards,
Navakishore.