| Message ID | 20220407133658.15699-2-i.bornyakov@metrotek.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Microchip Polarfire FPGA manager | expand |
On Thu, Apr 07, 2022 at 04:36:56PM +0300, Ivan Bornyakov wrote: > It is not always whole FPGA image buffer meant to be written to the > device. Thanks for improving the fpga core. Please see my comments inline. Maybe more description about the issue, i.e. in which case we don't write the whole buffer, what's the problem in current implementation. > > Add bitstream_start and bitstream_size to the fpga_image_info struct and > adjust fpga_mgr_write() callers with respect to them. > > If initial_header_size is not known beforehand, pass whole buffer to low > level driver's write_init() so it could setup info->bitstream_start and > info->bitstream_size regardless. > > Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru> > --- > drivers/fpga/fpga-mgr.c | 48 +++++++++++++++++++++++++++++------ > include/linux/fpga/fpga-mgr.h | 5 ++++ > 2 files changed, 45 insertions(+), 8 deletions(-) > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > index d49a9ce34568..c64e60e23a71 100644 > --- a/drivers/fpga/fpga-mgr.c > +++ b/drivers/fpga/fpga-mgr.c > @@ -139,7 +139,8 @@ EXPORT_SYMBOL_GPL(fpga_image_info_free); > * 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 > * receive an FPGA image. The low level driver only gets to see the first > - * initial_header_size bytes in the buffer. > + * initial_header_size bytes in the buffer, if initial_header_size is set. > + * Otherwise, the whole buffer will be passed. > */ > static int fpga_mgr_write_init_buf(struct fpga_manager *mgr, > struct fpga_image_info *info, > @@ -148,12 +149,10 @@ static int fpga_mgr_write_init_buf(struct fpga_manager *mgr, > int ret; > > mgr->state = FPGA_MGR_STATE_WRITE_INIT; > - if (!mgr->mops->initial_header_size) > - ret = fpga_mgr_write_init(mgr, info, NULL, 0); > - else > - ret = fpga_mgr_write_init( > - mgr, info, buf, min(mgr->mops->initial_header_size, count)); > + if (mgr->mops->initial_header_size) > + count = min(mgr->mops->initial_header_size, count); > > + ret = fpga_mgr_write_init(mgr, info, buf, count); Here we pass the whole buffer for write_init(). Maybe it works for mapped buf, but still doesn't work for sg buf. It is also inefficient if we change to map and copy all sg buffers just for write_init(). We could discuss on the solution. My quick mind is we introduce an optional fpga_manager_ops.parse_header() callback, and a header_size (dynamic header size) field in fpga_image_info. FPGA core starts with mapping a buf of initial_header_size for parse_header(), let the drivers decide the dynamic header_size. The parse_header() could be called several times with updated dynamic header_size, if drivers doesn't get enough buffer for final decision and return -EAGAIN. Then write_init() be called with the final dynamic header size. For mapped buffer, just passing the whole buffer for write_init() is fine. > if (ret) { > dev_err(&mgr->dev, "Error preparing FPGA for writing\n"); > mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR; > @@ -235,13 +234,33 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, > if (mgr->mops->write_sg) { > ret = fpga_mgr_write_sg(mgr, sgt); > } else { > + size_t offset, count, length, bitstream_size; > struct sg_mapping_iter miter; > > + offset = info->bitstream_start; > + bitstream_size = info->bitstream_size; > + count = 0; > + > sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); > while (sg_miter_next(&miter)) { > - ret = fpga_mgr_write(mgr, miter.addr, miter.length); > - if (ret) > + if (offset >= miter.length) { > + offset -= miter.length; > + continue; > + } > + > + if (bitstream_size) > + length = min(miter.length - offset, > + bitstream_size - count); > + else > + length = miter.length - offset; > + > + count += length; > + > + ret = fpga_mgr_write(mgr, miter.addr + offset, length); > + if (ret || count == bitstream_size) > break; > + > + offset = 0; > } > sg_miter_stop(&miter); > } > @@ -265,6 +284,19 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, > if (ret) > return ret; > > + if (info->bitstream_start > count) { > + dev_err(&mgr->dev, > + "Bitstream start %zd outruns firmware image %zd\n", > + info->bitstream_start, count); > + return -EINVAL; > + } > + > + if (info->bitstream_size) > + count = min(info->bitstream_start + info->bitstream_size, count); > + > + buf += info->bitstream_start; > + count -= info->bitstream_start; > + > /* > * Write the FPGA image to the FPGA. > */ > diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h > index 0f9468771bb9..32464fd10cca 100644 > --- a/include/linux/fpga/fpga-mgr.h > +++ b/include/linux/fpga/fpga-mgr.h > @@ -85,6 +85,9 @@ enum fpga_mgr_states { > * @sgt: scatter/gather table containing FPGA image > * @buf: contiguous buffer containing FPGA image > * @count: size of buf > + * @bitstream_start: offset in image buffer where bitstream data starts > + * @bitstream_size: size of bitstream. > + * If 0, (count - bitstream_start) will be used. > * @region_id: id of target region > * @dev: device that owns this > * @overlay: Device Tree overlay > @@ -98,6 +101,8 @@ struct fpga_image_info { > struct sg_table *sgt; > const char *buf; > size_t count; > + size_t bitstream_start; How about we name it header_size? > + size_t bitstream_size; And how about data_size? Thanks, Yilun > int region_id; > struct device *dev; > #ifdef CONFIG_OF > -- > 2.25.1 >
On Sat, Apr 09, 2022 at 01:04:23PM +0800, Xu Yilun wrote: > On Thu, Apr 07, 2022 at 04:36:56PM +0300, Ivan Bornyakov wrote: > > > > ... snip ... > > > > @@ -148,12 +149,10 @@ static int fpga_mgr_write_init_buf(struct fpga_manager *mgr, > > int ret; > > > > mgr->state = FPGA_MGR_STATE_WRITE_INIT; > > - if (!mgr->mops->initial_header_size) > > - ret = fpga_mgr_write_init(mgr, info, NULL, 0); > > - else > > - ret = fpga_mgr_write_init( > > - mgr, info, buf, min(mgr->mops->initial_header_size, count)); > > + if (mgr->mops->initial_header_size) > > + count = min(mgr->mops->initial_header_size, count); > > > > + ret = fpga_mgr_write_init(mgr, info, buf, count); > > Here we pass the whole buffer for write_init(). Maybe it works for mapped buf, > but still doesn't work for sg buf. > > It is also inefficient if we change to map and copy all sg buffers just for > write_init(). > > We could discuss on the solution. > > My quick mind is we introduce an optional fpga_manager_ops.parse_header() > callback, and a header_size (dynamic header size) field in > fpga_image_info. FPGA core starts with mapping a buf of initial_header_size > for parse_header(), let the drivers decide the dynamic header_size. > > The parse_header() could be called several times with updated dynamic > header_size, if drivers doesn't get enough buffer for final decision and > return -EAGAIN. > > Then write_init() be called with the final dynamic header size. > > For mapped buffer, just passing the whole buffer for write_init() is > fine. > Ok, that's sounds reasonable. Should we pass PAGE_SIZE of a buffer to the parse_header() if initial_header_size is not set? The other option would be to make initial_header_size to be mandatory for usage of parse_header(). > > > > ... snip ... > > > > @@ -98,6 +101,8 @@ struct fpga_image_info { > > struct sg_table *sgt; > > const char *buf; > > size_t count; > > + size_t bitstream_start; > > How about we name it header_size? > > > + size_t bitstream_size; > > And how about data_size? > Sure, I don't mind.
On Sat, Apr 09, 2022 at 03:38:51PM +0300, Ivan Bornyakov wrote: > On Sat, Apr 09, 2022 at 01:04:23PM +0800, Xu Yilun wrote: > > On Thu, Apr 07, 2022 at 04:36:56PM +0300, Ivan Bornyakov wrote: > > > > > > ... snip ... > > > > > > @@ -148,12 +149,10 @@ static int fpga_mgr_write_init_buf(struct fpga_manager *mgr, > > > int ret; > > > > > > mgr->state = FPGA_MGR_STATE_WRITE_INIT; > > > - if (!mgr->mops->initial_header_size) > > > - ret = fpga_mgr_write_init(mgr, info, NULL, 0); > > > - else > > > - ret = fpga_mgr_write_init( > > > - mgr, info, buf, min(mgr->mops->initial_header_size, count)); > > > + if (mgr->mops->initial_header_size) > > > + count = min(mgr->mops->initial_header_size, count); > > > > > > + ret = fpga_mgr_write_init(mgr, info, buf, count); > > > > Here we pass the whole buffer for write_init(). Maybe it works for mapped buf, > > but still doesn't work for sg buf. > > > > It is also inefficient if we change to map and copy all sg buffers just for > > write_init(). > > > > We could discuss on the solution. > > > > My quick mind is we introduce an optional fpga_manager_ops.parse_header() > > callback, and a header_size (dynamic header size) field in > > fpga_image_info. FPGA core starts with mapping a buf of initial_header_size > > for parse_header(), let the drivers decide the dynamic header_size. > > > > The parse_header() could be called several times with updated dynamic > > header_size, if drivers doesn't get enough buffer for final decision and > > return -EAGAIN. > > > > Then write_init() be called with the final dynamic header size. > > > > For mapped buffer, just passing the whole buffer for write_init() is > > fine. > > > > Ok, that's sounds reasonable. Should we pass PAGE_SIZE of a buffer to > the parse_header() if initial_header_size is not set? The other option I think it is not necessary, no initial_header_size means no need to parse image header. > would be to make initial_header_size to be mandatory for usage of > parse_header(). That's a good idea. If parse_header() is provided, initial_header_size is mandatory. Thanks, Yilun > > > > > > > ... snip ... > > > > > > @@ -98,6 +101,8 @@ struct fpga_image_info { > > > struct sg_table *sgt; > > > const char *buf; > > > size_t count; > > > + size_t bitstream_start; > > > > How about we name it header_size? > > > > > + size_t bitstream_size; > > > > And how about data_size? > > > > Sure, I don't mind.
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index d49a9ce34568..c64e60e23a71 100644 --- a/drivers/fpga/fpga-mgr.c +++ b/drivers/fpga/fpga-mgr.c @@ -139,7 +139,8 @@ EXPORT_SYMBOL_GPL(fpga_image_info_free); * 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 * receive an FPGA image. The low level driver only gets to see the first - * initial_header_size bytes in the buffer. + * initial_header_size bytes in the buffer, if initial_header_size is set. + * Otherwise, the whole buffer will be passed. */ static int fpga_mgr_write_init_buf(struct fpga_manager *mgr, struct fpga_image_info *info, @@ -148,12 +149,10 @@ static int fpga_mgr_write_init_buf(struct fpga_manager *mgr, int ret; mgr->state = FPGA_MGR_STATE_WRITE_INIT; - if (!mgr->mops->initial_header_size) - ret = fpga_mgr_write_init(mgr, info, NULL, 0); - else - ret = fpga_mgr_write_init( - mgr, info, buf, min(mgr->mops->initial_header_size, count)); + if (mgr->mops->initial_header_size) + count = min(mgr->mops->initial_header_size, count); + ret = fpga_mgr_write_init(mgr, info, buf, count); if (ret) { dev_err(&mgr->dev, "Error preparing FPGA for writing\n"); mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR; @@ -235,13 +234,33 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, if (mgr->mops->write_sg) { ret = fpga_mgr_write_sg(mgr, sgt); } else { + size_t offset, count, length, bitstream_size; struct sg_mapping_iter miter; + offset = info->bitstream_start; + bitstream_size = info->bitstream_size; + count = 0; + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG); while (sg_miter_next(&miter)) { - ret = fpga_mgr_write(mgr, miter.addr, miter.length); - if (ret) + if (offset >= miter.length) { + offset -= miter.length; + continue; + } + + if (bitstream_size) + length = min(miter.length - offset, + bitstream_size - count); + else + length = miter.length - offset; + + count += length; + + ret = fpga_mgr_write(mgr, miter.addr + offset, length); + if (ret || count == bitstream_size) break; + + offset = 0; } sg_miter_stop(&miter); } @@ -265,6 +284,19 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, if (ret) return ret; + if (info->bitstream_start > count) { + dev_err(&mgr->dev, + "Bitstream start %zd outruns firmware image %zd\n", + info->bitstream_start, count); + return -EINVAL; + } + + if (info->bitstream_size) + count = min(info->bitstream_start + info->bitstream_size, count); + + buf += info->bitstream_start; + count -= info->bitstream_start; + /* * Write the FPGA image to the FPGA. */ diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h index 0f9468771bb9..32464fd10cca 100644 --- a/include/linux/fpga/fpga-mgr.h +++ b/include/linux/fpga/fpga-mgr.h @@ -85,6 +85,9 @@ enum fpga_mgr_states { * @sgt: scatter/gather table containing FPGA image * @buf: contiguous buffer containing FPGA image * @count: size of buf + * @bitstream_start: offset in image buffer where bitstream data starts + * @bitstream_size: size of bitstream. + * If 0, (count - bitstream_start) will be used. * @region_id: id of target region * @dev: device that owns this * @overlay: Device Tree overlay @@ -98,6 +101,8 @@ struct fpga_image_info { struct sg_table *sgt; const char *buf; size_t count; + size_t bitstream_start; + size_t bitstream_size; int region_id; struct device *dev; #ifdef CONFIG_OF
It is not always whole FPGA image buffer meant to be written to the device. Add bitstream_start and bitstream_size to the fpga_image_info struct and adjust fpga_mgr_write() callers with respect to them. If initial_header_size is not known beforehand, pass whole buffer to low level driver's write_init() so it could setup info->bitstream_start and info->bitstream_size regardless. Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru> --- drivers/fpga/fpga-mgr.c | 48 +++++++++++++++++++++++++++++------ include/linux/fpga/fpga-mgr.h | 5 ++++ 2 files changed, 45 insertions(+), 8 deletions(-)