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

[Hans Verkuil]

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.