Re: [RFC 2/2] fpga manager: Add fpga_mgr_firmware_stream API

["Li, Yi" <yi1.li@linux.intel.com>]

Thanks Alan for the comments.


On 3/13/2017 1:00 PM, Alan Tull wrote:
> On Thu, Mar 9, 2017 at 6:18 PM,  <yi1.li@linux.intel.com> wrote:
>
> Hi Yi,
>
> Thanks for your RFC.  I believe this functionality is badly needed.
>
> I had a few comments about the chunk size and some nits about comments below...
>
>> From: Yi Li <yi1.li@linux.intel.com>
>>
>> Add fpga_mgr_firmware_stream API, which can load and program firmware
>>
>> in trucks to FPGA instead of the whole file.
>>
>> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
>> ---
>>   drivers/fpga/fpga-mgr.c       | 77 +++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/fpga/fpga-mgr.h |  4 +++
>>   2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index 3206a53..bb55b80 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -27,10 +27,15 @@
>>   #include <linux/slab.h>
>>   #include <linux/scatterlist.h>
>>   #include <linux/highmem.h>
>> +#include <linux/sizes.h>
>>
>>   static DEFINE_IDA(fpga_mgr_ida);
>>   static struct class *fpga_mgr_class;
>>
>> +static int streamsize = SZ_4K;
>> +module_param(streamsize, int, 0664);
>> +MODULE_PARM_DESC(streamsize, "buffer size for firmware streaming");
> I think we could fix the chunk size at PAGE_SIZE unless someone can
> state a  reason for needing something different.  Below I have one
> reason why we will sometimes need > PAGE_SIZE.
>
>> +
>>   /*
>>    * Call the low level driver's write_init function.  This will do the
>>    * device-specific things to get the FPGA into the state where it is ready to
>> @@ -309,6 +314,78 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>>   }
>>   EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
>>
>> +/**
>> + * fpga_mgr_firmware_load - request firmware and load to fpga
>> + * @mgr:       fpga manager
>> + * @info:      fpga image specific information
>> + * @image_name:        name of image file on the firmware search path
>> + *
>> + * Request an FPGA image using the firmware class, then write out to the FPGA.
>> + * Update the state before each step to provide info on what step failed if
>> + * there is a failure.  This code assumes the caller got the mgr pointer
>> + * from of_fpga_mgr_get() or fpga_mgr_get() and checked that it is not an error
>> + * code.
>> + *
>> + * Return: 0 on success, negative error code otherwise.
>> + */
>> +int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
>> +                          struct fpga_image_info *info,
>> +                          const char *image_name)
> No need for both fpga_mgr_firmware_load and fpga_mgr_firmware_stream.
> Just replace the old fpga_mgr_firmware_load with this function.

Sure, will do.

>
>> +{
>> +       struct device *dev = &mgr->dev;
>> +       const struct firmware *fw = NULL;
>> +       int ret;
>> +       size_t size = INT_MAX, offset = 0;
>> +       bool start_flag = 1;
>> +
>> +       dev_info(dev, "writing %s to %s with buffer size %d\n",
>> +                       image_name, mgr->name, streamsize);
>> +
>> +       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
>> +
>> +       while (size > 0) {
>> +               ret = stream_firmware(&fw, image_name, dev, offset, streamsize);
>> +               if (ret) {
>> +                       dev_err(dev, "Error reading firmware %d\n", ret);
>> +                       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
>> +                       break;
>> +               }
>> +
>> +               /*
>> +                * init.
>> +                */
> We don't really need this comment.
>
>> +               if (start_flag) {
>> +                       ret = fpga_mgr_write_init_buf(mgr, info, fw->data,
>> +                                                     fw->size);
> When the fpga manager is registered, one of the ops is
> initial_header_size, which specifies how much buffer the write_init
> needs.  So this call could fail if initial_header_size < streamsize.
>
> I suggest that you make the streamsize equal to the smallest multiple
> of PAGE_SIZE that is >= initial_header_size.

Agree, we can fix the streamsize to something bigger than 
initial_header_size for the bitstream file. Another reason to have the 
variable streamsize is for the performance, although with Altera Cyclone 
V FPGA CvP I did not see much difference when increasing the buffer 
size, since the bottleneck is with programming through PCIe and the time 
spent with firmware file reading is negligible.

>
>> +                       start_flag = 0;
>> +                       if (ret)
>> +                               break;
>> +               }
>> +
>> +               /*
>> +                * Write the FPGA image to the FPGA.
>> +                */
> Or this comment.
>
>> +               mgr->state = FPGA_MGR_STATE_WRITE;
>> +               ret = mgr->mops->write(mgr, fw->data, fw->size);
>> +               if (ret) {
>> +                       dev_err(dev, "Error while writing image data to FPGA\n");
>> +                       mgr->state = FPGA_MGR_STATE_WRITE_ERR;
>> +                       break;
>> +               }
>> +
>> +               offset += fw->size;
>> +               size -= fw->size;
>> +               if (fw->size < streamsize)
>> +                       break;
>> +       }
>> +
>> +       ret = fpga_mgr_write_complete(mgr, info);
>> +
>> +       release_firmware(fw);
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);
>> +
>>   int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
>>   {
>>          if (info->firmware_name)
>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>> index 0f5072c..a25362e 100644
>> --- a/include/linux/fpga/fpga-mgr.h
>> +++ b/include/linux/fpga/fpga-mgr.h
>> @@ -151,6 +151,10 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>>                             struct fpga_image_info *info,
>>                             const char *image_name);
>>
>> +int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
>> +                          struct fpga_image_info *info,
>> +                          const char *image_name);
> Don't need both fpga_mgr_firmware_load and fpga_mgr_firmware_stream,
> so no need to change the header.
>
>> +
>>   int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
>>
>>   int fpga_mgr_lock(struct fpga_manager *mgr);
>> --
>> 2.7.4
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html