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

[Hans Verkuil <hverkuil@xs4all.nl>]

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