[v2,06/37] libcamera: MappedFrameBuffer: Add MappedFrameBuffer::getPlaneFD()
diff mbox series

Message ID 20250824-b4-v0-5-2-gpuisp-v2-a-v2-6-96f4576c814e@linaro.org
State Superseded
Headers show
Series
  • Add GLES 2.0 GPUISP to libcamera
Related show

Commit Message

Bryan O'Donoghue Aug. 24, 2025, 12:48 a.m. UTC 
Add MappedFrameBuffer::getPlaneFD() which takes a plane index and returns
the file descriptor associated with it.

This fd will be used to feed into eglCreateImageKHR for both texture
creation on upload and directly render-to-texture where the texture buffer
comes from the fd given to eglCreateImageKHR.

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 include/libcamera/internal/mapped_framebuffer.h |  4 ++++
 src/libcamera/mapped_framebuffer.cpp            | 10 ++++++++++
 2 files changed, 14 insertions(+)

Comments

Barnabás Pőcze Sept. 25, 2025, 3:33 p.m. UTC | #1 
Hi

2025. 08. 24. 2:48 keltezéssel, Bryan O'Donoghue írta:
> Add MappedFrameBuffer::getPlaneFD() which takes a plane index and returns
> the file descriptor associated with it.
> 
> This fd will be used to feed into eglCreateImageKHR for both texture
> creation on upload and directly render-to-texture where the texture buffer
> comes from the fd given to eglCreateImageKHR.
> 
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   include/libcamera/internal/mapped_framebuffer.h |  4 ++++
>   src/libcamera/mapped_framebuffer.cpp            | 10 ++++++++++
>   2 files changed, 14 insertions(+)
> 
> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
> index 6aaabf5086b4658363e4e2ed02242248bfffdcbc..f7a6870764c4d035f77544354aa46a70095952a0 100644
> --- a/include/libcamera/internal/mapped_framebuffer.h
> +++ b/include/libcamera/internal/mapped_framebuffer.h
> @@ -55,6 +55,10 @@ public:
>   	using MapFlags = Flags<MapFlag>;
> 
>   	MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);
> +	int getPlaneFD(unsigned int plane);
> +
> +private:
> +	const FrameBuffer *buffer_;

I am not a fan of this approach. This is all but guaranteed to be
dangling in some cases. For example, `mali-c55.cpp:IPAMaliC55::mapBuffers`
will keep a whole map of `MappedFrameBuffers` with dangling pointers.
Similar thing happens in rpi.

Maybe extending the type stored in `planes_` is an option, but not sure how
that is best done due to the inheritance here (the only other derived class
appears to be `CameraBuffer::Private` in the android layer).


>   };
> 
>   LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapFlag)
> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> index f54bbf21f10987aecdf02c0a946edbb4931dd4c0..e85030d904ffaa5cd8cfae7b02d4b1b7010452a5 100644
> --- a/src/libcamera/mapped_framebuffer.cpp
> +++ b/src/libcamera/mapped_framebuffer.cpp
> @@ -238,6 +238,16 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
> 
>   		planes_.emplace_back(info.address + plane.offset, plane.length);
>   	}
> +
> +	buffer_ = buffer;
> +}
> +
> +int MappedFrameBuffer::getPlaneFD(unsigned int plane)
> +{
> +	if (plane > buffer_->planes().size())

This should be `plane >=` in any case.


Regards,
Barnabás Pőcze

> +		return -EINVAL;
> +
> +	return buffer_->planes()[plane].fd.get();
>   }
> 
>   } /* namespace libcamera */
> 
> --
> 2.50.1
>
Robert Mader Sept. 27, 2025, 8:15 p.m. UTC | #2 
Hi,

On 9/25/25 17:33, Barnabás Pőcze wrote:
> Hi
>
> 2025. 08. 24. 2:48 keltezéssel, Bryan O'Donoghue írta:
>> Add MappedFrameBuffer::getPlaneFD() which takes a plane index and 
>> returns
>> the file descriptor associated with it.
>>
>> This fd will be used to feed into eglCreateImageKHR for both texture
>> creation on upload and directly render-to-texture where the texture 
>> buffer
>> comes from the fd given to eglCreateImageKHR.
>>
>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   include/libcamera/internal/mapped_framebuffer.h |  4 ++++
>>   src/libcamera/mapped_framebuffer.cpp            | 10 ++++++++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/include/libcamera/internal/mapped_framebuffer.h 
>> b/include/libcamera/internal/mapped_framebuffer.h
>> index 
>> 6aaabf5086b4658363e4e2ed02242248bfffdcbc..f7a6870764c4d035f77544354aa46a70095952a0 
>> 100644
>> --- a/include/libcamera/internal/mapped_framebuffer.h
>> +++ b/include/libcamera/internal/mapped_framebuffer.h
>> @@ -55,6 +55,10 @@ public:
>>       using MapFlags = Flags<MapFlag>;
>>
>>       MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);
>> +    int getPlaneFD(unsigned int plane);
>> +
>> +private:
>> +    const FrameBuffer *buffer_;
>
> I am not a fan of this approach. This is all but guaranteed to be
> dangling in some cases. For example, 
> `mali-c55.cpp:IPAMaliC55::mapBuffers`
> will keep a whole map of `MappedFrameBuffers` with dangling pointers.
> Similar thing happens in rpi.
>
> Maybe extending the type stored in `planes_` is an option, but not 
> sure how
> that is best done due to the inheritance here (the only other derived 
> class
> appears to be `CameraBuffer::Private` in the android layer).

I'd like to add that using MappedFrameBuffer for querying the planes 
might be unnecessary as well as undesirable in the first place, for the 
following reasons:

 1. Output buffers should never need to get mapped in the GPU-ISP.
 2. Input buffer should only get mapped when necessary. If we can
    directly import buffers to the GPU, that avoids a full buffer copy
    plus the overhead of mmapping and syncing - and with the commit
    "libcamera: software_isp: Run sw-statistics once every 4th frame" we
    fully avoid mmaping/syncing the input buffer in many cases.
 3. Just using the FDs from FrameBuffer seems to work just as well, see
    A, which is a PoC for the above.

Best regards,

Robert

A.: 
https://gitlab.freedesktop.org/rmader/libcamera/-/commit/1f5c2764c4d42c916e52f9c06c44f4ae52519a78

>
>>   };
>>
>>   LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapFlag)
>> diff --git a/src/libcamera/mapped_framebuffer.cpp 
>> b/src/libcamera/mapped_framebuffer.cpp
>> index 
>> f54bbf21f10987aecdf02c0a946edbb4931dd4c0..e85030d904ffaa5cd8cfae7b02d4b1b7010452a5 
>> 100644
>> --- a/src/libcamera/mapped_framebuffer.cpp
>> +++ b/src/libcamera/mapped_framebuffer.cpp
>> @@ -238,6 +238,16 @@ MappedFrameBuffer::MappedFrameBuffer(const 
>> FrameBuffer *buffer, MapFlags flags)
>>
>>           planes_.emplace_back(info.address + plane.offset, 
>> plane.length);
>>       }
>> +
>> +    buffer_ = buffer;
>> +}
>> +
>> +int MappedFrameBuffer::getPlaneFD(unsigned int plane)
>> +{
>> +    if (plane > buffer_->planes().size())
>
> This should be `plane >=` in any case.
>
>
> Regards,
> Barnabás Pőcze
>
>> +        return -EINVAL;
>> +
>> +    return buffer_->planes()[plane].fd.get();
>>   }
>>
>>   } /* namespace libcamera */
>>
>> -- 
>> 2.50.1
>>
>
Bryan O'Donoghue Oct. 6, 2025, 9 p.m. UTC | #3 
On 27/09/2025 21:15, Robert Mader wrote:
> Hi,
> 
> On 9/25/25 17:33, Barnabás Pőcze wrote:
>> Hi
>>
>> 2025. 08. 24. 2:48 keltezéssel, Bryan O'Donoghue írta:
>>> Add MappedFrameBuffer::getPlaneFD() which takes a plane index and 
>>> returns
>>> the file descriptor associated with it.
>>>
>>> This fd will be used to feed into eglCreateImageKHR for both texture
>>> creation on upload and directly render-to-texture where the texture 
>>> buffer
>>> comes from the fd given to eglCreateImageKHR.
>>>
>>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> ---
>>>   include/libcamera/internal/mapped_framebuffer.h |  4 ++++
>>>   src/libcamera/mapped_framebuffer.cpp            | 10 ++++++++++
>>>   2 files changed, 14 insertions(+)
>>>
>>> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/ 
>>> include/libcamera/internal/mapped_framebuffer.h
>>> index 
>>> 6aaabf5086b4658363e4e2ed02242248bfffdcbc..f7a6870764c4d035f77544354aa46a70095952a0 100644
>>> --- a/include/libcamera/internal/mapped_framebuffer.h
>>> +++ b/include/libcamera/internal/mapped_framebuffer.h
>>> @@ -55,6 +55,10 @@ public:
>>>       using MapFlags = Flags<MapFlag>;
>>>
>>>       MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);
>>> +    int getPlaneFD(unsigned int plane);
>>> +
>>> +private:
>>> +    const FrameBuffer *buffer_;
>>
>> I am not a fan of this approach. This is all but guaranteed to be
>> dangling in some cases. For example, `mali- 
>> c55.cpp:IPAMaliC55::mapBuffers`
>> will keep a whole map of `MappedFrameBuffers` with dangling pointers.
>> Similar thing happens in rpi.
>>
>> Maybe extending the type stored in `planes_` is an option, but not 
>> sure how
>> that is best done due to the inheritance here (the only other derived 
>> class
>> appears to be `CameraBuffer::Private` in the android layer).
> 
> I'd like to add that using MappedFrameBuffer for querying the planes 
> might be unnecessary as well as undesirable in the first place, for the 
> following reasons:
> 
>  1. Output buffers should never need to get mapped in the GPU-ISP.
>  2. Input buffer should only get mapped when necessary. If we can
>     directly import buffers to the GPU, that avoids a full buffer copy
>     plus the overhead of mmapping and syncing - and with the commit
>     "libcamera: software_isp: Run sw-statistics once every 4th frame" we
>     fully avoid mmaping/syncing the input buffer in many cases.
>  3. Just using the FDs from FrameBuffer seems to work just as well, see
>     A, which is a PoC for the above.
> 
> Best regards,
> 
> Robert
> 
> A.: https://gitlab.freedesktop.org/rmader/libcamera/-/ 
> commit/1f5c2764c4d42c916e52f9c06c44f4ae52519a78
> 
>>
>>>   };
>>>
>>>   LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapFlag)
>>> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/ 
>>> mapped_framebuffer.cpp
>>> index 
>>> f54bbf21f10987aecdf02c0a946edbb4931dd4c0..e85030d904ffaa5cd8cfae7b02d4b1b7010452a5 100644
>>> --- a/src/libcamera/mapped_framebuffer.cpp
>>> +++ b/src/libcamera/mapped_framebuffer.cpp
>>> @@ -238,6 +238,16 @@ MappedFrameBuffer::MappedFrameBuffer(const 
>>> FrameBuffer *buffer, MapFlags flags)
>>>
>>>           planes_.emplace_back(info.address + plane.offset, 
>>> plane.length);
>>>       }
>>> +
>>> +    buffer_ = buffer;
>>> +}
>>> +
>>> +int MappedFrameBuffer::getPlaneFD(unsigned int plane)
>>> +{
>>> +    if (plane > buffer_->planes().size())
>>
>> This should be `plane >=` in any case.
Based on the response here and a code-fragment from Robert, I've dropped 
this patch for v3.

---
bod

Patch
diff mbox series 

diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
index 6aaabf5086b4658363e4e2ed02242248bfffdcbc..f7a6870764c4d035f77544354aa46a70095952a0 100644
--- a/include/libcamera/internal/mapped_framebuffer.h
+++ b/include/libcamera/internal/mapped_framebuffer.h
@@ -55,6 +55,10 @@  public:
 	using MapFlags = Flags<MapFlag>;
 
 	MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);
+	int getPlaneFD(unsigned int plane);
+
+private:
+	const FrameBuffer *buffer_;
 };
 
 LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapFlag)
diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
index f54bbf21f10987aecdf02c0a946edbb4931dd4c0..e85030d904ffaa5cd8cfae7b02d4b1b7010452a5 100644
--- a/src/libcamera/mapped_framebuffer.cpp
+++ b/src/libcamera/mapped_framebuffer.cpp
@@ -238,6 +238,16 @@  MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
 
 		planes_.emplace_back(info.address + plane.offset, plane.length);
 	}
+
+	buffer_ = buffer;
+}
+
+int MappedFrameBuffer::getPlaneFD(unsigned int plane)
+{
+	if (plane > buffer_->planes().size())
+		return -EINVAL;
+
+	return buffer_->planes()[plane].fd.get();
 }
 
 } /* namespace libcamera */