Hi Hans,
Thanks for your feedback.
On 2018-03-09 17:28:39 +0100, Hans Verkuil wrote:
> On 09/03/18 17:17, Niklas Söderlund wrote:
> > Hi Hans,
> >
> > Thanks for your feedback, I don't think I can appreciate how happy I'm
> > that you reviewed this patch-set, Thank you!
>
> You're welcome!
>
> >
> > On 2018-03-09 16:25:23 +0100, Hans Verkuil wrote:
> >> On 07/03/18 23:04, Niklas Söderlund wrote:
> >>> If the field is not supported by the driver it should not try to keep
> >>> the current field. Instead it should set it to a default fallback. Since
> >>> trying a format should always result in the same state regardless of the
> >>> current state of the device.
> >>>
> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >>> ---
> >>> drivers/media/platform/rcar-vin/rcar-v4l2.c | 9 +++------
> >>> 1 file changed, 3 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>> index c2265324c7c96308..ebcd78b1bb6e8cb6 100644
> >>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>> @@ -23,6 +23,7 @@
> >>> #include "rcar-vin.h"
> >>>
> >>> #define RVIN_DEFAULT_FORMAT V4L2_PIX_FMT_YUYV
> >>> +#define RVIN_DEFAULT_FIELD V4L2_FIELD_NONE
> >>>
> >>> /* -----------------------------------------------------------------------------
> >>> * Format Conversions
> >>> @@ -143,7 +144,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >>> case V4L2_FIELD_INTERLACED:
> >>> break;
> >>> default:
> >>> - vin->format.field = V4L2_FIELD_NONE;
> >>> + vin->format.field = RVIN_DEFAULT_FIELD;
> >>> break;
> >>> }
> >>>
> >>> @@ -213,10 +214,6 @@ static int __rvin_try_format(struct rvin_dev *vin,
> >>> u32 walign;
> >>> int ret;
> >>>
> >>> - /* Keep current field if no specific one is asked for */
> >>> - if (pix->field == V4L2_FIELD_ANY)
> >>> - pix->field = vin->format.field;
> >>> -
> >>> /* If requested format is not supported fallback to the default */
> >>> if (!rvin_format_from_pixel(pix->pixelformat)) {
> >>> vin_dbg(vin, "Format 0x%x not found, using default 0x%x\n",
> >>> @@ -246,7 +243,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
> >>> case V4L2_FIELD_INTERLACED:
> >>> break;
> >>> default:
> >>> - pix->field = V4L2_FIELD_NONE;
> >>> + pix->field = RVIN_DEFAULT_FIELD;
> >>> break;
> >>> }
> >>>
> >>>
> >>
> >> I wonder if this code is correct. What if the adv7180 is the source? Does that even
> >> support FIELD_NONE? I suspect that the default field should actually depend on the
> >> source. FIELD_NONE for dv_timings based or sensor based subdevs and FIELD_INTERLACED
> >> for SDTV (g/s_std) subdevs.
> >
> > I see what you mean but I think this is correct. The field is only set
> > to V4L2_FIELD_NONE if the field returned from the source is not one of
> > TOP, BOTTOM, ALTERNATE, NONE, INERLACED, INTERLACED_TB, INTERLACED_BT.
> > So it works perfectly with the adv7180 as it will return
> > V4L2_FIELD_INTERLACED and then VIN will accept that and not change it.
> > So the field do depend on the source both before and after this change.
>
> Is it? If I pass FIELD_ANY to VIDIOC_TRY_FMT then that is passed to the
> adv7180 via __rvin_try_format and __rvin_try_format_source. But
> __rvin_try_format_source puts back the old field value after calling
> set_fmt for the adv7180 (pix->field = field).
>
> So pix->field is still FIELD_ANY when it enters the switch and so falls
> into the default case and it becomes FIELD_NONE.
>
> What's weird is the 'pix->field = field' in __rvin_try_format_source().
> Could that be a bug? Without that line what you say here would be correct.
Ahh yes, you are correct. I did not see this as I handle this in a later
patch in the series '[PATCH v12 16/33] rcar-vin: simplify how formats
are set and reset':
+ if (field != V4L2_FIELD_ANY)
+ pix->field = field;
- pix->field = field;
If I move this change to this patch do you think that would address your
concern? The intent is that if V4L2_FIELD_ANY is requested by the user
it should get what the source provides but I still like to allow for the
user to request a specific field format. For example in follow up
patches to this series I add SEQ_TB/BT and the user might want to
request to receive frames in that format.
>
> Regards,
>
> Hans
>
> >
> > This check is just to block the driver reporting SEQ_TB/BT if a source
> > where to report that (I known of no source who reports that) to
> > userspace as the driver do not yet support this. I have patches to add
> > support for this but I will keep them back until this series are picked
> > up :-)
> >
> >>
> >> I might very well be missing something here but it looks suspicious.
> >>
> >> Regards,
> >>
> >> Hans
> >
>
--
Regards,
Niklas Söderlund
