2021-09-26 20:52:31

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: [PATCH 0/3] Fix streaming on/off logic

As discussed on:
https://github.com/hselasky/webcamd/issues/16

the dib0700 had a regression on Kernel 2.6.39. Such regression didn't
affect most devices, in practice, as it seems to happen only under
certain circunstances.

Michael came up with a solution for the issue (already submitted to
the ML) but let's take the opportunity to do a cleanup, as the resulting
code was still touching both adapters when an stream off command
was issued to one adapter, turning on the other one.

After the change, each adapter is idependently controlled by
a separate bit, as can be shown when its debug message
is turned on (tested on a dual-adapter device: Hauppauge
WinTV Nova TD):

[608855.124780] adapter 1, streaming ON: 0f 10 12
[608868.189827] adapter 0, streaming ON: 0f 10 13
[608879.584330] adapter 1, streaming OFF: 0f 00 11
[608887.014772] adapter 0, streaming OFF: 0f 00 10

Mauro Carvalho Chehab (2):
media: dib0700: cleanup start/stop streaming logic
media: dib0700: Only touch one bit when start/stop an adapter

Michael Kuron (1):
media: dib0700: fix undefined behavior in tuner shutdown

drivers/media/usb/dvb-usb/dib0700_core.c | 28 +++++++++++-------------
1 file changed, 13 insertions(+), 15 deletions(-)

--
2.31.1



2021-09-26 20:52:34

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: [PATCH 2/3] media: dib0700: cleanup start/stop streaming logic

Having two different pathes to start/stop streaming, depending
weather the USB endpoints are 0x82/0x83 or not makes it more
prune to errors.

Unify the logic.

Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/usb/dvb-usb/dib0700_core.c | 26 +++++++++++++-----------
1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c
index 7ea8f68b0f45..d7c5836b9271 100644
--- a/drivers/media/usb/dvb-usb/dib0700_core.c
+++ b/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -583,7 +583,7 @@ int dib0700_download_firmware(struct usb_device *udev, const struct firmware *fw
int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
{
struct dib0700_state *st = adap->dev->priv;
- int ret;
+ int ret, adapt_nr;

if ((onoff != 0) && (st->fw_version >= 0x10201)) {
/* for firmware later than 1.20.1,
@@ -610,24 +610,26 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)

st->buf[3] = 0x00;

- deb_info("modifying (%d) streaming state for %d\n", onoff, adap->id);
-
st->channel_state &= ~0x3;
+
if ((adap->fe_adap[0].stream.props.endpoint != 2)
- && (adap->fe_adap[0].stream.props.endpoint != 3)) {
- deb_info("the endpoint number (%i) is not correct, use the adapter id instead", adap->fe_adap[0].stream.props.endpoint);
- if (onoff)
- st->channel_state |= 1 << (adap->id);
+ && (adap->fe_adap[0].stream.props.endpoint != 3)) {
+ deb_info("the endpoint number (%i) is not correct, use the adapter id instead\n",
+ adap->fe_adap[0].stream.props.endpoint);
+ adapt_nr = adap->id;
} else {
- if (onoff)
- st->channel_state |= 1 << (adap->fe_adap[0].stream.props.endpoint-2);
- else
- st->channel_state |= 1 << (3-adap->fe_adap[0].stream.props.endpoint);
+ adapt_nr = adap->fe_adap[0].stream.props.endpoint - 2;
}

+ if (onoff)
+ st->channel_state |= 1 << adapt_nr;
+ else
+ st->channel_state |= 1 << (1 - adapt_nr);
+
st->buf[2] |= st->channel_state;

- deb_info("data for streaming: %x %x\n", st->buf[1], st->buf[2]);
+ deb_info("adapter %d, streaming %s: %*ph\n",
+ adapt_nr, onoff ? "ON" : "OFF", 3, st->buf);

ret = dib0700_ctrl_wr(adap->dev, st->buf, 4);
mutex_unlock(&adap->dev->usb_mutex);
--
2.31.1

2021-09-26 20:53:39

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: [PATCH 3/3] media: dib0700: Only touch one bit when start/stop an adapter

Only touch the right bit to enable/disable an adapter channel,
without touching the other adapter's one.

Tested on Nova-TD.

Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/usb/dvb-usb/dib0700_core.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c
index d7c5836b9271..1caabb51ea47 100644
--- a/drivers/media/usb/dvb-usb/dib0700_core.c
+++ b/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -610,8 +610,6 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)

st->buf[3] = 0x00;

- st->channel_state &= ~0x3;
-
if ((adap->fe_adap[0].stream.props.endpoint != 2)
&& (adap->fe_adap[0].stream.props.endpoint != 3)) {
deb_info("the endpoint number (%i) is not correct, use the adapter id instead\n",
@@ -624,7 +622,7 @@ int dib0700_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
if (onoff)
st->channel_state |= 1 << adapt_nr;
else
- st->channel_state |= 1 << (1 - adapt_nr);
+ st->channel_state &= ~(1 << adapt_nr);

st->buf[2] |= st->channel_state;

--
2.31.1

2021-12-21 06:43:43

by Hans Petter Selasky

[permalink] [raw]
Subject: Re: [PATCH 0/3] Fix streaming on/off logic

On 9/26/21 22:51, Mauro Carvalho Chehab wrote:
> As discussed on:
> https://github.com/hselasky/webcamd/issues/16
>
> the dib0700 had a regression on Kernel 2.6.39. Such regression didn't
> affect most devices, in practice, as it seems to happen only under
> certain circunstances.
>
> Michael came up with a solution for the issue (already submitted to
> the ML) but let's take the opportunity to do a cleanup, as the resulting
> code was still touching both adapters when an stream off command
> was issued to one adapter, turning on the other one.
>
> After the change, each adapter is idependently controlled by
> a separate bit, as can be shown when its debug message
> is turned on (tested on a dual-adapter device: Hauppauge
> WinTV Nova TD):
>
> [608855.124780] adapter 1, streaming ON: 0f 10 12
> [608868.189827] adapter 0, streaming ON: 0f 10 13
> [608879.584330] adapter 1, streaming OFF: 0f 00 11
> [608887.014772] adapter 0, streaming OFF: 0f 00 10
>
> Mauro Carvalho Chehab (2):
> media: dib0700: cleanup start/stop streaming logic
> media: dib0700: Only touch one bit when start/stop an adapter
>
> Michael Kuron (1):
> media: dib0700: fix undefined behavior in tuner shutdown
>
> drivers/media/usb/dvb-usb/dib0700_core.c | 28 +++++++++++-------------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>

Were these patches upstreamed yet?

--HPS

2021-12-22 11:50:17

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH 0/3] Fix streaming on/off logic

Em Tue, 21 Dec 2021 07:34:27 +0100
Hans Petter Selasky <[email protected]> escreveu:

> On 9/26/21 22:51, Mauro Carvalho Chehab wrote:
> > As discussed on:
> > https://github.com/hselasky/webcamd/issues/16
> >
> > the dib0700 had a regression on Kernel 2.6.39. Such regression didn't
> > affect most devices, in practice, as it seems to happen only under
> > certain circunstances.
> >
> > Michael came up with a solution for the issue (already submitted to
> > the ML) but let's take the opportunity to do a cleanup, as the resulting
> > code was still touching both adapters when an stream off command
> > was issued to one adapter, turning on the other one.
> >
> > After the change, each adapter is idependently controlled by
> > a separate bit, as can be shown when its debug message
> > is turned on (tested on a dual-adapter device: Hauppauge
> > WinTV Nova TD):
> >
> > [608855.124780] adapter 1, streaming ON: 0f 10 12
> > [608868.189827] adapter 0, streaming ON: 0f 10 13
> > [608879.584330] adapter 1, streaming OFF: 0f 00 11
> > [608887.014772] adapter 0, streaming OFF: 0f 00 10
> >
> > Mauro Carvalho Chehab (2):
> > media: dib0700: cleanup start/stop streaming logic
> > media: dib0700: Only touch one bit when start/stop an adapter
> >
> > Michael Kuron (1):
> > media: dib0700: fix undefined behavior in tuner shutdown
> >
> > drivers/media/usb/dvb-usb/dib0700_core.c | 28 +++++++++++-------------
> > 1 file changed, 13 insertions(+), 15 deletions(-)
> >
>
> Were these patches upstreamed yet?

Those were merged on media upstream tree. They should likely be
merged upstream before v5.17-rc1.

>
> --HPS



Thanks,
Mauro