On 08/10/2014 08:09 PM, Mauro Carvalho Chehab wrote: > Em Sun, 10 Aug 2014 19:30:59 +0200 > Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
