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@xxxxxxxxx>
> >> Signed-off-by: Jianhui Dai <jianhui.j.dai@xxxxxxxxx>
> >> ---
> >> 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.
--
Sakari Ailus
