From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Russell King - ARM Linux <linux@arm.linux.org.uk> Cc: linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU Date: Fri, 3 Jun 2011 02:12:47 +0200 [thread overview] Message-ID: <201106030212.48226.laurent.pinchart@ideasonboard.com> (raw) In-Reply-To: <20110601140306.GC6700@n2100.arm.linux.org.uk> 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
WARNING: multiple messages have this Message-ID (diff)
From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU Date: Fri, 3 Jun 2011 02:12:47 +0200 [thread overview] Message-ID: <201106030212.48226.laurent.pinchart@ideasonboard.com> (raw) In-Reply-To: <20110601140306.GC6700@n2100.arm.linux.org.uk> 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
next prev parent reply other threads:[~2011-06-03 0:12 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-05-30 12:47 [PATCH 0/2] OMAP3 IOMMU fixes Laurent Pinchart 2011-05-30 12:47 ` [PATCH 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU Laurent Pinchart 2011-05-31 13:22 ` Tony Lindgren 2011-06-01 12:25 ` [PATCH v2 " Laurent Pinchart 2011-06-01 13:11 ` Tony Lindgren 2011-06-01 12:25 ` [PATCH v2 2/2] omap3: iovmm: Support non page-aligned buffers in iommu_vmap Laurent Pinchart 2011-06-01 12:50 ` Tony Lindgren 2011-06-01 13:09 ` Laurent Pinchart 2011-06-01 13:10 ` Tony Lindgren 2011-06-01 13:17 ` Tony Lindgren 2011-06-01 13:30 ` [PATCH v3 1/2] omap3: iovmm: Work around sg_alloc_table size limitation in IOMMU Laurent Pinchart 2011-06-01 13:30 ` Laurent Pinchart 2011-06-01 13:43 ` Russell King - ARM Linux 2011-06-01 13:43 ` Russell King - ARM Linux 2011-06-01 13:50 ` Laurent Pinchart 2011-06-01 13:50 ` Laurent Pinchart 2011-06-01 14:03 ` Russell King - ARM Linux 2011-06-01 14:03 ` Russell King - ARM Linux 2011-06-03 0:12 ` Laurent Pinchart [this message] 2011-06-03 0:12 ` Laurent Pinchart 2011-06-03 6:32 ` Russell King - ARM Linux 2011-06-03 6:32 ` Russell King - ARM Linux 2011-06-06 16:23 ` Laurent Pinchart 2011-06-06 16:23 ` Laurent Pinchart 2011-06-06 16:44 ` Russell King - ARM Linux 2011-06-06 16:44 ` Russell King - ARM Linux 2011-06-06 16:54 ` Laurent Pinchart 2011-06-06 16:54 ` Laurent Pinchart 2011-06-06 18:00 ` Russell King - ARM Linux 2011-06-06 18:00 ` Russell King - ARM Linux 2011-06-08 10:33 ` Laurent Pinchart 2011-06-08 10:33 ` Laurent Pinchart 2011-06-03 9:39 ` Felipe Contreras 2011-06-03 9:39 ` Felipe Contreras 2011-06-01 13:30 ` [PATCH v3 2/2] omap3: iovmm: Support non page-aligned buffers in iommu_vmap Laurent Pinchart 2011-06-01 13:30 ` Laurent Pinchart 2011-05-30 12:47 ` [PATCH " Laurent Pinchart 2011-05-30 13:16 ` [PATCH 0/2] OMAP3 IOMMU fixes Hiroshi DOYU 2011-06-08 10:48 ` Laurent Pinchart 2011-06-13 13:40 ` Tony Lindgren 2011-06-13 16:41 ` Laurent Pinchart 2011-06-13 19:34 ` Ohad Ben-Cohen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=201106030212.48226.laurent.pinchart@ideasonboard.com \ --to=laurent.pinchart@ideasonboard.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-omap@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.