| Message ID | 20200804095850.275499-1-naush@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi, Sorry to pester again, but could I get a review for this patch set please? Particularly patches 10-12/12 which are new additions. Thanks, Naush On Tue, 4 Aug 2020 at 10:58, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi, > > Here is the next version of the zero-copy function patchset for the Raspberry Pi > platform. Patches 01-09/12 are identical to the previous patchset, and patches > 10-12/12 add functionality to support any external buffer allocation to be used > for stream captures. > > Regards, > Naush > > Naushir Patuck (12): > libcamera: pipeline: raspberrypi: Move RPiStream into a separate file > libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling > libcamera: pipeline: raspberrypi: Add some debug logging > libcamera: pipeline: raspberrypi: Increase the number of RAW buffers > libcamera: pipeline: raspberrypi: Remove const qualifier from > RPiStream > libcamera: pipeline: raspberrypi: Rework stream buffer logic for > zero-copy > libcamera: pipeline: raspberrypi: Fix bug in passing configuration to > IPA > libcamera: pipeline: raspberrypi: Add more robust stream buffer logic > libcamera: pipeline: ipa: raspberrypi: Remove use of FrameBuffer > cookie > pipeline: raspberrypi: Use an unordered_map for the stream buffer list > pipeline: raspberrypi: Use an unordered_set to store IPA buffer ids > pipeline: ipa: raspberrypi: Handle any externally allocated > FrameBuffer > > include/libcamera/ipa/raspberrypi.h | 13 +- > src/ipa/raspberrypi/raspberrypi.cpp | 26 +- > .../pipeline/raspberrypi/meson.build | 1 + > .../pipeline/raspberrypi/raspberrypi.cpp | 587 ++++++++---------- > .../pipeline/raspberrypi/rpi_stream.cpp | 244 ++++++++ > .../pipeline/raspberrypi/rpi_stream.h | 140 +++++ > 6 files changed, 662 insertions(+), 349 deletions(-) > create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.h > > -- > 2.25.1 >
Hi Naushir, On 2020-08-27 09:29:43 +0100, Naushir Patuck wrote: > Hi, > > Sorry to pester again, but could I get a review for this patch set > please? Particularly patches 10-12/12 which are new additions. Sorry for taking so long thinking about this series. After rereading it a few times the past week I still think we need to scale the scope back a bit. Would it be possible to restructure it to only deal with RAW buffers? I know it's possible to expose the statistic and sensor embedded data buffers using the same structure, but this is a problem we overall have not thought about yet. I fear taking this step in the RPi pipeline now could be premature and introduce regressions when we attack this issue on a project level. One thing that worries with the current implementation is how the statistic and embedded data buffers are mapped in the IPA on each request. I think this is something we can't really do and something else is needed. What I'm not sure yet. I'm sorry I was unable to communicate this fear in v5. I was hoping an integration of zero-copy raw would split these two issues naturally. So to be able to move both features forward could this series be split in two, one series intended to go in as soon as possible with the minimum to get zero-copy raw working and one on-top of that with a possible solution for exposing statistics and embedded data to applications? I feel with such a solution we could close the RAW topic and get the fixes in this series in while creating a good start of discussion for how to expose new type of information to applications. > > Thanks, > Naush > > On Tue, 4 Aug 2020 at 10:58, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > Hi, > > > > Here is the next version of the zero-copy function patchset for the Raspberry Pi > > platform. Patches 01-09/12 are identical to the previous patchset, and patches > > 10-12/12 add functionality to support any external buffer allocation to be used > > for stream captures. > > > > Regards, > > Naush > > > > Naushir Patuck (12): > > libcamera: pipeline: raspberrypi: Move RPiStream into a separate file > > libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling > > libcamera: pipeline: raspberrypi: Add some debug logging > > libcamera: pipeline: raspberrypi: Increase the number of RAW buffers > > libcamera: pipeline: raspberrypi: Remove const qualifier from > > RPiStream > > libcamera: pipeline: raspberrypi: Rework stream buffer logic for > > zero-copy > > libcamera: pipeline: raspberrypi: Fix bug in passing configuration to > > IPA > > libcamera: pipeline: raspberrypi: Add more robust stream buffer logic > > libcamera: pipeline: ipa: raspberrypi: Remove use of FrameBuffer > > cookie > > pipeline: raspberrypi: Use an unordered_map for the stream buffer list > > pipeline: raspberrypi: Use an unordered_set to store IPA buffer ids > > pipeline: ipa: raspberrypi: Handle any externally allocated > > FrameBuffer > > > > include/libcamera/ipa/raspberrypi.h | 13 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 26 +- > > .../pipeline/raspberrypi/meson.build | 1 + > > .../pipeline/raspberrypi/raspberrypi.cpp | 587 ++++++++---------- > > .../pipeline/raspberrypi/rpi_stream.cpp | 244 ++++++++ > > .../pipeline/raspberrypi/rpi_stream.h | 140 +++++ > > 6 files changed, 662 insertions(+), 349 deletions(-) > > create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.h > > > > -- > > 2.25.1 > > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for going through all the commits. Let me try and address some of your concerns below: On Sun, 30 Aug 2020 at 09:22, Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: > > Hi Naushir, > > On 2020-08-27 09:29:43 +0100, Naushir Patuck wrote: > > Hi, > > > > Sorry to pester again, but could I get a review for this patch set > > please? Particularly patches 10-12/12 which are new additions. > > Sorry for taking so long thinking about this series. > > After rereading it a few times the past week I still think we need to > scale the scope back a bit. Would it be possible to restructure it to > only deal with RAW buffers? I know it's possible to expose the statistic > and sensor embedded data buffers using the same structure, but this is a > problem we overall have not thought about yet. I fear taking this step > in the RPi pipeline now could be premature and introduce regressions > when we attack this issue on a project level. As you have probably seen from the code, the RPi pipeline handler treats all device streams (images, stats, embedded data) with the same abstraction. This was to allow us to easily provide any stream to the IPA and to the application should it request it. This abstraction has been there since before I started working on this patch set. I know that this is a different approach to what the other pipeline handlers do, but Raspberry Pi camera users are a demanding lot :) Collectively, they will want access to embedded data and stats in the application, and my approach to the pipeline handlers was to easily allow this. However, at present, the pipeline handler only advertises image streams available to the application as there is no mechanism in libcamera yet to request non-image buffers. So no application will be able to request statistics or embedded data buffers just yet. Depending on how libcamera decides to enable this feature, I would hope there is minimal work to do to the RPi pipeline handler to get these buffers out to the application with the stream abstraction we have. > > One thing that worries with the current implementation is how the > statistic and embedded data buffers are mapped in the IPA on each > request. I think this is something we can't really do and something else > is needed. What I'm not sure yet. If a stream buffer (for any stream) has been allocated externally and passed into a Request, I cannot see how we can avoid mmapping it if the IPA is to access the buffer. We could make things more efficient by perhaps having a cache of previously used buffers (like we discussed some time back). However, note that any buffers that have been exported by the V4L2 device (and not allocated externally) will be mmaped on startup in a batch, so this path is optimal. The reason I did the simplest thing (mmap and unmap on every external allocated buffer) for externally allocated buffers was because at present, our IPA only wants the stats and embedded data buffers, and as I mentioned above, these cannot be requested by the application just yet, so we will always go through the optimal path of doing the mmap up-front. We are currently discussing doing some pre-processing on the Bayer image in the IPA, but this is some longer term work, and we may have to address the mmapping per-frame issue then. > > I'm sorry I was unable to communicate this fear in v5. I was hoping an > integration of zero-copy raw would split these two issues naturally. So > to be able to move both features forward could this series be split in > two, one series intended to go in as soon as possible with the minimum > to get zero-copy raw working and one on-top of that with a possible > solution for exposing statistics and embedded data to applications? I > feel with such a solution we could close the RAW topic and get the fixes > in this series in while creating a good start of discussion for how to > expose new type of information to applications. Given that the RPi pipeline handler treats all streams the same since before this patch-set, the work for zero-copy RAW streams will simply apply to all other streams. My feeling is it would not be feasible to separate this out now for only RAW streams. Again, note that we do not advertise non-image streams for the application to request, and they actually cannot request stats or embedded data even if we did :) Sorry this is quite a troublesome patch-set to get through, but I am happy to address all issues that get flagged. It would be good to hear your thoughts after my comments above, and if you have any questions, please do send them my way. Best Regards, Naush > > > > > Thanks, > > Naush > > > > On Tue, 4 Aug 2020 at 10:58, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > Hi, > > > > > > Here is the next version of the zero-copy function patchset for the Raspberry Pi > > > platform. Patches 01-09/12 are identical to the previous patchset, and patches > > > 10-12/12 add functionality to support any external buffer allocation to be used > > > for stream captures. > > > > > > Regards, > > > Naush > > > > > > Naushir Patuck (12): > > > libcamera: pipeline: raspberrypi: Move RPiStream into a separate file > > > libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling > > > libcamera: pipeline: raspberrypi: Add some debug logging > > > libcamera: pipeline: raspberrypi: Increase the number of RAW buffers > > > libcamera: pipeline: raspberrypi: Remove const qualifier from > > > RPiStream > > > libcamera: pipeline: raspberrypi: Rework stream buffer logic for > > > zero-copy > > > libcamera: pipeline: raspberrypi: Fix bug in passing configuration to > > > IPA > > > libcamera: pipeline: raspberrypi: Add more robust stream buffer logic > > > libcamera: pipeline: ipa: raspberrypi: Remove use of FrameBuffer > > > cookie > > > pipeline: raspberrypi: Use an unordered_map for the stream buffer list > > > pipeline: raspberrypi: Use an unordered_set to store IPA buffer ids > > > pipeline: ipa: raspberrypi: Handle any externally allocated > > > FrameBuffer > > > > > > include/libcamera/ipa/raspberrypi.h | 13 +- > > > src/ipa/raspberrypi/raspberrypi.cpp | 26 +- > > > .../pipeline/raspberrypi/meson.build | 1 + > > > .../pipeline/raspberrypi/raspberrypi.cpp | 587 ++++++++---------- > > > .../pipeline/raspberrypi/rpi_stream.cpp | 244 ++++++++ > > > .../pipeline/raspberrypi/rpi_stream.h | 140 +++++ > > > 6 files changed, 662 insertions(+), 349 deletions(-) > > > create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.h > > > > > > -- > > > 2.25.1 > > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi, Here is the next version of the zero-copy function patchset for the Raspberry Pi platform. Patches 01-09/12 are identical to the previous patchset, and patches 10-12/12 add functionality to support any external buffer allocation to be used for stream captures. Regards, Naush Naushir Patuck (12): libcamera: pipeline: raspberrypi: Move RPiStream into a separate file libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling libcamera: pipeline: raspberrypi: Add some debug logging libcamera: pipeline: raspberrypi: Increase the number of RAW buffers libcamera: pipeline: raspberrypi: Remove const qualifier from RPiStream libcamera: pipeline: raspberrypi: Rework stream buffer logic for zero-copy libcamera: pipeline: raspberrypi: Fix bug in passing configuration to IPA libcamera: pipeline: raspberrypi: Add more robust stream buffer logic libcamera: pipeline: ipa: raspberrypi: Remove use of FrameBuffer cookie pipeline: raspberrypi: Use an unordered_map for the stream buffer list pipeline: raspberrypi: Use an unordered_set to store IPA buffer ids pipeline: ipa: raspberrypi: Handle any externally allocated FrameBuffer include/libcamera/ipa/raspberrypi.h | 13 +- src/ipa/raspberrypi/raspberrypi.cpp | 26 +- .../pipeline/raspberrypi/meson.build | 1 + .../pipeline/raspberrypi/raspberrypi.cpp | 587 ++++++++---------- .../pipeline/raspberrypi/rpi_stream.cpp | 244 ++++++++ .../pipeline/raspberrypi/rpi_stream.h | 140 +++++ 6 files changed, 662 insertions(+), 349 deletions(-) create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.cpp create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.h