[libcamera-devel,v4,5/8] libcamera: camera_sensor: Expose the camera device
diff mbox series

Message ID 20201215004811.602429-6-niklas.soderlund@ragnatech.se
State Superseded
Delegated to: Niklas Söderlund
Headers show
Series
  • libcamera: Add helper for controls that take effect with a delay
Related show

Commit Message

Niklas Söderlund Dec. 15, 2020, 12:48 a.m. UTC 
Expose the device backing the CameraSensor instance.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
* Changes since v3
- New, needed to be able to move the DelayedControls handling out of
  CameraSensor.
---
 include/libcamera/internal/camera_sensor.h | 2 ++
 src/libcamera/camera_sensor.cpp            | 6 ++++++
 2 files changed, 8 insertions(+)

Comments

Jacopo Mondi Dec. 17, 2020, 2:12 p.m. UTC | #1 
On Tue, Dec 15, 2020 at 01:48:08AM +0100, Niklas Söderlund wrote:
> Expose the device backing the CameraSensor instance.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * Changes since v3
> - New, needed to be able to move the DelayedControls handling out of
>   CameraSensor.
> ---
>  include/libcamera/internal/camera_sensor.h | 2 ++
>  src/libcamera/camera_sensor.cpp            | 6 ++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index f80d836161a564e7..a70e024aa327f69b 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -61,6 +61,8 @@ public:
>  	ControlList getControls(const std::vector<uint32_t> &ids);
>  	int setControls(ControlList *ctrls);
>
> +	V4L2Subdevice *device() { return subdev_.get(); }
> +
>  	const ControlList &properties() const { return properties_; }
>  	int sensorInfo(CameraSensorInfo *info) const;
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 1628ba9c892b0eaf..df45b2b803617bfd 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -486,6 +486,12 @@ ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)
>  	return subdev_->getControls(ids);
>  }
>
> +/**
> + * \fn CameraSensor::device()
> + * \brief Retrieve the camera sensor device
> + * \return The camera sensor device
> + */
> +

I wonder if subdevice() would be better.
A minor though

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

Thanks
  j

>  /**
>   * \fn CameraSensor::properties()
>   * \brief Retrieve the camera sensor properties
> --
> 2.29.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Jan. 8, 2021, 3:28 p.m. UTC | #2 
Hi Jacopo,

Thanks for your feedback.

On 2020-12-17 15:12:16 +0100, Jacopo Mondi wrote:
> On Tue, Dec 15, 2020 at 01:48:08AM +0100, Niklas Söderlund wrote:
> > Expose the device backing the CameraSensor instance.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> > * Changes since v3
> > - New, needed to be able to move the DelayedControls handling out of
> >   CameraSensor.
> > ---
> >  include/libcamera/internal/camera_sensor.h | 2 ++
> >  src/libcamera/camera_sensor.cpp            | 6 ++++++
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index f80d836161a564e7..a70e024aa327f69b 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -61,6 +61,8 @@ public:
> >  	ControlList getControls(const std::vector<uint32_t> &ids);
> >  	int setControls(ControlList *ctrls);
> >
> > +	V4L2Subdevice *device() { return subdev_.get(); }
> > +
> >  	const ControlList &properties() const { return properties_; }
> >  	int sensorInfo(CameraSensorInfo *info) const;
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 1628ba9c892b0eaf..df45b2b803617bfd 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -486,6 +486,12 @@ ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)
> >  	return subdev_->getControls(ids);
> >  }
> >
> > +/**
> > + * \fn CameraSensor::device()
> > + * \brief Retrieve the camera sensor device
> > + * \return The camera sensor device
> > + */
> > +
> 
> I wonder if subdevice() would be better.
> A minor though

I thin device is 'nicer' as what it does is retrieve the device which 
represents the sensor. That being said I don't feel strongly about it.  
And hopefully we can remove this method in the future and not expose it 
outside CameraSensor. This is only needed until we decide how to best 
integrate DelayedControls inside CameraSensro.

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

Thanks.

> 
> Thanks
>   j
> 
> >  /**
> >   * \fn CameraSensor::properties()
> >   * \brief Retrieve the camera sensor properties
> > --
> > 2.29.2
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 10, 2021, 2:03 p.m. UTC | #3 
Hi Niklas,

Thank you for the patch.

On Fri, Jan 08, 2021 at 04:28:54PM +0100, Niklas Söderlund wrote:
> On 2020-12-17 15:12:16 +0100, Jacopo Mondi wrote:
> > On Tue, Dec 15, 2020 at 01:48:08AM +0100, Niklas Söderlund wrote:
> > > Expose the device backing the CameraSensor instance.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > ---
> > > * Changes since v3
> > > - New, needed to be able to move the DelayedControls handling out of
> > >   CameraSensor.
> > > ---
> > >  include/libcamera/internal/camera_sensor.h | 2 ++
> > >  src/libcamera/camera_sensor.cpp            | 6 ++++++
> > >  2 files changed, 8 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > index f80d836161a564e7..a70e024aa327f69b 100644
> > > --- a/include/libcamera/internal/camera_sensor.h
> > > +++ b/include/libcamera/internal/camera_sensor.h
> > > @@ -61,6 +61,8 @@ public:
> > >  	ControlList getControls(const std::vector<uint32_t> &ids);
> > >  	int setControls(ControlList *ctrls);
> > >
> > > +	V4L2Subdevice *device() { return subdev_.get(); }
> > > +
> > >  	const ControlList &properties() const { return properties_; }
> > >  	int sensorInfo(CameraSensorInfo *info) const;
> > >
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index 1628ba9c892b0eaf..df45b2b803617bfd 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -486,6 +486,12 @@ ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)
> > >  	return subdev_->getControls(ids);
> > >  }
> > >
> > > +/**
> > > + * \fn CameraSensor::device()
> > > + * \brief Retrieve the camera sensor device
> > > + * \return The camera sensor device
> > > + */
> > > +
> > 
> > I wonder if subdevice() would be better.
> > A minor though
> 
> I thin device is 'nicer' as what it does is retrieve the device which 
> represents the sensor. That being said I don't feel strongly about it.  
> And hopefully we can remove this method in the future and not expose it 
> outside CameraSensor. This is only needed until we decide how to best 
> integrate DelayedControls inside CameraSensro.

Could you add a

	\todo Remove this function by integrating DelayedControl with
	CameraSensor

? With that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Do you have plans to address this integration ?

> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Thanks.
> 
> > >  /**
> > >   * \fn CameraSensor::properties()
> > >   * \brief Retrieve the camera sensor properties
Niklas Söderlund Jan. 16, 2021, 10:17 a.m. UTC | #4 
Hi Laurent,

Thanks for your comment.

On 2021-01-10 16:03:43 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Fri, Jan 08, 2021 at 04:28:54PM +0100, Niklas Söderlund wrote:
> > On 2020-12-17 15:12:16 +0100, Jacopo Mondi wrote:
> > > On Tue, Dec 15, 2020 at 01:48:08AM +0100, Niklas Söderlund wrote:
> > > > Expose the device backing the CameraSensor instance.
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > ---
> > > > * Changes since v3
> > > > - New, needed to be able to move the DelayedControls handling out of
> > > >   CameraSensor.
> > > > ---
> > > >  include/libcamera/internal/camera_sensor.h | 2 ++
> > > >  src/libcamera/camera_sensor.cpp            | 6 ++++++
> > > >  2 files changed, 8 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > index f80d836161a564e7..a70e024aa327f69b 100644
> > > > --- a/include/libcamera/internal/camera_sensor.h
> > > > +++ b/include/libcamera/internal/camera_sensor.h
> > > > @@ -61,6 +61,8 @@ public:
> > > >  	ControlList getControls(const std::vector<uint32_t> &ids);
> > > >  	int setControls(ControlList *ctrls);
> > > >
> > > > +	V4L2Subdevice *device() { return subdev_.get(); }
> > > > +
> > > >  	const ControlList &properties() const { return properties_; }
> > > >  	int sensorInfo(CameraSensorInfo *info) const;
> > > >
> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > index 1628ba9c892b0eaf..df45b2b803617bfd 100644
> > > > --- a/src/libcamera/camera_sensor.cpp
> > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > @@ -486,6 +486,12 @@ ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)
> > > >  	return subdev_->getControls(ids);
> > > >  }
> > > >
> > > > +/**
> > > > + * \fn CameraSensor::device()
> > > > + * \brief Retrieve the camera sensor device
> > > > + * \return The camera sensor device
> > > > + */
> > > > +
> > > 
> > > I wonder if subdevice() would be better.
> > > A minor though
> > 
> > I thin device is 'nicer' as what it does is retrieve the device which 
> > represents the sensor. That being said I don't feel strongly about it.  
> > And hopefully we can remove this method in the future and not expose it 
> > outside CameraSensor. This is only needed until we decide how to best 
> > integrate DelayedControls inside CameraSensro.
> 
> Could you add a
> 
> 	\todo Remove this function by integrating DelayedControl with
> 	CameraSensor
> 
> ? With that,

Good idea.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks.

> 
> Do you have plans to address this integration ?

Yes, I had a go at the integration in v1 and v2 but was pushed back to 
make this series less controversial. I hope to find a way to integrate 
this ASAP as I think some of the things this series introduce in 
pipeline handlers are quiet ugly :-) I think the killer combo for the 
first integration step will be the addition of the camera 
database/configuration files.

> 
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > Thanks.
> > 
> > > >  /**
> > > >   * \fn CameraSensor::properties()
> > > >   * \brief Retrieve the camera sensor properties
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index f80d836161a564e7..a70e024aa327f69b 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -61,6 +61,8 @@  public:
 	ControlList getControls(const std::vector<uint32_t> &ids);
 	int setControls(ControlList *ctrls);
 
+	V4L2Subdevice *device() { return subdev_.get(); }
+
 	const ControlList &properties() const { return properties_; }
 	int sensorInfo(CameraSensorInfo *info) const;
 
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index 1628ba9c892b0eaf..df45b2b803617bfd 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -486,6 +486,12 @@  ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)
 	return subdev_->getControls(ids);
 }
 
+/**
+ * \fn CameraSensor::device()
+ * \brief Retrieve the camera sensor device
+ * \return The camera sensor device
+ */
+
 /**
  * \fn CameraSensor::properties()
  * \brief Retrieve the camera sensor properties