Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-08 Thread Laurent Pinchart
Hi Russell,

On Monday 06 June 2011 20:00:52 Russell King - ARM Linux wrote:
 On Mon, Jun 06, 2011 at 06:54:10PM +0200, Laurent Pinchart wrote:
  Of course not, but if the scatterlist is only touched by kernel code, it
  doesn't need to be contiguous in memory. It could be allocated with
  vmalloc().
 
 Except vmalloc has a higher latency and a more restrictive API than
 other allocators (think about the coherency issues with SMP systems
 which may have to do TLB flushing on several cores, etc.)

Right, thank you for the explanation. It is now clear to me why we want to use 
the page allocator and handle scatter list chaining in the critical paths. We 
could still use vmalloc() in the iovmm driver, but that's probably not worth 
it if we can enable scatter list chaining for the system.

With your patch scatter list chaining can be enabled at the platform level for 
the ARM architecture. What are the platform requirements to enable scatter 
list chaining ?

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-06 Thread Laurent Pinchart
Hi Russell,

On Friday 03 June 2011 08:32:12 Russell King - ARM Linux wrote:
 On Fri, Jun 03, 2011 at 02:12:47AM +0200, Laurent Pinchart wrote:
  On Wednesday 01 June 2011 16:03:06 Russell King - ARM Linux wrote:
   On Wed, Jun 01, 2011 at 03:50:50PM +0200, Laurent Pinchart wrote:
In the specific iovmm case, the driver uses the sglist API to build a
list of page-size sg entries, and then process it in software. Is
that considered as an abuse of the sglist API, or valid usage ?

Anyway, sglist chaining is not needed by iovmm. As iovmm just walks
the sglist manually, it's easier to allocate it in one go rather
than using sglist chaining. This of course doesn't make your patch
unneeded or wrong.
   
   Well, there's a two issues here:
   1. Should iovmm use sg_phys(sg) with sg_dma_len(sg) ?
   
  Probably not, because a scatterlist before DMA API mapping is
  defined by sg_page(sg), sg-offset, sg-length and has N entries. 
  After DMA API mapping (n = dma_map_sg(dev, sg, N, dir)), it has n
  entries where n = N, and the DMA address/lengths are
  sg_dma_address(sg) and sg_dma_len(sg).  Both these are undefined
  for unmapped scatterlists.
  
  Getting this wrong means breakage when CONFIG_NEED_SG_DMA_LENGTH is
  enabled.
  
  iovmm abuses the sglist API, there's no doubt on that. It will break when
  CONFIG_NEED_SG_DMA_LENGTH is enabled. iovmm should probably not use the
  sglist API, and it should probably not even exist in the first place. I
  know that TI is working on moving the OMAP-specific iommu/iovmm
  implementation to the generic IOMMU API, but that will take time. In the
  meantime, I'd like to fix iovmm to avoid the userspace-triggerable
  BUG_ON().
  
   2. What would be the effect of enabling SG list chaining on iovmm?
   
  The code uses the correct SG list walking helpers (for_each_sg) so
  it should be able to cope with chained SG lists.
  
  Yes it should. It might be slightly less efficient, but I don't think we
  will notice.
  
   So, I think there's no problem here with chained SG lists, but there is
   an issue with using sg_dma_len().  I'd suggest converting stuff to use
   sg-length with sg_page(sg) rather than sg_dma_len(sg).
  
  With sg_page(sg) ? I'm not sure to follow you there.
 
 sg-length and sg_page(sg) are paired (and sg-length is paired with
 other stuff).  They describe the scatterlist _before_ DMA API mapping.
 After DMA API mapping, the scatterlist describes a list of regions
 defined by sg_dma_address(sg) and sg_dma_len(sg) - sg_dma_len(sg) is
 _only_ paired with sg_dma_address(sg).

OK. The driver is already using sg_phys(sg), which is a wrapper around 
sg_page(sg). I still need to replace sg_dma_len(sg) with sg-length.

   As for whether SG chaining is required or not, if you're running up
   against the maximum SG table size, then you do have a requirement for
   SG chaining.
  
  The SG table size limit makes sure that the SG list fits in a page, so
  that it can be passed to the hardware. This isn't needed by iovmm, as it
  processes the sglist in software. iovmm could use SG chaining, but we
  would then need to enable it for the SoCs on which iovmm is used. I
  don't know if they properly support that.
 
 Err, no.  scatterlists are _never_ passed to hardware.  They're a kernel
 internal description of a list of regions in memory, which initially
 start off as describing the kernels view of those regions.  After DMA
 mapping, they describe it in terms of the device's view of those
 regions.
 
 At that point, scatterlists get converted to whatever form is required
 by the hardware doing DMA, which most certainly won't be the layout which
 struct scatterlist describes.
 
 SG chaining has _nothing_ to do with hardware.  It's all to do with software
 and hitting the SG table limit.

What's the reason for limiting the SG table size to one page then ?

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-06 Thread Russell King - ARM Linux
On Mon, Jun 06, 2011 at 06:23:18PM +0200, Laurent Pinchart wrote:
 Hi Russell,
 
 On Friday 03 June 2011 08:32:12 Russell King - ARM Linux wrote:
  SG chaining has _nothing_ to do with hardware.  It's all to do with software
  and hitting the SG table limit.
 
 What's the reason for limiting the SG table size to one page then ?

As I say, it's got nothing to do with them ending up being passed to
hardware.  Take a look at their definition:

struct scatterlist {
#ifdef CONFIG_DEBUG_SG
unsigned long   sg_magic;
#endif
unsigned long   page_link;
unsigned intoffset;
unsigned intlength;
dma_addr_t  dma_address;
#ifdef CONFIG_NEED_SG_DMA_LENGTH
unsigned intdma_length;
#endif
};

That clearly isn't hardware specific - hardware won't cope with
CONFIG_DEBUG_SG being enabled or disabled, or whether the architecture
supports the dma_length field, or that this structure has developed from
being:

void *addr;
unsigend int length;
unsigned long dma_address;

to the above over the evolution of the kernel.  Or that we use the bottom
two bits of page_link as our own flag bits?

So no, this struct goes nowhere near hardware of any kind.  It's merely
used as a container to pass a list of scatter-gather locations in memory
internally around within the kernel, especially to dma_map_sg()/
dma_unmap_sg().

If you look at IDE or ATA code, or even SCSI code, you'll find the same
pattern.  They're passed a scatterlist.  They map it for dma using
dma_map_sg().  They then walk the scatterlist and extract the dma
address and length using sg_dma_address() and sg_dma_length() and create
the _hardware_ table from that information - and the hardware table very
much depends on the hardware itself.  Once DMA is complete, they unmap
the DMA region using dma_unmap_sg().

One very good reason that its limited to one page is because allocations
larger than one page are prone to failure.  Would you want your company
server failing to read/write data to its storage just because it couldn't
get a contiguous 8K page for a 5K long scatterlist?  I think if Linux
did that, it wouldn't have a future in the enterprise marketplace.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-06 Thread Laurent Pinchart
Hi Russell,

On Monday 06 June 2011 18:44:00 Russell King - ARM Linux wrote:
 On Mon, Jun 06, 2011 at 06:23:18PM +0200, Laurent Pinchart wrote:
  Hi Russell,
  
  On Friday 03 June 2011 08:32:12 Russell King - ARM Linux wrote:
   SG chaining has _nothing_ to do with hardware.  It's all to do with
   software and hitting the SG table limit.
  
  What's the reason for limiting the SG table size to one page then ?
 
 As I say, it's got nothing to do with them ending up being passed to
 hardware.  Take a look at their definition:
 
 struct scatterlist {
 #ifdef CONFIG_DEBUG_SG
 unsigned long   sg_magic;
 #endif
 unsigned long   page_link;
 unsigned intoffset;
 unsigned intlength;
 dma_addr_t  dma_address;
 #ifdef CONFIG_NEED_SG_DMA_LENGTH
 unsigned intdma_length;
 #endif
 };
 
 That clearly isn't hardware specific - hardware won't cope with
 CONFIG_DEBUG_SG being enabled or disabled, or whether the architecture
 supports the dma_length field, or that this structure has developed from
 being:
 
   void *addr;
   unsigend int length;
   unsigned long dma_address;
 
 to the above over the evolution of the kernel.  Or that we use the bottom
 two bits of page_link as our own flag bits?
 
 So no, this struct goes nowhere near hardware of any kind.  It's merely
 used as a container to pass a list of scatter-gather locations in memory
 internally around within the kernel, especially to dma_map_sg()/
 dma_unmap_sg().
 
 If you look at IDE or ATA code, or even SCSI code, you'll find the same
 pattern.  They're passed a scatterlist.  They map it for dma using
 dma_map_sg().  They then walk the scatterlist and extract the dma
 address and length using sg_dma_address() and sg_dma_length() and create
 the _hardware_ table from that information - and the hardware table very
 much depends on the hardware itself.  Once DMA is complete, they unmap
 the DMA region using dma_unmap_sg().
 
 One very good reason that its limited to one page is because allocations
 larger than one page are prone to failure.  Would you want your company
 server failing to read/write data to its storage just because it couldn't
 get a contiguous 8K page for a 5K long scatterlist?  I think if Linux
 did that, it wouldn't have a future in the enterprise marketplace.

Of course not, but if the scatterlist is only touched by kernel code, it 
doesn't need to be contiguous in memory. It could be allocated with vmalloc().

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-06 Thread Russell King - ARM Linux
On Mon, Jun 06, 2011 at 06:54:10PM +0200, Laurent Pinchart wrote:
 Of course not, but if the scatterlist is only touched by kernel code, it 
 doesn't need to be contiguous in memory. It could be allocated with vmalloc().

Except vmalloc has a higher latency and a more restrictive API than
other allocators (think about the coherency issues with SMP systems
which may have to do TLB flushing on several cores, etc.)
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-03 Thread Russell King - ARM Linux
On Fri, Jun 03, 2011 at 02:12:47AM +0200, Laurent Pinchart wrote:
 Hi Russell,
 
 On Wednesday 01 June 2011 16:03:06 Russell King - ARM Linux wrote:
  On Wed, Jun 01, 2011 at 03:50:50PM +0200, Laurent Pinchart wrote:
   In the specific iovmm case, the driver uses the sglist API to build a
   list of page-size sg entries, and then process it in software. Is that
   considered as an abuse of the sglist API, or valid usage ?
   
   Anyway, sglist chaining is not needed by iovmm. As iovmm just walks the
   sglist manually, it's easier to allocate it in one go rather than using
   sglist chaining. This of course doesn't make your patch unneeded or
   wrong.
  
  Well, there's a two issues here:
  1. Should iovmm use sg_phys(sg) with sg_dma_len(sg) ?
 Probably not, because a scatterlist before DMA API mapping is defined
 by sg_page(sg), sg-offset, sg-length and has N entries.  After DMA
 API mapping (n = dma_map_sg(dev, sg, N, dir)), it has n entries where
 n = N, and the DMA address/lengths are sg_dma_address(sg) and
 sg_dma_len(sg).  Both these are undefined for unmapped scatterlists.
  
 Getting this wrong means breakage when CONFIG_NEED_SG_DMA_LENGTH is
 enabled.
 
 iovmm abuses the sglist API, there's no doubt on that. It will break when 
 CONFIG_NEED_SG_DMA_LENGTH is enabled. iovmm should probably not use the 
 sglist 
 API, and it should probably not even exist in the first place. I know that TI 
 is working on moving the OMAP-specific iommu/iovmm implementation to the 
 generic IOMMU API, but that will take time. In the meantime, I'd like to fix 
 iovmm to avoid the userspace-triggerable BUG_ON().
 
  2. What would be the effect of enabling SG list chaining on iovmm?
 The code uses the correct SG list walking helpers (for_each_sg) so
 it should be able to cope with chained SG lists.
 
 Yes it should. It might be slightly less efficient, but I don't think we will 
 notice.
 
  So, I think there's no problem here with chained SG lists, but there is
  an issue with using sg_dma_len().  I'd suggest converting stuff to use
  sg-length with sg_page(sg) rather than sg_dma_len(sg).
 
 With sg_page(sg) ? I'm not sure to follow you there.

sg-length and sg_page(sg) are paired (and sg-length is paired with
other stuff).  They describe the scatterlist _before_ DMA API mapping.
After DMA API mapping, the scatterlist describes a list of regions
defined by sg_dma_address(sg) and sg_dma_len(sg) - sg_dma_len(sg) is
_only_ paired with sg_dma_address(sg).

  As for whether SG chaining is required or not, if you're running up against
  the maximum SG table size, then you do have a requirement for SG chaining.
 
 The SG table size limit makes sure that the SG list fits in a page, so that 
 it 
 can be passed to the hardware. This isn't needed by iovmm, as it processes 
 the 
 sglist in software. iovmm could use SG chaining, but we would then need to 
 enable it for the SoCs on which iovmm is used. I don't know if they properly 
 support that.

Err, no.  scatterlists are _never_ passed to hardware.  They're a kernel
internal description of a list of regions in memory, which initially
start off as describing the kernels view of those regions.  After DMA
mapping, they describe it in terms of the device's view of those
regions.

At that point, scatterlists get converted to whatever form is required
by the hardware doing DMA, which most certainly won't be the layout which
struct scatterlist describes.

SG chaining has _nothing_ to do with hardware.  It's all to do with
software and hitting the SG table limit.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-03 Thread Felipe Contreras
On Fri, Jun 3, 2011 at 3:12 AM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 Hi Russell,

 On Wednesday 01 June 2011 16:03:06 Russell King - ARM Linux wrote:
 On Wed, Jun 01, 2011 at 03:50:50PM +0200, Laurent Pinchart wrote:
  In the specific iovmm case, the driver uses the sglist API to build a
  list of page-size sg entries, and then process it in software. Is that
  considered as an abuse of the sglist API, or valid usage ?
 
  Anyway, sglist chaining is not needed by iovmm. As iovmm just walks the
  sglist manually, it's easier to allocate it in one go rather than using
  sglist chaining. This of course doesn't make your patch unneeded or
  wrong.

 Well, there's a two issues here:
 1. Should iovmm use sg_phys(sg) with sg_dma_len(sg) ?
    Probably not, because a scatterlist before DMA API mapping is defined
    by sg_page(sg), sg-offset, sg-length and has N entries.  After DMA
    API mapping (n = dma_map_sg(dev, sg, N, dir)), it has n entries where
    n = N, and the DMA address/lengths are sg_dma_address(sg) and
    sg_dma_len(sg).  Both these are undefined for unmapped scatterlists.

    Getting this wrong means breakage when CONFIG_NEED_SG_DMA_LENGTH is
    enabled.

 iovmm abuses the sglist API, there's no doubt on that. It will break when
 CONFIG_NEED_SG_DMA_LENGTH is enabled. iovmm should probably not use the sglist
 API, and it should probably not even exist in the first place. I know that TI
 is working on moving the OMAP-specific iommu/iovmm implementation to the
 generic IOMMU API, but that will take time. In the meantime, I'd like to fix
 iovmm to avoid the userspace-triggerable BUG_ON().

This would also allow the tidspbridge driver to use iommu.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-02 Thread Laurent Pinchart
Hi Russell,

On Wednesday 01 June 2011 16:03:06 Russell King - ARM Linux wrote:
 On Wed, Jun 01, 2011 at 03:50:50PM +0200, Laurent Pinchart wrote:
  In the specific iovmm case, the driver uses the sglist API to build a
  list of page-size sg entries, and then process it in software. Is that
  considered as an abuse of the sglist API, or valid usage ?
  
  Anyway, sglist chaining is not needed by iovmm. As iovmm just walks the
  sglist manually, it's easier to allocate it in one go rather than using
  sglist chaining. This of course doesn't make your patch unneeded or
  wrong.
 
 Well, there's a two issues here:
 1. Should iovmm use sg_phys(sg) with sg_dma_len(sg) ?
Probably not, because a scatterlist before DMA API mapping is defined
by sg_page(sg), sg-offset, sg-length and has N entries.  After DMA
API mapping (n = dma_map_sg(dev, sg, N, dir)), it has n entries where
n = N, and the DMA address/lengths are sg_dma_address(sg) and
sg_dma_len(sg).  Both these are undefined for unmapped scatterlists.
 
Getting this wrong means breakage when CONFIG_NEED_SG_DMA_LENGTH is
enabled.

iovmm abuses the sglist API, there's no doubt on that. It will break when 
CONFIG_NEED_SG_DMA_LENGTH is enabled. iovmm should probably not use the sglist 
API, and it should probably not even exist in the first place. I know that TI 
is working on moving the OMAP-specific iommu/iovmm implementation to the 
generic IOMMU API, but that will take time. In the meantime, I'd like to fix 
iovmm to avoid the userspace-triggerable BUG_ON().

 2. What would be the effect of enabling SG list chaining on iovmm?
The code uses the correct SG list walking helpers (for_each_sg) so
it should be able to cope with chained SG lists.

Yes it should. It might be slightly less efficient, but I don't think we will 
notice.

 So, I think there's no problem here with chained SG lists, but there is
 an issue with using sg_dma_len().  I'd suggest converting stuff to use
 sg-length with sg_page(sg) rather than sg_dma_len(sg).

With sg_page(sg) ? I'm not sure to follow you there.

 As for whether SG chaining is required or not, if you're running up against
 the maximum SG table size, then you do have a requirement for SG chaining.

The SG table size limit makes sure that the SG list fits in a page, so that it 
can be passed to the hardware. This isn't needed by iovmm, as it processes the 
sglist in software. iovmm could use SG chaining, but we would then need to 
enable it for the SoCs on which iovmm is used. I don't know if they properly 
support that.

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-01 Thread Laurent Pinchart
sg_alloc_table can only allocate multi-page scatter-gather list tables
if the architecture supports scatter-gather lists chaining. ARM doesn't
fit in that category.

The IOMMU driver abuses the sg table structure only to hold page
addresses without ever passing the table to the device.

Use __sg_alloc_table instead of sg_alloc_table and allocate all entries
in one go. Otherwise trying to use a large userspace buffers to capture
video will hit a BUG_ON in __sg_alloc_table.

Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
Acked-by: Hiroshi DOYU hiroshi.d...@nokia.com
---
 arch/arm/plat-omap/iovmm.c |   14 --
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 51ef43e..b82cef4 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -121,6 +121,16 @@ static unsigned sgtable_nents(size_t bytes, u32 da, u32 pa)
return nr_entries;
 }
 
+static struct scatterlist *sg_alloc(unsigned int nents, gfp_t gfp_mask)
+{
+   return kmalloc(nents * sizeof(struct scatterlist), gfp_mask);
+}
+
+static void sg_free(struct scatterlist *sg, unsigned int nents)
+{
+   kfree(sg);
+}
+
 /* allocate and initialize sg_table header(a kind of 'superblock') */
 static struct sg_table *sgtable_alloc(const size_t bytes, u32 flags,
u32 da, u32 pa)
@@ -146,7 +156,7 @@ static struct sg_table *sgtable_alloc(const size_t bytes, 
u32 flags,
if (!sgt)
return ERR_PTR(-ENOMEM);
 
-   err = sg_alloc_table(sgt, nr_entries, GFP_KERNEL);
+   err = __sg_alloc_table(sgt, nr_entries, -1, GFP_KERNEL, sg_alloc);
if (err) {
kfree(sgt);
return ERR_PTR(err);
@@ -163,7 +173,7 @@ static void sgtable_free(struct sg_table *sgt)
if (!sgt)
return;
 
-   sg_free_table(sgt);
+   __sg_free_table(sgt, -1, sg_free);
kfree(sgt);
 
pr_debug(%s: sgt:%p\n, __func__, sgt);
-- 
1.7.3.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-01 Thread Russell King - ARM Linux
On Wed, Jun 01, 2011 at 03:30:11PM +0200, Laurent Pinchart wrote:
 sg_alloc_table can only allocate multi-page scatter-gather list tables
 if the architecture supports scatter-gather lists chaining. ARM doesn't
 fit in that category.

Let's fix this properly, as I've said countless times and so far no one
has bothered to sort this out:

8
From: Russell King rmk+ker...@arm.linux.org.uk
Subject: ARM: Allow SoCs to enable scatterlist chaining

Allow SoCs to enable the scatterlist chaining support, which allows
scatterlist tables to be broken up into smaller allocations.

As support for this feature depends on the implementation details of
the users of the scatterlists, we can't enable this globally without
auditing all the users, which is a very big task.  Instead, let SoCs
progressively switch over to using this.

SoC drivers using scatterlists and SoC DMA implementations need
auditing before this option can be enabled for the SoC.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/Kconfig   |3 +++
 arch/arm/include/asm/scatterlist.h |4 
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 9adc278..cc0dcbf 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -37,6 +37,9 @@ config ARM
  Europe.  There is an ARM Linux project with a web page at
  http://www.arm.linux.org.uk/.
 
+config ARM_HAS_SG_CHAIN
+   bool
+
 config HAVE_PWM
bool
 
diff --git a/arch/arm/include/asm/scatterlist.h 
b/arch/arm/include/asm/scatterlist.h
index 2f87870..cefdb8f 100644
--- a/arch/arm/include/asm/scatterlist.h
+++ b/arch/arm/include/asm/scatterlist.h
@@ -1,6 +1,10 @@
 #ifndef _ASMARM_SCATTERLIST_H
 #define _ASMARM_SCATTERLIST_H
 
+#ifdef CONFIG_ARM_HAS_SG_CHAIN
+#define ARCH_HAS_SG_CHAIN
+#endif
+
 #include asm/memory.h
 #include asm/types.h
 #include asm-generic/scatterlist.h

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-01 Thread Laurent Pinchart
Hi Russell,

On Wednesday 01 June 2011 15:43:38 Russell King - ARM Linux wrote:
 On Wed, Jun 01, 2011 at 03:30:11PM +0200, Laurent Pinchart wrote:
  sg_alloc_table can only allocate multi-page scatter-gather list tables
  if the architecture supports scatter-gather lists chaining. ARM doesn't
  fit in that category.
 
 Let's fix this properly, as I've said countless times and so far no one
 has bothered to sort this out:
 
 8
 From: Russell King rmk+ker...@arm.linux.org.uk
 Subject: ARM: Allow SoCs to enable scatterlist chaining
 
 Allow SoCs to enable the scatterlist chaining support, which allows
 scatterlist tables to be broken up into smaller allocations.
 
 As support for this feature depends on the implementation details of
 the users of the scatterlists, we can't enable this globally without
 auditing all the users, which is a very big task.  Instead, let SoCs
 progressively switch over to using this.
 
 SoC drivers using scatterlists and SoC DMA implementations need
 auditing before this option can be enabled for the SoC.

In the specific iovmm case, the driver uses the sglist API to build a list of 
page-size sg entries, and then process it in software. Is that considered as 
an abuse of the sglist API, or valid usage ?

Anyway, sglist chaining is not needed by iovmm. As iovmm just walks the sglist 
manually, it's easier to allocate it in one go rather than using sglist 
chaining. This of course doesn't make your patch unneeded or wrong.

 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 ---
  arch/arm/Kconfig   |3 +++
  arch/arm/include/asm/scatterlist.h |4 
  2 files changed, 7 insertions(+), 0 deletions(-)
 
 diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
 index 9adc278..cc0dcbf 100644
 --- a/arch/arm/Kconfig
 +++ b/arch/arm/Kconfig
 @@ -37,6 +37,9 @@ config ARM
 Europe.  There is an ARM Linux project with a web page at
 http://www.arm.linux.org.uk/.
 
 +config ARM_HAS_SG_CHAIN
 + bool
 +
  config HAVE_PWM
   bool
 
 diff --git a/arch/arm/include/asm/scatterlist.h
 b/arch/arm/include/asm/scatterlist.h index 2f87870..cefdb8f 100644
 --- a/arch/arm/include/asm/scatterlist.h
 +++ b/arch/arm/include/asm/scatterlist.h
 @@ -1,6 +1,10 @@
  #ifndef _ASMARM_SCATTERLIST_H
  #define _ASMARM_SCATTERLIST_H
 
 +#ifdef CONFIG_ARM_HAS_SG_CHAIN
 +#define ARCH_HAS_SG_CHAIN
 +#endif
 +
  #include asm/memory.h
  #include asm/types.h
  #include asm-generic/scatterlist.h

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU

2011-06-01 Thread Russell King - ARM Linux
On Wed, Jun 01, 2011 at 03:50:50PM +0200, Laurent Pinchart wrote:
 In the specific iovmm case, the driver uses the sglist API to build a list of 
 page-size sg entries, and then process it in software. Is that considered as 
 an abuse of the sglist API, or valid usage ?
 
 Anyway, sglist chaining is not needed by iovmm. As iovmm just walks the 
 sglist 
 manually, it's easier to allocate it in one go rather than using sglist 
 chaining. This of course doesn't make your patch unneeded or wrong.

Well, there's a two issues here:
1. Should iovmm use sg_phys(sg) with sg_dma_len(sg) ?
   Probably not, because a scatterlist before DMA API mapping is defined
   by sg_page(sg), sg-offset, sg-length and has N entries.  After DMA
   API mapping (n = dma_map_sg(dev, sg, N, dir)), it has n entries where
   n = N, and the DMA address/lengths are sg_dma_address(sg) and
   sg_dma_len(sg).  Both these are undefined for unmapped scatterlists.

   Getting this wrong means breakage when CONFIG_NEED_SG_DMA_LENGTH is
   enabled.

2. What would be the effect of enabling SG list chaining on iovmm?
   The code uses the correct SG list walking helpers (for_each_sg) so
   it should be able to cope with chained SG lists.

So, I think there's no problem here with chained SG lists, but there is
an issue with using sg_dma_len().  I'd suggest converting stuff to use
sg-length with sg_page(sg) rather than sg_dma_len(sg).

As for whether SG chaining is required or not, if you're running up
against the maximum SG table size, then you do have a requirement for
SG chaining.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html