video: gc2145/stm32_dcmi support Crop and basic DMA recovery #97491

KurtE · 2025-10-13T19:05:28Z

Added support to video_stm32_dcmi for the new
set and get selection. These implementations simply forward the message to the underlying
camera object if they support these messages.

GC2145 was updated first to support setting a crop window, using the order
try out these changes. The camera now allows me to follow the call order
of calls that @josuah mentioned in another pr/issue.

With this driver I also updated it to allow more or less any video resolution:
{
.pixelformat = format, .width_min = width_l, .width_max = width_h,
.height_min = height_l, .height_max = height_h, .width_step = 0, .height_step = 0,
}

static const struct video_format_cap fmts[] = {
GC2145_VIDEO_FORMAT_CAP_HL(128, 1600, 128, 1200, VIDEO_PIX_FMT_RGB565),
GC2145_VIDEO_FORMAT_CAP_HL(128, 1600, 128, 1200, VIDEO_PIX_FMT_YUYV),
When resolution is set, it computes the scale factor. If you then later call set_crop, the same code is
used except it uses the ratios computed from the set_resolution.

In addition, I pulled in the minimal DCMI changes, that allow the camera code to recover after some DMA
errors.

This is a partial replacement for #93797 It does not support the SNAPSHOT mode that is part of that PR, but decided
it might be simpler and easier to understand if this was done instead in smaller steps.

avolmat-st

Thanks @KurtE for this PR. This is roughly good to me, some comments, small things to cleanup but nothing really big.
Only the last comment about the dcmi_dma_callback needs to be checked more. I recall seeing a potential issue before in the dma_stm32 code so I check this in parallel of your update of this PR.

drivers/video/gc2145.c

drivers/video/video_stm32_dcmi.c

JarmouniA

Reword 1st commit message to remove reference to video_stm32_dcmi, the cam driver could be used with any cam controller.
Reword 3rd commit message to remove the unnecessary talk about the other PR, and to have a message that actually describes the change and why it's needed.

KurtE · 2025-10-14T14:10:12Z

Twister failings... Is this another case where I should rebase again?

JarmouniA · 2025-10-14T17:29:34Z

Twister failings... Is this another case where I should rebase again?

You can see what is failing & why here
https://github.com/zephyrproject-rtos/zephyr/actions/runs/18497561009/job/52706085437?pr=97491
https://github.com/zephyrproject-rtos/zephyr/actions/runs/18497561009/job/52706085480?pr=97491
https://github.com/zephyrproject-rtos/zephyr/actions/runs/18497561009/job/52706085461?pr=97491

drivers/video/video_stm32_dcmi.c

avolmat-st · 2025-10-16T20:16:08Z

Hi @KurtE, sorry for the delay. I checked the latest update and also gave it a try on my STM32H747I-DISCO. While I am not able to see the exact same DMA error symptoms as you, I managed to see several points:

I think (to be rechecked and confirmed) that there is an issue in the dma_stm32.c code, in that transfer errors are not being transfered to the HAL_DCMI code. In case of DCMI, the DMA is fully managed by the HAL_DCMI so the Zephyr DMA driver has actually very few things to do apart from marking the channel as busy and properly forwarding the irq handle to the HAL). This is done in most of the cases, but for some reason, in case of TE (transfer errors), it wasn't.
I fixed that in the PR drivers: dma: stm32: avoid clear TE in case of hal_override #97741.
With this done, I can now force DMA errors (for that purpose, I made the driver go write to an area it cannot, hence leading to transfer errors) and can see that they go 'til the DCMI HAL driver.
With above modification in place, while the dma_stm32.c will call the dcmi_dma_callback function with an error status, this is not useful at anything since anyway the HAL_DCMI is doing the whole DMA handling itself via the HAL_DMA driver.
So, within the dcmi_dma_callback function, the following check is useless and should be removed:

        if (status < 0) {
                LOG_ERR("DMA callback error with channel %d.", channel);
        }

(since channel / status are no more used, it is necessary to have ARG_UNUSED for those two).

Could you check again on your side with PR #97741 applied and if ok, perform the above modifications as well in your PR ?

In my experiment, since I force errors (either via DMA transfer error or DCMI overrun), there were lot of DCMI Restart happening and the system was becoming barely usable (very very slow) but, I guess in a normal situation (where those errors only come from time to time) that would be ok.

KurtE · 2025-10-16T22:11:22Z

I manually applied your change and helps a lot.

I removed the if and the print and unused args change.

And now it recovers when I am capturing 480x320 images and displaying them on an ST7796 display of 480x320.
Note sometime the recovery is such that I go through periods where the image is torn...
Like:

But most of the time, after a bit, it will resync back to one full image. That is one of the nice things of Snapshot mode
where it always starts at a beginning of a frame. But I am happy with the progress.

avolmat-st · 2025-10-17T11:16:31Z

Thanks @KurtE for the check. Good news.

Regarding the distorded image, is it fixed like that or it is like you can see the DMA actually writing into the memory and the image slowly coming up from the top fo the bottom ?
Getting a fixed image like this shows that DMA is "desynchronized". It can happen for example if there has been an overrun of the DCMI, the the DMA has NOT been stopped, and this on the next frame the DCMI will start sending again the data to the DMA, leading to DMA transfer doing partial copy of frame N and partial of frame N+1.

At first I kind of thought that the patch I provided "helped" to get things a bit better but now I am not sure that is really the case, could you confirm ? This splitted frame is not something you had before ? What is better before my patch ?

Regarding your PR, except this point to be clarified (but considering that this is error handling and if things are better than before, we might be ok to first go with this first improvement), I am ok with your code so ok to Approve it.

If you can, could you just change the last patch commit message ? I think the part talking about PR is not needed and no need to have the update as well at the end. Once merged, those commits are no more related to PR so it doesn't make sense to refer to PRs.

KurtE · 2025-10-17T14:02:52Z

Thanks, the image does keep changing, so it is not hung and as I mentioned often times it will resync, although sometimes
takes awhile. This is not a new phenomena, seen this in the past, where for example maybe too many debug messages
in the callback, causes a delay before we continue the HAL: HAL_DCMI_Resume(hdcmi);

I think we still show two sometimes three messages pre recovery:

[00:00:36.259,000] <err> dma_stm32_v1: FiFo error.
[00:00:36.267,000] <inf> dma_stm32_v1: tc: 0, ht: 1, te: 0, dme: 0, fe: 1
[00:00:36.276,000] <err> dma_stm32: Unexpected irq happened.

Will update the commit message
EDIT: Done

josuah

Two minor comments, you can update them if you think this is worth it, but not critical.

Thanks a lot for all the modifications and bringing the first sensor with video_set_selection() support!

drivers/video/gc2145.c

josuah · 2025-10-17T23:12:19Z

Adding a DNM flag to let the author of the PR decide if they want another push, or if the PR is ready to go.

If the latter, then simply say so and we can remove the DNM flag, otherwise feel free to make the edits.

Thanks all for the help with the review!

avolmat-st · 2025-10-18T11:27:40Z

@KurtE , for information I also added a new commit in the PR #97741 in order to let the HAL code handle the Fifo error.

Implements the set_selection and get_selection APIs, if forwarded to it by a camera controller. It uses the new messages to allow you to set a crop window on top of the current format window. It also then allows you to move this crop window around in the frame window. With this driver I also updated it to allow any resolution from the displays min to max limits. static const struct video_format_cap fmts[] = { GC2145_VIDEO_FORMAT_CAP_HL(128, 1600, 128, 1200, VIDEO_PIX_FMT_RGB565), GC2145_VIDEO_FORMAT_CAP_HL(128, 1600, 128, 1200, VIDEO_PIX_FMT_YUYV), When the resolution is set, it computes the scale factor. Using the set_selection(VIDEO_SEL_TGT_CROP) allows you define a crop window within the format window. It clamps the ratio to a max of 3 as some other drivers limit it saying it helps with frame rates. Signed-off-by: Kurt Eckhardt <[email protected]>

Forward the get_selection and set_selection APIs to the camera objects, to allow some of the selections to be supported at the camera level. Signed-off-by: Kurt Eckhardt <[email protected]>

On several boards, such as the Arduino Giga and Portenta H7, they are often times setup with their camera buffers and potentially video buffers in SDRam. This can lead to a significant number of DMA errors, which currently stops the camera from returning any additional frames. Signed-off-by: Kurt Eckhardt <[email protected]>

KurtE · 2025-10-18T13:19:17Z

@josuah - I pushed up the two changes.

sonarqubecloud · 2025-10-18T13:45:24Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

zephyrbot added area: Video Video subsystem platform: STM32 ST Micro STM32 labels Oct 13, 2025

zephyrbot requested review from FRASTM, GeorgeCGV, avolmat-st, djiatsaf-st, erwango, etienne-lms, gautierg-st, josuah, loicpoulain, marwaiehm-st, mathieuchopstm and ngphibang October 13, 2025 19:07

zephyrbot assigned josuah and ngphibang Oct 13, 2025

avolmat-st requested changes Oct 13, 2025

View reviewed changes

JarmouniA requested changes Oct 13, 2025

View reviewed changes

KurtE force-pushed the camera_snapshot_minus_snap branch from 4e37920 to e21d91f Compare October 14, 2025 13:08

JarmouniA previously approved these changes Oct 14, 2025

View reviewed changes

JarmouniA reviewed Oct 14, 2025

View reviewed changes

drivers/video/video_stm32_dcmi.c Outdated Show resolved Hide resolved

KurtE dismissed JarmouniA’s stale review via d3244ca October 14, 2025 18:21

KurtE force-pushed the camera_snapshot_minus_snap branch from e21d91f to d3244ca Compare October 14, 2025 18:21

JarmouniA previously approved these changes Oct 14, 2025

View reviewed changes

JarmouniA added this to the v4.3.0 milestone Oct 14, 2025

KurtE requested a review from avolmat-st October 14, 2025 21:30

KurtE dismissed JarmouniA’s stale review via fd62451 October 16, 2025 22:03

KurtE force-pushed the camera_snapshot_minus_snap branch from d3244ca to fd62451 Compare October 16, 2025 22:03

KurtE force-pushed the camera_snapshot_minus_snap branch from fd62451 to 55fc348 Compare October 17, 2025 14:07

avolmat-st mentioned this pull request Oct 17, 2025

drivers: dma: stm32: avoid clear TE in case of hal_override #97741

Open

avolmat-st previously approved these changes Oct 17, 2025

View reviewed changes

JarmouniA previously approved these changes Oct 17, 2025

View reviewed changes

josuah previously approved these changes Oct 17, 2025

View reviewed changes

drivers/video/gc2145.c Outdated Show resolved Hide resolved

drivers/video/gc2145.c Outdated Show resolved Hide resolved

drivers/video/gc2145.c Show resolved Hide resolved

josuah added the DNM This PR should not be merged (Do Not Merge) label Oct 17, 2025

KurtE added 3 commits October 18, 2025 06:17

video: stm32_dcmi: forward selection

6cb14ac

Forward the get_selection and set_selection APIs to the camera objects, to allow some of the selections to be supported at the camera level. Signed-off-by: Kurt Eckhardt <[email protected]>

KurtE dismissed stale reviews from josuah, JarmouniA, and avolmat-st via faba9a3 October 18, 2025 13:17

KurtE force-pushed the camera_snapshot_minus_snap branch from 55fc348 to faba9a3 Compare October 18, 2025 13:17

avolmat-st approved these changes Oct 18, 2025

View reviewed changes

JarmouniA removed the DNM This PR should not be merged (Do Not Merge) label Oct 18, 2025

JarmouniA approved these changes Oct 18, 2025

View reviewed changes

josuah approved these changes Oct 19, 2025

View reviewed changes

video: gc2145/stm32_dcmi support Crop and basic DMA recovery #97491

Are you sure you want to change the base?

video: gc2145/stm32_dcmi support Crop and basic DMA recovery #97491

Conversation

KurtE commented Oct 13, 2025

Uh oh!

avolmat-st left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JarmouniA left a comment

Choose a reason for hiding this comment

Uh oh!

KurtE commented Oct 14, 2025

Uh oh!

JarmouniA commented Oct 14, 2025

Uh oh!

Uh oh!

avolmat-st commented Oct 16, 2025

Uh oh!

KurtE commented Oct 16, 2025

Uh oh!

avolmat-st commented Oct 17, 2025

Uh oh!

KurtE commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josuah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josuah commented Oct 17, 2025

Uh oh!

avolmat-st commented Oct 18, 2025

Uh oh!

KurtE commented Oct 18, 2025

Uh oh!

sonarqubecloud bot commented Oct 18, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

KurtE commented Oct 17, 2025 •

edited

Loading