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@xxxxxxxxx> --- 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
