[patch] Fix AF9015 Dual tuner i2c write failures

[patch] Fix AF9015 Dual tuner i2c write failures

[Andrew de Quincey]

[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]

Hi, this has been annoying me for some time, so this evening I fixed
it. If you use one of the above dual tuner devices (e.g. KWorld 399U),
you get random tuning failures and i2c errors reported in dmesg such
as:

af9013: I2C read failed reg:d607
af9015: command failed:1
mxl5005s I2C write failed
af9015: command failed:1
mxl5005s I2C write failed
af9015: command failed:2
af9013: I2C read failed reg:d607
af9015: command failed:2
af9013: I2C read failed reg:d607

Looking at the large comment in dvb-usb/af9015.c/af9015_i2c_xfer() it
seems the dual demods/tuners have the same i2c addresses and are
switched by an i2c gate. Since the two show up as separate DVB
devices, and there is no synchronisation of i2c accesses, its fairly
obvious the problem is because both tuners are being written to at the
same time, which is mucking up the i2c gate. I tested this with two
scripts on my development machine:

while [ 1 ];
do
        scan -a 0 /usr/share/dvb/dvb-t/uk-Craigkelly
done

AND

while [ 1 ];
do
        scan -a 1 /usr/share/dvb/dvb-t/uk-Craigkelly
done


If I run only one at a time, it works perfectly. Run two, and I start
to see errors.

Adding a "bus lock" to af9015_i2c_xfer() will not work as demod/tuner
accesses will take multiple i2c transactions.

Therefore, the following patch overrides the dvb_frontend_ops
functions to add a per-device lock around them: only one frontend can
now use the i2c bus at a time. Testing with the scripts above shows
this has eliminated the errors.

Signed-off-by: Andrew de Quincey <adq_dvb@lidskialf.net>

[-- Attachment #2: af9015-fix-dual-tuner-v1.patch --]
[-- Type: text/x-patch, Size: 4579 bytes --]

diff --git a/drivers/media/dvb/dvb-usb/af9015.c b/drivers/media/dvb/dvb-usb/af9015.c
index 31c0a0e..740d969 100644
--- a/drivers/media/dvb/dvb-usb/af9015.c
+++ b/drivers/media/dvb/dvb-usb/af9015.c
@@ -1083,6 +1083,104 @@ static int af9015_i2c_init(struct dvb_usb_device *d)
 	return ret;
 }
 
+static int af9015_lock_set_frontend(struct dvb_frontend* fe, struct dvb_frontend_parameters* params)
+{
+	int result;
+	struct dvb_usb_adapter *adap = fe->dvb->priv;
+	struct af9015_state *state = adap->dev->priv;
+
+	if (mutex_lock_interruptible(&adap->dev->usb_mutex))
+		return -EAGAIN;
+
+	result = state->fe_ops[adap->id].set_frontend(fe, params);
+	mutex_unlock(&adap->dev->usb_mutex);
+	return result;
+}
+                
+static int af9015_lock_get_frontend(struct dvb_frontend* fe, struct dvb_frontend_parameters* params)
+{
+	int result;
+	struct dvb_usb_adapter *adap = fe->dvb->priv;
+	struct af9015_state *state = adap->dev->priv;
+
+	if (mutex_lock_interruptible(&adap->dev->usb_mutex))
+		return -EAGAIN;
+
+	result = state->fe_ops[adap->id].get_frontend(fe, params);
+	mutex_unlock(&adap->dev->usb_mutex);
+	return result;
+}
+                        
+static int af9015_lock_read_status(struct dvb_frontend* fe, fe_status_t* status)
+{
+	int result;
+	struct dvb_usb_adapter *adap = fe->dvb->priv;
+	struct af9015_state *state = adap->dev->priv;
+
+	if (mutex_lock_interruptible(&adap->dev->usb_mutex))
+		return -EAGAIN;
+
+	result = state->fe_ops[adap->id].read_status(fe, status);
+	mutex_unlock(&adap->dev->usb_mutex);
+	return result;
+}
+
+static int af9015_lock_read_ber(struct dvb_frontend* fe, u32* ber)
+{
+	int result;
+	struct dvb_usb_adapter *adap = fe->dvb->priv;
+	struct af9015_state *state = adap->dev->priv;
+
+	if (mutex_lock_interruptible(&adap->dev->usb_mutex))
+		return -EAGAIN;
+
+	result = state->fe_ops[adap->id].read_ber(fe, ber);
+	mutex_unlock(&adap->dev->usb_mutex);
+	return result;
+}
+
+static int af9015_lock_read_signal_strength(struct dvb_frontend* fe, u16* strength)
+{
+	int result;
+	struct dvb_usb_adapter *adap = fe->dvb->priv;
+	struct af9015_state *state = adap->dev->priv;
+
+	if (mutex_lock_interruptible(&adap->dev->usb_mutex))
+		return -EAGAIN;
+
+	result = state->fe_ops[adap->id].read_signal_strength(fe, strength);
+	mutex_unlock(&adap->dev->usb_mutex);
+	return result;
+}
+
+static int af9015_lock_read_snr(struct dvb_frontend* fe, u16* snr)
+{
+	int result;
+	struct dvb_usb_adapter *adap = fe->dvb->priv;
+	struct af9015_state *state = adap->dev->priv;
+
+	if (mutex_lock_interruptible(&adap->dev->usb_mutex))
+		return -EAGAIN;
+
+	result = state->fe_ops[adap->id].read_snr(fe, snr);
+	mutex_unlock(&adap->dev->usb_mutex);
+	return result;
+}
+
+static int af9015_lock_read_ucblocks(struct dvb_frontend* fe, u32* ucblocks)
+{
+	int result;
+	struct dvb_usb_adapter *adap = fe->dvb->priv;
+	struct af9015_state *state = adap->dev->priv;
+
+	if (mutex_lock_interruptible(&adap->dev->usb_mutex))
+		return -EAGAIN;
+
+	result = state->fe_ops[adap->id].read_ucblocks(fe, ucblocks);
+	mutex_unlock(&adap->dev->usb_mutex);
+	return result;
+}
+
 static int af9015_af9013_frontend_attach(struct dvb_usb_adapter *adap)
 {
 	int ret;
@@ -1116,6 +1214,22 @@ static int af9015_af9013_frontend_attach(struct dvb_usb_adapter *adap)
 	/* attach demodulator */
 	adap->fe = dvb_attach(af9013_attach, &af9015_af9013_config[adap->id],
 		i2c_adap);
+		
+	memcpy(&state->fe_ops[adap->id], &adap->fe->ops, sizeof(struct dvb_frontend_ops));
+	if (adap->fe->ops.set_frontend)
+		adap->fe->ops.set_frontend = af9015_lock_set_frontend;
+	if (adap->fe->ops.get_frontend)
+		adap->fe->ops.get_frontend = af9015_lock_get_frontend;
+	if (adap->fe->ops.read_status)
+		adap->fe->ops.read_status = af9015_lock_read_status;
+	if (adap->fe->ops.read_ber)
+		adap->fe->ops.read_ber = af9015_lock_read_ber;
+	if (adap->fe->ops.read_signal_strength)
+		adap->fe->ops.read_signal_strength = af9015_lock_read_signal_strength;
+	if (adap->fe->ops.read_snr)
+		adap->fe->ops.read_snr = af9015_lock_read_snr;
+	if (adap->fe->ops.read_ucblocks)
+		adap->fe->ops.read_ucblocks = af9015_lock_read_ucblocks;
 
 	return adap->fe == NULL ? -ENODEV : 0;
 }
diff --git a/drivers/media/dvb/dvb-usb/af9015.h b/drivers/media/dvb/dvb-usb/af9015.h
index f20cfa6..759bb3f 100644
--- a/drivers/media/dvb/dvb-usb/af9015.h
+++ b/drivers/media/dvb/dvb-usb/af9015.h
@@ -102,6 +102,7 @@ struct af9015_state {
 	struct i2c_adapter i2c_adap; /* I2C adapter for 2nd FE */
 	u8 rc_repeat;
 	u32 rc_keycode;
+	struct dvb_frontend_ops fe_ops[2];
 };
 
 struct af9015_config {

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Antti Palosaari]

Wow, thanks!

On 03/04/2011 11:37 PM, Andrew de Quincey wrote:
> Hi, this has been annoying me for some time, so this evening I fixed
> it. If you use one of the above dual tuner devices (e.g. KWorld 399U),
> you get random tuning failures and i2c errors reported in dmesg such
> as:
[...]
> Adding a "bus lock" to af9015_i2c_xfer() will not work as demod/tuner
> accesses will take multiple i2c transactions.
>
> Therefore, the following patch overrides the dvb_frontend_ops
> functions to add a per-device lock around them: only one frontend can
> now use the i2c bus at a time. Testing with the scripts above shows
> this has eliminated the errors.

This have annoyed me too, but since it does not broken functionality 
much I haven't put much effort for fixing it. I like that fix since it 
is in AF9015 driver where it logically belongs to. But it looks still 
rather complex. I see you have also considered "bus lock" to 
af9015_i2c_xfer() which could be much smaller in code size (that's I 
have tried to implement long time back).

I would like to ask if it possible to check I2C gate open / close inside 
af9015_i2c_xfer() and lock according that? Something like:

typical command sequence:
 >> FE0 open gate
 >> FE0 write reg
 >> FE0 close gate
 >> FE1 open gate
 >> FE1 read reg
 >> FE1 close gate

if (locked == YES)
   if (locked_by != caller FE)
     return error locked by other FE
   else (locked_by == caller FE)
     allow reg access
     if (gate close req)
       locked = NO
       locked_by = NONE
else (locked == NO)
   locked = YES
   locked_by = caller FE
   allow reg access

Do you see it possible?

thanks
Antti
-- 
http://palosaari.fi/

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Andrew de Quincey]

[-- Attachment #1: Type: text/plain, Size: 1700 bytes --]

>> Adding a "bus lock" to af9015_i2c_xfer() will not work as demod/tuner
>> accesses will take multiple i2c transactions.
>>
>> Therefore, the following patch overrides the dvb_frontend_ops
>> functions to add a per-device lock around them: only one frontend can
>> now use the i2c bus at a time. Testing with the scripts above shows
>> this has eliminated the errors.
>
> This have annoyed me too, but since it does not broken functionality much I
> haven't put much effort for fixing it. I like that fix since it is in AF9015
> driver where it logically belongs to. But it looks still rather complex. I
> see you have also considered "bus lock" to af9015_i2c_xfer() which could be
> much smaller in code size (that's I have tried to implement long time back).
>
> I would like to ask if it possible to check I2C gate open / close inside
> af9015_i2c_xfer() and lock according that? Something like:

Hmm, I did think about that, but I felt overriding the functions was
just cleaner: I felt it was more obvious what it was doing. Doing
exactly this sort of tweaking was one of the main reasons we added
that function overriding feature.

I don't like the idea of returning "error locked by FE" since that'll
mean the tuning will randomly fail sometimes in a way visible to
userspace (unless we change the core dvb_frontend code), which was one
of the things I was trying to avoid. Unless, of course, I've
misunderstood your proposal.

However, looking at the code again, I realise it is possible to
simplify it. Since its only the demod gates that cause a problem, we
only /actually/ need to lock the get_frontend() and set_frontend()
calls.

Signed-off-by: Andrew de Quincey <adq_dvb@lidskialf.net>

[-- Attachment #2: af9015-fix-dual-tuner-v2.patch --]
[-- Type: text/x-patch, Size: 2150 bytes --]

diff --git a/drivers/media/dvb/dvb-usb/af9015.c b/drivers/media/dvb/dvb-usb/af9015.c
index 31c0a0e..52ac6ef 100644
--- a/drivers/media/dvb/dvb-usb/af9015.c
+++ b/drivers/media/dvb/dvb-usb/af9015.c
@@ -1083,6 +1083,34 @@ static int af9015_i2c_init(struct dvb_usb_device *d)
 	return ret;
 }
 
+static int af9015_lock_set_frontend(struct dvb_frontend* fe, struct dvb_frontend_parameters* params)
+{
+	int result;
+	struct dvb_usb_adapter *adap = fe->dvb->priv;
+	struct af9015_state *state = adap->dev->priv;
+
+	if (mutex_lock_interruptible(&adap->dev->usb_mutex))
+		return -EAGAIN;
+
+	result = state->fe_ops[adap->id].set_frontend(fe, params);
+	mutex_unlock(&adap->dev->usb_mutex);
+	return result;
+}
+                
+static int af9015_lock_get_frontend(struct dvb_frontend* fe, struct dvb_frontend_parameters* params)
+{
+	int result;
+	struct dvb_usb_adapter *adap = fe->dvb->priv;
+	struct af9015_state *state = adap->dev->priv;
+
+	if (mutex_lock_interruptible(&adap->dev->usb_mutex))
+		return -EAGAIN;
+
+	result = state->fe_ops[adap->id].get_frontend(fe, params);
+	mutex_unlock(&adap->dev->usb_mutex);
+	return result;
+}
+
 static int af9015_af9013_frontend_attach(struct dvb_usb_adapter *adap)
 {
 	int ret;
@@ -1116,6 +1144,12 @@ static int af9015_af9013_frontend_attach(struct dvb_usb_adapter *adap)
 	/* attach demodulator */
 	adap->fe = dvb_attach(af9013_attach, &af9015_af9013_config[adap->id],
 		i2c_adap);
+		
+	memcpy(&state->fe_ops[adap->id], &adap->fe->ops, sizeof(struct dvb_frontend_ops));
+	if (adap->fe->ops.set_frontend)
+		adap->fe->ops.set_frontend = af9015_lock_set_frontend;
+	if (adap->fe->ops.get_frontend)
+		adap->fe->ops.get_frontend = af9015_lock_get_frontend;
 
 	return adap->fe == NULL ? -ENODEV : 0;
 }
diff --git a/drivers/media/dvb/dvb-usb/af9015.h b/drivers/media/dvb/dvb-usb/af9015.h
index f20cfa6..759bb3f 100644
--- a/drivers/media/dvb/dvb-usb/af9015.h
+++ b/drivers/media/dvb/dvb-usb/af9015.h
@@ -102,6 +102,7 @@ struct af9015_state {
 	struct i2c_adapter i2c_adap; /* I2C adapter for 2nd FE */
 	u8 rc_repeat;
 	u32 rc_keycode;
+	struct dvb_frontend_ops fe_ops[2];
 };
 
 struct af9015_config {

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Antti Palosaari]

On 03/05/2011 12:44 AM, Andrew de Quincey wrote:
>>> Adding a "bus lock" to af9015_i2c_xfer() will not work as demod/tuner
>>> accesses will take multiple i2c transactions.
>>>
>>> Therefore, the following patch overrides the dvb_frontend_ops
>>> functions to add a per-device lock around them: only one frontend can
>>> now use the i2c bus at a time. Testing with the scripts above shows
>>> this has eliminated the errors.
>>
>> This have annoyed me too, but since it does not broken functionality much I
>> haven't put much effort for fixing it. I like that fix since it is in AF9015
>> driver where it logically belongs to. But it looks still rather complex. I
>> see you have also considered "bus lock" to af9015_i2c_xfer() which could be
>> much smaller in code size (that's I have tried to implement long time back).
>>
>> I would like to ask if it possible to check I2C gate open / close inside
>> af9015_i2c_xfer() and lock according that? Something like:
>
> Hmm, I did think about that, but I felt overriding the functions was
> just cleaner: I felt it was more obvious what it was doing. Doing
> exactly this sort of tweaking was one of the main reasons we added
> that function overriding feature.
>
> I don't like the idea of returning "error locked by FE" since that'll
> mean the tuning will randomly fail sometimes in a way visible to
> userspace (unless we change the core dvb_frontend code), which was one
> of the things I was trying to avoid. Unless, of course, I've
> misunderstood your proposal.

Not returning error, but waiting in lock like that:
if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
   return -EAGAIN;

> However, looking at the code again, I realise it is possible to
> simplify it. Since its only the demod gates that cause a problem, we
> only /actually/ need to lock the get_frontend() and set_frontend()
> calls.

I don't understand why .get_frontend() causes problem, since it does not 
access tuner at all. It only reads demod registers. The main problem is 
(like schema in af9015.c shows) that there is two tuners on same I2C bus 
using same address. And demod gate is only way to open access for 
desired tuner only.

You should block traffic based of tuner not demod. And I think those 
callbacks which are needed for override are tuner driver callbacks. 
Consider situation device goes it v4l-core calls same time both tuner 
.sleep() == problem.

Antti
-- 
http://palosaari.fi/

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Andrew de Quincey]

On 4 March 2011 22:59, Antti Palosaari <crope@iki.fi> wrote:
> On 03/05/2011 12:44 AM, Andrew de Quincey wrote:
>>>>
>>>> Adding a "bus lock" to af9015_i2c_xfer() will not work as demod/tuner
>>>> accesses will take multiple i2c transactions.
>>>>
>>>> Therefore, the following patch overrides the dvb_frontend_ops
>>>> functions to add a per-device lock around them: only one frontend can
>>>> now use the i2c bus at a time. Testing with the scripts above shows
>>>> this has eliminated the errors.
>>>
>>> This have annoyed me too, but since it does not broken functionality much
>>> I
>>> haven't put much effort for fixing it. I like that fix since it is in
>>> AF9015
>>> driver where it logically belongs to. But it looks still rather complex.
>>> I
>>> see you have also considered "bus lock" to af9015_i2c_xfer() which could
>>> be
>>> much smaller in code size (that's I have tried to implement long time
>>> back).
>>>
>>> I would like to ask if it possible to check I2C gate open / close inside
>>> af9015_i2c_xfer() and lock according that? Something like:
>>
>> Hmm, I did think about that, but I felt overriding the functions was
>> just cleaner: I felt it was more obvious what it was doing. Doing
>> exactly this sort of tweaking was one of the main reasons we added
>> that function overriding feature.
>>
>> I don't like the idea of returning "error locked by FE" since that'll
>> mean the tuning will randomly fail sometimes in a way visible to
>> userspace (unless we change the core dvb_frontend code), which was one
>> of the things I was trying to avoid. Unless, of course, I've
>> misunderstood your proposal.
>
> Not returning error, but waiting in lock like that:
> if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
>  return -EAGAIN;

Ah k, sorry

>> However, looking at the code again, I realise it is possible to
>> simplify it. Since its only the demod gates that cause a problem, we
>> only /actually/ need to lock the get_frontend() and set_frontend()
>> calls.
>
> I don't understand why .get_frontend() causes problem, since it does not
> access tuner at all. It only reads demod registers. The main problem is
> (like schema in af9015.c shows) that there is two tuners on same I2C bus
> using same address. And demod gate is only way to open access for desired
> tuner only.

AFAIR /some/ tuner code accesses the tuner hardware to read the exact
tuned frequency back on a get_frontend(); was just being extra
paranoid :)

> You should block traffic based of tuner not demod. And I think those
> callbacks which are needed for override are tuner driver callbacks. Consider
> situation device goes it v4l-core calls same time both tuner .sleep() ==
> problem.

Hmm, yeah, you're right, let me have another look tomorrow.

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

[-- Attachment #1: Type: text/plain, Size: 3289 bytes --]

On 4 March 2011 23:11, Andrew de Quincey <adq_dvb@lidskialf.net> wrote:
> On 4 March 2011 22:59, Antti Palosaari <crope@iki.fi> wrote:
>> On 03/05/2011 12:44 AM, Andrew de Quincey wrote:
>>>>>
>>>>> Adding a "bus lock" to af9015_i2c_xfer() will not work as demod/tuner
>>>>> accesses will take multiple i2c transactions.
>>>>>
>>>>> Therefore, the following patch overrides the dvb_frontend_ops
>>>>> functions to add a per-device lock around them: only one frontend can
>>>>> now use the i2c bus at a time. Testing with the scripts above shows
>>>>> this has eliminated the errors.
>>>>
>>>> This have annoyed me too, but since it does not broken functionality much
>>>> I
>>>> haven't put much effort for fixing it. I like that fix since it is in
>>>> AF9015
>>>> driver where it logically belongs to. But it looks still rather complex.
>>>> I
>>>> see you have also considered "bus lock" to af9015_i2c_xfer() which could
>>>> be
>>>> much smaller in code size (that's I have tried to implement long time
>>>> back).
>>>>
>>>> I would like to ask if it possible to check I2C gate open / close inside
>>>> af9015_i2c_xfer() and lock according that? Something like:
>>>
>>> Hmm, I did think about that, but I felt overriding the functions was
>>> just cleaner: I felt it was more obvious what it was doing. Doing
>>> exactly this sort of tweaking was one of the main reasons we added
>>> that function overriding feature.
>>>
>>> I don't like the idea of returning "error locked by FE" since that'll
>>> mean the tuning will randomly fail sometimes in a way visible to
>>> userspace (unless we change the core dvb_frontend code), which was one
>>> of the things I was trying to avoid. Unless, of course, I've
>>> misunderstood your proposal.
>>
>> Not returning error, but waiting in lock like that:
>> if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
>>  return -EAGAIN;
>
> Ah k, sorry
>
>>> However, looking at the code again, I realise it is possible to
>>> simplify it. Since its only the demod gates that cause a problem, we
>>> only /actually/ need to lock the get_frontend() and set_frontend()
>>> calls.
>>
>> I don't understand why .get_frontend() causes problem, since it does not
>> access tuner at all. It only reads demod registers. The main problem is
>> (like schema in af9015.c shows) that there is two tuners on same I2C bus
>> using same address. And demod gate is only way to open access for desired
>> tuner only.
>
> AFAIR /some/ tuner code accesses the tuner hardware to read the exact
> tuned frequency back on a get_frontend(); was just being extra
> paranoid :)
>
>> You should block traffic based of tuner not demod. And I think those
>> callbacks which are needed for override are tuner driver callbacks. Consider
>> situation device goes it v4l-core calls same time both tuner .sleep() ==
>> problem.
>
> Hmm, yeah, you're right, let me have another look tomorrow.
>

Hi, must admit I misunderstood your diagram originally, I thought it
was the demods AND the tuners that had the same i2c addresses.

As you say though. its just the tuners, so adding the locking into the
gate ctrl as you suggested makes perfect sense. Attached is v3
implementing this; it seems to be working fine here.

[-- Attachment #2: af9015-fix-dual-tuner-v3.patch --]
[-- Type: text/x-patch, Size: 1659 bytes --]

diff --git a/drivers/media/dvb/dvb-usb/af9015.c b/drivers/media/dvb/dvb-usb/af9015.c
index 31c0a0e..d9f32fe 100644
--- a/drivers/media/dvb/dvb-usb/af9015.c
+++ b/drivers/media/dvb/dvb-usb/af9015.c
@@ -1083,6 +1083,24 @@ static int af9015_i2c_init(struct dvb_usb_device *d)
 	return ret;
 }
 
+static int af9015_lock_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
+{
+	int result;
+	struct dvb_usb_adapter *adap = fe->dvb->priv;
+	struct af9015_state *state = adap->dev->priv;
+
+	if (enable)
+		if (mutex_lock_interruptible(&adap->dev->usb_mutex))
+			return -EAGAIN;
+
+	result = state->i2c_gate_ctrl[adap->id](fe, enable);
+	
+	if (!enable)
+		mutex_unlock(&adap->dev->usb_mutex);
+	
+	return result;
+}
+
 static int af9015_af9013_frontend_attach(struct dvb_usb_adapter *adap)
 {
 	int ret;
@@ -1116,6 +1134,11 @@ static int af9015_af9013_frontend_attach(struct dvb_usb_adapter *adap)
 	/* attach demodulator */
 	adap->fe = dvb_attach(af9013_attach, &af9015_af9013_config[adap->id],
 		i2c_adap);
+	
+	if (adap->fe && adap->fe->ops.i2c_gate_ctrl) {
+		state->i2c_gate_ctrl[adap->id] = adap->fe->ops.i2c_gate_ctrl;
+		adap->fe->ops.i2c_gate_ctrl = af9015_lock_i2c_gate_ctrl;
+	}
 
 	return adap->fe == NULL ? -ENODEV : 0;
 }
diff --git a/drivers/media/dvb/dvb-usb/af9015.h b/drivers/media/dvb/dvb-usb/af9015.h
index f20cfa6..094b1e3 100644
--- a/drivers/media/dvb/dvb-usb/af9015.h
+++ b/drivers/media/dvb/dvb-usb/af9015.h
@@ -102,6 +102,7 @@ struct af9015_state {
 	struct i2c_adapter i2c_adap; /* I2C adapter for 2nd FE */
 	u8 rc_repeat;
 	u32 rc_keycode;
+	int (*i2c_gate_ctrl[2])(struct dvb_frontend *fe, int enable);
 };
 
 struct af9015_config {

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

On 5 March 2011 01:43, adq <adq@lidskialf.net> wrote:
> On 4 March 2011 23:11, Andrew de Quincey <adq_dvb@lidskialf.net> wrote:
>> On 4 March 2011 22:59, Antti Palosaari <crope@iki.fi> wrote:
>>> On 03/05/2011 12:44 AM, Andrew de Quincey wrote:
>>>>>>
>>>>>> Adding a "bus lock" to af9015_i2c_xfer() will not work as demod/tuner
>>>>>> accesses will take multiple i2c transactions.
>>>>>>
>>>>>> Therefore, the following patch overrides the dvb_frontend_ops
>>>>>> functions to add a per-device lock around them: only one frontend can
>>>>>> now use the i2c bus at a time. Testing with the scripts above shows
>>>>>> this has eliminated the errors.
>>>>>
>>>>> This have annoyed me too, but since it does not broken functionality much
>>>>> I
>>>>> haven't put much effort for fixing it. I like that fix since it is in
>>>>> AF9015
>>>>> driver where it logically belongs to. But it looks still rather complex.
>>>>> I
>>>>> see you have also considered "bus lock" to af9015_i2c_xfer() which could
>>>>> be
>>>>> much smaller in code size (that's I have tried to implement long time
>>>>> back).
>>>>>
>>>>> I would like to ask if it possible to check I2C gate open / close inside
>>>>> af9015_i2c_xfer() and lock according that? Something like:
>>>>
>>>> Hmm, I did think about that, but I felt overriding the functions was
>>>> just cleaner: I felt it was more obvious what it was doing. Doing
>>>> exactly this sort of tweaking was one of the main reasons we added
>>>> that function overriding feature.
>>>>
>>>> I don't like the idea of returning "error locked by FE" since that'll
>>>> mean the tuning will randomly fail sometimes in a way visible to
>>>> userspace (unless we change the core dvb_frontend code), which was one
>>>> of the things I was trying to avoid. Unless, of course, I've
>>>> misunderstood your proposal.
>>>
>>> Not returning error, but waiting in lock like that:
>>> if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
>>>  return -EAGAIN;
>>
>> Ah k, sorry
>>
>>>> However, looking at the code again, I realise it is possible to
>>>> simplify it. Since its only the demod gates that cause a problem, we
>>>> only /actually/ need to lock the get_frontend() and set_frontend()
>>>> calls.
>>>
>>> I don't understand why .get_frontend() causes problem, since it does not
>>> access tuner at all. It only reads demod registers. The main problem is
>>> (like schema in af9015.c shows) that there is two tuners on same I2C bus
>>> using same address. And demod gate is only way to open access for desired
>>> tuner only.
>>
>> AFAIR /some/ tuner code accesses the tuner hardware to read the exact
>> tuned frequency back on a get_frontend(); was just being extra
>> paranoid :)
>>
>>> You should block traffic based of tuner not demod. And I think those
>>> callbacks which are needed for override are tuner driver callbacks. Consider
>>> situation device goes it v4l-core calls same time both tuner .sleep() ==
>>> problem.
>>
>> Hmm, yeah, you're right, let me have another look tomorrow.
>>
>
> Hi, must admit I misunderstood your diagram originally, I thought it
> was the demods AND the tuners that had the same i2c addresses.
>
> As you say though. its just the tuners, so adding the locking into the
> gate ctrl as you suggested makes perfect sense. Attached is v3
> implementing this; it seems to be working fine here.
>

Unfortunately even with this fix, I'm still seeing the problem I was
trying to fix to begin with.

Although I no longer get any i2c errors (or *any* reported errors),
after a bit, one of the frontends just.. stops working. All attempts
to tune it fail. I can even unload and reload the driver module, and
its stuck in the same state, indicating its a problem with the
hardware. :(

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Antti Palosaari]

Switching channels for long time seems to hang device (no errors seen 
but it does not lock anymore), I don't know why. It is not very easy to 
reproduce. For me it will take generally few days just tune from channel 
to channel in loop.

Antti

On 03/05/2011 10:56 AM, Juan Jesús García de Soria Lucena wrote:
> Hi, Andrew.
>
> This is what happens to me with both the KWorld dual tuner (when using only
> one tuner) and the Avermedia Volar Black (single tuner), both based on
> AF9015.
>
> I also got corrupted streams with the KWorld when capturing via both tuners
> (the video our the audio would show artifacts in mythtv each several
> seconds).
>
> As far as the loss of tuning ability goes, I think it's a problem related to
> tuning itself, since it wouldn't happen when you just left a channel tuned
> and streaming in a simple client, but would trigger after a random time when
> you left mythtv scanning the channels for EIT data.
>
> I don't think it's a problem with a specific HW implementation, since I got
> it with both AF9015-based cards. It could be either a chipset quirk our a
> bug in the driver.
>
> My informal and quick tests with Windows Media Center and these cards did
> not reproduce the problem, when trying to change channels as quickly as
> possible, admittedly for not so long a time.
>
> Best regards,
>     Juan Jesus.
> El 05/03/2011 02:53, "adq"<adq@lidskialf.net>  escribió:
>> On 5 March 2011 01:43, adq<adq@lidskialf.net>  wrote:
>>> On 4 March 2011 23:11, Andrew de Quincey<adq_dvb@lidskialf.net>  wrote:
>>>> On 4 March 2011 22:59, Antti Palosaari<crope@iki.fi>  wrote:
>>>>> On 03/05/2011 12:44 AM, Andrew de Quincey wrote:
>>>>>>>>
>>>>>>>> Adding a "bus lock" to af9015_i2c_xfer() will not work as
> demod/tuner
>>>>>>>> accesses will take multiple i2c transactions.
>>>>>>>>
>>>>>>>> Therefore, the following patch overrides the dvb_frontend_ops
>>>>>>>> functions to add a per-device lock around them: only one frontend
> can
>>>>>>>> now use the i2c bus at a time. Testing with the scripts above shows
>>>>>>>> this has eliminated the errors.
>>>>>>>
>>>>>>> This have annoyed me too, but since it does not broken functionality
> much
>>>>>>> I
>>>>>>> haven't put much effort for fixing it. I like that fix since it is in
>>>>>>> AF9015
>>>>>>> driver where it logically belongs to. But it looks still rather
> complex.
>>>>>>> I
>>>>>>> see you have also considered "bus lock" to af9015_i2c_xfer() which
> could
>>>>>>> be
>>>>>>> much smaller in code size (that's I have tried to implement long time
>>>>>>> back).
>>>>>>>
>>>>>>> I would like to ask if it possible to check I2C gate open / close
> inside
>>>>>>> af9015_i2c_xfer() and lock according that? Something like:
>>>>>>
>>>>>> Hmm, I did think about that, but I felt overriding the functions was
>>>>>> just cleaner: I felt it was more obvious what it was doing. Doing
>>>>>> exactly this sort of tweaking was one of the main reasons we added
>>>>>> that function overriding feature.
>>>>>>
>>>>>> I don't like the idea of returning "error locked by FE" since that'll
>>>>>> mean the tuning will randomly fail sometimes in a way visible to
>>>>>> userspace (unless we change the core dvb_frontend code), which was one
>>>>>> of the things I was trying to avoid. Unless, of course, I've
>>>>>> misunderstood your proposal.
>>>>>
>>>>> Not returning error, but waiting in lock like that:
>>>>> if (mutex_lock_interruptible(&d->i2c_mutex)<  0)
>>>>>   return -EAGAIN;
>>>>
>>>> Ah k, sorry
>>>>
>>>>>> However, looking at the code again, I realise it is possible to
>>>>>> simplify it. Since its only the demod gates that cause a problem, we
>>>>>> only /actually/ need to lock the get_frontend() and set_frontend()
>>>>>> calls.
>>>>>
>>>>> I don't understand why .get_frontend() causes problem, since it does
> not
>>>>> access tuner at all. It only reads demod registers. The main problem is
>>>>> (like schema in af9015.c shows) that there is two tuners on same I2C
> bus
>>>>> using same address. And demod gate is only way to open access for
> desired
>>>>> tuner only.
>>>>
>>>> AFAIR /some/ tuner code accesses the tuner hardware to read the exact
>>>> tuned frequency back on a get_frontend(); was just being extra
>>>> paranoid :)
>>>>
>>>>> You should block traffic based of tuner not demod. And I think those
>>>>> callbacks which are needed for override are tuner driver callbacks.
> Consider
>>>>> situation device goes it v4l-core calls same time both tuner .sleep()
> ==
>>>>> problem.
>>>>
>>>> Hmm, yeah, you're right, let me have another look tomorrow.
>>>>
>>>
>>> Hi, must admit I misunderstood your diagram originally, I thought it
>>> was the demods AND the tuners that had the same i2c addresses.
>>>
>>> As you say though. its just the tuners, so adding the locking into the
>>> gate ctrl as you suggested makes perfect sense. Attached is v3
>>> implementing this; it seems to be working fine here.
>>>
>>
>> Unfortunately even with this fix, I'm still seeing the problem I was
>> trying to fix to begin with.
>>
>> Although I no longer get any i2c errors (or *any* reported errors),
>> after a bit, one of the frontends just.. stops working. All attempts
>> to tune it fail. I can even unload and reload the driver module, and
>> its stuck in the same state, indicating its a problem with the
>> hardware. :(
>> --
>> 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
>


-- 
http://palosaari.fi/

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Andrew de Quincey]

Perhaps it depends on the characteristics of the transmissions being
received? I'm in the UK.

2011/3/5 Antti Palosaari <crope@iki.fi>:
> Switching channels for long time seems to hang device (no errors seen but it
> does not lock anymore), I don't know why. It is not very easy to reproduce.
> For me it will take generally few days just tune from channel to channel in
> loop.
>
> Antti
>
> On 03/05/2011 10:56 AM, Juan Jesús García de Soria Lucena wrote:
>>
>> Hi, Andrew.
>>
>> This is what happens to me with both the KWorld dual tuner (when using
>> only
>> one tuner) and the Avermedia Volar Black (single tuner), both based on
>> AF9015.
>>
>> I also got corrupted streams with the KWorld when capturing via both
>> tuners
>> (the video our the audio would show artifacts in mythtv each several
>> seconds).
>>
>> As far as the loss of tuning ability goes, I think it's a problem related
>> to
>> tuning itself, since it wouldn't happen when you just left a channel tuned
>> and streaming in a simple client, but would trigger after a random time
>> when
>> you left mythtv scanning the channels for EIT data.
>>
>> I don't think it's a problem with a specific HW implementation, since I
>> got
>> it with both AF9015-based cards. It could be either a chipset quirk our a
>> bug in the driver.
>>
>> My informal and quick tests with Windows Media Center and these cards did
>> not reproduce the problem, when trying to change channels as quickly as
>> possible, admittedly for not so long a time.
>>
>> Best regards,
>>    Juan Jesus.
>> El 05/03/2011 02:53, "adq"<adq@lidskialf.net>  escribió:
>>>
>>> On 5 March 2011 01:43, adq<adq@lidskialf.net>  wrote:
>>>>
>>>> On 4 March 2011 23:11, Andrew de Quincey<adq_dvb@lidskialf.net>  wrote:
>>>>>
>>>>> On 4 March 2011 22:59, Antti Palosaari<crope@iki.fi>  wrote:
>>>>>>
>>>>>> On 03/05/2011 12:44 AM, Andrew de Quincey wrote:
>>>>>>>>>
>>>>>>>>> Adding a "bus lock" to af9015_i2c_xfer() will not work as
>>
>> demod/tuner
>>>>>>>>>
>>>>>>>>> accesses will take multiple i2c transactions.
>>>>>>>>>
>>>>>>>>> Therefore, the following patch overrides the dvb_frontend_ops
>>>>>>>>> functions to add a per-device lock around them: only one frontend
>>
>> can
>>>>>>>>>
>>>>>>>>> now use the i2c bus at a time. Testing with the scripts above shows
>>>>>>>>> this has eliminated the errors.
>>>>>>>>
>>>>>>>> This have annoyed me too, but since it does not broken functionality
>>
>> much
>>>>>>>>
>>>>>>>> I
>>>>>>>> haven't put much effort for fixing it. I like that fix since it is
>>>>>>>> in
>>>>>>>> AF9015
>>>>>>>> driver where it logically belongs to. But it looks still rather
>>
>> complex.
>>>>>>>>
>>>>>>>> I
>>>>>>>> see you have also considered "bus lock" to af9015_i2c_xfer() which
>>
>> could
>>>>>>>>
>>>>>>>> be
>>>>>>>> much smaller in code size (that's I have tried to implement long
>>>>>>>> time
>>>>>>>> back).
>>>>>>>>
>>>>>>>> I would like to ask if it possible to check I2C gate open / close
>>
>> inside
>>>>>>>>
>>>>>>>> af9015_i2c_xfer() and lock according that? Something like:
>>>>>>>
>>>>>>> Hmm, I did think about that, but I felt overriding the functions was
>>>>>>> just cleaner: I felt it was more obvious what it was doing. Doing
>>>>>>> exactly this sort of tweaking was one of the main reasons we added
>>>>>>> that function overriding feature.
>>>>>>>
>>>>>>> I don't like the idea of returning "error locked by FE" since that'll
>>>>>>> mean the tuning will randomly fail sometimes in a way visible to
>>>>>>> userspace (unless we change the core dvb_frontend code), which was
>>>>>>> one
>>>>>>> of the things I was trying to avoid. Unless, of course, I've
>>>>>>> misunderstood your proposal.
>>>>>>
>>>>>> Not returning error, but waiting in lock like that:
>>>>>> if (mutex_lock_interruptible(&d->i2c_mutex)<  0)
>>>>>>  return -EAGAIN;
>>>>>
>>>>> Ah k, sorry
>>>>>
>>>>>>> However, looking at the code again, I realise it is possible to
>>>>>>> simplify it. Since its only the demod gates that cause a problem, we
>>>>>>> only /actually/ need to lock the get_frontend() and set_frontend()
>>>>>>> calls.
>>>>>>
>>>>>> I don't understand why .get_frontend() causes problem, since it does
>>
>> not
>>>>>>
>>>>>> access tuner at all. It only reads demod registers. The main problem
>>>>>> is
>>>>>> (like schema in af9015.c shows) that there is two tuners on same I2C
>>
>> bus
>>>>>>
>>>>>> using same address. And demod gate is only way to open access for
>>
>> desired
>>>>>>
>>>>>> tuner only.
>>>>>
>>>>> AFAIR /some/ tuner code accesses the tuner hardware to read the exact
>>>>> tuned frequency back on a get_frontend(); was just being extra
>>>>> paranoid :)
>>>>>
>>>>>> You should block traffic based of tuner not demod. And I think those
>>>>>> callbacks which are needed for override are tuner driver callbacks.
>>
>> Consider
>>>>>>
>>>>>> situation device goes it v4l-core calls same time both tuner .sleep()
>>
>> ==
>>>>>>
>>>>>> problem.
>>>>>
>>>>> Hmm, yeah, you're right, let me have another look tomorrow.
>>>>>
>>>>
>>>> Hi, must admit I misunderstood your diagram originally, I thought it
>>>> was the demods AND the tuners that had the same i2c addresses.
>>>>
>>>> As you say though. its just the tuners, so adding the locking into the
>>>> gate ctrl as you suggested makes perfect sense. Attached is v3
>>>> implementing this; it seems to be working fine here.
>>>>
>>>
>>> Unfortunately even with this fix, I'm still seeing the problem I was
>>> trying to fix to begin with.
>>>
>>> Although I no longer get any i2c errors (or *any* reported errors),
>>> after a bit, one of the frontends just.. stops working. All attempts
>>> to tune it fail. I can even unload and reload the driver module, and
>>> its stuck in the same state, indicating its a problem with the
>>> hardware. :(
>>> --
>>> 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
>>
>
>
> --
> http://palosaari.fi/
>

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/3/5 Juan Jesús García de Soria Lucena <skandalfo@gmail.com>:
> Hi, Andrew.
>
> This is what happens to me with both the KWorld dual tuner (when using only
> one tuner) and the Avermedia Volar Black (single tuner), both based on
> AF9015.
>
> I also got corrupted streams with the KWorld when capturing via both tuners
> (the video our the audio would show artifacts in mythtv each several
> seconds).
>
> As far as the loss of tuning ability goes, I think it's a problem related to
> tuning itself, since it wouldn't happen when you just left a channel tuned
> and streaming in a simple client, but would trigger after a random time when
> you left mythtv scanning the channels for EIT data.
>
> I don't think it's a problem with a specific HW implementation, since I got
> it with both AF9015-based cards. It could be either a chipset quirk our a
> bug in the driver.
>
> My informal and quick tests with Windows Media Center and these cards did
> not reproduce the problem, when trying to change channels as quickly as
> possible, admittedly for not so long a time.

Correct. I have two af9015 cards from different manufacturers as well,
and they both exhibit the same problem.

However, on a hunch last night, I went back to my original (-v1) patch
with the total i2c bus lock and left it running with my tuning scripts
for 10 hours. Both tuners are still working fine. That isn't
conclusive, but it is encouraging.

I'm just swapping back to a completely unpatched state to see how long
it takes to break and to check if its easily reproducible (on my live
system, it usually does it within a few hours of normal usage).

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/3/5 adq <adq@lidskialf.net>:
> 2011/3/5 Juan Jesús García de Soria Lucena <skandalfo@gmail.com>:
>> Hi, Andrew.
>>
>> This is what happens to me with both the KWorld dual tuner (when using only
>> one tuner) and the Avermedia Volar Black (single tuner), both based on
>> AF9015.
>>
>> I also got corrupted streams with the KWorld when capturing via both tuners
>> (the video our the audio would show artifacts in mythtv each several
>> seconds).
>>
>> As far as the loss of tuning ability goes, I think it's a problem related to
>> tuning itself, since it wouldn't happen when you just left a channel tuned
>> and streaming in a simple client, but would trigger after a random time when
>> you left mythtv scanning the channels for EIT data.
>>
>> I don't think it's a problem with a specific HW implementation, since I got
>> it with both AF9015-based cards. It could be either a chipset quirk our a
>> bug in the driver.
>>
>> My informal and quick tests with Windows Media Center and these cards did
>> not reproduce the problem, when trying to change channels as quickly as
>> possible, admittedly for not so long a time.
>
> Correct. I have two af9015 cards from different manufacturers as well,
> and they both exhibit the same problem.
>
> However, on a hunch last night, I went back to my original (-v1) patch
> with the total i2c bus lock and left it running with my tuning scripts
> for 10 hours. Both tuners are still working fine. That isn't
> conclusive, but it is encouraging.
>
> I'm just swapping back to a completely unpatched state to see how long
> it takes to break and to check if its easily reproducible (on my live
> system, it usually does it within a few hours of normal usage).
>

Hi, right, I can reproduce it when completely unpatched, but it takes
a while. I left HTS "tvheadend" running at the same time as "dvbsnoop"
monitoring each frontend's status (so I had lots of i2c traffic going
on), and it happened sometime overnight. I turned on all the idle
scanning and frontend monitoring features tvheadend has.

Now trying running the same with the -v1 patch.

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/3/6 adq <adq@lidskialf.net>:
> 2011/3/5 adq <adq@lidskialf.net>:
>> 2011/3/5 Juan Jesús García de Soria Lucena <skandalfo@gmail.com>:
>>> Hi, Andrew.
>>>
>>> This is what happens to me with both the KWorld dual tuner (when using only
>>> one tuner) and the Avermedia Volar Black (single tuner), both based on
>>> AF9015.
>>>
>>> I also got corrupted streams with the KWorld when capturing via both tuners
>>> (the video our the audio would show artifacts in mythtv each several
>>> seconds).
>>>
>>> As far as the loss of tuning ability goes, I think it's a problem related to
>>> tuning itself, since it wouldn't happen when you just left a channel tuned
>>> and streaming in a simple client, but would trigger after a random time when
>>> you left mythtv scanning the channels for EIT data.
>>>
>>> I don't think it's a problem with a specific HW implementation, since I got
>>> it with both AF9015-based cards. It could be either a chipset quirk our a
>>> bug in the driver.
>>>
>>> My informal and quick tests with Windows Media Center and these cards did
>>> not reproduce the problem, when trying to change channels as quickly as
>>> possible, admittedly for not so long a time.
>>
>> Correct. I have two af9015 cards from different manufacturers as well,
>> and they both exhibit the same problem.
>>
>> However, on a hunch last night, I went back to my original (-v1) patch
>> with the total i2c bus lock and left it running with my tuning scripts
>> for 10 hours. Both tuners are still working fine. That isn't
>> conclusive, but it is encouraging.
>>
>> I'm just swapping back to a completely unpatched state to see how long
>> it takes to break and to check if its easily reproducible (on my live
>> system, it usually does it within a few hours of normal usage).
>>
>
> Hi, right, I can reproduce it when completely unpatched, but it takes
> a while. I left HTS "tvheadend" running at the same time as "dvbsnoop"
> monitoring each frontend's status (so I had lots of i2c traffic going
> on), and it happened sometime overnight. I turned on all the idle
> scanning and frontend monitoring features tvheadend has.
>
> Now trying running the same with the -v1 patch.

Another issue I've noticed just now: The UCBLOCKS measure isn't reset:
it seems to be an accumulative counter, which isn't correct from the
DVB API (if I remember correctly).

This explains why tvheadend's "quallity" measure gradually tends to 0,
since it is assuming UCBLOCKS is non-accumulative.

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Antti Palosaari]

On 03/06/2011 02:24 PM, adq wrote:
> Another issue I've noticed just now: The UCBLOCKS measure isn't reset:
> it seems to be an accumulative counter, which isn't correct from the
> DVB API (if I remember correctly).
>
> This explains why tvheadend's "quallity" measure gradually tends to 0,
> since it is assuming UCBLOCKS is non-accumulative.

2.2.7 FE READ UNCORRECTED BLOCKS DESCRIPTION
This ioctl call returns the number of uncorrected blocks detected by the 
device driver during its lifetime. For meaningful measurements, the 
incrementin block count during a specific time interval should be 
calculated. For this command, read-only access to the device is sufficient.
Note that the counter will wrap to zero after its maximum count has been
reached.


Antti

-- 
http://palosaari.fi/

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/3/6 Antti Palosaari <crope@iki.fi>:
> On 03/06/2011 02:24 PM, adq wrote:
>>
>> Another issue I've noticed just now: The UCBLOCKS measure isn't reset:
>> it seems to be an accumulative counter, which isn't correct from the
>> DVB API (if I remember correctly).
>>
>> This explains why tvheadend's "quallity" measure gradually tends to 0,
>> since it is assuming UCBLOCKS is non-accumulative.
>
> 2.2.7 FE READ UNCORRECTED BLOCKS DESCRIPTION
> This ioctl call returns the number of uncorrected blocks detected by the
> device driver during its lifetime. For meaningful measurements, the
> incrementin block count during a specific time interval should be
> calculated. For this command, read-only access to the device is sufficient.
> Note that the counter will wrap to zero after its maximum count has been
> reached.

(this time to the list as well)

Hah! Sorry, its been a while :)

In that case, I need to look at the tvheadend source.

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/3/6 adq <adq@lidskialf.net>:
> 2011/3/6 Antti Palosaari <crope@iki.fi>:
>> On 03/06/2011 02:24 PM, adq wrote:
>>>
>>> Another issue I've noticed just now: The UCBLOCKS measure isn't reset:
>>> it seems to be an accumulative counter, which isn't correct from the
>>> DVB API (if I remember correctly).
>>>
>>> This explains why tvheadend's "quallity" measure gradually tends to 0,
>>> since it is assuming UCBLOCKS is non-accumulative.
>>
>> 2.2.7 FE READ UNCORRECTED BLOCKS DESCRIPTION
>> This ioctl call returns the number of uncorrected blocks detected by the
>> device driver during its lifetime. For meaningful measurements, the
>> incrementin block count during a specific time interval should be
>> calculated. For this command, read-only access to the device is sufficient.
>> Note that the counter will wrap to zero after its maximum count has been
>> reached.
>
> (this time to the list as well)
>
> Hah! Sorry, its been a while :)
>
> In that case, I need to look at the tvheadend source.

Hmm, lots of the frontend drivers have this incorrect then.

e.g. tda10021.c resets the UCBLOCKS counter on every read.

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/3/6 adq <adq@lidskialf.net>:
> 2011/3/5 adq <adq@lidskialf.net>:
>> 2011/3/5 Juan Jesús García de Soria Lucena <skandalfo@gmail.com>:
>>> Hi, Andrew.
>>>
>>> This is what happens to me with both the KWorld dual tuner (when using only
>>> one tuner) and the Avermedia Volar Black (single tuner), both based on
>>> AF9015.
>>>
>>> I also got corrupted streams with the KWorld when capturing via both tuners
>>> (the video our the audio would show artifacts in mythtv each several
>>> seconds).
>>>
>>> As far as the loss of tuning ability goes, I think it's a problem related to
>>> tuning itself, since it wouldn't happen when you just left a channel tuned
>>> and streaming in a simple client, but would trigger after a random time when
>>> you left mythtv scanning the channels for EIT data.
>>>
>>> I don't think it's a problem with a specific HW implementation, since I got
>>> it with both AF9015-based cards. It could be either a chipset quirk our a
>>> bug in the driver.
>>>
>>> My informal and quick tests with Windows Media Center and these cards did
>>> not reproduce the problem, when trying to change channels as quickly as
>>> possible, admittedly for not so long a time.
>>
>> Correct. I have two af9015 cards from different manufacturers as well,
>> and they both exhibit the same problem.
>>
>> However, on a hunch last night, I went back to my original (-v1) patch
>> with the total i2c bus lock and left it running with my tuning scripts
>> for 10 hours. Both tuners are still working fine. That isn't
>> conclusive, but it is encouraging.
>>
>> I'm just swapping back to a completely unpatched state to see how long
>> it takes to break and to check if its easily reproducible (on my live
>> system, it usually does it within a few hours of normal usage).
>>
>
> Hi, right, I can reproduce it when completely unpatched, but it takes
> a while. I left HTS "tvheadend" running at the same time as "dvbsnoop"
> monitoring each frontend's status (so I had lots of i2c traffic going
> on), and it happened sometime overnight. I turned on all the idle
> scanning and frontend monitoring features tvheadend has.
>
> Now trying running the same with the -v1 patch.
>

Hi, its been running for well over 24 hours now, and its still tuning fine.

I'm obviously I'm going to leave it testing for more days, but I'm
daring it to suddenly break as soon as I send this mail :)

As you will recall, v3 implemented a lock around the i2cgate so that
only one frontend could open it and therefore only one could access a
tuner at a time. Since this didn't fix the issue, it implies that if
the gate is open *any* other i2c access (e.g. just reading the
ucblocks) can somehow "crash" the tuner in such a way that it needs a
hardware reset. This means you'd need a complete lock around
*anything*  which can cause i2c traffic as v1 implements (assuming v1
does fix it that is).

Anyway, I'm keeping it running for the moment. It'd be good if someone
else who experiences this problem could try out v1 too though.

Oh, I see these every now and then:
af9015: recv bulk message failed:-71
af9015: af9015_rc_query: failed:-1
dvb-usb: error -1 while querying for an remote control event.

They don't seem to cause a problem, but seems odd they should occur at
all. Disabling rc polling "fixes" it as docced elsewhere, but why do
they occur in the first place I wonder?

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/3/7 adq <adq@lidskialf.net>:
> 2011/3/6 adq <adq@lidskialf.net>:
>> 2011/3/5 adq <adq@lidskialf.net>:
>>> 2011/3/5 Juan Jesús García de Soria Lucena <skandalfo@gmail.com>:
>>>> Hi, Andrew.
>>>>
>>>> This is what happens to me with both the KWorld dual tuner (when using only
>>>> one tuner) and the Avermedia Volar Black (single tuner), both based on
>>>> AF9015.
>>>>
>>>> I also got corrupted streams with the KWorld when capturing via both tuners
>>>> (the video our the audio would show artifacts in mythtv each several
>>>> seconds).
>>>>
>>>> As far as the loss of tuning ability goes, I think it's a problem related to
>>>> tuning itself, since it wouldn't happen when you just left a channel tuned
>>>> and streaming in a simple client, but would trigger after a random time when
>>>> you left mythtv scanning the channels for EIT data.
>>>>
>>>> I don't think it's a problem with a specific HW implementation, since I got
>>>> it with both AF9015-based cards. It could be either a chipset quirk our a
>>>> bug in the driver.
>>>>
>>>> My informal and quick tests with Windows Media Center and these cards did
>>>> not reproduce the problem, when trying to change channels as quickly as
>>>> possible, admittedly for not so long a time.
>>>
>>> Correct. I have two af9015 cards from different manufacturers as well,
>>> and they both exhibit the same problem.
>>>
>>> However, on a hunch last night, I went back to my original (-v1) patch
>>> with the total i2c bus lock and left it running with my tuning scripts
>>> for 10 hours. Both tuners are still working fine. That isn't
>>> conclusive, but it is encouraging.
>>>
>>> I'm just swapping back to a completely unpatched state to see how long
>>> it takes to break and to check if its easily reproducible (on my live
>>> system, it usually does it within a few hours of normal usage).
>>>
>>
>> Hi, right, I can reproduce it when completely unpatched, but it takes
>> a while. I left HTS "tvheadend" running at the same time as "dvbsnoop"
>> monitoring each frontend's status (so I had lots of i2c traffic going
>> on), and it happened sometime overnight. I turned on all the idle
>> scanning and frontend monitoring features tvheadend has.
>>
>> Now trying running the same with the -v1 patch.
>>
>
> Hi, its been running for well over 24 hours now, and its still tuning fine.
>
> I'm obviously I'm going to leave it testing for more days, but I'm
> daring it to suddenly break as soon as I send this mail :)
>
> As you will recall, v3 implemented a lock around the i2cgate so that
> only one frontend could open it and therefore only one could access a
> tuner at a time. Since this didn't fix the issue, it implies that if
> the gate is open *any* other i2c access (e.g. just reading the
> ucblocks) can somehow "crash" the tuner in such a way that it needs a
> hardware reset. This means you'd need a complete lock around
> *anything*  which can cause i2c traffic as v1 implements (assuming v1
> does fix it that is).
>
> Anyway, I'm keeping it running for the moment. It'd be good if someone
> else who experiences this problem could try out v1 too though.
>
> Oh, I see these every now and then:
> af9015: recv bulk message failed:-71
> af9015: af9015_rc_query: failed:-1
> dvb-usb: error -1 while querying for an remote control event.
>
> They don't seem to cause a problem, but seems odd they should occur at
> all. Disabling rc polling "fixes" it as docced elsewhere, but why do
> they occur in the first place I wonder?
>

Ah well, so its definitely /not/ conflicting i2c writes that cause the
tuner problem as it has finally just died. The festats for a "crashed"
tuner are:
  Sig: 50933  SNR: 50  BER: 0  UBLK: 5370554  Stat: 0x01 [SIG ]
(no other error messages)

For the other tuner, it  is:
  Sig: 55703  SNR: 120  BER: 0  UBLK: 919  Stat: 0x1f [SIG CARR VIT SYNC LOCK ]

Note the /massive/ difference in ubclocks; the tuner that died always
had a massively larger UCBLOCKS count even when it was working fine.

Antii, I'll try out your GPIO suggestions today or tomorrow, and I'll
try and snag an i2c register dump to see if that sheds any light...

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Antti Palosaari]

On 03/08/2011 12:12 AM, adq wrote:
> Ah well, so its definitely /not/ conflicting i2c writes that cause the
> tuner problem as it has finally just died. The festats for a "crashed"
> tuner are:
>    Sig: 50933  SNR: 50  BER: 0  UBLK: 5370554  Stat: 0x01 [SIG ]
> (no other error messages)
>
> For the other tuner, it  is:
>    Sig: 55703  SNR: 120  BER: 0  UBLK: 919  Stat: 0x1f [SIG CARR VIT SYNC LOCK ]
>
> Note the /massive/ difference in ubclocks; the tuner that died always
> had a massively larger UCBLOCKS count even when it was working fine.
>
> Antii, I'll try out your GPIO suggestions today or tomorrow, and I'll
> try and snag an i2c register dump to see if that sheds any light...

Any new findings?

Antti

-- 
http://palosaari.fi/

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/3/18 Antti Palosaari <crope@iki.fi>:
> On 03/08/2011 12:12 AM, adq wrote:
>>
>> Ah well, so its definitely /not/ conflicting i2c writes that cause the
>> tuner problem as it has finally just died. The festats for a "crashed"
>> tuner are:
>>   Sig: 50933  SNR: 50  BER: 0  UBLK: 5370554  Stat: 0x01 [SIG ]
>> (no other error messages)
>>
>> For the other tuner, it  is:
>>   Sig: 55703  SNR: 120  BER: 0  UBLK: 919  Stat: 0x1f [SIG CARR VIT SYNC
>> LOCK ]
>>
>> Note the /massive/ difference in ubclocks; the tuner that died always
>> had a massively larger UCBLOCKS count even when it was working fine.
>>
>> Antii, I'll try out your GPIO suggestions today or tomorrow, and I'll
>> try and snag an i2c register dump to see if that sheds any light...
>
> Any new findings?

Sorry, not had time yet :(

Will try and do it sometime this week.

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/3/18 Antti Palosaari <crope@iki.fi>:
> On 03/08/2011 12:12 AM, adq wrote:
>>
>> Ah well, so its definitely /not/ conflicting i2c writes that cause the
>> tuner problem as it has finally just died. The festats for a "crashed"
>> tuner are:
>>   Sig: 50933  SNR: 50  BER: 0  UBLK: 5370554  Stat: 0x01 [SIG ]
>> (no other error messages)
>>
>> For the other tuner, it  is:
>>   Sig: 55703  SNR: 120  BER: 0  UBLK: 919  Stat: 0x1f [SIG CARR VIT SYNC
>> LOCK ]
>>
>> Note the /massive/ difference in ubclocks; the tuner that died always
>> had a massively larger UCBLOCKS count even when it was working fine.
>>
>> Antii, I'll try out your GPIO suggestions today or tomorrow, and I'll
>> try and snag an i2c register dump to see if that sheds any light...
>
> Any new findings?

Hi, just been trying it out, with no success. On my test machine, FE0
no longer tunes, but FE1 is still fine, so I've just been testing FE0.

I've tried your suggestions, mainly concentrating on the af9013's
GPIOs, but I also tried your power management suggestion.

Since I was just using FE0, I've just been setting all the GPIOs at
the start of af9013.c's set_frontend() implementation; I've tried
turning them all off, all on, on->mdelay->off, and also
off->mdelay->on. Nothing works.

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Antti Palosaari]

On 04/02/2011 04:24 AM, adq wrote:
> Hi, just been trying it out, with no success. On my test machine, FE0
> no longer tunes, but FE1 is still fine, so I've just been testing FE0.

You try to say other frontend / tuner is physically dead? Which one?

> I've tried your suggestions, mainly concentrating on the af9013's
> GPIOs, but I also tried your power management suggestion.
>
> Since I was just using FE0, I've just been setting all the GPIOs at
> the start of af9013.c's set_frontend() implementation; I've tried
> turning them all off, all on, on->mdelay->off, and also
> off->mdelay->on. Nothing works.

So GPIOs are blocked out.

I wonder if someone can ran similar many day tuning stress test using 
Windows drivers to see if that happen.

Antti

-- 
http://palosaari.fi/

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/4/2 Antti Palosaari <crope@iki.fi>:
> On 04/02/2011 04:24 AM, adq wrote:
>>
>> Hi, just been trying it out, with no success. On my test machine, FE0
>> no longer tunes, but FE1 is still fine, so I've just been testing FE0.
>
> You try to say other frontend / tuner is physically dead? Which one?

No no - I can revive it by simply unplugging and replugging the
device, but I was avoiding doing that to see if we could either track
down something erroneous, or be able to reset it from software.

It'd be /really/ handy if they'd connected that reset tuner GPIO :(
There isn't a way to completely reset the device from software I take
it? Or any other GPIOs hanging about I could test with?

I have an MXL5005R tuner apparently - id 30 - BTW.

>> I've tried your suggestions, mainly concentrating on the af9013's
>> GPIOs, but I also tried your power management suggestion.
>>
>> Since I was just using FE0, I've just been setting all the GPIOs at
>> the start of af9013.c's set_frontend() implementation; I've tried
>> turning them all off, all on, on->mdelay->off, and also
>> off->mdelay->on. Nothing works.
>
> So GPIOs are blocked out.
>
> I wonder if someone can ran similar many day tuning stress test using
> Windows drivers to see if that happen.

Might be hard to script under windows I suppose...

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/4/2 adq <adq@lidskialf.net>:
> 2011/4/2 Antti Palosaari <crope@iki.fi>:
>> On 04/02/2011 04:24 AM, adq wrote:
>>>
>>> Hi, just been trying it out, with no success. On my test machine, FE0
>>> no longer tunes, but FE1 is still fine, so I've just been testing FE0.
>>
>> You try to say other frontend / tuner is physically dead? Which one?
>
> No no - I can revive it by simply unplugging and replugging the
> device, but I was avoiding doing that to see if we could either track
> down something erroneous, or be able to reset it from software.
>
> It'd be /really/ handy if they'd connected that reset tuner GPIO :(
> There isn't a way to completely reset the device from software I take
> it? Or any other GPIOs hanging about I could test with?
>
> I have an MXL5005R tuner apparently - id 30 - BTW.

Forgot to mention - its the tuner attached to the internal af9013
(fe0) that is having the problem. The one attached to the external one
(fe1) is still fine. I don't know if this is always the case though.

>>> I've tried your suggestions, mainly concentrating on the af9013's
>>> GPIOs, but I also tried your power management suggestion.
>>>
>>> Since I was just using FE0, I've just been setting all the GPIOs at
>>> the start of af9013.c's set_frontend() implementation; I've tried
>>> turning them all off, all on, on->mdelay->off, and also
>>> off->mdelay->on. Nothing works.
>>
>> So GPIOs are blocked out.
>>
>> I wonder if someone can ran similar many day tuning stress test using
>> Windows drivers to see if that happen.
>
> Might be hard to script under windows I suppose...
>

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Antti Palosaari]

On 04/02/2011 02:06 PM, adq wrote:
> 2011/4/2 Antti Palosaari<crope@iki.fi>:
>> On 04/02/2011 04:24 AM, adq wrote:
>>>
>>> Hi, just been trying it out, with no success. On my test machine, FE0
>>> no longer tunes, but FE1 is still fine, so I've just been testing FE0.
>>
>> You try to say other frontend / tuner is physically dead? Which one?
>
> No no - I can revive it by simply unplugging and replugging the
> device, but I was avoiding doing that to see if we could either track
> down something erroneous, or be able to reset it from software.
>
> It'd be /really/ handy if they'd connected that reset tuner GPIO :(
> There isn't a way to completely reset the device from software I take
> it? Or any other GPIOs hanging about I could test with?

There is few I know, USB command 0x13 boots AF9015 somehow, USB command 
0x5a reconnects it from USB bus. But most interesting one is demodulator 
reset register 0xe205, write 1 to that reg should reset it.

> I have an MXL5005R tuner apparently - id 30 - BTW.

I suspect it is demod which hangs since I have feeling it happens every 
tuner used.

>>> I've tried your suggestions, mainly concentrating on the af9013's
>>> GPIOs, but I also tried your power management suggestion.
>>>
>>> Since I was just using FE0, I've just been setting all the GPIOs at
>>> the start of af9013.c's set_frontend() implementation; I've tried
>>> turning them all off, all on, on->mdelay->off, and also
>>> off->mdelay->on. Nothing works.
>>
>> So GPIOs are blocked out.
>>
>> I wonder if someone can ran similar many day tuning stress test using
>> Windows drivers to see if that happen.
>
> Might be hard to script under windows I suppose...

I am not aware any good commandline BDA tuning app for Windows. Only one 
I know is ScanChannelsBDA.exe which is rather buggy - but works.

Antti
-- 
http://palosaari.fi/

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

2011/4/2 Antti Palosaari <crope@iki.fi>:
> On 04/02/2011 02:06 PM, adq wrote:
>>
>> 2011/4/2 Antti Palosaari<crope@iki.fi>:
>>>
>>> On 04/02/2011 04:24 AM, adq wrote:
>>>>
>>>> Hi, just been trying it out, with no success. On my test machine, FE0
>>>> no longer tunes, but FE1 is still fine, so I've just been testing FE0.
>>>
>>> You try to say other frontend / tuner is physically dead? Which one?
>>
>> No no - I can revive it by simply unplugging and replugging the
>> device, but I was avoiding doing that to see if we could either track
>> down something erroneous, or be able to reset it from software.
>>
>> It'd be /really/ handy if they'd connected that reset tuner GPIO :(
>> There isn't a way to completely reset the device from software I take
>> it? Or any other GPIOs hanging about I could test with?
>
> There is few I know, USB command 0x13 boots AF9015 somehow, USB command 0x5a
> reconnects it from USB bus. But most interesting one is demodulator reset
> register 0xe205, write 1 to that reg should reset it.

Just tried writing 1 to 0xe205 - no change.

Looks like USB commands 0x13/0x5a will be done as part of a normal
module reload? (In which case it doesn't fix it).

I can actually reboot the machine completely, and the problem stays!
Only physically unplugging the device sorts it.

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Antti Palosaari]

On 04/02/2011 04:45 PM, adq wrote:
> 2011/4/2 Antti Palosaari<crope@iki.fi>:
>> On 04/02/2011 02:06 PM, adq wrote:
>>>
>>> 2011/4/2 Antti Palosaari<crope@iki.fi>:
>>>>
>>>> On 04/02/2011 04:24 AM, adq wrote:
>>>>>
>>>>> Hi, just been trying it out, with no success. On my test machine, FE0
>>>>> no longer tunes, but FE1 is still fine, so I've just been testing FE0.
>>>>
>>>> You try to say other frontend / tuner is physically dead? Which one?
>>>
>>> No no - I can revive it by simply unplugging and replugging the
>>> device, but I was avoiding doing that to see if we could either track
>>> down something erroneous, or be able to reset it from software.
>>>
>>> It'd be /really/ handy if they'd connected that reset tuner GPIO :(
>>> There isn't a way to completely reset the device from software I take
>>> it? Or any other GPIOs hanging about I could test with?
>>
>> There is few I know, USB command 0x13 boots AF9015 somehow, USB command 0x5a
>> reconnects it from USB bus. But most interesting one is demodulator reset
>> register 0xe205, write 1 to that reg should reset it.
>
> Just tried writing 1 to 0xe205 - no change.

OK.

> Looks like USB commands 0x13/0x5a will be done as part of a normal
> module reload? (In which case it doesn't fix it).
>
> I can actually reboot the machine completely, and the problem stays!
> Only physically unplugging the device sorts it.

You are correct. I don't have any idea anymore. Only is wish to know if 
that happens in Windows too.

Antti

-- 
http://palosaari.fi/

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Antti Palosaari]

On 03/05/2011 03:51 AM, adq wrote:
>> As you say though. its just the tuners, so adding the locking into the
>> gate ctrl as you suggested makes perfect sense. Attached is v3
>> implementing this; it seems to be working fine here.
>>
>
> Unfortunately even with this fix, I'm still seeing the problem I was
> trying to fix to begin with.
>
> Although I no longer get any i2c errors (or *any* reported errors),
> after a bit, one of the frontends just.. stops working. All attempts
> to tune it fail. I can even unload and reload the driver module, and
> its stuck in the same state, indicating its a problem with the
> hardware. :(

How easily you can re-produce this? Does it work using your first patch?

Could try some changes for GPIOs, LEDs are not important but tuner 
GPIOs? Here is instructions how GPIOs should be done:

FE0 START
FE0 READ EEPROM
FE0 GPIO RESET and ENABLE FE1?
// set_gpio(0, 3);
// msleep(40)
// set_gpio(0, 7);
FE0 DOWNLOAD FW
FE0 enable POWER LED ???
// wr_reg(0xd734,  upper nibble 3)

FE1 set power management (0xd731)

FE0 tuner OFF
// set_gpio(3, 7);
FE0 LOCK LED OFF

FE1 tuner OFF
// set_gpio(0, 7);
FE1 LOCK LED OFF
*** DEVICE is NOW IN SLEEP ***

*** TUNE REQ for FE1 **
FE1 tuner ON
// set_gpio(0, b);
FE1 LOCK LED ON
*** now streaming **
FE1 tuner OFF
// set_gpio(0, 7);
FE1 LOCK LED OFF
*** DEVICE is NOW IN SLEEP ***

*** TUNE REQ for FE0 **
FE0 tuner ON
// set_gpio(3, b);
FE0 LOCK LED ON
*** now streaming **
FE0 tuner OFF
// set_gpio(3, 7);
FE0 LOCK LED OFF
*** DEVICE is NOW IN SLEEP ***

Also configure some power management for FE1, write register 0xd731 
value 0x47. Do that in af9013_init() before OFSM init (since it changes 
some bits in same register).

And this is list of used GPIOs, it is my latest understanding. I have 
ensured many of those by just testing.

AF9015 GPIO0 AF9013 reset
AF9015 GPIO1 NC (note: on MC44S803 device tuner reset)
AF9015 GPIO2 NC
AF9015 GPIO3 TUNER
AF9015 GPIO_LOCK1 LOCK LED
AF9015 GPIO_LOCK2 POWER LED (not sure, I don't have any device having 
power LED, but it looks like it could be)

AF9013 GPIO0 TUNER
AF9013 GPIO1 NC
AF9013 GPIO2 LOCK LED
AF9013 GPIO3 HW power down?
AF9013 GPIO_LOCK1 LOCK LED
AF9013 GPIO_LOCK2 NC

-- 
http://palosaari.fi/

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Antti Palosaari]

On 03/05/2011 03:43 AM, adq wrote:
> +static int af9015_lock_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
> +{
> +	int result;
> +	struct dvb_usb_adapter *adap = fe->dvb->priv;
> +	struct af9015_state *state = adap->dev->priv;
> +
> +	if (enable)
> +		if (mutex_lock_interruptible(&adap->dev->usb_mutex))
> +			return -EAGAIN;
> +
> +	result = state->i2c_gate_ctrl[adap->id](fe, enable);
> +	
> +	if (!enable)
> +		mutex_unlock(&adap->dev->usb_mutex);
> +	
> +	return result;
> +}

I think this will cause problems in case of tuner driver calls more than 
one time gate close or gate enable one after the other.


Antti

-- 
http://palosaari.fi/

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Mauro Carvalho Chehab]

Em 04-03-2011 20:11, Andrew de Quincey escreveu:
> On 4 March 2011 22:59, Antti Palosaari <crope@iki.fi> wrote:
>> On 03/05/2011 12:44 AM, Andrew de Quincey wrote:
>>>>>
>>>>> Adding a "bus lock" to af9015_i2c_xfer() will not work as demod/tuner
>>>>> accesses will take multiple i2c transactions.
>>>>>
>>>>> Therefore, the following patch overrides the dvb_frontend_ops
>>>>> functions to add a per-device lock around them: only one frontend can
>>>>> now use the i2c bus at a time. Testing with the scripts above shows
>>>>> this has eliminated the errors.
>>>>
>>>> This have annoyed me too, but since it does not broken functionality much
>>>> I
>>>> haven't put much effort for fixing it. I like that fix since it is in
>>>> AF9015
>>>> driver where it logically belongs to. But it looks still rather complex.
>>>> I
>>>> see you have also considered "bus lock" to af9015_i2c_xfer() which could
>>>> be
>>>> much smaller in code size (that's I have tried to implement long time
>>>> back).
>>>>
>>>> I would like to ask if it possible to check I2C gate open / close inside
>>>> af9015_i2c_xfer() and lock according that? Something like:
>>>
>>> Hmm, I did think about that, but I felt overriding the functions was
>>> just cleaner: I felt it was more obvious what it was doing. Doing
>>> exactly this sort of tweaking was one of the main reasons we added
>>> that function overriding feature.
>>>
>>> I don't like the idea of returning "error locked by FE" since that'll
>>> mean the tuning will randomly fail sometimes in a way visible to
>>> userspace (unless we change the core dvb_frontend code), which was one
>>> of the things I was trying to avoid. Unless, of course, I've
>>> misunderstood your proposal.
>>
>> Not returning error, but waiting in lock like that:
>> if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
>>  return -EAGAIN;
> 
> Ah k, sorry
> 
>>> However, looking at the code again, I realise it is possible to
>>> simplify it. Since its only the demod gates that cause a problem, we
>>> only /actually/ need to lock the get_frontend() and set_frontend()
>>> calls.
>>
>> I don't understand why .get_frontend() causes problem, since it does not
>> access tuner at all. It only reads demod registers. The main problem is
>> (like schema in af9015.c shows) that there is two tuners on same I2C bus
>> using same address. And demod gate is only way to open access for desired
>> tuner only.
> 
> AFAIR /some/ tuner code accesses the tuner hardware to read the exact
> tuned frequency back on a get_frontend(); was just being extra
> paranoid :)
> 
>> You should block traffic based of tuner not demod. And I think those
>> callbacks which are needed for override are tuner driver callbacks. Consider
>> situation device goes it v4l-core calls same time both tuner .sleep() ==
>> problem.
> 
> Hmm, yeah, you're right, let me have another look tomorrow.

Andrew, Antti,

I'm understanding that I should wait for another patch on this subject, right?
Please let me know when you have a patch ready for me to apply.

Btw, I think that the long term solution would be, instead, to provide some sort of
resource locking inside DVB (and V4L) core. I have here 3 devices not supported yet that
uses the same tuner (and the same demod - DRX-K) for both DVB-C and DVB-T. It would
be a way better to use some core-provided solution to prevent that both DVB-C and DVB-T
would be used at the same time on such devices, instead of cloning the same code
(or modified versions on it) on each driver that have such issues.

One solution could be to have a callback like:

enum dvb_fe_lock_type {
	ATV_DEMOD_LOCK,
	DTV_DEMOD_T_LOCK,
	DTV_DEMOD_S_LOCK,
	DTV_DEMOD_C_LOCK,
	TUNER_T_LOCK,
	TUNER_S_LOCK,
	TUNER_C_LOCK,
	TUNER_FM_LOCK,
	FM_DEMOD_LOCK,
};

/**
 * dvb_fe_lock - locks a frontend resource
 * @fe:		DVB frontend struct
 * @type:	type of resource to lock
 * @lock:	true indicates to lock the resource, false to unlock
 *
 * Returns true if the resource was locked, false otherwise.
 * routine may wait for a pending transaction to happen before returning.
 */
bool (*dvb_fe_lock)(struct dvb_frontend *fe, enum dvb_fe_lock_type type, bool lock);

What do you think?

Thanks,
Mauro.

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[adq]

On 22 March 2011 09:00, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> Em 04-03-2011 20:11, Andrew de Quincey escreveu:
>> On 4 March 2011 22:59, Antti Palosaari <crope@iki.fi> wrote:
>>> On 03/05/2011 12:44 AM, Andrew de Quincey wrote:
>>>>>>
>>>>>> Adding a "bus lock" to af9015_i2c_xfer() will not work as demod/tuner
>>>>>> accesses will take multiple i2c transactions.
>>>>>>
>>>>>> Therefore, the following patch overrides the dvb_frontend_ops
>>>>>> functions to add a per-device lock around them: only one frontend can
>>>>>> now use the i2c bus at a time. Testing with the scripts above shows
>>>>>> this has eliminated the errors.
>>>>>
>>>>> This have annoyed me too, but since it does not broken functionality much
>>>>> I
>>>>> haven't put much effort for fixing it. I like that fix since it is in
>>>>> AF9015
>>>>> driver where it logically belongs to. But it looks still rather complex.
>>>>> I
>>>>> see you have also considered "bus lock" to af9015_i2c_xfer() which could
>>>>> be
>>>>> much smaller in code size (that's I have tried to implement long time
>>>>> back).
>>>>>
>>>>> I would like to ask if it possible to check I2C gate open / close inside
>>>>> af9015_i2c_xfer() and lock according that? Something like:
>>>>
>>>> Hmm, I did think about that, but I felt overriding the functions was
>>>> just cleaner: I felt it was more obvious what it was doing. Doing
>>>> exactly this sort of tweaking was one of the main reasons we added
>>>> that function overriding feature.
>>>>
>>>> I don't like the idea of returning "error locked by FE" since that'll
>>>> mean the tuning will randomly fail sometimes in a way visible to
>>>> userspace (unless we change the core dvb_frontend code), which was one
>>>> of the things I was trying to avoid. Unless, of course, I've
>>>> misunderstood your proposal.
>>>
>>> Not returning error, but waiting in lock like that:
>>> if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
>>>  return -EAGAIN;
>>
>> Ah k, sorry
>>
>>>> However, looking at the code again, I realise it is possible to
>>>> simplify it. Since its only the demod gates that cause a problem, we
>>>> only /actually/ need to lock the get_frontend() and set_frontend()
>>>> calls.
>>>
>>> I don't understand why .get_frontend() causes problem, since it does not
>>> access tuner at all. It only reads demod registers. The main problem is
>>> (like schema in af9015.c shows) that there is two tuners on same I2C bus
>>> using same address. And demod gate is only way to open access for desired
>>> tuner only.
>>
>> AFAIR /some/ tuner code accesses the tuner hardware to read the exact
>> tuned frequency back on a get_frontend(); was just being extra
>> paranoid :)
>>
>>> You should block traffic based of tuner not demod. And I think those
>>> callbacks which are needed for override are tuner driver callbacks. Consider
>>> situation device goes it v4l-core calls same time both tuner .sleep() ==
>>> problem.
>>
>> Hmm, yeah, you're right, let me have another look tomorrow.
>
> Andrew, Antti,
>
> I'm understanding that I should wait for another patch on this subject, right?
> Please let me know when you have a patch ready for me to apply.
>
> Btw, I think that the long term solution would be, instead, to provide some sort of
> resource locking inside DVB (and V4L) core. I have here 3 devices not supported yet that
> uses the same tuner (and the same demod - DRX-K) for both DVB-C and DVB-T. It would
> be a way better to use some core-provided solution to prevent that both DVB-C and DVB-T
> would be used at the same time on such devices, instead of cloning the same code
> (or modified versions on it) on each driver that have such issues.
>
> One solution could be to have a callback like:
>
> enum dvb_fe_lock_type {
>        ATV_DEMOD_LOCK,
>        DTV_DEMOD_T_LOCK,
>        DTV_DEMOD_S_LOCK,
>        DTV_DEMOD_C_LOCK,
>        TUNER_T_LOCK,
>        TUNER_S_LOCK,
>        TUNER_C_LOCK,
>        TUNER_FM_LOCK,
>        FM_DEMOD_LOCK,
> };
>
> /**
>  * dvb_fe_lock - locks a frontend resource
>  * @fe:         DVB frontend struct
>  * @type:       type of resource to lock
>  * @lock:       true indicates to lock the resource, false to unlock
>  *
>  * Returns true if the resource was locked, false otherwise.
>  * routine may wait for a pending transaction to happen before returning.
>  */
> bool (*dvb_fe_lock)(struct dvb_frontend *fe, enum dvb_fe_lock_type type, bool lock);
>
> What do you think?

Have to think on it, but it does sound sensible to do it in the core
if there are other things like this.

Definitely hold off on the dual patch though, as it doesn't fix the
tuner lockup issue I'm seeing unfortunately.

Re: [patch] Fix AF9015 Dual tuner i2c write failures

[Mauro Carvalho Chehab]

Em 22-03-2011 15:26, adq escreveu:
> On 22 March 2011 09:00, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
>> Em 04-03-2011 20:11, Andrew de Quincey escreveu:
>>> On 4 March 2011 22:59, Antti Palosaari <crope@iki.fi> wrote:
>>>> On 03/05/2011 12:44 AM, Andrew de Quincey wrote:
>>>>>>>
>>>>>>> Adding a "bus lock" to af9015_i2c_xfer() will not work as demod/tuner
>>>>>>> accesses will take multiple i2c transactions.
>>>>>>>
>>>>>>> Therefore, the following patch overrides the dvb_frontend_ops
>>>>>>> functions to add a per-device lock around them: only one frontend can
>>>>>>> now use the i2c bus at a time. Testing with the scripts above shows
>>>>>>> this has eliminated the errors.
>>>>>>
>>>>>> This have annoyed me too, but since it does not broken functionality much
>>>>>> I
>>>>>> haven't put much effort for fixing it. I like that fix since it is in
>>>>>> AF9015
>>>>>> driver where it logically belongs to. But it looks still rather complex.
>>>>>> I
>>>>>> see you have also considered "bus lock" to af9015_i2c_xfer() which could
>>>>>> be
>>>>>> much smaller in code size (that's I have tried to implement long time
>>>>>> back).
>>>>>>
>>>>>> I would like to ask if it possible to check I2C gate open / close inside
>>>>>> af9015_i2c_xfer() and lock according that? Something like:
>>>>>
>>>>> Hmm, I did think about that, but I felt overriding the functions was
>>>>> just cleaner: I felt it was more obvious what it was doing. Doing
>>>>> exactly this sort of tweaking was one of the main reasons we added
>>>>> that function overriding feature.
>>>>>
>>>>> I don't like the idea of returning "error locked by FE" since that'll
>>>>> mean the tuning will randomly fail sometimes in a way visible to
>>>>> userspace (unless we change the core dvb_frontend code), which was one
>>>>> of the things I was trying to avoid. Unless, of course, I've
>>>>> misunderstood your proposal.
>>>>
>>>> Not returning error, but waiting in lock like that:
>>>> if (mutex_lock_interruptible(&d->i2c_mutex) < 0)
>>>>  return -EAGAIN;
>>>
>>> Ah k, sorry
>>>
>>>>> However, looking at the code again, I realise it is possible to
>>>>> simplify it. Since its only the demod gates that cause a problem, we
>>>>> only /actually/ need to lock the get_frontend() and set_frontend()
>>>>> calls.
>>>>
>>>> I don't understand why .get_frontend() causes problem, since it does not
>>>> access tuner at all. It only reads demod registers. The main problem is
>>>> (like schema in af9015.c shows) that there is two tuners on same I2C bus
>>>> using same address. And demod gate is only way to open access for desired
>>>> tuner only.
>>>
>>> AFAIR /some/ tuner code accesses the tuner hardware to read the exact
>>> tuned frequency back on a get_frontend(); was just being extra
>>> paranoid :)
>>>
>>>> You should block traffic based of tuner not demod. And I think those
>>>> callbacks which are needed for override are tuner driver callbacks. Consider
>>>> situation device goes it v4l-core calls same time both tuner .sleep() ==
>>>> problem.
>>>
>>> Hmm, yeah, you're right, let me have another look tomorrow.
>>
>> Andrew, Antti,
>>
>> I'm understanding that I should wait for another patch on this subject, right?
>> Please let me know when you have a patch ready for me to apply.
>>
>> Btw, I think that the long term solution would be, instead, to provide some sort of
>> resource locking inside DVB (and V4L) core. I have here 3 devices not supported yet that
>> uses the same tuner (and the same demod - DRX-K) for both DVB-C and DVB-T. It would
>> be a way better to use some core-provided solution to prevent that both DVB-C and DVB-T
>> would be used at the same time on such devices, instead of cloning the same code
>> (or modified versions on it) on each driver that have such issues.
>>
>> One solution could be to have a callback like:
>>
>> enum dvb_fe_lock_type {
>>        ATV_DEMOD_LOCK,
>>        DTV_DEMOD_T_LOCK,
>>        DTV_DEMOD_S_LOCK,
>>        DTV_DEMOD_C_LOCK,
>>        TUNER_T_LOCK,
>>        TUNER_S_LOCK,
>>        TUNER_C_LOCK,
>>        TUNER_FM_LOCK,
>>        FM_DEMOD_LOCK,
>> };
>>
>> /**
>>  * dvb_fe_lock - locks a frontend resource
>>  * @fe:         DVB frontend struct
>>  * @type:       type of resource to lock
>>  * @lock:       true indicates to lock the resource, false to unlock
>>  *
>>  * Returns true if the resource was locked, false otherwise.
>>  * routine may wait for a pending transaction to happen before returning.
>>  */
>> bool (*dvb_fe_lock)(struct dvb_frontend *fe, enum dvb_fe_lock_type type, bool lock);
>>
>> What do you think?
> 
> Have to think on it, but it does sound sensible to do it in the core
> if there are other things like this.

Yes, but the current way it is done right now (e. g. the "standard" i2c gate
control) is not fitting anymore. There are tons of cases where it is not covered,
and several different implementations to work around it. On complex devices like
cx231xx (that has 4 I2C buses, one of them with an internal switch inside the
bridge chip), locking it at I2C level is not enough.

So, it is better to provide a new way and replace the existing methods with the
new one were required.

> 
> Definitely hold off on the dual patch though, as it doesn't fix the
> tuner lockup issue I'm seeing unfortunately.

Ok, I'll mark those patches as Rejected at patchwork and I'll wait for a new
set.

Thanks!
Mauro