Re: [PATCH v12 11/33] rcar-vin: set a default field to fallback on
On 09/03/18 18:07, Niklas Söderlund wrote:
> 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
> Reviewed-by: Laurent Pinchart
> ---
> 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?
Yes, that would make a lot more sense.
After making that change you can add my:
Reviewed-by: Hans Verkuil
to this patch.
Regards,
Hans
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
Re: [PATCH v12 11/33] rcar-vin: set a default field to fallback on
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
> >>> Reviewed-by: Laurent Pinchart
> >>> ---
> >>> 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.
> >>
> >>
Re: [PATCH v12 11/33] rcar-vin: set a default field to fallback on
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
>>> Reviewed-by: Laurent Pinchart
>>> ---
>>> 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_FORMATV4L2_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.
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
>
Re: [PATCH v12 11/33] rcar-vin: set a default field to fallback on
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!
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
> > Reviewed-by: Laurent Pinchart
> > ---
> > 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_FORMATV4L2_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.
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
Re: [PATCH v12 11/33] rcar-vin: set a default field to fallback on
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
> Reviewed-by: Laurent Pinchart
> ---
> 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 might very well be missing something here but it looks suspicious.
Regards,
Hans
[PATCH v12 11/33] rcar-vin: set a default field to fallback on
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
Reviewed-by: Laurent Pinchart
---
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_FORMATV4L2_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;
}
--
2.16.2
