Re: [PATCH RFC 12/18] accel/qda: Add PRIME dma-buf import support

[Christian König]

On 4/2/26 10:36, Ekansh Gupta wrote:
> On 3/9/2026 12:29 PM, Ekansh Gupta wrote:
>>
>> On 2/24/2026 2:42 PM, Christian König wrote:
>>> On 2/23/26 20:09, Ekansh Gupta wrote:
>>>> [Sie erhalten nicht häufig E-Mails von ekansh.gupta@xxxxxxxxxxxxxxxx. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ]
>>>>
>>>> Add PRIME dma-buf import support for QDA GEM buffer objects and integrate
>>>> it with the existing per-process memory manager and IOMMU device model.
>>>>
>>>> The implementation extends qda_gem_obj to represent imported dma-bufs,
>>>> including dma_buf references, attachment state, scatter-gather tables
>>>> and an imported DMA address used for DSP-facing book-keeping. The
>>>> qda_gem_prime_import() path handles reimports of buffers originally
>>>> exported by QDA as well as imports of external dma-bufs, attaching them
>>>> to the assigned IOMMU device
>>> That is usually an absolutely clear NO-GO for DMA-bufs. Where exactly in the code is that?
>> dma_buf_attach* to comute-cb iommu devices are critical for DSPs to access the buffer.
>> This is needed if the buffer is exported by anyone other than QDA(say system heap). If this is not
>> the correct way, what should be the right way here? On the current fastrpc driver also,
>> the DMABUF is getting attached with iommu device[1] due to the same requirement.
>>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n779
> 
> Hi Christian,
> 
> Do you have any suggestions for the shared requirements?

Well I don't fully understand what you are trying to do with the iommu. Usually it is the job of the exporter to provide the importer with DMA addresses which are valid for its device structure, that includes IOMMU mapping.

Can you iterate what exactly this iommu group is and why you have to attach the imported buffers to it, how that attachment works and how lifetime is managed?

Regards,
Christian.

> 
> I'm reworking on the next version and currently I don't see any other way
> to handle dma_buf_attach* cases.
> 
> //Ekansh
> 
>>>> and mapping them through the memory manager
>>>> for DSP access. The GEM free path is updated to unmap and detach
>>>> imported buffers while preserving the existing behaviour for locally
>>>> allocated memory.
>>>>
>>>> The PRIME fd-to-handle path is implemented in qda_prime_fd_to_handle(),
>>>> which records the calling drm_file in a driver-private import context
>>>> before invoking the core DRM helpers. The GEM import callback retrieves
>>>> this context to ensure that an IOMMU device is assigned to the process
>>>> and that imported buffers follow the same per-process IOMMU selection
>>>> rules as natively allocated GEM objects.
>>>>
>>>> This patch prepares the driver for interoperable buffer sharing between
>>>> QDA and other dma-buf capable subsystems while keeping IOMMU mapping and
>>>> lifetime handling consistent with the existing GEM allocation flow.
>>>>
>>>> Signed-off-by: Ekansh Gupta <ekansh.gupta@xxxxxxxxxxxxxxxx>
>>> ...
>>>
>>>> @@ -15,23 +16,29 @@ static int validate_gem_obj_for_mmap(struct qda_gem_obj *qda_gem_obj)
>>>>                 qda_err(NULL, "Invalid GEM object size\n");
>>>>                 return -EINVAL;
>>>>         }
>>>> -       if (!qda_gem_obj->iommu_dev || !qda_gem_obj->iommu_dev->dev) {
>>>> -               qda_err(NULL, "Allocated buffer missing IOMMU device\n");
>>>> -               return -EINVAL;
>>>> -       }
>>>> -       if (!qda_gem_obj->iommu_dev->dev) {
>>>> -               qda_err(NULL, "Allocated buffer missing IOMMU device\n");
>>>> -               return -EINVAL;
>>>> -       }
>>>> -       if (!qda_gem_obj->virt) {
>>>> -               qda_err(NULL, "Allocated buffer missing virtual address\n");
>>>> -               return -EINVAL;
>>>> -       }
>>>> -       if (qda_gem_obj->dma_addr == 0) {
>>>> -               qda_err(NULL, "Allocated buffer missing DMA address\n");
>>>> -               return -EINVAL;
>>>> +       if (qda_gem_obj->is_imported) {
>>> Absolutely clear NAK to that. Imported buffers *can't* be mmaped through the importer!
>>>
>>> Userspace needs to mmap() them through the exporter.
>>>
>>> If you absolutely have to map them through the importer for uAPI backward compatibility then there is dma_buf_mmap() for that, but this is clearly not the case here.
>>>
>>> ...
>> Okay, the requirement is slightly different here. Any buffer which is not allocated using the
>> QDA GEM interface needs to be attached to the iommu device for that particular process to
>> enable DSP for the access. I should not call it `mmap` instead it should be called importing the
>> buffer to a particular iommu context bank. With this definition, is it fine to keep it this way? Or
>> should the dma_buf_attach* calls be moved to some other place?
>>>> +static int qda_memory_manager_map_imported(struct qda_memory_manager *mem_mgr,
>>>> +                                          struct qda_gem_obj *gem_obj,
>>>> +                                          struct qda_iommu_device *iommu_dev)
>>>> +{
>>>> +       struct scatterlist *sg;
>>>> +       dma_addr_t dma_addr;
>>>> +       int ret = 0;
>>>> +
>>>> +       if (!gem_obj->is_imported || !gem_obj->sgt || !iommu_dev) {
>>>> +               qda_err(NULL, "Invalid parameters for imported buffer mapping\n");
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       gem_obj->iommu_dev = iommu_dev;
>>>> +
>>>> +       sg = gem_obj->sgt->sgl;
>>>> +       if (sg) {
>>>> +               dma_addr = sg_dma_address(sg);
>>>> +               dma_addr += ((u64)iommu_dev->sid << 32);
>>>> +
>>>> +               gem_obj->imported_dma_addr = dma_addr;
>>> Well that looks like you are only using the first DMA address from the imported sgt. What about the others?
>> I might have a proper appach for this now, will update in the next spin.
>>> Regards,
>>> Christian.
>