[6/6] TVP514x:Switch to automode for s_input/querystd

  From: Vaibhav Hiremath <hvaibhav@ti.com>

Driver should switch to AutoSwitch mode on S_INPUT and QUERYSTD ioctls.
It has been observed that, if user configure the standard explicitely
then driver preserves the old settings.

Reviewed by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Brijesh Jadav <brijesh.j@ti.com>
---
 drivers/media/video/tvp514x.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

--
1.6.2.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Vaibhav,

See the review comments below.

On Tuesday 13 October 2009 17:12:59 hvaibhav@ti.com wrote:
> From: Vaibhav Hiremath <hvaibhav@ti.com>
> 
> Driver should switch to AutoSwitch mode on S_INPUT and QUERYSTD ioctls.
> It has been observed that, if user configure the standard explicitely
> then driver preserves the old settings.
> 
> Reviewed by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Brijesh Jadav <brijesh.j@ti.com>
> ---
>  drivers/media/video/tvp514x.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/tvp514x.c b/drivers/media/video/tvp514x.c
> index 2443726..0b0412d 100644
> --- a/drivers/media/video/tvp514x.c
> +++ b/drivers/media/video/tvp514x.c
> @@ -523,10 +523,18 @@ static int tvp514x_querystd(struct v4l2_subdev *sd, v4l2_std_id *std_id)
>  	enum tvp514x_std current_std;
>  	enum tvp514x_input input_sel;
>  	u8 sync_lock_status, lock_mask;
> +	int err;
> 
>  	if (std_id == NULL)
>  		return -EINVAL;
> 
> +	err = tvp514x_write_reg(sd, REG_VIDEO_STD,
> +			VIDEO_STD_AUTO_SWITCH_BIT);
> +	if (err < 0)
> +		return err;
> +
> +	msleep(LOCK_RETRY_DELAY);
> +

We have a problem here with the V4L2 spec.

The spec says that the standard should not change unless set explicitly by
the user. So switching to auto mode in querystd is not correct.

Is it possible to detect the standard without switching to automode? If it is,
then that's the preferred solution.

If it cannot be done, then we need to extend the API and add support for a
proper way of enabling automode.

This is actually a long standing issue that used to be pretty low prio since
it is very rare to see 'spontaneous' switches from e.g. PAL to NTSC.

But with the upcoming timings API for HDTV this will become much more common.
(e.g. switching from 1080p to 720p).

We need to define this quite carefully. In particular what will happen if the
standard switches while streaming. How does that relate to a scaler setup with
S_FMT? Do we know when this happens so that we can notify the application? Can
we lock the standard when starting capturing?

My gut feeling is that AUTO detect should only be allowed if the application
can be notified when the standard changes, or if the standard can be locked
when streaming starts.

The second part that is needed is some way to set the receiver into auto
switching mode. For SDTV that probably means adding a new AUTO standard bit.
Although to be honest I'm not keen on having to add something to v4l2_std_id.

For the HDTV timings API we probably need to add an AUTO preset.

Murali, can you think about this a bit and see how that will work out?

>  	/* get the current standard */
>  	current_std = tvp514x_get_current_std(sd);
>  	if (current_std == STD_INVALID)
> @@ -643,6 +651,15 @@ static int tvp514x_s_routing(struct v4l2_subdev *sd,
>  		/* Index out of bound */
>  		return -EINVAL;
> 
> +	/* Since this api is goint to detect the input, it is required
> +	   to set the standard in the auto switch mode */
> +	err = tvp514x_write_reg(sd, REG_VIDEO_STD,
> +			VIDEO_STD_AUTO_SWITCH_BIT);

Huh? I don't see what s_routing has to do with auto switch mode.

> +	if (err < 0)
> +		return err;
> +
> +	msleep(LOCK_RETRY_DELAY);
> +
>  	input_sel = input;
>  	output_sel = output;
> 
> --
> 1.6.2.4
> 

Regards,

	Hans
  

> -----Original Message-----
> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: Monday, November 09, 2009 6:10 PM
> To: Hiremath, Vaibhav
> Cc: linux-media@vger.kernel.org; Jadav, Brijesh R; Karicheri,
> Muralidharan
> Subject: Re: [PATCH 6/6] TVP514x:Switch to automode for
> s_input/querystd
> 
> Hi Vaibhav,
> 
> See the review comments below.
> 
> On Tuesday 13 October 2009 17:12:59 hvaibhav@ti.com wrote:
> > From: Vaibhav Hiremath <hvaibhav@ti.com>
> >
> > Driver should switch to AutoSwitch mode on S_INPUT and QUERYSTD
> ioctls.
> > It has been observed that, if user configure the standard
> explicitely
> > then driver preserves the old settings.
> >
> > Reviewed by: Vaibhav Hiremath <hvaibhav@ti.com>
> > Signed-off-by: Brijesh Jadav <brijesh.j@ti.com>
> > ---
> >  drivers/media/video/tvp514x.c |   17 +++++++++++++++++
> >  1 files changed, 17 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/media/video/tvp514x.c
> b/drivers/media/video/tvp514x.c
> > index 2443726..0b0412d 100644
> > --- a/drivers/media/video/tvp514x.c
> > +++ b/drivers/media/video/tvp514x.c
> > @@ -523,10 +523,18 @@ static int tvp514x_querystd(struct
> v4l2_subdev *sd, v4l2_std_id *std_id)
> >  	enum tvp514x_std current_std;
> >  	enum tvp514x_input input_sel;
> >  	u8 sync_lock_status, lock_mask;
> > +	int err;
> >
> >  	if (std_id == NULL)
> >  		return -EINVAL;
> >
> > +	err = tvp514x_write_reg(sd, REG_VIDEO_STD,
> > +			VIDEO_STD_AUTO_SWITCH_BIT);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	msleep(LOCK_RETRY_DELAY);
> > +
> 
> We have a problem here with the V4L2 spec.
> 
> The spec says that the standard should not change unless set
> explicitly by
> the user. So switching to auto mode in querystd is not correct.
> 
[Hiremath, Vaibhav] Hans, I have added this considering that; querystd means auto-detection of standard.

V4L2 spec says that, Sense the video standard received by the current input.

When user explicitly calls querystd, don't you think user should be aware of the fact that, driver is going to detect the standard automatically under this call.

> Is it possible to detect the standard without switching to automode?
> If it is,
> then that's the preferred solution.
> 
[Hiremath, Vaibhav] We can detect based on the TVP_VIDEO_STD_STATUS register, but will not be auto-detection. We will be reading the standard set by previous ioctls.

> If it cannot be done, then we need to extend the API and add support
> for a
> proper way of enabling automode.
> 
> This is actually a long standing issue that used to be pretty low
> prio since
> it is very rare to see 'spontaneous' switches from e.g. PAL to NTSC.
> 
> But with the upcoming timings API for HDTV this will become much
> more common.
> (e.g. switching from 1080p to 720p).
> 
> We need to define this quite carefully. In particular what will
> happen if the
> standard switches while streaming. How does that relate to a scaler
> setup with
> S_FMT? Do we know when this happens so that we can notify the
> application? 
[Hiremath, Vaibhav] At-least in TVP5146 we have interrupt which we can enable on standard change from user on current input, but we are not using it. This could be another interesting point, how to add such support.

> Can we lock the standard when starting capturing?
[Hiremath, Vaibhav] I think this makes sense to me, we should not allow changing the standard once streaming is going on. 

I think event manager may play a major role here, on standard change we should be able to send event back to application. Now the question is how application is going to handle this, since currently there is not way application can specify AUTOMODE, he has to query the standard and set it again. 

Thanks,
Vaibhav
> 
> My gut feeling is that AUTO detect should only be allowed if the
> application
> can be notified when the standard changes, or if the standard can be
> locked
> when streaming starts.
> 
> The second part that is needed is some way to set the receiver into
> auto
> switching mode. For SDTV that probably means adding a new AUTO
> standard bit.
> Although to be honest I'm not keen on having to add something to
> v4l2_std_id.
> 
> For the HDTV timings API we probably need to add an AUTO preset.
> 
> Murali, can you think about this a bit and see how that will work
> out?
> 
> >  	/* get the current standard */
> >  	current_std = tvp514x_get_current_std(sd);
> >  	if (current_std == STD_INVALID)
> > @@ -643,6 +651,15 @@ static int tvp514x_s_routing(struct
> v4l2_subdev *sd,
> >  		/* Index out of bound */
> >  		return -EINVAL;
> >
> > +	/* Since this api is goint to detect the input, it is required
> > +	   to set the standard in the auto switch mode */
> > +	err = tvp514x_write_reg(sd, REG_VIDEO_STD,
> > +			VIDEO_STD_AUTO_SWITCH_BIT);
> 
> Huh? I don't see what s_routing has to do with auto switch mode.
> 
> > +	if (err < 0)
> > +		return err;
> > +
> > +	msleep(LOCK_RETRY_DELAY);
> > +
> >  	input_sel = input;
> >  	output_sel = output;
> >
> > --
> > 1.6.2.4
> >
> 
> Regards,
> 
> 	Hans
> 
> 
> --
> Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Tuesday 10 November 2009 06:49:55 Hiremath, Vaibhav wrote:
> 
> > -----Original Message-----
> > From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> > Sent: Monday, November 09, 2009 6:10 PM
> > To: Hiremath, Vaibhav
> > Cc: linux-media@vger.kernel.org; Jadav, Brijesh R; Karicheri,
> > Muralidharan
> > Subject: Re: [PATCH 6/6] TVP514x:Switch to automode for
> > s_input/querystd
> > 
> > Hi Vaibhav,
> > 
> > See the review comments below.
> > 
> > On Tuesday 13 October 2009 17:12:59 hvaibhav@ti.com wrote:
> > > From: Vaibhav Hiremath <hvaibhav@ti.com>
> > >
> > > Driver should switch to AutoSwitch mode on S_INPUT and QUERYSTD
> > ioctls.
> > > It has been observed that, if user configure the standard
> > explicitely
> > > then driver preserves the old settings.
> > >
> > > Reviewed by: Vaibhav Hiremath <hvaibhav@ti.com>
> > > Signed-off-by: Brijesh Jadav <brijesh.j@ti.com>
> > > ---
> > >  drivers/media/video/tvp514x.c |   17 +++++++++++++++++
> > >  1 files changed, 17 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/media/video/tvp514x.c
> > b/drivers/media/video/tvp514x.c
> > > index 2443726..0b0412d 100644
> > > --- a/drivers/media/video/tvp514x.c
> > > +++ b/drivers/media/video/tvp514x.c
> > > @@ -523,10 +523,18 @@ static int tvp514x_querystd(struct
> > v4l2_subdev *sd, v4l2_std_id *std_id)
> > >  	enum tvp514x_std current_std;
> > >  	enum tvp514x_input input_sel;
> > >  	u8 sync_lock_status, lock_mask;
> > > +	int err;
> > >
> > >  	if (std_id == NULL)
> > >  		return -EINVAL;
> > >
> > > +	err = tvp514x_write_reg(sd, REG_VIDEO_STD,
> > > +			VIDEO_STD_AUTO_SWITCH_BIT);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	msleep(LOCK_RETRY_DELAY);
> > > +
> > 
> > We have a problem here with the V4L2 spec.
> > 
> > The spec says that the standard should not change unless set
> > explicitly by
> > the user. So switching to auto mode in querystd is not correct.
> > 
> [Hiremath, Vaibhav] Hans, I have added this considering that; querystd means auto-detection of standard.
> 
> V4L2 spec says that, Sense the video standard received by the current input.
> 
> When user explicitly calls querystd, don't you think user should be aware of the fact that, driver is going to detect the standard automatically under this call.

The spec is very silent about that :-)

Perhaps we should modify the documentation and add one additional clause:

QUERYSTD might return -EBUSY when capture is in progress depending on the driver.

With this in place we can put the tvp514x in autodetect mode as long as there
is no streaming going on. When streaming is started the tvp514x is set to the
last selected S_STD standard and kept there until streaming stops, at which
time it goes back to autodetect.

Calling QUERYSTD while streaming will return -EBUSY.

This makes sense to me.

> > Is it possible to detect the standard without switching to automode?
> > If it is,
> > then that's the preferred solution.
> > 
> [Hiremath, Vaibhav] We can detect based on the TVP_VIDEO_STD_STATUS register, but will not be auto-detection. We will be reading the standard set by previous ioctls.
> 
> > If it cannot be done, then we need to extend the API and add support
> > for a
> > proper way of enabling automode.
> > 
> > This is actually a long standing issue that used to be pretty low
> > prio since
> > it is very rare to see 'spontaneous' switches from e.g. PAL to NTSC.
> > 
> > But with the upcoming timings API for HDTV this will become much
> > more common.
> > (e.g. switching from 1080p to 720p).
> > 
> > We need to define this quite carefully. In particular what will
> > happen if the
> > standard switches while streaming. How does that relate to a scaler
> > setup with
> > S_FMT? Do we know when this happens so that we can notify the
> > application? 
> [Hiremath, Vaibhav] At-least in TVP5146 we have interrupt which we can enable on standard change from user on current input, but we are not using it. This could be another interesting point, how to add such support.
> 
> > Can we lock the standard when starting capturing?
> [Hiremath, Vaibhav] I think this makes sense to me, we should not allow changing the standard once streaming is going on. 
> 
> I think event manager may play a major role here, on standard change we should be able to send event back to application. Now the question is how application is going to handle this, since currently there is not way application can specify AUTOMODE, he has to query the standard and set it again. 

I don't think this is an issue for this device. And for HDTV devices we have
to wait and see how that will work out.

Regards,

	Hans

> 
> Thanks,
> Vaibhav
> > 
> > My gut feeling is that AUTO detect should only be allowed if the
> > application
> > can be notified when the standard changes, or if the standard can be
> > locked
> > when streaming starts.
> > 
> > The second part that is needed is some way to set the receiver into
> > auto
> > switching mode. For SDTV that probably means adding a new AUTO
> > standard bit.
> > Although to be honest I'm not keen on having to add something to
> > v4l2_std_id.
> > 
> > For the HDTV timings API we probably need to add an AUTO preset.
> > 
> > Murali, can you think about this a bit and see how that will work
> > out?
> > 
> > >  	/* get the current standard */
> > >  	current_std = tvp514x_get_current_std(sd);
> > >  	if (current_std == STD_INVALID)
> > > @@ -643,6 +651,15 @@ static int tvp514x_s_routing(struct
> > v4l2_subdev *sd,
> > >  		/* Index out of bound */
> > >  		return -EINVAL;
> > >
> > > +	/* Since this api is goint to detect the input, it is required
> > > +	   to set the standard in the auto switch mode */
> > > +	err = tvp514x_write_reg(sd, REG_VIDEO_STD,
> > > +			VIDEO_STD_AUTO_SWITCH_BIT);
> > 
> > Huh? I don't see what s_routing has to do with auto switch mode.
> > 
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	msleep(LOCK_RETRY_DELAY);
> > > +
> > >  	input_sel = input;
> > >  	output_sel = output;
> > >
> > > --
> > > 1.6.2.4
> > >
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > 
> > --
> > Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
> 
> 
> 
>
  

Hi,

IMO, querystd() should make use of the auto mode to detect the current
standard and is internal to the driver how it achieves that. Also I agree
that locking the standard while streaming is ongoing and reverting back
to auto mode when it stops is okay based on the use cases that I have
come across. Same is true for TVP7002 which does query preset. 

Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
email: m-karicheri2@ti.com

>-----Original Message-----
>From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
>Sent: Tuesday, November 10, 2009 2:36 AM
>To: Hiremath, Vaibhav
>Cc: linux-media@vger.kernel.org; Jadav, Brijesh R; Karicheri, Muralidharan
>Subject: Re: [PATCH 6/6] TVP514x:Switch to automode for s_input/querystd
>
>On Tuesday 10 November 2009 06:49:55 Hiremath, Vaibhav wrote:
>>
>> > -----Original Message-----
>> > From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
>> > Sent: Monday, November 09, 2009 6:10 PM
>> > To: Hiremath, Vaibhav
>> > Cc: linux-media@vger.kernel.org; Jadav, Brijesh R; Karicheri,
>> > Muralidharan
>> > Subject: Re: [PATCH 6/6] TVP514x:Switch to automode for
>> > s_input/querystd
>> >
>> > Hi Vaibhav,
>> >
>> > See the review comments below.
>> >
>> > On Tuesday 13 October 2009 17:12:59 hvaibhav@ti.com wrote:
>> > > From: Vaibhav Hiremath <hvaibhav@ti.com>
>> > >
>> > > Driver should switch to AutoSwitch mode on S_INPUT and QUERYSTD
>> > ioctls.
>> > > It has been observed that, if user configure the standard
>> > explicitely
>> > > then driver preserves the old settings.
>> > >
>> > > Reviewed by: Vaibhav Hiremath <hvaibhav@ti.com>
>> > > Signed-off-by: Brijesh Jadav <brijesh.j@ti.com>
>> > > ---
>> > >  drivers/media/video/tvp514x.c |   17 +++++++++++++++++
>> > >  1 files changed, 17 insertions(+), 0 deletions(-)
>> > >
>> > > diff --git a/drivers/media/video/tvp514x.c
>> > b/drivers/media/video/tvp514x.c
>> > > index 2443726..0b0412d 100644
>> > > --- a/drivers/media/video/tvp514x.c
>> > > +++ b/drivers/media/video/tvp514x.c
>> > > @@ -523,10 +523,18 @@ static int tvp514x_querystd(struct
>> > v4l2_subdev *sd, v4l2_std_id *std_id)
>> > >  	enum tvp514x_std current_std;
>> > >  	enum tvp514x_input input_sel;
>> > >  	u8 sync_lock_status, lock_mask;
>> > > +	int err;
>> > >
>> > >  	if (std_id == NULL)
>> > >  		return -EINVAL;
>> > >
>> > > +	err = tvp514x_write_reg(sd, REG_VIDEO_STD,
>> > > +			VIDEO_STD_AUTO_SWITCH_BIT);
>> > > +	if (err < 0)
>> > > +		return err;
>> > > +
>> > > +	msleep(LOCK_RETRY_DELAY);
>> > > +
>> >
>> > We have a problem here with the V4L2 spec.
>> >
>> > The spec says that the standard should not change unless set
>> > explicitly by
>> > the user. So switching to auto mode in querystd is not correct.
>> >
>> [Hiremath, Vaibhav] Hans, I have added this considering that; querystd
>means auto-detection of standard.
>>
>> V4L2 spec says that, Sense the video standard received by the current
>input.
>>
>> When user explicitly calls querystd, don't you think user should be aware
>of the fact that, driver is going to detect the standard automatically
>under this call.
>
>The spec is very silent about that :-)
>
>Perhaps we should modify the documentation and add one additional clause:
>
>QUERYSTD might return -EBUSY when capture is in progress depending on the
>driver.
>
>With this in place we can put the tvp514x in autodetect mode as long as
>there
>is no streaming going on. When streaming is started the tvp514x is set to
>the
>last selected S_STD standard and kept there until streaming stops, at which
>time it goes back to autodetect.
>
>Calling QUERYSTD while streaming will return -EBUSY.
>
>This makes sense to me.
>
>> > Is it possible to detect the standard without switching to automode?
>> > If it is,
>> > then that's the preferred solution.
>> >
>> [Hiremath, Vaibhav] We can detect based on the TVP_VIDEO_STD_STATUS
>register, but will not be auto-detection. We will be reading the standard
>set by previous ioctls.
>>
>> > If it cannot be done, then we need to extend the API and add support
>> > for a
>> > proper way of enabling automode.
>> >
>> > This is actually a long standing issue that used to be pretty low
>> > prio since
>> > it is very rare to see 'spontaneous' switches from e.g. PAL to NTSC.
>> >
>> > But with the upcoming timings API for HDTV this will become much
>> > more common.
>> > (e.g. switching from 1080p to 720p).
>> >
>> > We need to define this quite carefully. In particular what will
>> > happen if the
>> > standard switches while streaming. How does that relate to a scaler
>> > setup with
>> > S_FMT? Do we know when this happens so that we can notify the
>> > application?
>> [Hiremath, Vaibhav] At-least in TVP5146 we have interrupt which we can
>enable on standard change from user on current input, but we are not using
>it. This could be another interesting point, how to add such support.
>>
>> > Can we lock the standard when starting capturing?
>> [Hiremath, Vaibhav] I think this makes sense to me, we should not allow
>changing the standard once streaming is going on.
>>
>> I think event manager may play a major role here, on standard change we
>should be able to send event back to application. Now the question is how
>application is going to handle this, since currently there is not way
>application can specify AUTOMODE, he has to query the standard and set it
>again.
>
>I don't think this is an issue for this device. And for HDTV devices we
>have
>to wait and see how that will work out.
>
>Regards,
>
>	Hans
>
>>
>> Thanks,
>> Vaibhav
>> >
>> > My gut feeling is that AUTO detect should only be allowed if the
>> > application
>> > can be notified when the standard changes, or if the standard can be
>> > locked
>> > when streaming starts.
>> >
>> > The second part that is needed is some way to set the receiver into
>> > auto
>> > switching mode. For SDTV that probably means adding a new AUTO
>> > standard bit.
>> > Although to be honest I'm not keen on having to add something to
>> > v4l2_std_id.
>> >
>> > For the HDTV timings API we probably need to add an AUTO preset.
>> >
>> > Murali, can you think about this a bit and see how that will work
>> > out?
>> >
>> > >  	/* get the current standard */
>> > >  	current_std = tvp514x_get_current_std(sd);
>> > >  	if (current_std == STD_INVALID)
>> > > @@ -643,6 +651,15 @@ static int tvp514x_s_routing(struct
>> > v4l2_subdev *sd,
>> > >  		/* Index out of bound */
>> > >  		return -EINVAL;
>> > >
>> > > +	/* Since this api is goint to detect the input, it is required
>> > > +	   to set the standard in the auto switch mode */
>> > > +	err = tvp514x_write_reg(sd, REG_VIDEO_STD,
>> > > +			VIDEO_STD_AUTO_SWITCH_BIT);
>> >
>> > Huh? I don't see what s_routing has to do with auto switch mode.
>> >
>> > > +	if (err < 0)
>> > > +		return err;
>> > > +
>> > > +	msleep(LOCK_RETRY_DELAY);
>> > > +
>> > >  	input_sel = input;
>> > >  	output_sel = output;
>> > >
>> > > --
>> > > 1.6.2.4
>> > >
>> >
>> > Regards,
>> >
>> > 	Hans
>> >
>> >
>> > --
>> > Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
>>
>>
>>
>>
>
>
>
>--
>Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html