| Message ID | 20220518064844.2694651-1-tien.sung.ang@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fpga: altera-cvp: Truncated bitstream error support | expand |
On 5/17/22 11:48 PM, tien.sung.ang@intel.com wrote: > From: Ang Tien Sung <tien.sung.ang@intel.com> > > To support the error handling of a truncated bitstream sent. > The current AIB CvP firmware is not capable of handling a > data stream smaller than 4096bytes. The firmware's limitation > causes a hung-up as it's DMA engine waits forever for the > completion of the instructed 4096bytes. > To resolve this design limitation, both firmware and CvP > driver made several changes. At the CvP driver, we just > have to ensure that anything lesser than 4096bytes are > padded with extra bytes. The CvP will then, initiate the > tear-down by clearing the START_XFER and CVP_CONFIG bits. > We should also check for CVP_ERROR during the CvP completion. > A send_buf which is always 4096bytes is used to copy the > data during every transaction. > > Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com> > --- > drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c > index 4ffb9da537d8..80edcfb5e5fc 100644 > --- a/drivers/fpga/altera-cvp.c > +++ b/drivers/fpga/altera-cvp.c > @@ -81,6 +81,7 @@ struct altera_cvp_conf { > u8 numclks; > u32 sent_packets; > u32 vsec_offset; > + u8 *send_buf; Why is it necessary to hold the send_buf in this structure ? If it is used only in *_write, could it alloc/freed there ? Because the write happens rarely, my preference is to alloc/free in *_write(). > const struct cvp_priv *priv; > }; > > @@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, > } > > len = min(conf->priv->block_size, remaining); > - altera_cvp_send_block(conf, data, len); > + /* Copy the requested host data into the transmit buffer */ > + > + memcpy(conf->send_buf, data, len); Is a memset needed for a short buffer ? > + altera_cvp_send_block(conf, (const u32 *)conf->send_buf, > + conf->priv->block_size); > data += len / sizeof(u32); > done += len; > remaining -= len; > @@ -492,10 +497,13 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr, > if (ret) > return ret; > > - /* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */ > - altera_read_config_dword(conf, VSE_UNCOR_ERR_STATUS, &val); > - if (val & VSE_UNCOR_ERR_CVP_CFG_ERR) { > - dev_err(&mgr->dev, "detected CVP_CONFIG_ERROR_LATCHED!\n"); > + /* > + * STEP 16 - If bitstream error (truncated/miss-matched), > + * we shall exit here. > + */ > + ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val); Should this be STEP 17 ? the old 16 checked something else. Tom > + if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) { > + dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n"); > return -EPROTO; > } > > @@ -661,6 +669,12 @@ static int altera_cvp_probe(struct pci_dev *pdev, > > pci_set_drvdata(pdev, mgr); > > + /* Allocate the 4096 block size transmit buffer */ > + conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL); > + if (!conf->send_buf) { > + ret = -ENOMEM; > + goto err_unmap; > + } > return 0; > > err_unmap:
Le 18/05/2022 à 08:48, tien.sung.ang@intel.com a écrit : > From: Ang Tien Sung <tien.sung.ang@intel.com> > > To support the error handling of a truncated bitstream sent. > The current AIB CvP firmware is not capable of handling a > data stream smaller than 4096bytes. The firmware's limitation > causes a hung-up as it's DMA engine waits forever for the > completion of the instructed 4096bytes. > To resolve this design limitation, both firmware and CvP > driver made several changes. At the CvP driver, we just > have to ensure that anything lesser than 4096bytes are > padded with extra bytes. The CvP will then, initiate the > tear-down by clearing the START_XFER and CVP_CONFIG bits. > We should also check for CVP_ERROR during the CvP completion. > A send_buf which is always 4096bytes is used to copy the > data during every transaction. > > Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com> > --- > drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c > index 4ffb9da537d8..80edcfb5e5fc 100644 > --- a/drivers/fpga/altera-cvp.c > +++ b/drivers/fpga/altera-cvp.c > @@ -81,6 +81,7 @@ struct altera_cvp_conf { > u8 numclks; > u32 sent_packets; > u32 vsec_offset; > + u8 *send_buf; > const struct cvp_priv *priv; > }; > > @@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, > } > > len = min(conf->priv->block_size, remaining); > - altera_cvp_send_block(conf, data, len); > + /* Copy the requested host data into the transmit buffer */ > + > + memcpy(conf->send_buf, data, len); > + altera_cvp_send_block(conf, (const u32 *)conf->send_buf, > + conf->priv->block_size); Nitpicking: this should be aligned with 'conf'. CJ > data += len / sizeof(u32); > done += len; > remaining -= len; [...]
Thanks for pointing that out. It is always good to get all the alignments right. I will add this to the next revision of the patch.
The send_buf is always used throughout the life-span of the CvP driver. Hence, we thought it would be wise to just pre-allocate it one time at the start of the probe/init. It is also fine if we do it in the altera_cvp_write. The only issue we see in this is that, a minor hit on the performance as you need to then, allocate the buffer on every new CvP FPGA configuration write. As for STEP 16, the previous implementation checks the Error latch bit which stores the previous transaction's result. If an error occurs prior to this, the driver would throw an error which is not right. The correct step is to just check for the current CvP error status from the register. Hope that is fine with you. Thanks
On Thu, May 19, 2022 at 12:34:12PM +0800, tien.sung.ang@intel.com wrote: > The send_buf is always used throughout the life-span of the CvP driver. > Hence, we thought it would be wise to just pre-allocate it one time > at the start of the probe/init. > It is also fine if we do it in the altera_cvp_write. The only issue we > see in this is that, a minor hit on the performance as you need to > then, allocate the buffer on every new CvP FPGA configuration write. > > As for STEP 16, the previous implementation checks the Error latch bit > which stores the previous transaction's result. If an error occurs > prior to this, the driver would throw an error which is not right. > The correct step is to just check for the current CvP error status > from the register. > Hope that is fine with you. Thanks Please comment inline in the mail, like others do. Thanks, Yilun
On Wed, May 18, 2022 at 01:08:31PM -0700, Tom Rix wrote: > > On 5/17/22 11:48 PM, tien.sung.ang@intel.com wrote: > > From: Ang Tien Sung <tien.sung.ang@intel.com> > > > > To support the error handling of a truncated bitstream sent. > > The current AIB CvP firmware is not capable of handling a > > data stream smaller than 4096bytes. The firmware's limitation > > causes a hung-up as it's DMA engine waits forever for the > > completion of the instructed 4096bytes. > > To resolve this design limitation, both firmware and CvP > > driver made several changes. At the CvP driver, we just > > have to ensure that anything lesser than 4096bytes are > > padded with extra bytes. The CvP will then, initiate the > > tear-down by clearing the START_XFER and CVP_CONFIG bits. > > We should also check for CVP_ERROR during the CvP completion. > > A send_buf which is always 4096bytes is used to copy the > > data during every transaction. > > > > Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com> > > --- > > drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c > > index 4ffb9da537d8..80edcfb5e5fc 100644 > > --- a/drivers/fpga/altera-cvp.c > > +++ b/drivers/fpga/altera-cvp.c > > @@ -81,6 +81,7 @@ struct altera_cvp_conf { > > u8 numclks; > > u32 sent_packets; > > u32 vsec_offset; > > + u8 *send_buf; > > Why is it necessary to hold the send_buf in this structure ? > > If it is used only in *_write, could it alloc/freed there ? > > Because the write happens rarely, my preference is to alloc/free in > *_write(). Is it better alloc in write_init()? Thanks, Yilun > > > const struct cvp_priv *priv; > > }; > > @@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, > > } > > len = min(conf->priv->block_size, remaining); > > - altera_cvp_send_block(conf, data, len); > > + /* Copy the requested host data into the transmit buffer */ > > + > > + memcpy(conf->send_buf, data, len); > Is a memset needed for a short buffer ? > > + altera_cvp_send_block(conf, (const u32 *)conf->send_buf, > > + conf->priv->block_size); > > data += len / sizeof(u32); > > done += len; > > remaining -= len; > > @@ -492,10 +497,13 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr, > > if (ret) > > return ret; > > - /* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */ > > - altera_read_config_dword(conf, VSE_UNCOR_ERR_STATUS, &val); > > - if (val & VSE_UNCOR_ERR_CVP_CFG_ERR) { > > - dev_err(&mgr->dev, "detected CVP_CONFIG_ERROR_LATCHED!\n"); > > + /* > > + * STEP 16 - If bitstream error (truncated/miss-matched), > > + * we shall exit here. > > + */ > > + ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val); > Should this be STEP 17 ? the old 16 checked something else. > > Tom > > > + if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) { > > + dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n"); > > return -EPROTO; > > } > > @@ -661,6 +669,12 @@ static int altera_cvp_probe(struct pci_dev *pdev, > > pci_set_drvdata(pdev, mgr); > > + /* Allocate the 4096 block size transmit buffer */ > > + conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL); > > + if (!conf->send_buf) { > > + ret = -ENOMEM; > > + goto err_unmap; > > + } > > return 0; > > err_unmap:
On Thu, May 19, 2022 at 12:21:35PM +0800, tien.sung.ang@intel.com wrote: > Thanks for pointing that out. It is always good to get all the alignments right. Please run checkpatch --strict before sending, it helps find out most of alignment issues. Thanks, Yilun > I will add this to the next revision of the patch.
diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c index 4ffb9da537d8..80edcfb5e5fc 100644 --- a/drivers/fpga/altera-cvp.c +++ b/drivers/fpga/altera-cvp.c @@ -81,6 +81,7 @@ struct altera_cvp_conf { u8 numclks; u32 sent_packets; u32 vsec_offset; + u8 *send_buf; const struct cvp_priv *priv; }; @@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, } len = min(conf->priv->block_size, remaining); - altera_cvp_send_block(conf, data, len); + /* Copy the requested host data into the transmit buffer */ + + memcpy(conf->send_buf, data, len); + altera_cvp_send_block(conf, (const u32 *)conf->send_buf, + conf->priv->block_size); data += len / sizeof(u32); done += len; remaining -= len; @@ -492,10 +497,13 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr, if (ret) return ret; - /* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */ - altera_read_config_dword(conf, VSE_UNCOR_ERR_STATUS, &val); - if (val & VSE_UNCOR_ERR_CVP_CFG_ERR) { - dev_err(&mgr->dev, "detected CVP_CONFIG_ERROR_LATCHED!\n"); + /* + * STEP 16 - If bitstream error (truncated/miss-matched), + * we shall exit here. + */ + ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val); + if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) { + dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n"); return -EPROTO; } @@ -661,6 +669,12 @@ static int altera_cvp_probe(struct pci_dev *pdev, pci_set_drvdata(pdev, mgr); + /* Allocate the 4096 block size transmit buffer */ + conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL); + if (!conf->send_buf) { + ret = -ENOMEM; + goto err_unmap; + } return 0; err_unmap: