| Message ID | 20251010092226.41228-2-robert.mader@collabora.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Robert Mader (2025-10-10 10:22:25) > This has been shipped downstream in postmarketOS for a while and, in "This has been shipped downstream" is really a comment for after the '---' That's not relational to the commit. > some cases, seems to improve stability on not-so-great drivers. It also > brings the software ISP in line with the default used otherwise. This should justify the patch at a technical level - not just because it's applied somewhere else. Perhaps: """ pipeline: simple: Increase internal buffers for software ISP to 4 The Simple Pipeline handler supports a variety of hardware with different capabilities and performances. To improve performance and reliability of the cameras across the supported range, increase the number of internal buffers to 4. This allows lower performance devices more opportunity to process the frames and increases stability. Align the Simple Pipeline handler and Soft ISP buffering with the other hardware based platforms and use 4 internal buffers. --- This has been applied in postmarketOS since Xxxx and has fixed issues reported by users. """ > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 2dcba04ec..c4cb7c391 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -420,7 +420,7 @@ protected: > int queueRequestDevice(Camera *camera, Request *request) override; > > private: > - static constexpr unsigned int kNumInternalBuffers = 3; > + static constexpr unsigned int kNumInternalBuffers = 4; > > struct EntityData { > std::unique_ptr<V4L2VideoDevice> video; > -- > 2.51.0 >
On Fri, Oct 10, 2025 at 11:28:05AM +0100, Kieran Bingham wrote: > Quoting Robert Mader (2025-10-10 10:22:25) > > This has been shipped downstream in postmarketOS for a while and, in > > "This has been shipped downstream" is really a comment for after the > '---' That's not relational to the commit. > > > some cases, seems to improve stability on not-so-great drivers. It also > > brings the software ISP in line with the default used otherwise. > > > This should justify the patch at a technical level - not just because > it's applied somewhere else. Perhaps: > > """ > pipeline: simple: Increase internal buffers for software ISP to 4 > > The Simple Pipeline handler supports a variety of hardware with > different capabilities and performances. > > To improve performance and reliability of the cameras across the > supported range, increase the number of internal buffers to 4. > > This allows lower performance devices more opportunity to process the > frames and increases stability. It's better but still quite vague. A description of the actual issue would have been best. As the simple pipeline handler supports low-end devices, lowering memory usage is important. I would have liked to understand which devices perform better with this patch, and why. Otherwise, later reworks may undo this change one way or another, and we'll have no way to know from the git history why this was important. > Align the Simple Pipeline handler and Soft ISP buffering with the other > hardware based platforms and use 4 internal buffers. > > --- > > This has been applied in postmarketOS since Xxxx and has fixed issues > reported by users. > > """ > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > > --- > > src/libcamera/pipeline/simple/simple.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 2dcba04ec..c4cb7c391 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -420,7 +420,7 @@ protected: > > int queueRequestDevice(Camera *camera, Request *request) override; > > > > private: > > - static constexpr unsigned int kNumInternalBuffers = 3; > > + static constexpr unsigned int kNumInternalBuffers = 4; > > > > struct EntityData { > > std::unique_ptr<V4L2VideoDevice> video;
On 10/13/25 14:36, Laurent Pinchart wrote: > On Fri, Oct 10, 2025 at 11:28:05AM +0100, Kieran Bingham wrote: >> ... >> >> This should justify the patch at a technical level - not just because >> it's applied somewhere else. Perhaps: >> >> """ >> pipeline: simple: Increase internal buffers for software ISP to 4 >> >> The Simple Pipeline handler supports a variety of hardware with >> different capabilities and performances. >> >> To improve performance and reliability of the cameras across the >> supported range, increase the number of internal buffers to 4. >> >> This allows lower performance devices more opportunity to process the >> frames and increases stability. > It's better but still quite vague. A description of the actual issue > would have been best. As the simple pipeline handler supports low-end > devices, lowering memory usage is important. I would have liked to > understand which devices perform better with this patch, and why. > Otherwise, later reworks may undo this change one way or another, and > we'll have no way to know from the git history why this was important. I'm afraid I can't provide a proper analysis in the short term :/ The evidence wast pretty anecdotal - IIUC I myself saw (unfortunately not properly measured / documented) improvements on devices like the Pixel 3a (qcom-camss), the Librem5 (imx7-csi) and the original PinePhone (sun6i-csi) in the form of less hangs / frame-skips. That being said, I completely assume that the extra buffer shouldn't be necessary under optimal conditions. It should be noted that I'm not aware of any case where this patch was strictly necessary to get some camera to work - which also means that, should the code be modified in the future, any fallout wouldn't be critical. As for memory consumption: my personal test devices don't include any that needs the sw-ISP while also being so memory limited that the extra-buffer is really noticeable. If it is a concern, though, we could make it configurable in the GlobalConfiguration. At the current state I personally would consider that over-engineered though. Overall I personally find the anecdotal evidence combined with the fact that most other pipelines use 4 buffers by default convincing enough to justify the change. If you don't agree, we can drop again for now and I can try to have another look for 0.7. Best regards
On Mon, Oct 13, 2025 at 03:25:35PM +0200, Robert Mader wrote: > On 10/13/25 14:36, Laurent Pinchart wrote: > > On Fri, Oct 10, 2025 at 11:28:05AM +0100, Kieran Bingham wrote: > >> ... > >> > >> This should justify the patch at a technical level - not just because > >> it's applied somewhere else. Perhaps: > >> > >> """ > >> pipeline: simple: Increase internal buffers for software ISP to 4 > >> > >> The Simple Pipeline handler supports a variety of hardware with > >> different capabilities and performances. > >> > >> To improve performance and reliability of the cameras across the > >> supported range, increase the number of internal buffers to 4. > >> > >> This allows lower performance devices more opportunity to process the > >> frames and increases stability. > > It's better but still quite vague. A description of the actual issue > > would have been best. As the simple pipeline handler supports low-end > > devices, lowering memory usage is important. I would have liked to > > understand which devices perform better with this patch, and why. > > Otherwise, later reworks may undo this change one way or another, and > > we'll have no way to know from the git history why this was important. > > I'm afraid I can't provide a proper analysis in the short term :/ The > evidence wast pretty anecdotal - IIUC I myself saw (unfortunately not > properly measured / documented) improvements on devices like the Pixel > 3a (qcom-camss), the Librem5 (imx7-csi) and the original PinePhone > (sun6i-csi) in the form of less hangs / frame-skips. That being said, I > completely assume that the extra buffer shouldn't be necessary under > optimal conditions. > > It should be noted that I'm not aware of any case where this patch was > strictly necessary to get some camera to work - which also means that, > should the code be modified in the future, any fallout wouldn't be critical. > > As for memory consumption: my personal test devices don't include any > that needs the sw-ISP while also being so memory limited that the > extra-buffer is really noticeable. If it is a concern, though, we could > make it configurable in the GlobalConfiguration. At the current state I > personally would consider that over-engineered though. > > Overall I personally find the anecdotal evidence combined with the fact > that most other pipelines use 4 buffers by default convincing enough to > justify the change. If you don't agree, we can drop again for now and I > can try to have another look for 0.7. The patch has been merged and there's no need to revert it. Even if it hasn't been merged yet I think it could go in as-is :-) My concern is that future rework will likely change all this logic, and without a detailed explanation somewhere in the git history or mailing list archives, we may end up going back to 3 buffers by accident.
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 2dcba04ec..c4cb7c391 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -420,7 +420,7 @@ protected: int queueRequestDevice(Camera *camera, Request *request) override; private: - static constexpr unsigned int kNumInternalBuffers = 3; + static constexpr unsigned int kNumInternalBuffers = 4; struct EntityData { std::unique_ptr<V4L2VideoDevice> video;