Hi Laurent,
On 2018-02-14 00:31:21 +0200, Laurent Pinchart wrote:
> 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+rene...@ragnatech.se>
> > >> Reviewed-by: Hans Verkuil <hans.verk...@cisco.com>
> > >> ---
> > >>
> > >> 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 ?
I now see that the function name is misleading and I will change it as
per above. I will also add a comment and swap to v4l2_pix_format (which
was used before v10 but was changed due to your review comments, I'm
happy you come around :-)
>
> > 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
>
--
Regards,
Niklas Söderlund
