diff mbox series

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

Message ID 20210107104726.1294538-3-rmfrfs@gmail.com
State New
Headers show
Series media: im7: csi: fix csi for i.MX6UL/i.MX6ULL | expand

Commit Message

Rui Miguel Silva Jan. 7, 2021, 10:47 a.m. UTC 
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(-)

-- 
2.30.0

Comments

Hans Verkuil Jan. 13, 2021, 12:11 p.m. UTC | #1 
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;

>  

>
Rui Miguel Silva Jan. 13, 2021, 12:18 p.m. UTC | #2 
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;

> >  

> > 

>
Laurent Pinchart Feb. 14, 2021, 11:23 p.m. UTC | #3 
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
diff mbox series

Patch

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;