| Message ID | 20190326083902.26121-10-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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). > + 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 ? > return configs; > }
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@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
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); + 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" + << config->height << "-0x" << std::hex << std::setfill('0') + << std::setw(4) << config->pixelFormat; return configs; }
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. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 35 +++++++++++++--------------- 1 file changed, 16 insertions(+), 19 deletions(-)