1. Patchew
  2. QEMU
  3. View series

[PATCH] bcm2835_dma: Fix TD mode

Rene Stange posted 1 patch 4 years, 3 months ago
Test FreeBSD passed
Test docker-mingw@fedora passed
Test checkpatch passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/5099495.CBsx362VbF@desktop2
Maintainers: Andrew Baumann <Andrew.Baumann@microsoft.com>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, Peter Maydell <peter.maydell@linaro.org>
There is a newer version of this series
hw/dma/bcm2835_dma.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
[PATCH] bcm2835_dma: Fix TD mode
Posted by Rene Stange 4 years, 3 months ago 
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(-)

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;
-- 
2.16.4
Re: [PATCH] bcm2835_dma: Fix TD mode
Posted by Philippe Mathieu-Daudé 4 years, 3 months ago 
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;
>
Re: [PATCH] bcm2835_dma: Fix TD mode
Posted by Rene Stange 4 years, 3 months ago 
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;
> > 
> 
>
Re: [PATCH] bcm2835_dma: Fix TD mode
Posted by Philippe Mathieu-Daudé 4 years, 3 months ago 
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;
>>>
>>
>>
> 
> 
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.