[libcamera-devel,v2,2/3] rkisp1: Add camera lens to PH and expose it to the IPA
diff mbox series

Message ID 20220809144704.61682-3-dse@thaumatec.com
State New
Headers show
Series
  • libcamera: rkisp1: Add lens control
Related show

Commit Message

Daniel Semkowicz Aug. 9, 2022, 2:47 p.m. UTC 
Check in pipeline handler if camera lens exists, add expose its controls
to the IPA.

Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
---
 src/ipa/rkisp1/rkisp1.cpp                | 7 +++++++
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++++
 2 files changed, 12 insertions(+)

Comments

Jacopo Mondi Aug. 9, 2022, 3:20 p.m. UTC | #1 
Hi Daniel

On Tue, Aug 09, 2022 at 04:47:03PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> Check in pipeline handler if camera lens exists, add expose its controls
> to the IPA.
>
> Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> ---
>  src/ipa/rkisp1/rkisp1.cpp                | 7 +++++++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index ee484845..9e4c48a2 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -7,6 +7,7 @@
>
>  #include <algorithm>
>  #include <math.h>
> +#include <optional>
>  #include <queue>
>  #include <stdint.h>
>  #include <string.h>
> @@ -70,6 +71,7 @@ private:
>  	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
>
>  	ControlInfoMap sensorCtrls_;
> +	std::optional<ControlInfoMap> lensCtrls_;
>
>  	/* Camera sensor controls. */
>  	bool autoExposure_;
> @@ -201,6 +203,11 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>
>  	sensorCtrls_ = entityControls.at(0);
>
> +	auto lensControls = entityControls.find(1);
> +	if (lensControls != entityControls.end()) {
> +		lensCtrls_ = lensControls->second;
> +	}

no {} for single line statements..

I can fix while applying.

I would like to collect 1 and 2 asap as I'm reworking the RkISP IPA
interface and doing it on top of this would be easier

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> +
>  	const auto itExp = sensorCtrls_.find(V4L2_CID_EXPOSURE);
>  	if (itExp == sensorCtrls_.end()) {
>  		LOG(IPARkISP1, Error) << "Can't find exposure control";
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 93287332..5f10c26b 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -28,6 +28,7 @@
>  #include <libcamera/stream.h>
>
>  #include "libcamera/internal/camera.h"
> +#include "libcamera/internal/camera_lens.h"
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/delayed_controls.h"
>  #include "libcamera/internal/device_enumerator.h"
> @@ -694,6 +695,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	std::map<uint32_t, ControlInfoMap> entityControls;
>  	entityControls.emplace(0, data->sensor_->controls());
>
> +	CameraLens *lens = data->sensor_->focusLens();
> +	if (lens)
> +		entityControls.emplace(1, lens->controls());
> +
>  	ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);
>  	if (ret) {
>  		LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";
> --
> 2.34.1
>
Kieran Bingham Aug. 9, 2022, 3:41 p.m. UTC | #2 
Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:20:34)
> Hi Daniel
> 
> On Tue, Aug 09, 2022 at 04:47:03PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > Check in pipeline handler if camera lens exists, add expose its controls
> > to the IPA.
> >
> > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> > ---
> >  src/ipa/rkisp1/rkisp1.cpp                | 7 +++++++
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index ee484845..9e4c48a2 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -7,6 +7,7 @@
> >
> >  #include <algorithm>
> >  #include <math.h>
> > +#include <optional>
> >  #include <queue>
> >  #include <stdint.h>
> >  #include <string.h>
> > @@ -70,6 +71,7 @@ private:
> >       std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
> >
> >       ControlInfoMap sensorCtrls_;
> > +     std::optional<ControlInfoMap> lensCtrls_;
> >
> >       /* Camera sensor controls. */
> >       bool autoExposure_;
> > @@ -201,6 +203,11 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> >
> >       sensorCtrls_ = entityControls.at(0);
> >
> > +     auto lensControls = entityControls.find(1);
> > +     if (lensControls != entityControls.end()) {
> > +             lensCtrls_ = lensControls->second;
> > +     }
> 
> no {} for single line statements..
> 
> I can fix while applying.
> 
> I would like to collect 1 and 2 asap as I'm reworking the RkISP IPA
> interface and doing it on top of this would be easier
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

With the above sounds fine to me.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Thanks
>    j
> 
> > +
> >       const auto itExp = sensorCtrls_.find(V4L2_CID_EXPOSURE);
> >       if (itExp == sensorCtrls_.end()) {
> >               LOG(IPARkISP1, Error) << "Can't find exposure control";
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 93287332..5f10c26b 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -28,6 +28,7 @@
> >  #include <libcamera/stream.h>
> >
> >  #include "libcamera/internal/camera.h"
> > +#include "libcamera/internal/camera_lens.h"
> >  #include "libcamera/internal/camera_sensor.h"
> >  #include "libcamera/internal/delayed_controls.h"
> >  #include "libcamera/internal/device_enumerator.h"
> > @@ -694,6 +695,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >       std::map<uint32_t, ControlInfoMap> entityControls;
> >       entityControls.emplace(0, data->sensor_->controls());
> >
> > +     CameraLens *lens = data->sensor_->focusLens();
> > +     if (lens)
> > +             entityControls.emplace(1, lens->controls());
> > +
> >       ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> >       if (ret) {
> >               LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";
> > --
> > 2.34.1
> >
Daniel Semkowicz Aug. 10, 2022, 7:02 a.m. UTC | #3 
Hi!

On Tue, Aug 9, 2022 at 5:41 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:20:34)
> > Hi Daniel
> >
> > On Tue, Aug 09, 2022 at 04:47:03PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > Check in pipeline handler if camera lens exists, add expose its controls
> > > to the IPA.
> > >
> > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> > > ---
> > >  src/ipa/rkisp1/rkisp1.cpp                | 7 +++++++
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++++
> > >  2 files changed, 12 insertions(+)
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index ee484845..9e4c48a2 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -7,6 +7,7 @@
> > >
> > >  #include <algorithm>
> > >  #include <math.h>
> > > +#include <optional>
> > >  #include <queue>
> > >  #include <stdint.h>
> > >  #include <string.h>
> > > @@ -70,6 +71,7 @@ private:
> > >       std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
> > >
> > >       ControlInfoMap sensorCtrls_;
> > > +     std::optional<ControlInfoMap> lensCtrls_;
> > >
> > >       /* Camera sensor controls. */
> > >       bool autoExposure_;
> > > @@ -201,6 +203,11 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > >
> > >       sensorCtrls_ = entityControls.at(0);
> > >
> > > +     auto lensControls = entityControls.find(1);
> > > +     if (lensControls != entityControls.end()) {
> > > +             lensCtrls_ = lensControls->second;
> > > +     }
> >
> > no {} for single line statements..
> >
> > I can fix while applying.

Thanks!
I am wondering if We could add this to the ./utils/checkstyle.py script.
I see this is a common mistake for me, but the checheckstyle.py is
blind for these types of mistakes.

Best regards
Daniel

> >
> > I would like to collect 1 and 2 asap as I'm reworking the RkISP IPA
> > interface and doing it on top of this would be easier
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> With the above sounds fine to me.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >
> > Thanks
> >    j
> >
> > > +
> > >       const auto itExp = sensorCtrls_.find(V4L2_CID_EXPOSURE);
> > >       if (itExp == sensorCtrls_.end()) {
> > >               LOG(IPARkISP1, Error) << "Can't find exposure control";
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 93287332..5f10c26b 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -28,6 +28,7 @@
> > >  #include <libcamera/stream.h>
> > >
> > >  #include "libcamera/internal/camera.h"
> > > +#include "libcamera/internal/camera_lens.h"
> > >  #include "libcamera/internal/camera_sensor.h"
> > >  #include "libcamera/internal/delayed_controls.h"
> > >  #include "libcamera/internal/device_enumerator.h"
> > > @@ -694,6 +695,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > >       std::map<uint32_t, ControlInfoMap> entityControls;
> > >       entityControls.emplace(0, data->sensor_->controls());
> > >
> > > +     CameraLens *lens = data->sensor_->focusLens();
> > > +     if (lens)
> > > +             entityControls.emplace(1, lens->controls());
> > > +
> > >       ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > >       if (ret) {
> > >               LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";
> > > --
> > > 2.34.1
> > >

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index ee484845..9e4c48a2 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -7,6 +7,7 @@ 
 
 #include <algorithm>
 #include <math.h>
+#include <optional>
 #include <queue>
 #include <stdint.h>
 #include <string.h>
@@ -70,6 +71,7 @@  private:
 	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
 
 	ControlInfoMap sensorCtrls_;
+	std::optional<ControlInfoMap> lensCtrls_;
 
 	/* Camera sensor controls. */
 	bool autoExposure_;
@@ -201,6 +203,11 @@  int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
 
 	sensorCtrls_ = entityControls.at(0);
 
+	auto lensControls = entityControls.find(1);
+	if (lensControls != entityControls.end()) {
+		lensCtrls_ = lensControls->second;
+	}
+
 	const auto itExp = sensorCtrls_.find(V4L2_CID_EXPOSURE);
 	if (itExp == sensorCtrls_.end()) {
 		LOG(IPARkISP1, Error) << "Can't find exposure control";
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 93287332..5f10c26b 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -28,6 +28,7 @@ 
 #include <libcamera/stream.h>
 
 #include "libcamera/internal/camera.h"
+#include "libcamera/internal/camera_lens.h"
 #include "libcamera/internal/camera_sensor.h"
 #include "libcamera/internal/delayed_controls.h"
 #include "libcamera/internal/device_enumerator.h"
@@ -694,6 +695,10 @@  int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
 	std::map<uint32_t, ControlInfoMap> entityControls;
 	entityControls.emplace(0, data->sensor_->controls());
 
+	CameraLens *lens = data->sensor_->focusLens();
+	if (lens)
+		entityControls.emplace(1, lens->controls());
+
 	ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);
 	if (ret) {
 		LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";