diff mbox series

[v4] rcar-csi2: Propagate the FLD signal for NTSC and PAL

Message ID 20190411203444.23515-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive)
State New, archived
Headers show
Series [v4] rcar-csi2: Propagate the FLD signal for NTSC and PAL | expand

Commit Message

Niklas Söderlund April 11, 2019, 8:34 p.m. UTC 
Depending on which video standard is used the driver needs to setup the
hardware to correctly handle fields. If stream is identified as NTSC
or PAL setup field detection and propagate the field detection signal.

Later versions of the datasheet have been updated to make it clear
that FLD register should be set to 0 when dealing with non-interlaced
field formats.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Jacopo Mondi April 12, 2019, 8:43 a.m. UTC | #1 
Hi Niklas,
   I'm rebasing v4l2-mux series on top of your new VIN patches and
on this on I'm not sure how to proceed. Please see below.

Please also note this comment is not strictly on this patch, and should not
block its acceptance...

On Thu, Apr 11, 2019 at 10:34:44PM +0200, Niklas Söderlund wrote:
> Depending on which video standard is used the driver needs to setup the
> hardware to correctly handle fields. If stream is identified as NTSC
> or PAL setup field detection and propagate the field detection signal.
>
> Later versions of the datasheet have been updated to make it clear
> that FLD register should be set to 0 when dealing with non-interlaced
> field formats.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 799e526fd3df5554..c1b38ebd061dbc35 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -68,6 +68,7 @@ struct rcar_csi2;
>  /* Field Detection Control */
>  #define FLD_REG				0x1c
>  #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
> +#define FLD_DET_SEL(n)			(((n) & 0x3) << 4)
>  #define FLD_FLD_EN4			BIT(3)
>  #define FLD_FLD_EN3			BIT(2)
>  #define FLD_FLD_EN2			BIT(1)
> @@ -475,7 +476,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  {
>  	const struct rcar_csi2_format *format;
> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
>  	unsigned int i;
>  	int mbps, ret;
>
> @@ -507,6 +508,16 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
>  	}
>
> +	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {

How would you handle this once priv->mf is expanded to become an array
of 'struct v4l2_mbus_framefmt' entries [1] ? Which of them is relevant for
field detection?


> +		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
> +			| FLD_FLD_EN;
> +
> +		if (priv->mf.height == 240)

Same question here, even if I assume we could cycle on the entries and
collect the maximum size.

Thanks
  j

[1] Introduced by:
"rcar-csi2: use frame description information to configure CSI-2 bus"
> +			fld |= FLD_FLD_NUM(0);
> +		else
> +			fld |= FLD_FLD_NUM(1);
> +	}
> +
>  	phycnt = PHYCNT_ENABLECLK;
>  	phycnt |= (1 << priv->lanes) - 1;
>
> @@ -549,8 +560,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	rcsi2_write(priv, PHYCNT_REG, phycnt);
>  	rcsi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN |
>  		    LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP);
> -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> +	rcsi2_write(priv, FLD_REG, fld);
>  	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ);
>  	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | PHYCNT_RSTZ);
>
> --
> 2.21.0
>
Niklas Söderlund April 12, 2019, 9:13 a.m. UTC | #2 
Hi Jacopo,

Thanks for your feedback.

On 2019-04-12 10:43:46 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>    I'm rebasing v4l2-mux series on top of your new VIN patches and
> on this on I'm not sure how to proceed. Please see below.
> 
> Please also note this comment is not strictly on this patch, and should not
> block its acceptance...
> 
> On Thu, Apr 11, 2019 at 10:34:44PM +0200, Niklas Söderlund wrote:
> > Depending on which video standard is used the driver needs to setup the
> > hardware to correctly handle fields. If stream is identified as NTSC
> > or PAL setup field detection and propagate the field detection signal.
> >
> > Later versions of the datasheet have been updated to make it clear
> > that FLD register should be set to 0 when dealing with non-interlaced
> > field formats.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 799e526fd3df5554..c1b38ebd061dbc35 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -68,6 +68,7 @@ struct rcar_csi2;
> >  /* Field Detection Control */
> >  #define FLD_REG				0x1c
> >  #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
> > +#define FLD_DET_SEL(n)			(((n) & 0x3) << 4)
> >  #define FLD_FLD_EN4			BIT(3)
> >  #define FLD_FLD_EN3			BIT(2)
> >  #define FLD_FLD_EN2			BIT(1)
> > @@ -475,7 +476,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  {
> >  	const struct rcar_csi2_format *format;
> > -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> > +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> >  	unsigned int i;
> >  	int mbps, ret;
> >
> > @@ -507,6 +508,16 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >  	}
> >
> > +	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> 
> How would you handle this once priv->mf is expanded to become an array
> of 'struct v4l2_mbus_framefmt' entries [1] ? Which of them is relevant for
> field detection?
> 
> 
> > +		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
> > +			| FLD_FLD_EN;
> > +
> > +		if (priv->mf.height == 240)
> 
> Same question here, even if I assume we could cycle on the entries and
> collect the maximum size.

Well as the hardware do not support field detection of PAL and NTSC at 
the same time one would need to either disallow that and fail stream on 
or pick one and move on. Not sure which one would be best.

> 
> Thanks
>   j
> 
> [1] Introduced by:
> "rcar-csi2: use frame description information to configure CSI-2 bus"
> > +			fld |= FLD_FLD_NUM(0);
> > +		else
> > +			fld |= FLD_FLD_NUM(1);
> > +	}
> > +
> >  	phycnt = PHYCNT_ENABLECLK;
> >  	phycnt |= (1 << priv->lanes) - 1;
> >
> > @@ -549,8 +560,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  	rcsi2_write(priv, PHYCNT_REG, phycnt);
> >  	rcsi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN |
> >  		    LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP);
> > -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> > -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> > +	rcsi2_write(priv, FLD_REG, fld);
> >  	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ);
> >  	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | PHYCNT_RSTZ);
> >
> > --
> > 2.21.0
> >
Laurent Pinchart April 12, 2019, 9:47 a.m. UTC | #3 
Hi Niklas,

On Fri, Apr 12, 2019 at 11:13:21AM +0200, Niklas Söderlund wrote:
> On 2019-04-12 10:43:46 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >    I'm rebasing v4l2-mux series on top of your new VIN patches and
> > on this on I'm not sure how to proceed. Please see below.
> > 
> > Please also note this comment is not strictly on this patch, and should not
> > block its acceptance...
> > 
> > On Thu, Apr 11, 2019 at 10:34:44PM +0200, Niklas Söderlund wrote:
> >> Depending on which video standard is used the driver needs to setup the
> >> hardware to correctly handle fields. If stream is identified as NTSC
> >> or PAL setup field detection and propagate the field detection signal.
> >>
> >> Later versions of the datasheet have been updated to make it clear
> >> that FLD register should be set to 0 when dealing with non-interlaced
> >> field formats.
> >>
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> ---
> >>  drivers/media/platform/rcar-vin/rcar-csi2.c | 16 +++++++++++++---
> >>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> index 799e526fd3df5554..c1b38ebd061dbc35 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> @@ -68,6 +68,7 @@ struct rcar_csi2;
> >>  /* Field Detection Control */
> >>  #define FLD_REG				0x1c
> >>  #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
> >> +#define FLD_DET_SEL(n)			(((n) & 0x3) << 4)
> >>  #define FLD_FLD_EN4			BIT(3)
> >>  #define FLD_FLD_EN3			BIT(2)
> >>  #define FLD_FLD_EN2			BIT(1)
> >> @@ -475,7 +476,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>  {
> >>  	const struct rcar_csi2_format *format;
> >> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> >> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> >>  	unsigned int i;
> >>  	int mbps, ret;
> >>
> >> @@ -507,6 +508,16 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >>  	}
> >>
> >> +	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> > 
> > How would you handle this once priv->mf is expanded to become an array
> > of 'struct v4l2_mbus_framefmt' entries [1] ? Which of them is relevant for
> > field detection?
> > 
> > 
> >> +		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
> >> +			| FLD_FLD_EN;
> >> +
> >> +		if (priv->mf.height == 240)
> > 
> > Same question here, even if I assume we could cycle on the entries and
> > collect the maximum size.
> 
> Well as the hardware do not support field detection of PAL and NTSC at 
> the same time one would need to either disallow that and fail stream on 
> or pick one and move on. Not sure which one would be best.

Can't you do this through the VnMC.FOC bit ? I wonder if we shouldn't
hardcode FLD_NUM here and handle PAL vs. NTSC in the VIN.

> > [1] Introduced by:
> > "rcar-csi2: use frame description information to configure CSI-2 bus"
> >> +			fld |= FLD_FLD_NUM(0);
> >> +		else
> >> +			fld |= FLD_FLD_NUM(1);
> >> +	}
> >> +
> >>  	phycnt = PHYCNT_ENABLECLK;
> >>  	phycnt |= (1 << priv->lanes) - 1;
> >>
> >> @@ -549,8 +560,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>  	rcsi2_write(priv, PHYCNT_REG, phycnt);
> >>  	rcsi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN |
> >>  		    LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP);
> >> -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> >> -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> >> +	rcsi2_write(priv, FLD_REG, fld);
> >>  	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ);
> >>  	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | PHYCNT_RSTZ);
> >>
Niklas Söderlund April 12, 2019, 10:08 a.m. UTC | #4 
Hi Laurent,

On 2019-04-12 12:47:22 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Fri, Apr 12, 2019 at 11:13:21AM +0200, Niklas Söderlund wrote:
> > On 2019-04-12 10:43:46 +0200, Jacopo Mondi wrote:
> > > Hi Niklas,
> > >    I'm rebasing v4l2-mux series on top of your new VIN patches and
> > > on this on I'm not sure how to proceed. Please see below.
> > > 
> > > Please also note this comment is not strictly on this patch, and should not
> > > block its acceptance...
> > > 
> > > On Thu, Apr 11, 2019 at 10:34:44PM +0200, Niklas Söderlund wrote:
> > >> Depending on which video standard is used the driver needs to setup the
> > >> hardware to correctly handle fields. If stream is identified as NTSC
> > >> or PAL setup field detection and propagate the field detection signal.
> > >>
> > >> Later versions of the datasheet have been updated to make it clear
> > >> that FLD register should be set to 0 when dealing with non-interlaced
> > >> field formats.
> > >>
> > >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >> ---
> > >>  drivers/media/platform/rcar-vin/rcar-csi2.c | 16 +++++++++++++---
> > >>  1 file changed, 13 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> index 799e526fd3df5554..c1b38ebd061dbc35 100644
> > >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> @@ -68,6 +68,7 @@ struct rcar_csi2;
> > >>  /* Field Detection Control */
> > >>  #define FLD_REG				0x1c
> > >>  #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
> > >> +#define FLD_DET_SEL(n)			(((n) & 0x3) << 4)
> > >>  #define FLD_FLD_EN4			BIT(3)
> > >>  #define FLD_FLD_EN3			BIT(2)
> > >>  #define FLD_FLD_EN2			BIT(1)
> > >> @@ -475,7 +476,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> > >>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> > >>  {
> > >>  	const struct rcar_csi2_format *format;
> > >> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> > >> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> > >>  	unsigned int i;
> > >>  	int mbps, ret;
> > >>
> > >> @@ -507,6 +508,16 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> > >>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> > >>  	}
> > >>
> > >> +	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> > > 
> > > How would you handle this once priv->mf is expanded to become an array
> > > of 'struct v4l2_mbus_framefmt' entries [1] ? Which of them is relevant for
> > > field detection?
> > > 
> > > 
> > >> +		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
> > >> +			| FLD_FLD_EN;
> > >> +
> > >> +		if (priv->mf.height == 240)
> > > 
> > > Same question here, even if I assume we could cycle on the entries and
> > > collect the maximum size.
> > 
> > Well as the hardware do not support field detection of PAL and NTSC at 
> > the same time one would need to either disallow that and fail stream on 
> > or pick one and move on. Not sure which one would be best.
> 
> Can't you do this through the VnMC.FOC bit ? I wonder if we shouldn't
> hardcode FLD_NUM here and handle PAL vs. NTSC in the VIN.

I don't think so. My interpretation is that VnMC.FOC controls how the 
fields are combined when using the VIN mode of interlacing ALTERNETING 
and providing a INTERLACED frame to user-space.

My interpretation is that the change in this patch is visible VnINTS.FIS 
which can be used to finaly add support for delivering ALTERNATE 
directly to user-space as well as doing the right thing when interlacing 
with the VnMC.FOC setting.

> 
> > > [1] Introduced by:
> > > "rcar-csi2: use frame description information to configure CSI-2 bus"
> > >> +			fld |= FLD_FLD_NUM(0);
> > >> +		else
> > >> +			fld |= FLD_FLD_NUM(1);
> > >> +	}
> > >> +
> > >>  	phycnt = PHYCNT_ENABLECLK;
> > >>  	phycnt |= (1 << priv->lanes) - 1;
> > >>
> > >> @@ -549,8 +560,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> > >>  	rcsi2_write(priv, PHYCNT_REG, phycnt);
> > >>  	rcsi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN |
> > >>  		    LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP);
> > >> -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> > >> -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> > >> +	rcsi2_write(priv, FLD_REG, fld);
> > >>  	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ);
> > >>  	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | PHYCNT_RSTZ);
> > >>
> 
> -- 
> Regards,
> 
> Laurent Pinchart
diff mbox series

Patch

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 799e526fd3df5554..c1b38ebd061dbc35 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -68,6 +68,7 @@  struct rcar_csi2;
 /* Field Detection Control */
 #define FLD_REG				0x1c
 #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
+#define FLD_DET_SEL(n)			(((n) & 0x3) << 4)
 #define FLD_FLD_EN4			BIT(3)
 #define FLD_FLD_EN3			BIT(2)
 #define FLD_FLD_EN2			BIT(1)
@@ -475,7 +476,7 @@  static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
 static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 {
 	const struct rcar_csi2_format *format;
-	u32 phycnt, vcdt = 0, vcdt2 = 0;
+	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
 	unsigned int i;
 	int mbps, ret;
 
@@ -507,6 +508,16 @@  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 			vcdt2 |= vcdt_part << ((i % 2) * 16);
 	}
 
+	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
+		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
+			| FLD_FLD_EN;
+
+		if (priv->mf.height == 240)
+			fld |= FLD_FLD_NUM(0);
+		else
+			fld |= FLD_FLD_NUM(1);
+	}
+
 	phycnt = PHYCNT_ENABLECLK;
 	phycnt |= (1 << priv->lanes) - 1;
 
@@ -549,8 +560,7 @@  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 	rcsi2_write(priv, PHYCNT_REG, phycnt);
 	rcsi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN |
 		    LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP);
-	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
-		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
+	rcsi2_write(priv, FLD_REG, fld);
 	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ);
 	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | PHYCNT_RSTZ);