Skip to content

Allow STM32 DCMIPP to work with UVC sample #94562

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

Merged
merged 6 commits into from
Oct 23, 2025
+199 −48
Merged

Allow STM32 DCMIPP to work with UVC sample #94562

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dfe5139
video: dcmipp: expose dcmipp caps for all 3 pipes.
Aug 14, 2025
ee17acf
video: introduce video_set_compose_format helper
Aug 15, 2025
49458fc
samples: video: capture: use video_set_compose_format helper
Aug 15, 2025
fe96014
samples: usb: uvc: ensure video buffer is properly aligned
Aug 15, 2025
a5971e1
samples: usb: uvc: use video_set_compose_format
Aug 15, 2025
d2e3f66
samples: usb: uvc: select applicable resolutions from range
Aug 15, 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
21 changes: 21 additions & 0 deletions drivers/video/video_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,3 +468,24 @@ int video_estimate_fmt_size(struct video_format *fmt)

return 0;
}

int video_set_compose_format(const struct device *dev, struct video_format *fmt)
{
struct video_selection sel = {
.type = fmt->type,
.target = VIDEO_SEL_TGT_COMPOSE,
.rect.left = 0,
.rect.top = 0,
.rect.width = fmt->width,
.rect.height = fmt->height,
};
int ret;

ret = video_set_selection(dev, &sel);
if (ret < 0 && ret != -ENOSYS) {
LOG_ERR("Unable to set selection compose");
return ret;
}

return video_set_format(dev, fmt);
}
90 changes: 81 additions & 9 deletions drivers/video/video_stm32_dcmipp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,22 +1331,94 @@ static int stm32_dcmipp_dequeue(const struct device *dev, struct video_buffer **
}

/*
* TODO: caps aren't yet handled hence give back straight the caps given by the
* source. Normally this should be the intersection of what the source produces
* vs what the DCMIPP can input (for pipe0) and, for pipe 1 and 2, for a given
* input format, generate caps based on capabilities, color conversion, decimation
* etc
* For MAIN / AUX pipe, it is necessary that the pitch is a multiple of 16 bytes.
* Give here the multiple in number of pixels, which depends on the format chosen
*/
#define DCMIPP_CEIL_DIV_ROUND_UP_MUL(val, div, mul) \
((((val) + (div) - 1) / (div) + (mul) - 1) / (mul) * (mul))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we add a new line after each #define for better visibility ?


#define DCMIPP_CEIL_DIV(val, div) \
(((val) + (div) - 1) / (div))

#define DCMIPP_VIDEO_FORMAT_CAP(format, pixmul) { \
.pixelformat = VIDEO_PIX_FMT_##format, \
.width_min = DCMIPP_CEIL_DIV_ROUND_UP_MUL(CONFIG_VIDEO_STM32_DCMIPP_SENSOR_WIDTH, \
STM32_DCMIPP_MAX_PIPE_SCALE_FACTOR, \
pixmul), \
.width_max = CONFIG_VIDEO_STM32_DCMIPP_SENSOR_WIDTH / (pixmul) * (pixmul), \
.height_min = DCMIPP_CEIL_DIV(CONFIG_VIDEO_STM32_DCMIPP_SENSOR_HEIGHT, \
STM32_DCMIPP_MAX_PIPE_SCALE_FACTOR), \
.height_max = CONFIG_VIDEO_STM32_DCMIPP_SENSOR_HEIGHT, \
.width_step = pixmul, .height_step = 1, \
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This closing brace should be indented by a tab IMHO, since it's part of a macro.


static const struct video_format_cap stm32_dcmipp_dump_fmt[] = {
{
.pixelformat = VIDEO_FOURCC_FROM_STR(CONFIG_VIDEO_STM32_DCMIPP_SENSOR_PIXEL_FORMAT),
.width_min = CONFIG_VIDEO_STM32_DCMIPP_SENSOR_WIDTH,
.width_max = CONFIG_VIDEO_STM32_DCMIPP_SENSOR_WIDTH,
.height_min = CONFIG_VIDEO_STM32_DCMIPP_SENSOR_HEIGHT,
.height_max = CONFIG_VIDEO_STM32_DCMIPP_SENSOR_HEIGHT,
.width_step = 1, .height_step = 1,
},
{0},
};

static const struct video_format_cap stm32_dcmipp_main_fmts[] = {
DCMIPP_VIDEO_FORMAT_CAP(RGB565, 8),
DCMIPP_VIDEO_FORMAT_CAP(YUYV, 8),
DCMIPP_VIDEO_FORMAT_CAP(YVYU, 8),
DCMIPP_VIDEO_FORMAT_CAP(GREY, 16),
DCMIPP_VIDEO_FORMAT_CAP(RGB24, 16),
DCMIPP_VIDEO_FORMAT_CAP(BGR24, 16),
DCMIPP_VIDEO_FORMAT_CAP(ARGB32, 4),
DCMIPP_VIDEO_FORMAT_CAP(ABGR32, 4),
DCMIPP_VIDEO_FORMAT_CAP(RGBA32, 4),
DCMIPP_VIDEO_FORMAT_CAP(BGRA32, 4),
DCMIPP_VIDEO_FORMAT_CAP(NV12, 16),
DCMIPP_VIDEO_FORMAT_CAP(NV21, 16),
DCMIPP_VIDEO_FORMAT_CAP(NV16, 16),
DCMIPP_VIDEO_FORMAT_CAP(NV61, 16),
DCMIPP_VIDEO_FORMAT_CAP(YUV420, 16),
DCMIPP_VIDEO_FORMAT_CAP(YVU420, 16),
{0},
};
Comment on lines +1367 to +1385
Copy link
Contributor

Choose a reason for hiding this comment

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

The stm32_dcmipp_aux_fmts is a subset of this. Can we declare stm32_dcmipp_aux_fmts first and include it inside this to save some memory, something like:

static const struct video_format_cap *stm32_dcmipp_main_fmts[] = {
    &stm32_dcmipp_aux_fmts[0],  /* RGB565 */
    ...
    &stm32_dcmipp_aux_fmts[9],  /* BGRA32 */
   DCMIPP_VIDEO_FORMAT_CAP(NV21, 16),
  ...
};

Copy link
Author

Choose a reason for hiding this comment

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

This seems to me quite error prone. We would be mixing up static const struct video_format_cap table of structure for the aux and table of pointers for the main. Moreover if for some reason we add / delete on aux then we need to ensure it is ok as well on the main. So I feel like it is better to stick to this version which might indeed consume a bit more code space.

Copy link
Contributor

@josuah josuah Oct 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This can all be optimized as soon as there is a video_enum_pixfmt() + video_enum_frmsize() API? :D


static const struct video_format_cap stm32_dcmipp_aux_fmts[] = {
DCMIPP_VIDEO_FORMAT_CAP(RGB565, 8),
DCMIPP_VIDEO_FORMAT_CAP(YUYV, 8),
DCMIPP_VIDEO_FORMAT_CAP(YVYU, 8),
DCMIPP_VIDEO_FORMAT_CAP(GREY, 16),
DCMIPP_VIDEO_FORMAT_CAP(RGB24, 16),
DCMIPP_VIDEO_FORMAT_CAP(BGR24, 16),
DCMIPP_VIDEO_FORMAT_CAP(ARGB32, 4),
DCMIPP_VIDEO_FORMAT_CAP(ABGR32, 4),
DCMIPP_VIDEO_FORMAT_CAP(RGBA32, 4),
DCMIPP_VIDEO_FORMAT_CAP(BGRA32, 4),
{0},
};

static int stm32_dcmipp_get_caps(const struct device *dev, struct video_caps *caps)
{
const struct stm32_dcmipp_config *config = dev->config;
int ret;
struct stm32_dcmipp_pipe_data *pipe = dev->data;

ret = video_get_caps(config->source_dev, caps);
switch (pipe->id) {
case DCMIPP_PIPE0:
caps->format_caps = stm32_dcmipp_dump_fmt;
break;
case DCMIPP_PIPE1:
caps->format_caps = stm32_dcmipp_main_fmts;
break;
case DCMIPP_PIPE2:
caps->format_caps = stm32_dcmipp_aux_fmts;
break;
default:
CODE_UNREACHABLE;
}

caps->min_vbuf_count = 1;

return ret;
return 0;
}

static int stm32_dcmipp_get_frmival(const struct device *dev, struct video_frmival *frmival)
Expand Down
21 changes: 21 additions & 0 deletions include/zephyr/drivers/video.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,6 +982,27 @@ int64_t video_get_csi_link_freq(const struct device *dev, uint8_t bpp, uint8_t l
*/
int video_estimate_fmt_size(struct video_format *fmt);

/**
* @brief Set compose rectangle (if applicable) prior to setting format
*
* Some devices expose compose capabilities, allowing them to apply a transformation
* (downscale / upscale) to the frame. For those devices, it is necessary to set the
* compose rectangle before being able to apply the frame format (which must have the
* same width / height as the compose rectangle width / height).
* In order to allow non-compose aware application to be able to control such devices,
* introduce a helper which, if available, will apply the compose rectangle prior to
* setting the format.
*
* @param dev Pointer to the video device struct to set format
* @param fmt Pointer to a video format struct.
*
* @retval 0 Is successful.
* @retval -EINVAL If parameters are invalid.
* @retval -ENOTSUP If format is not supported.
* @retval -EIO General input / output error.
*/
int video_set_compose_format(const struct device *dev, struct video_format *fmt);

/**
* @defgroup video_pixel_formats Video pixel formats
* The '|' characters separate the pixels or logical blocks, and spaces separate the bytes.
Expand Down
45 changes: 9 additions & 36 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 @@ -98,10 +98,14 @@ int main(void)
struct video_frmival frmival;
struct video_frmival_enum fie;
enum video_buf_type type = VIDEO_BUF_TYPE_OUTPUT;
#if (CONFIG_VIDEO_SOURCE_CROP_WIDTH && CONFIG_VIDEO_SOURCE_CROP_HEIGHT) || \
CONFIG_VIDEO_FRAME_HEIGHT || CONFIG_VIDEO_FRAME_WIDTH
struct video_selection sel = {
#if (CONFIG_VIDEO_SOURCE_CROP_WIDTH && CONFIG_VIDEO_SOURCE_CROP_HEIGHT)
struct video_selection crop_sel = {
.type = VIDEO_BUF_TYPE_OUTPUT,
.target = VIDEO_SEL_TGT_CROP;
.rect.left = CONFIG_VIDEO_SOURCE_CROP_LEFT;
.rect.top = CONFIG_VIDEO_SOURCE_CROP_TOP;
.rect.width = CONFIG_VIDEO_SOURCE_CROP_WIDTH;
.rect.height = CONFIG_VIDEO_SOURCE_CROP_HEIGHT;
};
#endif
unsigned int frame = 0;
Expand Down Expand Up @@ -149,20 +153,14 @@ int main(void)

/* Set the crop setting if necessary */
#if CONFIG_VIDEO_SOURCE_CROP_WIDTH && CONFIG_VIDEO_SOURCE_CROP_HEIGHT
sel.target = VIDEO_SEL_TGT_CROP;
sel.rect.left = CONFIG_VIDEO_SOURCE_CROP_LEFT;
sel.rect.top = CONFIG_VIDEO_SOURCE_CROP_TOP;
sel.rect.width = CONFIG_VIDEO_SOURCE_CROP_WIDTH;
sel.rect.height = CONFIG_VIDEO_SOURCE_CROP_HEIGHT;
if (video_set_selection(video_dev, &sel)) {
if (video_set_selection(video_dev, &crop_sel)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer

	if (video_set_selection(video_dev, &crop_sel) < 0) {

Copy link
Contributor

Choose a reason for hiding this comment

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

That is nonsense, even would introduce undefined behavior and -1 from my side.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by an undefined behavior?
If you prefer:

	if (video_set_selection(video_dev, &crop_sel) != 0) {

I'm fine too.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is completely different proposal, at least for USB that would be preferable, but there is no difference between if (video_set_selection(video_dev, &crop_sel) != 0) { and if (video_set_selection(video_dev, &crop_sel)) {.

Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake. I misised we're here in a demo sample.

LOG_ERR("Unable to set selection crop");
return 0;
}
LOG_INF("Selection crop set to (%u,%u)/%ux%u",
sel.rect.left, sel.rect.top, sel.rect.width, sel.rect.height);
#endif

#if CONFIG_VIDEO_FRAME_HEIGHT || CONFIG_VIDEO_FRAME_WIDTH
#if CONFIG_VIDEO_FRAME_HEIGHT
fmt.height = CONFIG_VIDEO_FRAME_HEIGHT;
#endif
Expand All @@ -171,39 +169,14 @@ int main(void)
fmt.width = CONFIG_VIDEO_FRAME_WIDTH;
#endif

/*
* Check (if possible) if targeted size is same as crop
* and if compose is necessary
*/
sel.target = VIDEO_SEL_TGT_CROP;
err = video_get_selection(video_dev, &sel);
if (err < 0 && err != -ENOSYS) {
LOG_ERR("Unable to get selection crop");
return 0;
}

if (err == 0 && (sel.rect.width != fmt.width || sel.rect.height != fmt.height)) {
sel.target = VIDEO_SEL_TGT_COMPOSE;
sel.rect.left = 0;
sel.rect.top = 0;
sel.rect.width = fmt.width;
sel.rect.height = fmt.height;
err = video_set_selection(video_dev, &sel);
if (err < 0 && err != -ENOSYS) {
LOG_ERR("Unable to set selection compose");
return 0;
}
}
#endif

if (strcmp(CONFIG_VIDEO_PIXEL_FORMAT, "")) {
fmt.pixelformat = VIDEO_FOURCC_FROM_STR(CONFIG_VIDEO_PIXEL_FORMAT);
}

LOG_INF("- Video format: %s %ux%u",
VIDEO_FOURCC_TO_STR(fmt.pixelformat), fmt.width, fmt.height);

if (video_set_format(video_dev, &fmt)) {
if (video_set_compose_format(video_dev, &fmt)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto:

	if (video_set_compose_format(video_dev, &fmt) < 0) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as #94562 (comment)

LOG_ERR("Unable to set format");
return 0;
}
Expand Down