| Message ID | 20160802145107.24829-8-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[...] > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c > index a8b434b..c6fed71 100644 > --- a/drivers/media/i2c/adv7180.c > +++ b/drivers/media/i2c/adv7180.c > @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, > switch (format->format.field) { > case V4L2_FIELD_NONE: > if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) > - format->format.field = V4L2_FIELD_INTERLACED; > + format->format.field = V4L2_FIELD_ALTERNATE; > break; > default: > - format->format.field = V4L2_FIELD_INTERLACED; > + if (state->chip_info->flags & ADV7180_FLAG_I2P) > + format->format.field = V4L2_FIELD_INTERLACED; I'm not convinced this is correct. As far as I understand it when the I2P feature is enabled the core outputs full progressive frames at the full framerate. If it is bypassed it outputs half-frames. So we have the option of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I think this branch should setup the field format to be ALTERNATE regardless of whether the I2P feature is available. > + else > + format->format.field = V4L2_FIELD_ALTERNATE; > break; > } > -- 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 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: > [...] > > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c > > index a8b434b..c6fed71 100644 > > --- a/drivers/media/i2c/adv7180.c > > +++ b/drivers/media/i2c/adv7180.c > > @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, > > switch (format->format.field) { > > case V4L2_FIELD_NONE: > > if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) > > - format->format.field = V4L2_FIELD_INTERLACED; > > + format->format.field = V4L2_FIELD_ALTERNATE; > > break; > > default: > > - format->format.field = V4L2_FIELD_INTERLACED; > > + if (state->chip_info->flags & ADV7180_FLAG_I2P) > > + format->format.field = V4L2_FIELD_INTERLACED; > > I'm not convinced this is correct. As far as I understand it when the I2P > feature is enabled the core outputs full progressive frames at the full > framerate. If it is bypassed it outputs half-frames. So we have the option > of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I > think this branch should setup the field format to be ALTERNATE regardless > of whether the I2P feature is available. I be happy to update the patch in such manner, but I feel this is more for Steven to handle. I have no deep understanding of the adv7180 driver and the only HW I have is the adv7180 and not adv7280, adv7280_m, adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P flag. So I can't really test such a change. Steven do you want to update this patch and resend it? I can drop it from this series altogether since the rcar-vin changes do not depend on it. I only kept it since I want the rcar-vin changes to be picked up before the adv7180 changes, otherwise the rcar-vin driver will stop to function on Koelsch. I can also ofc make the change suggested by Lars if you prefer, but then I want your blessing to do so. I have already changed so much of your original patch :-) > > > + else > > + format->format.field = V4L2_FIELD_ALTERNATE; > > break; > > } > > >
On 08/03/2016 03:21 PM, Niklas Söderlund wrote: > On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: >> [...] >>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c >>> index a8b434b..c6fed71 100644 >>> --- a/drivers/media/i2c/adv7180.c >>> +++ b/drivers/media/i2c/adv7180.c >>> @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, >>> switch (format->format.field) { >>> case V4L2_FIELD_NONE: >>> if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) >>> - format->format.field = V4L2_FIELD_INTERLACED; >>> + format->format.field = V4L2_FIELD_ALTERNATE; >>> break; >>> default: >>> - format->format.field = V4L2_FIELD_INTERLACED; >>> + if (state->chip_info->flags & ADV7180_FLAG_I2P) >>> + format->format.field = V4L2_FIELD_INTERLACED; >> >> I'm not convinced this is correct. As far as I understand it when the I2P >> feature is enabled the core outputs full progressive frames at the full >> framerate. If it is bypassed it outputs half-frames. So we have the option >> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I >> think this branch should setup the field format to be ALTERNATE regardless >> of whether the I2P feature is available. Actually, that's not true. If the progressive frame is obtained by combining two fields, then it should return FIELD_INTERLACED. This is how most SDTV receivers operate. FIELD_NONE is reserved for pure progressive formats like 720p. Regards, Hans > > I be happy to update the patch in such manner, but I feel this is more > for Steven to handle. I have no deep understanding of the adv7180 driver > and the only HW I have is the adv7180 and not adv7280, adv7280_m, > adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P > flag. So I can't really test such a change. > > Steven do you want to update this patch and resend it? I can drop it > from this series altogether since the rcar-vin changes do not depend on > it. I only kept it since I want the rcar-vin changes to be picked up > before the adv7180 changes, otherwise the rcar-vin driver will stop to > function on Koelsch. I can also ofc make the change suggested by Lars if > you prefer, but then I want your blessing to do so. I have already > changed so much of your original patch :-) > >> >>> + else >>> + format->format.field = V4L2_FIELD_ALTERNATE; >>> break; >>> } >>> >> > -- 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 08/03/2016 04:11 PM, Hans Verkuil wrote: > > > On 08/03/2016 03:21 PM, Niklas Söderlund wrote: >> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: >>> [...] >>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c >>>> index a8b434b..c6fed71 100644 >>>> --- a/drivers/media/i2c/adv7180.c >>>> +++ b/drivers/media/i2c/adv7180.c >>>> @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, >>>> switch (format->format.field) { >>>> case V4L2_FIELD_NONE: >>>> if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) >>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>> + format->format.field = V4L2_FIELD_ALTERNATE; >>>> break; >>>> default: >>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>> + if (state->chip_info->flags & ADV7180_FLAG_I2P) >>>> + format->format.field = V4L2_FIELD_INTERLACED; >>> >>> I'm not convinced this is correct. As far as I understand it when the I2P >>> feature is enabled the core outputs full progressive frames at the full >>> framerate. If it is bypassed it outputs half-frames. So we have the option >>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I >>> think this branch should setup the field format to be ALTERNATE regardless >>> of whether the I2P feature is available. > > Actually, that's not true. If the progressive frame is obtained by combining > two fields, then it should return FIELD_INTERLACED. This is how most SDTV > receivers operate. This is definitely not covered by the current definition of INTERLACED. It says that the temporal order of the odd and even lines is the same for each frame. Whereas for a deinterlaced frame the temporal order changes from frame to frame. E.g. lets say you have half frames A, B, C, D, E, F ... The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ... The first frame is INTERLACED_TB, the second INTERLACED_BT, the third INTERLACED_TB again and so on. Also you get the same amount of pixels as for a progressive setup so the data-output-rate is higher. Maybe we need a FIELD_DEINTERLACED to denote such a setup? -- 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 08/03/2016 04:23 PM, Lars-Peter Clausen wrote: > On 08/03/2016 04:11 PM, Hans Verkuil wrote: >> >> >> On 08/03/2016 03:21 PM, Niklas Söderlund wrote: >>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: >>>> [...] >>>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c >>>>> index a8b434b..c6fed71 100644 >>>>> --- a/drivers/media/i2c/adv7180.c >>>>> +++ b/drivers/media/i2c/adv7180.c >>>>> @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, >>>>> switch (format->format.field) { >>>>> case V4L2_FIELD_NONE: >>>>> if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) >>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>> + format->format.field = V4L2_FIELD_ALTERNATE; >>>>> break; >>>>> default: >>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>> + if (state->chip_info->flags & ADV7180_FLAG_I2P) >>>>> + format->format.field = V4L2_FIELD_INTERLACED; >>>> >>>> I'm not convinced this is correct. As far as I understand it when the I2P >>>> feature is enabled the core outputs full progressive frames at the full >>>> framerate. If it is bypassed it outputs half-frames. So we have the option >>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I >>>> think this branch should setup the field format to be ALTERNATE regardless >>>> of whether the I2P feature is available. >> >> Actually, that's not true. If the progressive frame is obtained by combining >> two fields, then it should return FIELD_INTERLACED. This is how most SDTV >> receivers operate. > > This is definitely not covered by the current definition of INTERLACED. It > says that the temporal order of the odd and even lines is the same for each > frame. Whereas for a deinterlaced frame the temporal order changes from > frame to frame. > > E.g. lets say you have half frames A, B, C, D, E, F ... > > The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ... Just for completeness the output expected for INTERLACED would be (A, B), (C, D), (E, F), ... > > The first frame is INTERLACED_TB, the second INTERLACED_BT, the third > INTERLACED_TB again and so on. Also you get the same amount of pixels as for > a progressive setup so the data-output-rate is higher. Maybe we need a > FIELD_DEINTERLACED to denote such a setup? > > -- > 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 08/03/2016 04:23 PM, Lars-Peter Clausen wrote: > On 08/03/2016 04:11 PM, Hans Verkuil wrote: >> >> >> On 08/03/2016 03:21 PM, Niklas Söderlund wrote: >>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: >>>> [...] >>>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c >>>>> index a8b434b..c6fed71 100644 >>>>> --- a/drivers/media/i2c/adv7180.c >>>>> +++ b/drivers/media/i2c/adv7180.c >>>>> @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, >>>>> switch (format->format.field) { >>>>> case V4L2_FIELD_NONE: >>>>> if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) >>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>> + format->format.field = V4L2_FIELD_ALTERNATE; >>>>> break; >>>>> default: >>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>> + if (state->chip_info->flags & ADV7180_FLAG_I2P) >>>>> + format->format.field = V4L2_FIELD_INTERLACED; >>>> >>>> I'm not convinced this is correct. As far as I understand it when the I2P >>>> feature is enabled the core outputs full progressive frames at the full >>>> framerate. If it is bypassed it outputs half-frames. So we have the option >>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I >>>> think this branch should setup the field format to be ALTERNATE regardless >>>> of whether the I2P feature is available. >> >> Actually, that's not true. If the progressive frame is obtained by combining >> two fields, then it should return FIELD_INTERLACED. This is how most SDTV >> receivers operate. > > This is definitely not covered by the current definition of INTERLACED. It > says that the temporal order of the odd and even lines is the same for each > frame. Whereas for a deinterlaced frame the temporal order changes from > frame to frame. > > E.g. lets say you have half frames A, B, C, D, E, F ... > > The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ... Yuck. What most devices do is (A,B) (C,D) (E,F) ... That's FIELD_INTERLACED. > > The first frame is INTERLACED_TB, the second INTERLACED_BT, the third > INTERLACED_TB again and so on. Also you get the same amount of pixels as for > a progressive setup so the data-output-rate is higher. Maybe we need a > FIELD_DEINTERLACED to denote such a setup? > Yeah, this is a completely different mode. Do we even want to support this? Does anyone need this mode? I think we should leave it out until someone actually wants to use it. And then we need to come up with a new FIELD_ mode. 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 03/08/16 15:23, Lars-Peter Clausen wrote: > On 08/03/2016 04:11 PM, Hans Verkuil wrote: >> >> On 08/03/2016 03:21 PM, Niklas Söderlund wrote: >>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: >>>> [...] >>>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c >>>>> index a8b434b..c6fed71 100644 >>>>> --- a/drivers/media/i2c/adv7180.c >>>>> +++ b/drivers/media/i2c/adv7180.c >>>>> @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, >>>>> switch (format->format.field) { >>>>> case V4L2_FIELD_NONE: >>>>> if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) >>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>> + format->format.field = V4L2_FIELD_ALTERNATE; >>>>> break; >>>>> default: >>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>> + if (state->chip_info->flags & ADV7180_FLAG_I2P) >>>>> + format->format.field = V4L2_FIELD_INTERLACED; >>>> I'm not convinced this is correct. As far as I understand it when the I2P >>>> feature is enabled the core outputs full progressive frames at the full >>>> framerate. If it is bypassed it outputs half-frames. So we have the option >>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I >>>> think this branch should setup the field format to be ALTERNATE regardless >>>> of whether the I2P feature is available. >> Actually, that's not true. If the progressive frame is obtained by combining >> two fields, then it should return FIELD_INTERLACED. This is how most SDTV >> receivers operate. > This is definitely not covered by the current definition of INTERLACED. It > says that the temporal order of the odd and even lines is the same for each > frame. Whereas for a deinterlaced frame the temporal order changes from > frame to frame. > > E.g. lets say you have half frames A, B, C, D, E, F ... > > The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ... > > The first frame is INTERLACED_TB, the second INTERLACED_BT, the third > INTERLACED_TB again and so on. Also you get the same amount of pixels as for > a progressive setup so the data-output-rate is higher. Maybe we need a > FIELD_DEINTERLACED to denote such a setup? I don't think this is correct. The ADV7280 has no framestore, just a small linebuffer. It does I2P by line doubling plus some filtering and a little bit of proprietary magic, allegedly. I believe the output in I2P mode for your example would be (AA) (BB) (CC). The clock rate and pixel rate is doubled since it sends a full (faked up) frame per field time. I don't know what the FIELD_* mode is for line doubled pseudo-progressive. Also, I don't know why anyone would use this mode. I don't see a scenario where it would actually improve video quality over a more sophisticated motion adaptive deinterlace and to restore a 25/30fps feed you'd need to decimate and lose information. Quote from "Rob.Analog", who uses the word "frame" freely, here: https://ez.analog.com/thread/39382 "2) In I2P mode the number of lines per frame doubles. The ADV7280 still outputs 50 frames per second ( or 60 frames in NTSC mode) but each frame now consists of twice as many lines. e.g. if a frame consisted of 288 lines of active video in interlaced mode, this is doubled to 576 lines of active video in progressive mode. The line doubling is achieved by the ADV7280 interpolating between two lines of video (e.g. between lines 1 and 3 on an odd frame) and inserting an extra line (e.g. line 2). There are also some ADI propriety algorithms that prevent low angle noise artifacts. In order to achieve this line doubling, the LLC clock doubles from a nominal 27MHz to a nominal 54MHz" Regards, IanJ > > -- > 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 08/03/2016 04:42 PM, Ian Arkver wrote: > On 03/08/16 15:23, Lars-Peter Clausen wrote: >> On 08/03/2016 04:11 PM, Hans Verkuil wrote: >>> >>> On 08/03/2016 03:21 PM, Niklas Söderlund wrote: >>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: >>>>> [...] >>>>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c >>>>>> index a8b434b..c6fed71 100644 >>>>>> --- a/drivers/media/i2c/adv7180.c >>>>>> +++ b/drivers/media/i2c/adv7180.c >>>>>> @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, >>>>>> switch (format->format.field) { >>>>>> case V4L2_FIELD_NONE: >>>>>> if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) >>>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>>> + format->format.field = V4L2_FIELD_ALTERNATE; >>>>>> break; >>>>>> default: >>>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>>> + if (state->chip_info->flags & ADV7180_FLAG_I2P) >>>>>> + format->format.field = V4L2_FIELD_INTERLACED; >>>>> I'm not convinced this is correct. As far as I understand it when the I2P >>>>> feature is enabled the core outputs full progressive frames at the full >>>>> framerate. If it is bypassed it outputs half-frames. So we have the option >>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I >>>>> think this branch should setup the field format to be ALTERNATE regardless >>>>> of whether the I2P feature is available. >>> Actually, that's not true. If the progressive frame is obtained by combining >>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV >>> receivers operate. >> This is definitely not covered by the current definition of INTERLACED. It >> says that the temporal order of the odd and even lines is the same for each >> frame. Whereas for a deinterlaced frame the temporal order changes from >> frame to frame. >> >> E.g. lets say you have half frames A, B, C, D, E, F ... >> >> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ... >> >> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third >> INTERLACED_TB again and so on. Also you get the same amount of pixels as for >> a progressive setup so the data-output-rate is higher. Maybe we need a >> FIELD_DEINTERLACED to denote such a setup? > I don't think this is correct. The ADV7280 has no framestore, just a > small linebuffer. It does I2P by line doubling plus some filtering and a > little bit of proprietary magic, allegedly. > > I believe the output in I2P mode for your example would be (AA) (BB) > (CC). The clock rate and pixel rate is doubled since it sends a full > (faked up) frame per field time. > > I don't know what the FIELD_* mode is for line doubled pseudo-progressive. We don't have one for that either. You really shouldn't do tricks like that, it's rather pointless and gives horrible quality. > > Also, I don't know why anyone would use this mode. I don't see a > scenario where it would actually improve video quality over a more > sophisticated motion adaptive deinterlace and to restore a 25/30fps feed > you'd need to decimate and lose information. Indeed. > > Quote from "Rob.Analog", who uses the word "frame" freely, here: > https://ez.analog.com/thread/39382 > > "2) In I2P mode the number of lines per frame doubles. The ADV7280 still > outputs 50 frames per second ( or 60 frames in NTSC mode) but each frame > now consists of twice as many lines. e.g. if a frame consisted of 288 > lines of active video in interlaced mode, this is doubled to 576 lines > of active video in progressive mode. The line doubling is achieved by > the ADV7280 interpolating between two lines of video (e.g. between lines > 1 and 3 on an odd frame) and inserting an extra line (e.g. line 2). > There are also some ADI propriety algorithms that prevent low angle > noise artifacts. > > In order to achieve this line doubling, the LLC clock doubles from a > nominal 27MHz to a nominal 54MHz" It looks like you are correct. I wouldn't bother implementing this. 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 08/03/2016 04:42 PM, Ian Arkver wrote: > On 03/08/16 15:23, Lars-Peter Clausen wrote: >> On 08/03/2016 04:11 PM, Hans Verkuil wrote: >>> >>> On 08/03/2016 03:21 PM, Niklas Söderlund wrote: >>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: >>>>> [...] >>>>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c >>>>>> index a8b434b..c6fed71 100644 >>>>>> --- a/drivers/media/i2c/adv7180.c >>>>>> +++ b/drivers/media/i2c/adv7180.c >>>>>> @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct >>>>>> v4l2_subdev *sd, >>>>>> switch (format->format.field) { >>>>>> case V4L2_FIELD_NONE: >>>>>> if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) >>>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>>> + format->format.field = V4L2_FIELD_ALTERNATE; >>>>>> break; >>>>>> default: >>>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>>> + if (state->chip_info->flags & ADV7180_FLAG_I2P) >>>>>> + format->format.field = V4L2_FIELD_INTERLACED; >>>>> I'm not convinced this is correct. As far as I understand it when the I2P >>>>> feature is enabled the core outputs full progressive frames at the full >>>>> framerate. If it is bypassed it outputs half-frames. So we have the option >>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I >>>>> think this branch should setup the field format to be ALTERNATE regardless >>>>> of whether the I2P feature is available. >>> Actually, that's not true. If the progressive frame is obtained by combining >>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV >>> receivers operate. >> This is definitely not covered by the current definition of INTERLACED. It >> says that the temporal order of the odd and even lines is the same for each >> frame. Whereas for a deinterlaced frame the temporal order changes from >> frame to frame. >> >> E.g. lets say you have half frames A, B, C, D, E, F ... >> >> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ... >> >> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third >> INTERLACED_TB again and so on. Also you get the same amount of pixels as for >> a progressive setup so the data-output-rate is higher. Maybe we need a >> FIELD_DEINTERLACED to denote such a setup? > I don't think this is correct. The ADV7280 has no framestore, just a small > linebuffer. It does I2P by line doubling plus some filtering and a little > bit of proprietary magic, allegedly. > > I believe the output in I2P mode for your example would be (AA) (BB) (CC). > The clock rate and pixel rate is doubled since it sends a full (faked up) > frame per field time. > > I don't know what the FIELD_* mode is for line doubled pseudo-progressive. > > Also, I don't know why anyone would use this mode. I don't see a scenario > where it would actually improve video quality over a more sophisticated > motion adaptive deinterlace and to restore a 25/30fps feed you'd need to > decimate and lose information. > > Quote from "Rob.Analog", who uses the word "frame" freely, here: > https://ez.analog.com/thread/39382 > > "2) In I2P mode the number of lines per frame doubles. The ADV7280 still > outputs 50 frames per second ( or 60 frames in NTSC mode) but each frame now > consists of twice as many lines. e.g. if a frame consisted of 288 lines of > active video in interlaced mode, this is doubled to 576 lines of active > video in progressive mode. The line doubling is achieved by the ADV7280 > interpolating between two lines of video (e.g. between lines 1 and 3 on an > odd frame) and inserting an extra line (e.g. line 2). There are also some > ADI propriety algorithms that prevent low angle noise artifacts. > > In order to achieve this line doubling, the LLC clock doubles from a nominal > 27MHz to a nominal 54MHz" Hm, so it is basically just actually a scaling feature rather than a deinterlacer. I guess we should expose it as that then. -- 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 08/03/2016 06:21 AM, Niklas Söderlund wrote: > On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: >> [...] >>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c >>> index a8b434b..c6fed71 100644 >>> --- a/drivers/media/i2c/adv7180.c >>> +++ b/drivers/media/i2c/adv7180.c >>> @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, >>> switch (format->format.field) { >>> case V4L2_FIELD_NONE: >>> if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) >>> - format->format.field = V4L2_FIELD_INTERLACED; >>> + format->format.field = V4L2_FIELD_ALTERNATE; >>> break; >>> default: >>> - format->format.field = V4L2_FIELD_INTERLACED; >>> + if (state->chip_info->flags & ADV7180_FLAG_I2P) >>> + format->format.field = V4L2_FIELD_INTERLACED; >> I'm not convinced this is correct. As far as I understand it when the I2P >> feature is enabled the core outputs full progressive frames at the full >> framerate. If it is bypassed it outputs half-frames. So we have the option >> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I >> think this branch should setup the field format to be ALTERNATE regardless >> of whether the I2P feature is available. > I be happy to update the patch in such manner, but I feel this is more > for Steven to handle. I have no deep understanding of the adv7180 driver > and the only HW I have is the adv7180 and not adv7280, adv7280_m, > adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P > flag. So I can't really test such a change. > > Steven do you want to update this patch and resend it? Hi Niklas, I can update this patch, but it sounds like the whole thing is "up in the air" at this point, and we may want to yank out the I2P support altogether. I'll leave it up to Lars and others to work that out first. 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 08/03/2016 06:55 PM, Steve Longerbeam wrote: > On 08/03/2016 06:21 AM, Niklas Söderlund wrote: >> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: >>> [...] >>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c >>>> index a8b434b..c6fed71 100644 >>>> --- a/drivers/media/i2c/adv7180.c >>>> +++ b/drivers/media/i2c/adv7180.c >>>> @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, >>>> switch (format->format.field) { >>>> case V4L2_FIELD_NONE: >>>> if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) >>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>> + format->format.field = V4L2_FIELD_ALTERNATE; >>>> break; >>>> default: >>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>> + if (state->chip_info->flags & ADV7180_FLAG_I2P) >>>> + format->format.field = V4L2_FIELD_INTERLACED; >>> I'm not convinced this is correct. As far as I understand it when the I2P >>> feature is enabled the core outputs full progressive frames at the full >>> framerate. If it is bypassed it outputs half-frames. So we have the option >>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I >>> think this branch should setup the field format to be ALTERNATE regardless >>> of whether the I2P feature is available. >> I be happy to update the patch in such manner, but I feel this is more >> for Steven to handle. I have no deep understanding of the adv7180 driver >> and the only HW I have is the adv7180 and not adv7280, adv7280_m, >> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P >> flag. So I can't really test such a change. >> >> Steven do you want to update this patch and resend it? > > Hi Niklas, I can update this patch, but it sounds like the whole thing > is "up in the air" at this point, and we may want to yank out the I2P > support altogether. I'll leave it up to Lars and others to work that out > first. Yeah, we should remove the whole I2P stuff, I was misinformed about how it works. But either way I think this patch should simply not touch the current behavior, so don't add new if (FLAG_I2P) checks. -- 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 08/03/2016 09:58 AM, Lars-Peter Clausen wrote: > On 08/03/2016 06:55 PM, Steve Longerbeam wrote: >> On 08/03/2016 06:21 AM, Niklas Söderlund wrote: >>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: >>>> [...] >>>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c >>>>> index a8b434b..c6fed71 100644 >>>>> --- a/drivers/media/i2c/adv7180.c >>>>> +++ b/drivers/media/i2c/adv7180.c >>>>> @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, >>>>> switch (format->format.field) { >>>>> case V4L2_FIELD_NONE: >>>>> if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) >>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>> + format->format.field = V4L2_FIELD_ALTERNATE; >>>>> break; >>>>> default: >>>>> - format->format.field = V4L2_FIELD_INTERLACED; >>>>> + if (state->chip_info->flags & ADV7180_FLAG_I2P) >>>>> + format->format.field = V4L2_FIELD_INTERLACED; >>>> I'm not convinced this is correct. As far as I understand it when the I2P >>>> feature is enabled the core outputs full progressive frames at the full >>>> framerate. If it is bypassed it outputs half-frames. So we have the option >>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I >>>> think this branch should setup the field format to be ALTERNATE regardless >>>> of whether the I2P feature is available. >>> I be happy to update the patch in such manner, but I feel this is more >>> for Steven to handle. I have no deep understanding of the adv7180 driver >>> and the only HW I have is the adv7180 and not adv7280, adv7280_m, >>> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P >>> flag. So I can't really test such a change. >>> >>> Steven do you want to update this patch and resend it? >> Hi Niklas, I can update this patch, but it sounds like the whole thing >> is "up in the air" at this point, and we may want to yank out the I2P >> support altogether. I'll leave it up to Lars and others to work that out >> first. > Yeah, we should remove the whole I2P stuff, I was misinformed about how it > works. But either way I think this patch should simply not touch the current > behavior, so don't add new if (FLAG_I2P) checks. Hi Lars, Ok I can do that. I'll resubmit in next version of my patchset. 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 2016-08-03 10:14:45 -0700, Steve Longerbeam wrote: > On 08/03/2016 09:58 AM, Lars-Peter Clausen wrote: > > On 08/03/2016 06:55 PM, Steve Longerbeam wrote: > >> On 08/03/2016 06:21 AM, Niklas Söderlund wrote: > >>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote: > >>>> [...] > >>>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c > >>>>> index a8b434b..c6fed71 100644 > >>>>> --- a/drivers/media/i2c/adv7180.c > >>>>> +++ b/drivers/media/i2c/adv7180.c > >>>>> @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, > >>>>> switch (format->format.field) { > >>>>> case V4L2_FIELD_NONE: > >>>>> if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) > >>>>> - format->format.field = V4L2_FIELD_INTERLACED; > >>>>> + format->format.field = V4L2_FIELD_ALTERNATE; > >>>>> break; > >>>>> default: > >>>>> - format->format.field = V4L2_FIELD_INTERLACED; > >>>>> + if (state->chip_info->flags & ADV7180_FLAG_I2P) > >>>>> + format->format.field = V4L2_FIELD_INTERLACED; > >>>> I'm not convinced this is correct. As far as I understand it when the I2P > >>>> feature is enabled the core outputs full progressive frames at the full > >>>> framerate. If it is bypassed it outputs half-frames. So we have the option > >>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I > >>>> think this branch should setup the field format to be ALTERNATE regardless > >>>> of whether the I2P feature is available. > >>> I be happy to update the patch in such manner, but I feel this is more > >>> for Steven to handle. I have no deep understanding of the adv7180 driver > >>> and the only HW I have is the adv7180 and not adv7280, adv7280_m, > >>> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P > >>> flag. So I can't really test such a change. > >>> > >>> Steven do you want to update this patch and resend it? > >> Hi Niklas, I can update this patch, but it sounds like the whole thing > >> is "up in the air" at this point, and we may want to yank out the I2P > >> support altogether. I'll leave it up to Lars and others to work that out > >> first. > > Yeah, we should remove the whole I2P stuff, I was misinformed about how it > > works. But either way I think this patch should simply not touch the current > > behavior, so don't add new if (FLAG_I2P) checks. > > Hi Lars, Ok I can do that. I'll resubmit in next version of my patchset. Thanks Steven, then I will drop this patch in my v3. Can you pleas CC me when you send out your patch?
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c index a8b434b..c6fed71 100644 --- a/drivers/media/i2c/adv7180.c +++ b/drivers/media/i2c/adv7180.c @@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd, switch (format->format.field) { case V4L2_FIELD_NONE: if (!(state->chip_info->flags & ADV7180_FLAG_I2P)) - format->format.field = V4L2_FIELD_INTERLACED; + format->format.field = V4L2_FIELD_ALTERNATE; break; default: - format->format.field = V4L2_FIELD_INTERLACED; + if (state->chip_info->flags & ADV7180_FLAG_I2P) + format->format.field = V4L2_FIELD_INTERLACED; + else + format->format.field = V4L2_FIELD_ALTERNATE; break; } @@ -1253,8 +1256,13 @@ static int adv7180_probe(struct i2c_client *client, return -ENOMEM; state->client = client; - state->field = V4L2_FIELD_INTERLACED; state->chip_info = (struct adv7180_chip_info *)id->driver_data; + state->curr_norm = V4L2_STD_NTSC; + + if (state->chip_info->flags & ADV7180_FLAG_I2P) + state->field = V4L2_FIELD_INTERLACED; + else + state->field = V4L2_FIELD_ALTERNATE; if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) { state->csi_client = i2c_new_dummy(client->adapter, @@ -1274,7 +1282,6 @@ static int adv7180_probe(struct i2c_client *client, state->irq = client->irq; mutex_init(&state->mutex); - state->curr_norm = V4L2_STD_NTSC; if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED) state->powered = true; else