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 <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?
Yes, that would make a lot more sense.
After making that change you can add my:
Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
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 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
>>>
>>
>
