[libcamera-devel] [PATCH 02/15] libcamera: ipu3: Remove streams from generateConfiguration

[Niklas Söderlund]

Hi Jacopo,

On 2020-07-02 09:33:36 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Wed, Jul 01, 2020 at 06:23:06PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2020-07-01 14:30:23 +0200, Jacopo Mondi wrote:
> > > Remove stream assignement from the IPU3 pipeline handler
> > > generateConfiguration() implementation.
> > >
> > > The function aims to provide a suitable default for the requested use
> > > cases. Defer stream assignement to validation and only assign sizes
> > > and formats to stream configurations.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++--------------------
> > >  1 file changed, 27 insertions(+), 66 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 1bdad209de6e..97fc8b60c3cb 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -31,6 +31,9 @@ namespace libcamera {
> > >
> > >  LOG_DEFINE_CATEGORY(IPU3)
> > >
> > > +static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > > +static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> > > +
> > >  class IPU3CameraData : public CameraData
> > >  {
> > >  public:
> > > @@ -61,9 +64,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 +293,55 @@ 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();
> > > +	unsigned int rawCount = 0;
> > > +	unsigned int outCount = 0;
> >
> > Does it make sens to add check of number of streams here? In 7/15 you
> > add the same to validate() and validate is called at the and of
> > generateConfiguration().
> 
> It's a bit of a repetetion, I agree.
> 
> Should we check for the status returned by validate() then ?

I think that would be nicer as we then collect all checks on the 
configuration in validate() while at the same time guarantee the 
configuration returned from generateConfiguration() is OK.

> 
> >
> > Other then this open question I really like this patch!
> >
> > >  	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.
> > > +			 * Use the sensor resolution adjusted to respect the
> > > +			 * ImgU output alignement contraints.
> > >  			 */
> > > -			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;
> > > -			}
> > > +			cfg.pixelFormat = formats::NV12;
> > > +			cfg.size = sensorResolution;
> > > +			cfg.size.width &= ~7;
> > > +			cfg.size.height &= ~3;
> > > +			cfg.bufferCount = IPU3_BUFFER_COUNT;
> > >
> > > -			/*
> > > -			 * 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.
> > > -			 */
> > > -			cfg.size = { 2560, 1920 };
> > > +			outCount++;
> > >
> > >  			break;
> > >
> > >  		case StreamRole::StillCaptureRaw: {
> > > -			if (streams.find(&data->rawStream_) == streams.end()) {
> > > -				LOG(IPU3, Error)
> > > -					<< "Multiple raw streams are not supported";
> > > -				break;
> > > -			}
> > > +			cfg = data->cio2_.generateConfiguration(sensorResolution);
> > > +			cfg.bufferCount = 1;
> > >
> > > -			stream = &data->rawStream_;
> > > +			rawCount++;
> > >
> > > -			cfg.size = data->cio2_.sensor()->resolution();
> > > -
> > > -			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.
> > > -			 */
> > > -			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);
> > > +			unsigned int width = std::min(1280U, sensorResolution.width);
> > > +			unsigned int height = std::min(720U, sensorResolution.height);
> > >  			cfg.size = { width & ~7, height & ~3 };
> > > +			cfg.pixelFormat = formats::NV12;
> > > +			cfg.bufferCount = IPU3_BUFFER_COUNT;
> > > +
> > > +			outCount++;
> > >
> > >  			break;
> > >  		}
> > > @@ -388,16 +349,16 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > >  		default:
> > >  			LOG(IPU3, Error)
> > >  				<< "Requested stream role not supported: " << role;
> > > -			break;
> > > +			delete config;
> > > +			return nullptr;
> > >  		}
> > >
> > > -		if (!stream) {
> > > +		if (rawCount > 1 || outCount > 2) {
> > > +			LOG(IPU3, Error) << "Invalid stream roles requested";
> > >  			delete config;
> > >  			return nullptr;
> > >  		}
> > >
> > > -		streams.erase(stream);
> > > -
> > >  		config->addConfiguration(cfg);
> > >  	}
> > >
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund

-- 
Regards,
Niklas Söderlund