RE: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

["Manne, Nava kishore" <nava.kishore.manne@xxxxxxx>]

Hi Yilun,

> -----Original Message-----
> From: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> Sent: Wednesday, November 27, 2024 7:20 AM
> To: Manne, Nava kishore <nava.kishore.manne@xxxxxxx>
> Cc: git (AMD-Xilinx) <git@xxxxxxx>; mdf@xxxxxxxxxx; hao.wu@xxxxxxxxx;
> yilun.xu@xxxxxxxxx; trix@xxxxxxxxxx; robh@xxxxxxxxxx; saravanak@xxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-fpga@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime
> FPGA programming
> 
> > > > + * 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
> 
> 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.