[v2,2/3] media: ipu6: optimize the IPU6 MMU mapping flow

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(-)

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.