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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250902123805.GL13448@pendragon.ideasonboard.com>
Date: Tue, 2 Sep 2025 14:38:05 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Isaac Scott <isaac.scott@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@....fi>, 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

On Tue, Sep 02, 2025 at 01:28:37PM +0100, Isaac Scott wrote:
> 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?

As far as iI understand, Sakari would like a helper function that will
query the remote subdev for the number of data lanes it uses, and
validates that against the number of connected data lanes as described
by DT. I don't like the idea of parsing the endpoint properties every
time we do so, so I think the number of connected data lanes should be
passed by the driver to the helper instead. The helper would still query
the remote subdev, and validate the value.

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