[libcamera-devel,v2,08/13] libcamera: ipu3: imgu: Use specific functions to configure each sink
diff mbox series

Message ID 20200628001532.2685967-9-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: ipu3: Refactoring of ImgU
Related show

Commit Message

Niklas Söderlund June 28, 2020, 12:15 a.m. UTC 
When the IPU3 pipeline only provided streams to applications that came
from the ImgU it made sense to have a generic function to configure all
the different outputs. With the addition of the RAW stream this begins
to be cumbersome to read and make sense of in the PipelineHandlerIPU3
code. Replace the generic function that takes a specific argument for
which sink to configure with a specific function for each sink.

This makes the code easier to follow as it's always clear which of the
ImgU sinks are being configured without knowing the content of a
generically named variable. It also paves way for future improvements.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
* Changes since v1
- s/sens/sense/
- Retain comment for configureVideoDevice()
- Inline the accessors for configureVideoDevice() in the .h file
- Update comment in PipelineHandlerIPU3::configure()
---
 src/libcamera/pipeline/ipu3/imgu.cpp | 20 +++++++--------
 src/libcamera/pipeline/ipu3/imgu.h   | 28 ++++++++++++++++++--
 src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------
 3 files changed, 57 insertions(+), 29 deletions(-)

Comments

Laurent Pinchart June 28, 2020, 6:51 a.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Sun, Jun 28, 2020 at 02:15:27AM +0200, Niklas Söderlund wrote:
> When the IPU3 pipeline only provided streams to applications that came
> from the ImgU it made sense to have a generic function to configure all
> the different outputs. With the addition of the RAW stream this begins
> to be cumbersome to read and make sense of in the PipelineHandlerIPU3
> code. Replace the generic function that takes a specific argument for
> which sink to configure with a specific function for each sink.
> 
> This makes the code easier to follow as it's always clear which of the
> ImgU sinks are being configured without knowing the content of a
> generically named variable. It also paves way for future improvements.

s/paves way/paves the way/

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> * Changes since v1
> - s/sens/sense/
> - Retain comment for configureVideoDevice()
> - Inline the accessors for configureVideoDevice() in the .h file
> - Update comment in PipelineHandlerIPU3::configure()
> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp | 20 +++++++--------
>  src/libcamera/pipeline/ipu3/imgu.h   | 28 ++++++++++++++++++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------
>  3 files changed, 57 insertions(+), 29 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index 46d56a5d18dc3687..981239cba975f92e 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -139,18 +139,17 @@ int ImgUDevice::configureInput(const Size &size,
>  }
>  
>  /**
> - * \brief Configure the ImgU unit \a id video output
> - * \param[in] output The ImgU output device to configure
> + * \brief Configure a video device on the ImgU
> + * \param[in] dev The video device to configure
> + * \param[in] pad The pad of the ImgU subdevice
>   * \param[in] cfg The requested configuration
> + * \param[out] outputFormat The format set on the video device
>   * \return 0 on success or a negative error code otherwise
>   */
> -int ImgUDevice::configureOutput(ImgUOutput *output,
> -				const StreamConfiguration &cfg,
> -				V4L2DeviceFormat *outputFormat)
> +int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> +				     const StreamConfiguration &cfg,
> +				     V4L2DeviceFormat *outputFormat)
>  {
> -	V4L2VideoDevice *dev = output->dev;
> -	unsigned int pad = output->pad;
> -
>  	V4L2SubdeviceFormat imguFormat = {};
>  	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
>  	imguFormat.size = cfg.size;
> @@ -160,7 +159,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
>  		return ret;
>  
>  	/* No need to apply format to the stat node. */
> -	if (output == &stat_)
> +	if (dev == stat_.dev)
>  		return 0;
>  
>  	*outputFormat = {};
> @@ -172,7 +171,8 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
>  	if (ret)
>  		return ret;
>  
> -	LOG(IPU3, Debug) << "ImgU " << output->name << " format = "
> +	const char *name = dev == output_.dev ? "output" : "viewfinder";
> +	LOG(IPU3, Debug) << "ImgU " << name << " format = "
>  			 << outputFormat->toString();
>  
>  	return 0;
> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> index b6b08b4fef2d3a9d..5f1dbfd8f45f7924 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.h
> +++ b/src/libcamera/pipeline/ipu3/imgu.h
> @@ -49,9 +49,29 @@ public:
>  	}
>  
>  	int init(MediaDevice *media, unsigned int index);
> +
>  	int configureInput(const Size &size, V4L2DeviceFormat *inputFormat);
> -	int configureOutput(ImgUOutput *output, const StreamConfiguration &cfg,
> -			    V4L2DeviceFormat *outputFormat);
> +
> +	int configureOutput(const StreamConfiguration &cfg,
> +			    V4L2DeviceFormat *outputFormat)
> +	{
> +		return configureVideoDevice(output_.dev, PAD_OUTPUT, cfg,
> +					    outputFormat);
> +	}
> +
> +	int configureViewfinder(const StreamConfiguration &cfg,
> +				V4L2DeviceFormat *outputFormat)
> +	{
> +		return configureVideoDevice(viewfinder_.dev, PAD_VF, cfg,
> +					    outputFormat);
> +	}
> +
> +	int configureStat(const StreamConfiguration &cfg,
> +			  V4L2DeviceFormat *outputFormat)
> +	{
> +		return configureVideoDevice(stat_.dev, PAD_STAT, cfg,
> +					    outputFormat);
> +	}
>  
>  	int allocateBuffers(unsigned int bufferCount);
>  	void freeBuffers();
> @@ -78,6 +98,10 @@ private:
>  		      const std::string &sink, unsigned int sinkPad,
>  		      bool enable);
>  
> +	int configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> +				 const StreamConfiguration &cfg,
> +				 V4L2DeviceFormat *outputFormat);
> +
>  	std::string name_;
>  	MediaDevice *media_;
>  };
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index b41a789e8dc2a7b2..e817f842f1216a7f 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -504,19 +504,25 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  		stream->active_ = true;
>  		cfg.setStream(stream);
>  
> -		/*
> -		 * The RAW still capture stream just copies buffers from the
> -		 * internal queue and doesn't need any specific configuration.
> -		 */
> -		if (stream->raw_) {
> +		if (stream == outStream) {
> +			ret = imgu->configureOutput(cfg, &outputFormat);
> +			if (ret)
> +				return ret;
> +
> +			cfg.stride = outputFormat.planes[0].bpl;
> +		} else if (stream == vfStream) {
> +			ret = imgu->configureViewfinder(cfg, &outputFormat);
> +			if (ret)
> +				return ret;
> +
> +			cfg.stride = outputFormat.planes[0].bpl;
> +		} else {
> +			/*
> +			 * The RAW still capture stream only uses en externel

s/ en / an /

and an external what ?

> +			 * for the already configured CIO2 sink and doesn't need

Isn't the CIO2 a source ?

I know what you mean here as I know the code, but for someone who's not
familiar with the IPU3 pipeline handler that would be a bit confusing.

> +			 * any specific configuration of the ImgU.
> +			 */
>  			cfg.stride = cio2Format.planes[0].bpl;
> -		} else {
> -			ret = imgu->configureOutput(stream->device_, cfg,
> -						    &outputFormat);
> -			if (ret)
> -				return ret;
> -
> -			cfg.stride = outputFormat.planes[0].bpl;
>  		}
>  	}
>  
> @@ -526,15 +532,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	 * be at least one active stream in the configuration request).
>  	 */
>  	if (!outStream->active_) {
> -		ret = imgu->configureOutput(outStream->device_, config->at(0),
> -					    &outputFormat);
> +		ret = imgu->configureOutput(config->at(0), &outputFormat);
>  		if (ret)
>  			return ret;
>  	}
>  
>  	if (!vfStream->active_) {
> -		ret = imgu->configureOutput(vfStream->device_, config->at(0),
> -					    &outputFormat);
> +		ret = imgu->configureViewfinder(config->at(0), &outputFormat);
>  		if (ret)
>  			return ret;
>  	}
> @@ -546,7 +550,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	StreamConfiguration statCfg = {};
>  	statCfg.size = cio2Format.size;
>  
> -	ret = imgu->configureOutput(&imgu->stat_, statCfg, &outputFormat);
> +	ret = imgu->configureStat(statCfg, &outputFormat);
>  	if (ret)
>  		return ret;
>
Niklas Söderlund June 28, 2020, 11:56 a.m. UTC | #2 
Hi Laurent,

Thanks for your feedback.

On 2020-06-28 09:51:30 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sun, Jun 28, 2020 at 02:15:27AM +0200, Niklas Söderlund wrote:
> > When the IPU3 pipeline only provided streams to applications that came
> > from the ImgU it made sense to have a generic function to configure all
> > the different outputs. With the addition of the RAW stream this begins
> > to be cumbersome to read and make sense of in the PipelineHandlerIPU3
> > code. Replace the generic function that takes a specific argument for
> > which sink to configure with a specific function for each sink.
> > 
> > This makes the code easier to follow as it's always clear which of the
> > ImgU sinks are being configured without knowing the content of a
> > generically named variable. It also paves way for future improvements.
> 
> s/paves way/paves the way/
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > * Changes since v1
> > - s/sens/sense/
> > - Retain comment for configureVideoDevice()
> > - Inline the accessors for configureVideoDevice() in the .h file
> > - Update comment in PipelineHandlerIPU3::configure()
> > ---
> >  src/libcamera/pipeline/ipu3/imgu.cpp | 20 +++++++--------
> >  src/libcamera/pipeline/ipu3/imgu.h   | 28 ++++++++++++++++++--
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------
> >  3 files changed, 57 insertions(+), 29 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index 46d56a5d18dc3687..981239cba975f92e 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -139,18 +139,17 @@ int ImgUDevice::configureInput(const Size &size,
> >  }
> >  
> >  /**
> > - * \brief Configure the ImgU unit \a id video output
> > - * \param[in] output The ImgU output device to configure
> > + * \brief Configure a video device on the ImgU
> > + * \param[in] dev The video device to configure
> > + * \param[in] pad The pad of the ImgU subdevice
> >   * \param[in] cfg The requested configuration
> > + * \param[out] outputFormat The format set on the video device
> >   * \return 0 on success or a negative error code otherwise
> >   */
> > -int ImgUDevice::configureOutput(ImgUOutput *output,
> > -				const StreamConfiguration &cfg,
> > -				V4L2DeviceFormat *outputFormat)
> > +int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> > +				     const StreamConfiguration &cfg,
> > +				     V4L2DeviceFormat *outputFormat)
> >  {
> > -	V4L2VideoDevice *dev = output->dev;
> > -	unsigned int pad = output->pad;
> > -
> >  	V4L2SubdeviceFormat imguFormat = {};
> >  	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
> >  	imguFormat.size = cfg.size;
> > @@ -160,7 +159,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> >  		return ret;
> >  
> >  	/* No need to apply format to the stat node. */
> > -	if (output == &stat_)
> > +	if (dev == stat_.dev)
> >  		return 0;
> >  
> >  	*outputFormat = {};
> > @@ -172,7 +171,8 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> >  	if (ret)
> >  		return ret;
> >  
> > -	LOG(IPU3, Debug) << "ImgU " << output->name << " format = "
> > +	const char *name = dev == output_.dev ? "output" : "viewfinder";
> > +	LOG(IPU3, Debug) << "ImgU " << name << " format = "
> >  			 << outputFormat->toString();
> >  
> >  	return 0;
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> > index b6b08b4fef2d3a9d..5f1dbfd8f45f7924 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.h
> > +++ b/src/libcamera/pipeline/ipu3/imgu.h
> > @@ -49,9 +49,29 @@ public:
> >  	}
> >  
> >  	int init(MediaDevice *media, unsigned int index);
> > +
> >  	int configureInput(const Size &size, V4L2DeviceFormat *inputFormat);
> > -	int configureOutput(ImgUOutput *output, const StreamConfiguration &cfg,
> > -			    V4L2DeviceFormat *outputFormat);
> > +
> > +	int configureOutput(const StreamConfiguration &cfg,
> > +			    V4L2DeviceFormat *outputFormat)
> > +	{
> > +		return configureVideoDevice(output_.dev, PAD_OUTPUT, cfg,
> > +					    outputFormat);
> > +	}
> > +
> > +	int configureViewfinder(const StreamConfiguration &cfg,
> > +				V4L2DeviceFormat *outputFormat)
> > +	{
> > +		return configureVideoDevice(viewfinder_.dev, PAD_VF, cfg,
> > +					    outputFormat);
> > +	}
> > +
> > +	int configureStat(const StreamConfiguration &cfg,
> > +			  V4L2DeviceFormat *outputFormat)
> > +	{
> > +		return configureVideoDevice(stat_.dev, PAD_STAT, cfg,
> > +					    outputFormat);
> > +	}
> >  
> >  	int allocateBuffers(unsigned int bufferCount);
> >  	void freeBuffers();
> > @@ -78,6 +98,10 @@ private:
> >  		      const std::string &sink, unsigned int sinkPad,
> >  		      bool enable);
> >  
> > +	int configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> > +				 const StreamConfiguration &cfg,
> > +				 V4L2DeviceFormat *outputFormat);
> > +
> >  	std::string name_;
> >  	MediaDevice *media_;
> >  };
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index b41a789e8dc2a7b2..e817f842f1216a7f 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -504,19 +504,25 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  		stream->active_ = true;
> >  		cfg.setStream(stream);
> >  
> > -		/*
> > -		 * The RAW still capture stream just copies buffers from the
> > -		 * internal queue and doesn't need any specific configuration.
> > -		 */
> > -		if (stream->raw_) {
> > +		if (stream == outStream) {
> > +			ret = imgu->configureOutput(cfg, &outputFormat);
> > +			if (ret)
> > +				return ret;
> > +
> > +			cfg.stride = outputFormat.planes[0].bpl;
> > +		} else if (stream == vfStream) {
> > +			ret = imgu->configureViewfinder(cfg, &outputFormat);
> > +			if (ret)
> > +				return ret;
> > +
> > +			cfg.stride = outputFormat.planes[0].bpl;
> > +		} else {
> > +			/*
> > +			 * The RAW still capture stream only uses en externel
> 
> s/ en / an /
> 
> and an external what ?
> 
> > +			 * for the already configured CIO2 sink and doesn't need
> 
> Isn't the CIO2 a source ?
> 
> I know what you mean here as I know the code, but for someone who's not
> familiar with the IPU3 pipeline handler that would be a bit confusing.

How about?

	/*
	 * The RAW stream is configured as part of the CIO2 and
	 * no configuration is needed for the ImgU.
	 */

> 
> > +			 * any specific configuration of the ImgU.
> > +			 */
> >  			cfg.stride = cio2Format.planes[0].bpl;
> > -		} else {
> > -			ret = imgu->configureOutput(stream->device_, cfg,
> > -						    &outputFormat);
> > -			if (ret)
> > -				return ret;
> > -
> > -			cfg.stride = outputFormat.planes[0].bpl;
> >  		}
> >  	}
> >  
> > @@ -526,15 +532,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  	 * be at least one active stream in the configuration request).
> >  	 */
> >  	if (!outStream->active_) {
> > -		ret = imgu->configureOutput(outStream->device_, config->at(0),
> > -					    &outputFormat);
> > +		ret = imgu->configureOutput(config->at(0), &outputFormat);
> >  		if (ret)
> >  			return ret;
> >  	}
> >  
> >  	if (!vfStream->active_) {
> > -		ret = imgu->configureOutput(vfStream->device_, config->at(0),
> > -					    &outputFormat);
> > +		ret = imgu->configureViewfinder(config->at(0), &outputFormat);
> >  		if (ret)
> >  			return ret;
> >  	}
> > @@ -546,7 +550,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  	StreamConfiguration statCfg = {};
> >  	statCfg.size = cio2Format.size;
> >  
> > -	ret = imgu->configureOutput(&imgu->stat_, statCfg, &outputFormat);
> > +	ret = imgu->configureStat(statCfg, &outputFormat);
> >  	if (ret)
> >  		return ret;
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart June 28, 2020, 11:58 a.m. UTC | #3 
Hi Niklas,

On Sun, Jun 28, 2020 at 01:56:45PM +0200, Niklas Söderlund wrote:
> On 2020-06-28 09:51:30 +0300, Laurent Pinchart wrote:
> > On Sun, Jun 28, 2020 at 02:15:27AM +0200, Niklas Söderlund wrote:
> > > When the IPU3 pipeline only provided streams to applications that came
> > > from the ImgU it made sense to have a generic function to configure all
> > > the different outputs. With the addition of the RAW stream this begins
> > > to be cumbersome to read and make sense of in the PipelineHandlerIPU3
> > > code. Replace the generic function that takes a specific argument for
> > > which sink to configure with a specific function for each sink.
> > > 
> > > This makes the code easier to follow as it's always clear which of the
> > > ImgU sinks are being configured without knowing the content of a
> > > generically named variable. It also paves way for future improvements.
> > 
> > s/paves way/paves the way/
> > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > * Changes since v1
> > > - s/sens/sense/
> > > - Retain comment for configureVideoDevice()
> > > - Inline the accessors for configureVideoDevice() in the .h file
> > > - Update comment in PipelineHandlerIPU3::configure()
> > > ---
> > >  src/libcamera/pipeline/ipu3/imgu.cpp | 20 +++++++--------
> > >  src/libcamera/pipeline/ipu3/imgu.h   | 28 ++++++++++++++++++--
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------
> > >  3 files changed, 57 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > > index 46d56a5d18dc3687..981239cba975f92e 100644
> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > > @@ -139,18 +139,17 @@ int ImgUDevice::configureInput(const Size &size,
> > >  }
> > >  
> > >  /**
> > > - * \brief Configure the ImgU unit \a id video output
> > > - * \param[in] output The ImgU output device to configure
> > > + * \brief Configure a video device on the ImgU
> > > + * \param[in] dev The video device to configure
> > > + * \param[in] pad The pad of the ImgU subdevice
> > >   * \param[in] cfg The requested configuration
> > > + * \param[out] outputFormat The format set on the video device
> > >   * \return 0 on success or a negative error code otherwise
> > >   */
> > > -int ImgUDevice::configureOutput(ImgUOutput *output,
> > > -				const StreamConfiguration &cfg,
> > > -				V4L2DeviceFormat *outputFormat)
> > > +int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> > > +				     const StreamConfiguration &cfg,
> > > +				     V4L2DeviceFormat *outputFormat)
> > >  {
> > > -	V4L2VideoDevice *dev = output->dev;
> > > -	unsigned int pad = output->pad;
> > > -
> > >  	V4L2SubdeviceFormat imguFormat = {};
> > >  	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
> > >  	imguFormat.size = cfg.size;
> > > @@ -160,7 +159,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> > >  		return ret;
> > >  
> > >  	/* No need to apply format to the stat node. */
> > > -	if (output == &stat_)
> > > +	if (dev == stat_.dev)
> > >  		return 0;
> > >  
> > >  	*outputFormat = {};
> > > @@ -172,7 +171,8 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	LOG(IPU3, Debug) << "ImgU " << output->name << " format = "
> > > +	const char *name = dev == output_.dev ? "output" : "viewfinder";
> > > +	LOG(IPU3, Debug) << "ImgU " << name << " format = "
> > >  			 << outputFormat->toString();
> > >  
> > >  	return 0;
> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> > > index b6b08b4fef2d3a9d..5f1dbfd8f45f7924 100644
> > > --- a/src/libcamera/pipeline/ipu3/imgu.h
> > > +++ b/src/libcamera/pipeline/ipu3/imgu.h
> > > @@ -49,9 +49,29 @@ public:
> > >  	}
> > >  
> > >  	int init(MediaDevice *media, unsigned int index);
> > > +
> > >  	int configureInput(const Size &size, V4L2DeviceFormat *inputFormat);
> > > -	int configureOutput(ImgUOutput *output, const StreamConfiguration &cfg,
> > > -			    V4L2DeviceFormat *outputFormat);
> > > +
> > > +	int configureOutput(const StreamConfiguration &cfg,
> > > +			    V4L2DeviceFormat *outputFormat)
> > > +	{
> > > +		return configureVideoDevice(output_.dev, PAD_OUTPUT, cfg,
> > > +					    outputFormat);
> > > +	}
> > > +
> > > +	int configureViewfinder(const StreamConfiguration &cfg,
> > > +				V4L2DeviceFormat *outputFormat)
> > > +	{
> > > +		return configureVideoDevice(viewfinder_.dev, PAD_VF, cfg,
> > > +					    outputFormat);
> > > +	}
> > > +
> > > +	int configureStat(const StreamConfiguration &cfg,
> > > +			  V4L2DeviceFormat *outputFormat)
> > > +	{
> > > +		return configureVideoDevice(stat_.dev, PAD_STAT, cfg,
> > > +					    outputFormat);
> > > +	}
> > >  
> > >  	int allocateBuffers(unsigned int bufferCount);
> > >  	void freeBuffers();
> > > @@ -78,6 +98,10 @@ private:
> > >  		      const std::string &sink, unsigned int sinkPad,
> > >  		      bool enable);
> > >  
> > > +	int configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> > > +				 const StreamConfiguration &cfg,
> > > +				 V4L2DeviceFormat *outputFormat);
> > > +
> > >  	std::string name_;
> > >  	MediaDevice *media_;
> > >  };
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index b41a789e8dc2a7b2..e817f842f1216a7f 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -504,19 +504,25 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > >  		stream->active_ = true;
> > >  		cfg.setStream(stream);
> > >  
> > > -		/*
> > > -		 * The RAW still capture stream just copies buffers from the
> > > -		 * internal queue and doesn't need any specific configuration.
> > > -		 */
> > > -		if (stream->raw_) {
> > > +		if (stream == outStream) {
> > > +			ret = imgu->configureOutput(cfg, &outputFormat);
> > > +			if (ret)
> > > +				return ret;
> > > +
> > > +			cfg.stride = outputFormat.planes[0].bpl;
> > > +		} else if (stream == vfStream) {
> > > +			ret = imgu->configureViewfinder(cfg, &outputFormat);
> > > +			if (ret)
> > > +				return ret;
> > > +
> > > +			cfg.stride = outputFormat.planes[0].bpl;
> > > +		} else {
> > > +			/*
> > > +			 * The RAW still capture stream only uses en externel
> > 
> > s/ en / an /
> > 
> > and an external what ?
> > 
> > > +			 * for the already configured CIO2 sink and doesn't need
> > 
> > Isn't the CIO2 a source ?
> > 
> > I know what you mean here as I know the code, but for someone who's not
> > familiar with the IPU3 pipeline handler that would be a bit confusing.
> 
> How about?
> 
> 	/*
> 	 * The RAW stream is configured as part of the CIO2 and
> 	 * no configuration is needed for the ImgU.
> 	 */

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

> > > +			 * any specific configuration of the ImgU.
> > > +			 */
> > >  			cfg.stride = cio2Format.planes[0].bpl;
> > > -		} else {
> > > -			ret = imgu->configureOutput(stream->device_, cfg,
> > > -						    &outputFormat);
> > > -			if (ret)
> > > -				return ret;
> > > -
> > > -			cfg.stride = outputFormat.planes[0].bpl;
> > >  		}
> > >  	}
> > >  
> > > @@ -526,15 +532,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > >  	 * be at least one active stream in the configuration request).
> > >  	 */
> > >  	if (!outStream->active_) {
> > > -		ret = imgu->configureOutput(outStream->device_, config->at(0),
> > > -					    &outputFormat);
> > > +		ret = imgu->configureOutput(config->at(0), &outputFormat);
> > >  		if (ret)
> > >  			return ret;
> > >  	}
> > >  
> > >  	if (!vfStream->active_) {
> > > -		ret = imgu->configureOutput(vfStream->device_, config->at(0),
> > > -					    &outputFormat);
> > > +		ret = imgu->configureViewfinder(config->at(0), &outputFormat);
> > >  		if (ret)
> > >  			return ret;
> > >  	}
> > > @@ -546,7 +550,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > >  	StreamConfiguration statCfg = {};
> > >  	statCfg.size = cio2Format.size;
> > >  
> > > -	ret = imgu->configureOutput(&imgu->stat_, statCfg, &outputFormat);
> > > +	ret = imgu->configureStat(statCfg, &outputFormat);
> > >  	if (ret)
> > >  		return ret;
> > >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
index 46d56a5d18dc3687..981239cba975f92e 100644
--- a/src/libcamera/pipeline/ipu3/imgu.cpp
+++ b/src/libcamera/pipeline/ipu3/imgu.cpp
@@ -139,18 +139,17 @@  int ImgUDevice::configureInput(const Size &size,
 }
 
 /**
- * \brief Configure the ImgU unit \a id video output
- * \param[in] output The ImgU output device to configure
+ * \brief Configure a video device on the ImgU
+ * \param[in] dev The video device to configure
+ * \param[in] pad The pad of the ImgU subdevice
  * \param[in] cfg The requested configuration
+ * \param[out] outputFormat The format set on the video device
  * \return 0 on success or a negative error code otherwise
  */
-int ImgUDevice::configureOutput(ImgUOutput *output,
-				const StreamConfiguration &cfg,
-				V4L2DeviceFormat *outputFormat)
+int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
+				     const StreamConfiguration &cfg,
+				     V4L2DeviceFormat *outputFormat)
 {
-	V4L2VideoDevice *dev = output->dev;
-	unsigned int pad = output->pad;
-
 	V4L2SubdeviceFormat imguFormat = {};
 	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
 	imguFormat.size = cfg.size;
@@ -160,7 +159,7 @@  int ImgUDevice::configureOutput(ImgUOutput *output,
 		return ret;
 
 	/* No need to apply format to the stat node. */
-	if (output == &stat_)
+	if (dev == stat_.dev)
 		return 0;
 
 	*outputFormat = {};
@@ -172,7 +171,8 @@  int ImgUDevice::configureOutput(ImgUOutput *output,
 	if (ret)
 		return ret;
 
-	LOG(IPU3, Debug) << "ImgU " << output->name << " format = "
+	const char *name = dev == output_.dev ? "output" : "viewfinder";
+	LOG(IPU3, Debug) << "ImgU " << name << " format = "
 			 << outputFormat->toString();
 
 	return 0;
diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
index b6b08b4fef2d3a9d..5f1dbfd8f45f7924 100644
--- a/src/libcamera/pipeline/ipu3/imgu.h
+++ b/src/libcamera/pipeline/ipu3/imgu.h
@@ -49,9 +49,29 @@  public:
 	}
 
 	int init(MediaDevice *media, unsigned int index);
+
 	int configureInput(const Size &size, V4L2DeviceFormat *inputFormat);
-	int configureOutput(ImgUOutput *output, const StreamConfiguration &cfg,
-			    V4L2DeviceFormat *outputFormat);
+
+	int configureOutput(const StreamConfiguration &cfg,
+			    V4L2DeviceFormat *outputFormat)
+	{
+		return configureVideoDevice(output_.dev, PAD_OUTPUT, cfg,
+					    outputFormat);
+	}
+
+	int configureViewfinder(const StreamConfiguration &cfg,
+				V4L2DeviceFormat *outputFormat)
+	{
+		return configureVideoDevice(viewfinder_.dev, PAD_VF, cfg,
+					    outputFormat);
+	}
+
+	int configureStat(const StreamConfiguration &cfg,
+			  V4L2DeviceFormat *outputFormat)
+	{
+		return configureVideoDevice(stat_.dev, PAD_STAT, cfg,
+					    outputFormat);
+	}
 
 	int allocateBuffers(unsigned int bufferCount);
 	void freeBuffers();
@@ -78,6 +98,10 @@  private:
 		      const std::string &sink, unsigned int sinkPad,
 		      bool enable);
 
+	int configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
+				 const StreamConfiguration &cfg,
+				 V4L2DeviceFormat *outputFormat);
+
 	std::string name_;
 	MediaDevice *media_;
 };
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index b41a789e8dc2a7b2..e817f842f1216a7f 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -504,19 +504,25 @@  int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
 		stream->active_ = true;
 		cfg.setStream(stream);
 
-		/*
-		 * The RAW still capture stream just copies buffers from the
-		 * internal queue and doesn't need any specific configuration.
-		 */
-		if (stream->raw_) {
+		if (stream == outStream) {
+			ret = imgu->configureOutput(cfg, &outputFormat);
+			if (ret)
+				return ret;
+
+			cfg.stride = outputFormat.planes[0].bpl;
+		} else if (stream == vfStream) {
+			ret = imgu->configureViewfinder(cfg, &outputFormat);
+			if (ret)
+				return ret;
+
+			cfg.stride = outputFormat.planes[0].bpl;
+		} else {
+			/*
+			 * The RAW still capture stream only uses en externel
+			 * for the already configured CIO2 sink and doesn't need
+			 * any specific configuration of the ImgU.
+			 */
 			cfg.stride = cio2Format.planes[0].bpl;
-		} else {
-			ret = imgu->configureOutput(stream->device_, cfg,
-						    &outputFormat);
-			if (ret)
-				return ret;
-
-			cfg.stride = outputFormat.planes[0].bpl;
 		}
 	}
 
@@ -526,15 +532,13 @@  int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
 	 * be at least one active stream in the configuration request).
 	 */
 	if (!outStream->active_) {
-		ret = imgu->configureOutput(outStream->device_, config->at(0),
-					    &outputFormat);
+		ret = imgu->configureOutput(config->at(0), &outputFormat);
 		if (ret)
 			return ret;
 	}
 
 	if (!vfStream->active_) {
-		ret = imgu->configureOutput(vfStream->device_, config->at(0),
-					    &outputFormat);
+		ret = imgu->configureViewfinder(config->at(0), &outputFormat);
 		if (ret)
 			return ret;
 	}
@@ -546,7 +550,7 @@  int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
 	StreamConfiguration statCfg = {};
 	statCfg.size = cio2Format.size;
 
-	ret = imgu->configureOutput(&imgu->stat_, statCfg, &outputFormat);
+	ret = imgu->configureStat(statCfg, &outputFormat);
 	if (ret)
 		return ret;