-
Notifications
You must be signed in to change notification settings - Fork 7.9k
video: introduction of STM32 VENC driver for H264 hardware video compression #92884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
video: introduction of STM32 VENC driver for H264 hardware video compression #92884
Conversation
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
|
drivers/video/video_stm32_venc.c
Outdated
ISR_DIRECT_DECLARE(stm32_venc_isr) | ||
{ | ||
VENC_EWL_TypeDef *enc = &ewl_instance; | ||
u32 hw_handshake_status = READ_BIT(VENC_REG(BASE_HEncInstantInput >> 2U), (1U << 29U)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No HW define for (1 << 29)? (Nit: should be 1U << 29
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for preparing VENC!
This will really make video over network on Zephyr interesting.
It seems like Zephyr lacks a few APIs from Linux (no H.264 format definition, no planar API for NV12...), so if anything feels like missing, let us know so that the missing API can be provided in parallel.
drivers/video/video_stm32_venc.c
Outdated
input = k_fifo_get(&data->fifo_input, K_NO_WAIT); | ||
if (!input) | ||
return 0; | ||
LOG_DBG("input=%p\n", input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the meantime that a better buffering model is worked on, the way to video_buffer_free()
a buffer is by giving it back to the application which video_dequeue()
it and reuse it in the next cycle, for instance via data->fifo_input_out
(in addition to a data->fifo_input_in
).
If encountering errors where running out of buffers hopefully this can help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get the point sorry. If no input buffer is there we silently exit and we will retry at next encoding, do you see a problem around that ?
} | ||
|
||
data->pixelformat = fmt->pixelformat; | ||
data->pitch = fmt->pitch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following these changes, when the application sets the format, the driver sets the ->pitch
field.
But for H.264 the pitch might as well be 0 as a convention for variable data size...
zephyr/include/zephyr/drivers/video.h
Lines 1638 to 1647 in 9219c81
case VIDEO_PIX_FMT_ARGB32: | |
case VIDEO_PIX_FMT_ABGR32: | |
case VIDEO_PIX_FMT_RGBA32: | |
case VIDEO_PIX_FMT_BGRA32: | |
return 32; | |
default: | |
/* Variable number of bits per pixel or unknown format */ | |
return 0; | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pitch is used by tcpserversink application to allocate buffers, see
buffers[i] = video_buffer_alloc(fmt.pitch * fmt.height, K_FOREVER); |
No specific processing is necessary currently as part of the stm32_dcmipp_isp_start function hence remove the assert(0) currently preventing stop of the isp. Signed-off-by: Alain Volmat <[email protected]>
Allow to call HAL_DCMIPP_PIPE_SetConfig when the pipe state is READY. Currently it is only possible to use this function if the pipe state is RESET and ERROR states. Indeed, it is possible to reconfigure the PIPE as soon as the pipe is not currently being used, aka in the READY state. With that done, it becomes possible to change the PIPE configuration between two use-cases without having to DeInit / Init the HAL_DCMIPP or use the unitary pixel packer functions. Signed-off-by: Alain Volmat <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of comment, driver aside.
Please split the PR and do a specific PR for the first commits that are not directly linked to the VENC driver. These can be reviewed and integrated faster.
For the changes on the sample, I let video team decide if this should be kept within the tcpserver sample or split into a different sample
@@ -48,6 +48,15 @@ config VIDEO_PIXEL_FORMAT | |||
help | |||
Pixel format of the video frame. If not set, the default pixel format is used. | |||
|
|||
config VIDEO_NUM_FRAMES | |||
int "Capture N-buffering" | |||
default 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default 2
to fit the description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some pending comments, not sure why they were missing
|
||
#define TRACE_EWL | ||
|
||
#define EWL_USE_POLLING_SYNC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a dedicated Kconfig symbol if required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This first implementation only supports polling while interrupt mode is preferable anyway, but as it is not functional it is kept with polling mode for the time being.
a32b1da
to
deac1fb
Compare
deac1fb
to
938f1f9
Compare
depends on DT_HAS_ST_STM32_VENC_ENABLED | ||
select HAS_STM32LIB | ||
select USE_STM32_LL_VENC | ||
select USE_STM32_HAL_RIF if SOC_SERIES_STM32N6X |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
select RESET missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the driver doesn't do reset so it should either be added in the driver (and keeping this select) or avoid putting it here and removing it from the binding/dts.
|
||
struct video_stm32_venc_data { | ||
const struct device *dev; | ||
struct video_format fmt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except if I missed something, this fmt element of data seems to be unused, pixelformat/height/width/pitch stored within the data structure being directly accessed.
Maybe struct video_format can be used instead of the pixelformat/height/width/pitch, only the type element of struct video_format being unused probably.
/* read first part of the configuration stored in register 63 */ | ||
cfgval = LL_VENC_ReadRegister(63UL); | ||
|
||
cfg_info.maxEncodedWidth = cfgval & ((1U << 12U) - 1U); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No update ?
|
||
u32 EWLReadAsicID(void) | ||
{ | ||
return LL_VENC_ReadRegister(0UL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment: it seems that the encswhwregisters.h provide mechanism to parse the field and read/write the registers. My feeling is that this would allow to hide some of the parsing within the lower library and have a simpler Zephyr driver, but maybe at the cost of the code size. Is that correct ?
It seems that this create an image of the register map etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zephyr EWL wrapping layer code is copied/pasted from EWL reference code that can be found here: https://github.com/STMicroelectronics/STM32CubeN6/blob/main/Middlewares/ST/VideoEncoder_EWL/ewl_impl.c
I don't want to deviate too much from this sensible code, this will make the potential bug fixing reporting to this code much harder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will of course -it has already been done-, but reasonnably and I will keep strong focus on not big-banging this code.
} | ||
|
||
data->pixelformat = fmt->pixelformat; | ||
data->pitch = fmt->pitch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once verification of the parameter given by the application, the driver should set acceptable values for width/height/pixelformat and set the pitch corresponding to the data generated by the HW.
The application is not setting the pitch itself.
drivers/video/video_stm32_venc.c
Outdated
{ | ||
struct video_stm32_venc_data *data = dev->data; | ||
|
||
if (fmt == NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed since this check is already done within the framework in zephyr/include/driver/video.h before reaching the driver
static inline int video_get_format(const struct device *dev, struct video_format *fmt)
fmt->height = data->height; | ||
fmt->width = data->width; | ||
fmt->pitch = data->pitch; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_fmt function should return the corresponding format depending on the type (INPUT or OUTPUT)
drivers/video/video_stm32_venc.c
Outdated
ret = H264EncInit(&cfg, &data->encoder); | ||
if (ret != H264ENC_OK) { | ||
LOG_ERR("H264EncInit error=%d", ret); | ||
return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using -1 but use -Exxx value
struct video_format fmt; | ||
struct k_fifo fifo_input; | ||
struct k_fifo fifo_output_in; | ||
struct k_fifo fifo_output_out; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have difficulties understanding to which queue those fifo are referring to. I'd have expected 2 for the input and 2 for the output. Could you clarify ?
@@ -490,3 +491,7 @@ csi_interface: &dcmipp { | |||
csi_ep_in: endpoint { }; | |||
}; | |||
}; | |||
|
|||
&venc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is this new zephyr,videoenc to target an encoder available within a board, should the venc be kept to disabled (aka avoid this status = okay here) and be set to okay via the sample / application overlay ?
Setting the venc status = okay here would lead to enabling the VENC even if it is not needed.
938f1f9
to
638af99
Compare
const struct video_stm32_venc_config *config = dev->config; | ||
struct video_stm32_venc_data *data = dev->data; | ||
int err; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reset should most probably be added around here, checking that reset controller is ready, then reset_line_toggle_dt.
H264EncIn encIn = {0}; | ||
H264EncOut encOut = {0}; | ||
|
||
if (!data->encoder) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shound't this encoder starting be done at the set_stream(enable=true) time ? This would also make it symetrical to the set_stream(enable=false) into which the encoder is stopped.
Apart from that the encoder_end/encoder_start function would only be called in case of error handling.
In order to do that and since encoder_start do need one output buffer, the set_stream need to ensure that there is at least one buffer of type OUTPUT queued.
output->bytesused = encOut.streamSize; | ||
LOG_DBG("SPS/PPS generated, size= %d", output->bytesused); | ||
|
||
k_fifo_put(&data->fifo_output_in, output); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that at start time the encoder needs 1 output buffer which will be later on pushed back to the application. For that purpose, the buffer is pulled out of the output_in fifo at the beginning of the function.
However since it is pushed back to the applicatiob at the end via the fifo_output_out fifo, it shouldn't be pushed into the fifo_output_in as well.
output->bytesused = encOut.streamSize; | ||
LOG_DBG("output=%p, encOut.streamSize=%d", output, encOut.streamSize); | ||
|
||
k_fifo_put(&data->fifo_output_in, output); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well, could you clarify why same buffer is pushed back into both output_in and output_out fifo ?
return 0; | ||
} | ||
|
||
output = k_fifo_get(&data->fifo_output_in, K_FOREVER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that this function encode_frame is called after each queue of an input buffer, which ensure that there will always be a buffer available within fifo_input, however we are not sure about the fifo_output_in, meaning that this might block for a long time until a buffer is queued back into the output queue.
Moreover, doing such K_FOREVER k_fifo_get might make it impossible to actually stop the device since we would be blocked here. Is that correct ? For that purpose, k_fifo_cancel_wait could be called at set_stream(enable=false) time to unblock. In such case, this output ptr should be check for validity since it isn't ensured that it will be always valid.
Maybe a preferred approach would actually be to only call encode_frame when we are sure that there is a buffer available in BOTH input_in and output_in fifos. This can be checked via the k_fifo_is_empty function. This could be checked on the queue ops of the driver, the queue will only call encode_frame if it is sure that both input & output buffers are available. That done the driver will never be waiting into a k_fifo_get forever.
I implemented it in this way in the JPEG encoder driver I have been working on and hope to be able to propose some M2M helper since this buffer management code for M2M isn't really device specific so it might be possible to make it common to all M2M drivers if they want to.
{ | ||
struct video_stm32_venc_data *data = dev->data; | ||
|
||
*vbuf = k_fifo_get(&data->fifo_output_out, timeout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a pipeline management point of view, the "element" making the connection between the camera pipeline and the encoder would expect to be able to do usual queue/dequeue of buffers and perform the exchange between the encoder and the camera pipeline so I think it is still important to have the proper fifo_input_in / fifo_input_out to be able to give back input buffer to the application.
Addition of description for the STM32 Video encoder (VENC). Signed-off-by: Hugues Fruchet <[email protected]>
The STM32 video encoder (VENC) peripheral is a hardware accelerator allowing to compress RGB/YUV frames into H264 video bitstream chunks. Signed-off-by: Hugues Fruchet <[email protected]>
Add node describing the venc in stm32n6.dtsi Signed-off-by: Hugues Fruchet <[email protected]>
Enable video encoder on stm32n6570_dk discovery board. Signed-off-by: Hugues Fruchet <[email protected]>
638af99
to
c689972
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comments, bunch of fixes sent related to comments and also to QA tools.
Still some opened points on which I have replied...
Still some comments to process...
drivers/video/video_stm32_venc.c
Outdated
#define VENC_RATECTRL_FPS 30 | ||
#define VENC_RATECTRL_QP 25 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This first implementation is hardcoded 30fps QP 25 constant bitrate.
|
||
#define ALIGNED_SIZE(x) ((((x) + ALIGNMENT_INCR - 1UL) / ALIGNMENT_INCR) * ALIGNMENT_INCR) | ||
|
||
static VENC_EWL_TypeDef ewl_instance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't understand sorry...
/* read first part of the configuration stored in register 63 */ | ||
cfgval = LL_VENC_ReadRegister(63UL); | ||
|
||
cfg_info.maxEncodedWidth = cfgval & ((1U << 12U) - 1U); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is copied from encoder ewl reference implementation, I don't want to take the risk to refactor this code. See https://github.com/STMicroelectronics/STM32CubeN6/blob/main/Middlewares/ST/VideoEncoder_EWL/ewl_impl.c
cfg_info.busType = (cfgval >> 20U) & 15U; | ||
cfg_info.synthesisLanguage = (cfgval >> 16U) & 15U; | ||
cfg_info.busWidth = (cfgval >> 12U) & 15U; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is copied from encoder ewl reference implementation, I don't want to take the risk to refactor this code. See https://github.com/STMicroelectronics/STM32CubeN6/blob/main/Middlewares/ST/VideoEncoder_EWL/ewl_impl.c
info->busAddress = (ptr_t)info->virtualAddress; | ||
|
||
/* check buffer alignment */ | ||
__ASSERT_NO_MSG((info->busAddress & 0b111UL) == 0U); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from EWL reference code...
__ASSERT_NO_MSG(inst != NULL); | ||
} | ||
|
||
void *EWLmalloc(uint32_t n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from EWL reference code...
/* check how to clear IRQ flags for VENC */ | ||
uint32_t clrByWrite1 = (EWLReadReg(inst, BASE_HWFuse2) & HWCFGIrqClearSupport); | ||
do { | ||
irq_stats = VENC_REG(1UL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from EWL reference code...
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zephyr EWL wrapping layer code is copied/pasted from EWL reference code that can be found here: https://github.com/STMicroelectronics/STM32CubeN6/blob/main/Middlewares/ST/VideoEncoder_EWL/ewl_impl.c
I don't want to deviate too much from this sensible code, this will make the potential bug fixing reporting to this code much harder.
Code integrated in Zephyr shall follow Zephyr coding guidelines and best practices, use Zephyr APIs, recommended MACROs....
If there is a need to not modify it, it shall be hosted in the HAL repo instead.
This PR introduces H264 hardware video compression thanks to VENC peripheral of STM32N6.
A new stm32 video encoder driver has been introduced which relies on vc8000nanoe software stack [1].
This is a first basic porting to enable H264 encoding use-case.
RAM memory resources required by VENC encoding process are allocated in external PSRAM, camera frames
captured by DCMIPP and encoded by VENC are also allocated in external PSRAM.
The video encoder driver currently lack of controls to configure the encoding process, default configuration
targets H264 1080p 2Mb/s constant bitrate.
In order to test, the tcpserversink video sample has been enhanced to support video compression in order to stream small H264 compressed video chunks instead of big raw uncompressed images [2].
[1] zephyrproject-rtos/hal_stm32#295
[2] #95862