| Message ID | 1729835552-14825-2-git-send-email-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] media: ipu6: move the l2_unmap() up before l2_map() | expand |
Hi Bingbu, Thanks for the update. On Fri, Oct 25, 2024 at 01:52:31PM +0800, Bingbu Cao wrote: > ipu6_mmu_map() operated on a per-page basis, it leads frequent > spin_lock/unlock() and clflush_cache_range() for each page, it > will cause inefficiencies especially when handling dma-bufs > with large number of pages. However, the pages are likely concentrated > pages by IOMMU DMA driver, IPU MMU driver can map the concentrated > pages into less entries in l1 table. > > This change enhances ipu6_mmu_map() with batching process multiple > contiguous pages. It significantly reduces calls for spin_lock/unlock > and clflush_cache_range() and improve the performance. > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com> > --- > drivers/media/pci/intel/ipu6/ipu6-mmu.c | 144 +++++++++++++++----------------- > 1 file changed, 69 insertions(+), 75 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-mmu.c b/drivers/media/pci/intel/ipu6/ipu6-mmu.c > index 9ea6789bca5e..e957ccb4691d 100644 > --- a/drivers/media/pci/intel/ipu6/ipu6-mmu.c > +++ b/drivers/media/pci/intel/ipu6/ipu6-mmu.c > @@ -295,72 +295,90 @@ static size_t l2_unmap(struct ipu6_mmu_info *mmu_info, unsigned long iova, > static int l2_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, > phys_addr_t paddr, size_t size) > { > - u32 l1_idx = iova >> ISP_L1PT_SHIFT; > - u32 iova_start = iova; > + struct device *dev = mmu_info->dev; > + unsigned int l2_entries; > u32 *l2_pt, *l2_virt; > unsigned int l2_idx; > unsigned long flags; > + size_t mapped = 0; > dma_addr_t dma; > u32 l1_entry; > - > - dev_dbg(mmu_info->dev, > - "mapping l2 page table for l1 index %u (iova %8.8x)\n", > - l1_idx, (u32)iova); > + u32 l1_idx; > + int err = 0; > > spin_lock_irqsave(&mmu_info->lock, flags); > - l1_entry = mmu_info->l1_pt[l1_idx]; > - if (l1_entry == mmu_info->dummy_l2_pteval) { > - l2_virt = mmu_info->l2_pts[l1_idx]; > - if (likely(!l2_virt)) { > - l2_virt = alloc_l2_pt(mmu_info); > - if (!l2_virt) { > - spin_unlock_irqrestore(&mmu_info->lock, flags); > - return -ENOMEM; > - } > - } > - > - dma = map_single(mmu_info, l2_virt); > - if (!dma) { > - dev_err(mmu_info->dev, "Failed to map l2pt page\n"); > - free_page((unsigned long)l2_virt); > - spin_unlock_irqrestore(&mmu_info->lock, flags); > - return -EINVAL; > - } > - > - l1_entry = dma >> ISP_PADDR_SHIFT; > - > - dev_dbg(mmu_info->dev, "page for l1_idx %u %p allocated\n", > - l1_idx, l2_virt); > - mmu_info->l1_pt[l1_idx] = l1_entry; > - mmu_info->l2_pts[l1_idx] = l2_virt; > - clflush_cache_range((void *)&mmu_info->l1_pt[l1_idx], > - sizeof(mmu_info->l1_pt[l1_idx])); > - } > - > - l2_pt = mmu_info->l2_pts[l1_idx]; > - > - dev_dbg(mmu_info->dev, "l2_pt at %p with dma 0x%x\n", l2_pt, l1_entry); > > paddr = ALIGN(paddr, ISP_PAGE_SIZE); > + for (l1_idx = iova >> ISP_L1PT_SHIFT; > + size > 0 && l1_idx < ISP_L1PT_PTES; l1_idx++) { > + dev_dbg(dev, > + "mapping l2 page table for l1 index %u (iova %8.8x)\n", > + l1_idx, (u32)iova); > > - l2_idx = (iova_start & ISP_L2PT_MASK) >> ISP_L2PT_SHIFT; > + l1_entry = mmu_info->l1_pt[l1_idx]; > + if (l1_entry == mmu_info->dummy_l2_pteval) { > + l2_virt = mmu_info->l2_pts[l1_idx]; > + if (likely(!l2_virt)) { likely() should only be used if it's really, really uncommon on a hot path. I don't think that's the case here. Can it even happen l2_virt is NULL here? l1_entry has been assigned while the l2 PT page has been mapped so both are either dummy / NULL or valid values. The l2 pages aren't unmapped in unmap, but only in ipu6_mmu_destroy(). Same goes for the map_single() if l2_virt was valid already, the page has been mapped, too. This seems like a pre-existing problem. If there is no actual bug here but just inconsistent implementation (so it would seem), I think it's fine to address this on top. Can you conform this? > + l2_virt = alloc_l2_pt(mmu_info); > + if (!l2_virt) { > + err = -ENOMEM; > + goto error; > + } > + } > > - dev_dbg(mmu_info->dev, "l2_idx %u, phys 0x%8.8x\n", l2_idx, > - l2_pt[l2_idx]); > - if (l2_pt[l2_idx] != mmu_info->dummy_page_pteval) { > - spin_unlock_irqrestore(&mmu_info->lock, flags); > - return -EINVAL; > + dma = map_single(mmu_info, l2_virt); > + if (!dma) { > + dev_err(dev, "Failed to map l2pt page\n"); > + free_page((unsigned long)l2_virt); > + err = -EINVAL; > + goto error; > + } > + > + l1_entry = dma >> ISP_PADDR_SHIFT; > + > + dev_dbg(dev, "page for l1_idx %u %p allocated\n", > + l1_idx, l2_virt); > + mmu_info->l1_pt[l1_idx] = l1_entry; > + mmu_info->l2_pts[l1_idx] = l2_virt; > + > + clflush_cache_range(&mmu_info->l1_pt[l1_idx], > + sizeof(mmu_info->l1_pt[l1_idx])); > + } > + > + l2_pt = mmu_info->l2_pts[l1_idx]; > + l2_entries = 0; > + > + for (l2_idx = (iova & ISP_L2PT_MASK) >> ISP_L2PT_SHIFT; > + size > 0 && l2_idx < ISP_L2PT_PTES; l2_idx++) { > + l2_pt[l2_idx] = paddr >> ISP_PADDR_SHIFT; > + > + dev_dbg(dev, "l2 index %u mapped as 0x%8.8x\n", l2_idx, > + l2_pt[l2_idx]); > + > + iova += ISP_PAGE_SIZE; > + paddr += ISP_PAGE_SIZE; > + mapped += ISP_PAGE_SIZE; > + size -= ISP_PAGE_SIZE; > + > + l2_entries++; > + } > + > + WARN_ON_ONCE(!l2_entries); > + clflush_cache_range(&l2_pt[l2_idx - l2_entries], > + sizeof(l2_pt[0]) * l2_entries); > } > > - l2_pt[l2_idx] = paddr >> ISP_PADDR_SHIFT; > - > - clflush_cache_range((void *)&l2_pt[l2_idx], sizeof(l2_pt[l2_idx])); > spin_unlock_irqrestore(&mmu_info->lock, flags); > > - dev_dbg(mmu_info->dev, "l2 index %u mapped as 0x%8.8x\n", l2_idx, > - l2_pt[l2_idx]); > - > return 0; > + > +error: > + spin_unlock_irqrestore(&mmu_info->lock, flags); > + /* unroll mapping in case something went wrong */ > + if (mapped) > + l2_unmap(mmu_info, iova - mapped, paddr - mapped, mapped); > + > + return err; > } > > static int __ipu6_mmu_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, > @@ -692,10 +710,7 @@ size_t ipu6_mmu_unmap(struct ipu6_mmu_info *mmu_info, unsigned long iova, > int ipu6_mmu_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, > phys_addr_t paddr, size_t size) > { > - unsigned long orig_iova = iova; > unsigned int min_pagesz; > - size_t orig_size = size; > - int ret = 0; > > if (mmu_info->pgsize_bitmap == 0UL) > return -ENODEV; > @@ -718,28 +733,7 @@ int ipu6_mmu_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, > dev_dbg(mmu_info->dev, "map: iova 0x%lx pa %pa size 0x%zx\n", > iova, &paddr, size); > > - while (size) { > - size_t pgsize = ipu6_mmu_pgsize(mmu_info->pgsize_bitmap, > - iova | paddr, size); > - > - dev_dbg(mmu_info->dev, > - "mapping: iova 0x%lx pa %pa pgsize 0x%zx\n", > - iova, &paddr, pgsize); > - > - ret = __ipu6_mmu_map(mmu_info, iova, paddr, pgsize); > - if (ret) > - break; > - > - iova += pgsize; > - paddr += pgsize; > - size -= pgsize; > - } > - > - /* unroll mapping in case something went wrong */ > - if (ret) > - ipu6_mmu_unmap(mmu_info, orig_iova, orig_size - size); > - > - return ret; > + return __ipu6_mmu_map(mmu_info, iova, paddr, size); > } > > static void ipu6_mmu_destroy(struct ipu6_mmu *mmu)
Sakari, On 11/1/24 9:16 PM, Sakari Ailus wrote: > Hi Bingbu, > > Thanks for the update. > > On Fri, Oct 25, 2024 at 01:52:31PM +0800, Bingbu Cao wrote: >> ipu6_mmu_map() operated on a per-page basis, it leads frequent >> spin_lock/unlock() and clflush_cache_range() for each page, it >> will cause inefficiencies especially when handling dma-bufs >> with large number of pages. However, the pages are likely concentrated >> pages by IOMMU DMA driver, IPU MMU driver can map the concentrated >> pages into less entries in l1 table. >> >> This change enhances ipu6_mmu_map() with batching process multiple >> contiguous pages. It significantly reduces calls for spin_lock/unlock >> and clflush_cache_range() and improve the performance. >> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >> Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com> >> --- >> drivers/media/pci/intel/ipu6/ipu6-mmu.c | 144 +++++++++++++++----------------- >> 1 file changed, 69 insertions(+), 75 deletions(-) >> >> diff --git a/drivers/media/pci/intel/ipu6/ipu6-mmu.c b/drivers/media/pci/intel/ipu6/ipu6-mmu.c >> index 9ea6789bca5e..e957ccb4691d 100644 >> --- a/drivers/media/pci/intel/ipu6/ipu6-mmu.c >> +++ b/drivers/media/pci/intel/ipu6/ipu6-mmu.c >> @@ -295,72 +295,90 @@ static size_t l2_unmap(struct ipu6_mmu_info *mmu_info, unsigned long iova, >> static int l2_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, >> phys_addr_t paddr, size_t size) >> { >> - u32 l1_idx = iova >> ISP_L1PT_SHIFT; >> - u32 iova_start = iova; >> + struct device *dev = mmu_info->dev; >> + unsigned int l2_entries; >> u32 *l2_pt, *l2_virt; >> unsigned int l2_idx; >> unsigned long flags; >> + size_t mapped = 0; >> dma_addr_t dma; >> u32 l1_entry; >> - >> - dev_dbg(mmu_info->dev, >> - "mapping l2 page table for l1 index %u (iova %8.8x)\n", >> - l1_idx, (u32)iova); >> + u32 l1_idx; >> + int err = 0; >> >> spin_lock_irqsave(&mmu_info->lock, flags); >> - l1_entry = mmu_info->l1_pt[l1_idx]; >> - if (l1_entry == mmu_info->dummy_l2_pteval) { >> - l2_virt = mmu_info->l2_pts[l1_idx]; >> - if (likely(!l2_virt)) { >> - l2_virt = alloc_l2_pt(mmu_info); >> - if (!l2_virt) { >> - spin_unlock_irqrestore(&mmu_info->lock, flags); >> - return -ENOMEM; >> - } >> - } >> - >> - dma = map_single(mmu_info, l2_virt); >> - if (!dma) { >> - dev_err(mmu_info->dev, "Failed to map l2pt page\n"); >> - free_page((unsigned long)l2_virt); >> - spin_unlock_irqrestore(&mmu_info->lock, flags); >> - return -EINVAL; >> - } >> - >> - l1_entry = dma >> ISP_PADDR_SHIFT; >> - >> - dev_dbg(mmu_info->dev, "page for l1_idx %u %p allocated\n", >> - l1_idx, l2_virt); >> - mmu_info->l1_pt[l1_idx] = l1_entry; >> - mmu_info->l2_pts[l1_idx] = l2_virt; >> - clflush_cache_range((void *)&mmu_info->l1_pt[l1_idx], >> - sizeof(mmu_info->l1_pt[l1_idx])); >> - } >> - >> - l2_pt = mmu_info->l2_pts[l1_idx]; >> - >> - dev_dbg(mmu_info->dev, "l2_pt at %p with dma 0x%x\n", l2_pt, l1_entry); >> >> paddr = ALIGN(paddr, ISP_PAGE_SIZE); >> + for (l1_idx = iova >> ISP_L1PT_SHIFT; >> + size > 0 && l1_idx < ISP_L1PT_PTES; l1_idx++) { >> + dev_dbg(dev, >> + "mapping l2 page table for l1 index %u (iova %8.8x)\n", >> + l1_idx, (u32)iova); >> >> - l2_idx = (iova_start & ISP_L2PT_MASK) >> ISP_L2PT_SHIFT; >> + l1_entry = mmu_info->l1_pt[l1_idx]; >> + if (l1_entry == mmu_info->dummy_l2_pteval) { >> + l2_virt = mmu_info->l2_pts[l1_idx]; >> + if (likely(!l2_virt)) { > > likely() should only be used if it's really, really uncommon on a hot path. > I don't think that's the case here. > > Can it even happen l2_virt is NULL here? l1_entry has been assigned while > the l2 PT page has been mapped so both are either dummy / NULL or valid > values. The l2 pages aren't unmapped in unmap, but only in > ipu6_mmu_destroy(). Same goes for the map_single() if l2_virt was valid > already, the page has been mapped, too. > > This seems like a pre-existing problem. If there is no actual bug here but > just inconsistent implementation (so it would seem), I think it's fine to > address this on top. > > Can you conform this? I would like to address this in another patch instead of this series. What do you think? > >> + l2_virt = alloc_l2_pt(mmu_info); >> + if (!l2_virt) { >> + err = -ENOMEM; >> + goto error; >> + } >> + } >> >> - dev_dbg(mmu_info->dev, "l2_idx %u, phys 0x%8.8x\n", l2_idx, >> - l2_pt[l2_idx]); >> - if (l2_pt[l2_idx] != mmu_info->dummy_page_pteval) { >> - spin_unlock_irqrestore(&mmu_info->lock, flags); >> - return -EINVAL; >> + dma = map_single(mmu_info, l2_virt); >> + if (!dma) { >> + dev_err(dev, "Failed to map l2pt page\n"); >> + free_page((unsigned long)l2_virt); >> + err = -EINVAL; >> + goto error; >> + } >> + >> + l1_entry = dma >> ISP_PADDR_SHIFT; >> + >> + dev_dbg(dev, "page for l1_idx %u %p allocated\n", >> + l1_idx, l2_virt); >> + mmu_info->l1_pt[l1_idx] = l1_entry; >> + mmu_info->l2_pts[l1_idx] = l2_virt; >> + >> + clflush_cache_range(&mmu_info->l1_pt[l1_idx], >> + sizeof(mmu_info->l1_pt[l1_idx])); >> + } >> + >> + l2_pt = mmu_info->l2_pts[l1_idx]; >> + l2_entries = 0; >> + >> + for (l2_idx = (iova & ISP_L2PT_MASK) >> ISP_L2PT_SHIFT; >> + size > 0 && l2_idx < ISP_L2PT_PTES; l2_idx++) { >> + l2_pt[l2_idx] = paddr >> ISP_PADDR_SHIFT; >> + >> + dev_dbg(dev, "l2 index %u mapped as 0x%8.8x\n", l2_idx, >> + l2_pt[l2_idx]); >> + >> + iova += ISP_PAGE_SIZE; >> + paddr += ISP_PAGE_SIZE; >> + mapped += ISP_PAGE_SIZE; >> + size -= ISP_PAGE_SIZE; >> + >> + l2_entries++; >> + } >> + >> + WARN_ON_ONCE(!l2_entries); >> + clflush_cache_range(&l2_pt[l2_idx - l2_entries], >> + sizeof(l2_pt[0]) * l2_entries); >> } >> >> - l2_pt[l2_idx] = paddr >> ISP_PADDR_SHIFT; >> - >> - clflush_cache_range((void *)&l2_pt[l2_idx], sizeof(l2_pt[l2_idx])); >> spin_unlock_irqrestore(&mmu_info->lock, flags); >> >> - dev_dbg(mmu_info->dev, "l2 index %u mapped as 0x%8.8x\n", l2_idx, >> - l2_pt[l2_idx]); >> - >> return 0; >> + >> +error: >> + spin_unlock_irqrestore(&mmu_info->lock, flags); >> + /* unroll mapping in case something went wrong */ >> + if (mapped) >> + l2_unmap(mmu_info, iova - mapped, paddr - mapped, mapped); >> + >> + return err; >> } >> >> static int __ipu6_mmu_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, >> @@ -692,10 +710,7 @@ size_t ipu6_mmu_unmap(struct ipu6_mmu_info *mmu_info, unsigned long iova, >> int ipu6_mmu_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, >> phys_addr_t paddr, size_t size) >> { >> - unsigned long orig_iova = iova; >> unsigned int min_pagesz; >> - size_t orig_size = size; >> - int ret = 0; >> >> if (mmu_info->pgsize_bitmap == 0UL) >> return -ENODEV; >> @@ -718,28 +733,7 @@ int ipu6_mmu_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, >> dev_dbg(mmu_info->dev, "map: iova 0x%lx pa %pa size 0x%zx\n", >> iova, &paddr, size); >> >> - while (size) { >> - size_t pgsize = ipu6_mmu_pgsize(mmu_info->pgsize_bitmap, >> - iova | paddr, size); >> - >> - dev_dbg(mmu_info->dev, >> - "mapping: iova 0x%lx pa %pa pgsize 0x%zx\n", >> - iova, &paddr, pgsize); >> - >> - ret = __ipu6_mmu_map(mmu_info, iova, paddr, pgsize); >> - if (ret) >> - break; >> - >> - iova += pgsize; >> - paddr += pgsize; >> - size -= pgsize; >> - } >> - >> - /* unroll mapping in case something went wrong */ >> - if (ret) >> - ipu6_mmu_unmap(mmu_info, orig_iova, orig_size - size); >> - >> - return ret; >> + return __ipu6_mmu_map(mmu_info, iova, paddr, size); >> } >> >> static void ipu6_mmu_destroy(struct ipu6_mmu *mmu) >
Hi Bingbu, On Mon, Nov 04, 2024 at 12:04:29PM +0800, Bingbu Cao wrote: > Sakari, > > On 11/1/24 9:16 PM, Sakari Ailus wrote: > > Hi Bingbu, > > > > Thanks for the update. > > > > On Fri, Oct 25, 2024 at 01:52:31PM +0800, Bingbu Cao wrote: > >> ipu6_mmu_map() operated on a per-page basis, it leads frequent > >> spin_lock/unlock() and clflush_cache_range() for each page, it > >> will cause inefficiencies especially when handling dma-bufs > >> with large number of pages. However, the pages are likely concentrated > >> pages by IOMMU DMA driver, IPU MMU driver can map the concentrated > >> pages into less entries in l1 table. > >> > >> This change enhances ipu6_mmu_map() with batching process multiple > >> contiguous pages. It significantly reduces calls for spin_lock/unlock > >> and clflush_cache_range() and improve the performance. > >> > >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > >> Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com> > >> --- > >> drivers/media/pci/intel/ipu6/ipu6-mmu.c | 144 +++++++++++++++----------------- > >> 1 file changed, 69 insertions(+), 75 deletions(-) > >> > >> diff --git a/drivers/media/pci/intel/ipu6/ipu6-mmu.c b/drivers/media/pci/intel/ipu6/ipu6-mmu.c > >> index 9ea6789bca5e..e957ccb4691d 100644 > >> --- a/drivers/media/pci/intel/ipu6/ipu6-mmu.c > >> +++ b/drivers/media/pci/intel/ipu6/ipu6-mmu.c > >> @@ -295,72 +295,90 @@ static size_t l2_unmap(struct ipu6_mmu_info *mmu_info, unsigned long iova, > >> static int l2_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, > >> phys_addr_t paddr, size_t size) > >> { > >> - u32 l1_idx = iova >> ISP_L1PT_SHIFT; > >> - u32 iova_start = iova; > >> + struct device *dev = mmu_info->dev; > >> + unsigned int l2_entries; > >> u32 *l2_pt, *l2_virt; > >> unsigned int l2_idx; > >> unsigned long flags; > >> + size_t mapped = 0; > >> dma_addr_t dma; > >> u32 l1_entry; > >> - > >> - dev_dbg(mmu_info->dev, > >> - "mapping l2 page table for l1 index %u (iova %8.8x)\n", > >> - l1_idx, (u32)iova); > >> + u32 l1_idx; > >> + int err = 0; > >> > >> spin_lock_irqsave(&mmu_info->lock, flags); > >> - l1_entry = mmu_info->l1_pt[l1_idx]; > >> - if (l1_entry == mmu_info->dummy_l2_pteval) { > >> - l2_virt = mmu_info->l2_pts[l1_idx]; > >> - if (likely(!l2_virt)) { > >> - l2_virt = alloc_l2_pt(mmu_info); > >> - if (!l2_virt) { > >> - spin_unlock_irqrestore(&mmu_info->lock, flags); > >> - return -ENOMEM; > >> - } > >> - } > >> - > >> - dma = map_single(mmu_info, l2_virt); > >> - if (!dma) { > >> - dev_err(mmu_info->dev, "Failed to map l2pt page\n"); > >> - free_page((unsigned long)l2_virt); > >> - spin_unlock_irqrestore(&mmu_info->lock, flags); > >> - return -EINVAL; > >> - } > >> - > >> - l1_entry = dma >> ISP_PADDR_SHIFT; > >> - > >> - dev_dbg(mmu_info->dev, "page for l1_idx %u %p allocated\n", > >> - l1_idx, l2_virt); > >> - mmu_info->l1_pt[l1_idx] = l1_entry; > >> - mmu_info->l2_pts[l1_idx] = l2_virt; > >> - clflush_cache_range((void *)&mmu_info->l1_pt[l1_idx], > >> - sizeof(mmu_info->l1_pt[l1_idx])); > >> - } > >> - > >> - l2_pt = mmu_info->l2_pts[l1_idx]; > >> - > >> - dev_dbg(mmu_info->dev, "l2_pt at %p with dma 0x%x\n", l2_pt, l1_entry); > >> > >> paddr = ALIGN(paddr, ISP_PAGE_SIZE); > >> + for (l1_idx = iova >> ISP_L1PT_SHIFT; > >> + size > 0 && l1_idx < ISP_L1PT_PTES; l1_idx++) { > >> + dev_dbg(dev, > >> + "mapping l2 page table for l1 index %u (iova %8.8x)\n", > >> + l1_idx, (u32)iova); > >> > >> - l2_idx = (iova_start & ISP_L2PT_MASK) >> ISP_L2PT_SHIFT; > >> + l1_entry = mmu_info->l1_pt[l1_idx]; > >> + if (l1_entry == mmu_info->dummy_l2_pteval) { > >> + l2_virt = mmu_info->l2_pts[l1_idx]; > >> + if (likely(!l2_virt)) { > > > > likely() should only be used if it's really, really uncommon on a hot path. > > I don't think that's the case here. > > > > Can it even happen l2_virt is NULL here? l1_entry has been assigned while > > the l2 PT page has been mapped so both are either dummy / NULL or valid > > values. The l2 pages aren't unmapped in unmap, but only in > > ipu6_mmu_destroy(). Same goes for the map_single() if l2_virt was valid > > already, the page has been mapped, too. > > > > This seems like a pre-existing problem. If there is no actual bug here but > > just inconsistent implementation (so it would seem), I think it's fine to > > address this on top. > > > > Can you conform this? > > I would like to address this in another patch instead of this series. > What do you think? Works for me.
diff --git a/drivers/media/pci/intel/ipu6/ipu6-mmu.c b/drivers/media/pci/intel/ipu6/ipu6-mmu.c index 9ea6789bca5e..e957ccb4691d 100644 --- a/drivers/media/pci/intel/ipu6/ipu6-mmu.c +++ b/drivers/media/pci/intel/ipu6/ipu6-mmu.c @@ -295,72 +295,90 @@ static size_t l2_unmap(struct ipu6_mmu_info *mmu_info, unsigned long iova, static int l2_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, phys_addr_t paddr, size_t size) { - u32 l1_idx = iova >> ISP_L1PT_SHIFT; - u32 iova_start = iova; + struct device *dev = mmu_info->dev; + unsigned int l2_entries; u32 *l2_pt, *l2_virt; unsigned int l2_idx; unsigned long flags; + size_t mapped = 0; dma_addr_t dma; u32 l1_entry; - - dev_dbg(mmu_info->dev, - "mapping l2 page table for l1 index %u (iova %8.8x)\n", - l1_idx, (u32)iova); + u32 l1_idx; + int err = 0; spin_lock_irqsave(&mmu_info->lock, flags); - l1_entry = mmu_info->l1_pt[l1_idx]; - if (l1_entry == mmu_info->dummy_l2_pteval) { - l2_virt = mmu_info->l2_pts[l1_idx]; - if (likely(!l2_virt)) { - l2_virt = alloc_l2_pt(mmu_info); - if (!l2_virt) { - spin_unlock_irqrestore(&mmu_info->lock, flags); - return -ENOMEM; - } - } - - dma = map_single(mmu_info, l2_virt); - if (!dma) { - dev_err(mmu_info->dev, "Failed to map l2pt page\n"); - free_page((unsigned long)l2_virt); - spin_unlock_irqrestore(&mmu_info->lock, flags); - return -EINVAL; - } - - l1_entry = dma >> ISP_PADDR_SHIFT; - - dev_dbg(mmu_info->dev, "page for l1_idx %u %p allocated\n", - l1_idx, l2_virt); - mmu_info->l1_pt[l1_idx] = l1_entry; - mmu_info->l2_pts[l1_idx] = l2_virt; - clflush_cache_range((void *)&mmu_info->l1_pt[l1_idx], - sizeof(mmu_info->l1_pt[l1_idx])); - } - - l2_pt = mmu_info->l2_pts[l1_idx]; - - dev_dbg(mmu_info->dev, "l2_pt at %p with dma 0x%x\n", l2_pt, l1_entry); paddr = ALIGN(paddr, ISP_PAGE_SIZE); + for (l1_idx = iova >> ISP_L1PT_SHIFT; + size > 0 && l1_idx < ISP_L1PT_PTES; l1_idx++) { + dev_dbg(dev, + "mapping l2 page table for l1 index %u (iova %8.8x)\n", + l1_idx, (u32)iova); - l2_idx = (iova_start & ISP_L2PT_MASK) >> ISP_L2PT_SHIFT; + l1_entry = mmu_info->l1_pt[l1_idx]; + if (l1_entry == mmu_info->dummy_l2_pteval) { + l2_virt = mmu_info->l2_pts[l1_idx]; + if (likely(!l2_virt)) { + l2_virt = alloc_l2_pt(mmu_info); + if (!l2_virt) { + err = -ENOMEM; + goto error; + } + } - dev_dbg(mmu_info->dev, "l2_idx %u, phys 0x%8.8x\n", l2_idx, - l2_pt[l2_idx]); - if (l2_pt[l2_idx] != mmu_info->dummy_page_pteval) { - spin_unlock_irqrestore(&mmu_info->lock, flags); - return -EINVAL; + dma = map_single(mmu_info, l2_virt); + if (!dma) { + dev_err(dev, "Failed to map l2pt page\n"); + free_page((unsigned long)l2_virt); + err = -EINVAL; + goto error; + } + + l1_entry = dma >> ISP_PADDR_SHIFT; + + dev_dbg(dev, "page for l1_idx %u %p allocated\n", + l1_idx, l2_virt); + mmu_info->l1_pt[l1_idx] = l1_entry; + mmu_info->l2_pts[l1_idx] = l2_virt; + + clflush_cache_range(&mmu_info->l1_pt[l1_idx], + sizeof(mmu_info->l1_pt[l1_idx])); + } + + l2_pt = mmu_info->l2_pts[l1_idx]; + l2_entries = 0; + + for (l2_idx = (iova & ISP_L2PT_MASK) >> ISP_L2PT_SHIFT; + size > 0 && l2_idx < ISP_L2PT_PTES; l2_idx++) { + l2_pt[l2_idx] = paddr >> ISP_PADDR_SHIFT; + + dev_dbg(dev, "l2 index %u mapped as 0x%8.8x\n", l2_idx, + l2_pt[l2_idx]); + + iova += ISP_PAGE_SIZE; + paddr += ISP_PAGE_SIZE; + mapped += ISP_PAGE_SIZE; + size -= ISP_PAGE_SIZE; + + l2_entries++; + } + + WARN_ON_ONCE(!l2_entries); + clflush_cache_range(&l2_pt[l2_idx - l2_entries], + sizeof(l2_pt[0]) * l2_entries); } - l2_pt[l2_idx] = paddr >> ISP_PADDR_SHIFT; - - clflush_cache_range((void *)&l2_pt[l2_idx], sizeof(l2_pt[l2_idx])); spin_unlock_irqrestore(&mmu_info->lock, flags); - dev_dbg(mmu_info->dev, "l2 index %u mapped as 0x%8.8x\n", l2_idx, - l2_pt[l2_idx]); - return 0; + +error: + spin_unlock_irqrestore(&mmu_info->lock, flags); + /* unroll mapping in case something went wrong */ + if (mapped) + l2_unmap(mmu_info, iova - mapped, paddr - mapped, mapped); + + return err; } static int __ipu6_mmu_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, @@ -692,10 +710,7 @@ size_t ipu6_mmu_unmap(struct ipu6_mmu_info *mmu_info, unsigned long iova, int ipu6_mmu_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, phys_addr_t paddr, size_t size) { - unsigned long orig_iova = iova; unsigned int min_pagesz; - size_t orig_size = size; - int ret = 0; if (mmu_info->pgsize_bitmap == 0UL) return -ENODEV; @@ -718,28 +733,7 @@ int ipu6_mmu_map(struct ipu6_mmu_info *mmu_info, unsigned long iova, dev_dbg(mmu_info->dev, "map: iova 0x%lx pa %pa size 0x%zx\n", iova, &paddr, size); - while (size) { - size_t pgsize = ipu6_mmu_pgsize(mmu_info->pgsize_bitmap, - iova | paddr, size); - - dev_dbg(mmu_info->dev, - "mapping: iova 0x%lx pa %pa pgsize 0x%zx\n", - iova, &paddr, pgsize); - - ret = __ipu6_mmu_map(mmu_info, iova, paddr, pgsize); - if (ret) - break; - - iova += pgsize; - paddr += pgsize; - size -= pgsize; - } - - /* unroll mapping in case something went wrong */ - if (ret) - ipu6_mmu_unmap(mmu_info, orig_iova, orig_size - size); - - return ret; + return __ipu6_mmu_map(mmu_info, iova, paddr, size); } static void ipu6_mmu_destroy(struct ipu6_mmu *mmu)