[PATCH 0/3] Another series of PM fixes for au0828

[PATCH 0/3] Another series of PM fixes for au0828

[Mauro Carvalho Chehab]

There are still a few bugs that can happen when suspending and
a video stream is active. This patch series fix them. After
that, resume works fine, even it suspend happened while
streaming.

There is one remaining issue though: xc5000 firmware doesn't
load after resume.

What happens (on both analog and digital) is:

[  143.071323] xc5000: xc5000_suspend()
[  143.071324] xc5000: xc5000_tuner_reset()
[  143.099992] au0828: Suspend
[  143.099992] au0828: Stopping RC
[  143.101694] au0828: stopping V4L2
[  143.101695] au0828: stopping V4L2 active URBs
[  144.988637] au0828: Resume
[  145.342026] au0828: Restarting RC
[  145.343296] au0828: restarting V4L2
[  145.464413] xc5000: xc5000_is_firmware_loaded() returns True id = 0xffff
[  145.464414] xc5000: xc_set_signal_source(1) Source = CABLE
[  146.370861] xc5000: xc_set_signal_source(1) failed

I suspect that it has to do with a wrong value for the I2C
gateway. The proper fix is likely to convert au0828 to use
the I2C mux support, and remove the old i2c_gate_ctrl
approach. However, such patch would require more work, to
avoid breaking other drivers.

Mauro Carvalho Chehab (3):
  au0828: fix checks if dvb is initialized
  au0828: Fix DVB resume when streaming
  xc5000: be sure that the firmware is there before set params

 drivers/media/tuners/xc5000.c         | 10 +++++-----
 drivers/media/usb/au0828/au0828-dvb.c | 24 ++++++++++++++----------
 drivers/media/usb/au0828/au0828.h     |  4 ++--
 3 files changed, 21 insertions(+), 17 deletions(-)

-- 
1.9.3

[PATCH 1/3] au0828: fix checks if dvb is initialized

[Mauro Carvalho Chehab]

dev->dvb is always not null, as it is an area at the dev
memory. So, checking if (dev->dvb) is always true.

Instead of this stupid check, what the code wants to do is
to know if the DVB was successully registered.

Fix it by checking, instead, for dvb->frontend. It should
also be sure that this var will be NULL if the device was
not properly initialized.

Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
 drivers/media/usb/au0828/au0828-dvb.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-dvb.c b/drivers/media/usb/au0828/au0828-dvb.c
index ee45990c0be1..bc4ea5397e92 100644
--- a/drivers/media/usb/au0828/au0828-dvb.c
+++ b/drivers/media/usb/au0828/au0828-dvb.c
@@ -267,7 +267,7 @@ static int au0828_dvb_start_feed(struct dvb_demux_feed *feed)
 	if (!demux->dmx.frontend)
 		return -EINVAL;
 
-	if (dvb) {
+	if (dvb->frontend) {
 		mutex_lock(&dvb->lock);
 		dvb->start_count++;
 		dprintk(1, "%s(), start_count: %d, stop_count: %d\n", __func__,
@@ -296,7 +296,7 @@ static int au0828_dvb_stop_feed(struct dvb_demux_feed *feed)
 
 	dprintk(1, "%s()\n", __func__);
 
-	if (dvb) {
+	if (dvb->frontend) {
 		cancel_work_sync(&dev->restart_streaming);
 
 		mutex_lock(&dvb->lock);
@@ -526,8 +526,7 @@ void au0828_dvb_unregister(struct au0828_dev *dev)
 		for (i = 0; i < URB_COUNT; i++)
 			kfree(dev->dig_transfer_buffer[i]);
 	}
-
-
+	dvb->frontend = NULL;
 }
 
 /* All the DVB attach calls go here, this function get's modified
@@ -608,6 +607,7 @@ int au0828_dvb_register(struct au0828_dev *dev)
 	if (ret < 0) {
 		if (dvb->frontend->ops.release)
 			dvb->frontend->ops.release(dvb->frontend);
+		dvb->frontend = NULL;
 		return ret;
 	}
 
@@ -618,7 +618,7 @@ void au0828_dvb_suspend(struct au0828_dev *dev)
 {
 	struct au0828_dvb *dvb = &dev->dvb;
 
-	if (dvb && dev->urb_streaming) {
+	if (dvb->frontend && dev->urb_streaming) {
 		pr_info("stopping DVB\n");
 
 		cancel_work_sync(&dev->restart_streaming);
@@ -635,7 +635,7 @@ void au0828_dvb_resume(struct au0828_dev *dev)
 {
 	struct au0828_dvb *dvb = &dev->dvb;
 
-	if (dvb && dev->urb_streaming) {
+	if (dvb->frontend && dev->urb_streaming) {
 		pr_info("resuming DVB\n");
 
 		au0828_set_frontend(dvb->frontend);
-- 
1.9.3

[PATCH 2/3] au0828: Fix DVB resume when streaming

[Mauro Carvalho Chehab]

When DVB is streaming and suspend is called, it will call
au0828_stop_transport(), with will clean the streaming flag.

Due to that, stop_urb_transfer() will be called twice,
causing an oops.

So, we need another flag to be used at resume, telling it
to restart DVB.

While here, add a logic at stop_urb_transfer() to prevent
it of being called twice, and convert the usb_streaming
flag into boolean.

Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
 drivers/media/usb/au0828/au0828-dvb.c | 14 +++++++++-----
 drivers/media/usb/au0828/au0828.h     |  4 ++--
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-dvb.c b/drivers/media/usb/au0828/au0828-dvb.c
index bc4ea5397e92..821f86e92cde 100644
--- a/drivers/media/usb/au0828/au0828-dvb.c
+++ b/drivers/media/usb/au0828/au0828-dvb.c
@@ -121,7 +121,7 @@ static void urb_completion(struct urb *purb)
 		return;
 	}
 
-	if (dev->urb_streaming == 0) {
+	if (!dev->urb_streaming) {
 		dprintk(2, "%s: not streaming!\n", __func__);
 		return;
 	}
@@ -159,7 +159,10 @@ static int stop_urb_transfer(struct au0828_dev *dev)
 
 	dprintk(2, "%s()\n", __func__);
 
-	dev->urb_streaming = 0;
+	if (!dev->urb_streaming)
+		return 0;
+
+	dev->urb_streaming = false;
 	for (i = 0; i < URB_COUNT; i++) {
 		if (dev->urbs[i]) {
 			usb_kill_urb(dev->urbs[i]);
@@ -229,7 +232,7 @@ static int start_urb_transfer(struct au0828_dev *dev)
 		}
 	}
 
-	dev->urb_streaming = 1;
+	dev->urb_streaming = true;
 	ret = 0;
 
 err:
@@ -323,7 +326,7 @@ static void au0828_restart_dvb_streaming(struct work_struct *work)
 					      restart_streaming);
 	struct au0828_dvb *dvb = &dev->dvb;
 
-	if (dev->urb_streaming == 0)
+	if (!dev->urb_streaming)
 		return;
 
 	dprintk(1, "Restarting streaming...!\n");
@@ -628,6 +631,7 @@ void au0828_dvb_suspend(struct au0828_dev *dev)
 		stop_urb_transfer(dev);
 		au0828_stop_transport(dev, 1);
 		mutex_unlock(&dvb->lock);
+		dev->need_urb_start = 1;
 	}
 }
 
@@ -635,7 +639,7 @@ void au0828_dvb_resume(struct au0828_dev *dev)
 {
 	struct au0828_dvb *dvb = &dev->dvb;
 
-	if (dvb->frontend && dev->urb_streaming) {
+	if (dvb->frontend && dev->need_urb_start) {
 		pr_info("resuming DVB\n");
 
 		au0828_set_frontend(dvb->frontend);
diff --git a/drivers/media/usb/au0828/au0828.h b/drivers/media/usb/au0828/au0828.h
index d187129b96b7..a7cc6e397fdd 100644
--- a/drivers/media/usb/au0828/au0828.h
+++ b/drivers/media/usb/au0828/au0828.h
@@ -267,8 +267,8 @@ struct au0828_dev {
 	char *transfer_buffer[AU0828_MAX_ISO_BUFS];/* transfer buffers for isoc
 						   transfer */
 
-	/* USB / URB Related */
-	int		urb_streaming;
+	/* DVB USB / URB Related */
+	bool		urb_streaming, need_urb_start;
 	struct urb	*urbs[URB_COUNT];
 
 	/* Preallocated transfer digital transfer buffers */
-- 
1.9.3

[PATCH 3/3] xc5000: be sure that the firmware is there before set params

[Mauro Carvalho Chehab]

Now that xc5000_set_params() is also called during resume,
move the code that checks for the firmware to happen there.

This way, the firmware will be loaded either for analog or
digital TV when .resume callback is called.

Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
 drivers/media/tuners/xc5000.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
index c1905784b08e..512fe508bcd2 100644
--- a/drivers/media/tuners/xc5000.c
+++ b/drivers/media/tuners/xc5000.c
@@ -1040,6 +1040,11 @@ static int xc5000_set_params(struct dvb_frontend *fe)
 {
 	struct xc5000_priv *priv = fe->tuner_priv;
 
+	if (xc_load_fw_and_init_tuner(fe, 0) != 0) {
+		dprintk(1, "Unable to load firmware and init tuner\n");
+		return -EINVAL;
+	}
+
 	switch (priv->mode) {
 	case V4L2_TUNER_RADIO:
 		return xc5000_set_radio_freq(fe);
@@ -1061,11 +1066,6 @@ static int xc5000_set_analog_params(struct dvb_frontend *fe,
 	if (priv->i2c_props.adap == NULL)
 		return -EINVAL;
 
-	if (xc_load_fw_and_init_tuner(fe, 0) != 0) {
-		dprintk(1, "Unable to load firmware and init tuner\n");
-		return -EINVAL;
-	}
-
 	switch (params->mode) {
 	case V4L2_TUNER_RADIO:
 		ret = xc5000_config_radio(fe, params);
-- 
1.9.3

Re: [PATCH 0/3] Another series of PM fixes for au0828

[Hans Verkuil]

Hi Mauro,

The following are just some general remarks regarding PM and au0828, it's
not specific to this patch series. I'm just brainstorming here...

It's unfortunate that the au0828 isn't using vb2 yet. I would be interested in
seeing what can be done in vb2 to help implement suspend/resume. Basically vb2
has already most (all?) of the information it needs to handle this. Ideally all
you need to do in the driver is to call vb2_suspend or vb2_resume and vb2 will
take care of the rest, calling start/stop_streaming as needed. Some work would
have to be done there to ensure buffers are queued/dequeued to the right queues
and in the right state.

So vb2 would handle all the DMA/streaming aspects of suspend/resume, thus
simplifying the driver.

Is it perhaps an idea to convert au0828 to vb2 in order to pursue this further?

Besides, converting to vb2 tends to get rid of a substantial amount of code
which makes it much easier to work with.

Regards,

	Hans

On 08/10/2014 04:14 AM, Mauro Carvalho Chehab wrote:
> There are still a few bugs that can happen when suspending and
> a video stream is active. This patch series fix them. After
> that, resume works fine, even it suspend happened while
> streaming.
> 
> There is one remaining issue though: xc5000 firmware doesn't
> load after resume.
> 
> What happens (on both analog and digital) is:
> 
> [  143.071323] xc5000: xc5000_suspend()
> [  143.071324] xc5000: xc5000_tuner_reset()
> [  143.099992] au0828: Suspend
> [  143.099992] au0828: Stopping RC
> [  143.101694] au0828: stopping V4L2
> [  143.101695] au0828: stopping V4L2 active URBs
> [  144.988637] au0828: Resume
> [  145.342026] au0828: Restarting RC
> [  145.343296] au0828: restarting V4L2
> [  145.464413] xc5000: xc5000_is_firmware_loaded() returns True id = 0xffff
> [  145.464414] xc5000: xc_set_signal_source(1) Source = CABLE
> [  146.370861] xc5000: xc_set_signal_source(1) failed
> 
> I suspect that it has to do with a wrong value for the I2C
> gateway. The proper fix is likely to convert au0828 to use
> the I2C mux support, and remove the old i2c_gate_ctrl
> approach. However, such patch would require more work, to
> avoid breaking other drivers.
> 
> Mauro Carvalho Chehab (3):
>   au0828: fix checks if dvb is initialized
>   au0828: Fix DVB resume when streaming
>   xc5000: be sure that the firmware is there before set params
> 
>  drivers/media/tuners/xc5000.c         | 10 +++++-----
>  drivers/media/usb/au0828/au0828-dvb.c | 24 ++++++++++++++----------
>  drivers/media/usb/au0828/au0828.h     |  4 ++--
>  3 files changed, 21 insertions(+), 17 deletions(-)
> 

Re: [PATCH 0/3] Another series of PM fixes for au0828

[Mauro Carvalho Chehab]

Hi Hans,

Em Sun, 10 Aug 2014 17:16:17 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> Hi Mauro,
> 
> The following are just some general remarks regarding PM and au0828, it's
> not specific to this patch series. I'm just brainstorming here...
> 
> It's unfortunate that the au0828 isn't using vb2 yet. I would be interested in
> seeing what can be done in vb2 to help implement suspend/resume. Basically vb2
> has already most (all?) of the information it needs to handle this. Ideally all
> you need to do in the driver is to call vb2_suspend or vb2_resume and vb2 will
> take care of the rest, calling start/stop_streaming as needed. Some work would
> have to be done there to ensure buffers are queued/dequeued to the right queues
> and in the right state.

I guess one of the problems with any core-provided method is to ensure that
the tuner is initialized and locked before it, in order to avoid filling
buffers from the wrong channel.

Btw, this is one unsolved problem that we face with PM on media: we
need to assure that resume will follow a precise init order:
	- All core subsystems are initialized before everything else;
	- The firmwares are loaded;
	- The I2C gates are in the right state;
	- The tuner is set;
	- The gpio's are properly configured;
	- The analog and/or the digital demodulator are properly
	  configured;

Only after that, the driver (and VB2) can be resumed.

A proper PM support will require lots of changes at the media core, and
likely on other subsystems. We (I, Shuah and some people I'm hiring)
are looking into those issues, but a proper fix with proper core support
will take some time.

> So vb2 would handle all the DMA/streaming aspects of suspend/resume, thus
> simplifying the driver.

The changes will be minimal. Just one patch would be affected:
	https://patchwork.linuxtv.org/patch/25277/

IMHO, for USB drivers, the best strategy for suspend/resume is the one
taken on au0828, e. g. at suspend:
	- stop DMA;
	- cancel the pending URB;
	- stop any pending kthread;
	- call vb2_discard_done() to discard any pending buffers
	  (I think VB1 doesn't have anything similar);

And, at resume, restart them.

We could provide a core support to cancel the pending URBs, but most 
of the stuff are driver-specific, because the core doesn't have
any code to handle USB streams on a generic way (well, gspca has,
but it doesn't cover the DVB specifics).

I started writing a URB handling abstraction for the tm6000 driver
that I wanted to add at the core, but I didn't finish making it
generic enough. Moving it to core and porting the existing drivers
to use it would take a lot of time and effort. Not sure if it is
worth nowadays.

What I'm trying to say is that most of the issues related to
suspend/resume aren't at the streaming engine (being VB1, VB2
or a driver-specific one). Once we fix the main issues subsystem-wide,
then we can have a clearer view if are there anything we could do at
VB2 level.

> Is it perhaps an idea to convert au0828 to vb2 in order to pursue this further?
> 
> Besides, converting to vb2 tends to get rid of a substantial amount of code
> which makes it much easier to work with.

I don't think that converting au0828 to vb2 would help to improve
the suspend issues. Ok, should do it anyway at long term, as we want
one day to get rid of VB1, but we should take some care with this driver.
It has workarounds for several hardware bugs that cause the stream to
stop working under not well known situations. There are even two
threads there to detect and apply such workarounds when stream
suddenly stops without a (known) reason.

Regards,
Mauro

Re: [PATCH 0/3] Another series of PM fixes for au0828

[Hans Verkuil]

On 08/10/2014 07:05 PM, Mauro Carvalho Chehab wrote:
> Hi Hans,
> 
> Em Sun, 10 Aug 2014 17:16:17 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> Hi Mauro,
>>
>> The following are just some general remarks regarding PM and au0828, it's
>> not specific to this patch series. I'm just brainstorming here...
>>
>> It's unfortunate that the au0828 isn't using vb2 yet. I would be interested in
>> seeing what can be done in vb2 to help implement suspend/resume. Basically vb2
>> has already most (all?) of the information it needs to handle this. Ideally all
>> you need to do in the driver is to call vb2_suspend or vb2_resume and vb2 will
>> take care of the rest, calling start/stop_streaming as needed. Some work would
>> have to be done there to ensure buffers are queued/dequeued to the right queues
>> and in the right state.
> 
> I guess one of the problems with any core-provided method is to ensure that
> the tuner is initialized and locked before it, in order to avoid filling
> buffers from the wrong channel.
> 
> Btw, this is one unsolved problem that we face with PM on media: we
> need to assure that resume will follow a precise init order:
> 	- All core subsystems are initialized before everything else;
> 	- The firmwares are loaded;
> 	- The I2C gates are in the right state;
> 	- The tuner is set;
> 	- The gpio's are properly configured;
> 	- The analog and/or the digital demodulator are properly
> 	  configured;
> 
> Only after that, the driver (and VB2) can be resumed.

Those items are certainly out-of-scope of vb2 and are device specific.

> A proper PM support will require lots of changes at the media core, and
> likely on other subsystems. We (I, Shuah and some people I'm hiring)
> are looking into those issues, but a proper fix with proper core support
> will take some time.
> 
>> So vb2 would handle all the DMA/streaming aspects of suspend/resume, thus
>> simplifying the driver.
> 
> The changes will be minimal. Just one patch would be affected:
> 	https://patchwork.linuxtv.org/patch/25277/
> 
> IMHO, for USB drivers, the best strategy for suspend/resume is the one
> taken on au0828, e. g. at suspend:
> 	- stop DMA;
> 	- cancel the pending URB;
> 	- stop any pending kthread;
> 	- call vb2_discard_done() to discard any pending buffers
> 	  (I think VB1 doesn't have anything similar);

With proper vb2 support stop DMA, cancel pending USB and vb2_discard_done
would all be done by vb2. The first two would be done in the stop_streaming
vb2 op.

> 
> And, at resume, restart them.
> 
> We could provide a core support to cancel the pending URBs, but most 
> of the stuff are driver-specific, because the core doesn't have
> any code to handle USB streams on a generic way (well, gspca has,
> but it doesn't cover the DVB specifics).
> 
> I started writing a URB handling abstraction for the tm6000 driver
> that I wanted to add at the core, but I didn't finish making it
> generic enough. Moving it to core and porting the existing drivers
> to use it would take a lot of time and effort. Not sure if it is
> worth nowadays.
> 
> What I'm trying to say is that most of the issues related to
> suspend/resume aren't at the streaming engine (being VB1, VB2
> or a driver-specific one). Once we fix the main issues subsystem-wide,
> then we can have a clearer view if are there anything we could do at
> VB2 level.
> 
>> Is it perhaps an idea to convert au0828 to vb2 in order to pursue this further?
>>
>> Besides, converting to vb2 tends to get rid of a substantial amount of code
>> which makes it much easier to work with.
> 
> I don't think that converting au0828 to vb2 would help to improve
> the suspend issues. Ok, should do it anyway at long term, as we want
> one day to get rid of VB1, but we should take some care with this driver.
> It has workarounds for several hardware bugs that cause the stream to
> stop working under not well known situations. There are even two
> threads there to detect and apply such workarounds when stream
> suddenly stops without a (known) reason.

These types of weird errors are exactly why I would recommend to port to
vb2. The sequence of events is very clear and systematic in vb2, whereas it
will drive you bonkers if you try to understand the flow in vb1. I do not
trust *any* driver that uses vb1. There is no way drivers can cover all
corner cases since vb1 is one of the craziest pieces of code I've ever seen.
Especially since it doesn't look crazy at first glance, that's what makes
it such a nasty framework.

I've ported quite a few drivers by now to vb2 and in all cases things that
used to be flaky (or just plain broken) suddenly started working. This may
or may not apply to au0828, but it will certainly make the code a lot
easier to understand. Particularly crucial steps such as when the streaming
starts and stops is very well defined in vb2. Ditto for resource handling (who
'owns' the stream) which is handled by vb2 as well.

Just my 5 cents, of course.

Regards,

	Hans

Re: [PATCH 0/3] Another series of PM fixes for au0828

[Mauro Carvalho Chehab]

Em Sun, 10 Aug 2014 19:30:59 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 08/10/2014 07:05 PM, Mauro Carvalho Chehab wrote:
> > Hi Hans,
> > 
> > Em Sun, 10 Aug 2014 17:16:17 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > 
> >> Hi Mauro,
> >>
> >> The following are just some general remarks regarding PM and au0828, it's
> >> not specific to this patch series. I'm just brainstorming here...
> >>
> >> It's unfortunate that the au0828 isn't using vb2 yet. I would be interested in
> >> seeing what can be done in vb2 to help implement suspend/resume. Basically vb2
> >> has already most (all?) of the information it needs to handle this. Ideally all
> >> you need to do in the driver is to call vb2_suspend or vb2_resume and vb2 will
> >> take care of the rest, calling start/stop_streaming as needed. Some work would
> >> have to be done there to ensure buffers are queued/dequeued to the right queues
> >> and in the right state.
> > 
> > I guess one of the problems with any core-provided method is to ensure that
> > the tuner is initialized and locked before it, in order to avoid filling
> > buffers from the wrong channel.
> > 
> > Btw, this is one unsolved problem that we face with PM on media: we
> > need to assure that resume will follow a precise init order:
> > 	- All core subsystems are initialized before everything else;
> > 	- The firmwares are loaded;
> > 	- The I2C gates are in the right state;
> > 	- The tuner is set;
> > 	- The gpio's are properly configured;
> > 	- The analog and/or the digital demodulator are properly
> > 	  configured;
> > 
> > Only after that, the driver (and VB2) can be resumed.
> 
> Those items are certainly out-of-scope of vb2 and are device specific.
> 
> > A proper PM support will require lots of changes at the media core, and
> > likely on other subsystems. We (I, Shuah and some people I'm hiring)
> > are looking into those issues, but a proper fix with proper core support
> > will take some time.
> > 
> >> So vb2 would handle all the DMA/streaming aspects of suspend/resume, thus
> >> simplifying the driver.
> > 
> > The changes will be minimal. Just one patch would be affected:
> > 	https://patchwork.linuxtv.org/patch/25277/
> > 
> > IMHO, for USB drivers, the best strategy for suspend/resume is the one
> > taken on au0828, e. g. at suspend:
> > 	- stop DMA;
> > 	- cancel the pending URB;
> > 	- stop any pending kthread;
> > 	- call vb2_discard_done() to discard any pending buffers
> > 	  (I think VB1 doesn't have anything similar);
> 
> With proper vb2 support stop DMA, cancel pending USB and vb2_discard_done
> would all be done by vb2. The first two would be done in the stop_streaming
> vb2 op.

stop streaming will also free the buffers. This is not needed at
suspend, and would require to reallocate them at resume.

If you saw the code on the above patch, the suspend code is actually
just a few lines of the code: 

+	if (dev->stream_state == STREAM_ON) {
+		au0828_analog_stream_disable(dev);
+		/* stop urbs */
+		for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {
+			urb = dev->isoc_ctl.urb[i];
+			if (urb) {
+				if (!irqs_disabled())
+					usb_kill_urb(urb);
+				else
+					usb_unlink_urb(urb);
+			}
+		}
+	}
+
+	if (dev->vid_timeout_running)
+		del_timer_sync(&dev->vid_timeout);
+	if (dev->vbi_timeout_running)
+		del_timer_sync(&dev->vbi_timeout);
+}

Where the del_timer_sync() on au0828 is just due to stop the
hardware bug workaround code on this specific driver.

The resume is about the same size. No need to free buffers.
This is a way less things than calling stop at suspend and
start at resume.

> > 
> > And, at resume, restart them.
> > 
> > We could provide a core support to cancel the pending URBs, but most 
> > of the stuff are driver-specific, because the core doesn't have
> > any code to handle USB streams on a generic way (well, gspca has,
> > but it doesn't cover the DVB specifics).
> > 
> > I started writing a URB handling abstraction for the tm6000 driver
> > that I wanted to add at the core, but I didn't finish making it
> > generic enough. Moving it to core and porting the existing drivers
> > to use it would take a lot of time and effort. Not sure if it is
> > worth nowadays.
> > 
> > What I'm trying to say is that most of the issues related to
> > suspend/resume aren't at the streaming engine (being VB1, VB2
> > or a driver-specific one). Once we fix the main issues subsystem-wide,
> > then we can have a clearer view if are there anything we could do at
> > VB2 level.
> > 
> >> Is it perhaps an idea to convert au0828 to vb2 in order to pursue this further?
> >>
> >> Besides, converting to vb2 tends to get rid of a substantial amount of code
> >> which makes it much easier to work with.
> > 
> > I don't think that converting au0828 to vb2 would help to improve
> > the suspend issues. Ok, should do it anyway at long term, as we want
> > one day to get rid of VB1, but we should take some care with this driver.
> > It has workarounds for several hardware bugs that cause the stream to
> > stop working under not well known situations. There are even two
> > threads there to detect and apply such workarounds when stream
> > suddenly stops without a (known) reason.
> 
> These types of weird errors are exactly why I would recommend to port to
> vb2. The sequence of events is very clear and systematic in vb2, whereas it
> will drive you bonkers if you try to understand the flow in vb1. I do not
> trust *any* driver that uses vb1. There is no way drivers can cover all
> corner cases since vb1 is one of the craziest pieces of code I've ever seen.
> Especially since it doesn't look crazy at first glance, that's what makes
> it such a nasty framework.

Those weird errors are not due to VB1. They are due to a hardware bug
that sometimes misalign the DMA data at bit level. Even unload/reload
the driver sometimes is not enough to fix.

> I've ported quite a few drivers by now to vb2 and in all cases things that
> used to be flaky (or just plain broken) suddenly started working. This may
> or may not apply to au0828, but it will certainly make the code a lot
> easier to understand. Particularly crucial steps such as when the streaming
> starts and stops is very well defined in vb2. Ditto for resource handling (who
> 'owns' the stream) which is handled by vb2 as well.

I'm not against porting it to vb2. Just saying that it requires more
testing than usual, as the hardware is buggy.

I may eventually do it if I have some spare time next week.

Regards,
Mauro

> Just my 5 cents, of course.
> 
> Regards,
> 
> 	Hans

Re: [PATCH 0/3] Another series of PM fixes for au0828

[Hans Verkuil]

On 08/10/2014 08:09 PM, Mauro Carvalho Chehab wrote:
> Em Sun, 10 Aug 2014 19:30:59 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 08/10/2014 07:05 PM, Mauro Carvalho Chehab wrote:
>>> Hi Hans,
>>>
>>> Em Sun, 10 Aug 2014 17:16:17 +0200
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>
>>>> Hi Mauro,
>>>>
>>>> The following are just some general remarks regarding PM and au0828, it's
>>>> not specific to this patch series. I'm just brainstorming here...
>>>>
>>>> It's unfortunate that the au0828 isn't using vb2 yet. I would be interested in
>>>> seeing what can be done in vb2 to help implement suspend/resume. Basically vb2
>>>> has already most (all?) of the information it needs to handle this. Ideally all
>>>> you need to do in the driver is to call vb2_suspend or vb2_resume and vb2 will
>>>> take care of the rest, calling start/stop_streaming as needed. Some work would
>>>> have to be done there to ensure buffers are queued/dequeued to the right queues
>>>> and in the right state.
>>>
>>> I guess one of the problems with any core-provided method is to ensure that
>>> the tuner is initialized and locked before it, in order to avoid filling
>>> buffers from the wrong channel.
>>>
>>> Btw, this is one unsolved problem that we face with PM on media: we
>>> need to assure that resume will follow a precise init order:
>>> 	- All core subsystems are initialized before everything else;
>>> 	- The firmwares are loaded;
>>> 	- The I2C gates are in the right state;
>>> 	- The tuner is set;
>>> 	- The gpio's are properly configured;
>>> 	- The analog and/or the digital demodulator are properly
>>> 	  configured;
>>>
>>> Only after that, the driver (and VB2) can be resumed.
>>
>> Those items are certainly out-of-scope of vb2 and are device specific.
>>
>>> A proper PM support will require lots of changes at the media core, and
>>> likely on other subsystems. We (I, Shuah and some people I'm hiring)
>>> are looking into those issues, but a proper fix with proper core support
>>> will take some time.
>>>
>>>> So vb2 would handle all the DMA/streaming aspects of suspend/resume, thus
>>>> simplifying the driver.
>>>
>>> The changes will be minimal. Just one patch would be affected:
>>> 	https://patchwork.linuxtv.org/patch/25277/
>>>
>>> IMHO, for USB drivers, the best strategy for suspend/resume is the one
>>> taken on au0828, e. g. at suspend:
>>> 	- stop DMA;
>>> 	- cancel the pending URB;
>>> 	- stop any pending kthread;
>>> 	- call vb2_discard_done() to discard any pending buffers
>>> 	  (I think VB1 doesn't have anything similar);
>>
>> With proper vb2 support stop DMA, cancel pending USB and vb2_discard_done
>> would all be done by vb2. The first two would be done in the stop_streaming
>> vb2 op.
> 
> stop streaming will also free the buffers. This is not needed at
> suspend, and would require to reallocate them at resume.

No, stop_streaming doesn't free buffers. It stops the DMA/streaming and returns
any active (i.e. owned by the driver) buffers to the vb2 core. Only after
calling reqbufs(0) or when the filehandle is released are buffers freed.

> 
> If you saw the code on the above patch, the suspend code is actually
> just a few lines of the code: 

I saw it :-)

But it is still a second place where you have to stop/start streaming.
If it is all centralized in one place, then that reduces the chances
of bugs. Also, for other drivers you may have to do a lot more, of
course.

> 
> +	if (dev->stream_state == STREAM_ON) {
> +		au0828_analog_stream_disable(dev);
> +		/* stop urbs */
> +		for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {
> +			urb = dev->isoc_ctl.urb[i];
> +			if (urb) {
> +				if (!irqs_disabled())
> +					usb_kill_urb(urb);
> +				else
> +					usb_unlink_urb(urb);
> +			}
> +		}
> +	}
> +
> +	if (dev->vid_timeout_running)
> +		del_timer_sync(&dev->vid_timeout);
> +	if (dev->vbi_timeout_running)
> +		del_timer_sync(&dev->vbi_timeout);
> +}
> 
> Where the del_timer_sync() on au0828 is just due to stop the
> hardware bug workaround code on this specific driver.
> 
> The resume is about the same size. No need to free buffers.
> This is a way less things than calling stop at suspend and
> start at resume.
> 
>>>
>>> And, at resume, restart them.
>>>
>>> We could provide a core support to cancel the pending URBs, but most 
>>> of the stuff are driver-specific, because the core doesn't have
>>> any code to handle USB streams on a generic way (well, gspca has,
>>> but it doesn't cover the DVB specifics).
>>>
>>> I started writing a URB handling abstraction for the tm6000 driver
>>> that I wanted to add at the core, but I didn't finish making it
>>> generic enough. Moving it to core and porting the existing drivers
>>> to use it would take a lot of time and effort. Not sure if it is
>>> worth nowadays.
>>>
>>> What I'm trying to say is that most of the issues related to
>>> suspend/resume aren't at the streaming engine (being VB1, VB2
>>> or a driver-specific one). Once we fix the main issues subsystem-wide,
>>> then we can have a clearer view if are there anything we could do at
>>> VB2 level.
>>>
>>>> Is it perhaps an idea to convert au0828 to vb2 in order to pursue this further?
>>>>
>>>> Besides, converting to vb2 tends to get rid of a substantial amount of code
>>>> which makes it much easier to work with.
>>>
>>> I don't think that converting au0828 to vb2 would help to improve
>>> the suspend issues. Ok, should do it anyway at long term, as we want
>>> one day to get rid of VB1, but we should take some care with this driver.
>>> It has workarounds for several hardware bugs that cause the stream to
>>> stop working under not well known situations. There are even two
>>> threads there to detect and apply such workarounds when stream
>>> suddenly stops without a (known) reason.
>>
>> These types of weird errors are exactly why I would recommend to port to
>> vb2. The sequence of events is very clear and systematic in vb2, whereas it
>> will drive you bonkers if you try to understand the flow in vb1. I do not
>> trust *any* driver that uses vb1. There is no way drivers can cover all
>> corner cases since vb1 is one of the craziest pieces of code I've ever seen.
>> Especially since it doesn't look crazy at first glance, that's what makes
>> it such a nasty framework.
> 
> Those weird errors are not due to VB1. They are due to a hardware bug
> that sometimes misalign the DMA data at bit level. Even unload/reload
> the driver sometimes is not enough to fix.

Yuck. 

> 
>> I've ported quite a few drivers by now to vb2 and in all cases things that
>> used to be flaky (or just plain broken) suddenly started working. This may
>> or may not apply to au0828, but it will certainly make the code a lot
>> easier to understand. Particularly crucial steps such as when the streaming
>> starts and stops is very well defined in vb2. Ditto for resource handling (who
>> 'owns' the stream) which is handled by vb2 as well.
> 
> I'm not against porting it to vb2. Just saying that it requires more
> testing than usual, as the hardware is buggy.
> 
> I may eventually do it if I have some spare time next week.

That would be nice.

Regards,

	Hans