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