| Message ID | 20250305113802.897087-2-mathis.foerst@mt.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] media: imx: csi: Parse link configuration from fw_node | expand |
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@mt.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 }; > > 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; regards, dan carpenter
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@mt.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. > > > > 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; Indeed. get_mbus_config() presumably wouldn't return an error but correctness is usually a good idea.
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@mt.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
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 }; 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 the upstream sd does not implement get_mbus_config, + * try to parse the link configuration from its fw_node + */ + ep_node = fwnode_graph_get_endpoint_by_id(dev_fwnode(remote_sd->dev), + 0, 0, + FWNODE_GRAPH_ENDPOINT_NEXT); + if (!ep_node) + return -ENOTCONN; + + ret = v4l2_fwnode_endpoint_parse(ep_node, &ep); + fwnode_handle_put(ep_node); + if (ret) + return ret; + mbus_cfg->type = ep.bus_type; + switch (ep.bus_type) { + case V4L2_MBUS_PARALLEL: + case V4L2_MBUS_BT656: + mbus_cfg->bus.parallel = ep.bus.parallel; + break; + case V4L2_MBUS_CSI2_DPHY: + mbus_cfg->bus.mipi_csi2 = ep.bus.mipi_csi2; + break; + default: + v4l2_err(&priv->sd, "Unsupported mbus_type: %i\n", + ep.bus_type); + return -EINVAL; + } + } return ret; }
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@mt.com> --- drivers/staging/media/imx/imx-media-csi.c | 36 ++++++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-)