On 14/09/2023 22:33, Robin Murphy wrote: > On 2023-09-14 18:57, Michael Tretter wrote: >> Hi Robin, >> >> On Thu, 14 Sep 2023 16:06:27 +0100, Robin Murphy wrote: >>> On 2023-09-14 13:40, Michael Tretter wrote: >>>> The IOMMU of the RGA is programmed with a list of DMA descriptors that >>>> contain an 32 bit address per 4k page in the video buffers. The address >>>> in the descriptor points to the start address of the page. >>>> >>>> Introduce 'struct rga_dma_desc' to make the handling of the DMA >>>> descriptors explicit instead of hiding them behind standard types. >>>> >>>> As the descriptors only handle 32 bit addresses, addresses above 4 GB >>>> cannot be addressed. If this is detected, stop filling the descriptor >>>> list and report an error. >>> >>> That sounds unnecessary, since the only DMA addresses the RGA should be >>> seeing are those from a dma_map_sg() call using the RGA device itself, and >>> that would have failed if it was unable to provide a valid DMA address for >>> the device. >>> >>> The existing rga_buf_map() code is so clearly wrong I can't tell whether >>> that mapping is done somewhere out in the core framework or whether the >>> driver's supposed to be doing it for itself. >> >> The sg_table is filled by dma_map_sgtable in >> drivers/media/common/videobuf2/videobuf2-dma-sg.c. > > Ah, great - in that case all you should need to do here is fill out the DMA descriptors with the correct DMA address as you are doing. If buf->dev is the right thing and you've set your DMA masks > correctly then you can rely on the DMA addresses being appropriate already, since it's vb2-dma-sg's job to get that right (which per another recent thread it will do, but could do better...) A lot of media drivers that require 32 bit DMA set the vb2_queue gfp_flags: q->gfp_flags = __GFP_DMA32; This is ORed in the vb2 framework when calling alloc_pages(). That said, that's only used for the V4L2_MEMORY_MMAP streaming model, i.e. when the driver allocates the memory. For MEMORY_DMABUF this isn't used and you still need the correct DMA mask for the device. Since I do not see __GFP_DMA32, I suspect that allocating buffers in vb2 might still choose buffers above 4 GB, which would be wrong for this driver. Regards, Hans > >> Do you suggest to just drop the check for the addresses or is there something >> fundamentally wrong with filling the descriptor list in the driver? Can you >> explain what exactly is wrong with this code and do you have a pointer how to >> implement this correctly? > > The rest of your patch is frankly more correct than what it's replacing, you can just drop the redundant upper_32_bits() check :) > > Cheers, > Robin.
