| Message ID | 20160729174012.14331-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. :-( MBR, Sergei -- 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 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, 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 >
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 -- 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/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c index 10a5c10..346339f 100644 --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c @@ -102,6 +102,7 @@ static int __rvin_try_format_source(struct rvin_dev *vin, struct v4l2_subdev_format format = { .which = which, }; + enum v4l2_field field; int ret; sd = vin_to_source(vin); @@ -114,6 +115,8 @@ static int __rvin_try_format_source(struct rvin_dev *vin, format.pad = vin->src_pad_idx; + field = pix->field; + ret = v4l2_device_call_until_err(sd->v4l2_dev, 0, pad, set_fmt, pad_cfg, &format); if (ret < 0) @@ -121,6 +124,8 @@ static int __rvin_try_format_source(struct rvin_dev *vin, v4l2_fill_pix_format(pix, &format.format); + pix->field = field; + source->width = pix->width; source->height = pix->height;
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> --- drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 +++++ 1 file changed, 5 insertions(+)