Hi Kieran,
On Mon, 2020-03-02 at 02:33:18 -0800, Kieran Bingham wrote:
> Hi Hyun,
>
> On 28/02/2020 18:13, Hyun Kwon wrote:
> > Hi Kieran,
> >
> > Thanks for sharing a patch.
> >
> > Sorry for late response. I'm trying this driver along with Xilinx MIPI
> > pipeline and GMSL sensors with max96705. I wanted to confirm a few things
> > before replying. It's still on going, but replying before it gets too late.
> > Mostly it's questions and looking for some input.
>
> Not too late at all.
>
> We're very pleased to get input from other users and system configurations!
>
>
> > On Fri, 2020-02-14 at 03:54:19 -0800, Kieran Bingham wrote:
> >> Small update,
> >>
> >> On 14/02/2020 10:31, Kieran Bingham wrote:
> >>> The MAX9286 is a 4-channel GMSL deserializer with coax or STP input and
> >>> CSI-2 output. The device supports multicamera streaming applications,
> >>> and features the ability to synchronise the attached cameras.
> >>>
[snip]
> >>> +// SPDX-License-Identifier: GPL-2.0+
> >>> +/*
> >>> + * Maxim MAX9286 GMSL Deserializer Driver
> >>> + *
> >
> > [snip]
> >
> >>> +
> >>> +static const struct v4l2_async_notifier_operations max9286_notify_ops = {
> >>> + .bound = max9286_notify_bound,
> >>> + .unbind = max9286_notify_unbind,
> >>> +};
> >>> +
> >>> +static int max9286_v4l2_notifier_register(struct max9286_priv *priv)
> >>> +{
> >>> + struct device *dev = &priv->client->dev;
> >>> + struct max9286_source *source = NULL;
> >>> + int ret;
> >>> +
> >>> + if (!priv->nsources)
> >>> + return 0;
> >>> +
> >>> + v4l2_async_notifier_init(&priv->notifier);
> >>> +
> >>> + for_each_source(priv, source) {
> >>> + unsigned int i = to_index(priv, source);
> >>> +
> >>> + source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> >>> + source->asd.match.fwnode = source->fwnode;
> >>> +
> >>> + ret = v4l2_async_notifier_add_subdev(&priv->notifier,
> >>> + &source->asd);
> >>> + if (ret) {
> >>> + dev_err(dev, "Failed to add subdev for source %d", i);
> >>> + v4l2_async_notifier_cleanup(&priv->notifier);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + /*
> >>> + * Balance the reference counting handled through
> >>> + * v4l2_async_notifier_cleanup()
> >>> + */
> >>> + fwnode_handle_get(source->fwnode);
> >>> + }
> >>> +
> >>> + priv->notifier.ops = &max9286_notify_ops;
> >>> +
> >>> + ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
> >>> + if (ret) {
> >>> + dev_err(dev, "Failed to register subdev_notifier");
> >>> + v4l2_async_notifier_cleanup(&priv->notifier);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >
> > This was giving me some touble because this subdev notifier chain is a part
> > of a bigger graph (Xilinx Video pipeline graph). Those are defined using
> > same graph bindings, hence conflicting each other. For now, in order to
> > work around, I'm calling bound of subdev notifier if there's any match
> > already in parent's done list [1]. Do you have any input how this should be
> > handled properly?
>
> I suspect that it is likely your existing framework is matching at the
> device level instead of the endpoint level, as it sounds like a topic we
> hit on both this GMSL implementation and the ADV748x, but I'll have to
> take a deeper look to be sure.
>
The problem with subdev notifier is, for example, the Xilinx platform parses
and registers a notifier for the entire pipeline. The current single stream
(to be simpler for problem description) pipeline looks like,
sensor -> serializer -> deserializer -> mipi -> scaler -> dma
If the max9286 registers the subdev notifier within the video dev notifier,
it never gets to match and notified because the bound subdev is already
moved out of subdev list to the notifier done list. I feel v4l2-async should
be able to identify this and complete the subdev notifier if it's already
a part of bigger one. Let me see if I can figure out any sensible fix.
> Matching should be done on endpoints, not devices as there could now be
> multiple 'endpoints' connected to a single device.
>
>
> The RCar-VIN platform now solely uses endpoint matching, and I believe
> we may have to work through other platforms to update to the same approach.
>
>
> A 'temporary' solution may be in the form of this patch:
>
> https://git.linuxtv.org/sailus/media_tree.git/commit/?h=fwnode-const&id=35c32d99b2c3f5086b911ec817926de9b7bc3b41
>
> Which I had mistakenly thought was already accepted for upstream but
> appears to have stalled.
>
> I'll follow this up separately to see what we need to do here.
>
This doesn't fix the issue completely, because the Xilinx platform layer uses
the asd match node assuming it's device node. I can fix the Xilinx platform
part. I'm going through some previous email threads / patches, trying to get
better context.
>
> >>> +
> >>> +static void max9286_v4l2_notifier_unregister(struct max9286_priv *priv)
> >>> +{
> >>> + if (!priv->nsources)
> >>> + return;
> >>> +
> >>> + v4l2_async_notifier_unregister(&priv->notifier);
> >>> + v4l2_async_notifier_cleanup(&priv->notifier);
> >>> +}
> >>> +
> >
> > [snip]
> >
> >>> +};
> >>> +
[snip]
> >>> + /*
> >>> + * Reverse channel setup.
> >>> + *
> >>> + * - Enable custom reverse channel configuration (through register 0x3f)
> >>> + * and set the first pulse length to 35 clock cycles.
> >>> + * - Increase the reverse channel amplitude to 170mV to accommodate the
> >>> + * high threshold enabled by the serializer driver.
> >>> + */
> >>> + max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> >>> + max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> >>> + MAX9286_REV_AMP_X);
> >>> + usleep_range(2000, 2500);
> >>> +
> >>> + /*
> >>> + * Enable GMSL links, mask unused ones and autodetect link
> >>> + * used as CSI clock source.
> >>> + */
> >>> + max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> >>> + max9286_write(priv, 0x0b, link_order[priv->route_mask]);
> >>> + max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
> >>> +
> >>> + /*
> >>> + * Video format setup:
> >>> + * Disable CSI output, VC is set according to Link number.
> >>> + */
> >>> + max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
> >>> +
> >>> + /* Enable CSI-2 Lane D0-D3 only, DBL mode, YUV422 8-bit. */
> >>> + max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
> >>> + MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> >>> + MAX9286_DATATYPE_YUV422_8BIT);
> >>> +
> >>> + /* Automatic: FRAMESYNC taken from the slowest Link. */
> >>> + max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> >>> + MAX9286_FSYNCMETH_AUTO);
> >>> +
> >>> + /* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> >>> + max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
> >>> + MAX9286_HVSRC_D14);
> >
> > Some of these configs in fact need some handshake between serializer and
> > de-serializer. For example, I had to invert vsync in serializer [3] to make
> > it work with this patch.
> >
> > In addition to that, I need a couple of additional programmings on max9286
> > (registers 0x0 to 0x63/0x64- disable overlap window and 0xf4 to 0x1c which
> > oddly change reserved bits) to get frames. The datasheet doesn't explain
> > enough for me to understand. I'm talking to Maxim to get more details and
> > see if those can be handled by serilizer driver.
>
> It seems Jacopo also had to disable the overlap window for the RDACM21:
>
> https://jmondi.org/cgit/linux/commit/?h=gmsl/jmondi/platform/rdacm21&id=576bbaee7cc707869a0c5e90befd99c9e2cf754e
>
> Please let us know if you hear back from Maxim.
>
>
> > In a longer term, it'd be nice if such alignment between serializer and
> > de-serializer is handled in more scalable way.
>
> I agree, We are currently trying to tackle similar issues between our
> two current cameras "RDACM20" and "RDACM21" which have different
> requirements for the two configurations so we are trying to look at ways
> to handle that too.
>
> In particular we also need to handle:
> - Pixelrate control to determine CSI2 bus speed.
> - Link amplitude and threshold levels
> - Format differences
>
> Are you still using the 'Sensing/Vision' (AR0231?) camera module that
> Linaro were working with? (I currently have one, so let me know if you'd
> like me to do any testing)
>
>
> If they are of any use/reference to you, here are our current WIP
> drivers for the RDACM20 [0] and RDACM21 [1] which Jacopo has been
> developing which show how we currently separate the max9271 and sensor
> packaging:
Thanks for pointers. I was able to google-search some of those drivers already,
but maybe this is latest.. :-) I also checked to get same sensors (vendor,
distributor,,, and IdeasOnBoard as well :-)), but it's not available anymore.
So the setup that I have now,
AR0231 -> AP0202 ISP -> MAX96705 -> MAX9286 -> Xilinx MIPI -> Xilinx pipeline / DMA
Yesterday I was able to get correct frames, with some hacks and workarounds.
Only missing things from this max9286 driver for my setup are,
- disabling the overlap window
- programming 0x1c register. I believe lower bits have something to do with
three level BWS config, so changing from 0xf6 to 0xf4 changes from high
bandwidth mode to 24 bit mode. But I couldn't get it working with high bandwidth
mode (27 bit) yet.
I got some response from Maxim, but haven't found clarifications yet.
As mentioned above, what is the proposed way that is being looked for
this ser-des config, if any? I can try and align max96705 driver implementation
with it.
Thanks,
-hyun
