-
Notifications
You must be signed in to change notification settings - Fork 8.1k
video: Patch SDMA & capture sample to arbitrate height and width #96986
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
video: Patch SDMA & capture sample to arbitrate height and width #96986
Conversation
85af2ed
to
91be28e
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.
Please split this into 2 commits: driver changes, sample changes.
Also, explain why the driver change is needed in the commit message.
91be28e
to
bafe2fb
Compare
Thank you, this is done. |
bafe2fb
to
aa93037
Compare
aa93037
to
d0069f2
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.
Could you confirm again this sole commit fixes the issue? I don't have HW to test.
Last I checked as stated on discord, no this does not, one needs the samples changes as well. |
I’ll be at my lab in a couple of hours and will share the results. |
(oups, went too fast looking at previous comments, actually mine are close to some already made by @josuah) |
Adding a DNM flag that can be removed as soon as #96986 (comment) is ok |
@ngphibang ok here are the results without the samples commit
with the samples commit
let me know if I should add back the samples commit |
Do you mean the 2 proposals I mentioned above in the driver ? Well if that's easy and you have possibility to do that, why not ;) |
Oh no no, your comments I will add but there is a commit that we are unclear on it looks like below /frdm_mcxn236.conf
index 0f4576e59d1..ad449bf981a 100644
--- a/samples/drivers/video/capture/boards/frdm_mcxn236.conf
+++ b/samples/drivers/video/capture/boards/frdm_mcxn236.conf
@@ -1 +1,3 @@
CONFIG_VIDEO_BUFFER_POOL_SZ_MAX=40000
+CONFIG_VIDEO_FRAME_WIDTH=320
+CONFIG_VIDEO_FRAME_HEIGHT=240 I am positive this is needed but Phi thinks otherwise and I do trust he knows why but I cant seem to make it work without this. |
d0069f2
to
9d964a7
Compare
@avolmat-st @ngphibang @josuah I would appreciate another review round I think I applied all the required changes. |
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.
Although the 2nd commit is not really needed for the capture sample, configuring the frame width & height is still OK to ensure that if, for some reason, the sample application does not get the format before setting the format, it's still working.
BTW, could you change the commits title, to something like :
drivers: video: mcux_sdma: Reconfigure the source when getting format
Reconfigure the sensor format with the only supported format of the sdma when getting format
samples: video: capture: Configure frame width and heght for mcxn236
Thanks !
Can do! |
9d964a7
to
2ca712a
Compare
Reconfigure the sensor format with the only supported format of the sdma when getting format Signed-off-by: Zacck Osiemo <[email protected]>
SDMA as the video interface on this board requires this size frame to be set for the sample to work Signed-off-by: Zacck Osiemo <[email protected]>
2ca712a
to
f059ace
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.
The 2nd commit message needs a bit rewording but not a big problem to me. Thanks.
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.
Thanks for the commits/testing/bugfix!
This will help working with more sensors.
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 light commit message rewording could be:
**With** SDMA as the video interface on this board**, it** requires
**the frame size** to be set for the sample to work
Removed DNM label as it seems @josuah has approved but forgot to do it. |
Hi @zacck! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
The video capture sample does not work when using a combination of the FRDM-MCXN236 and the OV5640 camera and this is due to the sensor and the underlying interface having different defaults. This patch sets the interface resolution to that selected by the sample which in turn is set in the CONFIG.