Re: [PATCH 1/6] media: rcar-vin: allow field to be changed

[Hans Verkuil <hverkuil@xs4all.nl>]

On 08/01/2016 06:52 PM, Niklas Söderlund wrote:
> Hi Sergei,
> 
> Thanks for testing!
> 
> On 2016-07-30 00:04:33 +0300, Sergei Shtylyov wrote:
>> On 07/29/2016 08:40 PM, Niklas Söderlund wrote:
>>
>>> The driver forced whatever field was set by the source subdevice to be
>>> used. This patch allows the user to change from the default field.
>>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>
>>    I didn't apply this patch at first (thinking it was unnecessary), and the
>> capture worked fine. The field order appeared swapped again after I did
>> import this patch as well. :-(
> 
> I had a look at the test tool you told me you use 
> (https://linuxtv.org/downloads/v4l-dvb-apis/capture-example.html) and 
> the reason the field order is swapped is a combination of that tool and 
> how the rcar-vin driver interprets V4L2_FIELD_INTERLACED.
> 
> 1. The tool you use asks for V4L2_FIELD_INTERLACED if the -f switch is 
>    used. You told me #v4l that you do use that switch, but have modified 
>    the tool to use a different pixelformat than used in the link above, 
>    correct?
> 
> 2. The rcar-vin driver interprets V4L2_FIELD_INTERLACED as 
>    V4L2_FIELD_INTERLACED_TB. If this is correct or not I do not know, 

That's wrong. FIELD_INTERLACED is standard dependent: it is effectively
equal to INTERLACED_TB for 50 Hz formats and equal to INTERLACED_BT for
60 Hz formats. For non-SDTV timings (e.g. 720i) it is equal to INTERLACED_TB.

Stick to FIELD_INTERLACED, that's what you normally want to use.

>    the old soc-camera version of the driver do it this way so I have 
>    kept that logic.
> 
> This is the reason why the field order is wrong when you apply this 
> patch. Without it the field order would be locked to whatever the 
> subdevice reports, V4L2_FIELD_ALTERNATE in this case.
> 
> I don't know if it's correct to treat V4L2_FIELD_INTERLACED as 
> V4L2_FIELD_INTERLACED_TB or if I should try and use G_STD if 
> V4L2_FIELD_INTERLACED is requested and change the field to _TB or _BT 
> according to the result of that. I feel this will only push the problem 
> further down. What if G_STD is not implemented by the subdevice? Then a 
> default fallback interpretation to ether _TB or _BT would still be 
> needed. I'm open to suggestion on how to handle this case.
> 
> There is also a feature missing in this patch. The field order was set 
> to V4L2_FIELD_NONE if the requested format asks for V4L2_FIELD_ANY.  
> This have no effect for how you test but i did run into it while trying 
> to figure this out. I will send out a v2 which solves this by retaining 
> the current field mode if V4L2_FIELD_ANY is asked for.
> 
>>
>> MBR, Sergei
>>
> 

I plan on reviewing all these field-related patches tomorrow.

Regards,

	Hans