Sakari,
Thanks for the review.
On 10/24/24 6:34 PM, Sakari Ailus wrote:
> Hi Bingbu,
>
> Thank you for the update.
>
> On Thu, Oct 24, 2024 at 03:24:36PM +0800, Bingbu Cao wrote:
>> l2_map() and l2_unmap() are better to be grouped together, and
>> l2_unmap() could also be called in l2_map() for error handling.
>
> Can you confirm there are no changes to the function itself? This should be
> part of the commit message.
Yes, the l2_unmap() here is not changed.
>
> I'd also add that l2_unmap() will soon be called from l2_map(), rather than
> being something that could be done.
I will add that into v2.
>
>>
>> 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 | 80 ++++++++++++++++-----------------
>> 1 file changed, 40 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-mmu.c b/drivers/media/pci/intel/ipu6/ipu6-mmu.c
>> index c3a20507d6db..9ea6789bca5e 100644
>> --- a/drivers/media/pci/intel/ipu6/ipu6-mmu.c
>> +++ b/drivers/media/pci/intel/ipu6/ipu6-mmu.c
>> @@ -252,6 +252,46 @@ static u32 *alloc_l2_pt(struct ipu6_mmu_info *mmu_info)
>> return pt;
>> }
>>
>> +static size_t l2_unmap(struct ipu6_mmu_info *mmu_info, unsigned long iova,
>> + phys_addr_t dummy, size_t size)
>> +{
>> + u32 l1_idx = iova >> ISP_L1PT_SHIFT;
>> + u32 iova_start = iova;
>> + unsigned int l2_idx;
>> + size_t unmapped = 0;
>> + unsigned long flags;
>> + u32 *l2_pt;
>> +
>> + dev_dbg(mmu_info->dev, "unmapping l2 page table for l1 index %u (iova 0x%8.8lx)\n",
>> + l1_idx, iova);
>> +
>> + spin_lock_irqsave(&mmu_info->lock, flags);
>> + if (mmu_info->l1_pt[l1_idx] == mmu_info->dummy_l2_pteval) {
>> + spin_unlock_irqrestore(&mmu_info->lock, flags);
>> + dev_err(mmu_info->dev,
>> + "unmap iova 0x%8.8lx l1 idx %u which was not mapped\n",
>> + iova, l1_idx);
>> + return 0;
>> + }
>> +
>> + for (l2_idx = (iova_start & ISP_L2PT_MASK) >> ISP_L2PT_SHIFT;
>> + (iova_start & ISP_L1PT_MASK) + (l2_idx << ISP_PAGE_SHIFT)
>> + < iova_start + size && l2_idx < ISP_L2PT_PTES; l2_idx++) {
>> + l2_pt = mmu_info->l2_pts[l1_idx];
>> + dev_dbg(mmu_info->dev,
>> + "unmap l2 index %u with pteval 0x%10.10llx\n",
>> + l2_idx, TBL_PHYS_ADDR(l2_pt[l2_idx]));
>> + l2_pt[l2_idx] = mmu_info->dummy_page_pteval;
>> +
>> + clflush_cache_range((void *)&l2_pt[l2_idx],
>> + sizeof(l2_pt[l2_idx]));
>> + unmapped++;
>> + }
>> + spin_unlock_irqrestore(&mmu_info->lock, flags);
>> +
>> + return unmapped << ISP_PAGE_SHIFT;
>> +}
>> +
>> static int l2_map(struct ipu6_mmu_info *mmu_info, unsigned long iova,
>> phys_addr_t paddr, size_t size)
>> {
>> @@ -336,46 +376,6 @@ static int __ipu6_mmu_map(struct ipu6_mmu_info *mmu_info, unsigned long iova,
>> return l2_map(mmu_info, iova_start, paddr, size);
>> }
>>
>> -static size_t l2_unmap(struct ipu6_mmu_info *mmu_info, unsigned long iova,
>> - phys_addr_t dummy, size_t size)
>> -{
>> - u32 l1_idx = iova >> ISP_L1PT_SHIFT;
>> - u32 iova_start = iova;
>> - unsigned int l2_idx;
>> - size_t unmapped = 0;
>> - unsigned long flags;
>> - u32 *l2_pt;
>> -
>> - dev_dbg(mmu_info->dev, "unmapping l2 page table for l1 index %u (iova 0x%8.8lx)\n",
>> - l1_idx, iova);
>> -
>> - spin_lock_irqsave(&mmu_info->lock, flags);
>> - if (mmu_info->l1_pt[l1_idx] == mmu_info->dummy_l2_pteval) {
>> - spin_unlock_irqrestore(&mmu_info->lock, flags);
>> - dev_err(mmu_info->dev,
>> - "unmap iova 0x%8.8lx l1 idx %u which was not mapped\n",
>> - iova, l1_idx);
>> - return 0;
>> - }
>> -
>> - for (l2_idx = (iova_start & ISP_L2PT_MASK) >> ISP_L2PT_SHIFT;
>> - (iova_start & ISP_L1PT_MASK) + (l2_idx << ISP_PAGE_SHIFT)
>> - < iova_start + size && l2_idx < ISP_L2PT_PTES; l2_idx++) {
>> - l2_pt = mmu_info->l2_pts[l1_idx];
>> - dev_dbg(mmu_info->dev,
>> - "unmap l2 index %u with pteval 0x%10.10llx\n",
>> - l2_idx, TBL_PHYS_ADDR(l2_pt[l2_idx]));
>> - l2_pt[l2_idx] = mmu_info->dummy_page_pteval;
>> -
>> - clflush_cache_range((void *)&l2_pt[l2_idx],
>> - sizeof(l2_pt[l2_idx]));
>> - unmapped++;
>> - }
>> - spin_unlock_irqrestore(&mmu_info->lock, flags);
>> -
>> - return unmapped << ISP_PAGE_SHIFT;
>> -}
>> -
>> static size_t __ipu6_mmu_unmap(struct ipu6_mmu_info *mmu_info,
>> unsigned long iova, size_t size)
>> {
>> --
>> 2.7.4
>>
>
--
Best regards,
Bingbu Cao
