[PATCH 0/3] staging: media: imx: imx7_mipi_csis: miscellaneous fixes

[PATCH 0/3] staging: media: imx: imx7_mipi_csis: miscellaneous fixes

[Laurent Pinchart]

Hello,

Here's a small set of additional fixes for the imx7_mipi_csis driver.
There's not much else to say here, please see individual patches for
details.

Laurent Pinchart (3):
  media: imx: imx7_mipi_csis: Runtime suspend in .s_stream() error path
  media: imx: imx7_mipi_csis: Don't take state->lock in .link_setup()
  media: imx: imx7_mipi_csis: Ensure pads are connected

 drivers/staging/media/imx/imx7-mipi-csis.c | 27 +++++++++++-----------
 1 file changed, 13 insertions(+), 14 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/3] media: imx: imx7_mipi_csis: Runtime suspend in .s_stream() error path

[Laurent Pinchart]

If the .s_stream() handler fails after calling pm_runtime_get_sync(),
call pm_runtime_put() in the error path.

While at it add a few blank lines to make the code more readable.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index bd587e550d99..c6cd60896969 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -694,6 +694,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
 			return ret;
 
 		mipi_csis_clear_counters(state);
+
 		ret = pm_runtime_get_sync(&state->pdev->dev);
 		if (ret < 0) {
 			pm_runtime_put_noidle(&state->pdev->dev);
@@ -701,10 +702,11 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
 		}
 		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
 		if (ret < 0 && ret != -ENOIOCTLCMD)
-			return ret;
+			goto done;
 	}
 
 	mutex_lock(&state->lock);
+
 	if (enable) {
 		if (state->flags & ST_SUSPENDED) {
 			ret = -EBUSY;
@@ -732,7 +734,9 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
 
 unlock:
 	mutex_unlock(&state->lock);
-	if (!enable)
+
+done:
+	if (!enable || ret < 0)
 		pm_runtime_put(&state->pdev->dev);
 
 	return ret;
-- 
Regards,

Laurent Pinchart

[PATCH 2/3] media: imx: imx7_mipi_csis: Don't take state->lock in .link_setup()

[Laurent Pinchart]

The .link_setup() handler uses state->lock to protect the src_sd field.
This is only used in mipi_csis_s_stream(), which can't race
.link_setup() as the MC core prevents link setup when the pipeline is
streaming. Drop the lock.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index c6cd60896969..21774f3e66e0 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -749,7 +749,6 @@ static int mipi_csis_link_setup(struct media_entity *entity,
 	struct v4l2_subdev *mipi_sd = media_entity_to_v4l2_subdev(entity);
 	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
 	struct v4l2_subdev *remote_sd;
-	int ret = 0;
 
 	dev_dbg(state->dev, "link setup %s -> %s", remote_pad->entity->name,
 		local_pad->entity->name);
@@ -760,22 +759,16 @@ static int mipi_csis_link_setup(struct media_entity *entity,
 
 	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
 
-	mutex_lock(&state->lock);
-
 	if (flags & MEDIA_LNK_FL_ENABLED) {
-		if (state->src_sd) {
-			ret = -EBUSY;
-			goto out;
-		}
+		if (state->src_sd)
+			return -EBUSY;
 
 		state->src_sd = remote_sd;
 	} else {
 		state->src_sd = NULL;
 	}
 
-out:
-	mutex_unlock(&state->lock);
-	return ret;
+	return 0;
 }
 
 static struct v4l2_mbus_framefmt *
-- 
Regards,

Laurent Pinchart

[PATCH 3/3] media: imx: imx7_mipi_csis: Ensure pads are connected

[Laurent Pinchart]

The CSI-2 receiver requires both its sink pad and its source pad to be
connected through enabled links in order to be started. Ensure this by
setting the MEDIA_PAD_FL_MUST_CONNECT flag on both pads.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 21774f3e66e0..025fdc488bd6 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -1065,8 +1065,10 @@ static int mipi_csis_subdev_init(struct v4l2_subdev *mipi_sd,
 
 	v4l2_set_subdevdata(mipi_sd, &pdev->dev);
 
-	state->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
-	state->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+	state->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK
+					 | MEDIA_PAD_FL_MUST_CONNECT;
+	state->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE
+					   | MEDIA_PAD_FL_MUST_CONNECT;
 	return media_entity_pads_init(&mipi_sd->entity, CSIS_PADS_NUM,
 				      state->pads);
 }
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] media: imx: imx7_mipi_csis: Runtime suspend in .s_stream() error path

[Rui Miguel Silva]

Hi Laurent,
On Fri, Mar 19, 2021 at 03:17:33AM +0200, Laurent Pinchart wrote:
> If the .s_stream() handler fails after calling pm_runtime_get_sync(),
> call pm_runtime_put() in the error path.
> 
> While at it add a few blank lines to make the code more readable.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks LGTM,

Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>

------
Cheers,
     Rui

> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index bd587e550d99..c6cd60896969 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -694,6 +694,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
>  			return ret;
>  
>  		mipi_csis_clear_counters(state);
> +
>  		ret = pm_runtime_get_sync(&state->pdev->dev);
>  		if (ret < 0) {
>  			pm_runtime_put_noidle(&state->pdev->dev);
> @@ -701,10 +702,11 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
>  		}
>  		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
>  		if (ret < 0 && ret != -ENOIOCTLCMD)
> -			return ret;
> +			goto done;
>  	}
>  
>  	mutex_lock(&state->lock);
> +
>  	if (enable) {
>  		if (state->flags & ST_SUSPENDED) {
>  			ret = -EBUSY;
> @@ -732,7 +734,9 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
>  
>  unlock:
>  	mutex_unlock(&state->lock);
> -	if (!enable)
> +
> +done:
> +	if (!enable || ret < 0)
>  		pm_runtime_put(&state->pdev->dev);
>  
>  	return ret;
> -- 
> Regards,
> 
> Laurent Pinchart
>
> 

Re: [PATCH 2/3] media: imx: imx7_mipi_csis: Don't take state->lock in .link_setup()

[Rui Miguel Silva]

Hi Laurent,
On Fri, Mar 19, 2021 at 03:17:34AM +0200, Laurent Pinchart wrote:
> The .link_setup() handler uses state->lock to protect the src_sd field.
> This is only used in mipi_csis_s_stream(), which can't race
> .link_setup() as the MC core prevents link setup when the pipeline is
> streaming. Drop the lock.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>

------
Cheers,
     Rui
> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index c6cd60896969..21774f3e66e0 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -749,7 +749,6 @@ static int mipi_csis_link_setup(struct media_entity *entity,
>  	struct v4l2_subdev *mipi_sd = media_entity_to_v4l2_subdev(entity);
>  	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
>  	struct v4l2_subdev *remote_sd;
> -	int ret = 0;
>  
>  	dev_dbg(state->dev, "link setup %s -> %s", remote_pad->entity->name,
>  		local_pad->entity->name);
> @@ -760,22 +759,16 @@ static int mipi_csis_link_setup(struct media_entity *entity,
>  
>  	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
>  
> -	mutex_lock(&state->lock);
> -
>  	if (flags & MEDIA_LNK_FL_ENABLED) {
> -		if (state->src_sd) {
> -			ret = -EBUSY;
> -			goto out;
> -		}
> +		if (state->src_sd)
> +			return -EBUSY;
>  
>  		state->src_sd = remote_sd;
>  	} else {
>  		state->src_sd = NULL;
>  	}
>  
> -out:
> -	mutex_unlock(&state->lock);
> -	return ret;
> +	return 0;
>  }
>  
>  static struct v4l2_mbus_framefmt *
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH 3/3] media: imx: imx7_mipi_csis: Ensure pads are connected

[Rui Miguel Silva]

Hi Laurent,
Thanks for the all series.
On Fri, Mar 19, 2021 at 03:17:35AM +0200, Laurent Pinchart wrote:
> The CSI-2 receiver requires both its sink pad and its source pad to be
> connected through enabled links in order to be started. Ensure this by
> setting the MEDIA_PAD_FL_MUST_CONNECT flag on both pads.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>

------
Cheers,
     Rui

> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 21774f3e66e0..025fdc488bd6 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -1065,8 +1065,10 @@ static int mipi_csis_subdev_init(struct v4l2_subdev *mipi_sd,
>  
>  	v4l2_set_subdevdata(mipi_sd, &pdev->dev);
>  
> -	state->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> -	state->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +	state->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK
> +					 | MEDIA_PAD_FL_MUST_CONNECT;
> +	state->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE
> +					   | MEDIA_PAD_FL_MUST_CONNECT;
>  	return media_entity_pads_init(&mipi_sd->entity, CSIS_PADS_NUM,
>  				      state->pads);
>  }
> -- 
> Regards,
> 
> Laurent Pinchart
>