Re: [ros-svn] [hbirr] 17665: - Copy the map registers to the buffer only, if they are used (in IoFlushAdapterBuffers).

[Filip Navara]

[e-mail] wrote:
> Copy the map registers to the buffer only, if they are used (in IoFlushAdapterBuffers).
> Do not use the byte offset into the page from a given buffer if the map registers are used,
>
> because the caller didn't request for one additional register in the call to IoAllocateAdapterChannel 
>  and it will not work for a 64k buffer.

Sorry, but these changes are wrong. I won't argue that there isn't bug, 
but you're just workarounding it.

- At first, the UseMapRegisters variable you added to MAP_REGISTER_ENTRY 
structure isn't needed. IoFlushAdapterBuffers can determine the 
information itself and the case was clearly marked as UNIMPLEMENTED and 
FIXME.
- The byte offset is added for a reason. I know the stuff that it is 
supposed to solve is broken atm, but the rationale is that there can be 
more IoMapTransfer calls and together with MAP_REGISTER_ENTRY->Counter 
you can setup more transfers... It's explained in
http://download.microsoft.com/download/e/b/a/eba1050f-a31d-436b-9281-92cdfea....

I certainly welcome any help and review of the DMA code and I'll try to 
add comments where appropriate, but I would like to discuss these 
changes first on the Mailing List / IRC / mail since this piece of code 
is pretty complex and any changes are prone to break some feature...

- Filip

[Hartmut Birr]

Filip Navara wrote:
> At first, the UseMapRegisters variable you added to
>
> MAP_REGISTER_ENTRY structure isn't needed. IoFlushAdapterBuffers can
> determine the information itself and the case was clearly marked as
> UNIMPLEMENTED and FIXME.

This case isn't marked as UNIMPLEMENTED and FIXME. The using of hardware
scatter/gather is marked as UNIMPLEMENTED and FIXME. I've modified the
cache manager a little bit. The cache segments for the floppy driver
allocated from below the 16MB limit. I get no data from a floppy.
> The byte offset is added for a reason. I know the stuff that it is
>
> supposed to solve is broken atm, but the rationale is that there can
> be more IoMapTransfer calls and together with
> MAP_REGISTER_ENTRY->Counter you can setup more transfers... It's
> explained in
> http://download.microsoft.com/download/e/b/a/eba1050f-a31d-436b-9281-92cdfea...

It makes no sense to call IoMapTransfer more than ones without a call to
IoFlushAdapterBuffers for the pc dma controllers. For the pc dma
controllers, a transfer has to be finished, before the next can be
started. It is always possible to call IoFlushAdapterBuffers after each
transfer and before the next is started. The reference doesn't explain
something about to setup more transfers.

- Hartmut

[Filip Navara]

Hartmut Birr wrote:
> Filip Navara wrote:
> > At first, the UseMapRegisters variable you added to
> >
> > MAP_REGISTER_ENTRY structure isn't needed. IoFlushAdapterBuffers can
> > determine the information itself and the case was clearly marked as
> > UNIMPLEMENTED and FIXME.
>
> This case isn't marked as UNIMPLEMENTED and FIXME. The using of hardware
> scatter/gather is marked as UNIMPLEMENTED and FIXME. I've modified the
> cache manager a little bit. The cache segments for the floppy driver
> allocated from below the 16MB limit. I get no data from a floppy.

The UNIMPLEMENTED case is entered for any adapter that claims that it 
can support hardware scatter/gather regardless of it being used or not. 
A quick untested patch is attached (I'm not sure it will work, since it 
looks like I forgot to initialize MapRegisterBase->Counter...). I'm 
planning to look at it.
> > The byte offset is added for a reason. I know the stuff that it is
> >
> > supposed to solve is broken atm, but the rationale is that there can
> > be more IoMapTransfer calls and together with
> > MAP_REGISTER_ENTRY->Counter you can setup more transfers... It's
> > explained in
> > http://download.microsoft.com/download/e/b/a/eba1050f-a31d-436b-9281-92cdfea...
>
> It makes no sense to call IoMapTransfer more than ones without a call to
> IoFlushAdapterBuffers for the pc dma controllers. For the pc dma
> controllers, a transfer has to be finished, before the next can be
> started.

It applies to non-system-DMA... Quoting NT4 DDK documentation: "The 
driver of a busmaster device with scatter/gather support can use the 
returned logical address and updated Length value to build a 
scatter/gather list, calling IoMapTransfer repeatedly until it has used 
all available map registers for the transfer operation."
> It is always possible to call IoFlushAdapterBuffers after each
> transfer and before the next is started. The reference doesn't explain
> something about to setup more transfers.

See above.

I'll try to write more tomorrow, but my mind is getting foggy now...

Best regards,
Filip

[Hartmut Birr]

Filip Navara wrote:
> >
>
> The UNIMPLEMENTED case is entered for any adapter that claims that it
> can support hardware scatter/gather regardless of it being used or
> not. A quick untested patch is attached (I'm not sure it will work,
> since it looks like I forgot to initialize
> MapRegisterBase->Counter...). I'm planning to look at it.

The patch didn't work, because MAP_BASE_SW_SG is always set. The new
code in IoFlushAdapterBuffers isn't executed for the floppy dma.

- Hartmut

[Filip Navara]

Filip Navara wrote:
> Hartmut Birr wrote:
> > Filip Navara wrote:
> > > At first, the UseMapRegisters variable you added to
> > >
> > > MAP_REGISTER_ENTRY structure isn't needed. IoFlushAdapterBuffers can
> > > determine the information itself and the case was clearly marked as
> > > UNIMPLEMENTED and FIXME.
> >
> > This case isn't marked as UNIMPLEMENTED and FIXME. The using of hardware
> > scatter/gather is marked as UNIMPLEMENTED and FIXME. I've modified the
> > cache manager a little bit. The cache segments for the floppy driver
> > allocated from below the 16MB limit. I get no data from a floppy.
>
> The UNIMPLEMENTED case is entered for any adapter that claims that it 
> can support hardware scatter/gather regardless of it being used or 
> not. A quick untested patch is attached (I'm not sure it will work, 
> since it looks like I forgot to initialize 
> MapRegisterBase->Counter...). I'm planning to look at it.
> > > The byte offset is added for a reason. I know the stuff that it is
> > >
> > > supposed to solve is broken atm, but the rationale is that there can
> > > be more IoMapTransfer calls and together with
> > > MAP_REGISTER_ENTRY->Counter you can setup more transfers... It's
> > > explained in
> > > http://download.microsoft.com/download/e/b/a/eba1050f-a31d-436b-9281-92cdfea...
> >
> > It makes no sense to call IoMapTransfer more than ones without a call to
> > IoFlushAdapterBuffers for the pc dma controllers. For the pc dma
> > controllers, a transfer has to be finished, before the next can be
> > started.
>
> It applies to non-system-DMA... Quoting NT4 DDK documentation: "The 
> driver of a busmaster device with scatter/gather support can use the 
> returned logical address and updated Length value to build a 
> scatter/gather list, calling IoMapTransfer repeatedly until it has 
> used all available map registers for the transfer operation."
> > It is always possible to call IoFlushAdapterBuffers after each
> > transfer and before the next is started. The reference doesn't explain
> > something about to setup more transfers.
>
> See above.
>
> I'll try to write more tomorrow, but my mind is getting foggy now...

Oh, this time with the right patch...

- Filip

[Hartmut Birr]

Filip Navara wrote:
> Oh, this time with the right patch...

The patch works for me with a little modification. One test condition
must be changed in HalpCopyBufferMap.

- Hartmut

Index: hal/halx86/generic/dma.c
===================================================================
--- hal/halx86/generic/dma.c	(Revision 17704)
+++ hal/halx86/generic/dma.c	(Arbeitskopie)
@@ -1556,6 +1556,9 @@
 {
    BOOLEAN SlaveDma = FALSE;
    PMAP_REGISTER_ENTRY RealMapRegisterBase;
+   PHYSICAL_ADDRESS HighestAcceptableAddress;
+   PHYSICAL_ADDRESS PhysicalAddress;
+   PPFN_NUMBER MdlPagesPtr;

ASSERT_IRQL(DISPATCH_LEVEL);

@@ -1588,18 +1591,27 @@
    {
       if ((ULONG_PTR)MapRegisterBase & MAP_BASE_SW_SG)
       {
-         if (RealMapRegisterBase->Counter != ~0)
+         if (RealMapRegisterBase->Counter == ~0)
          {
             if (SlaveDma && !AdapterObject->IgnoreCount)
                Length -= HalReadDmaCounter(AdapterObject);
+            HalpCopyBufferMap(Mdl, RealMapRegisterBase, CurrentVa, Length, FALSE);
          }
-
-         HalpCopyBufferMap(Mdl, RealMapRegisterBase, CurrentVa, Length, FALSE);
       }
       else
       {
-         /* FIXME: Unimplemented case */
-         ASSERT(FALSE);
+         MdlPagesPtr = MmGetMdlPfnArray(Mdl);
+         MdlPagesPtr += ((ULONG_PTR)CurrentVa - (ULONG_PTR)Mdl->StartVa) >> PAGE_SHIFT;
+
+         PhysicalAddress.QuadPart = *MdlPagesPtr << PAGE_SHIFT;
+         PhysicalAddress.QuadPart += BYTE_OFFSET(CurrentVa);
+
+         HighestAcceptableAddress = HalpGetAdapterMaximumPhysicalAddress(AdapterObject);
+         if (PhysicalAddress.QuadPart + Length >
+             HighestAcceptableAddress.QuadPart)
+         {
+            HalpCopyBufferMap(Mdl, RealMapRegisterBase, CurrentVa, Length, FALSE);
+         }
       }
    }

@@ -1729,7 +1741,6 @@
     * pages that are physically contiguous and that don't cross the 
     * 64Kb boundary (this limitation applies only for ISA controllers).
     */
-
    while (TransferLength < *Length)
    {
       MdlPage1 = *MdlPagesPtr;
@@ -1784,11 +1795,12 @@
           HighestAcceptableAddress.QuadPart)
       {
          UseMapRegisters = TRUE;
-         PhysicalAddress = RealMapRegisterBase->PhysicalAddress;
+         PhysicalAddress = RealMapRegisterBase[Counter].PhysicalAddress;
          PhysicalAddress.QuadPart += ByteOffset;
          if ((ULONG_PTR)MapRegisterBase & MAP_BASE_SW_SG)
          {
             RealMapRegisterBase->Counter = ~0;
+            Counter = 0;
          }
       }
    }