[libcamera-devel] [RFC PATCH 05/10] ipa: rkisp1: Use offset in mapping IPABuffer

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Aug 16, 2021 at 01:31:33PM +0900, Hirokazu Honda wrote:
> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has
> now an offset. This uses the offset variable to map the IPABuffer.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/ipa/rkisp1/rkisp1.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 06fb9640..eb7a2705 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -165,7 +165,7 @@ void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
>  		 * to applications).
>  		 */
>  		buffersMemory_[buffer.id] = mmap(NULL,
> -						 fb.planes()[0].length,
> +						 fb.planes()[0].offset + fb.planes()[0].length,

Is this right ? You're mapping offset + length bytes at offset 0 from
the beginning of the dmabuf. The munmap() call is updated accordingly,
but the code that uses the mapped memory isn't, and will look at offset
0. Now, as those buffers, are used for statistics and ISP parameters,
the offset will always be zero, but the code isn't really correct.

How about updating the RkISP1 IPA to use MappedFrameBuffer instead, like
the other IPA modules ?

>  						 PROT_READ | PROT_WRITE,
>  						 MAP_SHARED,
>  						 fb.planes()[0].fd.fd(),
> @@ -186,7 +186,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
>  		if (fb == buffers_.end())
>  			continue;
>  
> -		munmap(buffersMemory_[id], fb->second.planes()[0].length);
> +		munmap(buffersMemory_[id], fb->second.planes()[0].offset + fb->second.planes()[0].length);
>  		buffersMemory_.erase(id);
>  		buffers_.erase(id);
>  	}

-- 
Regards,

Laurent Pinchart