linux-kernel - Re: [PATCH] imx-mipi-csis: Get the number of active lanes from mbus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <175681611736.1349241.9877873145029586025@isaac-ThinkPad-T16-Gen-2>
Date: Tue, 02 Sep 2025 13:28:37 +0100
From: Isaac Scott <isaac.scott@...asonboard.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>, Sakari Ailus <sakari.ailus@....fi>
Cc: linux-media@...r.kernel.org, rmfrfs@...il.com, martink@...teo.de, kernel@...i.sm, mchehab@...nel.org, shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com, imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] imx-mipi-csis: Get the number of active lanes from mbus_config

Hi All,

Quoting Laurent Pinchart (2025-08-19 03:44:13)
<snip>
> > > > > That would need to parse the endpoint every time we start streaming, it
> > > > > doesn't sound ideal.
> > > > 
> > > > Perhaps not, but does that matter in practice? Parsing the endpoint is,
> > > > after all, fairly trivial. The advantage would be simplifying drivers.
> > > 
> > > It's trivial from a code point of view, but it's not a cheap operation.
> > > I'd like to avoid making starting streaming more expensive.
> > 
> > How cheap is "not cheap"? I'd be surprised if parsing an endpoint took more
> > time than e.g. an I²C register write. Of course it depends on the CPU...
> 
> Still, it's not cheap, and I think it can easily be avoided.
> 
> > > > Alternatively we could think of caching this information somewhere but I
> > > > don't think it's worth it.
> > > 
> > > Drivers likely need to parse endpoints for other reasons. I'd cache the
> > > value in drivers, like done today, and pass it to a get_active_lanes
> > > helper.
> > 
> > Then drivers presumably would also validate this against the endpoint
> > configuration, wouldn't they? That's extra code in every CSI-2 receiver
> > driver.
> 
> Why so ? The number of connected lanes can be passed to the helper
> function, which can use it to validate the number of lanes reported by
> the source subdev.
> 

Apologies if I'm interpreting this wrong, but it seems that the main
thing I'm reading is that this is not the correct place to implement
this, and it should be implemented at a higher level (e.g. in v4l2) that
lets all MIPI CSI reciever drivers use it?

I have noticed that similar functionality has been implemented as part
of __v4l2_get_link_freq_pad. Are you suggesting that I take a similar
approach and resubmit as a new series?

> > > > > > The function could take struct media_pad pointer as an argument, or struct
> > > > > > v4l2_subdev pointer and the pad number.
> > > > > > 
> > > > > > I wonder if any other parameters could change dynamically but I can't think
> > > > > > of that now, so perhaps just the number of lanes is what the function
> > > > > > should indeed return.
> > > > > > 
> > > > > > > +
> > > > > > > +   return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > > >  {
> > > > > > >     struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > > > > > @@ -965,6 +1002,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > > >     format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
> > > > > > >     csis_fmt = find_csis_format(format->code);
> > > > > > >  
> > > > > > > +   ret = mipi_csis_get_active_lanes(sd);
> > > > > > > +   if (ret < 0)
> > > > > > > +           dev_dbg(csis->dev, "Failed to get active lanes: %d", ret);
> > > > > > > +
> > > > > > >     ret = mipi_csis_calculate_params(csis, csis_fmt);
> > > > > > >     if (ret < 0)
> > > > > > >             goto err_unlock;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Thank you very much for the help!

Best wishes,

Isaac