Skip to content

video: drivers misc fixes #87366

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

Merged
merged 13 commits into from
May 20, 2025

Conversation

ngphibang
Copy link
Collaborator

@ngphibang ngphibang commented Mar 19, 2025

  • Some misc fixes in individual video drivers
  • Moving the format pitch (bytesperline) which relating to image size from application into bridge drivers. The reason for that is the bridge (e.g., DMA, ISP) drivers actually handle the memory and know exactly the memory layout constraints (need alignment, padding, etc. or not). Application does not know this, it justs set the desired pixel format and resolution and must always read back this field to see what the driver actually sets. Also, drop format pitch setting in sensor drivers as this is not needed (and the sensor does not know this information neither). Some examples:

https://elixir.bootlin.com/linux/v6.11.1/source/drivers/media/platform/qcom/venus/vdec.c#L231
https://elixir.bootlin.com/linux/v6.11.1/source/drivers/media/platform/ti/omap3isp/ispvideo.c#L143
https://elixir.bootlin.com/linux/v6.11.1/source/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c#L404

  • Decoupling get / set_fmt() in some bridge drivers

@ngphibang
Copy link
Collaborator Author

@josuah : The video emul imager exposes 3 controls : VIDEO_CID_EXPOSURE, VIDEO_CID_GAIN and VIDEO_CID_TEST_PATTERN. However, it does nothing specific to each control type, just a "fake" register write (?). I know this is just for testing purpose but even if this is just an emulation, we need to emul something "real". If not, it will be very difficult to rework the video control framework as each control type needs a specific implementation. I am going to remove these controls in the driver and just keep 1 control of type "vendor-specific" as an emulated read/write. Is it ok for you to do that ?

@josuah
Copy link
Collaborator

josuah commented Mar 19, 2025

I added that commit here: db5abeb

Also available as a stand-alone commit on top of this nxp branch: tinyvision-ai-inc@e76b19d (git format-patch -1 attached) 0001-drivers-video-emul_imager-use-a-PRIV-video-CID.patch.txt

Whichever is most convenient!

Tested with: west twister -i -T tests/drivers/video/api/

josuah
josuah previously approved these changes Mar 19, 2025
@ngphibang ngphibang added the DNM This PR should not be merged (Do Not Merge) label Mar 20, 2025
@ngphibang ngphibang force-pushed the video_driver_misc_fixes branch from 00869da to 1b501e5 Compare March 21, 2025 10:31
@ngphibang
Copy link
Collaborator Author

ngphibang commented May 16, 2025

Seems to me there is again a rebase with updates of the code at the same time

Oh, sorry ... I did it as per usual to always test the PR on the latest main (forgot to push the rebase) to see if there is a problem and pushed it unconsciously. Will pay attention. Sorry again !

{SDE_CTRL2_REG, abs(sin_coef) & 0xFF}};

ret = ov5640_modify_reg(&cfg->i2c, SDE_CTRL8_REG, 0x7F, sign);
if (ret) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Recently we had a discussion with @josuah in which we discussed about whether or not we should enforce MISRA style. Normally this is apparently a Zephyr requirement but is not enforced so hard to decide actually. In doubt I update by ST-MIPID02 / GC2145 PR few days ago for all the code I modified (I didn't modify the whole driver when the driver already existed) to if in such similar situation if (ret < 0). I tend to actually not being a big fan of it but that was more in preparation of this style being enforced.
I honestly don't really know if we should enforce it now or not. An opinion ?

Copy link
Collaborator Author

@ngphibang ngphibang May 18, 2025

Choose a reason for hiding this comment

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

Recently we had a discussion with @josuah in which we discussed about whether or not we should enforce MISRA style.

To me, it's the same. Without MISRA rule, it's a bit shorten. Personally, I am not fan of it as well, e.g. when it comes to if else the style is a bit ugly to me:

https://github.com/zephyrproject-rtos/hal_nxp/blob/a961ce3ba3fa3a4f5c6d84918b1d2262197f46d3/mcux/mcux-sdk-ng/drivers/csi/fsl_csi.c#L864-#L879

But well, if it's enforced by Zephyr one day, we need to follow. Currently, to put it simple, I will change this in the next push ... personnaly, I tend to avoid any debate about things which are less technical :-).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pdgendt mentioned:

It's a required rule, but it's not enforced (yet) in CI

There is a doc page that tells the roadmap of for different level of coding style enforcement (including MISRA, and listing of areas) but I cannot find it again. 😅

Copy link
Collaborator

@avolmat-st avolmat-st left a comment

Choose a reason for hiding this comment

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

I am ok with the PR, with only the point about MISRA or not ;) just have to be decided maybe. Also the PR will have to be rebased since now the endpoint removal as been merged as far as I know.

ngphibang and others added 5 commits May 18, 2025 08:24
The MIPI CSI-2 Rx needs to be initialized after the camera sensor which
is generally initialized with CONFIG_VIDEO_INIT_PRIORITY.

This is currently true "by chance" due to the order the linker links the
object files. This linker order is not easily controlled, so use an
explicit priority value to ensure this requirement.

Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Fix some type range related to pixel rate which can cause overflow.

Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Fix coding style in a variable naming.

Signed-off-by: Farah Fliss <farah.fliss@nxp.com>
Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
The mt9m114 camera driver used to be single-instance.
Improve it to multi-instance.

Signed-off-by: Farah Fliss <farah.fliss@nxp.com>
Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Update the control value directly. No need for an internal variable.

Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
@ngphibang
Copy link
Collaborator Author

ngphibang commented May 18, 2025

@avolmat-st : To make sure I will push the branch correctly this time:

  • I rebased the branch on top of the lastest main and resolved the conflict when rebasing, then I push, right ? There are still many unrelated changes coming from main together with the conflict resolving though, is it ok like that ?

@ngphibang ngphibang force-pushed the video_driver_misc_fixes branch from f4ffe98 to c42c5f0 Compare May 18, 2025 12:28
@ngphibang
Copy link
Collaborator Author

Simply rebased and fixed conflict because #87070 was merged

trunghieulenxp and others added 8 commits May 18, 2025 14:41
SDE_CTRL8_REG's value must be modified using modify_register.

Signed-off-by: Trung Hieu Le <trunghieu.le@nxp.com>
Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Fix the sign register for brightness control

Signed-off-by: Trung Hieu Le <trunghieu.le@nxp.com>
Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Fix sign's register for constrast value.

Signed-off-by: Trung Hieu Le <trunghieu.le@nxp.com>
Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Compute bits per pixel according to the pixel format instead of
hardcoding it.

Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Drop video_stm32_dcmi_is_fmt_valid() as it is not needed. In this
function, (i) checking against a format based on another utility
function video_bits_per_pixel() is not robust, this check is done in
the sensor driver, (ii) checking against the heap size is not
appropriate because this should be done when allocating buffers, not
in get/set format.

Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
The format pitch (bytesperline) field is typically set by the bridge
drivers, i.e. DMA, ISP drivers who actually handle the memory as they
know exactly the memory layout constraints.

Application just set the pixel format and resolution and must always
read back this field to see what the driver actually sets (to allocate
buffers for example).

Also, drop format pitch setting in sensor drivers as this is not needed.

Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Do not set_format() when doing get_format(). This design seems initially
to simplify the sample (just get_format() and everything works out of the
box) but it makes thing incomprehensive and error prone.

Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Do not set_format() when doing get_format(). This design seems initially
to simplify the sample (just get_format() and everything works out of the
box) but it makes thing incomprehensive and error prone.

Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
@ngphibang ngphibang force-pushed the video_driver_misc_fixes branch from c42c5f0 to 1159950 Compare May 18, 2025 12:50
@ngphibang
Copy link
Collaborator Author

Forced push:

  • Replaced if (ret) by if (ret < 0) to be compliant with MISRA rule.

Copy link

@avolmat-st
Copy link
Collaborator

@avolmat-st : To make sure I will push the branch correctly this time:

  • I rebased the branch on top of the lastest main and resolved the conflict when rebasing, then I push, right ? There are still many unrelated changes coming from main together with the conflict resolving though, is it ok like that ?

Sorry for the late reply. Yes thanks a lot !!

@@ -67,7 +67,9 @@ static int emul_rx_set_fmt(const struct device *const dev, struct video_format *
}

/* Cache the format selected locally to use it for getting the size of the buffer */
fmt->pitch = fmt->width * video_bits_per_pixel(fmt->pixelformat) / BITS_PER_BYTE;
Copy link
Collaborator

@josuah josuah May 18, 2025

Choose a reason for hiding this comment

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

This is a small semantic change and I proposed a migration-guide entry here to avoid getting in the way of this PR:

This is not a breaking change, and a migration guide entry is only a hint for how to simplify applications after 4.2.

@erwango
Copy link
Member

erwango commented May 20, 2025

@mmahadevan108, @dleach02 PTL

@kartben kartben merged commit 075ee09 into zephyrproject-rtos:main May 20, 2025
26 checks passed
@ngphibang ngphibang deleted the video_driver_misc_fixes branch June 24, 2025 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Samples Samples area: Video Video subsystem platform: ESP32 Espressif ESP32 platform: NXP Drivers NXP Semiconductors, drivers platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants