Hi Niklas,
Thanks for giving this series a test. More comments below.
On Tue, Feb 04, 2025 at 01:39:25PM +0100, Niklas Söderlund wrote:
> Hi Laurentiu,
>
> Thanks for your work. I'm happy someone with a both GMSL cameras and a
> max96712 found time to work on this driver.
I don't have a max96712 unfortunately. Our setup is based on max96724.
>
> On 2025-01-31 18:33:54 +0200, Laurentiu Palcu wrote:
> > Hi,
> >
> > This series adds more functionality to the existing max96712 staging
> > driver allowing multiple sensors to be connected through other
> > compatible serializers. I tried to split the changes in smaller logical
> > changes to make them easier to review while not altering the existing
> > VPG functionality but I could squash all of them together if needed.
>
> With this series and it's listed dependencies applied my CI tests using
> the VPG breaks. The controls to select test-pattern seems to be invalid,
>
> $ yavta --set-control '0x009f0903 0' /dev/v4l-subdev6
> Device /dev/v4l-subdev6 opened.
> unable to set control 0x009f0903: Permission denied (13).
> Unable to get format: Inappropriate ioctl for device (25).
That's my bad... :/ I have never tried to change test patterns and I
didn't spot the bug. The below change should fix that:
diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
index b4c3d1d3c9539..6d6dea0a335c7 100644
--- a/drivers/staging/media/max96712/max96712.c
+++ b/drivers/staging/media/max96712/max96712.c
@@ -1315,10 +1315,10 @@ static int max96712_v4l2_register(struct max96712_priv *priv)
v4l2_ctrl_handler_init(&priv->ctrl_handler, 2);
- v4l2_ctrl_new_int_menu(&priv->ctrl_handler, NULL, V4L2_CID_LINK_FREQ,
+ link_freq_ctrl = v4l2_ctrl_new_int_menu(&priv->ctrl_handler, NULL, V4L2_CID_LINK_FREQ,
0, 0, &priv->link_freq);
- link_freq_ctrl = v4l2_ctrl_new_std_menu_items(&priv->ctrl_handler, &max96712_ctrl_ops,
+ v4l2_ctrl_new_std_menu_items(&priv->ctrl_handler, &max96712_ctrl_ops,
V4L2_CID_TEST_PATTERN,
ARRAY_SIZE(max96712_test_pattern) - 1,
0, 0, max96712_test_pattern);
>
> (/dev/v4l-subdev6 here is max96712 1-0049)
>
> $ yavta -c10 --file=/tmp/vin-capture/isp0-checkerboard-#.bin /dev/video0
> Device /dev/video0 opened.
> Device `R_Car_VIN' on `platform:e6ef0000.video' (driver 'rcar_vin') supports video, capture, without mplanes.
> Video format: ABGR32 (34325241) 1920x1080 (stride 7680) field none buffer size 8294400
> 8 buffers requested.
> length: 8294400 offset: 0 timestamp type/source: mono/EoF
> Buffer 0/0 mapped at address 0xffffbe5d7000.
> length: 8294400 offset: 32768 timestamp type/source: mono/EoF
> Buffer 1/0 mapped at address 0xffffbddee000.
> length: 8294400 offset: 65536 timestamp type/source: mono/EoF
> Buffer 2/0 mapped at address 0xffffbd605000.
> length: 8294400 offset: 98304 timestamp type/source: mono/EoF
> Buffer 3/0 mapped at address 0xffffbce1c000.
> length: 8294400 offset: 131072 timestamp type/source: mono/EoF
> Buffer 4/0 mapped at address 0xffffbc633000.
> length: 8294400 offset: 163840 timestamp type/source: mono/EoF
> Buffer 5/0 mapped at address 0xffffbbe4a000.
> length: 8294400 offset: 196608 timestamp type/source: mono/EoF
> Buffer 6/0 mapped at address 0xffffbb661000.
> length: 8294400 offset: 229376 timestamp type/source: mono/EoF
> Buffer 7/0 mapped at address 0xffffbae78000.
> Unable to start streaming: Invalid argument (22).
>
> I read in patch 12/12 "The user can also switch over to testing the test
> pattern by configuring the routes accordingly", but not how to do that
> to achieve the same functionality as the staging driver. Inspecting the
> media graph gives little help. Would it be possible to extend the cover
> letter with this information?
I can do that, sure, but routing setup depends on the board you test on... :/
Basically, the deserializer media node has 6 pads now. Pad 4 is first CSI
output port and pad 6 is the internal VPG source pad. By default, the route
from internal VPG pad to pad 4 is active. So, you shouldn't need to set any
routes on max96712 node for testing VPG. This is how it looks like:
- entity 120: max96712 2-0027 (7 pads, 5 links, 1 route)
type V4L2 subdev subtype Unknown flags 0
device node name /dev/v4l-subdev11
routes:
6/0 -> 4/0 [ACTIVE]
pad0: Sink
<- "max96717 8-0040":1 []
pad1: Sink
<- "max96717 9-0040":1 []
pad2: Sink
<- "max96717 10-0040":1 []
pad3: Sink
<- "max96717 14-0040":1 []
pad4: Source
[stream:0 fmt:RGB888_1X24/1920x1080 field:none colorspace:srgb]
-> "csidev-4ad30000.csi":0 []
pad5: Source
pad6: Sink, Internal
[stream:0 fmt:RGB888_1X24/1920x1080 field:none colorspace:srgb]
Below is the test script I use to set the routings and links for all the
pipeline in order to test VPG. I'm testing on an i.MX95 board.
./media-ctl -r
./media-ctl -d /dev/media0 -R '"max96712 2-0027" [6/0 -> 4/0 [1]]'
./media-ctl -d /dev/media0 --set-v4l2 '"max96712 2-0027":6/0 [fmt:RGB24/1920x1080 field:none]'
./media-ctl -d /dev/media0 -R '"csidev-4ad30000.csi" [0/0 -> 1/0 [1]]'
./media-ctl -d /dev/media0 --set-v4l2 '"csidev-4ad30000.csi":0/0 [fmt:RGB24/1920x1080 field:none]'
./media-ctl -d /dev/media0 -R '"4ac10000.syscon:formatter@20" [0/0 -> 1/0 [1]]'
./media-ctl -d /dev/media0 --set-v4l2 '"4ac10000.syscon:formatter@20":0/0 [fmt:RGB24/1920x1080 field:none]'
./media-ctl -d /dev/media0 -R '"crossbar" [2/0 -> 7/0 [1]]'
./media-ctl -d /dev/media0 --set-v4l2 '"crossbar":2/0 [fmt:RGB24/1920x1080 field:none]'
./media-ctl -d /dev/media0 --set-v4l2 '"mxc_isi.2":0/0 [fmt:RGB24/1920x1080 field:none]'
./media-ctl -d /dev/media0 -l "'max96712 2-0027':4 -> 'csidev-4ad30000.csi':0 [1]"
./v4l2-ctl --device /dev/video2 --set-fmt-video=width=1920,height=1080,pixelformat=RGB3 --stream-mmap --stream-count=100
However, I suspect that, in your case, the downstream drivers do not support
streams and streaming cannot start. But I might be wrong though...
>
> To be clear, I don't care about the change in behavior as this driver
> obviously primary aim should be to support GMSL2 cameras, not
> test-pattern generation. It is important for me that it is possible to
> enable the test pattern generation $somehow at runtime (i.e. no DTS
> changes). As this is the only method I have to test a bunch of boards.
That's the idea. I can switch between capturing from sensors and generating the
test pattern at runtime. I don't do any changes in the DTB. However, I believe
the downstream devices need to support streams as well in order to work.
>
> It would also be nice if the patterns generated (output frames) as
> closely as possible would resembles what is generated today. That way I
> don't have to alter my CI pipelines too much :-)
I didn't touch the pattern generation part at all. From what I can see, it
looks the same.
Thanks,
Laurentiu
>
> >
> > The series only supports tunneling mode and uses the first MIPI-CSI
> > port. Support for more functionality can be added later, if needed.
> >
> > I sent the set as a RFC because it depends on Sakari's pending internal
> > pads patch which is needed if we want to have an elegant solution for
> > allowing the user to switch between streaming from sensors or just
> > video pattern generation.
> >
> > Also, the set depends on my previous series which was not yet merged:
> > https://patchwork.linuxtv.org/project/linux-media/list/?series=14255
> >
> > Thanks,
> > Laurentiu
> >
> > Laurentiu Palcu (11):
> > dt-bindings: i2c: maxim,max96712: add a couple of new properties
> > staging: media: max96712: convert to using CCI register access helpers
> > staging: media: max96712: change DT parsing routine
> > staging: media: max96712: add link frequency V4L2 control
> > staging: media: max96712: add I2C mux support
> > staging: media: max96712: add support for streams
> > staging: media: max96712: allow enumerating MBUS codes
> > staging: media: max96712: add set_fmt routine
> > staging: media: max96712: add gpiochip functionality
> > staging: media: max96712: add fsync support
> > staging: media: max96712: allow streaming from connected sensors
> >
> > Sakari Ailus (1):
> > media: mc: Add INTERNAL pad flag
> >
> > .../bindings/media/i2c/maxim,max96712.yaml | 45 +
> > .../media/mediactl/media-types.rst | 8 +
> > drivers/media/mc/mc-entity.c | 10 +-
> > drivers/staging/media/max96712/max96712.c | 1406 +++++++++++++++--
> > include/uapi/linux/media.h | 1 +
> > 5 files changed, 1352 insertions(+), 118 deletions(-)
> >
> > --
> > 2.44.1
> >
>
> --
> Kind Regards,
> Niklas Söderlund
