| 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
| ||
|
Message-ID: <Z8r21Z2HthBwGDSq@mt.com>
Date: Fri, 7 Mar 2025 14:38:29 +0100
From: Mathis Foerst <mathis.foerst@...com>
To: Sakari Ailus <sakari.ailus@....fi>
Cc: Dan Carpenter <dan.carpenter@...aro.org>, linux-kernel@...r.kernel.org,
Steve Longerbeam <slongerbeam@...il.com>,
Philipp Zabel <p.zabel@...gutronix.de>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, linux-media@...r.kernel.org,
linux-staging@...ts.linux.dev, imx@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, manuel.traut@...com,
mathis.foerst@...hlke.com
Subject: Re: [PATCH v1 1/1] media: imx: csi: Parse link configuration from
fw_node
Hi Sakari, Hi Dan,
thanks a lot for your feedback.
On Thu, Mar 06, 2025 at 09:13:46PM +0000, Sakari Ailus wrote:
> Hi Dan,
>
> On Thu, Mar 06, 2025 at 10:07:20PM +0300, Dan Carpenter wrote:
> > On Thu, Mar 06, 2025 at 04:33:18PM +0000, Sakari Ailus wrote:
> > > Hi Mathis,
> > >
> > > Thanks for the patch.
> > >
> > > On Wed, Mar 05, 2025 at 12:38:02PM +0100, Mathis Foerst wrote:
> > > > The imx-media-csi driver requires upstream camera drivers to implement
> > > > the subdev-pad-op "get_mbus_config" [0]. Camera drivers that don't
> > > > implement this function are not usable on the i.MX6.
> > > >
> > > > The docs for get_mbus_config [1] say:
> > > > @get_mbus_config: get the media bus configuration of a remote sub-device.
> > > > The media bus configuration is usually retrieved from the
> > > > firmware interface at sub-device probe time, immediately
> > > > applied to the hardware and eventually adjusted by the
> > > > driver.
> > > >
> > > > Currently, the imx-media-csi driver is not incorporating the information
> > > > from the firmware interface and therefore relies on the implementation of
> > > > get_mbus_config by the camera driver.
> > > >
> > > > To be compatible with camera drivers not implementing get_mbus_config
> > > > (which is the usual case), use the bus information from the fw interface:
> > > >
> > > > The camera does not necessarily has a direct media bus link to the CSI as
> > > > the video-mux and/or the MIPI CSI-2 receiver of the i.MX6 might be in
> > > > between them on the media pipeline.
> > > > The CSI driver already implements the functionality to find the connected
> > > > camera sub-device to call get_mbus_config on it.
> > > >
> > > > At this point the driver is modified as follows:
> > > > In the case that get_mbus_config is not implemented by the upstream
> > > > camera, try to get its endpoint configuration from the firmware interface
> > > > usign v4l2_fwnode_endpoint_parse.
> > > > For the supported mbus_types (V4L2_MBUS_PARALLEL, V4L2_MBUS_BT656 and
> > > > V4L2_MBUS_CSI2_DPHY), extract the mbus_config from the endpoint
> > > > configuration.
> > > > For all other mbus_types, return an error.
> > > >
> > > > Note that parsing the mbus_config from the fw interface is not done during
> > > > probing because the camera that's connected to the CSI can change based on
> > > > the selected input of the video-mux at runtime.
> > > >
> > > > [0] drivers/staging/media/imx/imx-media-csi.c - line 211..216
> > > > [1] include/media/v4l2-subdev.h - line 814
> > > >
> > > > Signed-off-by: Mathis Foerst <mathis.foerst@...com>
> > > > ---
> > > > drivers/staging/media/imx/imx-media-csi.c | 36 ++++++++++++++++++++---
> > > > 1 file changed, 32 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> > > > index 3edbc57be2ca..394a9321a10b 100644
> > > > --- a/drivers/staging/media/imx/imx-media-csi.c
> > > > +++ b/drivers/staging/media/imx/imx-media-csi.c
> > > > @@ -169,6 +169,8 @@ static int csi_get_upstream_mbus_config(struct csi_priv *priv,
> > > > {
> > > > struct v4l2_subdev *sd, *remote_sd;
> > > > struct media_pad *remote_pad;
> > > > + struct fwnode_handle *ep_node;
> > > > + struct v4l2_fwnode_endpoint ep = { .bus_type = 0 };
> > >
> > > Are there any defaults in DT bindings (other than 0's)? Also initialising a
> > > field to zero this way is redundant, just use {}.
> > >
> >
> > I was going to respond in much the same way. This is equivalen to:
> >
> > struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_UNKNOWN };
>
> Thinking about this in a context of parsing the endpoint, in fact the
> bus_type should be specified. Presumably the hardware is D-PHY, so the
> correct value would be V4L2_MBUS_CSI2_DPHY. This way
> v4l2_fwnode_endpoint_parse() doesn't need to guess.
I think we must use "bus_type = V4L2_MBUS_UNKNOWN" here:
The i.MX6 has two types of camera interfaces: Parallel and MIPI CSI-2.
They are connected either directly or via a video-mux to the CSIs
(See IMX6DQRM.pdf - Figure 9-3 for the connection diagram)
Pre-defining V4L2_MBUS_CSI2_DPHY here would let
v4l2_fwnode_endpoint_parse() fail if the camera uses the parallel bus.
We could distinguish between MIPI CSI-2 and Parallel input by checking
the grp_id of the upstream device like it's already done in
csi_get_upstream_mbus_config().
But for the Parallel case we still can't know if we should set bus_type
to V4L2_MBUS_PARALLEL or to V4L2_MBUS_BT656 - the i.MX6 supports both
formats on the parallel interface.
That's why I would argue that v4l2_fwnode_endpoint_parse() must figure
out the bus_type from the fw node.
>
> >
> > > > int ret;
> > > >
> > > > if (!priv->src_sd)
> > > > @@ -210,11 +212,37 @@ static int csi_get_upstream_mbus_config(struct csi_priv *priv,
> > > >
> > > > ret = v4l2_subdev_call(remote_sd, pad, get_mbus_config,
> > > > remote_pad->index, mbus_cfg);
> > > > - if (ret == -ENOIOCTLCMD)
> > > > - v4l2_err(&priv->sd,
> > > > - "entity %s does not implement get_mbus_config()\n",
> > > > - remote_pad->entity->name);
> > > > + if (ret == -ENOIOCTLCMD) {
> > >
> > > if (!ret)
> > > return 0;
> > >
> > > And you can unindent the rest.
> >
> > I was going to say this too but then I thought actually this needs to
> > be:
> >
> > if (ret != -ENOIOCTLCMD)
> > return ret;
> >
> > Which is weird. Better to break all the new code into a separate
> > helper function.
> >
> > if (ret == -ENOIOCTLCMD)
> > ret = parse_fw_link_config_stuff();
> >
> > return ret;
Good point. I factored out a helper function as suggested.
>
> Indeed. get_mbus_config() presumably wouldn't return an error but
> correctness is usually a good idea.
>
> --
> Regards,
>
> Sakari Ailus
Best regards,
Mathis Foerst
Powered by blists - more mailing lists