| Message ID | 20190411203444.23515-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [v4] rcar-csi2: Propagate the FLD signal for NTSC and PAL | expand |
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 >
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 > >
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); > >>
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 --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);
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(-)