<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 7 Dec 2020 at 23:35, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote:<br>
> Hi,<br>
> <br>
> Here is patchset v3 for the work on passing controls on<br>
> camera::start(). I have addressed Jacopo's review comments, and<br>
> rebased to the top of master.<br>
<br>
This looks good to me. I've replied with minor comments on 1/3 and 2/3<br>
which I could address when applying the series. For 3/3, there's an open<br>
question of whether we should drop the initial control set at<br>
configure() time. If you send a v4 with this change, could you<br>
incorporate the review feedback for the other patches too ?<br></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Naushir Patuck (3):<br>
> libcamera: pipeline: Pass libcamera controls into<br>
> pipeline_handler::start()<br>
> libcamera: ipa: Pass a set of controls and return results from<br>
> ipa::start()<br>
> pipeline: ipa: raspberrypi: Pass controls to IPA on start<br>
> <br>
> Documentation/guides/pipeline-handler.rst | 4 +-<br>
> include/libcamera/camera.h | 2 +-<br>
> .../libcamera/internal/ipa_context_wrapper.h | 3 +-<br>
> include/libcamera/internal/pipeline_handler.h | 2 +-<br>
> include/libcamera/ipa/ipa_interface.h | 3 +-<br>
> include/libcamera/ipa/raspberrypi.h | 1 +<br>
> src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +-<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 55 ++++++++++++-------<br>
> src/ipa/rkisp1/rkisp1.cpp | 3 +-<br>
> src/ipa/vimc/vimc.cpp | 6 +-<br>
> src/libcamera/camera.cpp | 11 ++--<br>
> src/libcamera/ipa_context_wrapper.cpp | 5 +-<br>
> src/libcamera/ipa_interface.cpp | 7 +++<br>
> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++--<br>
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++-<br>
> src/libcamera/pipeline/simple/simple.cpp | 4 +-<br>
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-<br>
> src/libcamera/pipeline/vimc/vimc.cpp | 7 ++-<br>
> src/libcamera/pipeline_handler.cpp | 1 +<br>
> src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +-<br>
> src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++--<br>
> test/ipa/ipa_interface_test.cpp | 3 +-<br>
> test/ipa/ipa_wrappers_test.cpp | 5 +-<br>
> 24 files changed, 117 insertions(+), 62 deletions(-)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>