Re: [PATCH 2/2] [media] dvb_frontend: Require FE_HAS_PARAMETERS for get_frontend()
Hi Mauro, On Wednesday 18 January 2012 18:51:25 Mauro Carvalho Chehab wrote: Calling get_frontend() before having either the frontend locked or the network signaling carriers locked won't work. So, block it at the DVB core. I like the idea and also the implementation. But before merging this needs more comments from other on the list. Even though it does not break anything for any current frontend-driver it is important to have a wider base agreeing on that. Especially from some other frontend-driver-writers. For example I could imagine that a frontend HAS_LOCK, but is still not able to report the parameters (USB-firmware-based frontends might be poorly implemented). And so on... regards, -- Patrick Boettcher Kernel Labs Inc. http://www.kernellabs.com/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] [media] dvb_frontend: Require FE_HAS_PARAMETERS for get_frontend()
Em 19-01-2012 08:07, Patrick Boettcher escreveu: Hi Mauro, On Wednesday 18 January 2012 18:51:25 Mauro Carvalho Chehab wrote: Calling get_frontend() before having either the frontend locked or the network signaling carriers locked won't work. So, block it at the DVB core. I like the idea and also the implementation. But before merging this needs more comments from other on the list. Agreed. Even though it does not break anything for any current frontend-driver it is important to have a wider base agreeing on that. Especially from some other frontend-driver-writers. For example I could imagine that a frontend HAS_LOCK, but is still not able to report the parameters (USB-firmware-based frontends might be poorly implemented). Yes, I understand. The description for HAS_LOCK is currently too generic, as it says everything is locked. If HAS_PARAMETERS can happen after that, then the description for HAS_LOCK need to be changed to something like: Indicates that the frontend is ready to tune and zap into a channel. Note: The network detected parameters might not yet be locked. Please see FE_HAS_PARAMETERS if you need to call FE_GET_FRONTEND/FE_GET_PROPERTY. A change like that could speed up zapping, as, for zap, HAS_PARAMETERS is not needed. Looking inside the ISDB-T devices: In the specific case of mb86a20s (an ISDB-T frontend), the locks are inside a state machine. After the frame sync (state 7), it tests for TMCC. After TMCC is locked (state 8), it waits for a while to rise the TS output lock (state 9). So, at least on devices based on it, HAS_PARAMETERS will always happen before HAS_LOCK. At the Siano driver, the firmware API has only two lock's: RF and Demod. The Demod lock probably means both HAS_PARAMETERS and HAS_LOCK. So, Demod lock will probably mean HAS_PARAMETERS | HAS_LOCK. Interesting enough, with Siano, one frontend call will return the entire TMCC table, plus the per-layer frontend statistics, including a measure for the TMCC carrier errors. At dib8000, the locks seem to be independent, but maybe there are some hardware requirements that require TMCC demod to happen, before rising the TS locks, but you're the one that knows most about DibCom drivers ;) @@ -207,8 +202,12 @@ static void dvb_frontend_add_event(struct dvb_frontend *fe, fe_status_t status) dprintk (%s\n, __func__); -if ((status FE_HAS_LOCK) has_get_frontend(fe)) -dtv_get_frontend(fe, fepriv-parameters_out); +/* FE_HAS_LOCK implies that the frontend has parameters */ +if (status FE_HAS_LOCK) +status |= FE_HAS_PARAMETERS; + The above code should be replaced by pushing the FE_HAS_PARAMETERS lock flag into the frontends that implement get_frontend(). This way, each lock can be independently implemented. Also, by not rising it on devices that don't implement get_frontend() will allow scan tools to decide to not rely on the demod to retrieve the network parameters. I didn't write such patch because I was too lazy ;) Seriously, It is better to do push that flag to the drivers on a separate patch, as it would be easier for review. I'll only write such thing after having some discussions about that. Writing such patch will give the opportunity to review each driver's logic and put FE_HAS_PARAMETERS into the right place. And so on... regards, -- Patrick Boettcher Kernel Labs Inc. http://www.kernellabs.com/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] [media] dvb_frontend: Require FE_HAS_PARAMETERS for get_frontend()
Calling get_frontend() before having either the frontend locked
or the network signaling carriers locked won't work. So, block
it at the DVB core.
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
---
drivers/media/dvb/dvb-core/dvb_frontend.c | 50 ++--
1 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c
b/drivers/media/dvb/dvb-core/dvb_frontend.c
index fbbe545..a15c4ed 100644
--- a/drivers/media/dvb/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
@@ -144,11 +144,6 @@ static void dvb_frontend_wakeup(struct dvb_frontend *fe);
static int dtv_get_frontend(struct dvb_frontend *fe,
struct dvb_frontend_parameters *p_out);
-static bool has_get_frontend(struct dvb_frontend *fe)
-{
- return fe-ops.get_frontend;
-}
-
/*
* Due to DVBv3 API calls, a delivery system should be mapped into one of
* the 4 DVBv3 delivery systems (FE_QPSK, FE_QAM, FE_OFDM or FE_ATSC),
@@ -207,8 +202,12 @@ static void dvb_frontend_add_event(struct dvb_frontend
*fe, fe_status_t status)
dprintk (%s\n, __func__);
- if ((status FE_HAS_LOCK) has_get_frontend(fe))
- dtv_get_frontend(fe, fepriv-parameters_out);
+ /* FE_HAS_LOCK implies that the frontend has parameters */
+ if (status FE_HAS_LOCK)
+ status |= FE_HAS_PARAMETERS;
+
+ fepriv-status = status;
+ dtv_get_frontend(fe, fepriv-parameters_out);
mutex_lock(events-mtx);
@@ -465,7 +464,6 @@ static void dvb_frontend_swzigzag(struct dvb_frontend *fe)
fe-ops.read_status(fe, s);
if (s != fepriv-status) {
dvb_frontend_add_event(fe, s);
- fepriv-status = s;
}
}
@@ -663,7 +661,6 @@ restart:
if (s != fepriv-status
!(fepriv-tune_mode_flags FE_TUNE_MODE_ONESHOT)) {
dprintk(%s: state changed, adding
current state\n, __func__);
dvb_frontend_add_event(fe, s);
- fepriv-status = s;
}
break;
case DVBFE_ALGO_SW:
@@ -698,7 +695,6 @@ restart:
fe-ops.read_status(fe, s);
if (s != fepriv-status) {
dvb_frontend_add_event(fe, s); /*
update event list */
- fepriv-status = s;
if (!(s FE_HAS_LOCK)) {
fepriv-delay = HZ / 10;
fepriv-algo_status |=
DVBFE_ALGO_SEARCH_AGAIN;
@@ -1213,18 +1209,26 @@ static int dtv_property_legacy_params_sync(struct
dvb_frontend *fe,
static int dtv_get_frontend(struct dvb_frontend *fe,
struct dvb_frontend_parameters *p_out)
{
+ struct dvb_frontend_private *fepriv = fe-frontend_priv;
int r;
- if (fe-ops.get_frontend) {
- r = fe-ops.get_frontend(fe);
- if (unlikely(r 0))
- return r;
- if (p_out)
- dtv_property_legacy_params_sync(fe, p_out);
- return 0;
+ /*
+* If the frontend is not locked, the transmission information
+* is not available. So, there's no sense on calling the frontend
+* to get anything, as all it has is what is already inside the
+* cache.
+*/
+ if (fepriv-status FE_HAS_PARAMETERS) {
+ if (fe-ops.get_frontend) {
+ r = fe-ops.get_frontend(fe);
+ if (unlikely(r 0))
+ return r;
+ }
}
+ if (p_out)
+ dtv_property_legacy_params_sync(fe, p_out);
- /* As everything is in cache, get_frontend fops are always supported */
+ /* As everything is in cache, get_frontend is always supported */
return 0;
}
@@ -1725,7 +1729,6 @@ static int dvb_frontend_ioctl_properties(struct file
*file,
{
struct dvb_device *dvbdev = file-private_data;
struct dvb_frontend *fe = dvbdev-priv;
- struct dvb_frontend_private *fepriv = fe-frontend_priv;
struct dtv_frontend_properties *c = fe-dtv_property_cache;
int err = 0;
@@ -1795,11 +1798,9 @@ static int dvb_frontend_ioctl_properties(struct file
*file,
* the data retrieved from get_frontend, if the frontend
* is not idle. Otherwise, returns the cached content
*/
- if (fepriv-state != FESTATE_IDLE) {
- err = dtv_get_frontend(fe, NULL);
- if (err 0)
- goto out;
-
