[libcamera-devel,v2,02/20] libcamera: ipu3: Remove streams from generateConfiguration
diff mbox series

Message ID 20200709084128.5316-3-jacopo@jmondi.org
State Accepted
Headers show
Series
  • libcamera: ipu3: Rework configuration
Related show

Commit Message

Jacopo Mondi July 9, 2020, 8:41 a.m. UTC 
Remove stream assignment from the IPU3 pipeline handler
generateConfiguration() implementation.

The function aims to provide a suitable default for the requested use
cases. Defer stream assignment to validation and only initialize sizes
and formats.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 104 ++++++++-------------------
 1 file changed, 30 insertions(+), 74 deletions(-)

Comments

Niklas Söderlund July 9, 2020, 12:59 p.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2020-07-09 10:41:10 +0200, Jacopo Mondi wrote:
> Remove stream assignment from the IPU3 pipeline handler
> generateConfiguration() implementation.
> 
> The function aims to provide a suitable default for the requested use
> cases. Defer stream assignment to validation and only initialize sizes
> and formats.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 104 ++++++++-------------------
>  1 file changed, 30 insertions(+), 74 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index af51fb2d1718..85d21b4db046 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -31,6 +31,14 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPU3)
>  
> +static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> +static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> +static constexpr unsigned int IPU3_OUTPUT_MAX_WIDTH = 4480;
> +static constexpr unsigned int IPU3_OUTPUT_MAX_HEIGHT = 34004;

Just checking is 34004 the max height?

> +static const Size minIPU3OutputSize = { 2, 2 };
> +static constexpr unsigned int IPU3_OUTPUT_WIDTH_ALIGN = 0x3f;
> +static constexpr unsigned int IPU3_OUTPUT_HEIGHT_ALIGN = 0x3;
> +
>  class IPU3CameraData : public CameraData
>  {
>  public:
> @@ -61,9 +69,6 @@ public:
>  	const std::vector<const Stream *> &streams() { return streams_; }
>  
>  private:
> -	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> -	static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> -
>  	void assignStreams();
>  	void adjustStream(StreamConfiguration &cfg, bool scale);
>  
> @@ -293,94 +298,51 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  {
>  	IPU3CameraData *data = cameraData(camera);
>  	IPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);
> -	std::set<Stream *> streams = {
> -		&data->outStream_,
> -		&data->vfStream_,
> -		&data->rawStream_,
> -	};
>  
>  	if (roles.empty())
>  		return config;
>  
> +	Size sensorResolution = data->cio2_.sensor()->resolution();
>  	for (const StreamRole role : roles) {
>  		StreamConfiguration cfg = {};
> -		Stream *stream = nullptr;
> -
> -		cfg.pixelFormat = formats::NV12;
>  
>  		switch (role) {
>  		case StreamRole::StillCapture:
>  			/*
> -			 * Pick the output stream by default as the Viewfinder
> -			 * and VideoRecording roles are not allowed on
> -			 * the output stream.
> -			 */
> -			if (streams.find(&data->outStream_) != streams.end()) {
> -				stream = &data->outStream_;
> -			} else if (streams.find(&data->vfStream_) != streams.end()) {
> -				stream = &data->vfStream_;
> -			} else {
> -				LOG(IPU3, Error)
> -					<< "No stream available for requested role "
> -					<< role;
> -				break;
> -			}
> -
> -			/*
> -			 * FIXME: Soraka: the maximum resolution reported by
> -			 * both sensors (2592x1944 for ov5670 and 4224x3136 for
> -			 * ov13858) are returned as default configurations but
> -			 * they're not correctly processed by the ImgU.
> -			 * Resolutions up tp 2560x1920 have been validated.
> -			 *
> -			 * \todo Clarify ImgU alignment requirements.
> +			 * Use the sensor resolution aligned to the ImgU
> +			 * output constraints.
>  			 */
> -			cfg.size = { 2560, 1920 };
> +			cfg.size.width = std::min(sensorResolution.width,
> +						  IPU3_OUTPUT_MAX_WIDTH);
> +			cfg.size.height = std::min(sensorResolution.height,
> +						   IPU3_OUTPUT_MAX_HEIGHT);
> +			cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> +			cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
> +			cfg.pixelFormat = formats::NV12;
> +			cfg.bufferCount = IPU3_BUFFER_COUNT;
>  
>  			break;
>  
>  		case StreamRole::StillCaptureRaw: {
> -			if (streams.find(&data->rawStream_) == streams.end()) {
> -				LOG(IPU3, Error)
> -					<< "Multiple raw streams are not supported";
> -				break;
> -			}
> -
> -			stream = &data->rawStream_;
> -
> -			cfg.size = data->cio2_.sensor()->resolution();
> +			cfg = data->cio2_.generateConfiguration(sensorResolution);
> +			cfg.bufferCount = 1;

Setting bufferCount here is goes against the idea of having 
CIO2Device::generateConfiguration() control all the parameters of the 
RAW stream. I know there was some discussion if returning a 
StreamConfiguration from the CIO2Device is a good idea or not. I still 
think it's the best design but I'm open to change/discuss it. 

But if we really want to modify parameters of it here I agree returning 
a StreamConfiguration here is confusing, double so as bufferCount is set 
to CIO2_BUFFER_COUNT and then changed to 1 here.

>  
> -			cfg = data->cio2_.generateConfiguration(cfg.size);
>  			break;
>  		}
>  
>  		case StreamRole::Viewfinder:
>  		case StreamRole::VideoRecording: {
>  			/*
> -			 * We can't use the 'output' stream for viewfinder or
> -			 * video capture roles.
> -			 *
> -			 * \todo This is an artificial limitation until we
> -			 * figure out the exact capabilities of the hardware.
> +			 * Default viewfinder to 1280x720, capped to the maximum
> +			 * sensor resolution and aligned to the ImgU output
> +			 * constraints.

This is done for both Viewfinder and VideoRecording right?

>  			 */
> -			if (streams.find(&data->vfStream_) == streams.end()) {
> -				LOG(IPU3, Error)
> -					<< "No stream available for requested role "
> -					<< role;
> -				break;
> -			}
> -
> -			stream = &data->vfStream_;
> -
> -			/*
> -			 * Align the default viewfinder size to the maximum
> -			 * available sensor resolution and to the IPU3
> -			 * alignment constraints.
> -			 */
> -			const Size &res = data->cio2_.sensor()->resolution();
> -			unsigned int width = std::min(1280U, res.width);
> -			unsigned int height = std::min(720U, res.height);
> -			cfg.size = { width & ~7, height & ~3 };
> +			cfg.size.width = std::min(1280U, sensorResolution.width);
> +			cfg.size.height = std::min(720U, sensorResolution.height);
> +			cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> +			cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
> +			cfg.pixelFormat = formats::NV12;
> +			cfg.bufferCount = IPU3_BUFFER_COUNT;
>  
>  			break;
>  		}
> @@ -388,16 +350,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  		default:
>  			LOG(IPU3, Error)
>  				<< "Requested stream role not supported: " << role;
> -			break;

I would keep the break as it is the style elsewhere in camera is to 
terminate the default case with either break, return or continue.

> -		}
> -
> -		if (!stream) {
>  			delete config;
>  			return nullptr;
>  		}
>  
> -		streams.erase(stream);
> -
>  		config->addConfiguration(cfg);
>  	}
>  
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi July 10, 2020, 6:58 a.m. UTC | #2 
Hi Niklas,
   thanks for review

On Thu, Jul 09, 2020 at 02:59:42PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-07-09 10:41:10 +0200, Jacopo Mondi wrote:
> > Remove stream assignment from the IPU3 pipeline handler
> > generateConfiguration() implementation.
> >
> > The function aims to provide a suitable default for the requested use
> > cases. Defer stream assignment to validation and only initialize sizes
> > and formats.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 104 ++++++++-------------------
> >  1 file changed, 30 insertions(+), 74 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index af51fb2d1718..85d21b4db046 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -31,6 +31,14 @@ namespace libcamera {
> >
> >  LOG_DEFINE_CATEGORY(IPU3)
> >
> > +static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > +static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> > +static constexpr unsigned int IPU3_OUTPUT_MAX_WIDTH = 4480;
> > +static constexpr unsigned int IPU3_OUTPUT_MAX_HEIGHT = 34004;
>
> Just checking is 34004 the max height?
>

It seems so
drivers/staging/media/ipu3/ipu3.h-#define IPU3_OUTPUT_MAX_HEIGHT   34004U

> > +static const Size minIPU3OutputSize = { 2, 2 };
> > +static constexpr unsigned int IPU3_OUTPUT_WIDTH_ALIGN = 0x3f;
> > +static constexpr unsigned int IPU3_OUTPUT_HEIGHT_ALIGN = 0x3;
> > +
> >  class IPU3CameraData : public CameraData
> >  {
> >  public:
> > @@ -61,9 +69,6 @@ public:
> >  	const std::vector<const Stream *> &streams() { return streams_; }
> >
> >  private:
> > -	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > -	static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> > -
> >  	void assignStreams();
> >  	void adjustStream(StreamConfiguration &cfg, bool scale);
> >
> > @@ -293,94 +298,51 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> >  	IPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);
> > -	std::set<Stream *> streams = {
> > -		&data->outStream_,
> > -		&data->vfStream_,
> > -		&data->rawStream_,
> > -	};
> >
> >  	if (roles.empty())
> >  		return config;
> >
> > +	Size sensorResolution = data->cio2_.sensor()->resolution();
> >  	for (const StreamRole role : roles) {
> >  		StreamConfiguration cfg = {};
> > -		Stream *stream = nullptr;
> > -
> > -		cfg.pixelFormat = formats::NV12;
> >
> >  		switch (role) {
> >  		case StreamRole::StillCapture:
> >  			/*
> > -			 * Pick the output stream by default as the Viewfinder
> > -			 * and VideoRecording roles are not allowed on
> > -			 * the output stream.
> > -			 */
> > -			if (streams.find(&data->outStream_) != streams.end()) {
> > -				stream = &data->outStream_;
> > -			} else if (streams.find(&data->vfStream_) != streams.end()) {
> > -				stream = &data->vfStream_;
> > -			} else {
> > -				LOG(IPU3, Error)
> > -					<< "No stream available for requested role "
> > -					<< role;
> > -				break;
> > -			}
> > -
> > -			/*
> > -			 * FIXME: Soraka: the maximum resolution reported by
> > -			 * both sensors (2592x1944 for ov5670 and 4224x3136 for
> > -			 * ov13858) are returned as default configurations but
> > -			 * they're not correctly processed by the ImgU.
> > -			 * Resolutions up tp 2560x1920 have been validated.
> > -			 *
> > -			 * \todo Clarify ImgU alignment requirements.
> > +			 * Use the sensor resolution aligned to the ImgU
> > +			 * output constraints.
> >  			 */
> > -			cfg.size = { 2560, 1920 };
> > +			cfg.size.width = std::min(sensorResolution.width,
> > +						  IPU3_OUTPUT_MAX_WIDTH);
> > +			cfg.size.height = std::min(sensorResolution.height,
> > +						   IPU3_OUTPUT_MAX_HEIGHT);
> > +			cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> > +			cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
> > +			cfg.pixelFormat = formats::NV12;
> > +			cfg.bufferCount = IPU3_BUFFER_COUNT;
> >
> >  			break;
> >
> >  		case StreamRole::StillCaptureRaw: {
> > -			if (streams.find(&data->rawStream_) == streams.end()) {
> > -				LOG(IPU3, Error)
> > -					<< "Multiple raw streams are not supported";
> > -				break;
> > -			}
> > -
> > -			stream = &data->rawStream_;
> > -
> > -			cfg.size = data->cio2_.sensor()->resolution();
> > +			cfg = data->cio2_.generateConfiguration(sensorResolution);
> > +			cfg.bufferCount = 1;
>
> Setting bufferCount here is goes against the idea of having
> CIO2Device::generateConfiguration() control all the parameters of the
> RAW stream. I know there was some discussion if returning a
> StreamConfiguration from the CIO2Device is a good idea or not. I still
> think it's the best design but I'm open to change/discuss it.

Ah, this was not intentional in first place to be honest.

I would be happy to discuss (later) what is the best to return from
the CIO2 configuration, as I would have preferred not returning a
format, but that's oke for now.

>
> But if we really want to modify parameters of it here I agree returning
> a StreamConfiguration here is confusing, double so as bufferCount is set
> to CIO2_BUFFER_COUNT and then changed to 1 here.
>

What confused me is the pattern

		cfg.size = data->cio2_.sensor()->resolution();
		cfg = data->cio2_.generateConfiguration(cfg.size);

As using cfg.size just a place holder to pass it to the
generateConfiguration() function is not nice imho. But removing the
assignement of the whole configuration was not intentional, so I'll
fix it.


> >
> > -			cfg = data->cio2_.generateConfiguration(cfg.size);
> >  			break;
> >  		}
> >
> >  		case StreamRole::Viewfinder:
> >  		case StreamRole::VideoRecording: {
> >  			/*
> > -			 * We can't use the 'output' stream for viewfinder or
> > -			 * video capture roles.
> > -			 *
> > -			 * \todo This is an artificial limitation until we
> > -			 * figure out the exact capabilities of the hardware.
> > +			 * Default viewfinder to 1280x720, capped to the maximum
> > +			 * sensor resolution and aligned to the ImgU output
> > +			 * constraints.
>
> This is done for both Viewfinder and VideoRecording right?
>

Yes, I'll update the comment

> >  			 */
> > -			if (streams.find(&data->vfStream_) == streams.end()) {
> > -				LOG(IPU3, Error)
> > -					<< "No stream available for requested role "
> > -					<< role;
> > -				break;
> > -			}
> > -
> > -			stream = &data->vfStream_;
> > -
> > -			/*
> > -			 * Align the default viewfinder size to the maximum
> > -			 * available sensor resolution and to the IPU3
> > -			 * alignment constraints.
> > -			 */
> > -			const Size &res = data->cio2_.sensor()->resolution();
> > -			unsigned int width = std::min(1280U, res.width);
> > -			unsigned int height = std::min(720U, res.height);
> > -			cfg.size = { width & ~7, height & ~3 };
> > +			cfg.size.width = std::min(1280U, sensorResolution.width);
> > +			cfg.size.height = std::min(720U, sensorResolution.height);
> > +			cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> > +			cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
> > +			cfg.pixelFormat = formats::NV12;
> > +			cfg.bufferCount = IPU3_BUFFER_COUNT;
> >
> >  			break;
> >  		}
> > @@ -388,16 +350,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  		default:
> >  			LOG(IPU3, Error)
> >  				<< "Requested stream role not supported: " << role;
> > -			break;
>
> I would keep the break as it is the style elsewhere in camera is to
> terminate the default case with either break, return or continue.
>

The code now looks like this

		default:
			LOG(IPU3, Error)
				<< "Requested stream role not supported: " << role;
			delete config;
			return nullptr;

I think it's right

Thanks
   j

> > -		}
> > -
> > -		if (!stream) {
> >  			delete config;
> >  			return nullptr;
> >  		}
> >
> > -		streams.erase(stream);
> > -
> >  		config->addConfiguration(cfg);
> >  	}
> >
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
Laurent Pinchart July 10, 2020, 8:27 a.m. UTC | #3 
Hi Jacopo,

Thank you for the patch.

Isn't the usual practice to use "libcamera: pipeline:" as a prefix for
pipeline handler patches in the subject line ?

On Fri, Jul 10, 2020 at 08:58:54AM +0200, Jacopo Mondi wrote:
> On Thu, Jul 09, 2020 at 02:59:42PM +0200, Niklas Söderlund wrote:
> > On 2020-07-09 10:41:10 +0200, Jacopo Mondi wrote:
> > > Remove stream assignment from the IPU3 pipeline handler
> > > generateConfiguration() implementation.
> > >
> > > The function aims to provide a suitable default for the requested use
> > > cases. Defer stream assignment to validation and only initialize sizes
> > > and formats.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 104 ++++++++-------------------
> > >  1 file changed, 30 insertions(+), 74 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index af51fb2d1718..85d21b4db046 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -31,6 +31,14 @@ namespace libcamera {
> > >
> > >  LOG_DEFINE_CATEGORY(IPU3)
> > >
> > > +static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > > +static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> > > +static constexpr unsigned int IPU3_OUTPUT_MAX_WIDTH = 4480;
> > > +static constexpr unsigned int IPU3_OUTPUT_MAX_HEIGHT = 34004;
> >
> > Just checking is 34004 the max height?
> 
> It seems so
> drivers/staging/media/ipu3/ipu3.h-#define IPU3_OUTPUT_MAX_HEIGHT   34004U

A weird value, but why not.

> > > +static const Size minIPU3OutputSize = { 2, 2 };
> > > +static constexpr unsigned int IPU3_OUTPUT_WIDTH_ALIGN = 0x3f;
> > > +static constexpr unsigned int IPU3_OUTPUT_HEIGHT_ALIGN = 0x3;

I'd set these to 64 and 4, as the alignment constraints are multiple of
4 and multiple of 4, and subtract 1 in the code.

I would also rename the constants to use an IMGU prefix if they're
specific to the ImgU (those two seem to be, I don't know about the
previous ones).

> > > +
> > >  class IPU3CameraData : public CameraData
> > >  {
> > >  public:
> > > @@ -61,9 +69,6 @@ public:
> > >  	const std::vector<const Stream *> &streams() { return streams_; }
> > >
> > >  private:
> > > -	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > > -	static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> > > -
> > >  	void assignStreams();
> > >  	void adjustStream(StreamConfiguration &cfg, bool scale);
> > >
> > > @@ -293,94 +298,51 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > >  {
> > >  	IPU3CameraData *data = cameraData(camera);
> > >  	IPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);
> > > -	std::set<Stream *> streams = {
> > > -		&data->outStream_,
> > > -		&data->vfStream_,
> > > -		&data->rawStream_,
> > > -	};
> > >
> > >  	if (roles.empty())
> > >  		return config;
> > >
> > > +	Size sensorResolution = data->cio2_.sensor()->resolution();
> > >  	for (const StreamRole role : roles) {
> > >  		StreamConfiguration cfg = {};
> > > -		Stream *stream = nullptr;
> > > -
> > > -		cfg.pixelFormat = formats::NV12;
> > >
> > >  		switch (role) {
> > >  		case StreamRole::StillCapture:
> > >  			/*
> > > -			 * Pick the output stream by default as the Viewfinder
> > > -			 * and VideoRecording roles are not allowed on
> > > -			 * the output stream.
> > > -			 */
> > > -			if (streams.find(&data->outStream_) != streams.end()) {
> > > -				stream = &data->outStream_;
> > > -			} else if (streams.find(&data->vfStream_) != streams.end()) {
> > > -				stream = &data->vfStream_;
> > > -			} else {
> > > -				LOG(IPU3, Error)
> > > -					<< "No stream available for requested role "
> > > -					<< role;
> > > -				break;
> > > -			}
> > > -
> > > -			/*
> > > -			 * FIXME: Soraka: the maximum resolution reported by
> > > -			 * both sensors (2592x1944 for ov5670 and 4224x3136 for
> > > -			 * ov13858) are returned as default configurations but
> > > -			 * they're not correctly processed by the ImgU.
> > > -			 * Resolutions up tp 2560x1920 have been validated.
> > > -			 *
> > > -			 * \todo Clarify ImgU alignment requirements.
> > > +			 * Use the sensor resolution aligned to the ImgU
> > > +			 * output constraints.
> > >  			 */
> > > -			cfg.size = { 2560, 1920 };
> > > +			cfg.size.width = std::min(sensorResolution.width,
> > > +						  IPU3_OUTPUT_MAX_WIDTH);
> > > +			cfg.size.height = std::min(sensorResolution.height,
> > > +						   IPU3_OUTPUT_MAX_HEIGHT);
> > > +			cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> > > +			cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;

Would "[PATCH] libcamera: geometry: Add helper functions to the Size
class" help ?

			cfg.size = sensorResolution.boundedTo({IPU3_OUTPUT_MAX_WIDTH,
							       IPU3_OUTPUT_MAX_HEIGHT})
						   .alignedTo(IPU3_OUTPUT_WIDTH_ALIGN,
							      IPU3_OUTPUT_HEIGHT_ALIGN);

IPU3_OUTPUT_MAX_WIDTH and IPU3_OUTPUT_MAX_HEIGHT could become
IPU3_OUTPUT_MAX_SIZE to simplify it further.

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

> > > +			cfg.pixelFormat = formats::NV12;
> > > +			cfg.bufferCount = IPU3_BUFFER_COUNT;
> > >
> > >  			break;
> > >
> > >  		case StreamRole::StillCaptureRaw: {
> > > -			if (streams.find(&data->rawStream_) == streams.end()) {
> > > -				LOG(IPU3, Error)
> > > -					<< "Multiple raw streams are not supported";
> > > -				break;
> > > -			}
> > > -
> > > -			stream = &data->rawStream_;
> > > -
> > > -			cfg.size = data->cio2_.sensor()->resolution();
> > > +			cfg = data->cio2_.generateConfiguration(sensorResolution);
> > > +			cfg.bufferCount = 1;
> >
> > Setting bufferCount here is goes against the idea of having
> > CIO2Device::generateConfiguration() control all the parameters of the
> > RAW stream. I know there was some discussion if returning a
> > StreamConfiguration from the CIO2Device is a good idea or not. I still
> > think it's the best design but I'm open to change/discuss it.
> 
> Ah, this was not intentional in first place to be honest.
> 
> I would be happy to discuss (later) what is the best to return from
> the CIO2 configuration, as I would have preferred not returning a
> format, but that's oke for now.
> 
> > But if we really want to modify parameters of it here I agree returning
> > a StreamConfiguration here is confusing, double so as bufferCount is set
> > to CIO2_BUFFER_COUNT and then changed to 1 here.
> 
> What confused me is the pattern
> 
> 		cfg.size = data->cio2_.sensor()->resolution();
> 		cfg = data->cio2_.generateConfiguration(cfg.size);
> 
> As using cfg.size just a place holder to pass it to the
> generateConfiguration() function is not nice imho. But removing the
> assignement of the whole configuration was not intentional, so I'll
> fix it.
> 
> > >
> > > -			cfg = data->cio2_.generateConfiguration(cfg.size);
> > >  			break;
> > >  		}
> > >
> > >  		case StreamRole::Viewfinder:
> > >  		case StreamRole::VideoRecording: {
> > >  			/*
> > > -			 * We can't use the 'output' stream for viewfinder or
> > > -			 * video capture roles.
> > > -			 *
> > > -			 * \todo This is an artificial limitation until we
> > > -			 * figure out the exact capabilities of the hardware.
> > > +			 * Default viewfinder to 1280x720, capped to the maximum
> > > +			 * sensor resolution and aligned to the ImgU output
> > > +			 * constraints.
> >
> > This is done for both Viewfinder and VideoRecording right?
> 
> Yes, I'll update the comment
> 
> > >  			 */
> > > -			if (streams.find(&data->vfStream_) == streams.end()) {
> > > -				LOG(IPU3, Error)
> > > -					<< "No stream available for requested role "
> > > -					<< role;
> > > -				break;
> > > -			}
> > > -
> > > -			stream = &data->vfStream_;
> > > -
> > > -			/*
> > > -			 * Align the default viewfinder size to the maximum
> > > -			 * available sensor resolution and to the IPU3
> > > -			 * alignment constraints.
> > > -			 */
> > > -			const Size &res = data->cio2_.sensor()->resolution();
> > > -			unsigned int width = std::min(1280U, res.width);
> > > -			unsigned int height = std::min(720U, res.height);
> > > -			cfg.size = { width & ~7, height & ~3 };
> > > +			cfg.size.width = std::min(1280U, sensorResolution.width);
> > > +			cfg.size.height = std::min(720U, sensorResolution.height);
> > > +			cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> > > +			cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
> > > +			cfg.pixelFormat = formats::NV12;
> > > +			cfg.bufferCount = IPU3_BUFFER_COUNT;
> > >
> > >  			break;
> > >  		}
> > > @@ -388,16 +350,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > >  		default:
> > >  			LOG(IPU3, Error)
> > >  				<< "Requested stream role not supported: " << role;
> > > -			break;
> >
> > I would keep the break as it is the style elsewhere in camera is to
> > terminate the default case with either break, return or continue.
> 
> The code now looks like this
> 
> 		default:
> 			LOG(IPU3, Error)
> 				<< "Requested stream role not supported: " << role;
> 			delete config;
> 			return nullptr;
> 
> I think it's right
> 
> > > -		}
> > > -
> > > -		if (!stream) {
> > >  			delete config;
> > >  			return nullptr;
> > >  		}
> > >
> > > -		streams.erase(stream);
> > > -
> > >  		config->addConfiguration(cfg);
> > >  	}
> > >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index af51fb2d1718..85d21b4db046 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -31,6 +31,14 @@  namespace libcamera {
 
 LOG_DEFINE_CATEGORY(IPU3)
 
+static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
+static constexpr unsigned int IPU3_MAX_STREAMS = 3;
+static constexpr unsigned int IPU3_OUTPUT_MAX_WIDTH = 4480;
+static constexpr unsigned int IPU3_OUTPUT_MAX_HEIGHT = 34004;
+static const Size minIPU3OutputSize = { 2, 2 };
+static constexpr unsigned int IPU3_OUTPUT_WIDTH_ALIGN = 0x3f;
+static constexpr unsigned int IPU3_OUTPUT_HEIGHT_ALIGN = 0x3;
+
 class IPU3CameraData : public CameraData
 {
 public:
@@ -61,9 +69,6 @@  public:
 	const std::vector<const Stream *> &streams() { return streams_; }
 
 private:
-	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
-	static constexpr unsigned int IPU3_MAX_STREAMS = 3;
-
 	void assignStreams();
 	void adjustStream(StreamConfiguration &cfg, bool scale);
 
@@ -293,94 +298,51 @@  CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
 {
 	IPU3CameraData *data = cameraData(camera);
 	IPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);
-	std::set<Stream *> streams = {
-		&data->outStream_,
-		&data->vfStream_,
-		&data->rawStream_,
-	};
 
 	if (roles.empty())
 		return config;
 
+	Size sensorResolution = data->cio2_.sensor()->resolution();
 	for (const StreamRole role : roles) {
 		StreamConfiguration cfg = {};
-		Stream *stream = nullptr;
-
-		cfg.pixelFormat = formats::NV12;
 
 		switch (role) {
 		case StreamRole::StillCapture:
 			/*
-			 * Pick the output stream by default as the Viewfinder
-			 * and VideoRecording roles are not allowed on
-			 * the output stream.
-			 */
-			if (streams.find(&data->outStream_) != streams.end()) {
-				stream = &data->outStream_;
-			} else if (streams.find(&data->vfStream_) != streams.end()) {
-				stream = &data->vfStream_;
-			} else {
-				LOG(IPU3, Error)
-					<< "No stream available for requested role "
-					<< role;
-				break;
-			}
-
-			/*
-			 * FIXME: Soraka: the maximum resolution reported by
-			 * both sensors (2592x1944 for ov5670 and 4224x3136 for
-			 * ov13858) are returned as default configurations but
-			 * they're not correctly processed by the ImgU.
-			 * Resolutions up tp 2560x1920 have been validated.
-			 *
-			 * \todo Clarify ImgU alignment requirements.
+			 * Use the sensor resolution aligned to the ImgU
+			 * output constraints.
 			 */
-			cfg.size = { 2560, 1920 };
+			cfg.size.width = std::min(sensorResolution.width,
+						  IPU3_OUTPUT_MAX_WIDTH);
+			cfg.size.height = std::min(sensorResolution.height,
+						   IPU3_OUTPUT_MAX_HEIGHT);
+			cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
+			cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
+			cfg.pixelFormat = formats::NV12;
+			cfg.bufferCount = IPU3_BUFFER_COUNT;
 
 			break;
 
 		case StreamRole::StillCaptureRaw: {
-			if (streams.find(&data->rawStream_) == streams.end()) {
-				LOG(IPU3, Error)
-					<< "Multiple raw streams are not supported";
-				break;
-			}
-
-			stream = &data->rawStream_;
-
-			cfg.size = data->cio2_.sensor()->resolution();
+			cfg = data->cio2_.generateConfiguration(sensorResolution);
+			cfg.bufferCount = 1;
 
-			cfg = data->cio2_.generateConfiguration(cfg.size);
 			break;
 		}
 
 		case StreamRole::Viewfinder:
 		case StreamRole::VideoRecording: {
 			/*
-			 * We can't use the 'output' stream for viewfinder or
-			 * video capture roles.
-			 *
-			 * \todo This is an artificial limitation until we
-			 * figure out the exact capabilities of the hardware.
+			 * Default viewfinder to 1280x720, capped to the maximum
+			 * sensor resolution and aligned to the ImgU output
+			 * constraints.
 			 */
-			if (streams.find(&data->vfStream_) == streams.end()) {
-				LOG(IPU3, Error)
-					<< "No stream available for requested role "
-					<< role;
-				break;
-			}
-
-			stream = &data->vfStream_;
-
-			/*
-			 * Align the default viewfinder size to the maximum
-			 * available sensor resolution and to the IPU3
-			 * alignment constraints.
-			 */
-			const Size &res = data->cio2_.sensor()->resolution();
-			unsigned int width = std::min(1280U, res.width);
-			unsigned int height = std::min(720U, res.height);
-			cfg.size = { width & ~7, height & ~3 };
+			cfg.size.width = std::min(1280U, sensorResolution.width);
+			cfg.size.height = std::min(720U, sensorResolution.height);
+			cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
+			cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
+			cfg.pixelFormat = formats::NV12;
+			cfg.bufferCount = IPU3_BUFFER_COUNT;
 
 			break;
 		}
@@ -388,16 +350,10 @@  CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
 		default:
 			LOG(IPU3, Error)
 				<< "Requested stream role not supported: " << role;
-			break;
-		}
-
-		if (!stream) {
 			delete config;
 			return nullptr;
 		}
 
-		streams.erase(stream);
-
 		config->addConfiguration(cfg);
 	}