Re: [RFC PATCH] media: rcar-csi2: Fix field detection

[Niklas Söderlund]

Hi Steve,

Thanks for your work.

I upported most rcar-csi2 patches but a few of them are still pending, 
an alternating version based on this BSP patch is already on its way.

https://patchwork.linuxtv.org/patch/55623/

As a general feedback calling v4l2_subdev_call() for g_std is not the 
best way to handle this problem in a MC centric driver. There is a 
discussion about in in earlier versions of the patch mentioned above.

On 2019-04-16 16:38:07 -0700, Steve Longerbeam wrote:
> The RCAR Gen3 hardware manual doesn't mention that the "Word Count" field
> in the MIPI CSI-2 Frame Start packet is really the frame number. FS packets
> are short packets, and short packets do not have a WC but rather a 16-bit
> data field, and for Frame synchronization packets (FS/FE) the 16-bit data
> field contains a frame number.
> 
> Here is a reprinting from the MIPI CSI-2 specification, version 2
> (section 9.1.2 Low Level Protocol Short Packet Format):
> 
> "Figure 42 and Figure 43 show the Low Level Protocol Short Packet
> structures for the D-PHY and C-PHY physical layer options, respectively.
> For each option, the Short Packet structure matches the Packet Header of
> the corresponding Low Level Protocol Long Packet structure with the
> exception that the Packet Header Word Count (WC) field shall be replaced
> by the Short Packet Data Field...For Frame Synchronization Data Types
> the Short Packet Data Field shall be the frame number..."
> 
> Also in section 9.8.1 Frame Synchronization Packets, the CSI-2 spec reads:
> 
> "The behavior of the 16-bit frame number shall be as one of the following
> - Frame number is always zero â frame number is inoperative.
> - Frame number increments by 1 for every FS packet with the same Virtual
>   Channel and is periodically reset to one e.g. 1, 2, 1, 2, 1, 2, 1, 2 or
>   1, 2, 3, 4, 1, 2, 3, 4"
> 
> So from the above, FLD_NUM in the FLD register matches against the
> frame number transmitted by the CSI-2 source in the Frame Start Packet,
> and goes as "1,2,1,2,1,2" or "1,2,3,4,...,1,2,3,4". If there is a match,
> the RCAR CSI-2 receiver declares the field as the even, e.g. the BOTTOM,
> field.
> 
> NTSC transmits bottom/even field first, and PAL and HD transmit top/odd
> field first. So fields with a frame number 1 in the CSI-2 Frame Start
> Packet are bottom/even fields for NTSC, and frame number 2 are bottom/even
> fields for PAL/HD.
> 
> But the above assumes the transmitting CSI-2 sensor is sending frame
> numbers in the sequence "1,2,1,2,...". But according to the CSI-2 spec,
> sensors are also allowed to send frame numbers that simply increment as
> in "1,2,3,4,...". To generalize to catch those cases, set FLD_DET_SEL
> to 0x01, which according to the RCAR Gen3 hardware manual means "the
> field is detected as the EVEN field when FLD_NUM[0] matches WC[0]".
> Then, the even/bottom fields have odd frame numbers for NTSC, and even
> frame numbers for PAL (this patch assumes HDMI sources will not
> implement .g_std(), so std will be 0 in that case).
> 
> Finally, set the FLD register to non-zero value only if trnsmitter is
> sending ALTERNATE fields.
> 
> Signed-off-by: Steve Longerbeam <slongerbeam@xxxxxxxxx>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 37 ++++++++++++++++++---
>  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index a438ec2c218f..c5bee20d7907 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -67,6 +67,7 @@ struct rcar_csi2;
>  /* Field Detection Control */
>  #define FLD_REG				0x1c
>  #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
> +#define FLD_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)
> @@ -462,10 +463,11 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
>  	return mbps;
>  }
>  
> -static int rcsi2_start(struct rcar_csi2 *priv)
> +static int rcsi2_start(struct rcar_csi2 *priv, struct v4l2_subdev *nextsd)
>  {
>  	const struct rcar_csi2_format *format;
> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> +	v4l2_std_id std = 0;
>  	unsigned int i;
>  	int mbps, ret;
>  
> @@ -476,6 +478,10 @@ static int rcsi2_start(struct rcar_csi2 *priv)
>  	/* Code is validated in set_fmt. */
>  	format = rcsi2_code_to_fmt(priv->mf.code);
>  
> +	ret = v4l2_subdev_call(nextsd, video, g_std, &std);
> +	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> +		return ret;
> +
>  	/*
>  	 * Enable all supported CSI-2 channels with virtual channel and
>  	 * data type matching.
> @@ -509,9 +515,30 @@ static int rcsi2_start(struct rcar_csi2 *priv)
>  	rcsi2_reset(priv);
>  	rcsi2_write(priv, PHTC_REG, 0);
>  
> +	/*
> +	 * NTSC standard transmits even/bottom field first, then odd/top.
> +	 * PAL standard and interlaced HD transmit odd/top field first,
> +	 * then even/bottom.
> +	 *
> +	 * For CSI-2 sensors that transmit frame numbers (in the CSI-2
> +	 * Frame Start packet) as "1,2,1,2,..." or "1,2,3,4,5,...", then:
> +	 *
> +	 * - for NTSC, the even/bottom fields have odd frame numbers.
> +	 * - for PAL and HD, the even/bottom fields have even frame numbers.
> +	 */
> +	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> +		fld = FLD_FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 |
> +			FLD_FLD_EN2 | FLD_FLD_EN;
> +
> +		if (std & V4L2_STD_525_60)
> +			fld |= FLD_FLD_NUM(1); /* NTSC */
> +		else
> +			fld |= FLD_FLD_NUM(0); /* PAL or HD */
> +	}
> +
>  	/* Configure */
> -	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, VCDT_REG, vcdt);
>  	if (vcdt2)
>  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> @@ -591,7 +618,7 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (enable && priv->stream_count == 0) {
>  		pm_runtime_get_sync(priv->dev);
>  
> -		ret = rcsi2_start(priv);
> +		ret = rcsi2_start(priv, nextsd);
>  		if (ret) {
>  			pm_runtime_put(priv->dev);
>  			goto out;
> -- 
> 2.17.1
> 

-- 
Regards,
Niklas SÃderlund