video: introduction of STM32 DCMIPP driver #89407
Conversation
github-actions
bot
added
platform: STM32
github-actions
bot
requested review from
djiatsaf-st,
erwango,
FRASTM,
gautierg-st,
GeorgeCGV,
josuah,
loicpoulain,
marwaiehm-st,
mathieuchopstm and
ngphibang
There was a problem hiding this comment.
Actually here is one thing you might be interested in:
- Adding the board to the "build_all" video driver test, so that CI always checks that it at least build when someone else changes i.e. some API.
-> https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/build_all/video/boards
-> https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/build_all/video/testcase.yaml
[EDIT: I flagged as "change requested" just to raise awareness, but it is not up to me to decide I think]
|
Thank you for all those comments. I am almost done with the addition of the parallel interface addition so I'll push an update of the driver with parallel interface / possibility to instanciate it on other SOCs (with single pipe) and take care of the comments you provided. Thanks again. |
drivers/video/video_stm32_dcmipp.c
Outdated
| if (fmt->pitch != fmt->width * video_bits_per_pixel(fmt->pixelformat) / BITS_PER_BYTE) { | ||
| LOG_ERR("Format pitch (%d) does not match with format", fmt->pitch); | ||
| return -EINVAL; | ||
| } | ||
|
|
||
| if ((fmt->pitch * fmt->height) > CONFIG_VIDEO_BUFFER_POOL_SZ_MAX) { | ||
| LOG_ERR("Frame too large for VIDEO_BUFFER_POOL"); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
checking against a format based on another utility function e.g., video_bits_per_pixel() seems not robust as the format can be valid but may not present in the utility function, for example, I can define my own bbp in my driver:
static const struct mtk_camsv_format_info mtk_camsv_format_info[] = {
{
.fourcc = V4L2_PIX_FMT_MTISP_SBGGR12,
.code = MEDIA_BUS_FMT_SBGGR12_1X12,
.packed = true,
.bpp = 12,
}, {
.fourcc = V4L2_PIX_FMT_MTISP_SGBRG12,
.code = MEDIA_BUS_FMT_SGBRG12_1X12,
.packed = true,
.bpp = 12,
}, {
Moreover, this check will be done in the sensor driver, (ii) checking against the heap size will be done when allocating buffers as it is another aspect, I think.
FYI, in aa9607e, format pitch is decided by the bridge driver rather by the application, so here and in get_fmt(), we can determine it rather than checking it.
There was a problem hiding this comment.
On a general basis I'd rather rely on a generic function such as the one already available (video_bits_per_pixel) rather having to reimplement all the format informations within each and every drivers. If the receiver knows the formats it accept as an input, there there is no problem at adding specifically those formats within the driver in addition to the other ones for example. But even that, such format is that THAT common I think. Here, in the example above, this seems to be a packed version of the RAW12. We already have then within our enum.
Regarding the HEAP / pitch, yes I agree, same as within the Video misc PR.
avolmat-st
force-pushed
the
video_dcmipp_driver
branch
from
da3fb47 to
c6bc027
Compare
avolmat-st
force-pushed
the
video_dcmipp_driver
branch
from
c6bc027 to
dfb23cb
Compare
|
Rebased now on top of HEAD. DCMIPP driver now supports both CSI & Parallel input and has been tested on STM32N6 (in CSI version) and STM32MP13 (single pipe / parallel input). It should be also possible to use it on stm32h7rs however I haven't tested it on this platform. |
avolmat-st
force-pushed
the
video_dcmipp_driver
branch
from
dfb23cb to
09bda9c
Compare
|
Small fix to correct compliance check. |
avolmat-st
force-pushed
the
video_dcmipp_driver
branch
from
704e426 to
29cbd90
Compare
|
Rebased on top of HEAD since video CMakefiles / Kconfig were conflicting |
The STM32 Digital Camera Memory Interface Pixel Processor (DCMIPP) is a multi-pipeline camera interface allowing to capture and process frames from parallel or CSI interfaces depending on its version. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
Add weak functions and their call within the dcmipp driver so that externally provided ISP control functions can be called by the driver at right timing in order to perform the control of the ISP part of the DCMIPP. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
Add node describing the dcmipp in stm32n6.dtsi Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
Add configuration of the IC17 and IC18 clock dividers, used by the dcmipp and csi IPs in the stm32n6. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
Add csi_22pins_connector and related label in order to use shields relying on csi_pins_connector. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
Add node describing the DCMIPP available on stm32mp135. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
Add configuration for the STM32N6 DCMIPP driver which currently requires to have the sensor pixel format and resolutions set via KConfig Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
avolmat-st
force-pushed
the
video_dcmipp_driver
branch
from
29cbd90 to
1b54d25
Compare
| /* Weak function declaration in order to interface with external ISP handler */ | ||
| void __weak stm32_dcmipp_isp_vsync_update(DCMIPP_HandleTypeDef *hdcmipp, uint32_t Pipe) | ||
| { | ||
| } | ||
|
|
||
| int __weak stm32_dcmipp_isp_init(DCMIPP_HandleTypeDef *hdcmipp, const struct device *source) | ||
| { | ||
| return 0; | ||
| } | ||
|
|
||
| int __weak stm32_dcmipp_isp_start(void) | ||
| { | ||
| return 0; | ||
| } | ||
|
|
||
| int __weak stm32_dcmipp_isp_stop(void) | ||
| { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
For my better knowledge, could you give some more details about the external ISP handler who could override these functions ? Is it supposed to be the application, the drivers or some library ? Thanks
There was a problem hiding this comment.
The application (or a library linked together with the application) would implement those function which perform the ISP configuration via the DCMIPP HAL code. There are already available library for that purpose, such as
https://wiki.st.com/stm32mcu/wiki/ISP:ISP_middleware
https://github.com/STMicroelectronics/STM32CubeN6/tree/v1.1.0/Middlewares/ST/STM32_ISP_Library
So the Zephyr DCMIPP driver is performing all the pipeline configuration and the ISP part (aka, statistics analysis, configuration of various ISP blocks) are done via those handler.
There was a problem hiding this comment.
Looks like we need some thing equivalent to libcamera mechanisms in the future to support these cases generically
There was a problem hiding this comment.
As in #90919 (review), CI failure is not due to the PR change.
github-actions
bot
added
area: Devicetree
area: Samples
josuah
force-pushed
the
video_dcmipp_driver
branch
from
1e01198 to
1b54d25
Compare
|
I did not mean to push these two commits on I did not even know it was possible, probably due to the fact that the branch was flagged "editable by maintainers". We are now back to the original 1b54d25 (an occasion to re-trigger CI...) |
|
During zephyrproject-rtos#89407 a driver-specific header stm32_dcmipp.h got introduced to support new functionnalities not yet covered by video APIs. Move this header into a new <include/zephyr/drivers/video/> include direcctory like what other driver classes do. Signed-off-by: Josuah Demangeon <me@josuah.net>
During #89407 a driver-specific header stm32_dcmipp.h got introduced to support new functionnalities not yet covered by video APIs. Move this header into a new <include/zephyr/drivers/video/> include direcctory like what other driver classes do. Signed-off-by: Josuah Demangeon <me@josuah.net>
During zephyrproject-rtos#89407 a driver-specific header stm32_dcmipp.h got introduced to support new functionnalities not yet covered by video APIs. Move this header into a new <include/zephyr/drivers/video/> include direcctory like what other driver classes do. Signed-off-by: Josuah Demangeon <me@josuah.net>
This PR is the introduction of the DCMIPP (camera pipeline) for STM32. It currently supports the STM32N6, however it is also applicable on some other SOCs such as STM32H7RS or STM32MP13xx. (for those latter ones, some modifications are necessary since they have simpler pipelines and no CSI hence new compatible will be needed).
This PR depends on the PR fixing the 8 bit raw bayer formats: #89397
Some TODOs / FIXME remains within the driver which I plan to correct prior to merging this PR however I first publish the PR to hopefully get some first comments.
Some points might benefit to other drivers as well and as such would be candidate to go to the video-common part. For example I've added a macro to get the COLORSPACE from a pixelformat in order to know if it is YUV / RGB or Bayer. Moreover, as far as I know Zephyr video format currently do not have yet information about quantization which would be necessary in order to know if a format is encoded on the limited or full range. Indeed, the DCMIPP performs color-conversion and conversion matrix depends on the range (for now I've only kept simple color conversion while waiting for quantization).
I also chose to add Kconfig entries in otder to define what is expected to be use for the sensor format. Indeed, since the DCMIPP can perform format conversion, I feel that it makes the driver (hence code size) much simpler to be able to define which format / size is to be used from the sensor rather than having to figure out that within the driver. Here as well, some other camera pipeline might benefit from it, in which case thise could go to the common Kconfig.