Re: [PATCH] media: i2c: adv748x: initialize bit 7 of csi_tx_top_reg_1f

Re: [PATCH] media: i2c: adv748x: initialize bit 7 of csi_tx_top_reg_1f

[Kieran Bingham]

Hi Michael,

Thank you for the patch,

Could the patch title be a bit better described? Perhaps:

media: i2c: adv748x: Correct EAV F-bit handling


On 10/05/2019 16:30, Michael Rodin wrote:
> According to pages 188 and 193 of the "UG-747: ADV7481 Reference Manual"
> [1], the bit 7 (FRAMENUMBER_INTERLACED) of the register csi_tx_top_reg_1f
> "sets association of frame number in the FS and FE packets with the F bit
> in the EAV/SAV codes". EAV/SAV codes are defined in [2].
> According to [2]:

Could you detail where this reference is to help searching for it
please? (it looks like Table 2, page 5)

> F=0 for field 1
> F=1 for field 2

It would help to indent those lines to make the whole text a bit more
readable...

> The bit FRAMENUMBER_INTERLACED is not initialized anywhere in the current
> version of the adv748x driver and therefore it is always 0, which is the
> default value according to [1]. Therefore the current association of field
> number from EAV/SAV code with the frame number in CSI FS and FE packets is:
> field 1 (top field for PAL, bottom field for NTSC) -> CSI frame number 2
> field 2 (bottom field for PAL, top field for NTSC) -> CSI frame number 1
> But this breaks frame number based field detection of top/bottom fields
> in CSI receivers. Therefore it makes sense to initialize the
> FRAMENUMBER_INTERLACED bit to 1 so the association is as expected:
> field 1 -> frame number 1
> field 2 -> frame number 2

I'm a bit worried by this patch, as it implies that we would have had
our fields inverted in our testing, or if they are not - then applying
this patch will then invert them! So either way we need to check this
carefully.

I can see the EAV:F values match the description above in the document...

Niklas, how does RCar-CSI determine the top/bottom sequence?

Do we have field inversion currently? (or one which is perhaps swapped
somewhere along the pipeline in rcar-vin?)


> [1] https://www.analog.com/en/products/adv7481.html
> [2] https://www.itu.int/rec/R-REC-BT.656-5-200712-I/en
> 
> Suggested-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index f57cd77..4dd1a13 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -278,6 +278,9 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
>  	usleep_range(1000, 1500);
>  	adv748x_write_check(state, page, 0x31, 0x80, &ret);
>  
> +	/* set bit 7 (FRAMENUMBER_INTERLACED) in csi_tx_top_reg_1f */
> +	adv748x_write_check(state, page, 0x1f, 0x80, &ret);

I think it would be nice to store the bit as macro defines in
drivers/media/i2c/adv748x/adv748x.h

I appreciate that the rest of this function does not (yet) do that
however, (it has been derived from a table of ADI required writes) - but
I think now that it is split from a table to a function it could be nice
to clean up the 'magic numbers' along the way.

Of course if you have time to convert the rest of the function as well
(in a separate patch) that might be a nice cleanup, but not required.

> +
>  	return ret;
>  }
>  
> 

-- 
Regards
--
Kieran

Re: [PATCH] media: i2c: adv748x: initialize bit 7 of csi_tx_top_reg_1f

[Niklas Söderlund]

Hi Kieran and Michael,

On 2019-05-10 17:25:24 +0100, Kieran Bingham wrote:
<snip>

> 
> Niklas, how does RCar-CSI determine the top/bottom sequence?

That patch just got merged in the media tree a few days ago,

    9f7983bdc4925ae2 ("media: rcar-csi2: Propagate the FLD signal for NTSC and PAL")

> 
> Do we have field inversion currently? (or one which is perhaps swapped
> somewhere along the pipeline in rcar-vin?)
> 

I'm not sure which tree this patch is developed on but Steve Longerbeam 
posted a RFC which IMHO had the fields inverted, there was a discussion 
in his thread [1] where I tried to get to the bottom of the issue. My 
conclusions there might be wrong due to the issues addressed in this 
patch.

Michael: Did you have Steve's patch in your tree when testing this?

1. https://patchwork.kernel.org/patch/10904263/

-- 
Regards,
Niklas Söderlund

RE: [PATCH] media: i2c: adv748x: initialize bit 7 of csi_tx_top_reg_1f

[Rodin, Michael (Ferchau; ADITG/ESM1)]

Hi Kieran and Niklas,

thank you for your responses!

> Hi Kieran and Michael,
>
> On 2019-05-10 17:25:24 +0100, Kieran Bingham wrote:
> <snip>
>
> >
> > Niklas, how does RCar-CSI determine the top/bottom sequence?
>
> That patch just got merged in the media tree a few days ago,
>
>     9f7983bdc4925ae2 ("media: rcar-csi2: Propagate the FLD signal for NTSC
> and PAL")
>
> >
> > Do we have field inversion currently? (or one which is perhaps swapped
> > somewhere along the pipeline in rcar-vin?)
> >
>
> I'm not sure which tree this patch is developed on but Steve Longerbeam
> posted a RFC which IMHO had the fields inverted, there was a discussion in
his
> thread [1] where I tried to get to the bottom of the issue. My conclusions
> there might be wrong due to the issues addressed in this patch.
>
> Michael: Did you have Steve's patch in your tree when testing this?
>
> 1. https://patchwork.kernel.org/patch/10904263/
>
> --
> Regards,
> Niklas Söderlund

I had another version of Steve's patch when testing, but the FLD_NUM
setting was still the opposite compared to 9f7983bdc4925ae2
("media: rcar-csi2: Propagate the FLD signal for NTSC and PAL").
I could send all patches from the private pull request which Steve Longerbeam
has created for ADIT if you want to better understand my test results,
but probably they can not be applied to the current mainline development tree.
The patch for adv748x I used for testing looks a bit different as well,
so it can be applied to the tree used by ADIT. But the functionality
is the same (I can provide the patch as well if it is required.).
There are also concerns regarding VnMC.FOC bit (I tested V4L2_FIELD_INTERLACED mode 
and in my tests I figured out, that this bit does not exactly do
what the Renesas Hardware Manual describes and should be always set to 0
regardless whether NTSC or PAL are used. But I had only Raspberry Pi as
NTSC test source and no additional NTSC camera for verification,
so the results may be not reliable.).


Niklas, in [1] you mentioned that you could read the WC counter (which is in
fact the frame number in case of frame start packets) in the interrupt
INT_FSFE and it would start at 0. Could you please share the patch you used
for reading? As Steve Longerbeam mentioned in [2], this would be a CSI spec 
violation, which he has cited in the commit message of his RFC patch[3]. It's important 
to mention that the violation would be on the side of the adv748x chip (adv7482 on my 
Salvator-X board), because it creates the Frame Start packets. And according to the description 
of the FRAMENUMBER_INTERLACED bit in [4] (page 193), adv7481 should always send the
"1,2,1,2..." CSI frame number sequence (I could  not find a generic document
valid for all adv748x but I doubt that it would be  different there.).
So starting with CSI frame number 0 would even violate specification in it's 
own data sheet. Another possibility could also be a silicon bug in rcar CSI interface,
which would decrement the WC value by one. 


[1] https://patchwork.kernel.org/patch/10904263/#22594157
[2] https://patchwork.kernel.org/patch/10904263/#22594563
[3] https://patchwork.kernel.org/patch/10904263
[4] https://www.analog.com/media/en/technical-documentation/user-guides/ADV7481_UG-747.pdf

Regards,
Michael

Re: [PATCH] media: i2c: adv748x: initialize bit 7 of csi_tx_top_reg_1f

[niklas.soderlund]

Hi Rodin,

On 2019-05-16 14:24:13 +0000, Rodin, Michael (Ferchau; ADITG/ESM1) wrote:
> Hi Kieran and Niklas,
> 
> thank you for your responses!
> 
> > Hi Kieran and Michael,
> >
> > On 2019-05-10 17:25:24 +0100, Kieran Bingham wrote:
> > <snip>
> >
> > >
> > > Niklas, how does RCar-CSI determine the top/bottom sequence?
> >
> > That patch just got merged in the media tree a few days ago,
> >
> >     9f7983bdc4925ae2 ("media: rcar-csi2: Propagate the FLD signal for NTSC
> > and PAL")
> >
> > >
> > > Do we have field inversion currently? (or one which is perhaps swapped
> > > somewhere along the pipeline in rcar-vin?)
> > >
> >
> > I'm not sure which tree this patch is developed on but Steve Longerbeam
> > posted a RFC which IMHO had the fields inverted, there was a discussion in
> his
> > thread [1] where I tried to get to the bottom of the issue. My conclusions
> > there might be wrong due to the issues addressed in this patch.
> >
> > Michael: Did you have Steve's patch in your tree when testing this?
> >
> > 1. https://patchwork.kernel.org/patch/10904263/
> >
> > --
> > Regards,
> > Niklas Söderlund
> 
> I had another version of Steve's patch when testing, but the FLD_NUM
> setting was still the opposite compared to 9f7983bdc4925ae2
> ("media: rcar-csi2: Propagate the FLD signal for NTSC and PAL").
> I could send all patches from the private pull request which Steve Longerbeam
> has created for ADIT if you want to better understand my test results,
> but probably they can not be applied to the current mainline development tree.
> The patch for adv748x I used for testing looks a bit different as well,
> so it can be applied to the tree used by ADIT. But the functionality
> is the same (I can provide the patch as well if it is required.).
> There are also concerns regarding VnMC.FOC bit (I tested V4L2_FIELD_INTERLACED mode 
> and in my tests I figured out, that this bit does not exactly do
> what the Renesas Hardware Manual describes and should be always set to 0
> regardless whether NTSC or PAL are used. But I had only Raspberry Pi as
> NTSC test source and no additional NTSC camera for verification,
> so the results may be not reliable.).

Interesting, I will add VnMC.FOC to my list of things to look into.

> 
> 
> Niklas, in [1] you mentioned that you could read the WC counter (which is in
> fact the frame number in case of frame start packets) in the interrupt
> INT_FSFE and it would start at 0. Could you please share the patch you used
> for reading?

I'm sorry I no longer have that code. It was a small hack thrown 
together quickly. Idea was to read the register from the interrupt 
handler and printk() it. Not the best of methods but at the time it 
confirmed my theory of how it should work. It might be that I have drawn 
some premature conclusions.

> As Steve Longerbeam mentioned in [2], this would be a CSI spec 
>violation, which he has cited in the commit message of his RFC 
>patch[3]. It's important to mention that the violation would be on the 
>side of the adv748x chip (adv7482 on my Salvator-X board), because it 
>creates the Frame Start packets. And according to the description of 
>the FRAMENUMBER_INTERLACED bit in [4] (page 193), adv7481 should always 
>send the
> "1,2,1,2..." CSI frame number sequence (I could  not find a generic document
> valid for all adv748x but I doubt that it would be  different there.).
> So starting with CSI frame number 0 would even violate specification in it's 
> own data sheet. Another possibility could also be a silicon bug in rcar CSI interface,
> which would decrement the WC value by one. 

A more likely cause is that my way of testing was flawed ;-)

I will add this to my growing pile of things to look into. I don't know 
when I will find the time. If you got time and figure out a method to 
verify the settings in rcar-csi2 together with a good video source 
please post patches ;-) It would be great if this could be figured out 
together with the VnMC.FOC issue mentioned above. If you should happen 
to work on it you might be interested in the patches recently merged in 
the media-tree which adds the interrupt handler to rcar-csi2 making 
inspection hacks easier to add on top.

One idea I have is to use a programmable video source and "display" 
frames for capture where one can distinguish top/bottom (different 
colors for each line for example) fields on the capture side and somehow 
automate regression testing. I have had this plan for some years now and 
I still use a old video game system as my test rig so don't hold your 
breath ;-)

> 
> 
> [1] https://patchwork.kernel.org/patch/10904263/#22594157
> [2] https://patchwork.kernel.org/patch/10904263/#22594563
> [3] https://patchwork.kernel.org/patch/10904263
> [4] https://www.analog.com/media/en/technical-documentation/user-guides/ADV7481_UG-747.pdf
> 
> Regards,
> Michael

-- 
Regards,
Niklas Söderlund