Em Wed, 29 Oct 2014 08:54:20 +0100
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> On 10/28/2014 07:58 PM, Mauro Carvalho Chehab wrote:
> > Em Sat, 20 Sep 2014 14:41:37 +0200
> > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> >
> >> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >>
> >> This list is used some buffers have a different format, but that can
> >> never happen. Remove it and all associated code.
>
> Urgh. Can you fix the commit log to:
>
> "This list is only used if the width, height and/or format of a buffer has
> changed, but that can never happen. Remove it and all associated code."
>
> At least that's proper English :-)
Well, change it and resubmit ;) - After our discussions, of course.
>
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >> ---
> >> drivers/media/pci/cx88/cx88-mpeg.c | 31 -----------------------------
> >> drivers/media/pci/cx88/cx88-video.c | 39 +++----------------------------------
> >> drivers/media/pci/cx88/cx88.h | 1 -
> >> 3 files changed, 3 insertions(+), 68 deletions(-)
> >>
> >> diff --git a/drivers/media/pci/cx88/cx88-mpeg.c b/drivers/media/pci/cx88/cx88-mpeg.c
> >> index 2803b6f..5f59901 100644
> >> --- a/drivers/media/pci/cx88/cx88-mpeg.c
> >> +++ b/drivers/media/pci/cx88/cx88-mpeg.c
> >> @@ -210,37 +210,7 @@ static int cx8802_restart_queue(struct cx8802_dev *dev,
> >>
> >> dprintk( 1, "cx8802_restart_queue\n" );
> >> if (list_empty(&q->active))
> >> - {
> >> - struct cx88_buffer *prev;
> >> - prev = NULL;
> >> -
> >> - dprintk(1, "cx8802_restart_queue: queue is empty\n" );
> >
> > This is not bogus code. What happens here is that sometimes the DMA
> > engine stops on cx88 and it needs to be restarted under some temporary
> > errors.
> >
> > I don't remember the exact condition, as I don't touch on cx88 on
> > several years, but I think it happens when the signal drops (for
> > example, if the antenna cable gets removed, but not 100% sure).
> >
> > So, removing this code will cause regressions.
>
> No, it won't. Read carefully how the 'queued' list is used: the only time
> that an element is added to that list is in buffer_queue() if the width, height
> or format has changed while streaming. But that can never happen (at least,
> not after REQBUFS) and that case was removed in patch 01/16. So after patch
> 01/16 nobody is ever adding buffers to the queued list anymore and it can be
> removed.
>
> This patch has nothing to do with restarting DMA. Patch 04/16 is the one that
> removes the restarting of DMA.
>
> I did a fair amount of regression and duration testing to see if I could
> reproduce a stopped DMA without being able to. I'm fairly certain that included
> switching frequency between valid/invalid channels, but I can retry that just to
> be sure.
If I remember well, it is not switching between valid/invalid. That
happens if the signal is not too stable (e. g., either the chrominance
or luminance or audio carrier is not properly detected). You would likely
need a life weak signal and/or too much interference to be able to get it.
I'm pretty sure I got those "cx8802_restart_queue: " from time to time
without changing any parameters, during "normal" reception of some weak
channels where I used to live (basically, using internal antennas).
> I am very skeptical that this really has to do with DMA issues: it looks much more
> like a poorly written driver. As my commit log to patch 04 says the cx88 driver
> allows userspace to drain all buffers and at that moment it has to halt the DMA
> and restart when a new buffer is queued up. But it is much simpler and more
> robust to just keep streaming by always keeping one buffer around, just as almost
> all other non-usb drivers do.
>
> I have not been able to find any reports that actually mention that the DMA can
> stop. The same change was done to cx23885 and no reports of any DMA problems have
> been reported for that either, and I expect that both devices use very similar
> DMA IP.
Hmm... could be. I may try to test it with an internal antenna, but this can
take some time, as I'm currently trying to get rid of the patch backlog,
and I have other priorities to work after handling the backlog.
Regards,
Mauro
--
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
