[libcamera-devel] [PATCH v5 09/19] libcamera: ipu3: Set stream configuration from sensor

[Jacopo Mondi]

Hi Laurent,

On Tue, Apr 02, 2019 at 10:32:58AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Mar 26, 2019 at 09:38:52AM +0100, Jacopo Mondi wrote:
> > Inspect all image sizes provided by the sensor and select the
> > biggest of them, associated with an image format code supported by the
> > CIO2 unit.
>
> This isn't correct anymore, you don't inspect all sizes here, you use
> the cached maximum size.
>
> "While at it, replace the hardcoded numerical value for the number of
> buffers with a named constexpr."
>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 35 +++++++++++++---------------
> >  1 file changed, 16 insertions(+), 19 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index d42c81273cc6..04cd02653711 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -5,6 +5,7 @@
> >   * ipu3.cpp - Pipeline handler for Intel IPU3
> >   */
> >
> > +#include <iomanip>
> >  #include <memory>
> >  #include <vector>
> >
> > @@ -94,6 +95,8 @@ private:
> >  		std::pair<unsigned int, SizeRange> maxSizes_;
> >  	};
> >
> > +	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > +
> >  	IPU3CameraData *cameraData(const Camera *camera)
> >  	{
> >  		return static_cast<IPU3CameraData *>(
> > @@ -124,26 +127,20 @@ std::map<Stream *, StreamConfiguration>
> >  PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> >  					 std::set<Stream *> &streams)
> >  {
> > -	IPU3CameraData *data = cameraData(camera);
> >  	std::map<Stream *, StreamConfiguration> configs;
> > -	V4L2SubdeviceFormat format = {};
> > -
> > -	/*
> > -	 * FIXME: As of now, return the image format reported by the sensor.
> > -	 * In future good defaults should be provided for each stream.
> > -	 */
> > -	if (data->sensor_->getFormat(0, &format)) {
> > -		LOG(IPU3, Error) << "Failed to create stream configurations";
> > -		return configs;
> > -	}
> > -
> > -	StreamConfiguration config = {};
> > -	config.width = format.width;
> > -	config.height = format.height;
> > -	config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
> > -	config.bufferCount = 4;
> > -
> > -	configs[&data->stream_] = config;
> > +	IPU3CameraData *data = cameraData(camera);
>
> I'd keep this above the configs variable declaration to make the diff
> more readable (unless you think there's an advantage in this new order).
>

Yes, the declaration order follows reverse line lenght ordering. Diff
might be more confused a little, but what we care about is the end
result, isn't it?

> > +	StreamConfiguration *config = &configs[&data->stream_];
> > +	SizeRange &maxRange = data->maxSizes_.second;
> > +
> > +	config->width = maxRange.maxWidth;
> > +	config->height = maxRange.maxHeight;
> > +	config->pixelFormat = data->maxSizes_.first;
> > +	config->bufferCount = IPU3_BUFFER_COUNT;
> > +
> > +	LOG(IPU3, Debug)
> > +		<< "Stream format set to: " << config->width << "x"
>
> s/to:/to/
>
> > +		<< config->height << "-0x" << std::hex << std::setfill('0')
> > +		<< std::setw(4) << config->pixelFormat;
>
> Something to keep in mind for later, adding a Format class may be a good
> idea, to store pixel format, width and height. We should then give it a
> toString() method. Or do you think it would make sense to add a
> toString() method to StreamConfiguration and use it here ?

Time, and more users, will tell. We'll need a Format class for sure,
we could add pretty printing there.

Thanks
  j
>
> >  	return configs;
> >  }
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190402/1a7397fb/attachment.sig>