[libcamera-devel,00/20] libcamera: ipu3: Rework pipe configuration
mbox series

Message ID 20200714104212.48683-1-jacopo@jmondi.org
Headers show
Series
  • libcamera: ipu3: Rework pipe configuration
Related show

Message

Jacopo Mondi July 14, 2020, 10:41 a.m. UTC
Hello,
  quite a big update compared to v2, even if most changes happens in 3 patches

The new stream size computations which happens at generateConfiguration()
and validate() time now enforces a new rule
-  All YUV streams should be aligned to the 64 pixels in widht and 32 pixels
   in height multiple which is strictly smaller than the CIO2 output frame.

This is reflected in how configurations are generated, how the CIO2 size is
computed when no raw stream is requested, and how the YUV streams are adjusted
to respect this margin constraint and the device required alignment.

Most of this changes happens in patch [5/20] and [14/20]

I also have simplified a bit the ImgU pipe parameters calculation. There is
a huge space for improvements there probably, but I'm very cautious in touching
this. The changelog is in the patch for the records.

Also, compared to v2, the ImgU pipe gets configured only if there's at least
one YUV stream requested, otherwise setting sizes for the IF and BDS rectangle
without actually capturing frames from any of the ImgU output, stalls the
ImgU which require system to be rebooted to work correctly again.

Validated by using all the possible stream roles combinations, and inspecting
images which despite the horrible quality (as there's no 3A working) are
actually there.

I'm sure that poking the system with more size combinations will highlight more
issues, but I still think this is an acceptable start.

Thanks
  j

v2 -> v3:
- Rework stream size logic to respect alignements and margins
- Add utils::alignUp/utils::alignDown
- Rework stream adjustments
- Simplify a bit the ImgU pipe parameters calculation
- Do not configure the ImgU if only RAW stream is requested

Jacopo Mondi (19):
  libcamera: ipu3: Rename mbusCodesToInfo
  libcamera: utils: Add alignUp and alignDown functions
  libcamera: geometry: Add isNull() function to Rectangle class
  libcamera: ipu3: Remove streams from generateConfiguration
  libcamera: ipu3: Make sure the config is valid
  libcamera: ipu3: cio2: Report format and sizes
  libcamera: ipu3: Do not overwrite StreamConfiguration
  libcamera: ipu3: Report StreamFormats
  libcamera: ipu3: Remove initialization of Size
  libcamera: ipu3: Validate the stream combination
  libcamera: camera: Zero streams before validate()
  libcamera: ipu3: cio2: Mark sensor() as const
  libcamera: ipu3: Adjust and assign streams in validate()
  libcamera: ipu3: Remove streams from IPU3CameraConfiguration
  libcamera: ipu3: Remove camera_ from IPU3CameraConfiguration
  libcamera: ipu3: imgu: Calculate ImgU pipe configuration
  libcamera: ipu3: Validate the pipe configuration
  libcamera: ipu3: Configure ImgU with the computed parameters
  libcamera: ipu3: imgu: Rename configureInput()

Laurent Pinchart (1):
  libcamera: geometry: Add helper functions to the Size class

 include/libcamera/geometry.h         |  34 +++
 include/libcamera/internal/utils.h   |   3 +
 src/libcamera/camera.cpp             |   4 +-
 src/libcamera/geometry.cpp           |  42 +++
 src/libcamera/pipeline/ipu3/cio2.cpp |  54 +++-
 src/libcamera/pipeline/ipu3/cio2.h   |   7 +-
 src/libcamera/pipeline/ipu3/imgu.    |   0
 src/libcamera/pipeline/ipu3/imgu.cpp | 387 +++++++++++++++++++++++--
 src/libcamera/pipeline/ipu3/imgu.h   |  22 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp | 414 +++++++++++++--------------
 src/libcamera/utils.cpp              |  22 ++
 test/geometry.cpp                    |  35 +++
 test/utils.cpp                       |  18 ++
 13 files changed, 799 insertions(+), 243 deletions(-)
 create mode 100644 src/libcamera/pipeline/ipu3/imgu.

--
2.27.0

Comments

Laurent Pinchart July 14, 2020, 10:11 p.m. UTC | #1
Hi Jacopo,

On Tue, Jul 14, 2020 at 12:41:52PM +0200, Jacopo Mondi wrote:
> Hello,
>   quite a big update compared to v2, even if most changes happens in 3 patches
> 
> The new stream size computations which happens at generateConfiguration()
> and validate() time now enforces a new rule
> -  All YUV streams should be aligned to the 64 pixels in widht and 32 pixels
>    in height multiple which is strictly smaller than the CIO2 output frame.
> 
> This is reflected in how configurations are generated, how the CIO2 size is
> computed when no raw stream is requested, and how the YUV streams are adjusted
> to respect this margin constraint and the device required alignment.
> 
> Most of this changes happens in patch [5/20] and [14/20]
> 
> I also have simplified a bit the ImgU pipe parameters calculation. There is
> a huge space for improvements there probably, but I'm very cautious in touching
> this. The changelog is in the patch for the records.

I understand your concern here, so I won't push too hard, and we can
improve the code further on top.

> Also, compared to v2, the ImgU pipe gets configured only if there's at least
> one YUV stream requested, otherwise setting sizes for the IF and BDS rectangle
> without actually capturing frames from any of the ImgU output, stalls the
> ImgU which require system to be rebooted to work correctly again.

That's a lovely one :-) Could you capture this in a comment in the code
? Should we avoid starting the ImgU at all in that case ? To be very
clear, it's not something that needs to be fixed in this series.

> Validated by using all the possible stream roles combinations, and inspecting
> images which despite the horrible quality (as there's no 3A working) are
> actually there.
> 
> I'm sure that poking the system with more size combinations will highlight more
> issues, but I still think this is an acceptable start.

Absolutely ! Thank you for your great work here.

> v2 -> v3:
> - Rework stream size logic to respect alignements and margins
> - Add utils::alignUp/utils::alignDown
> - Rework stream adjustments
> - Simplify a bit the ImgU pipe parameters calculation
> - Do not configure the ImgU if only RAW stream is requested
> 
> Jacopo Mondi (19):
>   libcamera: ipu3: Rename mbusCodesToInfo
>   libcamera: utils: Add alignUp and alignDown functions
>   libcamera: geometry: Add isNull() function to Rectangle class
>   libcamera: ipu3: Remove streams from generateConfiguration
>   libcamera: ipu3: Make sure the config is valid
>   libcamera: ipu3: cio2: Report format and sizes
>   libcamera: ipu3: Do not overwrite StreamConfiguration
>   libcamera: ipu3: Report StreamFormats
>   libcamera: ipu3: Remove initialization of Size
>   libcamera: ipu3: Validate the stream combination
>   libcamera: camera: Zero streams before validate()
>   libcamera: ipu3: cio2: Mark sensor() as const
>   libcamera: ipu3: Adjust and assign streams in validate()
>   libcamera: ipu3: Remove streams from IPU3CameraConfiguration
>   libcamera: ipu3: Remove camera_ from IPU3CameraConfiguration
>   libcamera: ipu3: imgu: Calculate ImgU pipe configuration
>   libcamera: ipu3: Validate the pipe configuration
>   libcamera: ipu3: Configure ImgU with the computed parameters
>   libcamera: ipu3: imgu: Rename configureInput()
> 
> Laurent Pinchart (1):
>   libcamera: geometry: Add helper functions to the Size class
> 
>  include/libcamera/geometry.h         |  34 +++
>  include/libcamera/internal/utils.h   |   3 +
>  src/libcamera/camera.cpp             |   4 +-
>  src/libcamera/geometry.cpp           |  42 +++
>  src/libcamera/pipeline/ipu3/cio2.cpp |  54 +++-
>  src/libcamera/pipeline/ipu3/cio2.h   |   7 +-
>  src/libcamera/pipeline/ipu3/imgu.    |   0
>  src/libcamera/pipeline/ipu3/imgu.cpp | 387 +++++++++++++++++++++++--
>  src/libcamera/pipeline/ipu3/imgu.h   |  22 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 414 +++++++++++++--------------
>  src/libcamera/utils.cpp              |  22 ++
>  test/geometry.cpp                    |  35 +++
>  test/utils.cpp                       |  18 ++
>  13 files changed, 799 insertions(+), 243 deletions(-)
>  create mode 100644 src/libcamera/pipeline/ipu3/imgu.