Re: [PATCH 1/1] media: ipu6: Drop unused ipu6_dma_get_sgtable()

[Bingbu Cao <bingbu.cao@linux.intel.com>]

Sakari,

On 2/26/25 4:39 PM, Sakari Ailus wrote:
> Hi Bingbu,
> 
> On Wed, Feb 26, 2025 at 04:01:16PM +0800, Bingbu Cao wrote:
>>
>> On 2/26/25 3:58 PM, Sakari Ailus wrote:
>>> Hi Bingbu,
>>>
>>> On Wed, Feb 26, 2025 at 10:38:25AM +0800, Bingbu Cao wrote:
>>>> Sakari,
>>>>
>>>> On 2/25/25 5:45 PM, Sakari Ailus wrote:
>>>>> ipu6_dma_get_sgtable() is now unused. Drop it.
>>>>>
>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>> ---
>>>>>  drivers/media/pci/intel/ipu6/ipu6-dma.c | 33 -------------------------
>>>>>  drivers/media/pci/intel/ipu6/ipu6-dma.h |  3 ---
>>>>>  2 files changed, 36 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-dma.c b/drivers/media/pci/intel/ipu6/ipu6-dma.c
>>>>> index 44e24da33907..052e396a28af 100644
>>>>> --- a/drivers/media/pci/intel/ipu6/ipu6-dma.c
>>>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-dma.c
>>>>> @@ -455,36 +455,3 @@ void ipu6_dma_unmap_sgtable(struct ipu6_bus_device *sys, struct sg_table *sgt,
>>>>>  	ipu6_dma_unmap_sg(sys, sgt->sgl, sgt->nents, dir, attrs);
>>>>>  }
>>>>>  EXPORT_SYMBOL_NS_GPL(ipu6_dma_unmap_sgtable, "INTEL_IPU6");
>>>>> -
>>>>> -/*
>>>>> - * Create scatter-list for the already allocated DMA buffer
>>>>> - */
>>>>> -int ipu6_dma_get_sgtable(struct ipu6_bus_device *sys, struct sg_table *sgt,
>>>>> -			 void *cpu_addr, dma_addr_t handle, size_t size,
>>>>> -			 unsigned long attrs)
>>>>> -{
>>>>> -	struct device *dev = &sys->auxdev.dev;
>>>>> -	struct ipu6_mmu *mmu = sys->mmu;
>>>>> -	struct vm_info *info;
>>>>> -	int n_pages;
>>>>> -	int ret = 0;
>>>>> -
>>>>> -	info = get_vm_info(mmu, handle);
>>>>> -	if (!info)
>>>>> -		return -EFAULT;
>>>>> -
>>>>> -	if (!info->vaddr)
>>>>> -		return -EFAULT;
>>>>> -
>>>>> -	if (WARN_ON(!info->pages))
>>>>> -		return -ENOMEM;
>>>>> -
>>>>> -	n_pages = PFN_UP(size);
>>>>> -
>>>>> -	ret = sg_alloc_table_from_pages(sgt, info->pages, n_pages, 0, size,
>>>>> -					GFP_KERNEL);
>>>>> -	if (ret)
>>>>> -		dev_warn(dev, "get sgt table failed\n");
>>>>> -
>>>>> -	return ret;
>>>>> -}
>>>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-dma.h b/drivers/media/pci/intel/ipu6/ipu6-dma.h
>>>>> index b51244add9e6..2882850d9366 100644
>>>>> --- a/drivers/media/pci/intel/ipu6/ipu6-dma.h
>>>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-dma.h
>>>>> @@ -43,7 +43,4 @@ int ipu6_dma_map_sgtable(struct ipu6_bus_device *sys, struct sg_table *sgt,
>>>>>  			 enum dma_data_direction dir, unsigned long attrs);
>>>>>  void ipu6_dma_unmap_sgtable(struct ipu6_bus_device *sys, struct sg_table *sgt,
>>>>>  			    enum dma_data_direction dir, unsigned long attrs);
>>>>> -int ipu6_dma_get_sgtable(struct ipu6_bus_device *sys, struct sg_table *sgt,
>>>>> -			 void *cpu_addr, dma_addr_t handle, size_t size,
>>>>> -			 unsigned long attrs);
>>>>
>>>> ipu6_dma_get_sgtable() is used in downstream PS driver, I don't think it
>>>> is necessary to remove it as it will be hard to integrate with downstream PS
>>>> driver.
>>>
>>> When will there be an upstream PSYS driver?
>>>
>>> I suppose you can just as well apply a revert of this patch to your
>>> downstream tree?
>>>
>>
>> Sakari,
>>
>> I don't think all the OS vendors like this way, it make the downstream
>> development harder. It's a trade-off, I cannot see removing offer any
>> benefits that its side-effect.
> 
> I don't think the Linux distro maintainers like downstream drivers at all,
> independently of how many patches they consist of.
> 
> You could as well move the function to the PSYS driver and use
> find_vm_area(cpu_addr) instead to find the area.

My fault, I forgot I already removed the ipu6_dma_get_sgtable() from
downstream driver months ago, so,

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

;)

> 

-- 
Best regards,
Bingbu Cao