Re: [PATCH v4] media: vimc: Implement debayer control for mean window size
Hi Arthur,
I added this patch to my pull request, but I have a request for a follow-up
patch:
On 10/2/19 2:46 AM, Arthur Moraes do Lago wrote:
> Add mean window size parameter for debayer filter as a control in
> vimc-debayer.
>
> vimc-debayer was patched to allow changing mean window parameter
> of the filter without needing to reload the driver. The parameter
> can now be set using a v4l2-ctl control(mean_window_size).
>
> Co-developed-by: Laís Pessine do Carmo
> Signed-off-by: Laís Pessine do Carmo
> Signed-off-by: Arthur Moraes do Lago
> ---
> Changes in V2:
> - Updated documentation
> - Added v4l2_subev_core_ops to solve errors in v4l2-ctl compliance test
> - Changed control naming to follow English capitalization rules
> - Rebased to Shuah Khan's newest patch series 171283
> ("Collapse vimc single monolithic driver")
> - Change maximum value for mean window size
> Changes in V3:
> - Renamed debayer control
> - Fixed typo in documentation
> - Freed control handler in vimc_deb_release
> Changes in V4:
> - Removed unecessary function and checking for setting the control
>
> We had originally intended to leave that bit of code checking if the
> value is being set to make it similar to what's done in vimc-sensor,
> and in case some extra caution is needed when chaging control in the
> future. But I guess they really were not necessary.
>
> Thanks!
>
> ---
> Documentation/media/v4l-drivers/vimc.rst | 10 +--
> drivers/media/platform/vimc/vimc-common.h | 1 +
> drivers/media/platform/vimc/vimc-debayer.c | 81 ++
> 3 files changed, 71 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/media/v4l-drivers/vimc.rst
> b/Documentation/media/v4l-drivers/vimc.rst
> index a582af0509ee..28646c76dad5 100644
> --- a/Documentation/media/v4l-drivers/vimc.rst
> +++ b/Documentation/media/v4l-drivers/vimc.rst
> @@ -80,9 +80,7 @@ vimc-capture:
> Module options
> ---
>
> -Vimc has a few module parameters to configure the driver.
> -
> -param=value
> +Vimc has a module parameter to configure the driver.
>
> * ``sca_mult=``
>
> @@ -91,12 +89,6 @@ Vimc has a few module parameters to configure the driver.
> original one. Currently, only supports scaling up (the default value
> is 3).
>
> -* ``deb_mean_win_size=``
> -
> -Window size to calculate the mean. Note: the window size needs to be
> an
> -odd number, as the main pixel stays in the center of the window,
> -otherwise the next odd number is considered (the default value is 3).
> -
> Source code documentation
> -
>
> diff --git a/drivers/media/platform/vimc/vimc-common.h
> b/drivers/media/platform/vimc/vimc-common.h
> index 236412ad7548..3a5102ddf794 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -19,6 +19,7 @@
> #define VIMC_CID_VIMC_BASE (0x00f0 | 0xf000)
> #define VIMC_CID_VIMC_CLASS (0x00f0 | 1)
> #define VIMC_CID_TEST_PATTERN(VIMC_CID_VIMC_BASE + 0)
> +#define VIMC_CID_MEAN_WIN_SIZE (VIMC_CID_VIMC_BASE + 1)
>
> #define VIMC_FRAME_MAX_WIDTH 4096
> #define VIMC_FRAME_MAX_HEIGHT 2160
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c
> b/drivers/media/platform/vimc/vimc-debayer.c
> index 37f3767db469..ba0af4b2fb9b 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -11,17 +11,12 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
>
> #include "vimc-common.h"
>
> -static unsigned int deb_mean_win_size = 3;
> -module_param(deb_mean_win_size, uint, );
> -MODULE_PARM_DESC(deb_mean_win_size, " the window size to calculate the
> mean.\n"
> - "NOTE: the window size needs to be an odd number, as the main pixel "
> - "stays in the center of the window, otherwise the next odd number "
> - "is considered");
> -
> enum vimc_deb_rgb_colors {
> VIMC_DEB_RED = 0,
> VIMC_DEB_GREEN = 1,
> @@ -46,6 +41,8 @@ struct vimc_deb_device {
> u8 *src_frame;
> const struct vimc_deb_pix_map *sink_pix_map;
> unsigned int sink_bpp;
> + unsigned int mean_win_size;
> + struct v4l2_ctrl_handler hdl;
> };
>
> static const struct v4l2_mbus_framefmt sink_fmt_default = {
> @@ -346,11 +343,18 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd,
> int enable)
> return 0;
> }
>
> +static const struct v4l2_subdev_core_ops vimc_deb_core_ops = {
> + .log_status = v4l2_ctrl_subdev_log_status,
> + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> static const struct v4l2_subdev_video_ops vimc_deb_video_ops = {
> .s_stream = vimc_deb_s_stream,
> };
>
> static const struct v4l2_subdev_ops vimc_deb_ops = {
> + .core = _deb_core_ops,
> .pad =
Re: [Lkcamp] [PATCH v4] media: vimc: Implement debayer control for mean window size
On 10/1/19 9:46 PM, Arthur Moraes do Lago wrote:
> Add mean window size parameter for debayer filter as a control in
> vimc-debayer.
>
> vimc-debayer was patched to allow changing mean window parameter
> of the filter without needing to reload the driver. The parameter
> can now be set using a v4l2-ctl control(mean_window_size).
>
> Co-developed-by: Laís Pessine do Carmo
> Signed-off-by: Laís Pessine do Carmo
> Signed-off-by: Arthur Moraes do Lago
Now the right one.
lgtm
Acked-by: Helen Koike
> ---
> Changes in V2:
> - Updated documentation
> - Added v4l2_subev_core_ops to solve errors in v4l2-ctl compliance test
> - Changed control naming to follow English capitalization rules
> - Rebased to Shuah Khan's newest patch series 171283
> ("Collapse vimc single monolithic driver")
> - Change maximum value for mean window size
> Changes in V3:
> - Renamed debayer control
> - Fixed typo in documentation
> - Freed control handler in vimc_deb_release
> Changes in V4:
> - Removed unecessary function and checking for setting the control
>
> We had originally intended to leave that bit of code checking if the
> value is being set to make it similar to what's done in vimc-sensor,
> and in case some extra caution is needed when chaging control in the
> future. But I guess they really were not necessary.
>
> Thanks!
>
> ---
> Documentation/media/v4l-drivers/vimc.rst | 10 +--
> drivers/media/platform/vimc/vimc-common.h | 1 +
> drivers/media/platform/vimc/vimc-debayer.c | 81 ++
> 3 files changed, 71 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/media/v4l-drivers/vimc.rst
> b/Documentation/media/v4l-drivers/vimc.rst
> index a582af0509ee..28646c76dad5 100644
> --- a/Documentation/media/v4l-drivers/vimc.rst
> +++ b/Documentation/media/v4l-drivers/vimc.rst
> @@ -80,9 +80,7 @@ vimc-capture:
> Module options
> ---
>
> -Vimc has a few module parameters to configure the driver.
> -
> -param=value
> +Vimc has a module parameter to configure the driver.
>
> * ``sca_mult=``
>
> @@ -91,12 +89,6 @@ Vimc has a few module parameters to configure the driver.
> original one. Currently, only supports scaling up (the default value
> is 3).
>
> -* ``deb_mean_win_size=``
> -
> -Window size to calculate the mean. Note: the window size needs to be
> an
> -odd number, as the main pixel stays in the center of the window,
> -otherwise the next odd number is considered (the default value is 3).
> -
> Source code documentation
> -
>
> diff --git a/drivers/media/platform/vimc/vimc-common.h
> b/drivers/media/platform/vimc/vimc-common.h
> index 236412ad7548..3a5102ddf794 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -19,6 +19,7 @@
> #define VIMC_CID_VIMC_BASE (0x00f0 | 0xf000)
> #define VIMC_CID_VIMC_CLASS (0x00f0 | 1)
> #define VIMC_CID_TEST_PATTERN(VIMC_CID_VIMC_BASE + 0)
> +#define VIMC_CID_MEAN_WIN_SIZE (VIMC_CID_VIMC_BASE + 1)
>
> #define VIMC_FRAME_MAX_WIDTH 4096
> #define VIMC_FRAME_MAX_HEIGHT 2160
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c
> b/drivers/media/platform/vimc/vimc-debayer.c
> index 37f3767db469..ba0af4b2fb9b 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -11,17 +11,12 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
>
> #include "vimc-common.h"
>
> -static unsigned int deb_mean_win_size = 3;
> -module_param(deb_mean_win_size, uint, );
> -MODULE_PARM_DESC(deb_mean_win_size, " the window size to calculate the
> mean.\n"
> - "NOTE: the window size needs to be an odd number, as the main pixel "
> - "stays in the center of the window, otherwise the next odd number "
> - "is considered");
> -
> enum vimc_deb_rgb_colors {
> VIMC_DEB_RED = 0,
> VIMC_DEB_GREEN = 1,
> @@ -46,6 +41,8 @@ struct vimc_deb_device {
> u8 *src_frame;
> const struct vimc_deb_pix_map *sink_pix_map;
> unsigned int sink_bpp;
> + unsigned int mean_win_size;
> + struct v4l2_ctrl_handler hdl;
> };
>
> static const struct v4l2_mbus_framefmt sink_fmt_default = {
> @@ -346,11 +343,18 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd,
> int enable)
> return 0;
> }
>
> +static const struct v4l2_subdev_core_ops vimc_deb_core_ops = {
> + .log_status = v4l2_ctrl_subdev_log_status,
> + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> static const struct v4l2_subdev_video_ops vimc_deb_video_ops = {
> .s_stream = vimc_deb_s_stream,
> };
>
> static const struct v4l2_subdev_ops vimc_deb_ops = {
> + .core = _deb_core_ops,
> .pad = _deb_pad_ops,
> .video = _deb_video_ops,
> };
[PATCH v4] media: vimc: Implement debayer control for mean window size
Add mean window size parameter for debayer filter as a control in
vimc-debayer.
vimc-debayer was patched to allow changing mean window parameter
of the filter without needing to reload the driver. The parameter
can now be set using a v4l2-ctl control(mean_window_size).
Co-developed-by: Laís Pessine do Carmo
Signed-off-by: Laís Pessine do Carmo
Signed-off-by: Arthur Moraes do Lago
---
Changes in V2:
- Updated documentation
- Added v4l2_subev_core_ops to solve errors in v4l2-ctl compliance test
- Changed control naming to follow English capitalization rules
- Rebased to Shuah Khan's newest patch series 171283
("Collapse vimc single monolithic driver")
- Change maximum value for mean window size
Changes in V3:
- Renamed debayer control
- Fixed typo in documentation
- Freed control handler in vimc_deb_release
Changes in V4:
- Removed unecessary function and checking for setting the control
We had originally intended to leave that bit of code checking if the
value is being set to make it similar to what's done in vimc-sensor,
and in case some extra caution is needed when chaging control in the
future. But I guess they really were not necessary.
Thanks!
---
Documentation/media/v4l-drivers/vimc.rst | 10 +--
drivers/media/platform/vimc/vimc-common.h | 1 +
drivers/media/platform/vimc/vimc-debayer.c | 81 ++
3 files changed, 71 insertions(+), 21 deletions(-)
diff --git a/Documentation/media/v4l-drivers/vimc.rst
b/Documentation/media/v4l-drivers/vimc.rst
index a582af0509ee..28646c76dad5 100644
--- a/Documentation/media/v4l-drivers/vimc.rst
+++ b/Documentation/media/v4l-drivers/vimc.rst
@@ -80,9 +80,7 @@ vimc-capture:
Module options
---
-Vimc has a few module parameters to configure the driver.
-
-param=value
+Vimc has a module parameter to configure the driver.
* ``sca_mult=``
@@ -91,12 +89,6 @@ Vimc has a few module parameters to configure the driver.
original one. Currently, only supports scaling up (the default value
is 3).
-* ``deb_mean_win_size=``
-
-Window size to calculate the mean. Note: the window size needs to be an
-odd number, as the main pixel stays in the center of the window,
-otherwise the next odd number is considered (the default value is 3).
-
Source code documentation
-
diff --git a/drivers/media/platform/vimc/vimc-common.h
b/drivers/media/platform/vimc/vimc-common.h
index 236412ad7548..3a5102ddf794 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -19,6 +19,7 @@
#define VIMC_CID_VIMC_BASE (0x00f0 | 0xf000)
#define VIMC_CID_VIMC_CLASS(0x00f0 | 1)
#define VIMC_CID_TEST_PATTERN (VIMC_CID_VIMC_BASE + 0)
+#define VIMC_CID_MEAN_WIN_SIZE (VIMC_CID_VIMC_BASE + 1)
#define VIMC_FRAME_MAX_WIDTH 4096
#define VIMC_FRAME_MAX_HEIGHT 2160
diff --git a/drivers/media/platform/vimc/vimc-debayer.c
b/drivers/media/platform/vimc/vimc-debayer.c
index 37f3767db469..ba0af4b2fb9b 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -11,17 +11,12 @@
#include
#include
#include
+#include
+#include
#include
#include "vimc-common.h"
-static unsigned int deb_mean_win_size = 3;
-module_param(deb_mean_win_size, uint, );
-MODULE_PARM_DESC(deb_mean_win_size, " the window size to calculate the mean.\n"
- "NOTE: the window size needs to be an odd number, as the main pixel "
- "stays in the center of the window, otherwise the next odd number "
- "is considered");
-
enum vimc_deb_rgb_colors {
VIMC_DEB_RED = 0,
VIMC_DEB_GREEN = 1,
@@ -46,6 +41,8 @@ struct vimc_deb_device {
u8 *src_frame;
const struct vimc_deb_pix_map *sink_pix_map;
unsigned int sink_bpp;
+ unsigned int mean_win_size;
+ struct v4l2_ctrl_handler hdl;
};
static const struct v4l2_mbus_framefmt sink_fmt_default = {
@@ -346,11 +343,18 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int
enable)
return 0;
}
+static const struct v4l2_subdev_core_ops vimc_deb_core_ops = {
+ .log_status = v4l2_ctrl_subdev_log_status,
+ .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+ .unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
static const struct v4l2_subdev_video_ops vimc_deb_video_ops = {
.s_stream = vimc_deb_s_stream,
};
static const struct v4l2_subdev_ops vimc_deb_ops = {
+ .core = _deb_core_ops,
.pad = _deb_pad_ops,
.video = _deb_video_ops,
};
@@ -384,7 +388,7 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device
*vdeb,
* the top left corner of the mean window (considering the current
* pixel as the center)
*/
- seek = deb_mean_win_size / 2;
+ seek = vdeb->mean_win_size / 2;
/* Sum the values of the colors in the mean window */
