Re: [PATCH] bcm2835_dma: Fix TD mode

[Philippe Mathieu-Daudé]

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é 
> <address@hidden> 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 <address@hidden>
> > > ---
> > >    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;