[PATCH] [RFC] dib8000: return an error if the TMCC is not locked

[PATCH] [RFC] dib8000: return an error if the TMCC is not locked

[Mauro Carvalho Chehab]

On ISDB-T, a few carriers are reserved for TMCC decoding
(1 to 20 carriers, depending on the mode). Those carriers
use the DBPSK modulation, and contain the information about
each of the three layers of carriers (modulation, partial
reception, inner code, interleaving, and number of segments).

If the TMCC carrier was not locked and decoded, no information
would be provided by get_frontend(). So, instead of returning
false values, return -EAGAIN.

Another alternative for this patch would be to add a flag to
fe_status (FE_HAS_GET_FRONTEND?), to indicate that the ISDB-T
TMCC carriers (and DVB-T TPS?), required for get_frontend
to work, are locked.

Comments?

Cc: Patrick Boettcher <pboettcher@kernellabs.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/dvb/frontends/dib8000.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/frontends/dib8000.c b/drivers/media/dvb/frontends/dib8000.c
index 9ca34f4..c566be2 100644
--- a/drivers/media/dvb/frontends/dib8000.c
+++ b/drivers/media/dvb/frontends/dib8000.c
@@ -2813,7 +2813,7 @@ EXPORT_SYMBOL(dib8000_set_tune_state);
 static int dib8000_get_frontend(struct dvb_frontend *fe)
 {
 	struct dib8000_state *state = fe->demodulator_priv;
-	u16 i, val = 0;
+	u16 i, val = 0, lock;
 	fe_status_t stat;
 	u8 index_frontend, sub_index_frontend;
 
@@ -2840,7 +2840,7 @@ static int dib8000_get_frontend(struct dvb_frontend *fe)
 					}
 				}
 			}
-			return 0;
+			goto ret;
 		}
 	}
 
@@ -2953,6 +2953,17 @@ static int dib8000_get_frontend(struct dvb_frontend *fe)
 			state->fe[index_frontend]->dtv_property_cache.layer[i].modulation = fe->dtv_property_cache.layer[i].modulation;
 		}
 	}
+
+ret:
+	if (state->revision != 0x8090)
+		lock = dib8000_read_word(state, 568);
+	else
+		lock = dib8000_read_word(state, 570);
+
+	/* Check if the TMCC decoder is locked */
+	if ((lock & 0x1e) != 0x1e)
+		return -EAGAIN;
+
 	return 0;
 }
 
-- 
1.7.8

Re: [PATCH] [RFC] dib8000: return an error if the TMCC is not locked

[Patrick Boettcher]

On Tuesday 17 January 2012 19:45:28 you wrote:
> On ISDB-T, a few carriers are reserved for TMCC decoding
> (1 to 20 carriers, depending on the mode). Those carriers
> use the DBPSK modulation, and contain the information about
> each of the three layers of carriers (modulation, partial
> reception, inner code, interleaving, and number of segments).
> 
> If the TMCC carrier was not locked and decoded, no information
> would be provided by get_frontend(). So, instead of returning
> false values, return -EAGAIN.
> 
> Another alternative for this patch would be to add a flag to
> fe_status (FE_HAS_GET_FRONTEND?), to indicate that the ISDB-T
> TMCC carriers (and DVB-T TPS?), required for get_frontend
> to work, are locked.
> 
> Comments?

I think it changes the behaviour of get_frontend too much and in fact 
transmission parameter signaling (TPS for DVB-T, TMCC for ISDB-T) locks 
are already or should be if not integrated to the status locks.

Also those parameters can change over time and signal a 
"reconfiguration" of the transmission.

So, for me I would vote against this kind of implementation in favor a 
better one. Unfortunately I don't have a much better idea at hand right 
now.

--
Patrick Boettcher

Kernel Labs Inc.
http://www.kernellabs.com/

Re: [PATCH] [RFC] dib8000: return an error if the TMCC is not locked

[Mauro Carvalho Chehab]

Em 18-01-2012 10:49, Patrick Boettcher escreveu:
> On Tuesday 17 January 2012 19:45:28 you wrote:
>> On ISDB-T, a few carriers are reserved for TMCC decoding
>> (1 to 20 carriers, depending on the mode). Those carriers
>> use the DBPSK modulation, and contain the information about
>> each of the three layers of carriers (modulation, partial
>> reception, inner code, interleaving, and number of segments).
>>
>> If the TMCC carrier was not locked and decoded, no information
>> would be provided by get_frontend(). So, instead of returning
>> false values, return -EAGAIN.
>>
>> Another alternative for this patch would be to add a flag to
>> fe_status (FE_HAS_GET_FRONTEND?), to indicate that the ISDB-T
>> TMCC carriers (and DVB-T TPS?), required for get_frontend
>> to work, are locked.
>>
>> Comments?
> 
> I think it changes the behaviour of get_frontend too much and in fact 
> transmission parameter signaling (TPS for DVB-T, TMCC for ISDB-T) locks 
> are already or should be if not integrated to the status locks.
> 
> Also those parameters can change over time and signal a 
> "reconfiguration" of the transmission.
> 
> So, for me I would vote against this kind of implementation in favor a 
> better one. Unfortunately I don't have a much better idea at hand right 
> now.

The current status are:

typedef enum fe_status {
        FE_HAS_SIGNAL   = 0x01,   /* found something above the noise level */
        FE_HAS_CARRIER  = 0x02,   /* found a DVB signal  */
        FE_HAS_VITERBI  = 0x04,   /* FEC is stable  */
        FE_HAS_SYNC     = 0x08,   /* found sync bytes  */
        FE_HAS_LOCK     = 0x10,   /* everything's working... */
        FE_TIMEDOUT     = 0x20,   /* no lock within the last ~2 seconds */
        FE_REINIT	= 0x40    /* frontend was reinitialized,  */
} fe_status_t;                    /* application is recommended to reset */

There are a few options that can be done:

1) only rise FE_HAS_LOCK if TPS/TMCC demod were locked. The "description"
for FE_HAS_LOCK ("everything's working") seems to indicate that TMCC
lock/TPS lock is also part of "everything".

2) create a new status for it.

In any case, I'm in favor of explicitly telling about that at the specs.

It seems that the dynamic reconfiguration, if detected by the frontend,
could be indicated via FE_REINIT. This is not clear, as no driver uses
this flag (btw, FE_TIMEDOUT is barely used - only 4 drivers use it).

I'm not sure if it is a frontend's task to monitor the TMCC 
Indicator of transmission-parameter switching, in order to warn the
userspace that a transmission reconfiguration will happen (via FE_REINIT?),
or if some userspace-driven mechanism for that is needed.

With regards to dvb-core get_frontend() call, it only makes sense if
the TPS/TMCC is locked. So, I think that such call needs to be limited
to happen only when the lock were archived, like the enclosed patch.

-

[PATCH] dvb: only calls get_frontend() if the frontend is locked

While the device is not locked, calling get_frontend() shouldn't
return anything useful, as there are not locks there yet. So,
the frontend wouldn't return anything useful. Also, on most cases,
it will generate uneeded hardware register access, in general
via slow I2C transfers, in order to read the register contents of
the previous state.

Instead of doing that, the DVB frontend thread keeps the status
of the frontend lock, and knows when the frontend is locked. So,
relies on that, in order to allow calling the frontend's logic.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c b/drivers/media/dvb/dvb-core/dvb_frontend.c
index fbbe545..548aace 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,8 @@ 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);
+	fepriv->status = status;
+	dtv_get_frontend(fe, &fepriv->parameters_out);
 
 	mutex_lock(&events->mtx);
 
@@ -465,7 +460,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 +657,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 +691,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 +1205,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_LOCK) {
+		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 +1725,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 +1794,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;
-		}
+		err = dtv_get_frontend(fe, NULL);
+		if (err < 0)
+			goto out;
 		for (i = 0; i < tvps->num; i++) {
 			err = dtv_property_process_get(fe, c, tvp + i, file);
 			if (err < 0)
@@ -1922,7 +1919,6 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
 	dvb_frontend_clear_events(fe);
 	dvb_frontend_add_event(fe, 0);
 	dvb_frontend_wakeup(fe);
-	fepriv->status = 0;
 
 	return 0;
 }

Re: [PATCH] [RFC] dib8000: return an error if the TMCC is not locked

[Patrick Boettcher]

On Wednesday 18 January 2012 14:40:09 Mauro Carvalho Chehab wrote:
> Em 18-01-2012 10:49, Patrick Boettcher escreveu:
> > On Tuesday 17 January 2012 19:45:28 you wrote:
> >> On ISDB-T, a few carriers are reserved for TMCC decoding
> >> (1 to 20 carriers, depending on the mode). Those carriers
> >> use the DBPSK modulation, and contain the information about
> >> each of the three layers of carriers (modulation, partial
> >> reception, inner code, interleaving, and number of segments).
> >> 
> >> If the TMCC carrier was not locked and decoded, no information
> >> would be provided by get_frontend(). So, instead of returning
> >> false values, return -EAGAIN.
> >> 
> >> Another alternative for this patch would be to add a flag to
> >> fe_status (FE_HAS_GET_FRONTEND?), to indicate that the ISDB-T
> >> TMCC carriers (and DVB-T TPS?), required for get_frontend
> >> to work, are locked.
> >> 
> >> Comments?
> > 
> > I think it changes the behaviour of get_frontend too much and in
> > fact transmission parameter signaling (TPS for DVB-T, TMCC for
> > ISDB-T) locks are already or should be if not integrated to the
> > status locks.
> > 
> > Also those parameters can change over time and signal a
> > "reconfiguration" of the transmission.
> > 
> > So, for me I would vote against this kind of implementation in
> > favor a better one. Unfortunately I don't have a much better idea
> > at hand right now.
> 
> The current status are:
> 
> typedef enum fe_status {
>         FE_HAS_SIGNAL   = 0x01,   /* found something above the noise
> level */ FE_HAS_CARRIER  = 0x02,   /* found a DVB signal  */
>         FE_HAS_VITERBI  = 0x04,   /* FEC is stable  */
>         FE_HAS_SYNC     = 0x08,   /* found sync bytes  */
>         FE_HAS_LOCK     = 0x10,   /* everything's working... */
>         FE_TIMEDOUT     = 0x20,   /* no lock within the last ~2
> seconds */ FE_REINIT	= 0x40    /* frontend was reinitialized,  */ }
> fe_status_t;                    /* application is recommended to
> reset */
> 
> There are a few options that can be done:
> 
> 1) only rise FE_HAS_LOCK if TPS/TMCC demod were locked. The
> "description" for FE_HAS_LOCK ("everything's working") seems to
> indicate that TMCC lock/TPS lock is also part of "everything".

HAS_LOCK includes TPS-lock, right. But TPS-lock can raise much before 
HAS_LOCK and at worse signal conditions. In DVB-T and ISDB-T we can have 
the parameters at -1 dB SNR (or below) whereas data reception at least 
needs  ~3 dB ( QPSK 1/2 in DVB-T)  and much more for the modulations 
used currently on earth.

> 2) create a new status for it.

Maybe that's the way to go then. FE_HAS_PARAMETERS.

> With regards to dvb-core get_frontend() call, it only makes sense if
> the TPS/TMCC is locked. So, I think that such call needs to be
> limited to happen only when the lock were archived, like the
> enclosed patch.

OK.

--
Patrick Boettcher

Kernel Labs Inc.
http://www.kernellabs.com/

[PATCH 1/2] dvb: frontend API: Add a flag to indicate that get_frontend() can be called

[Mauro Carvalho Chehab]

get_frontend() can't be called too early, as the device may not have it
yet. Yet, get_frontend() on OFDM standards can happen before FE_HAS_LOCK,
as the TMCC carriers (ISDB-T) or the TPS carriers (DVB-T) require a very
low signal to noise relation to be detected. The other carriers use
different modulations, so they require a higher SNR.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 Documentation/DocBook/media/dvb/frontend.xml |   59 +++++++++++++++++++++-----
 include/linux/dvb/frontend.h                 |   33 ++++++++++----
 2 files changed, 72 insertions(+), 20 deletions(-)

diff --git a/Documentation/DocBook/media/dvb/frontend.xml b/Documentation/DocBook/media/dvb/frontend.xml
index aeaed59..5426bdc 100644
--- a/Documentation/DocBook/media/dvb/frontend.xml
+++ b/Documentation/DocBook/media/dvb/frontend.xml
@@ -207,18 +207,55 @@ spec.</para>
 <para>Several functions of the frontend device use the fe_status data type defined
 by</para>
 <programlisting>
- typedef enum fe_status {
-	 FE_HAS_SIGNAL     = 0x01,   /&#x22C6;  found something above the noise level &#x22C6;/
-	 FE_HAS_CARRIER    = 0x02,   /&#x22C6;  found a DVB signal  &#x22C6;/
-	 FE_HAS_VITERBI    = 0x04,   /&#x22C6;  FEC is stable  &#x22C6;/
-	 FE_HAS_SYNC       = 0x08,   /&#x22C6;  found sync bytes  &#x22C6;/
-	 FE_HAS_LOCK       = 0x10,   /&#x22C6;  everything's working... &#x22C6;/
-	 FE_TIMEDOUT       = 0x20,   /&#x22C6;  no lock within the last ~2 seconds &#x22C6;/
-	 FE_REINIT         = 0x40    /&#x22C6;  frontend was reinitialized,  &#x22C6;/
- } fe_status_t;                      /&#x22C6;  application is recommned to reset &#x22C6;/
+typedef enum fe_status {
+	FE_HAS_SIGNAL		= 0x01,
+	FE_HAS_CARRIER		= 0x02,
+	FE_HAS_VITERBI		= 0x04,
+	FE_HAS_SYNC		= 0x08,
+	FE_HAS_LOCK		= 0x10,
+	FE_TIMEDOUT		= 0x20,
+	FE_REINIT		= 0x40,
+	FE_HAS_PARAMETERS	= 0x80,
+} fe_status_t;
 </programlisting>
-<para>to indicate the current state and/or state changes of the frontend hardware.
-</para>
+<para>to indicate the current state and/or state changes of the frontend hardware:
+</para>
+
+<informaltable><tgroup cols="2"><tbody>
+<row>
+<entry align="char">FE_HAS_SIGNAL</entry>
+<entry align="char">The frontend has found something above the noise level</entry>
+</row><row>
+<entry align="char">FE_HAS_CARRIER</entry>
+<entry align="char">The frontend has found a DVB signal</entry>
+</row><row>
+<entry align="char">FE_HAS_VITERBI</entry>
+<entry align="char">The frontend FEC code is stable</entry>
+</row><row>
+<entry align="char">FE_HAS_SYNC</entry>
+<entry align="char">Syncronization bytes was found</entry>
+</row><row>
+<entry align="char">FE_HAS_LOCK</entry>
+<entry align="char">The DVB were locked and everything is working</entry>
+</row><row>
+<entry align="char">FE_TIMEDOUT</entry>
+<entry align="char">no lock within the last about 2 seconds</entry>
+</row><row>
+<entry align="char">FE_REINIT</entry>
+<entry align="char">The frontend was reinitialized, application is
+recommended to reset DiSEqC, tone and parameters</entry>
+</row><row>
+<entry align="char">FE_HAS_PARAMETERS</entry>
+<entry align="char"><link linkend="FE_GET_SET_PROPERTY">
+<constant>FE_GET_PROPERTY/FE_SET_PROPERTY</constant></link> or
+<link linkend="FE_GET_FRONTEND"><constant>FE_GET_FRONTEND</constant></link> can now be
+called to provide the detected network parameters.
+This should be risen for example when the DVB-T TPS/ISDB-T TMCC is locked.
+This status can be risen before FE_HAS_SYNC, as the SNR required for
+parameters detection is lower than the requirement for the other
+carriers on the OFDM delivery systems.
+</entry>
+</row></tbody></tgroup></informaltable>
 
 </section>
 
diff --git a/include/linux/dvb/frontend.h b/include/linux/dvb/frontend.h
index cb4428a..38fa9ef 100644
--- a/include/linux/dvb/frontend.h
+++ b/include/linux/dvb/frontend.h
@@ -121,16 +121,31 @@ typedef enum fe_sec_mini_cmd {
 } fe_sec_mini_cmd_t;
 
 
+/**
+ * enum fe_status - enumerates the possible frontend status
+ * @FE_HAS_SIGNAL:	found something above the noise level
+ * @FE_HAS_CARRIER:	found a DVB signal
+ * @FE_HAS_VITERBI:	FEC is stable
+ * @FE_HAS_SYNC:	found sync bytes
+ * @FE_HAS_LOCK:	everything's working
+ * @FE_TIMEDOUT:	no lock within the last ~2 seconds
+ * @FE_REINIT:		frontend was reinitialized, application is recommended
+ *			to reset DiSEqC, tone and parameters
+ * @FE_HAS_PARAMETERS:	get_frontend() can now be called to provide the
+ *			detected network parameters. This should be risen
+ *			for example when the DVB-T TPS/ISDB-T TMCC is locked.
+ */
+
 typedef enum fe_status {
-	FE_HAS_SIGNAL	= 0x01,   /* found something above the noise level */
-	FE_HAS_CARRIER	= 0x02,   /* found a DVB signal  */
-	FE_HAS_VITERBI	= 0x04,   /* FEC is stable  */
-	FE_HAS_SYNC	= 0x08,   /* found sync bytes  */
-	FE_HAS_LOCK	= 0x10,   /* everything's working... */
-	FE_TIMEDOUT	= 0x20,   /* no lock within the last ~2 seconds */
-	FE_REINIT	= 0x40    /* frontend was reinitialized,  */
-} fe_status_t;			  /* application is recommended to reset */
-				  /* DiSEqC, tone and parameters */
+	FE_HAS_SIGNAL		= 0x01,
+	FE_HAS_CARRIER		= 0x02,
+	FE_HAS_VITERBI		= 0x04,
+	FE_HAS_SYNC		= 0x08,
+	FE_HAS_LOCK		= 0x10,
+	FE_TIMEDOUT		= 0x20,
+	FE_REINIT		= 0x40,
+	FE_HAS_PARAMETERS	= 0x80,
+} fe_status_t;
 
 typedef enum fe_spectral_inversion {
 	INVERSION_OFF,
-- 
1.7.8

[PATCH 2/2] [media] dvb_frontend: Require FE_HAS_PARAMETERS for get_frontend()

[Mauro Carvalho Chehab]

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 <mchehab@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;
-		}
+		err = dtv_get_frontend(fe, NULL);
+		if (err < 0)
+			goto out;
 		for (i = 0; i < tvps->num; i++) {
 			err = dtv_property_process_get(fe, c, tvp + i, file);
 			if (err < 0)
@@ -1922,7 +1923,6 @@ static int dtv_set_frontend(struct dvb_frontend *fe)
 	dvb_frontend_clear_events(fe);
 	dvb_frontend_add_event(fe, 0);
 	dvb_frontend_wakeup(fe);
-	fepriv->status = 0;
 
 	return 0;
 }
-- 
1.7.8

Re: [PATCH 2/2] [media] dvb_frontend: Require FE_HAS_PARAMETERS for get_frontend()

[Patrick Boettcher]

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/

Re: [PATCH 2/2] [media] dvb_frontend: Require FE_HAS_PARAMETERS for get_frontend()

[Mauro Carvalho Chehab]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html