| Message ID | 20241122084152.1841419-5-naush@raspberrypi.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: bcm2835-unicam: Upstreaming various improvements | expand |
Hi Naush On Fri, Nov 22, 2024 at 08:41:51AM +0000, Naushir Patuck wrote: > The Unicam hardware has been observed to cause a buffer overrun when > using the dummy buffer as a circular buffer. The conditions that cause > the overrun are not fully known, but it seems to occur when the memory > bus is heavily loaded. > > To avoid the overrun, program the hardware with a buffer size of 0 when > using the dummy buffer. This will cause overrun into the allocated dummy > buffer, but avoid out of bounds writes. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > drivers/media/platform/broadcom/bcm2835-unicam.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c > index 550eb1b064f1..f10064107d54 100644 > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c > @@ -640,7 +640,14 @@ static inline void unicam_reg_write_field(struct unicam_device *unicam, u32 offs > static void unicam_wr_dma_addr(struct unicam_node *node, > struct unicam_buffer *buf) > { > - dma_addr_t endaddr = buf->dma_addr + buf->size; > + /* > + * Due to a HW bug causing buffer overruns in circular buffer mode under > + * certain (not yet fully known) conditions, the dummy buffer allocation > + * is set to a a single page size, but the hardware gets programmed with > + * a buffer size of 0. > + */ > + dma_addr_t endaddr = buf->dma_addr + > + (buf != &node->dummy_buf ? buf->size : 0); So the DMA engine doesn't actually write any data to dummy_buf anymore ? Does it still need to be allocated at all ? Or can we simply set the dma transfer size to 0 ? > > if (node->id == UNICAM_IMAGE_NODE) { > unicam_reg_write(node->dev, UNICAM_IBSA0, buf->dma_addr); > -- > 2.34.1 > >
Hi Jacopo, On Fri, 22 Nov 2024 at 11:20, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Fri, Nov 22, 2024 at 08:41:51AM +0000, Naushir Patuck wrote: > > The Unicam hardware has been observed to cause a buffer overrun when > > using the dummy buffer as a circular buffer. The conditions that cause > > the overrun are not fully known, but it seems to occur when the memory > > bus is heavily loaded. > > > > To avoid the overrun, program the hardware with a buffer size of 0 when > > using the dummy buffer. This will cause overrun into the allocated dummy > > buffer, but avoid out of bounds writes. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > drivers/media/platform/broadcom/bcm2835-unicam.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c > > index 550eb1b064f1..f10064107d54 100644 > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c > > @@ -640,7 +640,14 @@ static inline void unicam_reg_write_field(struct unicam_device *unicam, u32 offs > > static void unicam_wr_dma_addr(struct unicam_node *node, > > struct unicam_buffer *buf) > > { > > - dma_addr_t endaddr = buf->dma_addr + buf->size; > > + /* > > + * Due to a HW bug causing buffer overruns in circular buffer mode under > > + * certain (not yet fully known) conditions, the dummy buffer allocation > > + * is set to a a single page size, but the hardware gets programmed with > > + * a buffer size of 0. > > + */ > > + dma_addr_t endaddr = buf->dma_addr + > > + (buf != &node->dummy_buf ? buf->size : 0); > > So the DMA engine doesn't actually write any data to dummy_buf > anymore ? > > > Does it still need to be allocated at all ? Or can we simply set the > dma transfer size to 0 ? The DMA engine does still write to the buffer, so the allocation needs to occur. The zero size programmed into the register is a quirk of the HW itself, and is used to ensure the write wrap correctly in the buffer. Naush > > > > > if (node->id == UNICAM_IMAGE_NODE) { > > unicam_reg_write(node->dev, UNICAM_IBSA0, buf->dma_addr); > > -- > > 2.34.1 > > > >
Hi Naush On Fri, Nov 22, 2024 at 11:35:59AM +0000, Naushir Patuck wrote: > Hi Jacopo, Thanks for the explanation > > On Fri, 22 Nov 2024 at 11:20, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Naush > > > > On Fri, Nov 22, 2024 at 08:41:51AM +0000, Naushir Patuck wrote: > > > The Unicam hardware has been observed to cause a buffer overrun when > > > using the dummy buffer as a circular buffer. The conditions that cause > > > the overrun are not fully known, but it seems to occur when the memory > > > bus is heavily loaded. > > > > > > To avoid the overrun, program the hardware with a buffer size of 0 when > > > using the dummy buffer. This will cause overrun into the allocated dummy > > > buffer, but avoid out of bounds writes. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > > --- > > > drivers/media/platform/broadcom/bcm2835-unicam.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c > > > index 550eb1b064f1..f10064107d54 100644 > > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c > > > @@ -640,7 +640,14 @@ static inline void unicam_reg_write_field(struct unicam_device *unicam, u32 offs > > > static void unicam_wr_dma_addr(struct unicam_node *node, > > > struct unicam_buffer *buf) > > > { > > > - dma_addr_t endaddr = buf->dma_addr + buf->size; > > > + /* > > > + * Due to a HW bug causing buffer overruns in circular buffer mode under > > > + * certain (not yet fully known) conditions, the dummy buffer allocation > > > + * is set to a a single page size, but the hardware gets programmed with > > > + * a buffer size of 0. > > > + */ > > > + dma_addr_t endaddr = buf->dma_addr + > > > + (buf != &node->dummy_buf ? buf->size : 0); > > > > So the DMA engine doesn't actually write any data to dummy_buf > > anymore ? > > > > > > Does it still need to be allocated at all ? Or can we simply set the > > dma transfer size to 0 ? > > The DMA engine does still write to the buffer, so the allocation needs > to occur. The zero size programmed into the register is a quirk of the > HW itself, and is used to ensure the write wrap correctly in the > buffer. > > Naush > > > > > > > > > if (node->id == UNICAM_IMAGE_NODE) { > > > unicam_reg_write(node->dev, UNICAM_IBSA0, buf->dma_addr); > > > -- > > > 2.34.1 > > > > > > >
diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c index 550eb1b064f1..f10064107d54 100644 --- a/drivers/media/platform/broadcom/bcm2835-unicam.c +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c @@ -640,7 +640,14 @@ static inline void unicam_reg_write_field(struct unicam_device *unicam, u32 offs static void unicam_wr_dma_addr(struct unicam_node *node, struct unicam_buffer *buf) { - dma_addr_t endaddr = buf->dma_addr + buf->size; + /* + * Due to a HW bug causing buffer overruns in circular buffer mode under + * certain (not yet fully known) conditions, the dummy buffer allocation + * is set to a a single page size, but the hardware gets programmed with + * a buffer size of 0. + */ + dma_addr_t endaddr = buf->dma_addr + + (buf != &node->dummy_buf ? buf->size : 0); if (node->id == UNICAM_IMAGE_NODE) { unicam_reg_write(node->dev, UNICAM_IBSA0, buf->dma_addr);
The Unicam hardware has been observed to cause a buffer overrun when using the dummy buffer as a circular buffer. The conditions that cause the overrun are not fully known, but it seems to occur when the memory bus is heavily loaded. To avoid the overrun, program the hardware with a buffer size of 0 when using the dummy buffer. This will cause overrun into the allocated dummy buffer, but avoid out of bounds writes. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- drivers/media/platform/broadcom/bcm2835-unicam.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)