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: <aJ8ZJFSn5Wxhj2Aj@valkosipuli.retiisi.eu>
Date: Fri, 15 Aug 2025 11:25:24 +0000
From: Sakari Ailus <sakari.ailus@....fi>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Isaac Scott <isaac.scott@...asonboard.com>, 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 Laurent,

On Fri, Aug 15, 2025 at 01:32:05PM +0300, Laurent Pinchart wrote:
> On Fri, Aug 15, 2025 at 09:18:13AM +0000, Sakari Ailus wrote:
> > On Thu, Aug 14, 2025 at 12:37:01PM +0100, Isaac Scott wrote:
> > > Although 4 lanes may be physically available, we may not be using all of
> > > them. Get the number of configured lanes in the case a driver has
> > > implemented the get_mbus_config op.
> > > 
> > > Signed-off-by: Isaac Scott <isaac.scott@...asonboard.com>
> > > 
> > > ---
> > > 
> > > Currently, the imx-mipi-csis driver parses the device tree to determine
> > > the number of configured lanes for the CSI receiver. This may be
> > > incorrect in the case that the connected device only uses a subset of
> > > lanes, for example. Allow the drivers for these cameras to create a
> > > mbus_config to configure the number of lanes that are actually being
> > > used.
> > > 
> > > If the driver does not support the get_mbus_config op, this patch will
> > > have no functional change.
> > > 
> > > Compile tested against media-master (v6.17-rc1)
> > > ---
> > >  drivers/media/platform/nxp/imx-mipi-csis.c | 41 ++++++++++++++++++++++
> > >  1 file changed, 41 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > index 2beb5f43c2c0..efe4e2ad0382 100644
> > > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > @@ -939,6 +939,43 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
> > >  	return container_of(sdev, struct mipi_csis_device, sd);
> > >  }
> > >  
> > > +static int mipi_csis_get_active_lanes(struct v4l2_subdev *sd)
> > > +{
> > > +	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > +	struct v4l2_mbus_config mbus_config = { 0 };
> > > +	int ret;
> > > +
> > > +	ret = v4l2_subdev_call(csis->source.sd, pad, get_mbus_config,
> > > +			       0, &mbus_config);
> > > +	if (ret == -ENOIOCTLCMD) {
> > > +		dev_dbg(csis->dev, "No remote mbus configuration available\n");
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (ret) {
> > > +		dev_err(csis->dev, "Failed to get remote mbus configuration\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> > > +		dev_err(csis->dev, "Unsupported media bus type %u\n",
> > > +			mbus_config.type);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (mbus_config.bus.mipi_csi2.num_data_lanes > csis->bus.num_data_lanes) {
> > > +		dev_err(csis->dev,
> > > +			"Unsupported mbus config: too many data lanes %u\n",
> > > +			mbus_config.bus.mipi_csi2.num_data_lanes);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	csis->bus.num_data_lanes = mbus_config.bus.mipi_csi2.num_data_lanes;
> 
> There's a bug here, you override the number of lanes retrieved from DT,
> which is the number of connected lanes, with the number of lanes used by
> the source for its particular configuration. You will never be able to
> then use more lanes in a different source configuration.
> 
> > > +	dev_dbg(csis->dev, "Number of lanes: %d\n", csis->bus.num_data_lanes);
> > 
> > None of the above is really specific to this driver. Could you instead
> > implement a function that parses the information from the fwnode endpoint
> > and uses mbus configuration on top?
> 
> 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.

Alternatively we could think of caching this information somewhere but I
don't think it's worth it.

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

Sakari Ailus