Hi Niklas, On Tuesday, 13 February 2018 18:47:04 EET Niklas Söderlund wrote: > On 2018-02-13 18:26:34 +0200, Laurent Pinchart wrote: > > On Monday, 29 January 2018 18:34:15 EET Niklas Söderlund wrote: > >> There was never proper support in the VIN driver to deliver ALTERNATING > >> field format to user-space, remove this field option. The problem is > >> that ALTERNATING filed order requires the sequence numbers of buffers > >> returned to userspace to reflect if fields where dropped or not, > >> something which is not possible with the VIN drivers capture logic. > >> > >> The VIN driver can still capture from a video source which delivers > >> frames in ALTERNATING field order, but needs to combine them using the > >> VIN hardware into INTERLACED field order. Before this change if a source > >> was delivering fields using ALTERNATE the driver would default to > >> combining them using this hardware feature. Only if the user explicitly > >> requested ALTERNATE filed order would incorrect frames be delivered. > >> > >> The height should not be cut in half for the format for TOP or BOTTOM > >> fields settings. This was a mistake and it was made visible by the > >> scaling refactoring. Correct behavior is that the user should request a > >> frame size that fits the half height frame reflected in the field > >> setting. If not the VIN will do its best to scale the top or bottom to > >> the requested format and cropping and scaling do not work as expected. > >> > >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx> > >> Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > >> --- > >> > >> drivers/media/platform/rcar-vin/rcar-dma.c | 15 +------- > >> drivers/media/platform/rcar-vin/rcar-v4l2.c | 53 +++++++++++------------ > >> 2 files changed, 24 insertions(+), 44 deletions(-) [snip] > >> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > >> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index > >> 4d5be2d0c79c9c9a..9f7902d29c62e205 100644 > >> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > >> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > >> @@ -103,6 +103,28 @@ static int rvin_get_source_format(struct rvin_dev > >> *vin, > >> if (ret) > >> return ret; > >> > >> + switch (fmt.format.field) { > >> + case V4L2_FIELD_TOP: > >> + case V4L2_FIELD_BOTTOM: > >> + case V4L2_FIELD_NONE: > >> + case V4L2_FIELD_INTERLACED_TB: > >> + case V4L2_FIELD_INTERLACED_BT: > >> + case V4L2_FIELD_INTERLACED: > >> + break; > >> + case V4L2_FIELD_ALTERNATE: > >> + /* > >> + * Driver do not (yet) support outputting ALTERNATE to a > >> + * userspace. It dose support outputting INTERLACED so use > > > > s/dose/does/ > > > >> + * the VIN hardware to combine the two fields. > >> + */ > >> + fmt.format.field = V4L2_FIELD_INTERLACED; > >> + fmt.format.height *= 2; > >> + break; > > > > I don't like this much. The rvin_get_source_format() function is supposed > > to return the media bus format for the bus between the source and the > > VIN. It's the caller that should take the field limitations into account, > > otherwise you end up with a mix of source and VIN data in the same > > structure. > > When I read your comments I understand your argument better. And I > understand this function is perhaps poorly named. Maybe it should be > renamed to rvin_get_vin_format_from_source(). If you add a comment above the function I could live with that. Would it make sense to pass a v4l2_pix_format structure instead of a v4l2_mbus_framefmt ? > The source format is fetched at s_stream() time in order to do format > validation. At this time the field is also taken into account once more > to validate that the VIN format (calculated here) still is valid. It > also handles the question you ask later at s_stream() time, see bellow. > > >> + default: > >> + vin->format.field = V4L2_FIELD_NONE; > >> + break; > >> + } > >> + > >> memcpy(mbus_fmt, &fmt.format, sizeof(*mbus_fmt)); > >> > >> return 0; > >> @@ -139,33 +161,6 @@ static int rvin_reset_format(struct rvin_dev *vin) > >> > >> v4l2_fill_pix_format(&vin->format, &source_fmt); > >> > >> - /* > >> - * If the subdevice uses ALTERNATE field mode and G_STD is > >> - * implemented use the VIN HW to combine the two fields to > >> - * one INTERLACED frame. The ALTERNATE field mode can still > >> - * be requested in S_FMT and be respected, this is just the > >> - * default which is applied at probing or when S_STD is called. > >> - */ > >> - if (vin->format.field == V4L2_FIELD_ALTERNATE && > >> - v4l2_subdev_has_op(vin_to_source(vin), video, g_std)) > >> - vin->format.field = V4L2_FIELD_INTERLACED; > >> - > >> - switch (vin->format.field) { > >> - case V4L2_FIELD_TOP: > >> - case V4L2_FIELD_BOTTOM: > >> - case V4L2_FIELD_ALTERNATE: > >> - vin->format.height /= 2; > >> - break; > >> - case V4L2_FIELD_NONE: > >> - case V4L2_FIELD_INTERLACED_TB: > >> - case V4L2_FIELD_INTERLACED_BT: > >> - case V4L2_FIELD_INTERLACED: > >> - break; > >> - default: > >> - vin->format.field = V4L2_FIELD_NONE; > >> - break; > >> - } > >> - > >> ret = rvin_reset_crop_compose(vin); > >> if (ret) > >> return ret; > >> @@ -243,12 +238,10 @@ static int __rvin_try_format(struct rvin_dev *vin, > >> if (ret) > >> return ret; > >> > >> + /* Reject ALTERNATE until support is added to the driver */ > >> switch (pix->field) { > >> case V4L2_FIELD_TOP: > >> case V4L2_FIELD_BOTTOM: > >> - case V4L2_FIELD_ALTERNATE: > >> - pix->height /= 2; > >> - break; > >> case V4L2_FIELD_NONE: > >> case V4L2_FIELD_INTERLACED_TB: > >> case V4L2_FIELD_INTERLACED_BT: > > > > You will then set the field to V4L2_FIELD_NONE, but the source will still > > provide V4L2_FIELD_ALTERNATE. What will happen in the VIN, what will it > > produce ? > > As stated above this is just the format produced from the VIN to > user-space. The source field is validated at s_stream() time, if it is > V4L2_FIELD_ALTERNATE the driver will handle it and possibly interlace it > depending on how the user wants to consume it, which is what is > specified here. That was clearer when I read the patch that implemented .start_streaming() support for the MC mode. Defaulting to V4L2_FIELD_NONE seems fine to me. -- Regards, Laurent Pinchart