[PATCH 0/2] media: im7: csi: fix csi for i.MX6UL/i.MX6ULL

[PATCH 0/2] media: im7: csi: fix csi for i.MX6UL/i.MX6ULL

[Rui Miguel Silva]

Fabio during the testing of i.MX6UL using a sensor detected a regression in the
imx7 CSI driver which have the same IP but do not have a CSI MUX like the
others platforms that share the same block.

The commit:
86e02d07871c ("media: imx5/6/7: csi: Mark a bound video mux as a CSI mux")

made the assumption that this would not happen. This series make it work again
for the i.MX6UL/i.MX6ULL platforms.

Cheers,
   Rui


Fabio Estevam (1):
  media: imx7: csi: Fix regression for parallel cameras on i.MX6UL

Rui Miguel Silva (1):
  media: imx7: csi: Fix pad link validation

 drivers/staging/media/imx/imx7-media-csi.c | 27 +++++++++++++++-------
 1 file changed, 19 insertions(+), 8 deletions(-)

-- 
2.30.0

[PATCH 1/2] media: imx7: csi: Fix regression for parallel cameras on i.MX6UL

[Rui Miguel Silva]

From: Fabio Estevam <festevam@gmail.com>

Commit 86e02d07871c ("media: imx5/6/7: csi: Mark a bound video mux as
a CSI mux") made an incorrect assumption that for imx7-media-csi,
the bound subdev must always be a CSI mux. On i.MX6UL/i.MX6ULL there
is no CSI mux at all, so do not return an error when the entity is not a
video mux and assign the IMX_MEDIA_GRP_ID_CSI_MUX group id only when
appropriate.

This is the same approach as done in imx-media-csi.c and it fixes the
csi probe regression on i.MX6UL.

Tested on a imx6ull-evk board.

Fixes: 86e02d07871c ("media: imx5/6/7: csi: Mark a bound video mux as a CSI mux")
Signed-off-by: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Rui Miguel Silva <rmfrfs@gmail.com>
---
 drivers/staging/media/imx/imx7-media-csi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index a3f3df901704..31e36168f9d0 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -1164,12 +1164,12 @@ static int imx7_csi_notify_bound(struct v4l2_async_notifier *notifier,
 	struct imx7_csi *csi = imx7_csi_notifier_to_dev(notifier);
 	struct media_pad *sink = &csi->sd.entity.pads[IMX7_CSI_PAD_SINK];
 
-	/* The bound subdev must always be the CSI mux */
-	if (WARN_ON(sd->entity.function != MEDIA_ENT_F_VID_MUX))
-		return -ENXIO;
-
-	/* Mark it as such via its group id */
-	sd->grp_id = IMX_MEDIA_GRP_ID_CSI_MUX;
+	/*
+	 * If the subdev is a video mux, it must be one of the CSI
+	 * muxes. Mark it as such via its group id.
+	 */
+	if (sd->entity.function == MEDIA_ENT_F_VID_MUX)
+		sd->grp_id = IMX_MEDIA_GRP_ID_CSI_MUX;
 
 	return v4l2_create_fwnode_links_to_pad(sd, sink);
 }
-- 
2.30.0

[PATCH 2/2] media: imx7: csi: Fix pad link validation

[Rui Miguel Silva]

We can not make the assumption that the bound subdev is always a CSI
mux, in i.MX6UL/i.MX6ULL that is not the case. So, just get the entity
selected by source directly upstream from the CSI.

Fixes: 86e02d07871c ("media: imx5/6/7: csi: Mark a bound video mux as a CSI mux")
Reported-by: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Rui Miguel Silva <rmfrfs@gmail.com>
Tested-by: Fabio Estevam <festevam@gmail.com>
Tested-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
---
 drivers/staging/media/imx/imx7-media-csi.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index 31e36168f9d0..ac52b1daf991 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -499,6 +499,7 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
 				      struct v4l2_subdev_format *sink_fmt)
 {
 	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+	struct media_entity *src;
 	struct media_pad *pad;
 	int ret;
 
@@ -509,11 +510,21 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
 	if (!csi->src_sd)
 		return -EPIPE;
 
+	src = &csi->src_sd->entity;
+
+	/*
+	 * if the source is neither a CSI MUX or CSI-2 get the one directly
+	 * upstream from this CSI
+	 */
+	if (src->function != MEDIA_ENT_F_VID_IF_BRIDGE &&
+	    src->function != MEDIA_ENT_F_VID_MUX)
+		src = &csi->sd.entity;
+
 	/*
-	 * find the entity that is selected by the CSI mux. This is needed
+	 * find the entity that is selected by the source. This is needed
 	 * to distinguish between a parallel or CSI-2 pipeline.
 	 */
-	pad = imx_media_pipeline_pad(&csi->src_sd->entity, 0, 0, true);
+	pad = imx_media_pipeline_pad(src, 0, 0, true);
 	if (!pad)
 		return -ENODEV;
 
-- 
2.30.0

Re: [PATCH 2/2] media: imx7: csi: Fix pad link validation

[Hans Verkuil]

On 07/01/2021 11:47, Rui Miguel Silva wrote:
> We can not make the assumption that the bound subdev is always a CSI
> mux, in i.MX6UL/i.MX6ULL that is not the case. So, just get the entity
> selected by source directly upstream from the CSI.

scripts/checkpatch.pl --strict:

WARNING: From:/Signed-off-by: email address mismatch: 'From: Rui Miguel Silva <rui.silva@linaro.org>' != 'Signed-off-by: Rui Miguel Silva
<rmfrfs@gmail.com>'

I can fix this if you let me know which email address I should pick.

Regards,

	Hans

> 
> Fixes: 86e02d07871c ("media: imx5/6/7: csi: Mark a bound video mux as a CSI mux")
> Reported-by: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Rui Miguel Silva <rmfrfs@gmail.com>
> Tested-by: Fabio Estevam <festevam@gmail.com>
> Tested-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 31e36168f9d0..ac52b1daf991 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -499,6 +499,7 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
>  				      struct v4l2_subdev_format *sink_fmt)
>  {
>  	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> +	struct media_entity *src;
>  	struct media_pad *pad;
>  	int ret;
>  
> @@ -509,11 +510,21 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
>  	if (!csi->src_sd)
>  		return -EPIPE;
>  
> +	src = &csi->src_sd->entity;
> +
> +	/*
> +	 * if the source is neither a CSI MUX or CSI-2 get the one directly
> +	 * upstream from this CSI
> +	 */
> +	if (src->function != MEDIA_ENT_F_VID_IF_BRIDGE &&
> +	    src->function != MEDIA_ENT_F_VID_MUX)
> +		src = &csi->sd.entity;
> +
>  	/*
> -	 * find the entity that is selected by the CSI mux. This is needed
> +	 * find the entity that is selected by the source. This is needed
>  	 * to distinguish between a parallel or CSI-2 pipeline.
>  	 */
> -	pad = imx_media_pipeline_pad(&csi->src_sd->entity, 0, 0, true);
> +	pad = imx_media_pipeline_pad(src, 0, 0, true);
>  	if (!pad)
>  		return -ENODEV;
>  
> 

Re: [PATCH 2/2] media: imx7: csi: Fix pad link validation

[Rui Miguel Silva]

Hi Hans,
On Wed, Jan 13, 2021 at 01:11:15PM +0100, Hans Verkuil wrote:
> On 07/01/2021 11:47, Rui Miguel Silva wrote:
> > We can not make the assumption that the bound subdev is always a CSI
> > mux, in i.MX6UL/i.MX6ULL that is not the case. So, just get the entity
> > selected by source directly upstream from the CSI.
> 
> scripts/checkpatch.pl --strict:
> 
> WARNING: From:/Signed-off-by: email address mismatch: 'From: Rui Miguel Silva <rui.silva@linaro.org>' != 'Signed-off-by: Rui Miguel Silva
> <rmfrfs@gmail.com>'
> 
> I can fix this if you let me know which email address I should pick.

Errr... Sorry about that, pick the gmail.com one please.
Thanks.

------
Cheers,
     Rui
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Fixes: 86e02d07871c ("media: imx5/6/7: csi: Mark a bound video mux as a CSI mux")
> > Reported-by: Fabio Estevam <festevam@gmail.com>
> > Signed-off-by: Rui Miguel Silva <rmfrfs@gmail.com>
> > Tested-by: Fabio Estevam <festevam@gmail.com>
> > Tested-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
> > ---
> >  drivers/staging/media/imx/imx7-media-csi.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > index 31e36168f9d0..ac52b1daf991 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -499,6 +499,7 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
> >  				      struct v4l2_subdev_format *sink_fmt)
> >  {
> >  	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> > +	struct media_entity *src;
> >  	struct media_pad *pad;
> >  	int ret;
> >  
> > @@ -509,11 +510,21 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
> >  	if (!csi->src_sd)
> >  		return -EPIPE;
> >  
> > +	src = &csi->src_sd->entity;
> > +
> > +	/*
> > +	 * if the source is neither a CSI MUX or CSI-2 get the one directly
> > +	 * upstream from this CSI
> > +	 */
> > +	if (src->function != MEDIA_ENT_F_VID_IF_BRIDGE &&
> > +	    src->function != MEDIA_ENT_F_VID_MUX)
> > +		src = &csi->sd.entity;
> > +
> >  	/*
> > -	 * find the entity that is selected by the CSI mux. This is needed
> > +	 * find the entity that is selected by the source. This is needed
> >  	 * to distinguish between a parallel or CSI-2 pipeline.
> >  	 */
> > -	pad = imx_media_pipeline_pad(&csi->src_sd->entity, 0, 0, true);
> > +	pad = imx_media_pipeline_pad(src, 0, 0, true);
> >  	if (!pad)
> >  		return -ENODEV;
> >  
> > 
> 

Re: [PATCH 2/2] media: imx7: csi: Fix pad link validation

[Laurent Pinchart]

Hi Rui,

Thank you for the patch.

On Thu, Jan 07, 2021 at 10:47:26AM +0000, Rui Miguel Silva wrote:
> We can not make the assumption that the bound subdev is always a CSI
> mux, in i.MX6UL/i.MX6ULL that is not the case. So, just get the entity
> selected by source directly upstream from the CSI.
> 
> Fixes: 86e02d07871c ("media: imx5/6/7: csi: Mark a bound video mux as a CSI mux")
> Reported-by: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Rui Miguel Silva <rmfrfs@gmail.com>
> Tested-by: Fabio Estevam <festevam@gmail.com>
> Tested-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 31e36168f9d0..ac52b1daf991 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -499,6 +499,7 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
>  				      struct v4l2_subdev_format *sink_fmt)
>  {
>  	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> +	struct media_entity *src;
>  	struct media_pad *pad;
>  	int ret;
>  
> @@ -509,11 +510,21 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
>  	if (!csi->src_sd)
>  		return -EPIPE;
>  
> +	src = &csi->src_sd->entity;
> +
> +	/*
> +	 * if the source is neither a CSI MUX or CSI-2 get the one directly
> +	 * upstream from this CSI
> +	 */
> +	if (src->function != MEDIA_ENT_F_VID_IF_BRIDGE &&
> +	    src->function != MEDIA_ENT_F_VID_MUX)
> +		src = &csi->sd.entity;
> +
>  	/*
> -	 * find the entity that is selected by the CSI mux. This is needed
> +	 * find the entity that is selected by the source. This is needed
>  	 * to distinguish between a parallel or CSI-2 pipeline.
>  	 */
> -	pad = imx_media_pipeline_pad(&csi->src_sd->entity, 0, 0, true);
> +	pad = imx_media_pipeline_pad(src, 0, 0, true);
>  	if (!pad)
>  		return -ENODEV;

This is a bit over-complicated. If the source entity isn't a mux or
bridge, then the input is always parallel. I'll send a follow up patch
to simplify this, as this one has been merged already.

By the way, it would be entirely possible for an external entity,
connected through a parallel interface, to have a function set to
MEDIA_ENT_F_VID_IF_BRIDGE or MEDIA_ENT_F_VID_MUX. This code is thus not
very robust, and will need to be fixed.


-- 
Regards,

Laurent Pinchart