On Wed, Dec 04, 2024 at 06:40:18AM +0000, Manne, Nava kishore wrote:
> 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.
I prefer more to configfs cause it provides standard FS way to create
the fpga_image_info object, e.g. which attributes are visible for
OF/non-OF region, which attributes come from image blob and can only be
RO, etc.
Of couse ioctl() could achieve the same goal but would add much more
specific rules (maybe flags/types) for user to follow.
Thanks,
Yilun
>
> 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 = ®ion->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.
