[PATCH v2 0/3] Miscellaneous small things

[PATCH v2 0/3] Miscellaneous small things

[Sakari Ailus]

Hi folks,

This set does three things: refactor link creation in ipu3-cio2 driver by
using v4l2_fwnode_create_links_for_pad(), add debug prints in
v4l2_fwnode_create_links_for_pad() and drop an unneeded debug print in
media_relase().

since v1:

- Drop mistakenly added Prabhakar's Tested-by:.

- Print entity names and pad numbers for links that already exist in
  v4l2_create_fwnode_links_to_pad().

- Use "was" instead of "is" when comparing pad indices.

Sakari Ailus (3):
  media: ipu3-cio2: Further clean up async subdev link creation
  media: v4l2-mc: Add debug prints for
    v4l2_fwnode_create_links_for_pad()
  media: mc: Drop useless debug print on file handle release

 drivers/media/mc/mc-devnode.c            |  1 -
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 22 +++++-----------------
 drivers/media/v4l2-core/v4l2-mc.c        | 23 +++++++++++++++++++----
 3 files changed, 24 insertions(+), 22 deletions(-)

-- 
2.39.2

[PATCH v2 1/3] media: ipu3-cio2: Further clean up async subdev link creation

[Sakari Ailus]

Use v4l2_create_fwnode_links_to_pad() to create links from async
sub-devices to the CSI-2 receiver subdevs.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index ed08bf4178f0..83e29c56fe33 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -28,6 +28,7 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
+#include <media/v4l2-mc.h>
 #include <media/v4l2-ioctl.h>
 #include <media/videobuf2-dma-sg.h>
 
@@ -1407,7 +1408,6 @@ static void cio2_notifier_unbind(struct v4l2_async_notifier *notifier,
 static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
 {
 	struct cio2_device *cio2 = to_cio2_device(notifier);
-	struct device *dev = &cio2->pci_dev->dev;
 	struct sensor_async_subdev *s_asd;
 	struct v4l2_async_connection *asd;
 	struct cio2_queue *q;
@@ -1417,23 +1417,10 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
 		s_asd = to_sensor_asd(asd);
 		q = &cio2->queue[s_asd->csi2.port];
 
-		ret = media_entity_get_fwnode_pad(&q->sensor->entity,
-						  s_asd->asd.match.fwnode,
-						  MEDIA_PAD_FL_SOURCE);
-		if (ret < 0) {
-			dev_err(dev, "no pad for endpoint %pfw (%d)\n",
-				s_asd->asd.match.fwnode, ret);
-			return ret;
-		}
-
-		ret = media_create_pad_link(&q->sensor->entity, ret,
-					    &q->subdev.entity, CIO2_PAD_SINK,
-					    0);
-		if (ret) {
-			dev_err(dev, "failed to create link for %s (endpoint %pfw, error %d)\n",
-				q->sensor->name, s_asd->asd.match.fwnode, ret);
+		ret = v4l2_create_fwnode_links_to_pad(asd->sd,
+						      &q->subdev_pads[CIO2_PAD_SINK], 0);
+		if (ret)
 			return ret;
-		}
 	}
 
 	return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
@@ -1572,6 +1559,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q)
 	v4l2_subdev_init(subdev, &cio2_subdev_ops);
 	subdev->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 	subdev->owner = THIS_MODULE;
+	subdev->dev = dev;
 	snprintf(subdev->name, sizeof(subdev->name),
 		 CIO2_ENTITY_NAME " %td", q - cio2->queue);
 	subdev->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
-- 
2.39.2

[PATCH v2 2/3] media: v4l2-mc: Add debug prints for v4l2_fwnode_create_links_for_pad()

[Sakari Ailus]

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

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

[PATCH v2 3/3] media: mc: Drop useless debug print on file handle release

[Sakari Ailus]

Drop a debug print in media_release(), which is a release callback for a
file handle. Printing a debug message here is simply not necessary.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/mc/mc-devnode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 680fbb3a9340..9c8fe9335dc1 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -190,7 +190,6 @@ static int media_release(struct inode *inode, struct file *filp)
 	   return value is ignored. */
 	put_device(&devnode->dev);
 
-	pr_debug("%s: Media Release\n", __func__);
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH v2 2/3] media: v4l2-mc: Add debug prints for v4l2_fwnode_create_links_for_pad()

[Laurent Pinchart]

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,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] media: v4l2-mc: Add debug prints for v4l2_fwnode_create_links_for_pad()

[Sakari Ailus]

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

-- 
Regards,

Sakari Ailus