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@xxxxxxxxxxxx>
> > >> ---
> > >> 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
--
Regards,
Niklas Söderlund
