Re: [PATCH] bcm2835_dma: Fix TD mode

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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;
>>>
>>
>>
> 
> 
>