[PATCH 1/3] au8028: Fix cleanup on kzalloc fail

[PATCH 1/3] au8028: Fix cleanup on kzalloc fail

[Tim Mester]

Free what was allocated if there is a failure allocating
transfer buffers.

Stop the feed on a start feed error.  The stop feed is not always called
if start feed fails.  If the feed is not stopped on error, then the driver
will be stuck so that it can never start feeding again.

Signed-off-by: Tim Mester <tmester@ieee.org>
---
 linux/drivers/media/usb/au0828/au0828-dvb.c | 70 +++++++++++++++++++++--------
 linux/drivers/media/usb/au0828/au0828.h     |  2 +
 2 files changed, 53 insertions(+), 19 deletions(-)

diff --git a/linux/drivers/media/usb/au0828/au0828-dvb.c b/linux/drivers/media/usb/au0828/au0828-dvb.c
index 9a6f156..2312381 100644
--- a/linux/drivers/media/usb/au0828/au0828-dvb.c
+++ b/linux/drivers/media/usb/au0828/au0828-dvb.c
@@ -153,9 +153,11 @@ static int stop_urb_transfer(struct au0828_dev *dev)
 
 	dev->urb_streaming = 0;
 	for (i = 0; i < URB_COUNT; i++) {
-		usb_kill_urb(dev->urbs[i]);
-		kfree(dev->urbs[i]->transfer_buffer);
-		usb_free_urb(dev->urbs[i]);
+		if (dev->urbs[i]) {
+			usb_kill_urb(dev->urbs[i]);
+			kfree(dev->urbs[i]->transfer_buffer);
+			usb_free_urb(dev->urbs[i]);
+		}
 	}
 
 	return 0;
@@ -185,6 +187,8 @@ static int start_urb_transfer(struct au0828_dev *dev)
 		if (!purb->transfer_buffer) {
 			usb_free_urb(purb);
 			dev->urbs[i] = NULL;
+			printk(KERN_ERR "%s: failed big buffer allocation, "
+			       "err = %d\n", __func__, ret);
 			goto err;
 		}
 
@@ -217,6 +221,27 @@ err:
 	return ret;
 }
 
+static void au0828_start_transport(struct au0828_dev *dev)
+{
+	au0828_write(dev, 0x608, 0x90);
+	au0828_write(dev, 0x609, 0x72);
+	au0828_write(dev, 0x60a, 0x71);
+	au0828_write(dev, 0x60b, 0x01);
+
+}
+
+static void au0828_stop_transport(struct au0828_dev *dev, int full_stop)
+{
+	if (full_stop) {
+		au0828_write(dev, 0x608, 0x00);
+		au0828_write(dev, 0x609, 0x00);
+		au0828_write(dev, 0x60a, 0x00);
+	}
+	au0828_write(dev, 0x60b, 0x00);
+}
+
+
+
 static int au0828_dvb_start_feed(struct dvb_demux_feed *feed)
 {
 	struct dvb_demux *demux = feed->demux;
@@ -231,13 +256,17 @@ static int au0828_dvb_start_feed(struct dvb_demux_feed *feed)
 
 	if (dvb) {
 		mutex_lock(&dvb->lock);
+		dvb->start_count++;
+		dprintk(1, "%s(), start_count: %d, stop_count: %d\n", __func__,
+			dvb->start_count, dvb->stop_count);
 		if (dvb->feeding++ == 0) {
 			/* Start transport */
-			au0828_write(dev, 0x608, 0x90);
-			au0828_write(dev, 0x609, 0x72);
-			au0828_write(dev, 0x60a, 0x71);
-			au0828_write(dev, 0x60b, 0x01);
+			au0828_start_transport(dev);
 			ret = start_urb_transfer(dev);
+			if (ret < 0) {
+				au0828_stop_transport(dev, 0);
+				dvb->feeding--;	/* We ran out of memory... */
+			}
 		}
 		mutex_unlock(&dvb->lock);
 	}
@@ -256,10 +285,16 @@ static int au0828_dvb_stop_feed(struct dvb_demux_feed *feed)
 
 	if (dvb) {
 		mutex_lock(&dvb->lock);
-		if (--dvb->feeding == 0) {
-			/* Stop transport */
-			ret = stop_urb_transfer(dev);
-			au0828_write(dev, 0x60b, 0x00);
+		dvb->stop_count++;
+		dprintk(1, "%s(), start_count: %d, stop_count: %d\n", __func__,
+			dvb->start_count, dvb->stop_count);
+		if (dvb->feeding > 0) {
+			dvb->feeding--;
+			if (dvb->feeding == 0) {
+				/* Stop transport */
+				ret = stop_urb_transfer(dev);
+				au0828_stop_transport(dev, 0);
+			}
 		}
 		mutex_unlock(&dvb->lock);
 	}
@@ -282,16 +317,10 @@ static void au0828_restart_dvb_streaming(struct work_struct *work)
 
 	/* Stop transport */
 	stop_urb_transfer(dev);
-	au0828_write(dev, 0x608, 0x00);
-	au0828_write(dev, 0x609, 0x00);
-	au0828_write(dev, 0x60a, 0x00);
-	au0828_write(dev, 0x60b, 0x00);
+	au0828_stop_transport(dev, 1);
 
 	/* Start transport */
-	au0828_write(dev, 0x608, 0x90);
-	au0828_write(dev, 0x609, 0x72);
-	au0828_write(dev, 0x60a, 0x71);
-	au0828_write(dev, 0x60b, 0x01);
+	au0828_start_transport(dev);
 	start_urb_transfer(dev);
 
 	mutex_unlock(&dvb->lock);
@@ -375,6 +404,9 @@ static int dvb_register(struct au0828_dev *dev)
 
 	/* register network adapter */
 	dvb_net_init(&dvb->adapter, &dvb->net, &dvb->demux.dmx);
+
+	dvb->start_count = 0;
+	dvb->stop_count = 0;
 	return 0;
 
 fail_fe_conn:
diff --git a/linux/drivers/media/usb/au0828/au0828.h b/linux/drivers/media/usb/au0828/au0828.h
index ef1f57f..a00b400 100644
--- a/linux/drivers/media/usb/au0828/au0828.h
+++ b/linux/drivers/media/usb/au0828/au0828.h
@@ -102,6 +102,8 @@ struct au0828_dvb {
 	struct dmx_frontend fe_mem;
 	struct dvb_net net;
 	int feeding;
+	int start_count;
+	int stop_count;
 };
 
 enum au0828_stream_state {
-- 
1.8.1.4

[PATCH 2/3] au0828: Add option to preallocate digital transfer buffers

[Tim Mester]

Added command line parameter preallocate_big_buffers so that the digital
transfer buffers can be allocated when the driver is registered. They
do not have to be allocated every time a feed is started.

Signed-off-by: Tim Mester <tmester@ieee.org>
---
 linux/drivers/media/usb/au0828/au0828-core.c | 13 +++++---
 linux/drivers/media/usb/au0828/au0828-dvb.c  | 46 ++++++++++++++++++++++++++--
 linux/drivers/media/usb/au0828/au0828.h      |  4 +++
 3 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/linux/drivers/media/usb/au0828/au0828-core.c b/linux/drivers/media/usb/au0828/au0828-core.c
index bd9d19a..ab45a6f 100644
--- a/linux/drivers/media/usb/au0828/au0828-core.c
+++ b/linux/drivers/media/usb/au0828/au0828-core.c
@@ -173,9 +173,8 @@ static int au0828_usb_probe(struct usb_interface *interface,
 	const struct usb_device_id *id)
 {
 	int ifnum;
-#ifdef CONFIG_VIDEO_AU0828_V4L2
-	int retval;
-#endif
+	int retval = 0;
+
 	struct au0828_dev *dev;
 	struct usb_device *usbdev = interface_to_usbdev(interface);
 
@@ -257,7 +256,11 @@ static int au0828_usb_probe(struct usb_interface *interface,
 #endif
 
 	/* Digital TV */
-	au0828_dvb_register(dev);
+	retval = au0828_dvb_register(dev);
+	if (retval)
+		pr_err("%s() au0282_dev_register failed\n",
+		       __func__);
+
 
 	/* Store the pointer to the au0828_dev so it can be accessed in
 	   au0828_usb_disconnect */
@@ -268,7 +271,7 @@ static int au0828_usb_probe(struct usb_interface *interface,
 
 	mutex_unlock(&dev->lock);
 
-	return 0;
+	return retval;
 }
 
 static struct usb_driver au0828_usb_driver = {
diff --git a/linux/drivers/media/usb/au0828/au0828-dvb.c b/linux/drivers/media/usb/au0828/au0828-dvb.c
index 2312381..1673c88 100644
--- a/linux/drivers/media/usb/au0828/au0828-dvb.c
+++ b/linux/drivers/media/usb/au0828/au0828-dvb.c
@@ -33,6 +33,10 @@
 #include "mxl5007t.h"
 #include "tda18271.h"
 
+int preallocate_big_buffers;
+module_param_named(preallocate_big_buffers, preallocate_big_buffers, int, 0644);
+MODULE_PARM_DESC(preallocate_big_buffers, "Preallocate the larger transfer buffers at module load time");
+
 DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 
 #define _AU0828_BULKPIPE 0x83
@@ -155,7 +159,9 @@ static int stop_urb_transfer(struct au0828_dev *dev)
 	for (i = 0; i < URB_COUNT; i++) {
 		if (dev->urbs[i]) {
 			usb_kill_urb(dev->urbs[i]);
-			kfree(dev->urbs[i]->transfer_buffer);
+			if (!preallocate_big_buffers)
+				kfree(dev->urbs[i]->transfer_buffer);
+
 			usb_free_urb(dev->urbs[i]);
 		}
 	}
@@ -183,7 +189,12 @@ static int start_urb_transfer(struct au0828_dev *dev)
 
 		purb = dev->urbs[i];
 
-		purb->transfer_buffer = kzalloc(URB_BUFSIZE, GFP_KERNEL);
+		if (preallocate_big_buffers)
+			purb->transfer_buffer = dev->dig_transfer_buffer[i];
+		else
+			purb->transfer_buffer = kzalloc(URB_BUFSIZE,
+					GFP_KERNEL);
+
 		if (!purb->transfer_buffer) {
 			usb_free_urb(purb);
 			dev->urbs[i] = NULL;
@@ -333,6 +344,22 @@ static int dvb_register(struct au0828_dev *dev)
 
 	dprintk(1, "%s()\n", __func__);
 
+	if (preallocate_big_buffers) {
+		int i;
+		for (i = 0; i < URB_COUNT; i++) {
+			dev->dig_transfer_buffer[i] = kzalloc(URB_BUFSIZE,
+					GFP_KERNEL);
+
+			if (!dev->dig_transfer_buffer[i]) {
+				result = -ENOMEM;
+
+				printk(KERN_ERR "%s: failed buffer allocation"
+				       "(errno = %d)\n", DRIVER_NAME, result);
+				goto fail_adapter;
+			}
+		}
+	}
+
 	INIT_WORK(&dev->restart_streaming, au0828_restart_dvb_streaming);
 
 	/* register adapter */
@@ -423,6 +450,13 @@ fail_frontend:
 	dvb_frontend_detach(dvb->frontend);
 	dvb_unregister_adapter(&dvb->adapter);
 fail_adapter:
+
+	if (preallocate_big_buffers) {
+		int i;
+		for (i = 0; i < URB_COUNT; i++)
+			kfree(dev->dig_transfer_buffer[i]);
+	}
+
 	return result;
 }
 
@@ -443,6 +477,14 @@ void au0828_dvb_unregister(struct au0828_dev *dev)
 	dvb_unregister_frontend(dvb->frontend);
 	dvb_frontend_detach(dvb->frontend);
 	dvb_unregister_adapter(&dvb->adapter);
+
+	if (preallocate_big_buffers) {
+		int i;
+		for (i = 0; i < URB_COUNT; i++)
+			kfree(dev->dig_transfer_buffer[i]);
+	}
+
+
 }
 
 /* All the DVB attach calls go here, this function get's modified
diff --git a/linux/drivers/media/usb/au0828/au0828.h b/linux/drivers/media/usb/au0828/au0828.h
index a00b400..5439772 100644
--- a/linux/drivers/media/usb/au0828/au0828.h
+++ b/linux/drivers/media/usb/au0828/au0828.h
@@ -262,6 +262,10 @@ struct au0828_dev {
 	/* USB / URB Related */
 	int		urb_streaming;
 	struct urb	*urbs[URB_COUNT];
+
+	/* Preallocated transfer digital transfer buffers */
+
+	char *dig_transfer_buffer[URB_COUNT];
 };
 
 /* ----------------------------------------------------------- */
-- 
1.8.1.4

Re: [PATCH 2/3] au0828: Add option to preallocate digital transfer buffers

[Devin Heitmueller]

On Mon, Jan 6, 2014 at 11:29 PM, Tim Mester <ttmesterr@gmail.com> wrote:
> Added command line parameter preallocate_big_buffers so that the digital
> transfer buffers can be allocated when the driver is registered. They
> do not have to be allocated every time a feed is started.
>
> Signed-off-by: Tim Mester <tmester@ieee.org>
> ---
>  linux/drivers/media/usb/au0828/au0828-core.c | 13 +++++---
>  linux/drivers/media/usb/au0828/au0828-dvb.c  | 46 ++++++++++++++++++++++++++--
>  linux/drivers/media/usb/au0828/au0828.h      |  4 +++
>  3 files changed, 56 insertions(+), 7 deletions(-)
>
> diff --git a/linux/drivers/media/usb/au0828/au0828-core.c b/linux/drivers/media/usb/au0828/au0828-core.c
> index bd9d19a..ab45a6f 100644
> --- a/linux/drivers/media/usb/au0828/au0828-core.c
> +++ b/linux/drivers/media/usb/au0828/au0828-core.c
> @@ -173,9 +173,8 @@ static int au0828_usb_probe(struct usb_interface *interface,
>         const struct usb_device_id *id)
>  {
>         int ifnum;
> -#ifdef CONFIG_VIDEO_AU0828_V4L2
> -       int retval;
> -#endif
> +       int retval = 0;
> +
>         struct au0828_dev *dev;
>         struct usb_device *usbdev = interface_to_usbdev(interface);
>
> @@ -257,7 +256,11 @@ static int au0828_usb_probe(struct usb_interface *interface,
>  #endif
>
>         /* Digital TV */
> -       au0828_dvb_register(dev);
> +       retval = au0828_dvb_register(dev);
> +       if (retval)
> +               pr_err("%s() au0282_dev_register failed\n",
> +                      __func__);
> +
>
>         /* Store the pointer to the au0828_dev so it can be accessed in
>            au0828_usb_disconnect */
> @@ -268,7 +271,7 @@ static int au0828_usb_probe(struct usb_interface *interface,
>
>         mutex_unlock(&dev->lock);
>
> -       return 0;
> +       return retval;
>  }
>
>  static struct usb_driver au0828_usb_driver = {
> diff --git a/linux/drivers/media/usb/au0828/au0828-dvb.c b/linux/drivers/media/usb/au0828/au0828-dvb.c
> index 2312381..1673c88 100644
> --- a/linux/drivers/media/usb/au0828/au0828-dvb.c
> +++ b/linux/drivers/media/usb/au0828/au0828-dvb.c
> @@ -33,6 +33,10 @@
>  #include "mxl5007t.h"
>  #include "tda18271.h"
>
> +int preallocate_big_buffers;
> +module_param_named(preallocate_big_buffers, preallocate_big_buffers, int, 0644);
> +MODULE_PARM_DESC(preallocate_big_buffers, "Preallocate the larger transfer buffers at module load time");
> +
>  DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
>
>  #define _AU0828_BULKPIPE 0x83
> @@ -155,7 +159,9 @@ static int stop_urb_transfer(struct au0828_dev *dev)
>         for (i = 0; i < URB_COUNT; i++) {
>                 if (dev->urbs[i]) {
>                         usb_kill_urb(dev->urbs[i]);
> -                       kfree(dev->urbs[i]->transfer_buffer);
> +                       if (!preallocate_big_buffers)
> +                               kfree(dev->urbs[i]->transfer_buffer);
> +
>                         usb_free_urb(dev->urbs[i]);
>                 }
>         }
> @@ -183,7 +189,12 @@ static int start_urb_transfer(struct au0828_dev *dev)
>
>                 purb = dev->urbs[i];
>
> -               purb->transfer_buffer = kzalloc(URB_BUFSIZE, GFP_KERNEL);
> +               if (preallocate_big_buffers)
> +                       purb->transfer_buffer = dev->dig_transfer_buffer[i];
> +               else
> +                       purb->transfer_buffer = kzalloc(URB_BUFSIZE,
> +                                       GFP_KERNEL);
> +
>                 if (!purb->transfer_buffer) {
>                         usb_free_urb(purb);
>                         dev->urbs[i] = NULL;
> @@ -333,6 +344,22 @@ static int dvb_register(struct au0828_dev *dev)
>
>         dprintk(1, "%s()\n", __func__);
>
> +       if (preallocate_big_buffers) {
> +               int i;
> +               for (i = 0; i < URB_COUNT; i++) {
> +                       dev->dig_transfer_buffer[i] = kzalloc(URB_BUFSIZE,
> +                                       GFP_KERNEL);
> +
> +                       if (!dev->dig_transfer_buffer[i]) {
> +                               result = -ENOMEM;
> +
> +                               printk(KERN_ERR "%s: failed buffer allocation"
> +                                      "(errno = %d)\n", DRIVER_NAME, result);
> +                               goto fail_adapter;
> +                       }
> +               }
> +       }
> +
>         INIT_WORK(&dev->restart_streaming, au0828_restart_dvb_streaming);
>
>         /* register adapter */
> @@ -423,6 +450,13 @@ fail_frontend:
>         dvb_frontend_detach(dvb->frontend);
>         dvb_unregister_adapter(&dvb->adapter);
>  fail_adapter:
> +
> +       if (preallocate_big_buffers) {
> +               int i;
> +               for (i = 0; i < URB_COUNT; i++)
> +                       kfree(dev->dig_transfer_buffer[i]);
> +       }
> +
>         return result;
>  }
>
> @@ -443,6 +477,14 @@ void au0828_dvb_unregister(struct au0828_dev *dev)
>         dvb_unregister_frontend(dvb->frontend);
>         dvb_frontend_detach(dvb->frontend);
>         dvb_unregister_adapter(&dvb->adapter);
> +
> +       if (preallocate_big_buffers) {
> +               int i;
> +               for (i = 0; i < URB_COUNT; i++)
> +                       kfree(dev->dig_transfer_buffer[i]);
> +       }
> +
> +
>  }
>
>  /* All the DVB attach calls go here, this function get's modified
> diff --git a/linux/drivers/media/usb/au0828/au0828.h b/linux/drivers/media/usb/au0828/au0828.h
> index a00b400..5439772 100644
> --- a/linux/drivers/media/usb/au0828/au0828.h
> +++ b/linux/drivers/media/usb/au0828/au0828.h
> @@ -262,6 +262,10 @@ struct au0828_dev {
>         /* USB / URB Related */
>         int             urb_streaming;
>         struct urb      *urbs[URB_COUNT];
> +
> +       /* Preallocated transfer digital transfer buffers */
> +
> +       char *dig_transfer_buffer[URB_COUNT];
>  };
>
>  /* ----------------------------------------------------------- */
> --
> 1.8.1.4
>
> --
> 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

Acked-by: Devin Heitmueller <dheitmueller@kernellabs.com>

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

[PATCH 3/3] au8522, au0828: Added demodulator reset

[Tim Mester]

The demodulator can get in a state in ATSC mode where just
restarting the feed alone does not correct the corrupted stream.  The
demodulator reset in addition to the feed restart seems to correct
the condition.

The au8522 driver has been modified so that when set_frontend() is
called with the same frequency and modulation mode, the demodulator
will be reset.  The au0282 drives uses this feature when it attempts
to restart the feed.

Signed-off-by: Tim Mester <tmester@ieee.org>
---
 linux/drivers/media/dvb-frontends/au8522_dig.c | 38 +++++++++++++++++++++++++-
 linux/drivers/media/usb/au0828/au0828-dvb.c    |  8 ++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/linux/drivers/media/dvb-frontends/au8522_dig.c b/linux/drivers/media/dvb-frontends/au8522_dig.c
index a68974f..821cc70 100644
--- a/linux/drivers/media/dvb-frontends/au8522_dig.c
+++ b/linux/drivers/media/dvb-frontends/au8522_dig.c
@@ -469,6 +469,38 @@ static struct {
 	{ 0x8526, 0x01 },
 };
 
+/*
+ * Reset the demodulator.  Currently this only does something if
+ * configured in ATSC mode.  This reset is needed in marginal signal
+ * levels when the feed (in MPEG-TS format) is correupted.
+ *
+ */
+
+static int au8522_reset_demodulator(struct dvb_frontend *fe)
+{
+	struct au8522_state *state = fe->demodulator_priv;
+
+	switch (state->current_modulation) {
+	case VSB_8:
+		dprintk("%s() VSB_8\n", __func__);
+		au8522_writereg(state, 0x80a4, 0);
+		au8522_writereg(state, 0x80a5, 0);
+		au8522_writereg(state, 0x80a4, 0xe8);
+		au8522_writereg(state, 0x80a5, 0x40);
+		break;
+	case QAM_64:
+	case QAM_256:
+		dprintk("%s() QAM 64/256\n", __func__);
+		break;
+	default:
+		dprintk("%s() Invalid modulation\n", __func__);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+
 static int au8522_enable_modulation(struct dvb_frontend *fe,
 				    fe_modulation_t m)
 {
@@ -522,8 +554,12 @@ static int au8522_set_frontend(struct dvb_frontend *fe)
 	dprintk("%s(frequency=%d)\n", __func__, c->frequency);
 
 	if ((state->current_frequency == c->frequency) &&
-	    (state->current_modulation == c->modulation))
+	    (state->current_modulation == c->modulation)) {
+
+		au8522_reset_demodulator(fe);
+		msleep(100);
 		return 0;
+	}
 
 	if (fe->ops.tuner_ops.set_params) {
 		if (fe->ops.i2c_gate_ctrl)
diff --git a/linux/drivers/media/usb/au0828/au0828-dvb.c b/linux/drivers/media/usb/au0828/au0828-dvb.c
index 1673c88..483c587 100644
--- a/linux/drivers/media/usb/au0828/au0828-dvb.c
+++ b/linux/drivers/media/usb/au0828/au0828-dvb.c
@@ -330,6 +330,14 @@ static void au0828_restart_dvb_streaming(struct work_struct *work)
 	stop_urb_transfer(dev);
 	au0828_stop_transport(dev, 1);
 
+	/*
+	 * In ATSC mode, we also need to reset the demodulator in lower signal
+	 * level cases to help realign the data stream.
+	 */
+
+	if (dvb->frontend->ops.set_frontend)
+		dvb->frontend->ops.set_frontend(dvb->frontend);
+
 	/* Start transport */
 	au0828_start_transport(dev);
 	start_urb_transfer(dev);
-- 
1.8.1.4

Re: [PATCH 3/3] au8522, au0828: Added demodulator reset

[Devin Heitmueller]

On Mon, Jan 6, 2014 at 11:29 PM, Tim Mester <ttmesterr@gmail.com> wrote:
> The demodulator can get in a state in ATSC mode where just
> restarting the feed alone does not correct the corrupted stream.  The
> demodulator reset in addition to the feed restart seems to correct
> the condition.
>
> The au8522 driver has been modified so that when set_frontend() is
> called with the same frequency and modulation mode, the demodulator
> will be reset.  The au0282 drives uses this feature when it attempts
> to restart the feed.

What is the actual "corruption" that you are seeing?  Can you describe
it in greater detail?  The original fix was specifically related to
the internal FIFO on the au0828 where it can get shifted by one or
more bits (i.e. the leading byte is no longer 0x47 but 0x47 << X).
Hence it's an issue unrelated to the actual au8522.

I suspect this is actually a different problem which out of dumb luck
gets "fixed" by resetting the chip.  Without more details on the
specific behavior you are seeing though I cannot really advise on what
the correct change is.

This patch should not be accepted upstream without more discussion.

Regards,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 3/3] au8522, au0828: Added demodulator reset

[Devin Heitmueller]

> I suspect this is actually a different problem which out of dumb luck
> gets "fixed" by resetting the chip.  Without more details on the
> specific behavior you are seeing though I cannot really advise on what
> the correct change is.

Tim,

It might be worth trying out the following patch series and see if it
addresses the problem you're seeing.  There was a host of problems
with the clock management on the device which could result in the
various sub-blocks getting wedged.  The TS output block was just one
of those cases.

http://git.kernellabs.com/?p=dheitmueller/linuxtv.git;a=shortlog;h=refs/heads/950q_improv

I'm not against the hack you've proposed if it's really warranted, but
a reset is really a last resort and I'm very concerned it's masking
over the real problem.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 3/3] au8522, au0828: Added demodulator reset

[Mauro Carvalho Chehab]

Hi Devin,

Em Mon, 06 Jan 2014 23:53:15 -0500
Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:

> What is the actual "corruption" that you are seeing?  Can you describe
> it in greater detail?  The original fix was specifically related to
> the internal FIFO on the au0828 where it can get shifted by one or
> more bits (i.e. the leading byte is no longer 0x47 but 0x47 << X).
> Hence it's an issue unrelated to the actual au8522.
> 
> I suspect this is actually a different problem which out of dumb luck
> gets "fixed" by resetting the chip.  Without more details on the
> specific behavior you are seeing though I cannot really advise on what
> the correct change is.
> 
> This patch should not be accepted upstream without more discussion.

Patches 1 and 2 are ok? If so, could you please reply to them with your 
ack?

> http://git.kernellabs.com/?p=dheitmueller/linuxtv.git;a=shortlog;h=refs/heads/950q_improv
> 
> I'm not against the hack you've proposed if it's really warranted, but
> a reset is really a last resort and I'm very concerned it's masking
> over the real problem.

Are you planning to submit the above patches upstream soon?



-- 

Cheers,
Mauro

Re: [PATCH 3/3] au8522, au0828: Added demodulator reset

[Devin Heitmueller]

On Tue, Jan 7, 2014 at 9:58 AM, Mauro Carvalho Chehab
<m.chehab@samsung.com> wrote:
> Patches 1 and 2 are ok? If so, could you please reply to them with your
> ack?

Sure, no problem.

>> http://git.kernellabs.com/?p=dheitmueller/linuxtv.git;a=shortlog;h=refs/heads/950q_improv
>>
>> I'm not against the hack you've proposed if it's really warranted, but
>> a reset is really a last resort and I'm very concerned it's masking
>> over the real problem.
>
> Are you planning to submit the above patches upstream soon?

Yeah, I've just been busy and haven't had a chance to send them to the
list (except for the last patch on the series, which is customer
specific).  I've got a couple of others which haven't been pushed and
need to be put onto that series before it can be merged.  Will
definitely be good to get it all upstream before somebody does some
refactoring and then a rebase is required.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 3/3] au8522, au0828: Added demodulator reset

[Tim Mester]

On Mon, Jan 6, 2014 at 9:53 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
>> I suspect this is actually a different problem which out of dumb luck
>> gets "fixed" by resetting the chip.  Without more details on the
>> specific behavior you are seeing though I cannot really advise on what
>> the correct change is.
>
> Tim,
>
> It might be worth trying out the following patch series and see if it
> addresses the problem you're seeing.  There was a host of problems
> with the clock management on the device which could result in the
> various sub-blocks getting wedged.  The TS output block was just one
> of those cases.
>
> http://git.kernellabs.com/?p=dheitmueller/linuxtv.git;a=shortlog;h=refs/heads/950q_improv
>
> I'm not against the hack you've proposed if it's really warranted, but
> a reset is really a last resort and I'm very concerned it's masking
> over the real problem.
>
> Devin
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com

Devin,

  Commit 2e68a75990011ccd looks interesting.  It makes sense to me
that if we are gating the clock, and it is possible that we are
glitching the clock line, it could put the internal synchronous logic
into a bad state.  If that happens, it would generally require a reset
under a stable clock to get out of that condition.  I will give that
patch a try an see if it addresses issue 1), mentioned above.

However, I'm not sure if that will do anything about issue 2). Do you
have any insight into that one?

 Thanks,

 Tim

Re: [PATCH 3/3] au8522, au0828: Added demodulator reset

[Devin Heitmueller]

Hi Tim,

On Wed, Jan 8, 2014 at 12:12 AM, Tim Mester <tmester@ieee.org> wrote:
>   Commit 2e68a75990011ccd looks interesting.  It makes sense to me
> that if we are gating the clock, and it is possible that we are
> glitching the clock line, it could put the internal synchronous logic
> into a bad state.  If that happens, it would generally require a reset
> under a stable clock to get out of that condition.  I will give that
> patch a try an see if it addresses issue 1), mentioned above.

Yeah, the whole thing about the clocks not being enabled/disabled in
the correct order relative to enabling the sub-blocks did result in
some strange cases where sub-block wouldn't reactivate properly,
requiring a reset to return it to a working state.  It was
specifically this issue I was concerned about might be the "right fix"
for the problem you are hitting.

Note:  you need more than just 2e68a75990011ccd.  That is actually an
add-on to the real commit that restructures the clock managment:
39c39b0e612b5d35feb00329b527b266df8f7fd2

> However, I'm not sure if that will do anything about issue 2). Do you
> have any insight into that one?

Well, I've never been a fan of how the code just does a blind "return
0" if the target modulation and frequency are the same as it's in
theory already tuned to.  Have you tried commenting out just that
block and see if it makes a difference?  IIRC, the dvb-frontend kernel
thread should automatically re-issue a set_frontend() call if the
signal lock drops out.

As for the underlying problem, I'm not sure.  Generally once the
signal is locked it continues to work.  If you set the xc5000 debug=1
modprobe option, do you see lines in the log that say "xc5000: PLL not
locked"?

How reproducible is the issue, and how often does it happen?  I've got
some newer firmware that might be worth trying which isn't upstream
yet (assuming for a moment that it's an xc5000 issue).  If you believe
you can repro the issue pretty regularly, you and I can work offline
to try that out.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 3/3] au8522, au0828: Added demodulator reset

[Tim Mester]

On Wed, Jan 8, 2014 at 1:26 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> Hi Tim,
>
> On Wed, Jan 8, 2014 at 12:12 AM, Tim Mester <tmester@ieee.org> wrote:
>>   Commit 2e68a75990011ccd looks interesting.  It makes sense to me
>> that if we are gating the clock, and it is possible that we are
>> glitching the clock line, it could put the internal synchronous logic
>> into a bad state.  If that happens, it would generally require a reset
>> under a stable clock to get out of that condition.  I will give that
>> patch a try an see if it addresses issue 1), mentioned above.
>
> Yeah, the whole thing about the clocks not being enabled/disabled in
> the correct order relative to enabling the sub-blocks did result in
> some strange cases where sub-block wouldn't reactivate properly,
> requiring a reset to return it to a working state.  It was
> specifically this issue I was concerned about might be the "right fix"
> for the problem you are hitting.
>
> Note:  you need more than just 2e68a75990011ccd.  That is actually an
> add-on to the real commit that restructures the clock managment:
> 39c39b0e612b5d35feb00329b527b266df8f7fd2
>
>> However, I'm not sure if that will do anything about issue 2). Do you
>> have any insight into that one?
>
> Well, I've never been a fan of how the code just does a blind "return
> 0" if the target modulation and frequency are the same as it's in
> theory already tuned to.  Have you tried commenting out just that
> block and see if it makes a difference?  IIRC, the dvb-frontend kernel
> thread should automatically re-issue a set_frontend() call if the
> signal lock drops out.
>
> As for the underlying problem, I'm not sure.  Generally once the
> signal is locked it continues to work.  If you set the xc5000 debug=1
> modprobe option, do you see lines in the log that say "xc5000: PLL not
> locked"?
>
> How reproducible is the issue, and how often does it happen?  I've got
> some newer firmware that might be worth trying which isn't upstream
> yet (assuming for a moment that it's an xc5000 issue).  If you believe
> you can repro the issue pretty regularly, you and I can work offline
> to try that out.
>
> Devin
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com

Devin,

  My device is the 950q, so it uses the AU8522_DEMODLOCKING method.
It does not appear to be an xc5000 issue on the surface.   When I
originally put the patch together, I removed the return if the
frequency was the same, and added the reset_demodulator() call at the
end of the set_frontend() function. It seemed to work the same as the
patch that I submitted.

I have not been able to tell that it keeps the au8522 from losing
lock, but it allows it to come back.  I see this issue about once a
every 2-3 weeks on average, which is less frequent than the other
issues.

If you believe that this issue could result in a xc5000 and au8522
interaction, then I should be able to try out the updated firmware. It
will just take some time to know the results.

 Thanks,

 Tim

Re: [PATCH 3/3] au8522, au0828: Added demodulator reset

[Devin Heitmueller]

On Sat, Jan 11, 2014 at 5:12 PM, Tim Mester <tmester@ieee.org> wrote:
>   My device is the 950q, so it uses the AU8522_DEMODLOCKING method.

No devices do tuner locking for digital (it's always the demodulator).
 That code should really just be ripped out.

> It does not appear to be an xc5000 issue on the surface.   When I
> originally put the patch together, I removed the return if the
> frequency was the same, and added the reset_demodulator() call at the
> end of the set_frontend() function. It seemed to work the same as the
> patch that I submitted.

I'm pretty sure that we accomplish the same thing through the patch I
have which reworks the clock management. except for removing the part
where the set_frontend() call returns if the freq/modulation are
already set.

> I have not been able to tell that it keeps the au8522 from losing
> lock, but it allows it to come back.  I see this issue about once a
> every 2-3 weeks on average, which is less frequent than the other
> issues.
>
> If you believe that this issue could result in a xc5000 and au8522
> interaction, then I should be able to try out the updated firmware. It
> will just take some time to know the results.

My instinct would be to get you to try that patch series I have in
git.  I suspect that it will address your issue with no further
patches required (we might need the one additional patch to remove the
early return in set_frontend, but let's see).  The testing of the new
firmware can be done in a separate series (the issue it addresses is
completely unrelated to the behavior you are seeing).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 3/3] au8522, au0828: Added demodulator reset

[Tim Mester]

On Mon, Jan 6, 2014 at 9:45 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
>
> On Mon, Jan 6, 2014 at 11:29 PM, Tim Mester <ttmesterr@gmail.com> wrote:
> > The demodulator can get in a state in ATSC mode where just
> > restarting the feed alone does not correct the corrupted stream.  The
> > demodulator reset in addition to the feed restart seems to correct
> > the condition.
> >
> > The au8522 driver has been modified so that when set_frontend() is
> > called with the same frequency and modulation mode, the demodulator
> > will be reset.  The au0282 drives uses this feature when it attempts
> > to restart the feed.
>
> What is the actual "corruption" that you are seeing?  Can you describe
> it in greater detail?  The original fix was specifically related to
> the internal FIFO on the au0828 where it can get shifted by one or
> more bits (i.e. the leading byte is no longer 0x47 but 0x47 << X).
> Hence it's an issue unrelated to the actual au8522.
>
> I suspect this is actually a different problem which out of dumb luck
> gets "fixed" by resetting the chip.  Without more details on the
> specific behavior you are seeing though I cannot really advise on what
> the correct change is.
>
> This patch should not be accepted upstream without more discussion.
>
> Regards,
>
> Devin
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com


The patch really address two issues:

1) The driver reports that the TS sync byte has not been found (the
0x47), and that it is attempting to restart streaming. However, this
message gets repeated indefinitely, and the streaming can never begin.
Eventually, Mythtv gives up on the recoding.  When I reset the
demodulator, the streaming immediately starts. I have not examined the
data to see if it is a shifted sync byte or not, but the symptom seems
similar.

2) Occasionally, the au8522 report that it lost lock, and the TS
stream stops flowing.  The au8522_set_frontend(), gets called, but
because we are already tuned to the same frequency, nothing happens,
and lock is never  re-established until we tune to a different
channel, then back to the original one where lock was lost.

I have only seen this behavior in ATSC mode. Back when I was using the
device in QAM256 mode, I did not observe the Lock lost or the unable
to start streaming issues.


Thanks,

Tim

Re: [PATCH 1/3] au8028: Fix cleanup on kzalloc fail

[Devin Heitmueller]

On Mon, Jan 6, 2014 at 11:29 PM, Tim Mester <ttmesterr@gmail.com> wrote:
> Free what was allocated if there is a failure allocating
> transfer buffers.
>
> Stop the feed on a start feed error.  The stop feed is not always called
> if start feed fails.  If the feed is not stopped on error, then the driver
> will be stuck so that it can never start feeding again.
>
> Signed-off-by: Tim Mester <tmester@ieee.org>
> ---
>  linux/drivers/media/usb/au0828/au0828-dvb.c | 70 +++++++++++++++++++++--------
>  linux/drivers/media/usb/au0828/au0828.h     |  2 +
>  2 files changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/linux/drivers/media/usb/au0828/au0828-dvb.c b/linux/drivers/media/usb/au0828/au0828-dvb.c
> index 9a6f156..2312381 100644
> --- a/linux/drivers/media/usb/au0828/au0828-dvb.c
> +++ b/linux/drivers/media/usb/au0828/au0828-dvb.c
> @@ -153,9 +153,11 @@ static int stop_urb_transfer(struct au0828_dev *dev)
>
>         dev->urb_streaming = 0;
>         for (i = 0; i < URB_COUNT; i++) {
> -               usb_kill_urb(dev->urbs[i]);
> -               kfree(dev->urbs[i]->transfer_buffer);
> -               usb_free_urb(dev->urbs[i]);
> +               if (dev->urbs[i]) {
> +                       usb_kill_urb(dev->urbs[i]);
> +                       kfree(dev->urbs[i]->transfer_buffer);
> +                       usb_free_urb(dev->urbs[i]);
> +               }
>         }
>
>         return 0;
> @@ -185,6 +187,8 @@ static int start_urb_transfer(struct au0828_dev *dev)
>                 if (!purb->transfer_buffer) {
>                         usb_free_urb(purb);
>                         dev->urbs[i] = NULL;
> +                       printk(KERN_ERR "%s: failed big buffer allocation, "
> +                              "err = %d\n", __func__, ret);
>                         goto err;
>                 }
>
> @@ -217,6 +221,27 @@ err:
>         return ret;
>  }
>
> +static void au0828_start_transport(struct au0828_dev *dev)
> +{
> +       au0828_write(dev, 0x608, 0x90);
> +       au0828_write(dev, 0x609, 0x72);
> +       au0828_write(dev, 0x60a, 0x71);
> +       au0828_write(dev, 0x60b, 0x01);
> +
> +}
> +
> +static void au0828_stop_transport(struct au0828_dev *dev, int full_stop)
> +{
> +       if (full_stop) {
> +               au0828_write(dev, 0x608, 0x00);
> +               au0828_write(dev, 0x609, 0x00);
> +               au0828_write(dev, 0x60a, 0x00);
> +       }
> +       au0828_write(dev, 0x60b, 0x00);
> +}
> +
> +
> +
>  static int au0828_dvb_start_feed(struct dvb_demux_feed *feed)
>  {
>         struct dvb_demux *demux = feed->demux;
> @@ -231,13 +256,17 @@ static int au0828_dvb_start_feed(struct dvb_demux_feed *feed)
>
>         if (dvb) {
>                 mutex_lock(&dvb->lock);
> +               dvb->start_count++;
> +               dprintk(1, "%s(), start_count: %d, stop_count: %d\n", __func__,
> +                       dvb->start_count, dvb->stop_count);
>                 if (dvb->feeding++ == 0) {
>                         /* Start transport */
> -                       au0828_write(dev, 0x608, 0x90);
> -                       au0828_write(dev, 0x609, 0x72);
> -                       au0828_write(dev, 0x60a, 0x71);
> -                       au0828_write(dev, 0x60b, 0x01);
> +                       au0828_start_transport(dev);
>                         ret = start_urb_transfer(dev);
> +                       if (ret < 0) {
> +                               au0828_stop_transport(dev, 0);
> +                               dvb->feeding--; /* We ran out of memory... */
> +                       }
>                 }
>                 mutex_unlock(&dvb->lock);
>         }
> @@ -256,10 +285,16 @@ static int au0828_dvb_stop_feed(struct dvb_demux_feed *feed)
>
>         if (dvb) {
>                 mutex_lock(&dvb->lock);
> -               if (--dvb->feeding == 0) {
> -                       /* Stop transport */
> -                       ret = stop_urb_transfer(dev);
> -                       au0828_write(dev, 0x60b, 0x00);
> +               dvb->stop_count++;
> +               dprintk(1, "%s(), start_count: %d, stop_count: %d\n", __func__,
> +                       dvb->start_count, dvb->stop_count);
> +               if (dvb->feeding > 0) {
> +                       dvb->feeding--;
> +                       if (dvb->feeding == 0) {
> +                               /* Stop transport */
> +                               ret = stop_urb_transfer(dev);
> +                               au0828_stop_transport(dev, 0);
> +                       }
>                 }
>                 mutex_unlock(&dvb->lock);
>         }
> @@ -282,16 +317,10 @@ static void au0828_restart_dvb_streaming(struct work_struct *work)
>
>         /* Stop transport */
>         stop_urb_transfer(dev);
> -       au0828_write(dev, 0x608, 0x00);
> -       au0828_write(dev, 0x609, 0x00);
> -       au0828_write(dev, 0x60a, 0x00);
> -       au0828_write(dev, 0x60b, 0x00);
> +       au0828_stop_transport(dev, 1);
>
>         /* Start transport */
> -       au0828_write(dev, 0x608, 0x90);
> -       au0828_write(dev, 0x609, 0x72);
> -       au0828_write(dev, 0x60a, 0x71);
> -       au0828_write(dev, 0x60b, 0x01);
> +       au0828_start_transport(dev);
>         start_urb_transfer(dev);
>
>         mutex_unlock(&dvb->lock);
> @@ -375,6 +404,9 @@ static int dvb_register(struct au0828_dev *dev)
>
>         /* register network adapter */
>         dvb_net_init(&dvb->adapter, &dvb->net, &dvb->demux.dmx);
> +
> +       dvb->start_count = 0;
> +       dvb->stop_count = 0;
>         return 0;
>
>  fail_fe_conn:
> diff --git a/linux/drivers/media/usb/au0828/au0828.h b/linux/drivers/media/usb/au0828/au0828.h
> index ef1f57f..a00b400 100644
> --- a/linux/drivers/media/usb/au0828/au0828.h
> +++ b/linux/drivers/media/usb/au0828/au0828.h
> @@ -102,6 +102,8 @@ struct au0828_dvb {
>         struct dmx_frontend fe_mem;
>         struct dvb_net net;
>         int feeding;
> +       int start_count;
> +       int stop_count;
>  };
>
>  enum au0828_stream_state {
> --
> 1.8.1.4
>
> --
> 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

Acked-by: Devin Heitmueller <dheitmueller@kernellabs.com>

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com