| Message ID | 20200724072218.943245-1-naush@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi all, Is there any further changes anyone would like me to make for this series? I know Laurent had comments on what I was trying to achieve in patches 09/10 and 10/10 - I have made some minor changes with variable names and comments to hopefully explain the code better. If there are other clarifications to be made, please let me know and I will be happy to do so. Regards, Naush On Fri, 24 Jul 2020 at 08:22, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi, > > This is patchset v5 of the zero copy RAW stream work for the Raspberry Pi platform. > All minor changes in the review feedback have been addressed. The only other difference > is in patch 9/10 where I have renamed requeueBuffers_ to requestBuffers_ and made minor > changes to the comments to hopefully make the usage slightly more understandable. I > have left the review tags in place, as there is no functional change, hope that is ok. > > Regards, > Naush > > Naushir Patuck (10): > libcamera: pipeline: raspberrypi: Move RPiStream into a separate file > libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling > libcamera: request: Add log point on a completed request > 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 > > include/libcamera/ipa/raspberrypi.h | 2 +- > src/ipa/raspberrypi/raspberrypi.cpp | 20 +- > .../pipeline/raspberrypi/meson.build | 1 + > .../pipeline/raspberrypi/raspberrypi.cpp | 522 +++++++----------- > .../pipeline/raspberrypi/rpi_stream.cpp | 216 ++++++++ > .../pipeline/raspberrypi/rpi_stream.h | 131 +++++ > src/libcamera/request.cpp | 3 + > 7 files changed, 563 insertions(+), 332 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-07-30 09:39:38 +0100, Naushir Patuck wrote: > Hi all, > > Is there any further changes anyone would like me to make for this > series? I know Laurent had comments on what I was trying to achieve > in patches 09/10 and 10/10 - I have made some minor changes with > variable names and comments to hopefully explain the code better. If > there are other clarifications to be made, please let me know and I > will be happy to do so. On one hand I think this series improves the pipeline but at the same time it's hard to know if the direction will solve the zero-copy problem as that patch have been dropped from the series. If you and others are pushing for this series to be integrated I would be fine doing so, but if possible it would be nice to see a version with the zero-copy raw feature added back. > > Regards, > Naush > > On Fri, 24 Jul 2020 at 08:22, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > Hi, > > > > This is patchset v5 of the zero copy RAW stream work for the Raspberry Pi platform. > > All minor changes in the review feedback have been addressed. The only other difference > > is in patch 9/10 where I have renamed requeueBuffers_ to requestBuffers_ and made minor > > changes to the comments to hopefully make the usage slightly more understandable. I > > have left the review tags in place, as there is no functional change, hope that is ok. > > > > Regards, > > Naush > > > > Naushir Patuck (10): > > libcamera: pipeline: raspberrypi: Move RPiStream into a separate file > > libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling > > libcamera: request: Add log point on a completed request > > 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 > > > > include/libcamera/ipa/raspberrypi.h | 2 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 20 +- > > .../pipeline/raspberrypi/meson.build | 1 + > > .../pipeline/raspberrypi/raspberrypi.cpp | 522 +++++++----------- > > .../pipeline/raspberrypi/rpi_stream.cpp | 216 ++++++++ > > .../pipeline/raspberrypi/rpi_stream.h | 131 +++++ > > src/libcamera/request.cpp | 3 + > > 7 files changed, 563 insertions(+), 332 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, On Sat, 1 Aug 2020, 10:22 am Niklas Söderlund, < niklas.soderlund@ragnatech.se> wrote: > Hi Naushir, > > On 2020-07-30 09:39:38 +0100, Naushir Patuck wrote: > > Hi all, > > > > Is there any further changes anyone would like me to make for this > > series? I know Laurent had comments on what I was trying to achieve > > in patches 09/10 and 10/10 - I have made some minor changes with > > variable names and comments to hopefully explain the code better. If > > there are other clarifications to be made, please let me know and I > > will be happy to do so. > > On one hand I think this series improves the pipeline but at the same > time it's hard to know if the direction will solve the zero-copy problem > as that patch have been dropped from the series. If you and others are > pushing for this series to be integrated I would be fine doing so, but > if possible it would be nice to see a version with the zero-copy raw > feature added back. > All the patches in the series will enable zero-copy RAW (or any other stream really) captures in the raspberry pi platform. I've been testing this with cam, qcam, and our in-house libcamera application. The only feature missing is that applications cannot use buffers that have not been allocated or exported by the v4l2 videodevice (e.g. a random dmabuf), but none of the above applications do that right now as far as I can tell. I have a patch series waiting to add support for this and will submit for review once this is gone through. Hope that is ok? Regards, Naush > > > > Regards, > > Naush > > > > On Fri, 24 Jul 2020 at 08:22, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > > > Hi, > > > > > > This is patchset v5 of the zero copy RAW stream work for the Raspberry > Pi platform. > > > All minor changes in the review feedback have been addressed. The > only other difference > > > is in patch 9/10 where I have renamed requeueBuffers_ to > requestBuffers_ and made minor > > > changes to the comments to hopefully make the usage slightly more > understandable. I > > > have left the review tags in place, as there is no functional change, > hope that is ok. > > > > > > Regards, > > > Naush > > > > > > Naushir Patuck (10): > > > libcamera: pipeline: raspberrypi: Move RPiStream into a separate file > > > libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling > > > libcamera: request: Add log point on a completed request > > > 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 > > > > > > include/libcamera/ipa/raspberrypi.h | 2 +- > > > src/ipa/raspberrypi/raspberrypi.cpp | 20 +- > > > .../pipeline/raspberrypi/meson.build | 1 + > > > .../pipeline/raspberrypi/raspberrypi.cpp | 522 +++++++----------- > > > .../pipeline/raspberrypi/rpi_stream.cpp | 216 ++++++++ > > > .../pipeline/raspberrypi/rpi_stream.h | 131 +++++ > > > src/libcamera/request.cpp | 3 + > > > 7 files changed, 563 insertions(+), 332 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 Naushir, On 2020-08-01 11:32:19 +0100, Naushir Patuck wrote: > Hi Niklas, > > > On Sat, 1 Aug 2020, 10:22 am Niklas Söderlund, < > niklas.soderlund@ragnatech.se> wrote: > > > Hi Naushir, > > > > On 2020-07-30 09:39:38 +0100, Naushir Patuck wrote: > > > Hi all, > > > > > > Is there any further changes anyone would like me to make for this > > > series? I know Laurent had comments on what I was trying to achieve > > > in patches 09/10 and 10/10 - I have made some minor changes with > > > variable names and comments to hopefully explain the code better. If > > > there are other clarifications to be made, please let me know and I > > > will be happy to do so. > > > > On one hand I think this series improves the pipeline but at the same > > time it's hard to know if the direction will solve the zero-copy problem > > as that patch have been dropped from the series. If you and others are > > pushing for this series to be integrated I would be fine doing so, but > > if possible it would be nice to see a version with the zero-copy raw > > feature added back. > > > > All the patches in the series will enable zero-copy RAW (or any other > stream really) captures in the raspberry pi platform. I've been testing > this with cam, qcam, and our in-house libcamera application. You are right, what version of the series did I look at? I thought the copyFrom() was still present in the pipeline. My bad, sorry about that. > > The only feature missing is that applications cannot use buffers that have > not been allocated or exported by the v4l2 videodevice (e.g. a random > dmabuf), but none of the above applications do that right now as far as I > can tell. I have a patch series waiting to add support for this and will > submit for review once this is gone through. Hope that is ok? > This is unfortunately a problem as it introduces a regression. What's in master work's with external buffers, but just in a zero-copy fashion. The Android HAL component uses external buffers which libcamera can't control so I think removing the ability to work with external buffers does more harm then good. Would it be possible to merge the two series? Or in some other way break out the cleanup in one and the entablement of zero-copy in another? > Regards, > Naush > > > > > > > > > Regards, > > > Naush > > > > > > On Fri, 24 Jul 2020 at 08:22, Naushir Patuck <naush@raspberrypi.com> > > wrote: > > > > > > > > Hi, > > > > > > > > This is patchset v5 of the zero copy RAW stream work for the Raspberry > > Pi platform. > > > > All minor changes in the review feedback have been addressed. The > > only other difference > > > > is in patch 9/10 where I have renamed requeueBuffers_ to > > requestBuffers_ and made minor > > > > changes to the comments to hopefully make the usage slightly more > > understandable. I > > > > have left the review tags in place, as there is no functional change, > > hope that is ok. > > > > > > > > Regards, > > > > Naush > > > > > > > > Naushir Patuck (10): > > > > libcamera: pipeline: raspberrypi: Move RPiStream into a separate file > > > > libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling > > > > libcamera: request: Add log point on a completed request > > > > 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 > > > > > > > > include/libcamera/ipa/raspberrypi.h | 2 +- > > > > src/ipa/raspberrypi/raspberrypi.cpp | 20 +- > > > > .../pipeline/raspberrypi/meson.build | 1 + > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 522 +++++++----------- > > > > .../pipeline/raspberrypi/rpi_stream.cpp | 216 ++++++++ > > > > .../pipeline/raspberrypi/rpi_stream.h | 131 +++++ > > > > src/libcamera/request.cpp | 3 + > > > > 7 files changed, 563 insertions(+), 332 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 Niklas, On Mon, 3 Aug 2020 at 12:50, Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: > > Hi Naushir, > > On 2020-08-01 11:32:19 +0100, Naushir Patuck wrote: > > Hi Niklas, > > > > > > On Sat, 1 Aug 2020, 10:22 am Niklas Söderlund, < > > niklas.soderlund@ragnatech.se> wrote: > > > > > Hi Naushir, > > > > > > On 2020-07-30 09:39:38 +0100, Naushir Patuck wrote: > > > > Hi all, > > > > > > > > Is there any further changes anyone would like me to make for this > > > > series? I know Laurent had comments on what I was trying to achieve > > > > in patches 09/10 and 10/10 - I have made some minor changes with > > > > variable names and comments to hopefully explain the code better. If > > > > there are other clarifications to be made, please let me know and I > > > > will be happy to do so. > > > > > > On one hand I think this series improves the pipeline but at the same > > > time it's hard to know if the direction will solve the zero-copy problem > > > as that patch have been dropped from the series. If you and others are > > > pushing for this series to be integrated I would be fine doing so, but > > > if possible it would be nice to see a version with the zero-copy raw > > > feature added back. > > > > > > > All the patches in the series will enable zero-copy RAW (or any other > > stream really) captures in the raspberry pi platform. I've been testing > > this with cam, qcam, and our in-house libcamera application. > > You are right, what version of the series did I look at? I thought the > copyFrom() was still present in the pipeline. My bad, sorry about that. > > > > > The only feature missing is that applications cannot use buffers that have > > not been allocated or exported by the v4l2 videodevice (e.g. a random > > dmabuf), but none of the above applications do that right now as far as I > > can tell. I have a patch series waiting to add support for this and will > > submit for review once this is gone through. Hope that is ok? > > > > This is unfortunately a problem as it introduces a regression. What's in > master work's with external buffers, but just in a zero-copy fashion. > The Android HAL component uses external buffers which libcamera can't > control so I think removing the ability to work with external buffers > does more harm then good. > > Would it be possible to merge the two series? Or in some other way break > out the cleanup in one and the entablement of zero-copy in another? I see now. Was not aware the Android HAL worked in this way. Easiest thing for me would be to merge the two series so that the functionality remains intact for the patchset. I'll post an update shortly. Regards, Naush > > > Regards, > > Naush > > > > > > > > > > > > > > Regards, > > > > Naush > > > > > > > > On Fri, 24 Jul 2020 at 08:22, Naushir Patuck <naush@raspberrypi.com> > > > wrote: > > > > > > > > > > Hi, > > > > > > > > > > This is patchset v5 of the zero copy RAW stream work for the Raspberry > > > Pi platform. > > > > > All minor changes in the review feedback have been addressed. The > > > only other difference > > > > > is in patch 9/10 where I have renamed requeueBuffers_ to > > > requestBuffers_ and made minor > > > > > changes to the comments to hopefully make the usage slightly more > > > understandable. I > > > > > have left the review tags in place, as there is no functional change, > > > hope that is ok. > > > > > > > > > > Regards, > > > > > Naush > > > > > > > > > > Naushir Patuck (10): > > > > > libcamera: pipeline: raspberrypi: Move RPiStream into a separate file > > > > > libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling > > > > > libcamera: request: Add log point on a completed request > > > > > 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 > > > > > > > > > > include/libcamera/ipa/raspberrypi.h | 2 +- > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 20 +- > > > > > .../pipeline/raspberrypi/meson.build | 1 + > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 522 +++++++----------- > > > > > .../pipeline/raspberrypi/rpi_stream.cpp | 216 ++++++++ > > > > > .../pipeline/raspberrypi/rpi_stream.h | 131 +++++ > > > > > src/libcamera/request.cpp | 3 + > > > > > 7 files changed, 563 insertions(+), 332 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 > > > > > -- > Regards, > Niklas Söderlund
Hi, This is patchset v5 of the zero copy RAW stream work for the Raspberry Pi platform. All minor changes in the review feedback have been addressed. The only other difference is in patch 9/10 where I have renamed requeueBuffers_ to requestBuffers_ and made minor changes to the comments to hopefully make the usage slightly more understandable. I have left the review tags in place, as there is no functional change, hope that is ok. Regards, Naush Naushir Patuck (10): libcamera: pipeline: raspberrypi: Move RPiStream into a separate file libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling libcamera: request: Add log point on a completed request 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 include/libcamera/ipa/raspberrypi.h | 2 +- src/ipa/raspberrypi/raspberrypi.cpp | 20 +- .../pipeline/raspberrypi/meson.build | 1 + .../pipeline/raspberrypi/raspberrypi.cpp | 522 +++++++----------- .../pipeline/raspberrypi/rpi_stream.cpp | 216 ++++++++ .../pipeline/raspberrypi/rpi_stream.h | 131 +++++ src/libcamera/request.cpp | 3 + 7 files changed, 563 insertions(+), 332 deletions(-) create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.cpp create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.h