[1/2,v5-test] add function DMA_set_return and DMA_set_channel_async in dma.c

To avoid re-enter function fdctrl_transfer_handler, according paolo's suggestion, the patch is used to test.

Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
---
 hw/dma.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
 hw/isa.h |    2 ++
 2 files changed, 48 insertions(+), 2 deletions(-)

Il 19/04/2012 16:07, Li Zhi Hui ha scritto:
> To avoid re-enter function fdctrl_transfer_handler, according paolo's suggestion, the patch is used to test.
> 
> Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
> ---
>  hw/dma.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
>  hw/isa.h |    2 ++
>  2 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/dma.c b/hw/dma.c
> index 0a9322d..0a59164 100644
> --- a/hw/dma.c
> +++ b/hw/dma.c
> @@ -45,6 +45,8 @@ struct dma_regs {
>      uint8_t eop;
>      DMA_transfer_handler transfer_handler;
>      void *opaque;
> +    bool channel_running;
> +    bool channel_is_asynchronous;
>  };
>  
>  #define ADDR 0
> @@ -138,6 +140,8 @@ static inline void init_chan (struct dma_cont *d, int ichan)
>      r = d->regs + ichan;
>      r->now[ADDR] = r->base[ADDR] << d->dshift;
>      r->now[COUNT] = 0;
> +    r->channel_running = false;
> +    r->channel_is_asynchronous = false;
>  }
>  
>  static inline int getff (struct dma_cont *d)
> @@ -375,9 +379,20 @@ static void DMA_run (void)
>  
>              mask = 1 << ichan;
>  
> -            if ((0 == (d->mask & mask)) && (0 != (d->status & (mask << 4)))) {
> -                channel_run (icont, ichan);
> +            while ((0 == (d->mask & mask)) &&
> +                   (0 != (d->status & (mask << 4)))) {
> +                struct dma_regs *r = &dma_controllers[icont].regs[ichan];
> +                if (r->channel_running) {
> +                    assert(r->channel_is_asynchronous);
> +                    break;
> +                }
> +                r->channel_running = true;
> +                channel_run(icont, ichan);
>                  rearm = 1;
> +                if (r->channel_is_asynchronous) {
> +                    break;
> +                }
> +                r->channel_running = false;
>              }
>          }
>      }
> @@ -388,6 +403,35 @@ out:
>          qemu_bh_schedule_idle(dma_bh);
>  }
>  
> +void DMA_set_channel_async(int nchan, bool val)
> +{
> +    int icont, ichan;
> +    struct dma_regs *r;
> +
> +    icont = nchan > 3;
> +    ichan = nchan & 3;
> +    r = &dma_controllers[icont].regs[ichan];
> +    r->channel_is_asynchronous = val;
> +}
> +
> +void DMA_set_return(int nret, int nchan)
> +{
> +    struct dma_regs *r;
> +    struct dma_cont *d;
> +    int icont, ichan;
> +
> +    icont = nchan > 3;
> +    ichan = nchan & 3;
> +    d = dma_controllers;
> +    r = &d[icont].regs[ichan];
> +    r->now[COUNT] = nret;
> +    assert(r->channel_is_asynchronous);
> +    assert(r->channel_running);

Thanks, this is very much like what I had in mind, except that here I
would have called DMA_run.  Also you can then remove the bottom half
(and rearm logic) completely.

If calling DMA_run is not working, perhaps it is because you didn't
remove the bottom half.

I would hope that you can also change the "if (running) goto out;" to an
"assert(!running)", but I'm not so sure.

Paolo

>> +
>> +void DMA_set_return(int nret, int nchan)
>> +{
>> +    struct dma_regs *r;
>> +    struct dma_cont *d;
>> +    int icont, ichan;
>> +
>> +    icont = nchan>  3;
>> +    ichan = nchan&  3;
>> +    d = dma_controllers;
>> +    r =&d[icont].regs[ichan];
>> +    r->now[COUNT] = nret;
>> +    assert(r->channel_is_asynchronous);
>> +    assert(r->channel_running);
>
> Thanks, this is very much like what I had in mind, except that here I
> would have called DMA_run.  Also you can then remove the bottom half
> (and rearm logic) completely.
>
> If calling DMA_run is not working, perhaps it is because you didn't
> remove the bottom half.

You have written to me :

    assert(channel_is_asynchronous[ichan]);
    assert(channel_running[ichan] == 1);
    channel_running[ichan]--;
    if ((0 == (d->mask & mask)) && (0 != (d->status & (mask << 4)))) {
        channel_run (icont, ichan);
    }

If I add the code :
    if ((0 == (d->mask & mask)) && (0 != (d->status & (mask << 4)))) {
        channel_run (icont, ichan);
    }
Because function DMA_set_return is called in fdctrl_read_DMA_cb and 
fdctrl_write_DMA_cb, the fdctrl_stop_transfer will release the channel,
but fdctrl_stop_transfer is after the DMA_set_return, so the channel_run 
will run again, so here I delete the above code.

>
> I would hope that you can also change the "if (running) goto out;" to an
> "assert(!running)", but I'm not so sure.
>

sorry, here I don't understand well,or you can explain in detail.  :)


Thank you very much for your feedback!   :)

Il 20/04/2012 04:36, Zhi Hui Li ha scritto:
> If I add the code :
>    if ((0 == (d->mask & mask)) && (0 != (d->status & (mask << 4)))) {
>        channel_run (icont, ichan);
>    }
> Because function DMA_set_return is called in fdctrl_read_DMA_cb and
> fdctrl_write_DMA_cb, the fdctrl_stop_transfer will release the channel,
> but fdctrl_stop_transfer is after the DMA_set_return, so the channel_run
> will run again, so here I delete the above code.

I looked again at the code, and man it is painful! :)  (Both the
synchronous and asynchronous versions).

I think the best bet is to do the transfer one sector at a time even in
the asynchronous code, and reuse the fifo buffer for this).

Remember that your final objective is to execute exactly the same code
that is in QEMU right now, only scattered across multiple function calls.

Some other notes:

- you don't need the opaque_cb.  Just pass the fdctrl, the dma_len can
be stored in there and the channel is already available

- the scan support is completely broken in QEMU already.  It always
returns SEH (scan equal hit), probably because of misplaced gotos.

Paolo