* [PATCH 0/1] media: i2c: max96717: add test pattern ctrl @ 2024-06-17 14:59 Tommaso Merciai 2024-06-17 14:59 ` [PATCH 1/1] " Tommaso Merciai 0 siblings, 1 reply; 6+ messages in thread From: Tommaso Merciai @ 2024-06-17 14:59 UTC (permalink / raw) Cc: linuxfancy, sakari.ailus, laurent.pinchart, tomm.merciai, julien.massot, Mauro Carvalho Chehab, linux-media, linux-kernel Hi, Taking as reference the work done by Julien Massot on the MAX96716 driver the test pattern generator on the serializer side MAX96717 has been implemented. This work was done also to test and validate the GMSL2 version of the Alvium camera on i.MX8MP ISI path. HW setup: - alvium -> max96717 -> max96716 -> i.MX8MP (ISI path) During the test it's mandatory to put also the de-serializer into pixel mode disabling tunnel mode. This can be done writing bit 0 of MIPI_TX52 (0x474) TUN_EN reg. Test pattern output: - https://pasteboard.co/d9yFAR6nTrn9.png Max96717 datasheet: - https://www.analog.com/media/en/technical-documentation/data-sheets/max96717.pdf Thanks & Regards, Tommaso Tommaso Merciai (1): media: i2c: max96717: add test pattern ctrl drivers/media/i2c/max96717.c | 210 ++++++++++++++++++++++++++++++++--- 1 file changed, 194 insertions(+), 16 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] media: i2c: max96717: add test pattern ctrl 2024-06-17 14:59 [PATCH 0/1] media: i2c: max96717: add test pattern ctrl Tommaso Merciai @ 2024-06-17 14:59 ` Tommaso Merciai 2024-06-25 7:54 ` Julien Massot 0 siblings, 1 reply; 6+ messages in thread From: Tommaso Merciai @ 2024-06-17 14:59 UTC (permalink / raw) Cc: linuxfancy, sakari.ailus, laurent.pinchart, tomm.merciai, julien.massot, Mauro Carvalho Chehab, linux-media, linux-kernel Add v4l2 test pattern control. Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> --- drivers/media/i2c/max96717.c | 210 ++++++++++++++++++++++++++++++++--- 1 file changed, 194 insertions(+), 16 deletions(-) diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c index 949306485873..c263bbca7318 100644 --- a/drivers/media/i2c/max96717.c +++ b/drivers/media/i2c/max96717.c @@ -16,6 +16,7 @@ #include <linux/regmap.h> #include <media/v4l2-cci.h> +#include <media/v4l2-ctrls.h> #include <media/v4l2-fwnode.h> #include <media/v4l2-subdev.h> @@ -36,11 +37,37 @@ #define MAX96717_DEV_ID CCI_REG8(0xd) #define MAX96717_DEV_REV CCI_REG8(0xe) #define MAX96717_DEV_REV_MASK GENMASK(3, 0) +#define MAX96717_IO_CHK0 CCI_REG8(0x24f) +#define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1) /* VID_TX Z */ +#define MAX96717_VIDEO_TX0 CCI_REG8(0x110) +#define MAX96717_VIDEO_AUTO_BPP BIT(3) #define MAX96717_VIDEO_TX2 CCI_REG8(0x112) #define MAX96717_VIDEO_PCLKDET BIT(7) +/* VRX_PATGEN_0 */ +#define MAX96717_PATGEN_0 CCI_REG8(0x24e) +#define MAX96717_PATGEN_1 CCI_REG8(0x26b) +#define MAX96717_PATGEN_MODE GENMASK(1, 0) +#define MAX96717_PATGEN_VS_DLY CCI_REG24(0x250) +#define MAX96717_PATGEN_VS_HIGH CCI_REG24(0x253) +#define MAX96717_PATGEN_VS_LOW CCI_REG24(0x256) +#define MAX96717_PATGEN_V2H CCI_REG24(0x259) +#define MAX96717_PATGEN_HS_HIGH CCI_REG16(0x25c) +#define MAX96717_PATGEN_HS_LOW CCI_REG16(0x25e) +#define MAX96717_PATGEN_HS_CNT CCI_REG16(0x260) +#define MAX96717_PATGEN_V2D CCI_REG24(0x262) +#define MAX96717_PATGEN_DE_HIGH CCI_REG16(0x265) +#define MAX96717_PATGEN_DE_LOW CCI_REG16(0x267) +#define MAX96717_PATGEN_DE_CNT CCI_REG16(0x269) +#define MAX96717_PATGEN_GRAD_INC CCI_REG8(0x26c) +#define MAX96717_PATGEN_CHKB_COLOR_A CCI_REG24(0x26d) +#define MAX96717_PATGEN_CHKB_COLOR_B CCI_REG24(0x270) +#define MAX96717_PATGEN_CHKB_RPT_CNT_A CCI_REG8(0x273) +#define MAX96717_PATGEN_CHKB_RPT_CNT_B CCI_REG8(0x274) +#define MAX96717_PATGEN_CHKB_ALT CCI_REG8(0x275) + /* GPIO */ #define MAX96717_NUM_GPIO 11 #define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3) @@ -82,6 +109,12 @@ /* MISC */ #define PIO_SLEW_1 CCI_REG8(0x570) +enum max96717_vpg_mode { + MAX96717_VPG_DISABLED = 0, + MAX96717_VPG_CHECKERBOARD = 1, + MAX96717_VPG_GRADIENT = 2, +}; + struct max96717_priv { struct i2c_client *client; struct regmap *regmap; @@ -89,6 +122,7 @@ struct max96717_priv { struct v4l2_mbus_config_mipi_csi2 mipi_csi2; struct v4l2_subdev sd; struct media_pad pads[MAX96717_PORTS]; + struct v4l2_ctrl_handler ctrl_handler; struct v4l2_async_notifier notifier; struct v4l2_subdev *source_sd; u16 source_sd_pad; @@ -96,6 +130,7 @@ struct max96717_priv { u8 pll_predef_index; struct clk_hw clk_hw; struct gpio_chip gpio_chip; + enum max96717_vpg_mode pattern; }; static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd) @@ -131,6 +166,115 @@ static inline int max96717_start_csi(struct max96717_priv *priv, bool start) start ? MAX96717_START_PORT_B : 0, NULL); } +static int max96717_apply_patgen_timing(struct max96717_priv *priv, + struct v4l2_subdev_state *state) +{ + struct v4l2_mbus_framefmt *fmt = + v4l2_subdev_state_get_format(state, MAX96717_PAD_SOURCE); + const u32 h_active = fmt->width; + const u32 h_fp = 88; + const u32 h_sw = 44; + const u32 h_bp = 148; + u32 h_tot; + const u32 v_active = fmt->height; + const u32 v_fp = 4; + const u32 v_sw = 5; + const u32 v_bp = 36; + u32 v_tot; + int ret = 0; + + h_tot = h_active + h_fp + h_sw + h_bp; + v_tot = v_active + v_fp + v_sw + v_bp; + + /* 75 Mhz pixel clock */ + cci_update_bits(priv->regmap, MAX96717_IO_CHK0, + MAX96717_PATTERN_CLK_FREQ, 0xa, &ret); + + dev_info(&priv->client->dev, "height: %d width: %d\n", fmt->height, + fmt->width); + + cci_write(priv->regmap, MAX96717_PATGEN_VS_DLY, 0, &ret); + cci_write(priv->regmap, MAX96717_PATGEN_VS_HIGH, v_sw * h_tot, &ret); + cci_write(priv->regmap, MAX96717_PATGEN_VS_LOW, + (v_active + v_fp + v_bp) * h_tot, &ret); + cci_write(priv->regmap, MAX96717_PATGEN_HS_HIGH, h_sw, &ret); + cci_write(priv->regmap, MAX96717_PATGEN_HS_LOW, h_active + h_fp + h_bp, + &ret); + cci_write(priv->regmap, MAX96717_PATGEN_V2D, + h_tot * (v_sw + v_bp) + (h_sw + h_bp), &ret); + cci_write(priv->regmap, MAX96717_PATGEN_HS_CNT, v_tot, &ret); + cci_write(priv->regmap, MAX96717_PATGEN_DE_HIGH, h_active, &ret); + cci_write(priv->regmap, MAX96717_PATGEN_DE_LOW, h_fp + h_sw + h_bp, + &ret); + cci_write(priv->regmap, MAX96717_PATGEN_DE_CNT, v_active, &ret); + /* B G R */ + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_A, 0xfecc00, &ret); + /* B G R */ + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_B, 0x006aa7, &ret); + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_A, 0x3c, &ret); + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_B, 0x3c, &ret); + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_ALT, 0x3c, &ret); + cci_write(priv->regmap, MAX96717_PATGEN_GRAD_INC, 0x10, &ret); + + return ret; +} + +static int max96717_apply_patgen(struct max96717_priv *priv, + struct v4l2_subdev_state *state) +{ + unsigned int val; + int ret = 0; + + if (priv->pattern) + ret = max96717_apply_patgen_timing(priv, state); + + cci_write(priv->regmap, MAX96717_PATGEN_0, priv->pattern ? 0xfb : 0, + &ret); + + val = FIELD_PREP(MAX96717_PATGEN_MODE, priv->pattern); + cci_update_bits(priv->regmap, MAX96717_PATGEN_1, MAX96717_PATGEN_MODE, + val, &ret); + return ret; +} + +static int max96717_s_ctrl(struct v4l2_ctrl *ctrl) +{ + struct max96717_priv *priv = + container_of(ctrl->handler, struct max96717_priv, ctrl_handler); + int ret; + + switch (ctrl->id) { + case V4L2_CID_TEST_PATTERN: + if (priv->enabled_source_streams) + return -EBUSY; + priv->pattern = ctrl->val; + break; + default: + return -EINVAL; + } + + /* Use bpp from bpp register */ + ret = cci_update_bits(priv->regmap, MAX96717_VIDEO_TX0, + MAX96717_VIDEO_AUTO_BPP, + priv->pattern ? 0 : MAX96717_VIDEO_AUTO_BPP, + NULL); + + /* Pattern generator doesn't work with tunnel mode */ + return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11, + MAX96717_TUN_MODE, + priv->pattern ? 0 : MAX96717_TUN_MODE, &ret); +} + +static const char * const max96717_test_pattern[] = { + "Disabled", + "Checkerboard", + "Gradient" +}; + +static const struct v4l2_ctrl_ops max96717_ctrl_ops = { + .s_ctrl = max96717_s_ctrl, +}; + static int max96717_gpiochip_get(struct gpio_chip *gpiochip, unsigned int offset) { @@ -352,20 +496,28 @@ static int max96717_enable_streams(struct v4l2_subdev *sd, u64 sink_streams; int ret; - sink_streams = v4l2_subdev_state_xlate_streams(state, - MAX96717_PAD_SOURCE, - MAX96717_PAD_SINK, - &streams_mask); - if (!priv->enabled_source_streams) max96717_start_csi(priv, true); - ret = v4l2_subdev_enable_streams(priv->source_sd, priv->source_sd_pad, - sink_streams); - if (ret) { - dev_err(dev, "Fail to start streams:%llu on remote subdev\n", - sink_streams); + ret = max96717_apply_patgen(priv, state); + if (ret) goto stop_csi; + + if (!priv->pattern) { + sink_streams = + v4l2_subdev_state_xlate_streams(state, + MAX96717_PAD_SOURCE, + MAX96717_PAD_SINK, + &streams_mask); + + ret = v4l2_subdev_enable_streams(priv->source_sd, + priv->source_sd_pad, + sink_streams); + if (ret) { + dev_err(dev, "Fail to start streams:%llu on remote subdev\n", + sink_streams); + goto stop_csi; + } } priv->enabled_source_streams |= streams_mask; @@ -394,13 +546,23 @@ static int max96717_disable_streams(struct v4l2_subdev *sd, if (!priv->enabled_source_streams) max96717_start_csi(priv, false); - sink_streams = v4l2_subdev_state_xlate_streams(state, - MAX96717_PAD_SOURCE, - MAX96717_PAD_SINK, - &streams_mask); + if (!priv->pattern) { + int ret; + + sink_streams = + v4l2_subdev_state_xlate_streams(state, + MAX96717_PAD_SOURCE, + MAX96717_PAD_SINK, + &streams_mask); - return v4l2_subdev_disable_streams(priv->source_sd, priv->source_sd_pad, - sink_streams); + ret = v4l2_subdev_disable_streams(priv->source_sd, + priv->source_sd_pad, + sink_streams); + if (ret) + return ret; + } + + return 0; } static const struct v4l2_subdev_pad_ops max96717_pad_ops = { @@ -513,6 +675,19 @@ static int max96717_subdev_init(struct max96717_priv *priv) v4l2_i2c_subdev_init(&priv->sd, priv->client, &max96717_subdev_ops); priv->sd.internal_ops = &max96717_internal_ops; + v4l2_ctrl_handler_init(&priv->ctrl_handler, 1); + priv->sd.ctrl_handler = &priv->ctrl_handler; + + v4l2_ctrl_new_std_menu_items(&priv->ctrl_handler, + &max96717_ctrl_ops, + V4L2_CID_TEST_PATTERN, + ARRAY_SIZE(max96717_test_pattern) - 1, + 0, 0, max96717_test_pattern); + if (priv->ctrl_handler.error) { + ret = priv->ctrl_handler.error; + goto err_free_ctrl; + } + priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; priv->sd.entity.ops = &max96717_entity_ops; @@ -552,6 +727,8 @@ static int max96717_subdev_init(struct max96717_priv *priv) v4l2_subdev_cleanup(&priv->sd); err_entity_cleanup: media_entity_cleanup(&priv->sd.entity); +err_free_ctrl: + v4l2_ctrl_handler_free(&priv->ctrl_handler); return ret; } @@ -563,6 +740,7 @@ static void max96717_subdev_uninit(struct max96717_priv *priv) v4l2_async_nf_cleanup(&priv->notifier); v4l2_subdev_cleanup(&priv->sd); media_entity_cleanup(&priv->sd.entity); + v4l2_ctrl_handler_free(&priv->ctrl_handler); } struct max96717_pll_predef_freq { -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: i2c: max96717: add test pattern ctrl 2024-06-17 14:59 ` [PATCH 1/1] " Tommaso Merciai @ 2024-06-25 7:54 ` Julien Massot 2024-06-25 17:31 ` Tommaso Merciai 0 siblings, 1 reply; 6+ messages in thread From: Julien Massot @ 2024-06-25 7:54 UTC (permalink / raw) To: Tommaso Merciai, Tomi Valkeinen Cc: linuxfancy, sakari.ailus, laurent.pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel Hi Tommaso, Thanks for your patch. I tested it on my setup and can capture frames. Here's a link to an example: https://pasteboard.co/j8yHuE4YdYDV.jpg. I had some trouble with link validation because my sensor doesn't support the RGB format. Once we have internal pad support, we won't need to worry about the serializer creating an incompatible video stream for the sensor. In the future, it would be great if we could support the serializer without needing a connected sensor. This way, we can use it as a pattern generator with this patch. However, not all GMSL2 serializers work this way. For example, the MAX9295A can't generate an internal PCLK and relies on the sensor to provide the video stream. Overall, this patch is very useful as it lets us receive a pattern from the serializer, which helps validate the GMSL2 connection. It also handles issues related to generator timing, bits per pixel (bpp), and tunnel mode that users might encounter. On 6/17/24 4:59 PM, Tommaso Merciai wrote: > Add v4l2 test pattern control. > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> > --- > drivers/media/i2c/max96717.c | 210 ++++++++++++++++++++++++++++++++--- > 1 file changed, 194 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c > index 949306485873..c263bbca7318 100644 > --- a/drivers/media/i2c/max96717.c > +++ b/drivers/media/i2c/max96717.c > @@ -16,6 +16,7 @@ > #include <linux/regmap.h> > > #include <media/v4l2-cci.h> > +#include <media/v4l2-ctrls.h> > #include <media/v4l2-fwnode.h> > #include <media/v4l2-subdev.h> > > @@ -36,11 +37,37 @@ > #define MAX96717_DEV_ID CCI_REG8(0xd) > #define MAX96717_DEV_REV CCI_REG8(0xe) > #define MAX96717_DEV_REV_MASK GENMASK(3, 0) > +#define MAX96717_IO_CHK0 CCI_REG8(0x24f) In MAX96717 datasheet this register is named VTX1. Can you also move these definitions to the VTX section? > +#define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1) > > /* VID_TX Z */ > +#define MAX96717_VIDEO_TX0 CCI_REG8(0x110) > +#define MAX96717_VIDEO_AUTO_BPP BIT(3) > #define MAX96717_VIDEO_TX2 CCI_REG8(0x112) > #define MAX96717_VIDEO_PCLKDET BIT(7) > > +/* VRX_PATGEN_0 */ For serializer these registers are named VTX_Z instead of VRX. > +#define MAX96717_PATGEN_0 CCI_REG8(0x24e) #define MAX96717_VTX_0 CCI_REG8(0x24e) > +#define MAX96717_PATGEN_1 CCI_REG8(0x26b) Can you keep this define ordered by address? > +#define MAX96717_PATGEN_MODE GENMASK(1, 0) > +#define MAX96717_PATGEN_VS_DLY CCI_REG24(0x250) #define MAX96717_VTX_VS_DLY > +#define MAX96717_PATGEN_VS_HIGH CCI_REG24(0x253) > +#define MAX96717_PATGEN_VS_LOW CCI_REG24(0x256) > +#define MAX96717_PATGEN_V2H CCI_REG24(0x259) > +#define MAX96717_PATGEN_HS_HIGH CCI_REG16(0x25c) > +#define MAX96717_PATGEN_HS_LOW CCI_REG16(0x25e) > +#define MAX96717_PATGEN_HS_CNT CCI_REG16(0x260) > +#define MAX96717_PATGEN_V2D CCI_REG24(0x262) > +#define MAX96717_PATGEN_DE_HIGH CCI_REG16(0x265) > +#define MAX96717_PATGEN_DE_LOW CCI_REG16(0x267) > +#define MAX96717_PATGEN_DE_CNT CCI_REG16(0x269) > +#define MAX96717_PATGEN_GRAD_INC CCI_REG8(0x26c) > +#define MAX96717_PATGEN_CHKB_COLOR_A CCI_REG24(0x26d) > +#define MAX96717_PATGEN_CHKB_COLOR_B CCI_REG24(0x270) > +#define MAX96717_PATGEN_CHKB_RPT_CNT_A CCI_REG8(0x273) > +#define MAX96717_PATGEN_CHKB_RPT_CNT_B CCI_REG8(0x274) > +#define MAX96717_PATGEN_CHKB_ALT CCI_REG8(0x275) > + > /* GPIO */ > #define MAX96717_NUM_GPIO 11 > #define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3) > @@ -82,6 +109,12 @@ > /* MISC */ > #define PIO_SLEW_1 CCI_REG8(0x570) > > +enum max96717_vpg_mode { > + MAX96717_VPG_DISABLED = 0, > + MAX96717_VPG_CHECKERBOARD = 1, > + MAX96717_VPG_GRADIENT = 2, > +}; > + > struct max96717_priv { > struct i2c_client *client; > struct regmap *regmap; > @@ -89,6 +122,7 @@ struct max96717_priv { > struct v4l2_mbus_config_mipi_csi2 mipi_csi2; > struct v4l2_subdev sd; > struct media_pad pads[MAX96717_PORTS]; > + struct v4l2_ctrl_handler ctrl_handler; > struct v4l2_async_notifier notifier; > struct v4l2_subdev *source_sd; > u16 source_sd_pad; > @@ -96,6 +130,7 @@ struct max96717_priv { > u8 pll_predef_index; > struct clk_hw clk_hw; > struct gpio_chip gpio_chip; > + enum max96717_vpg_mode pattern; > }; > > static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd) > @@ -131,6 +166,115 @@ static inline int max96717_start_csi(struct max96717_priv *priv, bool start) > start ? MAX96717_START_PORT_B : 0, NULL); > } > > +static int max96717_apply_patgen_timing(struct max96717_priv *priv, > + struct v4l2_subdev_state *state) > +{ > + struct v4l2_mbus_framefmt *fmt = > + v4l2_subdev_state_get_format(state, MAX96717_PAD_SOURCE); > + const u32 h_active = fmt->width; > + const u32 h_fp = 88; > + const u32 h_sw = 44; > + const u32 h_bp = 148; > + u32 h_tot; > + const u32 v_active = fmt->height; > + const u32 v_fp = 4; > + const u32 v_sw = 5; > + const u32 v_bp = 36; > + u32 v_tot; > + int ret = 0; > + > + h_tot = h_active + h_fp + h_sw + h_bp; > + v_tot = v_active + v_fp + v_sw + v_bp; > + > + /* 75 Mhz pixel clock */ > + cci_update_bits(priv->regmap, MAX96717_IO_CHK0, > + MAX96717_PATTERN_CLK_FREQ, 0xa, &ret); > + > + dev_info(&priv->client->dev, "height: %d width: %d\n", fmt->height, > + fmt->width); > + > + cci_write(priv->regmap, MAX96717_PATGEN_VS_DLY, 0, &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_VS_HIGH, v_sw * h_tot, &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_VS_LOW, > + (v_active + v_fp + v_bp) * h_tot, &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_HS_HIGH, h_sw, &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_HS_LOW, h_active + h_fp + h_bp, > + &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_V2D, > + h_tot * (v_sw + v_bp) + (h_sw + h_bp), &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_HS_CNT, v_tot, &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_DE_HIGH, h_active, &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_DE_LOW, h_fp + h_sw + h_bp, > + &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_DE_CNT, v_active, &ret); > + /* B G R */ > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_A, 0xfecc00, &ret); > + /* B G R */ > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_B, 0x006aa7, &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_A, 0x3c, &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_B, 0x3c, &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_ALT, 0x3c, &ret); > + cci_write(priv->regmap, MAX96717_PATGEN_GRAD_INC, 0x10, &ret); > + > + return ret; > +} > + > +static int max96717_apply_patgen(struct max96717_priv *priv, > + struct v4l2_subdev_state *state) > +{ > + unsigned int val; > + int ret = 0; > + > + if (priv->pattern) > + ret = max96717_apply_patgen_timing(priv, state); > + > + cci_write(priv->regmap, MAX96717_PATGEN_0, priv->pattern ? 0xfb : 0, > + &ret); > + > + val = FIELD_PREP(MAX96717_PATGEN_MODE, priv->pattern); > + cci_update_bits(priv->regmap, MAX96717_PATGEN_1, MAX96717_PATGEN_MODE, > + val, &ret); > + return ret; > +} > + > +static int max96717_s_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct max96717_priv *priv = > + container_of(ctrl->handler, struct max96717_priv, ctrl_handler); > + int ret; > + > + switch (ctrl->id) { > + case V4L2_CID_TEST_PATTERN: > + if (priv->enabled_source_streams) > + return -EBUSY; > + priv->pattern = ctrl->val; > + break; > + default: > + return -EINVAL; > + } > + > + /* Use bpp from bpp register */ > + ret = cci_update_bits(priv->regmap, MAX96717_VIDEO_TX0, > + MAX96717_VIDEO_AUTO_BPP, > + priv->pattern ? 0 : MAX96717_VIDEO_AUTO_BPP, > + NULL); > + > + /* Pattern generator doesn't work with tunnel mode */ Can you add a comment saying that the deserializer should manage the link in pixel mode as well. > + return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11, > + MAX96717_TUN_MODE, > + priv->pattern ? 0 : MAX96717_TUN_MODE, &ret); > +} > + > +static const char * const max96717_test_pattern[] = { > + "Disabled", > + "Checkerboard", > + "Gradient" > +}; > + > +static const struct v4l2_ctrl_ops max96717_ctrl_ops = { > + .s_ctrl = max96717_s_ctrl, > +}; > + > static int max96717_gpiochip_get(struct gpio_chip *gpiochip, > unsigned int offset) > { > @@ -352,20 +496,28 @@ static int max96717_enable_streams(struct v4l2_subdev *sd, > u64 sink_streams; > int ret; > > - sink_streams = v4l2_subdev_state_xlate_streams(state, > - MAX96717_PAD_SOURCE, > - MAX96717_PAD_SINK, > - &streams_mask); > - > if (!priv->enabled_source_streams) > max96717_start_csi(priv, true); > > - ret = v4l2_subdev_enable_streams(priv->source_sd, priv->source_sd_pad, > - sink_streams); > - if (ret) { > - dev_err(dev, "Fail to start streams:%llu on remote subdev\n", > - sink_streams); > + ret = max96717_apply_patgen(priv, state); > + if (ret) > goto stop_csi; > + > + if (!priv->pattern) { > + sink_streams = > + v4l2_subdev_state_xlate_streams(state, > + MAX96717_PAD_SOURCE, > + MAX96717_PAD_SINK, > + &streams_mask); > + > + ret = v4l2_subdev_enable_streams(priv->source_sd, > + priv->source_sd_pad, > + sink_streams); > + if (ret) { > + dev_err(dev, "Fail to start streams:%llu on remote subdev\n", > + sink_streams); > + goto stop_csi; > + } > } > > priv->enabled_source_streams |= streams_mask; > @@ -394,13 +546,23 @@ static int max96717_disable_streams(struct v4l2_subdev *sd, > if (!priv->enabled_source_streams) > max96717_start_csi(priv, false); > > - sink_streams = v4l2_subdev_state_xlate_streams(state, > - MAX96717_PAD_SOURCE, > - MAX96717_PAD_SINK, > - &streams_mask); > + if (!priv->pattern) { > + int ret; > + > + sink_streams = > + v4l2_subdev_state_xlate_streams(state, > + MAX96717_PAD_SOURCE, > + MAX96717_PAD_SINK, > + &streams_mask); > > - return v4l2_subdev_disable_streams(priv->source_sd, priv->source_sd_pad, > - sink_streams); > + ret = v4l2_subdev_disable_streams(priv->source_sd, > + priv->source_sd_pad, > + sink_streams); > + if (ret) > + return ret; > + } > + > + return 0; > } > > static const struct v4l2_subdev_pad_ops max96717_pad_ops = { > @@ -513,6 +675,19 @@ static int max96717_subdev_init(struct max96717_priv *priv) > v4l2_i2c_subdev_init(&priv->sd, priv->client, &max96717_subdev_ops); > priv->sd.internal_ops = &max96717_internal_ops; > > + v4l2_ctrl_handler_init(&priv->ctrl_handler, 1); > + priv->sd.ctrl_handler = &priv->ctrl_handler; > + > + v4l2_ctrl_new_std_menu_items(&priv->ctrl_handler, > + &max96717_ctrl_ops, > + V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(max96717_test_pattern) - 1, > + 0, 0, max96717_test_pattern); > + if (priv->ctrl_handler.error) { > + ret = priv->ctrl_handler.error; > + goto err_free_ctrl; > + } > + > priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; > priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > priv->sd.entity.ops = &max96717_entity_ops; > @@ -552,6 +727,8 @@ static int max96717_subdev_init(struct max96717_priv *priv) > v4l2_subdev_cleanup(&priv->sd); > err_entity_cleanup: > media_entity_cleanup(&priv->sd.entity); > +err_free_ctrl: > + v4l2_ctrl_handler_free(&priv->ctrl_handler); > > return ret; > } > @@ -563,6 +740,7 @@ static void max96717_subdev_uninit(struct max96717_priv *priv) > v4l2_async_nf_cleanup(&priv->notifier); > v4l2_subdev_cleanup(&priv->sd); > media_entity_cleanup(&priv->sd.entity); > + v4l2_ctrl_handler_free(&priv->ctrl_handler); > } > > struct max96717_pll_predef_freq { Regards, Julien ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: i2c: max96717: add test pattern ctrl 2024-06-25 7:54 ` Julien Massot @ 2024-06-25 17:31 ` Tommaso Merciai 2024-06-26 8:37 ` Tommaso Merciai 0 siblings, 1 reply; 6+ messages in thread From: Tommaso Merciai @ 2024-06-25 17:31 UTC (permalink / raw) To: Julien Massot Cc: Tomi Valkeinen, linuxfancy, sakari.ailus, laurent.pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel On Tue, Jun 25, 2024 at 09:54:18AM +0200, Julien Massot wrote: Hi Julien, > Hi Tommaso, > > Thanks for your patch. > > I tested it on my setup and can capture frames. Here's a link to an example: > https://pasteboard.co/j8yHuE4YdYDV.jpg. Nice! Thanks for sharing and testing. > > I had some trouble with link validation because my sensor doesn't support > the RGB format. Once we have internal pad support, we won't need to worry > about the serializer creating an incompatible video stream for the sensor. > > In the future, it would be great if we could support the serializer without > needing a connected sensor. This way, we can use it as a pattern generator > with this patch. However, not all GMSL2 serializers work this way. For > example, the MAX9295A can't generate an internal PCLK and relies on the > sensor to provide the video stream. Fully agree. > > Overall, this patch is very useful as it lets us receive a pattern from the > serializer, which helps validate the GMSL2 connection. > > It also handles issues related to generator timing, bits per pixel (bpp), > and tunnel mode that users might encounter. Fully agree, in my case enabling this test pattern help me a lot on validating the gmsl2 pipe. > > On 6/17/24 4:59 PM, Tommaso Merciai wrote: > > Add v4l2 test pattern control. > > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> > > --- > > drivers/media/i2c/max96717.c | 210 ++++++++++++++++++++++++++++++++--- > > 1 file changed, 194 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c > > index 949306485873..c263bbca7318 100644 > > --- a/drivers/media/i2c/max96717.c > > +++ b/drivers/media/i2c/max96717.c > > @@ -16,6 +16,7 @@ > > #include <linux/regmap.h> > > #include <media/v4l2-cci.h> > > +#include <media/v4l2-ctrls.h> > > #include <media/v4l2-fwnode.h> > > #include <media/v4l2-subdev.h> > > @@ -36,11 +37,37 @@ > > #define MAX96717_DEV_ID CCI_REG8(0xd) > > #define MAX96717_DEV_REV CCI_REG8(0xe) > > #define MAX96717_DEV_REV_MASK GENMASK(3, 0) > > +#define MAX96717_IO_CHK0 CCI_REG8(0x24f) > In MAX96717 datasheet this register is named VTX1. > Can you also move these definitions to the VTX section? > > +#define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1) > > /* VID_TX Z */ > > +#define MAX96717_VIDEO_TX0 CCI_REG8(0x110) > > +#define MAX96717_VIDEO_AUTO_BPP BIT(3) > > #define MAX96717_VIDEO_TX2 CCI_REG8(0x112) > > #define MAX96717_VIDEO_PCLKDET BIT(7) > > +/* VRX_PATGEN_0 */ > For serializer these registers are named VTX_Z instead of VRX. > > > +#define MAX96717_PATGEN_0 CCI_REG8(0x24e) > #define MAX96717_VTX_0 CCI_REG8(0x24e) > > +#define MAX96717_PATGEN_1 CCI_REG8(0x26b) > Can you keep this define ordered by address? > > +#define MAX96717_PATGEN_MODE GENMASK(1, 0) > > +#define MAX96717_PATGEN_VS_DLY CCI_REG24(0x250) > #define MAX96717_VTX_VS_DLY > > +#define MAX96717_PATGEN_VS_HIGH CCI_REG24(0x253) > > +#define MAX96717_PATGEN_VS_LOW CCI_REG24(0x256) > > +#define MAX96717_PATGEN_V2H CCI_REG24(0x259) > > +#define MAX96717_PATGEN_HS_HIGH CCI_REG16(0x25c) > > +#define MAX96717_PATGEN_HS_LOW CCI_REG16(0x25e) > > +#define MAX96717_PATGEN_HS_CNT CCI_REG16(0x260) > > +#define MAX96717_PATGEN_V2D CCI_REG24(0x262) > > +#define MAX96717_PATGEN_DE_HIGH CCI_REG16(0x265) > > +#define MAX96717_PATGEN_DE_LOW CCI_REG16(0x267) > > +#define MAX96717_PATGEN_DE_CNT CCI_REG16(0x269) > > +#define MAX96717_PATGEN_GRAD_INC CCI_REG8(0x26c) > > +#define MAX96717_PATGEN_CHKB_COLOR_A CCI_REG24(0x26d) > > +#define MAX96717_PATGEN_CHKB_COLOR_B CCI_REG24(0x270) > > +#define MAX96717_PATGEN_CHKB_RPT_CNT_A CCI_REG8(0x273) > > +#define MAX96717_PATGEN_CHKB_RPT_CNT_B CCI_REG8(0x274) > > +#define MAX96717_PATGEN_CHKB_ALT CCI_REG8(0x275) > > + So your plan is to move all this stuff here: /* VTX_Z */ #define MAX96717_VTX0 CCI_REG8(0x24e) #define MAX96717_VTX1 CCI_REG8(0x24f) #define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1) #define MAX96717_VTX_VS_DLY CCI_REG24(0x250) #define MAX96717_VTX_VS_HIGH CCI_REG24(0x253) #define MAX96717_VTX_VS_LOW CCI_REG24(0x256) #define MAX96717_VTX_V2H CCI_REG24(0x259) #define MAX96717_VTX_HS_HIGH CCI_REG16(0x25c) #define MAX96717_VTX_HS_LOW CCI_REG16(0x25e) #define MAX96717_VTX_HS_CNT CCI_REG16(0x260) #define MAX96717_VTX_V2D CCI_REG24(0x262) #define MAX96717_VTX_DE_HIGH CCI_REG16(0x265) #define MAX96717_VTX_DE_LOW CCI_REG16(0x267) #define MAX96717_VTX_DE_CNT CCI_REG16(0x269) #define MAX96717_VTX29 CCI_REG8(0x26b) #define MAX96717_VTX_MODE GENMASK(1, 0) #define MAX96717_VTX_GRAD_INC CCI_REG8(0x26c) #define MAX96717_VTX_CHKB_COLOR_A CCI_REG24(0x26d) #define MAX96717_VTX_CHKB_COLOR_B CCI_REG24(0x270) #define MAX96717_VTX_CHKB_RPT_CNT_A CCI_REG8(0x273) #define MAX96717_VTX_CHKB_RPT_CNT_B CCI_REG8(0x274) #define MAX96717_VTX_CHKB_ALT CCI_REG8(0x275) In a fixed order right? :-) > > /* GPIO */ > > #define MAX96717_NUM_GPIO 11 > > #define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3) > > @@ -82,6 +109,12 @@ > > /* MISC */ > > #define PIO_SLEW_1 CCI_REG8(0x570) > > +enum max96717_vpg_mode { > > + MAX96717_VPG_DISABLED = 0, > > + MAX96717_VPG_CHECKERBOARD = 1, > > + MAX96717_VPG_GRADIENT = 2, > > +}; > > + > > struct max96717_priv { > > struct i2c_client *client; > > struct regmap *regmap; > > @@ -89,6 +122,7 @@ struct max96717_priv { > > struct v4l2_mbus_config_mipi_csi2 mipi_csi2; > > struct v4l2_subdev sd; > > struct media_pad pads[MAX96717_PORTS]; > > + struct v4l2_ctrl_handler ctrl_handler; > > struct v4l2_async_notifier notifier; > > struct v4l2_subdev *source_sd; > > u16 source_sd_pad; > > @@ -96,6 +130,7 @@ struct max96717_priv { > > u8 pll_predef_index; > > struct clk_hw clk_hw; > > struct gpio_chip gpio_chip; > > + enum max96717_vpg_mode pattern; > > }; > > static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd) > > @@ -131,6 +166,115 @@ static inline int max96717_start_csi(struct max96717_priv *priv, bool start) > > start ? MAX96717_START_PORT_B : 0, NULL); > > } > > +static int max96717_apply_patgen_timing(struct max96717_priv *priv, > > + struct v4l2_subdev_state *state) > > +{ > > + struct v4l2_mbus_framefmt *fmt = > > + v4l2_subdev_state_get_format(state, MAX96717_PAD_SOURCE); > > + const u32 h_active = fmt->width; > > + const u32 h_fp = 88; > > + const u32 h_sw = 44; > > + const u32 h_bp = 148; > > + u32 h_tot; > > + const u32 v_active = fmt->height; > > + const u32 v_fp = 4; > > + const u32 v_sw = 5; > > + const u32 v_bp = 36; > > + u32 v_tot; > > + int ret = 0; > > + > > + h_tot = h_active + h_fp + h_sw + h_bp; > > + v_tot = v_active + v_fp + v_sw + v_bp; > > + > > + /* 75 Mhz pixel clock */ > > + cci_update_bits(priv->regmap, MAX96717_IO_CHK0, > > + MAX96717_PATTERN_CLK_FREQ, 0xa, &ret); > > + > > + dev_info(&priv->client->dev, "height: %d width: %d\n", fmt->height, > > + fmt->width); > > + > > + cci_write(priv->regmap, MAX96717_PATGEN_VS_DLY, 0, &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_VS_HIGH, v_sw * h_tot, &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_VS_LOW, > > + (v_active + v_fp + v_bp) * h_tot, &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_HS_HIGH, h_sw, &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_HS_LOW, h_active + h_fp + h_bp, > > + &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_V2D, > > + h_tot * (v_sw + v_bp) + (h_sw + h_bp), &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_HS_CNT, v_tot, &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_DE_HIGH, h_active, &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_DE_LOW, h_fp + h_sw + h_bp, > > + &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_DE_CNT, v_active, &ret); > > + /* B G R */ > > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_A, 0xfecc00, &ret); > > + /* B G R */ > > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_B, 0x006aa7, &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_A, 0x3c, &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_B, 0x3c, &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_ALT, 0x3c, &ret); > > + cci_write(priv->regmap, MAX96717_PATGEN_GRAD_INC, 0x10, &ret); > > + > > + return ret; > > +} > > + > > +static int max96717_apply_patgen(struct max96717_priv *priv, > > + struct v4l2_subdev_state *state) > > +{ > > + unsigned int val; > > + int ret = 0; > > + > > + if (priv->pattern) > > + ret = max96717_apply_patgen_timing(priv, state); > > + > > + cci_write(priv->regmap, MAX96717_PATGEN_0, priv->pattern ? 0xfb : 0, > > + &ret); > > + > > + val = FIELD_PREP(MAX96717_PATGEN_MODE, priv->pattern); > > + cci_update_bits(priv->regmap, MAX96717_PATGEN_1, MAX96717_PATGEN_MODE, > > + val, &ret); > > + return ret; > > +} > > + > > +static int max96717_s_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct max96717_priv *priv = > > + container_of(ctrl->handler, struct max96717_priv, ctrl_handler); > > + int ret; > > + > > + switch (ctrl->id) { > > + case V4L2_CID_TEST_PATTERN: > > + if (priv->enabled_source_streams) > > + return -EBUSY; > > + priv->pattern = ctrl->val; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + /* Use bpp from bpp register */ > > + ret = cci_update_bits(priv->regmap, MAX96717_VIDEO_TX0, > > + MAX96717_VIDEO_AUTO_BPP, > > + priv->pattern ? 0 : MAX96717_VIDEO_AUTO_BPP, > > + NULL); > > + > > + /* Pattern generator doesn't work with tunnel mode */ > Can you add a comment saying that the deserializer should manage the link in > pixel mode as well. Something like: /* Pattern generator doesn't work with tunnel mode. Is mandatory to put also the deserializer into pixel mode. */ What do you think? Thanks & Regards, Tommaso > > + return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11, > > + MAX96717_TUN_MODE, > > + priv->pattern ? 0 : MAX96717_TUN_MODE, &ret); > > +} > > + > > +static const char * const max96717_test_pattern[] = { > > + "Disabled", > > + "Checkerboard", > > + "Gradient" > > +}; > > + > > +static const struct v4l2_ctrl_ops max96717_ctrl_ops = { > > + .s_ctrl = max96717_s_ctrl, > > +}; > > + > > static int max96717_gpiochip_get(struct gpio_chip *gpiochip, > > unsigned int offset) > > { > > @@ -352,20 +496,28 @@ static int max96717_enable_streams(struct v4l2_subdev *sd, > > u64 sink_streams; > > int ret; > > - sink_streams = v4l2_subdev_state_xlate_streams(state, > > - MAX96717_PAD_SOURCE, > > - MAX96717_PAD_SINK, > > - &streams_mask); > > - > > if (!priv->enabled_source_streams) > > max96717_start_csi(priv, true); > > - ret = v4l2_subdev_enable_streams(priv->source_sd, priv->source_sd_pad, > > - sink_streams); > > - if (ret) { > > - dev_err(dev, "Fail to start streams:%llu on remote subdev\n", > > - sink_streams); > > + ret = max96717_apply_patgen(priv, state); > > + if (ret) > > goto stop_csi; > > + > > + if (!priv->pattern) { > > + sink_streams = > > + v4l2_subdev_state_xlate_streams(state, > > + MAX96717_PAD_SOURCE, > > + MAX96717_PAD_SINK, > > + &streams_mask); > > + > > + ret = v4l2_subdev_enable_streams(priv->source_sd, > > + priv->source_sd_pad, > > + sink_streams); > > + if (ret) { > > + dev_err(dev, "Fail to start streams:%llu on remote subdev\n", > > + sink_streams); > > + goto stop_csi; > > + } > > } > > priv->enabled_source_streams |= streams_mask; > > @@ -394,13 +546,23 @@ static int max96717_disable_streams(struct v4l2_subdev *sd, > > if (!priv->enabled_source_streams) > > max96717_start_csi(priv, false); > > - sink_streams = v4l2_subdev_state_xlate_streams(state, > > - MAX96717_PAD_SOURCE, > > - MAX96717_PAD_SINK, > > - &streams_mask); > > + if (!priv->pattern) { > > + int ret; > > + > > + sink_streams = > > + v4l2_subdev_state_xlate_streams(state, > > + MAX96717_PAD_SOURCE, > > + MAX96717_PAD_SINK, > > + &streams_mask); > > - return v4l2_subdev_disable_streams(priv->source_sd, priv->source_sd_pad, > > - sink_streams); > > + ret = v4l2_subdev_disable_streams(priv->source_sd, > > + priv->source_sd_pad, > > + sink_streams); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > } > > static const struct v4l2_subdev_pad_ops max96717_pad_ops = { > > @@ -513,6 +675,19 @@ static int max96717_subdev_init(struct max96717_priv *priv) > > v4l2_i2c_subdev_init(&priv->sd, priv->client, &max96717_subdev_ops); > > priv->sd.internal_ops = &max96717_internal_ops; > > + v4l2_ctrl_handler_init(&priv->ctrl_handler, 1); > > + priv->sd.ctrl_handler = &priv->ctrl_handler; > > + > > + v4l2_ctrl_new_std_menu_items(&priv->ctrl_handler, > > + &max96717_ctrl_ops, > > + V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(max96717_test_pattern) - 1, > > + 0, 0, max96717_test_pattern); > > + if (priv->ctrl_handler.error) { > > + ret = priv->ctrl_handler.error; > > + goto err_free_ctrl; > > + } > > + > > priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; > > priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > > priv->sd.entity.ops = &max96717_entity_ops; > > @@ -552,6 +727,8 @@ static int max96717_subdev_init(struct max96717_priv *priv) > > v4l2_subdev_cleanup(&priv->sd); > > err_entity_cleanup: > > media_entity_cleanup(&priv->sd.entity); > > +err_free_ctrl: > > + v4l2_ctrl_handler_free(&priv->ctrl_handler); > > return ret; > > } > > @@ -563,6 +740,7 @@ static void max96717_subdev_uninit(struct max96717_priv *priv) > > v4l2_async_nf_cleanup(&priv->notifier); > > v4l2_subdev_cleanup(&priv->sd); > > media_entity_cleanup(&priv->sd.entity); > > + v4l2_ctrl_handler_free(&priv->ctrl_handler); > > } > > struct max96717_pll_predef_freq { > > Regards, > Julien ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: i2c: max96717: add test pattern ctrl 2024-06-25 17:31 ` Tommaso Merciai @ 2024-06-26 8:37 ` Tommaso Merciai 2024-06-27 12:37 ` Julien Massot 0 siblings, 1 reply; 6+ messages in thread From: Tommaso Merciai @ 2024-06-26 8:37 UTC (permalink / raw) To: Julien Massot Cc: Tomi Valkeinen, linuxfancy, sakari.ailus, laurent.pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel On Tue, Jun 25, 2024 at 07:31:05PM +0200, Tommaso Merciai wrote: > On Tue, Jun 25, 2024 at 09:54:18AM +0200, Julien Massot wrote: > > Hi Julien, > > > Hi Tommaso, > > > > Thanks for your patch. > > > > I tested it on my setup and can capture frames. Here's a link to an example: > > https://pasteboard.co/j8yHuE4YdYDV.jpg. > > Nice! Thanks for sharing and testing. > > > > > I had some trouble with link validation because my sensor doesn't support > > the RGB format. Once we have internal pad support, we won't need to worry > > about the serializer creating an incompatible video stream for the sensor. > > > > In the future, it would be great if we could support the serializer without > > needing a connected sensor. This way, we can use it as a pattern generator > > with this patch. However, not all GMSL2 serializers work this way. For > > example, the MAX9295A can't generate an internal PCLK and relies on the > > sensor to provide the video stream. > > Fully agree. > > > > > Overall, this patch is very useful as it lets us receive a pattern from the > > serializer, which helps validate the GMSL2 connection. > > > > It also handles issues related to generator timing, bits per pixel (bpp), > > and tunnel mode that users might encounter. > > Fully agree, in my case enabling this test pattern help me a lot on > validating the gmsl2 pipe. > > > > > On 6/17/24 4:59 PM, Tommaso Merciai wrote: > > > Add v4l2 test pattern control. > > > > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> > > > --- > > > drivers/media/i2c/max96717.c | 210 ++++++++++++++++++++++++++++++++--- > > > 1 file changed, 194 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c > > > index 949306485873..c263bbca7318 100644 > > > --- a/drivers/media/i2c/max96717.c > > > +++ b/drivers/media/i2c/max96717.c > > > @@ -16,6 +16,7 @@ > > > #include <linux/regmap.h> > > > #include <media/v4l2-cci.h> > > > +#include <media/v4l2-ctrls.h> > > > #include <media/v4l2-fwnode.h> > > > #include <media/v4l2-subdev.h> > > > @@ -36,11 +37,37 @@ > > > #define MAX96717_DEV_ID CCI_REG8(0xd) > > > #define MAX96717_DEV_REV CCI_REG8(0xe) > > > #define MAX96717_DEV_REV_MASK GENMASK(3, 0) > > > +#define MAX96717_IO_CHK0 CCI_REG8(0x24f) > > In MAX96717 datasheet this register is named VTX1. > > Can you also move these definitions to the VTX section? > > > +#define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1) > > > /* VID_TX Z */ > > > +#define MAX96717_VIDEO_TX0 CCI_REG8(0x110) > > > +#define MAX96717_VIDEO_AUTO_BPP BIT(3) > > > #define MAX96717_VIDEO_TX2 CCI_REG8(0x112) > > > #define MAX96717_VIDEO_PCLKDET BIT(7) > > > +/* VRX_PATGEN_0 */ > > For serializer these registers are named VTX_Z instead of VRX. > > > > > +#define MAX96717_PATGEN_0 CCI_REG8(0x24e) > > #define MAX96717_VTX_0 CCI_REG8(0x24e) > > > +#define MAX96717_PATGEN_1 CCI_REG8(0x26b) > > Can you keep this define ordered by address? > > > +#define MAX96717_PATGEN_MODE GENMASK(1, 0) > > > +#define MAX96717_PATGEN_VS_DLY CCI_REG24(0x250) > > #define MAX96717_VTX_VS_DLY > > > +#define MAX96717_PATGEN_VS_HIGH CCI_REG24(0x253) > > > +#define MAX96717_PATGEN_VS_LOW CCI_REG24(0x256) > > > +#define MAX96717_PATGEN_V2H CCI_REG24(0x259) > > > +#define MAX96717_PATGEN_HS_HIGH CCI_REG16(0x25c) > > > +#define MAX96717_PATGEN_HS_LOW CCI_REG16(0x25e) > > > +#define MAX96717_PATGEN_HS_CNT CCI_REG16(0x260) > > > +#define MAX96717_PATGEN_V2D CCI_REG24(0x262) > > > +#define MAX96717_PATGEN_DE_HIGH CCI_REG16(0x265) > > > +#define MAX96717_PATGEN_DE_LOW CCI_REG16(0x267) > > > +#define MAX96717_PATGEN_DE_CNT CCI_REG16(0x269) > > > +#define MAX96717_PATGEN_GRAD_INC CCI_REG8(0x26c) > > > +#define MAX96717_PATGEN_CHKB_COLOR_A CCI_REG24(0x26d) > > > +#define MAX96717_PATGEN_CHKB_COLOR_B CCI_REG24(0x270) > > > +#define MAX96717_PATGEN_CHKB_RPT_CNT_A CCI_REG8(0x273) > > > +#define MAX96717_PATGEN_CHKB_RPT_CNT_B CCI_REG8(0x274) > > > +#define MAX96717_PATGEN_CHKB_ALT CCI_REG8(0x275) > > > + > > So your plan is to move all this stuff here: > > > /* VTX_Z */ > #define MAX96717_VTX0 CCI_REG8(0x24e) > #define MAX96717_VTX1 CCI_REG8(0x24f) > #define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1) > #define MAX96717_VTX_VS_DLY CCI_REG24(0x250) > #define MAX96717_VTX_VS_HIGH CCI_REG24(0x253) > #define MAX96717_VTX_VS_LOW CCI_REG24(0x256) > #define MAX96717_VTX_V2H CCI_REG24(0x259) > #define MAX96717_VTX_HS_HIGH CCI_REG16(0x25c) > #define MAX96717_VTX_HS_LOW CCI_REG16(0x25e) > #define MAX96717_VTX_HS_CNT CCI_REG16(0x260) > #define MAX96717_VTX_V2D CCI_REG24(0x262) > #define MAX96717_VTX_DE_HIGH CCI_REG16(0x265) > #define MAX96717_VTX_DE_LOW CCI_REG16(0x267) > #define MAX96717_VTX_DE_CNT CCI_REG16(0x269) > #define MAX96717_VTX29 CCI_REG8(0x26b) > #define MAX96717_VTX_MODE GENMASK(1, 0) > #define MAX96717_VTX_GRAD_INC CCI_REG8(0x26c) > #define MAX96717_VTX_CHKB_COLOR_A CCI_REG24(0x26d) > #define MAX96717_VTX_CHKB_COLOR_B CCI_REG24(0x270) > #define MAX96717_VTX_CHKB_RPT_CNT_A CCI_REG8(0x273) > #define MAX96717_VTX_CHKB_RPT_CNT_B CCI_REG8(0x274) > #define MAX96717_VTX_CHKB_ALT CCI_REG8(0x275) > > In a fixed order right? :-) > > > > > > /* GPIO */ > > > #define MAX96717_NUM_GPIO 11 > > > #define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3) > > > @@ -82,6 +109,12 @@ > > > /* MISC */ > > > #define PIO_SLEW_1 CCI_REG8(0x570) > > > +enum max96717_vpg_mode { > > > + MAX96717_VPG_DISABLED = 0, > > > + MAX96717_VPG_CHECKERBOARD = 1, > > > + MAX96717_VPG_GRADIENT = 2, > > > +}; > > > + > > > struct max96717_priv { > > > struct i2c_client *client; > > > struct regmap *regmap; > > > @@ -89,6 +122,7 @@ struct max96717_priv { > > > struct v4l2_mbus_config_mipi_csi2 mipi_csi2; > > > struct v4l2_subdev sd; > > > struct media_pad pads[MAX96717_PORTS]; > > > + struct v4l2_ctrl_handler ctrl_handler; > > > struct v4l2_async_notifier notifier; > > > struct v4l2_subdev *source_sd; > > > u16 source_sd_pad; > > > @@ -96,6 +130,7 @@ struct max96717_priv { > > > u8 pll_predef_index; > > > struct clk_hw clk_hw; > > > struct gpio_chip gpio_chip; > > > + enum max96717_vpg_mode pattern; > > > }; > > > static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd) > > > @@ -131,6 +166,115 @@ static inline int max96717_start_csi(struct max96717_priv *priv, bool start) > > > start ? MAX96717_START_PORT_B : 0, NULL); > > > } > > > +static int max96717_apply_patgen_timing(struct max96717_priv *priv, > > > + struct v4l2_subdev_state *state) > > > +{ > > > + struct v4l2_mbus_framefmt *fmt = > > > + v4l2_subdev_state_get_format(state, MAX96717_PAD_SOURCE); > > > + const u32 h_active = fmt->width; > > > + const u32 h_fp = 88; > > > + const u32 h_sw = 44; > > > + const u32 h_bp = 148; > > > + u32 h_tot; > > > + const u32 v_active = fmt->height; > > > + const u32 v_fp = 4; > > > + const u32 v_sw = 5; > > > + const u32 v_bp = 36; > > > + u32 v_tot; > > > + int ret = 0; > > > + > > > + h_tot = h_active + h_fp + h_sw + h_bp; > > > + v_tot = v_active + v_fp + v_sw + v_bp; > > > + > > > + /* 75 Mhz pixel clock */ > > > + cci_update_bits(priv->regmap, MAX96717_IO_CHK0, > > > + MAX96717_PATTERN_CLK_FREQ, 0xa, &ret); > > > + > > > + dev_info(&priv->client->dev, "height: %d width: %d\n", fmt->height, > > > + fmt->width); > > > + > > > + cci_write(priv->regmap, MAX96717_PATGEN_VS_DLY, 0, &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_VS_HIGH, v_sw * h_tot, &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_VS_LOW, > > > + (v_active + v_fp + v_bp) * h_tot, &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_HS_HIGH, h_sw, &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_HS_LOW, h_active + h_fp + h_bp, > > > + &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_V2D, > > > + h_tot * (v_sw + v_bp) + (h_sw + h_bp), &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_HS_CNT, v_tot, &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_DE_HIGH, h_active, &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_DE_LOW, h_fp + h_sw + h_bp, > > > + &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_DE_CNT, v_active, &ret); > > > + /* B G R */ > > > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_A, 0xfecc00, &ret); > > > + /* B G R */ > > > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_B, 0x006aa7, &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_A, 0x3c, &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_B, 0x3c, &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_ALT, 0x3c, &ret); > > > + cci_write(priv->regmap, MAX96717_PATGEN_GRAD_INC, 0x10, &ret); > > > + > > > + return ret; > > > +} > > > + > > > +static int max96717_apply_patgen(struct max96717_priv *priv, > > > + struct v4l2_subdev_state *state) > > > +{ > > > + unsigned int val; > > > + int ret = 0; > > > + > > > + if (priv->pattern) > > > + ret = max96717_apply_patgen_timing(priv, state); > > > + > > > + cci_write(priv->regmap, MAX96717_PATGEN_0, priv->pattern ? 0xfb : 0, > > > + &ret); > > > + > > > + val = FIELD_PREP(MAX96717_PATGEN_MODE, priv->pattern); > > > + cci_update_bits(priv->regmap, MAX96717_PATGEN_1, MAX96717_PATGEN_MODE, > > > + val, &ret); > > > + return ret; > > > +} > > > + > > > +static int max96717_s_ctrl(struct v4l2_ctrl *ctrl) > > > +{ > > > + struct max96717_priv *priv = > > > + container_of(ctrl->handler, struct max96717_priv, ctrl_handler); > > > + int ret; > > > + > > > + switch (ctrl->id) { > > > + case V4L2_CID_TEST_PATTERN: > > > + if (priv->enabled_source_streams) > > > + return -EBUSY; > > > + priv->pattern = ctrl->val; > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + /* Use bpp from bpp register */ > > > + ret = cci_update_bits(priv->regmap, MAX96717_VIDEO_TX0, > > > + MAX96717_VIDEO_AUTO_BPP, > > > + priv->pattern ? 0 : MAX96717_VIDEO_AUTO_BPP, > > > + NULL); > > > + > > > + /* Pattern generator doesn't work with tunnel mode */ > > Can you add a comment saying that the deserializer should manage the link in > > pixel mode as well. > > Something like: > /* Pattern generator doesn't work with tunnel mode. > Is mandatory to put also the deserializer into pixel mode. > */ Actually I plan to add to add the following comment: /* * Pattern generator doesn't work with tunnel mode. * Needs RGB color format and deserializer tunnel mode must be disabled. */ return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11, MAX96717_TUN_MODE, priv->pattern ? 0 : MAX96717_TUN_MODE, &ret); Let me know if this can be ok for you. Thanks & Regards, Tommaso > > What do you think? > > Thanks & Regards, > Tommaso > > > > + return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11, > > > + MAX96717_TUN_MODE, > > > + priv->pattern ? 0 : MAX96717_TUN_MODE, &ret); > > > +} > > > + > > > +static const char * const max96717_test_pattern[] = { > > > + "Disabled", > > > + "Checkerboard", > > > + "Gradient" > > > +}; > > > + > > > +static const struct v4l2_ctrl_ops max96717_ctrl_ops = { > > > + .s_ctrl = max96717_s_ctrl, > > > +}; > > > + > > > static int max96717_gpiochip_get(struct gpio_chip *gpiochip, > > > unsigned int offset) > > > { > > > @@ -352,20 +496,28 @@ static int max96717_enable_streams(struct v4l2_subdev *sd, > > > u64 sink_streams; > > > int ret; > > > - sink_streams = v4l2_subdev_state_xlate_streams(state, > > > - MAX96717_PAD_SOURCE, > > > - MAX96717_PAD_SINK, > > > - &streams_mask); > > > - > > > if (!priv->enabled_source_streams) > > > max96717_start_csi(priv, true); > > > - ret = v4l2_subdev_enable_streams(priv->source_sd, priv->source_sd_pad, > > > - sink_streams); > > > - if (ret) { > > > - dev_err(dev, "Fail to start streams:%llu on remote subdev\n", > > > - sink_streams); > > > + ret = max96717_apply_patgen(priv, state); > > > + if (ret) > > > goto stop_csi; > > > + > > > + if (!priv->pattern) { > > > + sink_streams = > > > + v4l2_subdev_state_xlate_streams(state, > > > + MAX96717_PAD_SOURCE, > > > + MAX96717_PAD_SINK, > > > + &streams_mask); > > > + > > > + ret = v4l2_subdev_enable_streams(priv->source_sd, > > > + priv->source_sd_pad, > > > + sink_streams); > > > + if (ret) { > > > + dev_err(dev, "Fail to start streams:%llu on remote subdev\n", > > > + sink_streams); > > > + goto stop_csi; > > > + } > > > } > > > priv->enabled_source_streams |= streams_mask; > > > @@ -394,13 +546,23 @@ static int max96717_disable_streams(struct v4l2_subdev *sd, > > > if (!priv->enabled_source_streams) > > > max96717_start_csi(priv, false); > > > - sink_streams = v4l2_subdev_state_xlate_streams(state, > > > - MAX96717_PAD_SOURCE, > > > - MAX96717_PAD_SINK, > > > - &streams_mask); > > > + if (!priv->pattern) { > > > + int ret; > > > + > > > + sink_streams = > > > + v4l2_subdev_state_xlate_streams(state, > > > + MAX96717_PAD_SOURCE, > > > + MAX96717_PAD_SINK, > > > + &streams_mask); > > > - return v4l2_subdev_disable_streams(priv->source_sd, priv->source_sd_pad, > > > - sink_streams); > > > + ret = v4l2_subdev_disable_streams(priv->source_sd, > > > + priv->source_sd_pad, > > > + sink_streams); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + return 0; > > > } > > > static const struct v4l2_subdev_pad_ops max96717_pad_ops = { > > > @@ -513,6 +675,19 @@ static int max96717_subdev_init(struct max96717_priv *priv) > > > v4l2_i2c_subdev_init(&priv->sd, priv->client, &max96717_subdev_ops); > > > priv->sd.internal_ops = &max96717_internal_ops; > > > + v4l2_ctrl_handler_init(&priv->ctrl_handler, 1); > > > + priv->sd.ctrl_handler = &priv->ctrl_handler; > > > + > > > + v4l2_ctrl_new_std_menu_items(&priv->ctrl_handler, > > > + &max96717_ctrl_ops, > > > + V4L2_CID_TEST_PATTERN, > > > + ARRAY_SIZE(max96717_test_pattern) - 1, > > > + 0, 0, max96717_test_pattern); > > > + if (priv->ctrl_handler.error) { > > > + ret = priv->ctrl_handler.error; > > > + goto err_free_ctrl; > > > + } > > > + > > > priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; > > > priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > > > priv->sd.entity.ops = &max96717_entity_ops; > > > @@ -552,6 +727,8 @@ static int max96717_subdev_init(struct max96717_priv *priv) > > > v4l2_subdev_cleanup(&priv->sd); > > > err_entity_cleanup: > > > media_entity_cleanup(&priv->sd.entity); > > > +err_free_ctrl: > > > + v4l2_ctrl_handler_free(&priv->ctrl_handler); > > > return ret; > > > } > > > @@ -563,6 +740,7 @@ static void max96717_subdev_uninit(struct max96717_priv *priv) > > > v4l2_async_nf_cleanup(&priv->notifier); > > > v4l2_subdev_cleanup(&priv->sd); > > > media_entity_cleanup(&priv->sd.entity); > > > + v4l2_ctrl_handler_free(&priv->ctrl_handler); > > > } > > > struct max96717_pll_predef_freq { > > > > Regards, > > Julien ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] media: i2c: max96717: add test pattern ctrl 2024-06-26 8:37 ` Tommaso Merciai @ 2024-06-27 12:37 ` Julien Massot 0 siblings, 0 replies; 6+ messages in thread From: Julien Massot @ 2024-06-27 12:37 UTC (permalink / raw) To: Tommaso Merciai Cc: Tomi Valkeinen, linuxfancy, sakari.ailus, laurent.pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel Hi Tommaso, On 6/26/24 10:37 AM, Tommaso Merciai wrote: > On Tue, Jun 25, 2024 at 07:31:05PM +0200, Tommaso Merciai wrote: >> On Tue, Jun 25, 2024 at 09:54:18AM +0200, Julien Massot wrote: >> >> Hi Julien, >> >>> Hi Tommaso, >>> >>> Thanks for your patch. >>> >>> I tested it on my setup and can capture frames. Here's a link to an example: >>> https://pasteboard.co/j8yHuE4YdYDV.jpg. >> >> Nice! Thanks for sharing and testing. >> >>> >>> I had some trouble with link validation because my sensor doesn't support >>> the RGB format. Once we have internal pad support, we won't need to worry >>> about the serializer creating an incompatible video stream for the sensor. >>> >>> In the future, it would be great if we could support the serializer without >>> needing a connected sensor. This way, we can use it as a pattern generator >>> with this patch. However, not all GMSL2 serializers work this way. For >>> example, the MAX9295A can't generate an internal PCLK and relies on the >>> sensor to provide the video stream. >> >> Fully agree. >> >>> >>> Overall, this patch is very useful as it lets us receive a pattern from the >>> serializer, which helps validate the GMSL2 connection. >>> >>> It also handles issues related to generator timing, bits per pixel (bpp), >>> and tunnel mode that users might encounter. >> >> Fully agree, in my case enabling this test pattern help me a lot on >> validating the gmsl2 pipe. >> >>> >>> On 6/17/24 4:59 PM, Tommaso Merciai wrote: >>>> Add v4l2 test pattern control. >>>> >>>> Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> >>>> --- >>>> drivers/media/i2c/max96717.c | 210 ++++++++++++++++++++++++++++++++--- >>>> 1 file changed, 194 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c >>>> index 949306485873..c263bbca7318 100644 >>>> --- a/drivers/media/i2c/max96717.c >>>> +++ b/drivers/media/i2c/max96717.c >>>> @@ -16,6 +16,7 @@ >>>> #include <linux/regmap.h> >>>> #include <media/v4l2-cci.h> >>>> +#include <media/v4l2-ctrls.h> >>>> #include <media/v4l2-fwnode.h> >>>> #include <media/v4l2-subdev.h> >>>> @@ -36,11 +37,37 @@ >>>> #define MAX96717_DEV_ID CCI_REG8(0xd) >>>> #define MAX96717_DEV_REV CCI_REG8(0xe) >>>> #define MAX96717_DEV_REV_MASK GENMASK(3, 0) >>>> +#define MAX96717_IO_CHK0 CCI_REG8(0x24f) >>> In MAX96717 datasheet this register is named VTX1. >>> Can you also move these definitions to the VTX section? >>>> +#define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1) >>>> /* VID_TX Z */ >>>> +#define MAX96717_VIDEO_TX0 CCI_REG8(0x110) >>>> +#define MAX96717_VIDEO_AUTO_BPP BIT(3) >>>> #define MAX96717_VIDEO_TX2 CCI_REG8(0x112) >>>> #define MAX96717_VIDEO_PCLKDET BIT(7) >>>> +/* VRX_PATGEN_0 */ >>> For serializer these registers are named VTX_Z instead of VRX. >>> >>>> +#define MAX96717_PATGEN_0 CCI_REG8(0x24e) >>> #define MAX96717_VTX_0 CCI_REG8(0x24e) >>>> +#define MAX96717_PATGEN_1 CCI_REG8(0x26b) >>> Can you keep this define ordered by address? >>>> +#define MAX96717_PATGEN_MODE GENMASK(1, 0) >>>> +#define MAX96717_PATGEN_VS_DLY CCI_REG24(0x250) >>> #define MAX96717_VTX_VS_DLY >>>> +#define MAX96717_PATGEN_VS_HIGH CCI_REG24(0x253) >>>> +#define MAX96717_PATGEN_VS_LOW CCI_REG24(0x256) >>>> +#define MAX96717_PATGEN_V2H CCI_REG24(0x259) >>>> +#define MAX96717_PATGEN_HS_HIGH CCI_REG16(0x25c) >>>> +#define MAX96717_PATGEN_HS_LOW CCI_REG16(0x25e) >>>> +#define MAX96717_PATGEN_HS_CNT CCI_REG16(0x260) >>>> +#define MAX96717_PATGEN_V2D CCI_REG24(0x262) >>>> +#define MAX96717_PATGEN_DE_HIGH CCI_REG16(0x265) >>>> +#define MAX96717_PATGEN_DE_LOW CCI_REG16(0x267) >>>> +#define MAX96717_PATGEN_DE_CNT CCI_REG16(0x269) >>>> +#define MAX96717_PATGEN_GRAD_INC CCI_REG8(0x26c) >>>> +#define MAX96717_PATGEN_CHKB_COLOR_A CCI_REG24(0x26d) >>>> +#define MAX96717_PATGEN_CHKB_COLOR_B CCI_REG24(0x270) >>>> +#define MAX96717_PATGEN_CHKB_RPT_CNT_A CCI_REG8(0x273) >>>> +#define MAX96717_PATGEN_CHKB_RPT_CNT_B CCI_REG8(0x274) >>>> +#define MAX96717_PATGEN_CHKB_ALT CCI_REG8(0x275) >>>> + >> >> So your plan is to move all this stuff here: >> >> >> /* VTX_Z */ >> #define MAX96717_VTX0 CCI_REG8(0x24e) >> #define MAX96717_VTX1 CCI_REG8(0x24f) >> #define MAX96717_PATTERN_CLK_FREQ GENMASK(3, 1) >> #define MAX96717_VTX_VS_DLY CCI_REG24(0x250) >> #define MAX96717_VTX_VS_HIGH CCI_REG24(0x253) >> #define MAX96717_VTX_VS_LOW CCI_REG24(0x256) >> #define MAX96717_VTX_V2H CCI_REG24(0x259) >> #define MAX96717_VTX_HS_HIGH CCI_REG16(0x25c) >> #define MAX96717_VTX_HS_LOW CCI_REG16(0x25e) >> #define MAX96717_VTX_HS_CNT CCI_REG16(0x260) >> #define MAX96717_VTX_V2D CCI_REG24(0x262) >> #define MAX96717_VTX_DE_HIGH CCI_REG16(0x265) >> #define MAX96717_VTX_DE_LOW CCI_REG16(0x267) >> #define MAX96717_VTX_DE_CNT CCI_REG16(0x269) >> #define MAX96717_VTX29 CCI_REG8(0x26b) >> #define MAX96717_VTX_MODE GENMASK(1, 0) >> #define MAX96717_VTX_GRAD_INC CCI_REG8(0x26c) >> #define MAX96717_VTX_CHKB_COLOR_A CCI_REG24(0x26d) >> #define MAX96717_VTX_CHKB_COLOR_B CCI_REG24(0x270) >> #define MAX96717_VTX_CHKB_RPT_CNT_A CCI_REG8(0x273) >> #define MAX96717_VTX_CHKB_RPT_CNT_B CCI_REG8(0x274) >> #define MAX96717_VTX_CHKB_ALT CCI_REG8(0x275) >> >> In a fixed order right? :-) Yes sounds good to me >> >> >> >>>> /* GPIO */ >>>> #define MAX96717_NUM_GPIO 11 >>>> #define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3) >>>> @@ -82,6 +109,12 @@ >>>> /* MISC */ >>>> #define PIO_SLEW_1 CCI_REG8(0x570) >>>> +enum max96717_vpg_mode { >>>> + MAX96717_VPG_DISABLED = 0, >>>> + MAX96717_VPG_CHECKERBOARD = 1, >>>> + MAX96717_VPG_GRADIENT = 2, >>>> +}; >>>> + >>>> struct max96717_priv { >>>> struct i2c_client *client; >>>> struct regmap *regmap; >>>> @@ -89,6 +122,7 @@ struct max96717_priv { >>>> struct v4l2_mbus_config_mipi_csi2 mipi_csi2; >>>> struct v4l2_subdev sd; >>>> struct media_pad pads[MAX96717_PORTS]; >>>> + struct v4l2_ctrl_handler ctrl_handler; >>>> struct v4l2_async_notifier notifier; >>>> struct v4l2_subdev *source_sd; >>>> u16 source_sd_pad; >>>> @@ -96,6 +130,7 @@ struct max96717_priv { >>>> u8 pll_predef_index; >>>> struct clk_hw clk_hw; >>>> struct gpio_chip gpio_chip; >>>> + enum max96717_vpg_mode pattern; >>>> }; >>>> static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd) >>>> @@ -131,6 +166,115 @@ static inline int max96717_start_csi(struct max96717_priv *priv, bool start) >>>> start ? MAX96717_START_PORT_B : 0, NULL); >>>> } >>>> +static int max96717_apply_patgen_timing(struct max96717_priv *priv, >>>> + struct v4l2_subdev_state *state) >>>> +{ >>>> + struct v4l2_mbus_framefmt *fmt = >>>> + v4l2_subdev_state_get_format(state, MAX96717_PAD_SOURCE); >>>> + const u32 h_active = fmt->width; >>>> + const u32 h_fp = 88; >>>> + const u32 h_sw = 44; >>>> + const u32 h_bp = 148; >>>> + u32 h_tot; >>>> + const u32 v_active = fmt->height; >>>> + const u32 v_fp = 4; >>>> + const u32 v_sw = 5; >>>> + const u32 v_bp = 36; >>>> + u32 v_tot; >>>> + int ret = 0; >>>> + >>>> + h_tot = h_active + h_fp + h_sw + h_bp; >>>> + v_tot = v_active + v_fp + v_sw + v_bp; >>>> + >>>> + /* 75 Mhz pixel clock */ >>>> + cci_update_bits(priv->regmap, MAX96717_IO_CHK0, >>>> + MAX96717_PATTERN_CLK_FREQ, 0xa, &ret); >>>> + >>>> + dev_info(&priv->client->dev, "height: %d width: %d\n", fmt->height, >>>> + fmt->width); >>>> + >>>> + cci_write(priv->regmap, MAX96717_PATGEN_VS_DLY, 0, &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_VS_HIGH, v_sw * h_tot, &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_VS_LOW, >>>> + (v_active + v_fp + v_bp) * h_tot, &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_HS_HIGH, h_sw, &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_HS_LOW, h_active + h_fp + h_bp, >>>> + &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_V2D, >>>> + h_tot * (v_sw + v_bp) + (h_sw + h_bp), &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_HS_CNT, v_tot, &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_DE_HIGH, h_active, &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_DE_LOW, h_fp + h_sw + h_bp, >>>> + &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_DE_CNT, v_active, &ret); >>>> + /* B G R */ >>>> + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_A, 0xfecc00, &ret); >>>> + /* B G R */ >>>> + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_COLOR_B, 0x006aa7, &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_A, 0x3c, &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_RPT_CNT_B, 0x3c, &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_CHKB_ALT, 0x3c, &ret); >>>> + cci_write(priv->regmap, MAX96717_PATGEN_GRAD_INC, 0x10, &ret); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int max96717_apply_patgen(struct max96717_priv *priv, >>>> + struct v4l2_subdev_state *state) >>>> +{ >>>> + unsigned int val; >>>> + int ret = 0; >>>> + >>>> + if (priv->pattern) >>>> + ret = max96717_apply_patgen_timing(priv, state); >>>> + >>>> + cci_write(priv->regmap, MAX96717_PATGEN_0, priv->pattern ? 0xfb : 0, >>>> + &ret); >>>> + >>>> + val = FIELD_PREP(MAX96717_PATGEN_MODE, priv->pattern); >>>> + cci_update_bits(priv->regmap, MAX96717_PATGEN_1, MAX96717_PATGEN_MODE, >>>> + val, &ret); >>>> + return ret; >>>> +} >>>> + >>>> +static int max96717_s_ctrl(struct v4l2_ctrl *ctrl) >>>> +{ >>>> + struct max96717_priv *priv = >>>> + container_of(ctrl->handler, struct max96717_priv, ctrl_handler); >>>> + int ret; >>>> + >>>> + switch (ctrl->id) { >>>> + case V4L2_CID_TEST_PATTERN: >>>> + if (priv->enabled_source_streams) >>>> + return -EBUSY; >>>> + priv->pattern = ctrl->val; >>>> + break; >>>> + default: >>>> + return -EINVAL; >>>> + } >>>> + >>>> + /* Use bpp from bpp register */ >>>> + ret = cci_update_bits(priv->regmap, MAX96717_VIDEO_TX0, >>>> + MAX96717_VIDEO_AUTO_BPP, >>>> + priv->pattern ? 0 : MAX96717_VIDEO_AUTO_BPP, >>>> + NULL); >>>> + >>>> + /* Pattern generator doesn't work with tunnel mode */ >>> Can you add a comment saying that the deserializer should manage the link in >>> pixel mode as well. >> >> Something like: >> /* Pattern generator doesn't work with tunnel mode. >> Is mandatory to put also the deserializer into pixel mode. >> */ > > Actually I plan to add to add the following comment: > > /* > * Pattern generator doesn't work with tunnel mode. > * Needs RGB color format and deserializer tunnel mode must be disabled. > */ > return cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11, > MAX96717_TUN_MODE, > priv->pattern ? 0 : MAX96717_TUN_MODE, &ret); > > Let me know if this can be ok for you. Yes perfect Regards, -- Julien ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-27 12:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-06-17 14:59 [PATCH 0/1] media: i2c: max96717: add test pattern ctrl Tommaso Merciai 2024-06-17 14:59 ` [PATCH 1/1] " Tommaso Merciai 2024-06-25 7:54 ` Julien Massot 2024-06-25 17:31 ` Tommaso Merciai 2024-06-26 8:37 ` Tommaso Merciai 2024-06-27 12:37 ` Julien Massot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome
