| Message ID | 1546105882-15693-8-git-send-email-brad@nextdimension.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | si2157: Analog tuning and optimizations | expand |
On Sat, Dec 29, 2018 at 11:51:16AM -0600, Brad Love wrote: > Some software reports no signal found on a frequency due to immediately > checking for lock as soon as set_params returns. This waits up 40ms for > tuning operation, then from 30-150ms (dig/analog) for signal lock before > returning from set_params and set_analog_params. > > Tuning typically completes in 20-30ms. Digital tuning will additionally > wait depending on signal characteristics. Analog tuning will wait the > full timeout in case of no signal. This looks like a workaround for broken userspace. Very possibly this is software was tested on a different frontend which does wait for tuning to complete. This is a change in uapi and will have to be done for all frontends, and documented. However I doubt such a change is acceptable. What software are you refering to? Sean > > Signed-off-by: Brad Love <brad@nextdimension.cc> > --- > drivers/media/tuners/si2157.c | 87 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 87 insertions(+) > > diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c > index ff462ba..1737007 100644 > --- a/drivers/media/tuners/si2157.c > +++ b/drivers/media/tuners/si2157.c > @@ -318,6 +318,89 @@ static int si2157_sleep(struct dvb_frontend *fe) > return ret; > } > > +static int si2157_tune_wait(struct i2c_client *client, u8 is_digital) > +{ > +#define TUN_TIMEOUT 40 > +#define DIG_TIMEOUT 30 > +#define ANALOG_TIMEOUT 150 > + struct si2157_dev *dev = i2c_get_clientdata(client); > + int ret; > + unsigned long timeout; > + unsigned long start_time; > + u8 wait_status; > + u8 tune_lock_mask; > + > + if (is_digital) > + tune_lock_mask = 0x04; > + else > + tune_lock_mask = 0x02; > + > + mutex_lock(&dev->i2c_mutex); > + > + /* wait tuner command complete */ > + start_time = jiffies; > + timeout = start_time + msecs_to_jiffies(TUN_TIMEOUT); > + while (!time_after(jiffies, timeout)) { > + ret = i2c_master_recv(client, &wait_status, > + sizeof(wait_status)); > + if (ret < 0) { > + goto err_mutex_unlock; > + } else if (ret != sizeof(wait_status)) { > + ret = -EREMOTEIO; > + goto err_mutex_unlock; > + } > + > + /* tuner done? */ > + if ((wait_status & 0x81) == 0x81) > + break; > + usleep_range(5000, 10000); > + } > + > + dev_dbg(&client->dev, "tuning took %d ms, status=0x%x\n", > + jiffies_to_msecs(jiffies) - jiffies_to_msecs(start_time), > + wait_status); > + > + /* if we tuned ok, wait a bit for tuner lock */ > + if ((wait_status & 0x81) == 0x81) { > + if (is_digital) > + timeout = jiffies + msecs_to_jiffies(DIG_TIMEOUT); > + else > + timeout = jiffies + msecs_to_jiffies(ANALOG_TIMEOUT); > + while (!time_after(jiffies, timeout)) { > + ret = i2c_master_recv(client, &wait_status, > + sizeof(wait_status)); > + if (ret < 0) { > + goto err_mutex_unlock; > + } else if (ret != sizeof(wait_status)) { > + ret = -EREMOTEIO; > + goto err_mutex_unlock; > + } > + > + /* tuner locked? */ > + if (wait_status & tune_lock_mask) > + break; > + usleep_range(5000, 10000); > + } > + } > + > + dev_dbg(&client->dev, "tuning+lock took %d ms, status=0x%x\n", > + jiffies_to_msecs(jiffies) - jiffies_to_msecs(start_time), > + wait_status); > + > + if ((wait_status & 0xc0) != 0x80) { > + ret = -ETIMEDOUT; > + goto err_mutex_unlock; > + } > + > + mutex_unlock(&dev->i2c_mutex); > + return 0; > + > +err_mutex_unlock: > + mutex_unlock(&dev->i2c_mutex); > + dev_dbg(&client->dev, "failed=%d\n", ret); > + return ret; > +} > + > static int si2157_set_params(struct dvb_frontend *fe) > { > struct i2c_client *client = fe->tuner_priv; > @@ -417,6 +500,8 @@ static int si2157_set_params(struct dvb_frontend *fe) > dev->bandwidth = bandwidth; > dev->frequency = c->frequency; > > + si2157_tune_wait(client, 1); /* wait to complete, ignore any errors */ > + > return 0; > err: > dev->bandwidth = 0; > @@ -626,6 +711,8 @@ static int si2157_set_analog_params(struct dvb_frontend *fe, > #endif > dev->bandwidth = bandwidth; > > + si2157_tune_wait(client, 0); /* wait to complete, ignore any errors */ > + > return 0; > err: > dev->bandwidth = 0; > -- > 2.7.4
On 05/04/2019 05.29, Sean Young wrote: > On Sat, Dec 29, 2018 at 11:51:16AM -0600, Brad Love wrote: >> Some software reports no signal found on a frequency due to immediately >> checking for lock as soon as set_params returns. This waits up 40ms for >> tuning operation, then from 30-150ms (dig/analog) for signal lock before >> returning from set_params and set_analog_params. >> >> Tuning typically completes in 20-30ms. Digital tuning will additionally >> wait depending on signal characteristics. Analog tuning will wait the >> full timeout in case of no signal. > This looks like a workaround for broken userspace. Very possibly this > is software was tested on a different frontend which does wait for tuning > to complete. > > This is a change in uapi and will have to be done for all frontends, and > documented. However I doubt such a change is acceptable. > > What software are you refering to? Hi Sean, I would have to check with support to find out the various software that expect when a tune command returns for that tuning operation to have actually completed. In the current state when you tune si2157 it immediately returns, no care of the tuning operation completion, no care of the pll lock success. This is correct? Not so according to Silicon Labs documentation, which suggests a brief wait. I just took a look at other drivers, sampling only those with set_analog_params, all but two have similar code in place to actually allow the tune operation time to complete (both digital and analog) before returning to userland. The other drivers just insert arbitrary time delays to accommodate the operations completion time. At least with my patch I am monitoring the hardware to know when the operation actually completes. I see in tuners (not frontends): mt2063 - waits up to 100ms for lock status and return r820t - sleep 100ms for pll lock status and return tda827x - 170-500ms wait after tune before checking status and return tda8290 - sleep 100ms up to 3x while checking tune status and return tuner-xc2028 - sleep for 110ms awaiting lock status and return xc4000 - 10ms sleep after tune, unless debug, then 100ms and return xc5000 - 20ms sleep after tune, if pll not locked, re-tune and sleep again, repeat until success and return There's also other arbitrary sleeps peppered throughout the operations. Then you have si2157 that fires off the tuning commands and goes right back to userland immediately, when with instrumented testing, the operation takes time to complete and lock. The operation does not happen instantaneously. Software that expects clocks to be locked when the function returns determine this is an error. They query the tuner immediately when tune returns and they output tuning failed. Please explain why awaiting the hardware to say the "tuning operation you requested is done and clocks are locked" is not ok. If it's not ok, fine, but then a lot of other drivers are currently "not ok" as well. Regards, Brad > > > Sean > >> Signed-off-by: Brad Love <brad@nextdimension.cc> >> --- >> drivers/media/tuners/si2157.c | 87 +++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 87 insertions(+) >> >> diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c >> index ff462ba..1737007 100644 >> --- a/drivers/media/tuners/si2157.c >> +++ b/drivers/media/tuners/si2157.c >> @@ -318,6 +318,89 @@ static int si2157_sleep(struct dvb_frontend *fe) >> return ret; >> } >> >> +static int si2157_tune_wait(struct i2c_client *client, u8 is_digital) >> +{ >> +#define TUN_TIMEOUT 40 >> +#define DIG_TIMEOUT 30 >> +#define ANALOG_TIMEOUT 150 >> + struct si2157_dev *dev = i2c_get_clientdata(client); >> + int ret; >> + unsigned long timeout; >> + unsigned long start_time; >> + u8 wait_status; >> + u8 tune_lock_mask; >> + >> + if (is_digital) >> + tune_lock_mask = 0x04; >> + else >> + tune_lock_mask = 0x02; >> + >> + mutex_lock(&dev->i2c_mutex); >> + >> + /* wait tuner command complete */ >> + start_time = jiffies; >> + timeout = start_time + msecs_to_jiffies(TUN_TIMEOUT); >> + while (!time_after(jiffies, timeout)) { >> + ret = i2c_master_recv(client, &wait_status, >> + sizeof(wait_status)); >> + if (ret < 0) { >> + goto err_mutex_unlock; >> + } else if (ret != sizeof(wait_status)) { >> + ret = -EREMOTEIO; >> + goto err_mutex_unlock; >> + } >> + >> + /* tuner done? */ >> + if ((wait_status & 0x81) == 0x81) >> + break; >> + usleep_range(5000, 10000); >> + } >> + >> + dev_dbg(&client->dev, "tuning took %d ms, status=0x%x\n", >> + jiffies_to_msecs(jiffies) - jiffies_to_msecs(start_time), >> + wait_status); >> + >> + /* if we tuned ok, wait a bit for tuner lock */ >> + if ((wait_status & 0x81) == 0x81) { >> + if (is_digital) >> + timeout = jiffies + msecs_to_jiffies(DIG_TIMEOUT); >> + else >> + timeout = jiffies + msecs_to_jiffies(ANALOG_TIMEOUT); >> + while (!time_after(jiffies, timeout)) { >> + ret = i2c_master_recv(client, &wait_status, >> + sizeof(wait_status)); >> + if (ret < 0) { >> + goto err_mutex_unlock; >> + } else if (ret != sizeof(wait_status)) { >> + ret = -EREMOTEIO; >> + goto err_mutex_unlock; >> + } >> + >> + /* tuner locked? */ >> + if (wait_status & tune_lock_mask) >> + break; >> + usleep_range(5000, 10000); >> + } >> + } >> + >> + dev_dbg(&client->dev, "tuning+lock took %d ms, status=0x%x\n", >> + jiffies_to_msecs(jiffies) - jiffies_to_msecs(start_time), >> + wait_status); >> + >> + if ((wait_status & 0xc0) != 0x80) { >> + ret = -ETIMEDOUT; >> + goto err_mutex_unlock; >> + } >> + >> + mutex_unlock(&dev->i2c_mutex); >> + return 0; >> + >> +err_mutex_unlock: >> + mutex_unlock(&dev->i2c_mutex); >> + dev_dbg(&client->dev, "failed=%d\n", ret); >> + return ret; >> +} >> + >> static int si2157_set_params(struct dvb_frontend *fe) >> { >> struct i2c_client *client = fe->tuner_priv; >> @@ -417,6 +500,8 @@ static int si2157_set_params(struct dvb_frontend *fe) >> dev->bandwidth = bandwidth; >> dev->frequency = c->frequency; >> >> + si2157_tune_wait(client, 1); /* wait to complete, ignore any errors */ >> + >> return 0; >> err: >> dev->bandwidth = 0; >> @@ -626,6 +711,8 @@ static int si2157_set_analog_params(struct dvb_frontend *fe, >> #endif >> dev->bandwidth = bandwidth; >> >> + si2157_tune_wait(client, 0); /* wait to complete, ignore any errors */ >> + >> return 0; >> err: >> dev->bandwidth = 0; >> -- >> 2.7.4
On Mon, Apr 08, 2019 at 01:14:08PM -0500, Brad Love wrote: > > On 05/04/2019 05.29, Sean Young wrote: > > On Sat, Dec 29, 2018 at 11:51:16AM -0600, Brad Love wrote: > >> Some software reports no signal found on a frequency due to immediately > >> checking for lock as soon as set_params returns. This waits up 40ms for > >> tuning operation, then from 30-150ms (dig/analog) for signal lock before > >> returning from set_params and set_analog_params. > >> > >> Tuning typically completes in 20-30ms. Digital tuning will additionally > >> wait depending on signal characteristics. Analog tuning will wait the > >> full timeout in case of no signal. > > This looks like a workaround for broken userspace. Very possibly this > > is software was tested on a different frontend which does wait for tuning > > to complete. > > > > This is a change in uapi and will have to be done for all frontends, and > > documented. However I doubt such a change is acceptable. > > > > What software are you refering to? > > > Hi Sean, > > I would have to check with support to find out the various software that > expect when a tune command returns for that tuning operation to have > actually completed. In the current state when you tune si2157 it > immediately returns, no care of the tuning operation completion, no care > of the pll lock success. This is correct? Not so according to Silicon > Labs documentation, which suggests a brief wait. I just took a look at > other drivers, sampling only those with set_analog_params, all but two > have similar code in place to actually allow the tune operation time to > complete (both digital and analog) before returning to userland. The > other drivers just insert arbitrary time delays to accommodate the > operations completion time. At least with my patch I am monitoring the > hardware to know when the operation actually completes. > > I see in tuners (not frontends): > > mt2063 - waits up to 100ms for lock status and return > r820t - sleep 100ms for pll lock status and return > tda827x - 170-500ms wait after tune before checking status and return > tda8290 - sleep 100ms up to 3x while checking tune status and return > tuner-xc2028 - sleep for 110ms awaiting lock status and return > xc4000 - 10ms sleep after tune, unless debug, then 100ms and return > xc5000 - 20ms sleep after tune, if pll not locked, re-tune and sleep > again, repeat until success and return > > There's also other arbitrary sleeps peppered throughout the operations. > > Then you have si2157 that fires off the tuning commands and goes right > back to userland immediately, when with instrumented testing, the > operation takes time to complete and lock. The operation does not happen > instantaneously. Software that expects clocks to be locked when the > function returns determine this is an error. They query the tuner > immediately when tune returns and they output tuning failed. > > Please explain why awaiting the hardware to say the "tuning operation > you requested is done and clocks are locked" is not ok. If it's not ok, > fine, but then a lot of other drivers are currently "not ok" as well. If you read the dvb userspace api, there is nothing in the DTV_TUNE property that says it will block until a lock has been made. https://www.kernel.org/doc/html/latest/media/uapi/dvb/fe_property_parameters.html#dtv-tune The locking status can be queried using read status. https://www.kernel.org/doc/html/latest/media/uapi/dvb/fe-read-status.html#fe-read-status Using this mechanism, there is a minimum of sleeping in the kernel which helps interactivity. (uninterruptable) sleeping in drivers something that really should be avoided if at all possible. Userspace can't do anything during that time. So if you look at how dvbv5-zap finds a lock, you can see that it sets DTV_TUNE (amongst others) and then uses the frontend read_status() to query for locking until either timeout or a lock is found (not sure why it polls one per second though, seems a bit overly conservative). https://git.linuxtv.org/v4l-utils.git/tree/utils/dvb/dvbv5-zap.c#n494 Now, if other drivers have a sleep to wait for tuning lock in them and whether they should be removed, is another question. Looking at the tda8290 driver it needs to try various things in order to make a lock, so there is (currently) no way to avoid this. It would be nice if it could be changed to interruptable sleeps though, so that dvb userspace does not "hang" until a lock is made or failed. Thanks, Sean
Hi Sean, On 08/04/2019 15.55, Sean Young wrote: > On Mon, Apr 08, 2019 at 01:14:08PM -0500, Brad Love wrote: >> On 05/04/2019 05.29, Sean Young wrote: >>> On Sat, Dec 29, 2018 at 11:51:16AM -0600, Brad Love wrote: >>>> Some software reports no signal found on a frequency due to immediately >>>> checking for lock as soon as set_params returns. This waits up 40ms for >>>> tuning operation, then from 30-150ms (dig/analog) for signal lock before >>>> returning from set_params and set_analog_params. >>>> >>>> Tuning typically completes in 20-30ms. Digital tuning will additionally >>>> wait depending on signal characteristics. Analog tuning will wait the >>>> full timeout in case of no signal. >>> This looks like a workaround for broken userspace. Very possibly this >>> is software was tested on a different frontend which does wait for tuning >>> to complete. >>> >>> This is a change in uapi and will have to be done for all frontends, and >>> documented. However I doubt such a change is acceptable. >>> >>> What software are you refering to? >> >> Hi Sean, >> >> I would have to check with support to find out the various software that >> expect when a tune command returns for that tuning operation to have >> actually completed. In the current state when you tune si2157 it >> immediately returns, no care of the tuning operation completion, no care >> of the pll lock success. This is correct? Not so according to Silicon >> Labs documentation, which suggests a brief wait. I just took a look at >> other drivers, sampling only those with set_analog_params, all but two >> have similar code in place to actually allow the tune operation time to >> complete (both digital and analog) before returning to userland. The >> other drivers just insert arbitrary time delays to accommodate the >> operations completion time. At least with my patch I am monitoring the >> hardware to know when the operation actually completes. >> >> I see in tuners (not frontends): >> >> mt2063 - waits up to 100ms for lock status and return >> r820t - sleep 100ms for pll lock status and return >> tda827x - 170-500ms wait after tune before checking status and return >> tda8290 - sleep 100ms up to 3x while checking tune status and return >> tuner-xc2028 - sleep for 110ms awaiting lock status and return >> xc4000 - 10ms sleep after tune, unless debug, then 100ms and return >> xc5000 - 20ms sleep after tune, if pll not locked, re-tune and sleep >> again, repeat until success and return >> >> There's also other arbitrary sleeps peppered throughout the operations. >> >> Then you have si2157 that fires off the tuning commands and goes right >> back to userland immediately, when with instrumented testing, the >> operation takes time to complete and lock. The operation does not happen >> instantaneously. Software that expects clocks to be locked when the >> function returns determine this is an error. They query the tuner >> immediately when tune returns and they output tuning failed. >> >> Please explain why awaiting the hardware to say the "tuning operation >> you requested is done and clocks are locked" is not ok. If it's not ok, >> fine, but then a lot of other drivers are currently "not ok" as well. > If you read the dvb userspace api, there is nothing in the DTV_TUNE > property that says it will block until a lock has been made. > > https://www.kernel.org/doc/html/latest/media/uapi/dvb/fe_property_parameters.html#dtv-tune > > The locking status can be queried using read status. > > https://www.kernel.org/doc/html/latest/media/uapi/dvb/fe-read-status.html#fe-read-status > > Using this mechanism, there is a minimum of sleeping in the kernel which helps > interactivity. (uninterruptable) sleeping in drivers something that really > should be avoided if at all possible. Userspace can't do anything during that > time. This is not dealing with signal lock. As far as I understand the documentation I have, this is hardware tuning completion, aka the pll's and whatever else are properly configured and the tuning operation itself is finished successfully. The byte read is looking for TUNINT/ATVINT/DTVINT bits set, meaning the operation is complete. Right now I ignore errors, which probably should be returned to userland, as if a tune fails the userland has no way to determine this fact. This process comes directly from a flowchart in the SL guide. Signal lock is determined later, using the methods you describe. No property registers are being queried here, the success of the previous (tuning) operation is. > So if you look at how dvbv5-zap finds a lock, you can see that it sets > DTV_TUNE (amongst others) and then uses the frontend read_status() to query > for locking until either timeout or a lock is found (not sure why it polls > one per second though, seems a bit overly conservative). > > https://git.linuxtv.org/v4l-utils.git/tree/utils/dvb/dvbv5-zap.c#n494 > > Now, if other drivers have a sleep to wait for tuning lock in them and whether > they should be removed, is another question. Looking at the tda8290 driver > it needs to try various things in order to make a lock, so there is (currently) > no way to avoid this. It would be nice if it could be changed to interruptable > sleeps though, so that dvb userspace does not "hang" until a lock is made > or failed. I indeed noticed none of the other drivers use usleep_range, which is interruptible, correct? Regards, Brad > > > Thanks, > > Sean
Hi Sean, On 08/04/2019 16.37, Brad Love wrote: > Hi Sean, > > > On 08/04/2019 15.55, Sean Young wrote: >> On Mon, Apr 08, 2019 at 01:14:08PM -0500, Brad Love wrote: >>> On 05/04/2019 05.29, Sean Young wrote: >>>> On Sat, Dec 29, 2018 at 11:51:16AM -0600, Brad Love wrote: >>>>> Some software reports no signal found on a frequency due to immediately >>>>> checking for lock as soon as set_params returns. This waits up 40ms for >>>>> tuning operation, then from 30-150ms (dig/analog) for signal lock before >>>>> returning from set_params and set_analog_params. >>>>> >>>>> Tuning typically completes in 20-30ms. Digital tuning will additionally >>>>> wait depending on signal characteristics. Analog tuning will wait the >>>>> full timeout in case of no signal. >>>> This looks like a workaround for broken userspace. Very possibly this >>>> is software was tested on a different frontend which does wait for tuning >>>> to complete. >>>> >>>> This is a change in uapi and will have to be done for all frontends, and >>>> documented. However I doubt such a change is acceptable. >>>> >>>> What software are you refering to? >>> Hi Sean, >>> >>> I would have to check with support to find out the various software that >>> expect when a tune command returns for that tuning operation to have >>> actually completed. In the current state when you tune si2157 it >>> immediately returns, no care of the tuning operation completion, no care >>> of the pll lock success. This is correct? Not so according to Silicon >>> Labs documentation, which suggests a brief wait. I just took a look at >>> other drivers, sampling only those with set_analog_params, all but two >>> have similar code in place to actually allow the tune operation time to >>> complete (both digital and analog) before returning to userland. The >>> other drivers just insert arbitrary time delays to accommodate the >>> operations completion time. At least with my patch I am monitoring the >>> hardware to know when the operation actually completes. >>> >>> I see in tuners (not frontends): >>> >>> mt2063 - waits up to 100ms for lock status and return >>> r820t - sleep 100ms for pll lock status and return >>> tda827x - 170-500ms wait after tune before checking status and return >>> tda8290 - sleep 100ms up to 3x while checking tune status and return >>> tuner-xc2028 - sleep for 110ms awaiting lock status and return >>> xc4000 - 10ms sleep after tune, unless debug, then 100ms and return >>> xc5000 - 20ms sleep after tune, if pll not locked, re-tune and sleep >>> again, repeat until success and return >>> >>> There's also other arbitrary sleeps peppered throughout the operations. >>> >>> Then you have si2157 that fires off the tuning commands and goes right >>> back to userland immediately, when with instrumented testing, the >>> operation takes time to complete and lock. The operation does not happen >>> instantaneously. Software that expects clocks to be locked when the >>> function returns determine this is an error. They query the tuner >>> immediately when tune returns and they output tuning failed. >>> >>> Please explain why awaiting the hardware to say the "tuning operation >>> you requested is done and clocks are locked" is not ok. If it's not ok, >>> fine, but then a lot of other drivers are currently "not ok" as well. >> If you read the dvb userspace api, there is nothing in the DTV_TUNE >> property that says it will block until a lock has been made. >> >> https://www.kernel.org/doc/html/latest/media/uapi/dvb/fe_property_parameters.html#dtv-tune >> >> The locking status can be queried using read status. >> >> https://www.kernel.org/doc/html/latest/media/uapi/dvb/fe-read-status.html#fe-read-status >> >> Using this mechanism, there is a minimum of sleeping in the kernel which helps >> interactivity. (uninterruptable) sleeping in drivers something that really >> should be avoided if at all possible. Userspace can't do anything during that >> time. > > This is not dealing with signal lock. As far as I understand the > documentation I have, this is hardware tuning completion, aka the pll's > and whatever else are properly configured and the tuning operation > itself is finished successfully. The byte read is looking for > TUNINT/ATVINT/DTVINT bits set, meaning the operation is complete. Right > now I ignore errors, which probably should be returned to userland, as > if a tune fails the userland has no way to determine this fact. This > process comes directly from a flowchart in the SL guide. > > Signal lock is determined later, using the methods you describe. No > property registers are being queried here, the success of the previous > (tuning) operation is. Hi Sean, Just noticing now that my commit message does not agree with what I'm saying. I will need to do some testing to see which is correct. I've been basing my statements here off the SL reference I have, but I wrote the commit message over a year ago probably. I'll need to do some testing, I seem to recall the timing being consistent whether a signal is found or not. I might have mis-wrote the message long ago when I was splitting this up. Regards, Brad > > >> So if you look at how dvbv5-zap finds a lock, you can see that it sets >> DTV_TUNE (amongst others) and then uses the frontend read_status() to query >> for locking until either timeout or a lock is found (not sure why it polls >> one per second though, seems a bit overly conservative). >> >> https://git.linuxtv.org/v4l-utils.git/tree/utils/dvb/dvbv5-zap.c#n494 >> >> Now, if other drivers have a sleep to wait for tuning lock in them and whether >> they should be removed, is another question. Looking at the tda8290 driver >> it needs to try various things in order to make a lock, so there is (currently) >> no way to avoid this. It would be nice if it could be changed to interruptable >> sleeps though, so that dvb userspace does not "hang" until a lock is made >> or failed. > > I indeed noticed none of the other drivers use usleep_range, which is > interruptible, correct? > > Regards, > > Brad > > >> >> Thanks, >> >> Sean
diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c index ff462ba..1737007 100644 --- a/drivers/media/tuners/si2157.c +++ b/drivers/media/tuners/si2157.c @@ -318,6 +318,89 @@ static int si2157_sleep(struct dvb_frontend *fe) return ret; } +static int si2157_tune_wait(struct i2c_client *client, u8 is_digital) +{ +#define TUN_TIMEOUT 40 +#define DIG_TIMEOUT 30 +#define ANALOG_TIMEOUT 150 + struct si2157_dev *dev = i2c_get_clientdata(client); + int ret; + unsigned long timeout; + unsigned long start_time; + u8 wait_status; + u8 tune_lock_mask; + + if (is_digital) + tune_lock_mask = 0x04; + else + tune_lock_mask = 0x02; + + mutex_lock(&dev->i2c_mutex); + + /* wait tuner command complete */ + start_time = jiffies; + timeout = start_time + msecs_to_jiffies(TUN_TIMEOUT); + while (!time_after(jiffies, timeout)) { + ret = i2c_master_recv(client, &wait_status, + sizeof(wait_status)); + if (ret < 0) { + goto err_mutex_unlock; + } else if (ret != sizeof(wait_status)) { + ret = -EREMOTEIO; + goto err_mutex_unlock; + } + + /* tuner done? */ + if ((wait_status & 0x81) == 0x81) + break; + usleep_range(5000, 10000); + } + + dev_dbg(&client->dev, "tuning took %d ms, status=0x%x\n", + jiffies_to_msecs(jiffies) - jiffies_to_msecs(start_time), + wait_status); + + /* if we tuned ok, wait a bit for tuner lock */ + if ((wait_status & 0x81) == 0x81) { + if (is_digital) + timeout = jiffies + msecs_to_jiffies(DIG_TIMEOUT); + else + timeout = jiffies + msecs_to_jiffies(ANALOG_TIMEOUT); + while (!time_after(jiffies, timeout)) { + ret = i2c_master_recv(client, &wait_status, + sizeof(wait_status)); + if (ret < 0) { + goto err_mutex_unlock; + } else if (ret != sizeof(wait_status)) { + ret = -EREMOTEIO; + goto err_mutex_unlock; + } + + /* tuner locked? */ + if (wait_status & tune_lock_mask) + break; + usleep_range(5000, 10000); + } + } + + dev_dbg(&client->dev, "tuning+lock took %d ms, status=0x%x\n", + jiffies_to_msecs(jiffies) - jiffies_to_msecs(start_time), + wait_status); + + if ((wait_status & 0xc0) != 0x80) { + ret = -ETIMEDOUT; + goto err_mutex_unlock; + } + + mutex_unlock(&dev->i2c_mutex); + return 0; + +err_mutex_unlock: + mutex_unlock(&dev->i2c_mutex); + dev_dbg(&client->dev, "failed=%d\n", ret); + return ret; +} + static int si2157_set_params(struct dvb_frontend *fe) { struct i2c_client *client = fe->tuner_priv; @@ -417,6 +500,8 @@ static int si2157_set_params(struct dvb_frontend *fe) dev->bandwidth = bandwidth; dev->frequency = c->frequency; + si2157_tune_wait(client, 1); /* wait to complete, ignore any errors */ + return 0; err: dev->bandwidth = 0; @@ -626,6 +711,8 @@ static int si2157_set_analog_params(struct dvb_frontend *fe, #endif dev->bandwidth = bandwidth; + si2157_tune_wait(client, 0); /* wait to complete, ignore any errors */ + return 0; err: dev->bandwidth = 0;
Some software reports no signal found on a frequency due to immediately checking for lock as soon as set_params returns. This waits up 40ms for tuning operation, then from 30-150ms (dig/analog) for signal lock before returning from set_params and set_analog_params. Tuning typically completes in 20-30ms. Digital tuning will additionally wait depending on signal characteristics. Analog tuning will wait the full timeout in case of no signal. Signed-off-by: Brad Love <brad@nextdimension.cc> --- drivers/media/tuners/si2157.c | 87 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+)