| Message ID | 1467846004-12731-7-git-send-email-steve_longerbeam@mentor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/07/2016 12:59 AM, Steve Longerbeam wrote: > Add a device tree boolean property "bt656-4" to allow setting > the ITU-R BT.656-4 compatible bit. > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> Hi, Thanks for the patch. > --- > drivers/media/i2c/adv7180.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c > index 92e2f37..fff887c 100644 > --- a/drivers/media/i2c/adv7180.c > +++ b/drivers/media/i2c/adv7180.c > @@ -58,7 +58,7 @@ > > #define ADV7180_REG_OUTPUT_CONTROL 0x0003 > #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL 0x0004 > -#define ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS 0xC5 > +#define ADV7180_EXTENDED_OUTPUT_CONTROL_BT656_4 0x80 > > #define ADV7180_REG_AUTODETECT_ENABLE 0x0007 > #define ADV7180_AUTODETECT_DEFAULT 0x7f > @@ -216,6 +216,7 @@ struct adv7180_state { > struct gpio_desc *pwdn_gpio; > v4l2_std_id curr_norm; > bool autodetect; > + bool bt656_4; /* use bt.656-4 standard for NTSC */ > bool powered; > u8 input; > > @@ -1281,6 +1282,17 @@ static int init_device(struct adv7180_state *state) > if (ret) > goto out_unlock; > > + if (state->bt656_4) { > + ret = adv7180_read(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL); > + if (ret < 0) > + goto out_unlock; > + ret |= ADV7180_EXTENDED_OUTPUT_CONTROL_BT656_4; > + ret = adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL, > + ret); > + if (ret < 0) > + goto out_unlock; > + } > + > ret = adv7180_program_std(state); > if (ret) > goto out_unlock; > @@ -1332,6 +1344,10 @@ static int adv7180_of_parse(struct adv7180_state *state) > return PTR_ERR(state->pwdn_gpio); > } > > + /* select ITU-R BT.656-4 compatible? */ > + if (of_property_read_bool(client->dev.of_node, "bt656-4")) > + state->bt656_4 = true; This property needs to be documented. In my opinion it should also be a property of the endpoint sub-node rather than the toplevel device node since this is a configuration of the endpoint format. > + > return 0; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote: > On 07/07/2016 12:59 AM, Steve Longerbeam wrote: >> Add a device tree boolean property "bt656-4" to allow setting >> the ITU-R BT.656-4 compatible bit. >> >> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> > > + /* select ITU-R BT.656-4 compatible? */ > + if (of_property_read_bool(client->dev.of_node, "bt656-4")) > + state->bt656_4 = true; > This property needs to be documented. In my opinion it should also be a > property of the endpoint sub-node rather than the toplevel device node since > this is a configuration of the endpoint format. Agreed, it's really a config of the backend capture endpoint. I'll move it there and also document it in Documentation/devicetree/bindings/media/i2c/adv7180.txt. Steve -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/2016 11:59 AM, Steve Longerbeam wrote: > > > On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote: >> On 07/07/2016 12:59 AM, Steve Longerbeam wrote: >>> Add a device tree boolean property "bt656-4" to allow setting >>> the ITU-R BT.656-4 compatible bit. >>> >>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >> >> + /* select ITU-R BT.656-4 compatible? */ >> + if (of_property_read_bool(client->dev.of_node, "bt656-4")) >> + state->bt656_4 = true; >> This property needs to be documented. In my opinion it should also be a >> property of the endpoint sub-node rather than the toplevel device >> node since >> this is a configuration of the endpoint format. > > Agreed, it's really a config of the backend capture endpoint. I'll > move it > there and also document it in > Documentation/devicetree/bindings/media/i2c/adv7180.txt. er, scratch that. ITU-R BT.656 compatibility is really a property of the bt.656 bus. It should be added to v4l2 endpoints and parsed by v4l2_of_parse_endpoint(). I've created a patch to add a "bt656-mode" property to v4l2 endpoints and will copy all the maintainers. Steve -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/2016 02:10 PM, Steve Longerbeam wrote: > > > On 07/09/2016 11:59 AM, Steve Longerbeam wrote: >> >> >> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote: >>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote: >>>> Add a device tree boolean property "bt656-4" to allow setting >>>> the ITU-R BT.656-4 compatible bit. >>>> >>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>> >>> + /* select ITU-R BT.656-4 compatible? */ >>> + if (of_property_read_bool(client->dev.of_node, "bt656-4")) >>> + state->bt656_4 = true; >>> This property needs to be documented. In my opinion it should also be a >>> property of the endpoint sub-node rather than the toplevel device >>> node since >>> this is a configuration of the endpoint format. >> >> Agreed, it's really a config of the backend capture endpoint. I'll >> move it >> there and also document it in >> Documentation/devicetree/bindings/media/i2c/adv7180.txt. > > er, scratch that. ITU-R BT.656 compatibility is really a property of > the bt.656 bus. It > should be added to v4l2 endpoints and parsed by > v4l2_of_parse_endpoint(). I've created > a patch to add a "bt656-mode" property to v4l2 endpoints and will copy > all the maintainers. But I'm going back and forth on whether this property should be added to the adv7180's local endpoint, or to the remote endpoint. I'm leaning towards the remote endpoint, since this is more about how the backend wants the bus configured. Steve -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/2016 11:36 PM, Steve Longerbeam wrote: > > > On 07/09/2016 02:10 PM, Steve Longerbeam wrote: >> >> >> On 07/09/2016 11:59 AM, Steve Longerbeam wrote: >>> >>> >>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote: >>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote: >>>>> Add a device tree boolean property "bt656-4" to allow setting >>>>> the ITU-R BT.656-4 compatible bit. >>>>> >>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>>> >>>> + /* select ITU-R BT.656-4 compatible? */ >>>> + if (of_property_read_bool(client->dev.of_node, "bt656-4")) >>>> + state->bt656_4 = true; >>>> This property needs to be documented. In my opinion it should also be a >>>> property of the endpoint sub-node rather than the toplevel device node >>>> since >>>> this is a configuration of the endpoint format. >>> >>> Agreed, it's really a config of the backend capture endpoint. I'll move it >>> there and also document it in >>> Documentation/devicetree/bindings/media/i2c/adv7180.txt. >> >> er, scratch that. ITU-R BT.656 compatibility is really a property of the >> bt.656 bus. It >> should be added to v4l2 endpoints and parsed by v4l2_of_parse_endpoint(). >> I've created >> a patch to add a "bt656-mode" property to v4l2 endpoints and will copy all >> the maintainers. > > But I'm going back and forth on whether this property should be added to the > adv7180's > local endpoint, or to the remote endpoint. I'm leaning towards the remote > endpoint, since > this is more about how the backend wants the bus configured. I think it should be set for both, as both endpoints need to agree on the format. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote: > On 07/09/2016 11:36 PM, Steve Longerbeam wrote: >> >> >> On 07/09/2016 02:10 PM, Steve Longerbeam wrote: >>> >>> >>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote: >>>> >>>> >>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote: >>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote: >>>>>> Add a device tree boolean property "bt656-4" to allow setting >>>>>> the ITU-R BT.656-4 compatible bit. >>>>>> >>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>>>> >>>>> + /* select ITU-R BT.656-4 compatible? */ Please don't use the '-4': BT.656 is sufficient. The -4 is just the version number of the standard (and 5 is the latest anyway). >>>>> + if (of_property_read_bool(client->dev.of_node, "bt656-4")) >>>>> + state->bt656_4 = true; >>>>> This property needs to be documented. In my opinion it should also be a >>>>> property of the endpoint sub-node rather than the toplevel device node >>>>> since >>>>> this is a configuration of the endpoint format. >>>> >>>> Agreed, it's really a config of the backend capture endpoint. I'll move it >>>> there and also document it in >>>> Documentation/devicetree/bindings/media/i2c/adv7180.txt. >>> >>> er, scratch that. ITU-R BT.656 compatibility is really a property of the >>> bt.656 bus. It >>> should be added to v4l2 endpoints and parsed by v4l2_of_parse_endpoint(). >>> I've created >>> a patch to add a "bt656-mode" property to v4l2 endpoints and will copy all >>> the maintainers. >> >> But I'm going back and forth on whether this property should be added to the >> adv7180's >> local endpoint, or to the remote endpoint. I'm leaning towards the remote >> endpoint, since >> this is more about how the backend wants the bus configured. > > I think it should be set for both, as both endpoints need to agree on the > format. Is it needed at all? Setting the polarity flags to H/VSYNC_ACTIVE_HIGH/LOW will already signal BT.656 mode. See include/media/v4l2-mediabus.h and v4l2-of.c. I haven't looked in detail at adv7180, so I may have missed something, but this looks strange. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/07/16 13:55, Hans Verkuil wrote: > On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote: >> On 07/09/2016 11:36 PM, Steve Longerbeam wrote: >>> >>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote: >>>> >>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote: >>>>> >>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote: >>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote: >>>>>>> Add a device tree boolean property "bt656-4" to allow setting >>>>>>> the ITU-R BT.656-4 compatible bit. >>>>>>> >>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>>>>> + /* select ITU-R BT.656-4 compatible? */ > Please don't use the '-4': BT.656 is sufficient. The -4 is just the version > number of the standard (and 5 is the latest anyway). From the ADV7180 DS [1]: BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7] Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU has changed the toggling position for the V bit within the SAV EAV codes for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an output mode that is compliant with either the previous or new standard. For further information, visit the International Telecommunication Union website. Note that the standard change only affects NTSC and has no bearing on PAL. When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is used. The V bit goes low at EAV of Line 10 and Line 273. When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The V bit goes low at EAV of Line 20 and Line 283. [1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf > >>>>>> + if (of_property_read_bool(client->dev.of_node, "bt656-4")) >>>>>> + state->bt656_4 = true; >>>>>> This property needs to be documented. In my opinion it should also be a >>>>>> property of the endpoint sub-node rather than the toplevel device node >>>>>> since >>>>>> this is a configuration of the endpoint format. >>>>> Agreed, it's really a config of the backend capture endpoint. I'll move it >>>>> there and also document it in >>>>> Documentation/devicetree/bindings/media/i2c/adv7180.txt. >>>> er, scratch that. ITU-R BT.656 compatibility is really a property of the >>>> bt.656 bus. It >>>> should be added to v4l2 endpoints and parsed by v4l2_of_parse_endpoint(). >>>> I've created >>>> a patch to add a "bt656-mode" property to v4l2 endpoints and will copy all >>>> the maintainers. >>> But I'm going back and forth on whether this property should be added to the >>> adv7180's >>> local endpoint, or to the remote endpoint. I'm leaning towards the remote >>> endpoint, since >>> this is more about how the backend wants the bus configured. >> I think it should be set for both, as both endpoints need to agree on the >> format. > Is it needed at all? Setting the polarity flags to H/VSYNC_ACTIVE_HIGH/LOW > will already signal BT.656 mode. See include/media/v4l2-mediabus.h and > v4l2-of.c. > > I haven't looked in detail at adv7180, so I may have missed something, but this > looks strange. > > Regards, > > Hans > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/10/2016 04:17 PM, Ian Arkver wrote: > On 10/07/16 13:55, Hans Verkuil wrote: >> On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote: >>> On 07/09/2016 11:36 PM, Steve Longerbeam wrote: >>>> >>>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote: >>>>> >>>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote: >>>>>> >>>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote: >>>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote: >>>>>>>> Add a device tree boolean property "bt656-4" to allow setting >>>>>>>> the ITU-R BT.656-4 compatible bit. >>>>>>>> >>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>>>>>> + /* select ITU-R BT.656-4 compatible? */ >> Please don't use the '-4': BT.656 is sufficient. The -4 is just the version >> number of the standard (and 5 is the latest anyway). > From the ADV7180 DS [1]: > > BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7] > > Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU > has changed the toggling position for the V bit within the SAV EAV codes > for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an > output mode that is compliant with either the previous or new standard. > For further information, visit the International Telecommunication Union > website. > > Note that the standard change only affects NTSC and has no bearing on PAL. > > When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is > used. The V bit goes low at EAV of Line 10 and Line 273. > > When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The > V bit goes low at EAV of Line 20 and Line 283. > > [1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf Rev 4 came in in 1998. I would say that that is the default. We have had any problems with this and I would need some proof that this is really needed. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/10/2016 07:30 AM, Hans Verkuil wrote: > On 07/10/2016 04:17 PM, Ian Arkver wrote: >> On 10/07/16 13:55, Hans Verkuil wrote: >>> On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote: >>>> On 07/09/2016 11:36 PM, Steve Longerbeam wrote: >>>>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote: >>>>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote: >>>>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote: >>>>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote: >>>>>>>>> Add a device tree boolean property "bt656-4" to allow setting >>>>>>>>> the ITU-R BT.656-4 compatible bit. >>>>>>>>> >>>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>>>>>>> + /* select ITU-R BT.656-4 compatible? */ >>> Please don't use the '-4': BT.656 is sufficient. The -4 is just the version >>> number of the standard (and 5 is the latest anyway). >> From the ADV7180 DS [1]: >> >> BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7] >> >> Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU >> has changed the toggling position for the V bit within the SAV EAV codes >> for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an >> output mode that is compliant with either the previous or new standard. >> For further information, visit the International Telecommunication Union >> website. >> >> Note that the standard change only affects NTSC and has no bearing on PAL. >> >> When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is >> used. The V bit goes low at EAV of Line 10 and Line 273. >> >> When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The >> V bit goes low at EAV of Line 20 and Line 283. >> >> [1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf > Rev 4 came in in 1998. I would say that that is the default. We have had any > problems with this and I would need some proof that this is really needed. Hi Hans, yeah I agree with you that new capture drivers should not expect BT.656-3 compatible buss' any longer. The problem with i.MX6 however, is that the IPU CSI CCIR_CODE registers, which inform the IPU about the AV code positions, are very poorly documented. In order for i.MX6 to support BT.656-4 it would require very time-consuming reverse engineering to find the right values to plug into those registers to conform to the BT.656-4 AV code positions. Steve -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/07/16 23:34, Steve Longerbeam wrote: > > > On 07/10/2016 07:30 AM, Hans Verkuil wrote: >> On 07/10/2016 04:17 PM, Ian Arkver wrote: >>> On 10/07/16 13:55, Hans Verkuil wrote: >>>> On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote: >>>>> On 07/09/2016 11:36 PM, Steve Longerbeam wrote: >>>>>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote: >>>>>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote: >>>>>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote: >>>>>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote: >>>>>>>>>> Add a device tree boolean property "bt656-4" to allow setting >>>>>>>>>> the ITU-R BT.656-4 compatible bit. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>>>>>>>> + /* select ITU-R BT.656-4 compatible? */ >>>> Please don't use the '-4': BT.656 is sufficient. The -4 is just the >>>> version >>>> number of the standard (and 5 is the latest anyway). >>> From the ADV7180 DS [1]: >>> >>> BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7] >>> >>> Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the >>> ITU >>> has changed the toggling position for the V bit within the SAV EAV >>> codes >>> for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an >>> output mode that is compliant with either the previous or new standard. >>> For further information, visit the International Telecommunication >>> Union >>> website. >>> >>> Note that the standard change only affects NTSC and has no bearing >>> on PAL. >>> >>> When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is >>> used. The V bit goes low at EAV of Line 10 and Line 273. >>> >>> When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The >>> V bit goes low at EAV of Line 20 and Line 283. >>> >>> [1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf >>> >> Rev 4 came in in 1998. I would say that that is the default. We have >> had any >> problems with this and I would need some proof that this is really >> needed. > > Hi Hans, yeah I agree with you that new capture drivers should not > expect BT.656-3 compatible buss' any longer. The problem with i.MX6 > however, is that the IPU CSI CCIR_CODE registers, which inform the IPU > about the AV code positions, are very poorly documented. In order for > i.MX6 to support BT.656-4 it would require very time-consuming reverse > engineering to find the right values to plug into those registers to > conform > to the BT.656-4 AV code positions. > > Steve > Hi Steve, Once you dsicover that the 3-bit fields in each CCIR_CODE register correspond to the H, V and F flags in the SAV/EAV code (in that order, MSbit->LSbit), those registers make more sense. Pity that's not mentioned in the Ref Manual. However, I don't think that's relevant here, since the spec revision didn't change the codes, but just moved the lines the V bit is sent on. I believe the spec change switched the NTSC timing from VSYNC to VBLANK, but the net effect was 10 fewer "active" video lines per field. Looking at a sample of one other video decoder (tvp5150), the default seems to be to change V on lines 20 and 283, as per the newer revision of the spec., though again bets may have been hedged with a programmable override. In any case, I'm wondering if your extra ten lines per field are more related to this snippet from calc_default_crop in imx-camif.c, which seems like it would break other decoder front ends and adv7180 in bt656-4 mode... /* * FIXME: For NTSC standards, top must be set to an * offset of 13 lines to match fixed CCIR programming * in the IPU. */ if (std != V4L2_STD_UNKNOWN && (std & V4L2_STD_525_60)) rect->top = 13; I believe tvp5150 at least sends 243 active lines per field for an NTSC source and the top 3 lines are generally dropped. Regards, Ian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/11/2016 12:06 AM, Ian Arkver wrote: > On 10/07/16 23:34, Steve Longerbeam wrote: >> >> >> On 07/10/2016 07:30 AM, Hans Verkuil wrote: >>> On 07/10/2016 04:17 PM, Ian Arkver wrote: >>>> On 10/07/16 13:55, Hans Verkuil wrote: >>>>> On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote: >>>>>> On 07/09/2016 11:36 PM, Steve Longerbeam wrote: >>>>>>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote: >>>>>>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote: >>>>>>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote: >>>>>>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote: >>>>>>>>>>> Add a device tree boolean property "bt656-4" to allow setting >>>>>>>>>>> the ITU-R BT.656-4 compatible bit. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>>>>>>>>> + /* select ITU-R BT.656-4 compatible? */ >>>>> Please don't use the '-4': BT.656 is sufficient. The -4 is just the version >>>>> number of the standard (and 5 is the latest anyway). >>>> From the ADV7180 DS [1]: >>>> >>>> BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7] >>>> >>>> Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU >>>> has changed the toggling position for the V bit within the SAV EAV codes >>>> for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an >>>> output mode that is compliant with either the previous or new standard. >>>> For further information, visit the International Telecommunication Union >>>> website. >>>> >>>> Note that the standard change only affects NTSC and has no bearing on PAL. >>>> >>>> When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is >>>> used. The V bit goes low at EAV of Line 10 and Line 273. >>>> >>>> When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The >>>> V bit goes low at EAV of Line 20 and Line 283. >>>> >>>> [1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf >>> Rev 4 came in in 1998. I would say that that is the default. We have had any >>> problems with this and I would need some proof that this is really needed. >> >> Hi Hans, yeah I agree with you that new capture drivers should not >> expect BT.656-3 compatible buss' any longer. The problem with i.MX6 >> however, is that the IPU CSI CCIR_CODE registers, which inform the IPU >> about the AV code positions, are very poorly documented. In order for >> i.MX6 to support BT.656-4 it would require very time-consuming reverse >> engineering to find the right values to plug into those registers to conform >> to the BT.656-4 AV code positions. >> >> Steve >> > Hi Steve, > > Once you dsicover that the 3-bit fields in each CCIR_CODE register correspond to the H, V and F flags in the SAV/EAV code (in that order, MSbit->LSbit), those registers make more sense. Pity that's > not mentioned in the Ref Manual. Hi Ian, that's a plausible theory, but it doesn't work. I looked at the value programmed to CCIR_CODE_1 register (according to imx6 ref manual is values for first field), for NTSC : 0xD07DF. Comparing with the definition of the H/V/F bits in the AV codes in the bt.656 spec: F = 1 during field 2, 0 during field 1 V = 1 during field blanking, 0 elsewhere H = 1 in EAV, 0 in SAV There's no correspondence, for example F bit should be zero everywhere in CCIR_CODE_1. And wutz this "first/second blanking line command" stuff about? None of it makes any sense to me. > However, I don't think that's relevant here, since the spec revision didn't change the codes, but just moved the lines the V bit is sent on. I believe the spec change switched the NTSC timing from > VSYNC to VBLANK, but the net effect was 10 fewer "active" video lines per field. Looking at a sample of one other video decoder (tvp5150), the default seems to be to change V on lines 20 and 283, as > per the newer revision of the spec., though again bets may have been hedged with a programmable override. > > In any case, I'm wondering if your extra ten lines per field are more related to this snippet from calc_default_crop in imx-camif.c, which seems like it would break other decoder front ends and > adv7180 in bt656-4 mode... > > /* > * FIXME: For NTSC standards, top must be set to an > * offset of 13 lines to match fixed CCIR programming > * in the IPU. > */ > if (std != V4L2_STD_UNKNOWN && (std & V4L2_STD_525_60)) > rect->top = 13; could be. I'll try removing that FIXME block and try with bt.656-4 mode. Steve > > I believe tvp5150 at least sends 243 active lines per field for an NTSC source and the top 3 lines are generally dropped. > > Regards, > Ian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This conversation has drifted off topic, sorry. It now relates to code in drivers/gpu/ipu-v3/ipu-csi.c, so adding Philipp Zabel. On 11/07/16 23:03, Steve Longerbeam wrote: > On 07/11/2016 12:06 AM, Ian Arkver wrote: >> On 10/07/16 23:34, Steve Longerbeam wrote: >>> >>> On 07/10/2016 07:30 AM, Hans Verkuil wrote: >>>> On 07/10/2016 04:17 PM, Ian Arkver wrote: >>>>> On 10/07/16 13:55, Hans Verkuil wrote: >>>>>> On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote: >>>>>>> On 07/09/2016 11:36 PM, Steve Longerbeam wrote: >>>>>>>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote: >>>>>>>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote: >>>>>>>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote: >>>>>>>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote: >>>>>>>>>>>> Add a device tree boolean property "bt656-4" to allow setting >>>>>>>>>>>> the ITU-R BT.656-4 compatible bit. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>>>>>>>>>> + /* select ITU-R BT.656-4 compatible? */ >>>>>> Please don't use the '-4': BT.656 is sufficient. The -4 is just the version >>>>>> number of the standard (and 5 is the latest anyway). >>>>> From the ADV7180 DS [1]: >>>>> >>>>> BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7] >>>>> >>>>> Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU >>>>> has changed the toggling position for the V bit within the SAV EAV codes >>>>> for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an >>>>> output mode that is compliant with either the previous or new standard. >>>>> For further information, visit the International Telecommunication Union >>>>> website. >>>>> >>>>> Note that the standard change only affects NTSC and has no bearing on PAL. >>>>> >>>>> When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is >>>>> used. The V bit goes low at EAV of Line 10 and Line 273. >>>>> >>>>> When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The >>>>> V bit goes low at EAV of Line 20 and Line 283. >>>>> >>>>> [1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf >>>> Rev 4 came in in 1998. I would say that that is the default. We have had any >>>> problems with this and I would need some proof that this is really needed. >>> Hi Hans, yeah I agree with you that new capture drivers should not >>> expect BT.656-3 compatible buss' any longer. The problem with i.MX6 >>> however, is that the IPU CSI CCIR_CODE registers, which inform the IPU >>> about the AV code positions, are very poorly documented. In order for >>> i.MX6 to support BT.656-4 it would require very time-consuming reverse >>> engineering to find the right values to plug into those registers to conform >>> to the BT.656-4 AV code positions. >>> >>> Steve >>> >> Hi Steve, >> >> Once you dsicover that the 3-bit fields in each CCIR_CODE register correspond to the H, V and F flags in the SAV/EAV code (in that order, MSbit->LSbit), those registers make more sense. Pity that's >> not mentioned in the Ref Manual. > Hi Ian, that's a plausible theory, but it doesn't work. I looked at the value > programmed to CCIR_CODE_1 register (according to imx6 ref manual is > values for first field), for NTSC : 0xD07DF. Comparing with the definition of > the H/V/F bits in the AV codes in the bt.656 spec: > > F = 1 during field 2, 0 during field 1 > V = 1 during field blanking, 0 elsewhere > H = 1 in EAV, 0 in SAV > > There's no correspondence, for example F bit should be zero everywhere in > CCIR_CODE_1. And wutz this "first/second blanking line command" stuff about? > None of it makes any sense to me. Hi, d07df = 001 101 xxxx 011 111 011 111 H V F CSI0_STRT_FLD0_ACTV 0 0 1 CSI0_END_FLD0_ACTV 1 0 1 CSI0_STRT_FLD0_BLNK_2ND 0 1 1 CSI0_END_FLD0_BLNK_2ND 1 1 1 CSI0_STRT_FLD0_BLNK_1ST 0 1 1 CSI0_END_FLD0_BLNK_1ST 1 1 1 40596 = 000 100 xxxx 010 110 010 110 405A6 = 000 100 xxxx 010 110 100 110 H V F CSI0_STRT_FLD1_ACTV 0 0 0 CSI0_END_FLD1_ACTV 1 0 0 CSI0_STRT_FLD1_BLNK_2ND 0 1 0 CSI0_END_FLD1_BLNK_2ND 1 1 0 CSI0_STRT_FLD1_BLNK_1ST 0 1 0 or 1 0 0 ? CSI0_END_FLD1_BLNK_1ST 1 1 0 For PAL: IPU_CSI0_CCIR_CODE_1 = 0x40596, IPU_CSI0_CCIR_CODE_1 = 0xd07df For NTSC: IPU_CSI0_CCIR_CODE_1 = 0xd07df, IPU_CSI0_CCIR_CODE_1 = 0x405A6 So, for the PAL case the field values make sense to me. The F bit is constant throughout each field. I'm not sure why the NTSC case has 405A6 instead of 40596 again. This looks like a bug to me. If you look back at the original Freescale capture driver[1], they use 40596 for both PAL and NTSC. The values are swapped between NTSC and PAL to account for the differing field order. I think using the capture width and height to do this is poor as any "bt.656-like" source with non standard dimensions will end up in the dev_err("Unsupported") case. Also, looking at BT.1120-8, for interlaced video the same SAV and EAV codes as PAL are used, so IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_SDR and IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_DDR modes are currently broken. I think the 1st and 2nd blanking values might be needed for some odd non 656 streams maybe? Also for progressive (originally) the value 40030 is used which is 40030 = 000 100 xxxx 000 000 110 000 This leaves the 2nd blank values empty. In the HW I'd guess both fire on SAV but there's a fixed event priority in the state machine, or it ignores events where both STRT and END fire on the same clock tick. btw: The patch I sent you a while back[2] dumps the CCIR values in octal which makes them a bit more readable. It also fixes up theCSI0_STRT_FLD0_BLNK_1ST value for progressive bt.656 to be {0 1 0}, which matches what's seen in the stream. In theory the blanking end value should be {1 1 0}, but it seems to work OK with the same code as unblanked SAV {0 0 0}. If it's useful I can rebase and post that patch to the ML. Maybe I should create an RFC patch for ipu-csi.c and stop hijacking this thread? 1: https://github.com/Freescale/linux-fslc/blob/3.14-1.1.x-imx/drivers/mxc/ipu3/ipu_capture.c#L168 2: 0003-ipu_csi-more-debug.-Fix-Blanking-SAV-for-progressive.patch > >> However, I don't think that's relevant here, since the spec revision didn't change the codes, but just moved the lines the V bit is sent on. I believe the spec change switched the NTSC timing from >> VSYNC to VBLANK, but the net effect was 10 fewer "active" video lines per field. Looking at a sample of one other video decoder (tvp5150), the default seems to be to change V on lines 20 and 283, as >> per the newer revision of the spec., though again bets may have been hedged with a programmable override. >> >> In any case, I'm wondering if your extra ten lines per field are more related to this snippet from calc_default_crop in imx-camif.c, which seems like it would break other decoder front ends and >> adv7180 in bt656-4 mode... >> >> /* >> * FIXME: For NTSC standards, top must be set to an >> * offset of 13 lines to match fixed CCIR programming >> * in the IPU. >> */ >> if (std != V4L2_STD_UNKNOWN && (std & V4L2_STD_525_60)) >> rect->top = 13; > could be. I'll try removing that FIXME block and try with bt.656-4 mode. > > Steve > >> I believe tvp5150 at least sends 243 active lines per field for an NTSC source and the top 3 lines are generally dropped. >> >> Regards, >> Ian All the best, Ian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ian, On 07/12/2016 03:25 AM, Ian Arkver wrote: > This conversation has drifted off topic, sorry. > It now relates to code in drivers/gpu/ipu-v3/ipu-csi.c, so adding Philipp Zabel. > <snip> > On 11/07/16 23:03, Steve Longerbeam wrote: >> On 07/11/2016 12:06 AM, Ian Arkver wrote: >>> >>> Hi Steve, >>> >>> Once you dsicover that the 3-bit fields in each CCIR_CODE register correspond to the H, V and F flags in the SAV/EAV code (in that order, MSbit->LSbit), those registers make more sense. Pity that's >>> not mentioned in the Ref Manual. >> Hi Ian, that's a plausible theory, but it doesn't work. I looked at the value >> programmed to CCIR_CODE_1 register (according to imx6 ref manual is >> values for first field), for NTSC : 0xD07DF. Comparing with the definition of >> the H/V/F bits in the AV codes in the bt.656 spec: >> >> F = 1 during field 2, 0 during field 1 >> V = 1 during field blanking, 0 elsewhere >> H = 1 in EAV, 0 in SAV >> >> There's no correspondence, for example F bit should be zero everywhere in >> CCIR_CODE_1. And wutz this "first/second blanking line command" stuff about? >> None of it makes any sense to me. > Hi, > > d07df = 001 101 xxxx 011 111 011 111 Oops, I was misreading the register fields in the ref manual. I misread the reserved field starting at bit 12 as 3 bits long, but it is 4 bits long. The H,V,F bits for those CCIR_CODE values make sense now. So this clears up a lot for me. Due to confusion surrounding these registers, one of my theories as to what these registers were doing was that they were telling the CSI actually where to look for the SAV/EAV codes in the stream, as a line number or something. But I see now that the registers are simply telling the IPU about the standard, so they are more like "set it and forget it" registers, and would only be set to something other than what the standard specifies in order to deal with non-standard streams (like adv718x "NEWAVMODE", see below). So the IPU must be detecting the AV codes using the AV code preamble, which is a good thing. Also, it clears up how to deal with bt.656-3 vs. bt.656-4 in the imx6 backend a bit more. The CCIR code registers do not need to be touched when switching between bt.656-3 and bt.656-4. It's more a matter of the number of active lines the decoder sends (bt.656-3 has 10 extra active lines). > > H V F > CSI0_STRT_FLD0_ACTV 0 0 1 > CSI0_END_FLD0_ACTV 1 0 1 > CSI0_STRT_FLD0_BLNK_2ND 0 1 1 > CSI0_END_FLD0_BLNK_2ND 1 1 1 > CSI0_STRT_FLD0_BLNK_1ST 0 1 1 > CSI0_END_FLD0_BLNK_1ST 1 1 1 > > 40596 = 000 100 xxxx 010 110 010 110 > 405A6 = 000 100 xxxx 010 110 100 110 > > H V F > CSI0_STRT_FLD1_ACTV 0 0 0 > CSI0_END_FLD1_ACTV 1 0 0 > CSI0_STRT_FLD1_BLNK_2ND 0 1 0 > CSI0_END_FLD1_BLNK_2ND 1 1 0 > CSI0_STRT_FLD1_BLNK_1ST 0 1 0 or 1 0 0 ? > CSI0_END_FLD1_BLNK_1ST 1 1 0 > > For PAL: IPU_CSI0_CCIR_CODE_1 = 0x40596, IPU_CSI0_CCIR_CODE_1 = 0xd07df > For NTSC: IPU_CSI0_CCIR_CODE_1 = 0xd07df, IPU_CSI0_CCIR_CODE_1 = 0x405A6 > > So, for the PAL case the field values make sense to me. The F bit is constant throughout each field. > I'm not sure why the NTSC case has 405A6 instead of 40596 again. This looks like a bug to me. Actually we made this change to deal with "NEWAVMODE" setting in the adv7180. But now I see that NEWAVMODE breaks the bt.656 standard, and will create problems for other backends. So I will remove the NEWAVMODE patch to adv7180, and revert the patch that sets the above 0x405A6 value in IPU_CSI0_CCIR_CODE_1. > If you look back at the original Freescale capture driver[1], they use 40596 for both PAL and NTSC. > > The values are swapped between NTSC and PAL to account for the differing field order. I think using > the capture width and height to do this is poor as any "bt.656-like" source with non standard > dimensions will end up in the dev_err("Unsupported") case. yes this code was inherited originally from Freescale. There's needs to be a better API for that. > Also, looking at BT.1120-8, for interlaced > video the same SAV and EAV codes as PAL are used, so IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_SDR and > IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_DDR modes are currently broken. right, I haven't even touched bt.1120 support. I don't have any h/w to test. > > I think the 1st and 2nd blanking values might be needed for some odd non 656 streams maybe? > Also for progressive (originally) the value 40030 is used which is > > 40030 = 000 100 xxxx 000 000 110 000 > > This leaves the 2nd blank values empty. In the HW I'd guess both fire on SAV but there's a fixed > event priority in the state machine, or it ignores events where both STRT and END fire on the same > clock tick. Maybe the "1st" and "2nd" blanking fields just provide alternative matches for the received AV codes. > > btw: The patch I sent you a while back[2] dumps the CCIR values in octal which makes them > a bit more readable. It also fixes up theCSI0_STRT_FLD0_BLNK_1ST value for progressive bt.656 to > be {0 1 0}, which matches what's seen in the stream. In theory the blanking end value should > be {1 1 0}, but it seems to work OK with the same code as unblanked SAV {0 0 0}. If it's useful > I can rebase and post that patch to the ML. > > Maybe I should create an RFC patch for ipu-csi.c and stop hijacking this thread? Sure please do. > > 1: https://github.com/Freescale/linux-fslc/blob/3.14-1.1.x-imx/drivers/mxc/ipu3/ipu_capture.c#L168 > 2: 0003-ipu_csi-more-debug.-Fix-Blanking-SAV-for-progressive.patch > > >> >>> However, I don't think that's relevant here, since the spec revision didn't change the codes, but just moved the lines the V bit is sent on. I believe the spec change switched the NTSC timing from >>> VSYNC to VBLANK, but the net effect was 10 fewer "active" video lines per field. Looking at a sample of one other video decoder (tvp5150), the default seems to be to change V on lines 20 and 283, as >>> per the newer revision of the spec., though again bets may have been hedged with a programmable override. >>> >>> In any case, I'm wondering if your extra ten lines per field are more related to this snippet from calc_default_crop in imx-camif.c, which seems like it would break other decoder front ends and >>> adv7180 in bt656-4 mode... >>> >>> /* >>> * FIXME: For NTSC standards, top must be set to an >>> * offset of 13 lines to match fixed CCIR programming >>> * in the IPU. >>> */ >>> if (std != V4L2_STD_UNKNOWN && (std & V4L2_STD_525_60)) >>> rect->top = 13; >> could be. I'll try removing that FIXME block and try with bt.656-4 mode. Setting rect->top = 3 with bt.656-4 produces stable video. And I will fix the FIXME comment, which reflects my confusion about the CCIR code registers. Steve -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c index 92e2f37..fff887c 100644 --- a/drivers/media/i2c/adv7180.c +++ b/drivers/media/i2c/adv7180.c @@ -58,7 +58,7 @@ #define ADV7180_REG_OUTPUT_CONTROL 0x0003 #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL 0x0004 -#define ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS 0xC5 +#define ADV7180_EXTENDED_OUTPUT_CONTROL_BT656_4 0x80 #define ADV7180_REG_AUTODETECT_ENABLE 0x0007 #define ADV7180_AUTODETECT_DEFAULT 0x7f @@ -216,6 +216,7 @@ struct adv7180_state { struct gpio_desc *pwdn_gpio; v4l2_std_id curr_norm; bool autodetect; + bool bt656_4; /* use bt.656-4 standard for NTSC */ bool powered; u8 input; @@ -1281,6 +1282,17 @@ static int init_device(struct adv7180_state *state) if (ret) goto out_unlock; + if (state->bt656_4) { + ret = adv7180_read(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL); + if (ret < 0) + goto out_unlock; + ret |= ADV7180_EXTENDED_OUTPUT_CONTROL_BT656_4; + ret = adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL, + ret); + if (ret < 0) + goto out_unlock; + } + ret = adv7180_program_std(state); if (ret) goto out_unlock; @@ -1332,6 +1344,10 @@ static int adv7180_of_parse(struct adv7180_state *state) return PTR_ERR(state->pwdn_gpio); } + /* select ITU-R BT.656-4 compatible? */ + if (of_property_read_bool(client->dev.of_node, "bt656-4")) + state->bt656_4 = true; + return 0; }
Add a device tree boolean property "bt656-4" to allow setting the ITU-R BT.656-4 compatible bit. Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> --- drivers/media/i2c/adv7180.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)