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@xxxxxx >-----Original Message----- >From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx] >Sent: Tuesday, November 10, 2009 2:36 AM >To: Hiremath, Vaibhav >Cc: linux-media@xxxxxxxxxxxxxxx; 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@xxxxxxxxx] >> > Sent: Monday, November 09, 2009 6:10 PM >> > To: Hiremath, Vaibhav >> > Cc: linux-media@xxxxxxxxxxxxxxx; 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@xxxxxx wrote: >> > > From: Vaibhav Hiremath <hvaibhav@xxxxxx> >> > > >> > > 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@xxxxxx> >> > > Signed-off-by: Brijesh Jadav <brijesh.j@xxxxxx> >> > > --- >> > > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
