| Message ID | 20201204153121.66136-1-naush@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote: > Hi, > > Here is patchset v3 for the work on passing controls on > camera::start(). I have addressed Jacopo's review comments, and > rebased to the top of master. This looks good to me. I've replied with minor comments on 1/3 and 2/3 which I could address when applying the series. For 3/3, there's an open question of whether we should drop the initial control set at configure() time. If you send a v4 with this change, could you incorporate the review feedback for the other patches too ? > Naushir Patuck (3): > libcamera: pipeline: Pass libcamera controls into > pipeline_handler::start() > libcamera: ipa: Pass a set of controls and return results from > ipa::start() > pipeline: ipa: raspberrypi: Pass controls to IPA on start > > Documentation/guides/pipeline-handler.rst | 4 +- > include/libcamera/camera.h | 2 +- > .../libcamera/internal/ipa_context_wrapper.h | 3 +- > include/libcamera/internal/pipeline_handler.h | 2 +- > include/libcamera/ipa/ipa_interface.h | 3 +- > include/libcamera/ipa/raspberrypi.h | 1 + > src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +- > src/ipa/raspberrypi/raspberrypi.cpp | 55 ++++++++++++------- > src/ipa/rkisp1/rkisp1.cpp | 3 +- > src/ipa/vimc/vimc.cpp | 6 +- > src/libcamera/camera.cpp | 11 ++-- > src/libcamera/ipa_context_wrapper.cpp | 5 +- > src/libcamera/ipa_interface.cpp | 7 +++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++-- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++- > src/libcamera/pipeline/simple/simple.cpp | 4 +- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++- > src/libcamera/pipeline_handler.cpp | 1 + > src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +- > src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++-- > test/ipa/ipa_interface_test.cpp | 3 +- > test/ipa/ipa_wrappers_test.cpp | 5 +- > 24 files changed, 117 insertions(+), 62 deletions(-)
Hi Laurent, On Mon, 7 Dec 2020 at 23:35, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote: > > Hi, > > > > Here is patchset v3 for the work on passing controls on > > camera::start(). I have addressed Jacopo's review comments, and > > rebased to the top of master. > > This looks good to me. I've replied with minor comments on 1/3 and 2/3 > which I could address when applying the series. For 3/3, there's an open > question of whether we should drop the initial control set at > configure() time. If you send a v4 with this change, could you > incorporate the review feedback for the other patches too ? > Thanks for the review feedback for all the patches. Regarding the question of removing the initial control set at configure() time, I think it still is needed. That set at configure() applies the default exposure time/gain to the sensor at start of today, whereas the set at start() only applies the exposure/gain if the controller wants a change to those values. On the first run, the controller will have no state history, so will not provide values to set in start(). The only time this is not true is if a user has provided some manual exposure/gain controls for the agc to use. Then we do the RPi::IPA_CONFIG_SENSOR twice, once with the default values, then overriding that with the user values. If you are ok with everything else, feel free to apply all your suggested changes to the patches and merge. If instead you would prefer me to create a v4 with all the changes, happy to do so as well, let me know. Regards, Naush > > Naushir Patuck (3): > > libcamera: pipeline: Pass libcamera controls into > > pipeline_handler::start() > > libcamera: ipa: Pass a set of controls and return results from > > ipa::start() > > pipeline: ipa: raspberrypi: Pass controls to IPA on start > > > > Documentation/guides/pipeline-handler.rst | 4 +- > > include/libcamera/camera.h | 2 +- > > .../libcamera/internal/ipa_context_wrapper.h | 3 +- > > include/libcamera/internal/pipeline_handler.h | 2 +- > > include/libcamera/ipa/ipa_interface.h | 3 +- > > include/libcamera/ipa/raspberrypi.h | 1 + > > src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 55 ++++++++++++------- > > src/ipa/rkisp1/rkisp1.cpp | 3 +- > > src/ipa/vimc/vimc.cpp | 6 +- > > src/libcamera/camera.cpp | 11 ++-- > > src/libcamera/ipa_context_wrapper.cpp | 5 +- > > src/libcamera/ipa_interface.cpp | 7 +++ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > > .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++-- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++- > > src/libcamera/pipeline/simple/simple.cpp | 4 +- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++- > > src/libcamera/pipeline_handler.cpp | 1 + > > src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +- > > src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++-- > > test/ipa/ipa_interface_test.cpp | 3 +- > > test/ipa/ipa_wrappers_test.cpp | 5 +- > > 24 files changed, 117 insertions(+), 62 deletions(-) > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Tue, Dec 08, 2020 at 09:57:17AM +0000, Naushir Patuck wrote: > On Mon, 7 Dec 2020 at 23:35, Laurent Pinchart wrote: > > On Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote: > > > Hi, > > > > > > Here is patchset v3 for the work on passing controls on > > > camera::start(). I have addressed Jacopo's review comments, and > > > rebased to the top of master. > > > > This looks good to me. I've replied with minor comments on 1/3 and 2/3 > > which I could address when applying the series. For 3/3, there's an open > > question of whether we should drop the initial control set at > > configure() time. If you send a v4 with this change, could you > > incorporate the review feedback for the other patches too ? > > Thanks for the review feedback for all the patches. Regarding the question > of removing the initial control set at configure() time, I think it still > is needed. That set at configure() applies the default exposure time/gain > to the sensor at start of today, whereas the set at start() only applies > the exposure/gain if the controller wants a change to those values. On the > first run, the controller will have no state history, so will not provide > values to set in start(). The only time this is not true is if a user has > provided some manual exposure/gain controls for the agc to use. Then we do > the RPi::IPA_CONFIG_SENSOR twice, once with the default values, then > overriding that with the user values. Right, I understand that. My question was whether we shouldn't instead skip RPi::IPA_CONFIG_SENSOR at configure() time, and do it unconditionally at start() time. Now that I've read your answer, I understand we don't necessarily want to apply new control values at start() time when restarting the camera without reconfiguring it, but I'm concerned about how this will interact with staggered writes (or the new DelayedControls class). For instance, if an application calls Camera::configure() multiple times in a row before Camera::start(), there will be multiple sensor control writes enqueued in the staggered writes mechanism, which I think could create issues. Wouldn't it be simpler, from a pipeline handler point of view, if every time the camera is started the IPA provided initial sensor controls once and once only ? > If you are ok with everything else, feel free to apply all your suggested > changes to the patches and merge. If instead you would prefer me to create > a v4 with all the changes, happy to do so as well, let me know. > > > > Naushir Patuck (3): > > > libcamera: pipeline: Pass libcamera controls into > > > pipeline_handler::start() > > > libcamera: ipa: Pass a set of controls and return results from > > > ipa::start() > > > pipeline: ipa: raspberrypi: Pass controls to IPA on start > > > > > > Documentation/guides/pipeline-handler.rst | 4 +- > > > include/libcamera/camera.h | 2 +- > > > .../libcamera/internal/ipa_context_wrapper.h | 3 +- > > > include/libcamera/internal/pipeline_handler.h | 2 +- > > > include/libcamera/ipa/ipa_interface.h | 3 +- > > > include/libcamera/ipa/raspberrypi.h | 1 + > > > src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +- > > > src/ipa/raspberrypi/raspberrypi.cpp | 55 ++++++++++++------- > > > src/ipa/rkisp1/rkisp1.cpp | 3 +- > > > src/ipa/vimc/vimc.cpp | 6 +- > > > src/libcamera/camera.cpp | 11 ++-- > > > src/libcamera/ipa_context_wrapper.cpp | 5 +- > > > src/libcamera/ipa_interface.cpp | 7 +++ > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++-- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++- > > > src/libcamera/pipeline/simple/simple.cpp | 4 +- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++- > > > src/libcamera/pipeline_handler.cpp | 1 + > > > src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +- > > > src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++-- > > > test/ipa/ipa_interface_test.cpp | 3 +- > > > test/ipa/ipa_wrappers_test.cpp | 5 +- > > > 24 files changed, 117 insertions(+), 62 deletions(-)
Hi Laurent, On Tue, 8 Dec 2020 at 11:07, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Tue, Dec 08, 2020 at 09:57:17AM +0000, Naushir Patuck wrote: > > On Mon, 7 Dec 2020 at 23:35, Laurent Pinchart wrote: > > > On Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote: > > > > Hi, > > > > > > > > Here is patchset v3 for the work on passing controls on > > > > camera::start(). I have addressed Jacopo's review comments, and > > > > rebased to the top of master. > > > > > > This looks good to me. I've replied with minor comments on 1/3 and 2/3 > > > which I could address when applying the series. For 3/3, there's an > open > > > question of whether we should drop the initial control set at > > > configure() time. If you send a v4 with this change, could you > > > incorporate the review feedback for the other patches too ? > > > > Thanks for the review feedback for all the patches. Regarding the > question > > of removing the initial control set at configure() time, I think it still > > is needed. That set at configure() applies the default exposure > time/gain > > to the sensor at start of today, whereas the set at start() only applies > > the exposure/gain if the controller wants a change to those values. On > the > > first run, the controller will have no state history, so will not provide > > values to set in start(). The only time this is not true is if a user > has > > provided some manual exposure/gain controls for the agc to use. Then we > do > > the RPi::IPA_CONFIG_SENSOR twice, once with the default values, then > > overriding that with the user values. > > Right, I understand that. My question was whether we shouldn't instead > skip RPi::IPA_CONFIG_SENSOR at configure() time, and do it > unconditionally at start() time. Now that I've read your answer, I > understand we don't necessarily want to apply new control values at > start() time when restarting the camera without reconfiguring it, but > I'm concerned about how this will interact with staggered writes (or the > new DelayedControls class). For instance, if an application calls > Camera::configure() multiple times in a row before Camera::start(), > there will be multiple sensor control writes enqueued in the staggered > writes mechanism, which I think could create issues. Wouldn't it be > simpler, from a pipeline handler point of view, if every time the camera > is started the IPA provided initial sensor controls once and once only ? > Yes, this is a valid concern. I get around this issue in the staggered writer by having a reset() mechanism. Just before stream on, I call staggered write reset(), and this method write the last set control values to the device, and throw away all earlier values. This means that on start(), there will only ever be one (the latest) set of controls to use. DelayedControls has implemented a similar reset() mechanism. However, in DelayedControls::reset(), it gets given a list of controls as an argument to replace the existing controls in the queue. Niklas, please correct me if this is wrong :) I did talk about a flush() mechanism that mirrors closer to what the staggered writer does as well. Regards, Naush > > > If you are ok with everything else, feel free to apply all your suggested > > changes to the patches and merge. If instead you would prefer me to > create > > a v4 with all the changes, happy to do so as well, let me know. > > > > > > Naushir Patuck (3): > > > > libcamera: pipeline: Pass libcamera controls into > > > > pipeline_handler::start() > > > > libcamera: ipa: Pass a set of controls and return results from > > > > ipa::start() > > > > pipeline: ipa: raspberrypi: Pass controls to IPA on start > > > > > > > > Documentation/guides/pipeline-handler.rst | 4 +- > > > > include/libcamera/camera.h | 2 +- > > > > .../libcamera/internal/ipa_context_wrapper.h | 3 +- > > > > include/libcamera/internal/pipeline_handler.h | 2 +- > > > > include/libcamera/ipa/ipa_interface.h | 3 +- > > > > include/libcamera/ipa/raspberrypi.h | 1 + > > > > src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +- > > > > src/ipa/raspberrypi/raspberrypi.cpp | 55 > ++++++++++++------- > > > > src/ipa/rkisp1/rkisp1.cpp | 3 +- > > > > src/ipa/vimc/vimc.cpp | 6 +- > > > > src/libcamera/camera.cpp | 11 ++-- > > > > src/libcamera/ipa_context_wrapper.cpp | 5 +- > > > > src/libcamera/ipa_interface.cpp | 7 +++ > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++-- > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++- > > > > src/libcamera/pipeline/simple/simple.cpp | 4 +- > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++- > > > > src/libcamera/pipeline_handler.cpp | 1 + > > > > src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +- > > > > src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++-- > > > > test/ipa/ipa_interface_test.cpp | 3 +- > > > > test/ipa/ipa_wrappers_test.cpp | 5 +- > > > > 24 files changed, 117 insertions(+), 62 deletions(-) > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Tue, Dec 08, 2020 at 11:20:56AM +0000, Naushir Patuck wrote: > On Tue, 8 Dec 2020 at 11:07, Laurent Pinchart wrote: > > On Tue, Dec 08, 2020 at 09:57:17AM +0000, Naushir Patuck wrote: > > > On Mon, 7 Dec 2020 at 23:35, Laurent Pinchart wrote: > > > > On Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote: > > > > > Hi, > > > > > > > > > > Here is patchset v3 for the work on passing controls on > > > > > camera::start(). I have addressed Jacopo's review comments, and > > > > > rebased to the top of master. > > > > > > > > This looks good to me. I've replied with minor comments on 1/3 and 2/3 > > > > which I could address when applying the series. For 3/3, there's an open > > > > question of whether we should drop the initial control set at > > > > configure() time. If you send a v4 with this change, could you > > > > incorporate the review feedback for the other patches too ? > > > > > > Thanks for the review feedback for all the patches. Regarding the question > > > of removing the initial control set at configure() time, I think it still > > > is needed. That set at configure() applies the default exposure time/gain > > > to the sensor at start of today, whereas the set at start() only applies > > > the exposure/gain if the controller wants a change to those values. On the > > > first run, the controller will have no state history, so will not provide > > > values to set in start(). The only time this is not true is if a user has > > > provided some manual exposure/gain controls for the agc to use. Then we do > > > the RPi::IPA_CONFIG_SENSOR twice, once with the default values, then > > > overriding that with the user values. > > > > Right, I understand that. My question was whether we shouldn't instead > > skip RPi::IPA_CONFIG_SENSOR at configure() time, and do it > > unconditionally at start() time. Now that I've read your answer, I > > understand we don't necessarily want to apply new control values at > > start() time when restarting the camera without reconfiguring it, but > > I'm concerned about how this will interact with staggered writes (or the > > new DelayedControls class). For instance, if an application calls > > Camera::configure() multiple times in a row before Camera::start(), > > there will be multiple sensor control writes enqueued in the staggered > > writes mechanism, which I think could create issues. Wouldn't it be > > simpler, from a pipeline handler point of view, if every time the camera > > is started the IPA provided initial sensor controls once and once only ? > > Yes, this is a valid concern. I get around this issue in the staggered > writer by having a reset() mechanism. Just before stream on, I call > staggered write reset(), and this method write the last set control values > to the device, and throw away all earlier values. This means that on > start(), there will only ever be one (the latest) set of controls to use. > DelayedControls has implemented a similar reset() mechanism. However, in > DelayedControls::reset(), it gets given a list of controls as an argument > to replace the existing controls in the queue. Niklas, please correct me > if this is wrong :) I did talk about a flush() mechanism that mirrors > closer to what the staggered writer does as well. Thank you for the explanation. My understanding is that there are multiple ways to address this issue, with different pending patch series that effects this, and it hasn't been decided yet how it will all look like in the end. There's thus no need to change the way controls are handled in this series, we can rework that as part of the work on DelayedControls, if needed. I've pushed the series with the small suggested changes. > > > If you are ok with everything else, feel free to apply all your suggested > > > changes to the patches and merge. If instead you would prefer me to create > > > a v4 with all the changes, happy to do so as well, let me know. > > > > > > > > Naushir Patuck (3): > > > > > libcamera: pipeline: Pass libcamera controls into > > > > > pipeline_handler::start() > > > > > libcamera: ipa: Pass a set of controls and return results from > > > > > ipa::start() > > > > > pipeline: ipa: raspberrypi: Pass controls to IPA on start > > > > > > > > > > Documentation/guides/pipeline-handler.rst | 4 +- > > > > > include/libcamera/camera.h | 2 +- > > > > > .../libcamera/internal/ipa_context_wrapper.h | 3 +- > > > > > include/libcamera/internal/pipeline_handler.h | 2 +- > > > > > include/libcamera/ipa/ipa_interface.h | 3 +- > > > > > include/libcamera/ipa/raspberrypi.h | 1 + > > > > > src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +- > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 55 ++++++++++++------- > > > > > src/ipa/rkisp1/rkisp1.cpp | 3 +- > > > > > src/ipa/vimc/vimc.cpp | 6 +- > > > > > src/libcamera/camera.cpp | 11 ++-- > > > > > src/libcamera/ipa_context_wrapper.cpp | 5 +- > > > > > src/libcamera/ipa_interface.cpp | 7 +++ > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++-- > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++- > > > > > src/libcamera/pipeline/simple/simple.cpp | 4 +- > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++- > > > > > src/libcamera/pipeline_handler.cpp | 1 + > > > > > src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +- > > > > > src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++-- > > > > > test/ipa/ipa_interface_test.cpp | 3 +- > > > > > test/ipa/ipa_wrappers_test.cpp | 5 +- > > > > > 24 files changed, 117 insertions(+), 62 deletions(-)
Hi, Here is patchset v3 for the work on passing controls on camera::start(). I have addressed Jacopo's review comments, and rebased to the top of master. Thanks, Naush Naushir Patuck (3): libcamera: pipeline: Pass libcamera controls into pipeline_handler::start() libcamera: ipa: Pass a set of controls and return results from ipa::start() pipeline: ipa: raspberrypi: Pass controls to IPA on start Documentation/guides/pipeline-handler.rst | 4 +- include/libcamera/camera.h | 2 +- .../libcamera/internal/ipa_context_wrapper.h | 3 +- include/libcamera/internal/pipeline_handler.h | 2 +- include/libcamera/ipa/ipa_interface.h | 3 +- include/libcamera/ipa/raspberrypi.h | 1 + src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +- src/ipa/raspberrypi/raspberrypi.cpp | 55 ++++++++++++------- src/ipa/rkisp1/rkisp1.cpp | 3 +- src/ipa/vimc/vimc.cpp | 6 +- src/libcamera/camera.cpp | 11 ++-- src/libcamera/ipa_context_wrapper.cpp | 5 +- src/libcamera/ipa_interface.cpp | 7 +++ src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++-- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++- src/libcamera/pipeline/simple/simple.cpp | 4 +- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- src/libcamera/pipeline/vimc/vimc.cpp | 7 ++- src/libcamera/pipeline_handler.cpp | 1 + src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +- src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++-- test/ipa/ipa_interface_test.cpp | 3 +- test/ipa/ipa_wrappers_test.cpp | 5 +- 24 files changed, 117 insertions(+), 62 deletions(-)