Skip to content

Video: Display: NXP: Decouple PxP from eLCDIF. Make a new driver for PxP in video m2m category #92366

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.

Sign up for GitHub

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
+531 −553
Draft

Video: Display: NXP: Decouple PxP from eLCDIF. Make a new driver for PxP in video m2m category #92366

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7906f4d
drivers: display: elcdif: Decouple PxP from the display controller
ngphibang Jun 24, 2025
065487a
drivers: dma: Drop NXP's PxP driver
ngphibang Jun 26, 2025
bad94d3
video: controls: Add VIDEO_CID_ROTATE
ngphibang Jun 24, 2025
81acde2
drivers: video: Add PxP m2m driver
ngphibang Jun 29, 2025
b4c8ede
samples: video: capture: Add support for m2m transformation
ngphibang Jul 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions samples/drivers/video/capture/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@ project(video_capture)

FILE(GLOB app_sources src/*.c)
target_sources(app PRIVATE ${app_sources})

if(CONFIG_VIDEO_TRANSFORM)
target_sources(app PRIVATE boards/${BOARD}/transform.c)
endif()
5 changes: 5 additions & 0 deletions samples/drivers/video/capture/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ config VIDEO_CTRL_VFLIP
help
If set, mirror the video frame vertically

config VIDEO_TRANSFORM
bool "Perform some m2m transforms on the video frame"
help
If set, the video frame will go through some m2m transforms before being displayed

endmenu

source "Kconfig.zephyr"
106 changes: 106 additions & 0 deletions samples/drivers/video/capture/boards/mimxrt1170_evk/transform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There does not seem to be any source in the boards/ directories of samples:
find samples/ -path '*/boards/*.c' | grep -v samples/boards/

However there are some in src/ at times:
find samples/ -name src -exec ls {} +

How about renaming the file to be slightly more generic, such as samples/drivers/video/capture/src/transform_mcux_pxp.c so that all boards that supports pxp can include this from CMakeLists.txt? i.e. a build condition on CONFIG_VIDEO_MCUX_PXP && CONFIG_VIDEO_SAMPLE_TRANSFORM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File name is transform.c and this is done on purpose

if(CONFIG_VIDEO_TRANSFORM)
    target_sources(app PRIVATE boards/${BOARD}/transform.c)
endif()

to allow each board to write their custom transform.c without cluttering the CMakeList.txt with plenty of if(), I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need a libZcamera 😅

Copy link
Contributor

@josuah josuah Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zephyr strives to abstract the hardware and planned no room for an application-side board.c which we seem to need now.

Is something like this acceptable compromise for keeping all sources in src?

if(CONFIG_VIDEO_TRANSFORM)
    target_sources(app PRIVATE src/transform_${BOARD}.c)
endif()

This could make things easy to group:

src/transform_mimxrt1180_evk.c
src/transform_mimxrt1170_evk.c
src/transform_mimxrt1160_evk.c
src/transform_mimxrt11xx_evk.h

Probably other variants are possible too...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another way of placing the applicatation board's specific source code, but placing them all in src/ seems pollluting the common space ? Is it better to place them in board/ instead as config, overlay are also placed there ?

if(CONFIG_VIDEO_TRANSFORM)
    target_sources(app PRIVATE boards/transform_${BOARD}.c)
endif()

It seems there is no recomendation for where to place the application's board-specific source code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked recommendations on the #build-system channel, and from what I understood:

  • if it is a board specific sample then it should go to samples/boards/<board>
  • if it is some filter for which should use which strategy, then this might still fit samples/drivers/video/
  • in either way, .c files better be kept out of ./boards/ expected to be scanned for config files by the build system

Maybe boards can define CONFIG_VIDEO_SAMPLE_TRANSFORM_PXP=y in their prj.conf? Then this reduces the number of files in ./src/ while allowing all the per-board configuration in ./boards/...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this info !

Maybe boards can define CONFIG_VIDEO_SAMPLE_TRANSFORM_PXP=y in their prj.conf? Then this reduces the number of files in ./src/ while allowing all the per-board configuration in ./boards/...?

But then in CMakeList.txt it seems we need to do if(CONFIG_VIDEO_SAMPLE_TRANSFORM_PXP) for each HW (PxP, JPEG decoder, etc.) ?

How about ?

if(CONFIG_VIDEO_TRANSFORM)
    target_sources(app PRIVATE src/${BOARD}/transform.c)
endif()

of it is preferred to not create sub-folders in src/, we can do as before:

if(CONFIG_VIDEO_TRANSFORM)
    target_sources(app PRIVATE src/transform_${BOARD}.c)
endif()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a reference for this but does not cause problem as per the build system.

What I wonder, however, is how to avoid an explosion of number of board-specific files due to then:

  • number of vendors
  • number of SoC families per vendor
  • number of variant per SoC family
  • number of devkit variant per SoC variant (i.e. "EVK" vs "FRDM")
  • number of community-made variants per SoC family

A compromise could be defining the string in Kconfig so that the makefile does not need to be changed:

if(CONFIG_VIDEO_TRANSFORM)
    target_sources(app PRIVATE src/transform_${CONFIG_VIDEO_TRANSFORM}.c)
endif()

Then boards can set any of this depending on how board-specific things are:

CONFIG_VIDEO_TRANSFORM="nxp_pxp"
CONFIG_VIDEO_TRANSFORM="nxp_smartdma"
CONFIG_VIDEO_TRANSFORM="teensy_mm"
CONFIG_VIDEO_TRANSFORM="dcmipp"
CONFIG_VIDEO_TRANSFORM="stm32mp1_dk"

However, I am also glad with what you proposed, in particular in the beginning where not many boards will need a custom transform.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

putting source files in <app>/boards is not part of the zephyr guidelines, that is for board dtc and Kconfig fragment overlays only

* Copyright 2025 NXP
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <zephyr/device.h>
#include <zephyr/drivers/video.h>
#include <zephyr/drivers/video-controls.h>

#include <zephyr/logging/log.h>

LOG_MODULE_REGISTER(transform, CONFIG_LOG_DEFAULT_LEVEL);

static const struct device *transform_dev;
static struct video_buffer *transformed_buffer;

int video_transform_setup(struct video_format in_fmt, struct video_format *out_fmt)
{
int ret;
struct video_control ctrl = {.id = VIDEO_CID_ROTATE, .val = 90};

transform_dev = DEVICE_DT_GET(DT_NODELABEL(pxp));

out_fmt->pixelformat = in_fmt.pixelformat;
out_fmt->width = in_fmt.height;
out_fmt->height = in_fmt.width;
out_fmt->type = VIDEO_BUF_TYPE_OUTPUT;

/* Set input and output formats */
in_fmt.type = VIDEO_BUF_TYPE_INPUT;
ret = video_set_format(transform_dev, &in_fmt);
if (ret) {
LOG_ERR("Unable to set input format");
return ret;
}

ret = video_set_format(transform_dev, out_fmt);
if (ret) {
LOG_ERR("Unable to set output format");
return ret;
}

/* Set controls */
video_set_ctrl(transform_dev, &ctrl);

/* Allocate a buffer for output queue */
transformed_buffer = video_buffer_aligned_alloc(in_fmt.pitch * in_fmt.height,
CONFIG_VIDEO_BUFFER_POOL_ALIGN, K_FOREVER);

if (transformed_buffer == NULL) {
LOG_ERR("Unable to alloc video buffer");
return -ENOMEM;
}

return 0;
}

int video_transform_process(struct video_buffer *in_buf, struct video_buffer **out_buf)
{
int err;
struct video_buffer *transformed_buf = &(struct video_buffer){};
/* Store the input buffer type to recover later */
enum video_buf_type in_buf_type = in_buf->type;

in_buf->type = VIDEO_BUF_TYPE_INPUT;
err = video_enqueue(transform_dev, in_buf);
if (err) {
LOG_ERR("Unable to enqueue input buffer");
return err;
}

transformed_buffer->type = VIDEO_BUF_TYPE_OUTPUT;
err = video_enqueue(transform_dev, transformed_buffer);
if (err) {
LOG_ERR("Unable to enqueue output buffer");
return err;
}

/* Same start for both input and output */
if (video_stream_start(transform_dev, VIDEO_BUF_TYPE_INPUT)) {
LOG_ERR("Unable to start PxP");
return 0;
}

/* Dequeue input and output buffers */
err = video_dequeue(transform_dev, &in_buf, K_FOREVER);
if (err) {
LOG_ERR("Unable to dequeue PxP video buf in");
return err;
}

transformed_buf->type = VIDEO_BUF_TYPE_OUTPUT;
err = video_dequeue(transform_dev, &transformed_buf, K_FOREVER);
if (err) {
LOG_ERR("Unable to dequeue PxP video buf out");
return err;
}

*out_buf = transformed_buf;

/* Keep the input buffer intact */
in_buf->type = in_buf_type;

return 0;
}
4 changes: 3 additions & 1 deletion samples/drivers/video/capture/boards/mimxrt1170_evk_mimxrt1176_cm7.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
CONFIG_VIDEO_BUFFER_POOL_SZ_MAX=3686800
CONFIG_VIDEO_TRANSFORM=y
CONFIG_VIDEO_BUFFER_POOL_SZ_MAX=6530200
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a mistake. Should keep the same buffer size as before

CONFIG_VIDEO_BUFFER_POOL_NUM_MAX=3
47 changes: 39 additions & 8 deletions samples/drivers/video/capture/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,22 @@ LOG_MODULE_REGISTER(main, CONFIG_LOG_DEFAULT_LEVEL);
#error No camera chosen in devicetree. Missing "--shield" or "--snippet video-sw-generator" flag?
#endif

#define NUM_BUFS 2

int __weak video_transform_setup(struct video_format in_fmt, struct video_format *out_fmt)
{
*out_fmt = in_fmt;

return 0;
}

int __weak video_transform_process(struct video_buffer *in_buf, struct video_buffer **out_buf)
{
*out_buf = in_buf;

return 0;
}

#if DT_HAS_CHOSEN(zephyr_display)
static inline int display_setup(const struct device *const display_dev, const uint32_t pixfmt)
{
Expand Down Expand Up @@ -91,10 +107,11 @@ static inline void video_display_frame(const struct device *const display_dev,

int main(void)
{
struct video_buffer *buffers[CONFIG_VIDEO_BUFFER_POOL_NUM_MAX];
struct video_buffer *vbuf = &(struct video_buffer){};
struct video_buffer *transformed_buf = &(struct video_buffer){};
const struct device *video_dev;
struct video_format fmt;
struct video_format transformed_fmt;
struct video_caps caps;
struct video_frmival frmival;
struct video_frmival_enum fie;
Expand Down Expand Up @@ -262,6 +279,13 @@ int main(void)
tp_set_ret = video_set_ctrl(video_dev, &ctrl);
}

/* Set up transformation */
err = video_transform_setup(fmt, &transformed_fmt);
if (err) {
LOG_ERR("Unable to set up transformation");
return 0;
}

#if DT_HAS_CHOSEN(zephyr_display)
const struct device *const display_dev = DEVICE_DT_GET(DT_CHOSEN(zephyr_display));

Expand All @@ -270,7 +294,7 @@ int main(void)
return 0;
}

err = display_setup(display_dev, fmt.pixelformat);
err = display_setup(display_dev, transformed_fmt.pixelformat);
if (err) {
LOG_ERR("Unable to set up display");
return err;
Expand All @@ -285,19 +309,19 @@ int main(void)
}

/* Alloc video buffers and enqueue for capture */
for (i = 0; i < ARRAY_SIZE(buffers); i++) {
for (i = 0; i < NUM_BUFS; i++) {
/*
* For some hardwares, such as the PxP used on i.MX RT1170 to do image rotation,
* buffer alignment is needed in order to achieve the best performance
*/
buffers[i] = video_buffer_aligned_alloc(bsize, CONFIG_VIDEO_BUFFER_POOL_ALIGN,
vbuf = video_buffer_aligned_alloc(bsize, CONFIG_VIDEO_BUFFER_POOL_ALIGN,
K_FOREVER);
if (buffers[i] == NULL) {
if (vbuf == NULL) {
LOG_ERR("Unable to alloc video buffer");
return 0;
}
buffers[i]->type = type;
video_enqueue(video_dev, buffers[i]);
vbuf->type = type;
video_enqueue(video_dev, vbuf);
}

/* Start video capture */
Expand Down Expand Up @@ -328,8 +352,15 @@ int main(void)
}
#endif

/* Do transformation */
err = video_transform_process(vbuf, &transformed_buf);
if (err) {
LOG_ERR("Unable to transform video frame");
return 0;
}

#if DT_HAS_CHOSEN(zephyr_display)
video_display_frame(display_dev, vbuf, fmt);
video_display_frame(display_dev, transformed_buf, transformed_fmt);
#endif

err = video_enqueue(video_dev, vbuf);
Expand Down