| Message ID | 5099495.CBsx362VbF@desktop2 |
|---|---|
| State | New |
| Headers | show |
| Series | bcm2835_dma: Fix TD mode | expand |
Hi Rene, On 1/24/20 6:55 PM, Rene Stange wrote: > TD (two dimensions) DMA mode did not work, because the xlen variable has > not been re-initialized before each additional ylen run through in > bcm2835_dma_update(). Furthermore ylen has to be increased by one after > reading it from the TXFR_LEN register, because a value of zero has to > result in one run through of the ylen loop. Both issues have been fixed. What were you running, how can we reproduce? > > Signed-off-by: Rene Stange <rsta2@o2online.de> > --- > hw/dma/bcm2835_dma.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/dma/bcm2835_dma.c b/hw/dma/bcm2835_dma.c > index 1e458d7fba..0881c9506e 100644 > --- a/hw/dma/bcm2835_dma.c > +++ b/hw/dma/bcm2835_dma.c > @@ -54,7 +54,7 @@ > static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) > { > BCM2835DMAChan *ch = &s->chan[c]; > - uint32_t data, xlen, ylen; > + uint32_t data, xlen, xlen_td, ylen; > int16_t dst_stride, src_stride; > > if (!(s->enable & (1 << c))) { > @@ -72,13 +72,13 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) > > if (ch->ti & BCM2708_DMA_TDMODE) { > /* 2D transfer mode */ > - ylen = (ch->txfr_len >> 16) & 0x3fff; > - xlen = ch->txfr_len & 0xffff; > + ylen = ((ch->txfr_len >> 16) & 0x3fff) + 1; > + xlen_td = xlen = ch->txfr_len & 0xffff; > dst_stride = ch->stride >> 16; > src_stride = ch->stride & 0xffff; > } else { > ylen = 1; > - xlen = ch->txfr_len; > + xlen_td = xlen = ch->txfr_len; > dst_stride = 0; > src_stride = 0; > } > @@ -117,6 +117,7 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) > if (--ylen != 0) { > ch->source_ad += src_stride; > ch->dest_ad += dst_stride; > + xlen = xlen_td; > } > } > ch->cs |= BCM2708_DMA_END; >
Hi Philippe, I'm running an example program for my Circle bare metal framework for the Raspberry Pi using the LittlevGL graphics library. It uses the TD DMA mode to transfer pixel data to the screen buffer (10 lines at a time). Without the given patch applied to QEMU only the first pixel line of each transfer is shown in TigerVNC viewer, after applying it, a solid image is shown. You can reproduce the problem on a 64-bit Linux machine as follows. The "sed" command modifies the example program, so that it doesn't try to access the (not available) USB HCI controller of the Raspberry Pi 3. Regards, Rene cd mkdir dma-test cd dma-test/ wget https://developer.arm.com/-/media/Files/downloads/gnu-a/8.3-2019.03/binrel/gcc-arm-8.3-2019.03-x86_64-aarch64-elf.tar.xz tar xJf gcc-arm-8.3-2019.03-x86_64-aarch64-elf.tar.xz git clone https://github.com/rsta2/circle.git cd circle git submodule update --init echo "AARCH = 64" > Config.mk echo "RASPPI = 3" >> Config.mk echo "PREFIX64 = ~/dma-test/gcc-arm-8.3-2019.03-x86_64-aarch64-elf/bin/aarch64-elf-" >> Config.mk ./makeall cd addon/littlevgl/ make cd sample/ sed -i -e "s/bOK = m_USBHCI/\/\/bOK = m_USBHCI/" kernel.cpp make qemu-system-aarch64 -M raspi3 -kernel kernel8.img On Monday, 27 January 2020, 09:29:59 CET, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Hi Rene, > > On 1/24/20 6:55 PM, Rene Stange wrote: > > TD (two dimensions) DMA mode did not work, because the xlen variable has > > not been re-initialized before each additional ylen run through in > > bcm2835_dma_update(). Furthermore ylen has to be increased by one after > > reading it from the TXFR_LEN register, because a value of zero has to > > result in one run through of the ylen loop. Both issues have been fixed. > > What were you running, how can we reproduce? > > > > > Signed-off-by: Rene Stange <rsta2@o2online.de> > > --- > > hw/dma/bcm2835_dma.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/hw/dma/bcm2835_dma.c b/hw/dma/bcm2835_dma.c > > index 1e458d7fba..0881c9506e 100644 > > --- a/hw/dma/bcm2835_dma.c > > +++ b/hw/dma/bcm2835_dma.c > > @@ -54,7 +54,7 @@ > > static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) > > { > > BCM2835DMAChan *ch = &s->chan[c]; > > - uint32_t data, xlen, ylen; > > + uint32_t data, xlen, xlen_td, ylen; > > int16_t dst_stride, src_stride; > > > > if (!(s->enable & (1 << c))) { > > @@ -72,13 +72,13 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) > > > > if (ch->ti & BCM2708_DMA_TDMODE) { > > /* 2D transfer mode */ > > - ylen = (ch->txfr_len >> 16) & 0x3fff; > > - xlen = ch->txfr_len & 0xffff; > > + ylen = ((ch->txfr_len >> 16) & 0x3fff) + 1; > > + xlen_td = xlen = ch->txfr_len & 0xffff; > > dst_stride = ch->stride >> 16; > > src_stride = ch->stride & 0xffff; > > } else { > > ylen = 1; > > - xlen = ch->txfr_len; > > + xlen_td = xlen = ch->txfr_len; > > dst_stride = 0; > > src_stride = 0; > > } > > @@ -117,6 +117,7 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) > > if (--ylen != 0) { > > ch->source_ad += src_stride; > > ch->dest_ad += dst_stride; > > + xlen = xlen_td; > > } > > } > > ch->cs |= BCM2708_DMA_END; > > > >
Hi Rene, On 1/27/20 12:20 PM, Rene Stange wrote: > Hi Philippe, > > I'm running an example program for my Circle bare metal framework for the > Raspberry Pi using the LittlevGL graphics library. It uses the TD DMA mode to > transfer pixel data to the screen buffer (10 lines at a time). Without the > given patch applied to QEMU only the first pixel line of each transfer is > shown in TigerVNC viewer, after applying it, a solid image is shown. > > You can reproduce the problem on a 64-bit Linux machine as follows. The "sed" > command modifies the example program, so that it doesn't try to access the > (not available) USB HCI controller of the Raspberry Pi 3. > > Regards, > > Rene > > > cd > mkdir dma-test > cd dma-test/ > wget https://developer.arm.com/-/media/Files/downloads/gnu-a/8.3-2019.03/binrel/gcc-arm-8.3-2019.03-x86_64-aarch64-elf.tar.xz > tar xJf gcc-arm-8.3-2019.03-x86_64-aarch64-elf.tar.xz > git clone https://github.com/rsta2/circle.git > cd circle > git submodule update --init > echo "AARCH = 64" > Config.mk > echo "RASPPI = 3" >> Config.mk > echo "PREFIX64 = ~/dma-test/gcc-arm-8.3-2019.03-x86_64-aarch64-elf/bin/aarch64-elf-" >> Config.mk > ./makeall > cd addon/littlevgl/ > make > cd sample/ > sed -i -e "s/bOK = m_USBHCI/\/\/bOK = m_USBHCI/" kernel.cpp > make > qemu-system-aarch64 -M raspi3 -kernel kernel8.img > > > On Monday, 27 January 2020, 09:29:59 CET, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> Hi Rene, >> >> On 1/24/20 6:55 PM, Rene Stange wrote: >>> TD (two dimensions) DMA mode did not work, because the xlen variable has >>> not been re-initialized before each additional ylen run through in >>> bcm2835_dma_update(). Furthermore ylen has to be increased by one after >>> reading it from the TXFR_LEN register, because a value of zero has to >>> result in one run through of the ylen loop. Both issues have been fixed. So we have 2 fixes in 1 patch. I'd rather see 2 different commits: 1: Fix the ylen loop -- >8 -- --- a/hw/dma/bcm2835_dma.c +++ b/hw/dma/bcm2835_dma.c @@ -70,14 +70,14 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) ch->stride = ldl_le_phys(&s->dma_as, ch->conblk_ad + 16); ch->nextconbk = ldl_le_phys(&s->dma_as, ch->conblk_ad + 20); + ylen = 1; if (ch->ti & BCM2708_DMA_TDMODE) { /* 2D transfer mode */ - ylen = (ch->txfr_len >> 16) & 0x3fff; + ylen += (ch->txfr_len >> 16) & 0x3fff; xlen = ch->txfr_len & 0xffff; dst_stride = ch->stride >> 16; src_stride = ch->stride & 0xffff; } else { - ylen = 1; xlen = ch->txfr_len; dst_stride = 0; src_stride = 0; --- 2: re-initialized xlen: -- >8 -- --- a/hw/dma/bcm2835_dma.c +++ b/hw/dma/bcm2835_dma.c @@ -54,7 +54,7 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) { BCM2835DMAChan *ch = &s->chan[c]; - uint32_t data, xlen, ylen; + uint32_t data, xlen, xlen_td, ylen; int16_t dst_stride, src_stride; if (!(s->enable & (1 << c))) { @@ -82,6 +82,7 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) dst_stride = 0; src_stride = 0; } + xlen_td = xlen; while (ylen != 0) { /* Normal transfer mode */ @@ -117,6 +118,7 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) if (--ylen != 0) { ch->source_ad += src_stride; ch->dest_ad += dst_stride; + xlen = xlen_td; } } ch->cs |= BCM2708_DMA_END; --- What do you think? If this sounds good to you, do you mind sending a v2 with the 2 patches? Thanks, Phil. >> >> What were you running, how can we reproduce? >> >>> >>> Signed-off-by: Rene Stange <rsta2@o2online.de> >>> --- >>> hw/dma/bcm2835_dma.c | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/dma/bcm2835_dma.c b/hw/dma/bcm2835_dma.c >>> index 1e458d7fba..0881c9506e 100644 >>> --- a/hw/dma/bcm2835_dma.c >>> +++ b/hw/dma/bcm2835_dma.c >>> @@ -54,7 +54,7 @@ >>> static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) >>> { >>> BCM2835DMAChan *ch = &s->chan[c]; >>> - uint32_t data, xlen, ylen; >>> + uint32_t data, xlen, xlen_td, ylen; >>> int16_t dst_stride, src_stride; >>> >>> if (!(s->enable & (1 << c))) { >>> @@ -72,13 +72,13 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) >>> >>> if (ch->ti & BCM2708_DMA_TDMODE) { >>> /* 2D transfer mode */ >>> - ylen = (ch->txfr_len >> 16) & 0x3fff; >>> - xlen = ch->txfr_len & 0xffff; >>> + ylen = ((ch->txfr_len >> 16) & 0x3fff) + 1; >>> + xlen_td = xlen = ch->txfr_len & 0xffff; >>> dst_stride = ch->stride >> 16; >>> src_stride = ch->stride & 0xffff; >>> } else { >>> ylen = 1; >>> - xlen = ch->txfr_len; >>> + xlen_td = xlen = ch->txfr_len; >>> dst_stride = 0; >>> src_stride = 0; >>> } >>> @@ -117,6 +117,7 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) >>> if (--ylen != 0) { >>> ch->source_ad += src_stride; >>> ch->dest_ad += dst_stride; >>> + xlen = xlen_td; >>> } >>> } >>> ch->cs |= BCM2708_DMA_END; >>> >> >> > > >
diff --git a/hw/dma/bcm2835_dma.c b/hw/dma/bcm2835_dma.c index 1e458d7fba..0881c9506e 100644 --- a/hw/dma/bcm2835_dma.c +++ b/hw/dma/bcm2835_dma.c @@ -54,7 +54,7 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) { BCM2835DMAChan *ch = &s->chan[c]; - uint32_t data, xlen, ylen; + uint32_t data, xlen, xlen_td, ylen; int16_t dst_stride, src_stride; if (!(s->enable & (1 << c))) { @@ -72,13 +72,13 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) if (ch->ti & BCM2708_DMA_TDMODE) { /* 2D transfer mode */ - ylen = (ch->txfr_len >> 16) & 0x3fff; - xlen = ch->txfr_len & 0xffff; + ylen = ((ch->txfr_len >> 16) & 0x3fff) + 1; + xlen_td = xlen = ch->txfr_len & 0xffff; dst_stride = ch->stride >> 16; src_stride = ch->stride & 0xffff; } else { ylen = 1; - xlen = ch->txfr_len; + xlen_td = xlen = ch->txfr_len; dst_stride = 0; src_stride = 0; } @@ -117,6 +117,7 @@ static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c) if (--ylen != 0) { ch->source_ad += src_stride; ch->dest_ad += dst_stride; + xlen = xlen_td; } } ch->cs |= BCM2708_DMA_END;
TD (two dimensions) DMA mode did not work, because the xlen variable has not been re-initialized before each additional ylen run through in bcm2835_dma_update(). Furthermore ylen has to be increased by one after reading it from the TXFR_LEN register, because a value of zero has to result in one run through of the ylen loop. Both issues have been fixed. Signed-off-by: Rene Stange <rsta2@o2online.de> --- hw/dma/bcm2835_dma.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)