From 426cefda290c50055b395bda8dbda2e86d08224c Mon Sep 17 00:00:00 2001 From: Josuah Demangeon Date: Tue, 1 Apr 2025 17:49:34 +0000 Subject: [PATCH 1/3] drivers: video: common: fix video_closest_frmival() fie.index Fix bug introduced in 46a262ffe62d5e29bf54453de34104779d3e7385 where the fie.index field was expected to be incremented by the driver, while it is the responsibility of the caller to increment it. Fixes #89663 Signed-off-by: Josuah Demangeon --- drivers/video/video_common.c | 8 ++------ drivers/video/video_emul_imager.c | 1 - tests/drivers/video/api/prj.conf | 1 + tests/drivers/video/api/src/video_emul.c | 19 +++++++++++++------ 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/drivers/video/video_common.c b/drivers/video/video_common.c index 5028a5f67a1a..aaeb07983e84 100644 --- a/drivers/video/video_common.c +++ b/drivers/video/video_common.c @@ -135,7 +135,7 @@ void video_closest_frmival(const struct device *dev, enum video_endpoint_id ep, __ASSERT(match->type != VIDEO_FRMIVAL_TYPE_STEPWISE, "cannot find range matching the range, only a value matching the range"); - while (video_enum_frmival(dev, ep, &fie) == 0) { + for (fie.index = 0; video_enum_frmival(dev, ep, &fie) == 0; fie.index++) { struct video_frmival tmp = {0}; uint64_t diff_nsec = 0, a, b; @@ -155,12 +155,8 @@ void video_closest_frmival(const struct device *dev, enum video_endpoint_id ep, diff_nsec = a > b ? a - b : b - a; if (diff_nsec < best_diff_nsec) { best_diff_nsec = diff_nsec; + match->index = fie.index; memcpy(&match->discrete, &tmp, sizeof(tmp)); - - /* The video_enum_frmival() function will increment fie.index every time. - * Compensate for it to get the current index, not the next index. - */ - match->index = fie.index - 1; } } } diff --git a/drivers/video/video_emul_imager.c b/drivers/video/video_emul_imager.c index 8dffbba11f6b..f4a74559b1bc 100644 --- a/drivers/video/video_emul_imager.c +++ b/drivers/video/video_emul_imager.c @@ -277,7 +277,6 @@ static int emul_imager_enum_frmival(const struct device *dev, enum video_endpoin fie->type = VIDEO_FRMIVAL_TYPE_DISCRETE; fie->discrete.numerator = 1; fie->discrete.denominator = mode->fps; - fie->index++; return mode->fps == 0; } diff --git a/tests/drivers/video/api/prj.conf b/tests/drivers/video/api/prj.conf index 8a6c2151c273..e0b2fee641ed 100644 --- a/tests/drivers/video/api/prj.conf +++ b/tests/drivers/video/api/prj.conf @@ -1,4 +1,5 @@ CONFIG_ZTEST=y +CONFIG_ASSERT=y CONFIG_VIDEO=y # Just enough for a single frame in RGB565 format: 320 * 420 * 2 + some margin diff --git a/tests/drivers/video/api/src/video_emul.c b/tests/drivers/video/api/src/video_emul.c index cd58bbf0506b..374619e230c5 100644 --- a/tests/drivers/video/api/src/video_emul.c +++ b/tests/drivers/video/api/src/video_emul.c @@ -83,10 +83,10 @@ ZTEST(video_common, test_video_frmival) /* Do a first enumeration of frame intervals, expected to work */ zexpect_ok(video_enum_frmival(imager_dev, VIDEO_EP_OUT, &fie)); - zexpect_equal(fie.index, 1, "fie's index should increment by one at every iteration"); + zexpect_equal(fie.index, 0, "fie's index should not increment on its own"); /* Test that every value of the frame interval enumerator can be applied */ - do { + for (fie.index = 0; video_enum_frmival(imager_dev, VIDEO_EP_OUT, &fie) == 0; fie.index++) { struct video_frmival q, a; uint32_t min, max, step; @@ -111,18 +111,25 @@ ZTEST(video_common, test_video_frmival) for (q.numerator = min; q.numerator <= max; q.numerator += step) { zexpect_ok(video_set_frmival(imager_dev, VIDEO_EP_OUT, &q)); zexpect_ok(video_get_frmival(imager_dev, VIDEO_EP_OUT, &a)); - zexpect_equal(video_frmival_nsec(&q), video_frmival_nsec(&a)); + zexpect_equal(video_frmival_nsec(&q), video_frmival_nsec(&a), + "query %u/%u (%u nsec) answer %u/%u (%u nsec, sw)", + q.numerator, q.denominator, video_frmival_nsec(&q), + a.numerator, a.denominator, video_frmival_nsec(&a)); } break; case VIDEO_FRMIVAL_TYPE_DISCRETE: /* There is just one frame interval to test */ - zexpect_ok(video_set_frmival(imager_dev, VIDEO_EP_OUT, &fie.discrete)); + memcpy(&q, &fie.discrete, sizeof(q)); + zexpect_ok(video_set_frmival(imager_dev, VIDEO_EP_OUT, &q)); zexpect_ok(video_get_frmival(imager_dev, VIDEO_EP_OUT, &a)); - zexpect_equal(video_frmival_nsec(&fie.discrete), video_frmival_nsec(&a)); + zexpect_equal(video_frmival_nsec(&fie.discrete), video_frmival_nsec(&a), + "query %u/%u (%u nsec) answer %u/%u (%u nsec, discrete)", + q.numerator, q.denominator, video_frmival_nsec(&q), + a.numerator, a.denominator, video_frmival_nsec(&a)); break; } - } while (video_enum_frmival(imager_dev, VIDEO_EP_OUT, &fie) == 0); + } } ZTEST(video_common, test_video_ctrl) From 6321131d9972ee8edde81561638c421e29588c29 Mon Sep 17 00:00:00 2001 From: Josuah Demangeon Date: Thu, 8 May 2025 15:13:49 +0000 Subject: [PATCH 2/3] drivers: video: common: optimizations for video_closest_frmival() In video_closest_frmival(), immediately stop searching when an exact match is found, as a small performance optimization. Variables that could be computed only once were moved further outside. Signed-off-by: Josuah Demangeon --- drivers/video/video_common.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/drivers/video/video_common.c b/drivers/video/video_common.c index aaeb07983e84..5ad983b3b673 100644 --- a/drivers/video/video_common.c +++ b/drivers/video/video_common.c @@ -128,16 +128,18 @@ void video_closest_frmival_stepwise(const struct video_frmival_stepwise *stepwis void video_closest_frmival(const struct device *dev, enum video_endpoint_id ep, struct video_frmival_enum *match) { - uint64_t best_diff_nsec = INT32_MAX; struct video_frmival desired = match->discrete; struct video_frmival_enum fie = {.format = match->format}; + uint64_t best_diff_nsec = INT32_MAX; + uint64_t goal_nsec = video_frmival_nsec(&desired); __ASSERT(match->type != VIDEO_FRMIVAL_TYPE_STEPWISE, "cannot find range matching the range, only a value matching the range"); for (fie.index = 0; video_enum_frmival(dev, ep, &fie) == 0; fie.index++) { struct video_frmival tmp = {0}; - uint64_t diff_nsec = 0, a, b; + uint64_t diff_nsec = 0; + uint64_t tmp_nsec; switch (fie.type) { case VIDEO_FRMIVAL_TYPE_DISCRETE: @@ -150,13 +152,18 @@ void video_closest_frmival(const struct device *dev, enum video_endpoint_id ep, __ASSERT(false, "invalid answer from the queried video device"); } - a = video_frmival_nsec(&desired); - b = video_frmival_nsec(&tmp); - diff_nsec = a > b ? a - b : b - a; + tmp_nsec = video_frmival_nsec(&tmp); + diff_nsec = tmp_nsec > goal_nsec ? tmp_nsec - goal_nsec : goal_nsec - tmp_nsec; + if (diff_nsec < best_diff_nsec) { best_diff_nsec = diff_nsec; match->index = fie.index; - memcpy(&match->discrete, &tmp, sizeof(tmp)); + match->discrete = tmp; + } + + if (diff_nsec == 0) { + /* Exact match, stop searching a better match */ + break; } } } From df243bcfc6129b885a483fd2bfd35031b7ffacdc Mon Sep 17 00:00:00 2001 From: Josuah Demangeon Date: Thu, 8 May 2025 23:10:28 +0000 Subject: [PATCH 3/3] drivers: video: common: turn ASSERT(false) into CODE_UNREACHABLE The assert message was not very helpful, CODE_UNREACHABLE is more readable and requires fewer effort while proofreading. Signed-off-by: Josuah Demangeon --- drivers/video/video_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/video/video_common.c b/drivers/video/video_common.c index 5ad983b3b673..64d838a4febd 100644 --- a/drivers/video/video_common.c +++ b/drivers/video/video_common.c @@ -149,7 +149,7 @@ void video_closest_frmival(const struct device *dev, enum video_endpoint_id ep, video_closest_frmival_stepwise(&fie.stepwise, &desired, &tmp); break; default: - __ASSERT(false, "invalid answer from the queried video device"); + CODE_UNREACHABLE; } tmp_nsec = video_frmival_nsec(&tmp);