Em Wed, 03 Sep 2014 13:57:28 +0200
Hans Verkuil <hansverk@xxxxxxxxx> escreveu:
> Hi Mauro,
>
> On 09/03/14 13:32, Mauro Carvalho Chehab wrote:
> > Em Thu, 14 Aug 2014 11:54:00 +0200
> > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> >
> >> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >>
> >> As usual, this patch is very large due to the fact that half a vb2 conversion
> >> isn't possible. And since this affects 417, alsa, core, dvb, vbi and video the
> >> changes are all over.
> >>
> >> What made this more difficult was the peculiar way the risc program was setup.
> >> The driver allowed for running out of buffers in which case the DMA would stop
> >> and restart when the next buffer was queued. There was also a complicated
> >> timeout system for when buffers weren't filled. This was replaced by a much
> >> simpler scheme where there is always one buffer around and the DMA will just
> >> cycle that buffer until a new buffer is queued. In that case the previous
> >> buffer will be chained to the new buffer. An interrupt is generated at the
> >> start of the new buffer telling the driver that the previous buffer can be
> >> passed on to userspace.
> >>
> >> Much simpler and more robust. The old code seems to be copied from the
> >> cx88 driver. But it didn't fit the vb2 ops very well and replacing it with
> >> the new scheme made the code easier to understand. Not to mention that this
> >> patch removes 600 lines of code.
> >
> > Great job!
>
> Thank you.
>
> > Still, there are some issue. In special, the RISC changes should go
> > to a separate patch, as such changes have the potential of causing
> > some regressions. See below.
>
> I tried (I mentioned that in my git pull request), but I was not able
> to separate the two. I couldn't make the risc changes to work with vb1,
> and the reverse would be equally painful. Not only that, but I would
> have to test this twice (once with just the risc changes or vb2 changes,
> and again when both are in place).
>
> I can try to do the vb2 conversion first and the risc changes second, but
> this too might be too complicated to get to work.
Ok. Then, just split that VBI fixup on a separate patch.
Regards,
Mauro
>
> <snip>
>
> >> @@ -1320,7 +1373,6 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> >> struct v4l2_format *f)
> >> {
> >> struct cx23885_dev *dev = video_drvdata(file);
> >> - struct cx23885_fh *fh = file->private_data;
> >>
> >> f->fmt.pix.pixelformat = V4L2_PIX_FMT_MPEG;
> >> f->fmt.pix.bytesperline = 0;
> >> @@ -1329,9 +1381,9 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> >> f->fmt.pix.colorspace = 0;
> >> f->fmt.pix.width = dev->ts1.width;
> >> f->fmt.pix.height = dev->ts1.height;
> >> - f->fmt.pix.field = fh->mpegq.field;
> >> - dprintk(1, "VIDIOC_G_FMT: w: %d, h: %d, f: %d\n",
> >> - dev->ts1.width, dev->ts1.height, fh->mpegq.field);
> >> + f->fmt.pix.field = V4L2_FIELD_INTERLACED;
> >
> > Why? There are other supported formats, right?
>
> Not for the compressed video node. Only MPEG is supported there.
>
> >
> >> + dprintk(1, "VIDIOC_G_FMT: w: %d, h: %d\n",
> >> + dev->ts1.width, dev->ts1.height);
> >> return 0;
> >> }
> >>
> >> @@ -1339,15 +1391,15 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
> >> struct v4l2_format *f)
> >> {
> >> struct cx23885_dev *dev = video_drvdata(file);
> >> - struct cx23885_fh *fh = file->private_data;
> >>
> >> f->fmt.pix.pixelformat = V4L2_PIX_FMT_MPEG;
> >> f->fmt.pix.bytesperline = 0;
> >> f->fmt.pix.sizeimage =
> >> dev->ts1.ts_packet_size * dev->ts1.ts_packet_count;
> >> f->fmt.pix.colorspace = 0;
> >> - dprintk(1, "VIDIOC_TRY_FMT: w: %d, h: %d, f: %d\n",
> >> - dev->ts1.width, dev->ts1.height, fh->mpegq.field);
> >> + f->fmt.pix.field = V4L2_FIELD_INTERLACED;
> >
> > Why? There are other supported formats, right?
>
> Ditto.
>
> >
> >> + dprintk(1, "VIDIOC_TRY_FMT: w: %d, h: %d\n",
> >> + dev->ts1.width, dev->ts1.height);
> >> return 0;
> >> }
> >>
>
> <snip>
>
> >>
> >> int cx23885_sram_channel_setup(struct cx23885_dev *dev,
> >> @@ -482,8 +466,8 @@ int cx23885_sram_channel_setup(struct cx23885_dev *dev,
> >> lines = 6;
> >> BUG_ON(lines < 2);
> >>
> >> - cx_write(8 + 0, RISC_JUMP | RISC_IRQ1 | RISC_CNT_INC);
> >> - cx_write(8 + 4, 8);
> >> + cx_write(8 + 0, RISC_JUMP | RISC_CNT_RESET);
> >> + cx_write(8 + 4, 12);
> >
> > The above doesn't sound as being a pure vb2 conversion, and might cause
> > regressions, as we're changing the channel setups. I would very much
> > prefer to have such changes on a separate changeset, as it makes easier
> > to do bisect if ever needed.
>
> See my comment at the top.
>
> >
> >> cx_write(8 + 8, 0);
> >>
> >> /* write CDT */
> >> @@ -699,10 +683,6 @@ static int get_resources(struct cx23885_dev *dev)
> >> return -EBUSY;
> >> }
> >>
> >> -static void cx23885_timeout(unsigned long data);
> >> -int cx23885_risc_stopper(struct pci_dev *pci, struct btcx_riscmem *risc,
> >> - u32 reg, u32 mask, u32 value);
> >> -
> >> static int cx23885_init_tsport(struct cx23885_dev *dev,
> >> struct cx23885_tsport *port, int portno)
> >> {
> >> @@ -719,11 +699,6 @@ static int cx23885_init_tsport(struct cx23885_dev *dev,
> >> port->nr = portno;
> >>
> >> INIT_LIST_HEAD(&port->mpegq.active);
> >> - INIT_LIST_HEAD(&port->mpegq.queued);
> >> - port->mpegq.timeout.function = cx23885_timeout;
> >> - port->mpegq.timeout.data = (unsigned long)port;
> >> - init_timer(&port->mpegq.timeout);
> >> -
> >> mutex_init(&port->frontends.lock);
> >> INIT_LIST_HEAD(&port->frontends.felist);
> >> port->frontends.active_fe_id = 0;
> >> @@ -776,9 +751,6 @@ static int cx23885_init_tsport(struct cx23885_dev *dev,
> >> BUG();
> >> }
> >>
> >> - cx23885_risc_stopper(dev->pci, &port->mpegq.stopper,
> >> - port->reg_dma_ctl, port->dma_ctl_val, 0x00);
> >> -
> >> return 0;
> >> }
> >>
> >> @@ -1089,11 +1061,18 @@ static void cx23885_dev_unregister(struct cx23885_dev *dev)
> >> static __le32 *cx23885_risc_field(__le32 *rp, struct scatterlist *sglist,
> >> unsigned int offset, u32 sync_line,
> >> unsigned int bpl, unsigned int padding,
> >> - unsigned int lines, unsigned int lpi)
> >> + unsigned int lines, unsigned int lpi, bool jump)
> >> {
> >> struct scatterlist *sg;
> >> unsigned int line, todo, sol;
> >>
> >> +
> >> + if (jump) {
> >> + *(rp++) = cpu_to_le32(RISC_JUMP);
> >> + *(rp++) = cpu_to_le32(0);
> >> + *(rp++) = cpu_to_le32(0); /* bits 63-32 */
> >> + }
> >> +
> >
> > Here it seem clear: you're now adding a code to support different
> > frame interlacing layouts, but the best is to have such changes on
> > a separate changeset, as this is one thing that we may have troubles
> > in the future.
> >
> > The way I see is that we might start having a flood of complains about
> > regressions, and all of them will point to this single patch, making
> > really hard to identify what part of the change broke it.
> >
> > So, let's split those risc changes on a pre (or post) patch, making
> > easier if someone needs to report an issue, for us to track what
> > patch broke it.
> >
> >> /* sync instruction */
> >> if (sync_line != NO_SYNC_LINE)
> >> *(rp++) = cpu_to_le32(RISC_RESYNC | sync_line);
> >> @@ -1168,7 +1147,7 @@ int cx23885_risc_buffer(struct pci_dev *pci, struct btcx_riscmem *risc,
> >> /* write and jump need and extra dword */
> >> instructions = fields * (1 + ((bpl + padding) * lines)
> >> / PAGE_SIZE + lines);
> >> - instructions += 2;
> >> + instructions += 5;
> >> rc = btcx_riscmem_alloc(pci, risc, instructions*12);
> >> if (rc < 0)
> >> return rc;
> >> @@ -1177,10 +1156,10 @@ int cx23885_risc_buffer(struct pci_dev *pci, struct btcx_riscmem *risc,
> >> rp = risc->cpu;
> >> if (UNSET != top_offset)
> >> rp = cx23885_risc_field(rp, sglist, top_offset, 0,
> >> - bpl, padding, lines, 0);
> >> + bpl, padding, lines, 0, true);
> >> if (UNSET != bottom_offset)
> >> rp = cx23885_risc_field(rp, sglist, bottom_offset, 0x200,
> >> - bpl, padding, lines, 0);
> >> + bpl, padding, lines, 0, UNSET == top_offset);
> >>
> >> /* save pointer to jmp instruction address */
> >> risc->jmp = rp;
> >> @@ -1204,7 +1183,7 @@ int cx23885_risc_databuffer(struct pci_dev *pci,
> >> than PAGE_SIZE */
> >> /* Jump and write need an extra dword */
> >> instructions = 1 + (bpl * lines) / PAGE_SIZE + lines;
> >> - instructions += 1;
> >> + instructions += 4;
> >>
> >> rc = btcx_riscmem_alloc(pci, risc, instructions*12);
> >> if (rc < 0)
> >> @@ -1213,7 +1192,7 @@ int cx23885_risc_databuffer(struct pci_dev *pci,
> >> /* write risc instructions */
> >> rp = risc->cpu;
> >> rp = cx23885_risc_field(rp, sglist, 0, NO_SYNC_LINE,
> >> - bpl, 0, lines, lpi);
> >> + bpl, 0, lines, lpi, lpi == 0);
> >>
> >> /* save pointer to jmp instruction address */
> >> risc->jmp = rp;
> >> @@ -1243,7 +1222,7 @@ int cx23885_risc_vbibuffer(struct pci_dev *pci, struct btcx_riscmem *risc,
> >> /* write and jump need and extra dword */
> >> instructions = fields * (1 + ((bpl + padding) * lines)
> >> / PAGE_SIZE + lines);
> >> - instructions += 2;
> >> + instructions += 5;
> >> rc = btcx_riscmem_alloc(pci, risc, instructions*12);
> >> if (rc < 0)
> >> return rc;
> >> @@ -1253,12 +1232,12 @@ int cx23885_risc_vbibuffer(struct pci_dev *pci, struct btcx_riscmem *risc,
> >> /* Sync to line 6, so US CC line 21 will appear in line '12'
> >> * in the userland vbi payload */
> >> if (UNSET != top_offset)
> >> - rp = cx23885_risc_field(rp, sglist, top_offset, 6,
> >> - bpl, padding, lines, 0);
> >> + rp = cx23885_risc_field(rp, sglist, top_offset, 0,
> >> + bpl, padding, lines, 0, true);
> >>
> >> if (UNSET != bottom_offset)
> >> - rp = cx23885_risc_field(rp, sglist, bottom_offset, 0x207,
> >> - bpl, padding, lines, 0);
> >> + rp = cx23885_risc_field(rp, sglist, bottom_offset, 0x200,
> >> + bpl, padding, lines, 0, UNSET == top_offset);
> >
> > Why to change the 4th argument of cx23885_risc_field() call?
>
> This was a bug. Hmm, perhaps that should be moved to a separate patch.
> The VBI offset was wrong without this.
>
> >
> >>
> >>
> >>
> >> @@ -1269,38 +1248,10 @@ int cx23885_risc_vbibuffer(struct pci_dev *pci, struct btcx_riscmem *risc,
> >> }
> >>
> >>
> >> -int cx23885_risc_stopper(struct pci_dev *pci, struct btcx_riscmem *risc,
> >> - u32 reg, u32 mask, u32 value)
> >
> > What happened with this function?
>
> No longer needed after the risc changes.
>
> 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
