| Message ID | 20240105083757.197846-3-sakari.ailus@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 53aa6b38f10cc42136551acf115a8b04e013ec89 |
| Headers | show |
| Series | Miscellaneous small things | expand |
Hi Sakari, Thank you for the patch. On Fri, Jan 05, 2024 at 10:37:56AM +0200, Sakari Ailus wrote: > Add relevant debug prints for v4l2_fwnode_create_links_for_pad(). This > should help debugging when things go wrong. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-mc.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c > index 52d349e72b8c..e394f3e505d8 100644 > --- a/drivers/media/v4l2-core/v4l2-mc.c > +++ b/drivers/media/v4l2-core/v4l2-mc.c > @@ -337,12 +337,18 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd, > src_idx = media_entity_get_fwnode_pad(&src_sd->entity, > endpoint, > MEDIA_PAD_FL_SOURCE); > - if (src_idx < 0) > + if (src_idx < 0) { > + dev_dbg(src_sd->dev, "no pad found for %pfw\n", Make is "no source pad found", as we're looking for source pads only and the message would be confusing if the entity has sink pads. > + endpoint); > continue; > + } > > remote_ep = fwnode_graph_get_remote_endpoint(endpoint); > - if (!remote_ep) > + if (!remote_ep) { > + dev_dbg(src_sd->dev, "no remote ep found for %pfw\n", > + endpoint); > continue; > + } > > /* > * ask the sink to verify it owns the remote endpoint, > @@ -353,8 +359,12 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd, > MEDIA_PAD_FL_SINK); > fwnode_handle_put(remote_ep); > > - if (sink_idx < 0 || sink_idx != sink->index) > + if (sink_idx < 0 || sink_idx != sink->index) { > + dev_dbg(src_sd->dev, > + "sink pad index mismatch or error (is %d, expected %u)\n", > + sink_idx, sink->index); > continue; > + } > > /* > * the source endpoint corresponds to one of its source pads, > @@ -367,8 +377,13 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd, > src = &src_sd->entity.pads[src_idx]; > > /* skip if link already exists */ > - if (media_entity_find_link(src, sink)) > + if (media_entity_find_link(src, sink)) { > + dev_dbg(src_sd->dev, > + "link %s:%d -> %s:%d already exists\n", > + src_sd->entity.name, src_idx, Is this worth a debug message ? If the link already exists, do you expect this to cause any kind of issue that someone will want to debug ? Overall, are the new debug messages in this patch helped debugging a real life problem ? > + sink->entity->name, sink_idx); > continue; > + } > > dev_dbg(src_sd->dev, "creating link %s:%d -> %s:%d\n", > src_sd->entity.name, src_idx,
Hi Laurent, On Tue, Jan 09, 2024 at 02:09:50PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Fri, Jan 05, 2024 at 10:37:56AM +0200, Sakari Ailus wrote: > > Add relevant debug prints for v4l2_fwnode_create_links_for_pad(). This > > should help debugging when things go wrong. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! > > > --- > > drivers/media/v4l2-core/v4l2-mc.c | 23 +++++++++++++++++++---- > > 1 file changed, 19 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c > > index 52d349e72b8c..e394f3e505d8 100644 > > --- a/drivers/media/v4l2-core/v4l2-mc.c > > +++ b/drivers/media/v4l2-core/v4l2-mc.c > > @@ -337,12 +337,18 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd, > > src_idx = media_entity_get_fwnode_pad(&src_sd->entity, > > endpoint, > > MEDIA_PAD_FL_SOURCE); > > - if (src_idx < 0) > > + if (src_idx < 0) { > > + dev_dbg(src_sd->dev, "no pad found for %pfw\n", > > Make is "no source pad found", as we're looking for source pads only and > the message would be confusing if the entity has sink pads. I'll add this. > > > + endpoint); > > continue; > > + } > > > > remote_ep = fwnode_graph_get_remote_endpoint(endpoint); > > - if (!remote_ep) > > + if (!remote_ep) { > > + dev_dbg(src_sd->dev, "no remote ep found for %pfw\n", > > + endpoint); > > continue; > > + } > > > > /* > > * ask the sink to verify it owns the remote endpoint, > > @@ -353,8 +359,12 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd, > > MEDIA_PAD_FL_SINK); > > fwnode_handle_put(remote_ep); > > > > - if (sink_idx < 0 || sink_idx != sink->index) > > + if (sink_idx < 0 || sink_idx != sink->index) { > > + dev_dbg(src_sd->dev, > > + "sink pad index mismatch or error (is %d, expected %u)\n", > > + sink_idx, sink->index); > > continue; > > + } > > > > /* > > * the source endpoint corresponds to one of its source pads, > > @@ -367,8 +377,13 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd, > > src = &src_sd->entity.pads[src_idx]; > > > > /* skip if link already exists */ > > - if (media_entity_find_link(src, sink)) > > + if (media_entity_find_link(src, sink)) { > > + dev_dbg(src_sd->dev, > > + "link %s:%d -> %s:%d already exists\n", > > + src_sd->entity.name, src_idx, > > Is this worth a debug message ? If the link already exists, do you > expect this to cause any kind of issue that someone will want to debug ? > > Overall, are the new debug messages in this patch helped debugging a > real life problem ? They would have helped debugging development time versions of the previous patch. :-) Multiple people have also had issues debugging drivers depending on matching sub-devices and creating links between them due to the complexity of the firmware interface and the in-kernel framework. Telling what is taking happening here is one way to address that. If a link already exists, something is probably wrong. I didn't want to make this an error in this patch as I didn't want to introduce a functional change here. I think it could be made later on. > > > + sink->entity->name, sink_idx); > > continue; > > + } > > > > dev_dbg(src_sd->dev, "creating link %s:%d -> %s:%d\n", > > src_sd->entity.name, src_idx, >
diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c index 52d349e72b8c..e394f3e505d8 100644 --- a/drivers/media/v4l2-core/v4l2-mc.c +++ b/drivers/media/v4l2-core/v4l2-mc.c @@ -337,12 +337,18 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd, src_idx = media_entity_get_fwnode_pad(&src_sd->entity, endpoint, MEDIA_PAD_FL_SOURCE); - if (src_idx < 0) + if (src_idx < 0) { + dev_dbg(src_sd->dev, "no pad found for %pfw\n", + endpoint); continue; + } remote_ep = fwnode_graph_get_remote_endpoint(endpoint); - if (!remote_ep) + if (!remote_ep) { + dev_dbg(src_sd->dev, "no remote ep found for %pfw\n", + endpoint); continue; + } /* * ask the sink to verify it owns the remote endpoint, @@ -353,8 +359,12 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd, MEDIA_PAD_FL_SINK); fwnode_handle_put(remote_ep); - if (sink_idx < 0 || sink_idx != sink->index) + if (sink_idx < 0 || sink_idx != sink->index) { + dev_dbg(src_sd->dev, + "sink pad index mismatch or error (is %d, expected %u)\n", + sink_idx, sink->index); continue; + } /* * the source endpoint corresponds to one of its source pads, @@ -367,8 +377,13 @@ int v4l2_create_fwnode_links_to_pad(struct v4l2_subdev *src_sd, src = &src_sd->entity.pads[src_idx]; /* skip if link already exists */ - if (media_entity_find_link(src, sink)) + if (media_entity_find_link(src, sink)) { + dev_dbg(src_sd->dev, + "link %s:%d -> %s:%d already exists\n", + src_sd->entity.name, src_idx, + sink->entity->name, sink_idx); continue; + } dev_dbg(src_sd->dev, "creating link %s:%d -> %s:%d\n", src_sd->entity.name, src_idx,
Add relevant debug prints for v4l2_fwnode_create_links_for_pad(). This should help debugging when things go wrong. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-mc.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)