| Message ID | 20200709084128.5316-13-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2020-07-09 10:41:20 +0200, Jacopo Mondi wrote: > The requirement of having the ImgU output height 32 pixels smaller > than the input frame produced by the CIO2 makes it complicated to > re-adjust the sensor produced size after the alignement has been > applied. > > To simplify the procedure, always ask for the full frame size from the > CIO2 unit. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index d07f1a7b5ae8..feabffe641e1 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -156,8 +156,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > unsigned int rawCount = 0; > unsigned int outCount = 0; > Size yuvSize; > - Size size; > - > for (const StreamConfiguration &cfg : config_) { > const PixelFormatInfo &info = > PixelFormatInfo::info(cfg.pixelFormat); > @@ -174,11 +172,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > if (cfg.size > yuvSize) > yuvSize = cfg.size; > } > - > - if (cfg.size.width > size.width) > - size.width = cfg.size.width; > - if (cfg.size.height > size.height) > - size.height = cfg.size.height; > } > if (rawCount > 1 || outCount > 2) { > LOG(IPU3, Error) > @@ -189,10 +182,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > } > > /* Generate raw configuration from CIO2. */ > - cio2Configuration_ = data_->cio2_.generateConfiguration(size); > + Size sensorSize = data_->cio2_.sensor()->resolution(); > + cio2Configuration_ = data_->cio2_.generateConfiguration(sensorSize); Could this not impact framerate for the CIO2 if we always request the largest sensor size? Would it be possible instead to increase the size passed to CIO2Device::generateConfiguration() by 32? > if (!cio2Configuration_.pixelFormat.isValid()) > return Invalid; > > + LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString(); > + > /* > * Adjust the configurations if needed and assign streams while > * iterating them. > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Thu, Jul 09, 2020 at 03:46:54PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2020-07-09 10:41:20 +0200, Jacopo Mondi wrote: > > The requirement of having the ImgU output height 32 pixels smaller > > than the input frame produced by the CIO2 makes it complicated to > > re-adjust the sensor produced size after the alignement has been > > applied. > > > > To simplify the procedure, always ask for the full frame size from the > > CIO2 unit. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index d07f1a7b5ae8..feabffe641e1 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -156,8 +156,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > unsigned int rawCount = 0; > > unsigned int outCount = 0; > > Size yuvSize; > > - Size size; > > - > > for (const StreamConfiguration &cfg : config_) { > > const PixelFormatInfo &info = > > PixelFormatInfo::info(cfg.pixelFormat); > > @@ -174,11 +172,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > if (cfg.size > yuvSize) > > yuvSize = cfg.size; > > } > > - > > - if (cfg.size.width > size.width) > > - size.width = cfg.size.width; > > - if (cfg.size.height > size.height) > > - size.height = cfg.size.height; > > } > > if (rawCount > 1 || outCount > 2) { > > LOG(IPU3, Error) > > @@ -189,10 +182,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > } > > > > /* Generate raw configuration from CIO2. */ > > - cio2Configuration_ = data_->cio2_.generateConfiguration(size); > > + Size sensorSize = data_->cio2_.sensor()->resolution(); > > + cio2Configuration_ = data_->cio2_.generateConfiguration(sensorSize); > > Could this not impact framerate for the CIO2 if we always request the > largest sensor size? Would it be possible instead to increase the size > passed to CIO2Device::generateConfiguration() by 32? > Yes it does, and this is probably just wrong, as it ignores the requested raw capture size. Fun fact which actually lead me to add this change: if I keep the previous version where the sensor is asked for the smaller as possible resolution that supports all stream (+32 as you here suggest) the IPU3 pipeline parameter calculations fail on Soraka for some output resolutions. In example: calculating the configuration for a single stream in 1280x720 with a frame resolution of 2112x1188 (which is the smaller ov13858 resolution that is large enough) fails: the tool is not able to find any suitable configuration that satisfied the pipeline constraints: $ python pipe_config.py input=2112x1188 main=1280x720 IndexError: list index out of range If I use the full frame size, everything is unicorns&rainbows again $ python pipe_config.py input=4224x3136 main=1280x720 -------- The selected pipe configuration: -------------- output_res_if:4220x2600 output_res_bds:1688x1040 output_res_gdc:1280x720 FOV Horizontal:0.757576, FOV vertical:0.573980 Why ? I have no idea, and I think we should stop waiting for Intel to actually care about this platform anymore, so I don't expect they could help us debugging the tool. The re-implementation I made performs the same calculations as the script, hence reducing the frame size makes a lot of configurations fail. If you look at https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov13858.xml The sensor is always asked for the full frame, so I suspect that's how the system is intenteded to work (even if, I wonder, what happens if I connect a sensor that can only produce the above failing 2112x1188 resolution ?) I -fear- for IPU3 we should always go full frame size to make the pipe config happy, or there's maybe a requirement to respect some aspect ratio I still haven't been able to figure out. In any case, this would hinder the ability to capture raw+YUV, as the raw frame resolution in that case is selected from the application, and it might prevent the pipeline from being able to calculate a configuration. Not nice, I know. If that's how thing have to work, a software scaler for raw frames would nee to be plugged to the pipeline to down-scale raw frames to the desired resolution. Or we restrict RAW streams to the full size only. What do you think ? > > if (!cio2Configuration_.pixelFormat.isValid()) > > return Invalid; > > > > + LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString(); > > + > > /* > > * Adjust the configurations if needed and assign streams while > > * iterating them. > > -- > > 2.27.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi Jacopo, Thank you for the patch. On Fri, Jul 10, 2020 at 09:38:53AM +0200, Jacopo Mondi wrote: > On Thu, Jul 09, 2020 at 03:46:54PM +0200, Niklas Söderlund wrote: > > On 2020-07-09 10:41:20 +0200, Jacopo Mondi wrote: > > > The requirement of having the ImgU output height 32 pixels smaller > > > than the input frame produced by the CIO2 makes it complicated to > > > re-adjust the sensor produced size after the alignement has been > > > applied. > > > > > > To simplify the procedure, always ask for the full frame size from the > > > CIO2 unit. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++-------- > > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index d07f1a7b5ae8..feabffe641e1 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -156,8 +156,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > unsigned int rawCount = 0; > > > unsigned int outCount = 0; > > > Size yuvSize; > > > - Size size; > > > - > > > for (const StreamConfiguration &cfg : config_) { > > > const PixelFormatInfo &info = > > > PixelFormatInfo::info(cfg.pixelFormat); > > > @@ -174,11 +172,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > if (cfg.size > yuvSize) > > > yuvSize = cfg.size; > > > } > > > - > > > - if (cfg.size.width > size.width) > > > - size.width = cfg.size.width; > > > - if (cfg.size.height > size.height) > > > - size.height = cfg.size.height; > > > } > > > if (rawCount > 1 || outCount > 2) { > > > LOG(IPU3, Error) > > > @@ -189,10 +182,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > } > > > > > > /* Generate raw configuration from CIO2. */ > > > - cio2Configuration_ = data_->cio2_.generateConfiguration(size); > > > + Size sensorSize = data_->cio2_.sensor()->resolution(); > > > + cio2Configuration_ = data_->cio2_.generateConfiguration(sensorSize); > > > > Could this not impact framerate for the CIO2 if we always request the > > largest sensor size? Would it be possible instead to increase the size > > passed to CIO2Device::generateConfiguration() by 32? > > Yes it does, and this is probably just wrong, as it ignores the > requested raw capture size. > > Fun fact which actually lead me to add this change: if I keep the > previous version where the sensor is asked for the smaller as possible > resolution that supports all stream (+32 as you here suggest) the IPU3 > pipeline parameter calculations fail on Soraka for some output > resolutions. > > In example: calculating the configuration for a single stream in > 1280x720 with a frame resolution of 2112x1188 (which is the smaller > ov13858 resolution that is large enough) fails: the tool is not able > to find any suitable configuration that satisfied the pipeline > constraints: > > $ python pipe_config.py input=2112x1188 main=1280x720 > IndexError: list index out of range > > If I use the full frame size, everything is unicorns&rainbows again > > $ python pipe_config.py input=4224x3136 main=1280x720 > -------- The selected pipe configuration: -------------- > output_res_if:4220x2600 > output_res_bds:1688x1040 > output_res_gdc:1280x720 > FOV Horizontal:0.757576, FOV vertical:0.573980 > > Why ? I have no idea, and I think we should stop waiting for Intel to > actually care about this platform anymore, so I don't expect they > could help us debugging the tool. What's the alternative, to you want to debug it yourself ? :-) If there are issue, please report them, and you can CC Bingbu and Tomasz. > The re-implementation I made performs the same calculations as the > script, hence reducing the frame size makes a lot of configurations > fail. > > If you look at > https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml > https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov13858.xml > > The sensor is always asked for the full frame, Is it ? For the ov5670 I see three configurations using 1296x972, and for the ov13858 I see nine configurations with smaller sensor resolutions. > so I suspect that's how > the system is intenteded to work (even if, I wonder, what happens if I > connect a sensor that can only produce the above failing 2112x1188 > resolution ?) > > I -fear- for IPU3 we should always go full frame size to make the pipe > config happy, or there's maybe a requirement to respect some aspect > ratio I still haven't been able to figure out. In any case, this would > hinder the ability to capture raw+YUV, as the raw frame resolution in > that case is selected from the application, and it might prevent the > pipeline from being able to calculate a configuration. > > Not nice, I know. > > If that's how thing have to work, a software scaler for raw frames > would nee to be plugged to the pipeline to down-scale raw frames to > the desired resolution. Or we restrict RAW streams to the full size > only. What do you think ? No software scaler, no. If a configuration isn't supported by the hardware we can obviously reject it. For instance if the application requests raw in 640x480 and wants YUV in a larger resolution, as the ImgU can't downscale, we can't accept that, and we can go for a higher raw resolution or a small YUV resolution. There's nothing new under the sun here, and I'm OK with temporarily restricting the raw frames to the full sensor resolution if it helps moving forward, but let's keep in mind we may have to fix that at some point in the future, so we shouldn't hardcode too much. > > > if (!cio2Configuration_.pixelFormat.isValid()) > > > return Invalid; > > > > > > + LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString(); > > > + > > > /* > > > * Adjust the configurations if needed and assign streams while > > > * iterating them.
Hi Laurent, On Fri, Jul 10, 2020 at 03:07:40PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Jul 10, 2020 at 09:38:53AM +0200, Jacopo Mondi wrote: > > On Thu, Jul 09, 2020 at 03:46:54PM +0200, Niklas Söderlund wrote: > > > On 2020-07-09 10:41:20 +0200, Jacopo Mondi wrote: > > > > The requirement of having the ImgU output height 32 pixels smaller > > > > than the input frame produced by the CIO2 makes it complicated to > > > > re-adjust the sensor produced size after the alignement has been > > > > applied. > > > > > > > > To simplify the procedure, always ask for the full frame size from the > > > > CIO2 unit. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++-------- > > > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > index d07f1a7b5ae8..feabffe641e1 100644 > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > @@ -156,8 +156,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > > unsigned int rawCount = 0; > > > > unsigned int outCount = 0; > > > > Size yuvSize; > > > > - Size size; > > > > - > > > > for (const StreamConfiguration &cfg : config_) { > > > > const PixelFormatInfo &info = > > > > PixelFormatInfo::info(cfg.pixelFormat); > > > > @@ -174,11 +172,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > > if (cfg.size > yuvSize) > > > > yuvSize = cfg.size; > > > > } > > > > - > > > > - if (cfg.size.width > size.width) > > > > - size.width = cfg.size.width; > > > > - if (cfg.size.height > size.height) > > > > - size.height = cfg.size.height; > > > > } > > > > if (rawCount > 1 || outCount > 2) { > > > > LOG(IPU3, Error) > > > > @@ -189,10 +182,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > > } > > > > > > > > /* Generate raw configuration from CIO2. */ > > > > - cio2Configuration_ = data_->cio2_.generateConfiguration(size); > > > > + Size sensorSize = data_->cio2_.sensor()->resolution(); > > > > + cio2Configuration_ = data_->cio2_.generateConfiguration(sensorSize); > > > > > > Could this not impact framerate for the CIO2 if we always request the > > > largest sensor size? Would it be possible instead to increase the size > > > passed to CIO2Device::generateConfiguration() by 32? > > > > Yes it does, and this is probably just wrong, as it ignores the > > requested raw capture size. > > > > Fun fact which actually lead me to add this change: if I keep the > > previous version where the sensor is asked for the smaller as possible > > resolution that supports all stream (+32 as you here suggest) the IPU3 > > pipeline parameter calculations fail on Soraka for some output > > resolutions. > > > > In example: calculating the configuration for a single stream in > > 1280x720 with a frame resolution of 2112x1188 (which is the smaller > > ov13858 resolution that is large enough) fails: the tool is not able > > to find any suitable configuration that satisfied the pipeline > > constraints: > > > > $ python pipe_config.py input=2112x1188 main=1280x720 > > IndexError: list index out of range > > > > If I use the full frame size, everything is unicorns&rainbows again > > > > $ python pipe_config.py input=4224x3136 main=1280x720 > > -------- The selected pipe configuration: -------------- > > output_res_if:4220x2600 > > output_res_bds:1688x1040 > > output_res_gdc:1280x720 > > FOV Horizontal:0.757576, FOV vertical:0.573980 > > > > Why ? I have no idea, and I think we should stop waiting for Intel to > > actually care about this platform anymore, so I don't expect they > > could help us debugging the tool. > > What's the alternative, to you want to debug it yourself ? :-) If there > are issue, please report them, and you can CC Bingbu and Tomasz. > The alternative is to live with the risk of the procedure not working for all resolutions. Which is anyway better thant what we have today > > The re-implementation I made performs the same calculations as the > > script, hence reducing the frame size makes a lot of configurations > > fail. > > > > If you look at > > https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml > > https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov13858.xml > > > > The sensor is always asked for the full frame, > > Is it ? For the ov5670 I see three configurations using 1296x972, and > for the ov13858 I see nine configurations with smaller sensor > resolutions. Thanks for checking, I grepped on the xml file, but visual inspection revealed that I was not doing it right <input width="1296" height="972" format="CIO2" /> Now I see this too > > > so I suspect that's how > > the system is intenteded to work (even if, I wonder, what happens if I > > connect a sensor that can only produce the above failing 2112x1188 > > resolution ?) > > > > I -fear- for IPU3 we should always go full frame size to make the pipe > > config happy, or there's maybe a requirement to respect some aspect > > ratio I still haven't been able to figure out. In any case, this would > > hinder the ability to capture raw+YUV, as the raw frame resolution in > > that case is selected from the application, and it might prevent the > > pipeline from being able to calculate a configuration. > > > > Not nice, I know. > > > > If that's how thing have to work, a software scaler for raw frames > > would nee to be plugged to the pipeline to down-scale raw frames to > > the desired resolution. Or we restrict RAW streams to the full size > > only. What do you think ? > > No software scaler, no. If a configuration isn't supported by the > hardware we can obviously reject it. For instance if the application > requests raw in 640x480 and wants YUV in a larger resolution, as the > ImgU can't downscale, we can't accept that, and we can go for a higher > raw resolution or a small YUV resolution. There's nothing new under the > sun here, and I'm OK with temporarily restricting the raw frames to the > full sensor resolution if it helps moving forward, but let's keep in > mind we may have to fix that at some point in the future, so we > shouldn't hardcode too much. I see. I'll try to rework this with this in mind. Thanks j > > > > > if (!cio2Configuration_.pixelFormat.isValid()) > > > > return Invalid; > > > > > > > > + LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString(); > > > > + > > > > /* > > > > * Adjust the configurations if needed and assign streams while > > > > * iterating them. > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index d07f1a7b5ae8..feabffe641e1 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -156,8 +156,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() unsigned int rawCount = 0; unsigned int outCount = 0; Size yuvSize; - Size size; - for (const StreamConfiguration &cfg : config_) { const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat); @@ -174,11 +172,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() if (cfg.size > yuvSize) yuvSize = cfg.size; } - - if (cfg.size.width > size.width) - size.width = cfg.size.width; - if (cfg.size.height > size.height) - size.height = cfg.size.height; } if (rawCount > 1 || outCount > 2) { LOG(IPU3, Error) @@ -189,10 +182,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() } /* Generate raw configuration from CIO2. */ - cio2Configuration_ = data_->cio2_.generateConfiguration(size); + Size sensorSize = data_->cio2_.sensor()->resolution(); + cio2Configuration_ = data_->cio2_.generateConfiguration(sensorSize); if (!cio2Configuration_.pixelFormat.isValid()) return Invalid; + LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString(); + /* * Adjust the configurations if needed and assign streams while * iterating them.
The requirement of having the ImgU output height 32 pixels smaller than the input frame produced by the CIO2 makes it complicated to re-adjust the sensor produced size after the alignement has been applied. To simplify the procedure, always ask for the full frame size from the CIO2 unit. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)