[PATCH 02/13] media: rockchip: rga: extract helper to fill descriptors

[Robin Murphy]

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.