| Message ID | 1457543851-17823-1-git-send-email-robert.jarzmik@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Robert, A quick review below. I assume this is the first step to moving the pxa_camera driver out of soc-camera? On 03/09/2016 06:17 PM, Robert Jarzmik wrote: > Convert pxa_camera from videobuf to videobuf2. > > As the soc_camera was already compatible with videobuf2, the port is > quite straightforward. > > The only special port of this code is that if the vb2 to prepare is "too > big" in terms of size for the new capture format, the pxa_camera will > accept it anyway, and adapt the dmaengine transfer accordingly. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > --- > I'm not very sure about the locking of the video buffers yet. > --- > drivers/media/platform/soc_camera/Kconfig | 4 +- > drivers/media/platform/soc_camera/pxa_camera.c | 598 ++++++++++++------------- > 2 files changed, 290 insertions(+), 312 deletions(-) > > diff --git a/drivers/media/platform/soc_camera/Kconfig b/drivers/media/platform/soc_camera/Kconfig > index e5e2d6cf6638..39fd9f2d3ca8 100644 > --- a/drivers/media/platform/soc_camera/Kconfig > +++ b/drivers/media/platform/soc_camera/Kconfig > @@ -28,8 +28,8 @@ config VIDEO_MX3 > > config VIDEO_PXA27x > tristate "PXA27x Quick Capture Interface driver" > - depends on VIDEO_DEV && PXA27x && SOC_CAMERA > - select VIDEOBUF_DMA_SG > + depends on VIDEO_DEV && PXA27x && SOC_CAMERA && HAS_DMA > + select VIDEOBUF2_DMA_SG > select SG_SPLIT > ---help--- > This is a v4l2 driver for the PXA27x Quick Capture Interface > diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c > index 2aaf4a8f71a0..141a5ba603a8 100644 > --- a/drivers/media/platform/soc_camera/pxa_camera.c > +++ b/drivers/media/platform/soc_camera/pxa_camera.c > @@ -34,7 +34,7 @@ > > #include <media/v4l2-common.h> > #include <media/v4l2-dev.h> > -#include <media/videobuf-dma-sg.h> > +#include <media/videobuf2-dma-sg.h> > #include <media/soc_camera.h> > #include <media/drv-intf/soc_mediabus.h> > #include <media/v4l2-of.h> > @@ -180,13 +180,16 @@ enum pxa_camera_active_dma { > /* buffer for one video frame */ > struct pxa_buffer { > /* common v4l buffer stuff -- must be first */ > - struct videobuf_buffer vb; > + struct vb2_v4l2_buffer vbuf; > + struct list_head queue; > u32 code; > + int nb_planes; > /* our descriptor lists for Y, U and V channels */ > struct dma_async_tx_descriptor *descs[3]; > dma_cookie_t cookie[3]; > struct scatterlist *sg[3]; > int sg_len[3]; > + size_t plane_sizes[3]; > int inwork; > enum pxa_camera_active_dma active_dma; > }; > @@ -222,6 +225,8 @@ struct pxa_camera_dev { > struct tasklet_struct task_eof; > > u32 save_cicr[5]; > + > + void *alloc_ctx; > }; > > struct pxa_cam { > @@ -235,54 +240,16 @@ static unsigned int vid_limit = 16; /* Video memory limit, in Mb */ > /* > * Videobuf operations > */ > -static int pxa_videobuf_setup(struct videobuf_queue *vq, unsigned int *count, > - unsigned int *size) > +static struct pxa_buffer *vb2_to_pxa_buffer(struct vb2_buffer *vb) > { > - struct soc_camera_device *icd = vq->priv_data; > - > - dev_dbg(icd->parent, "count=%d, size=%d\n", *count, *size); > - > - *size = icd->sizeimage; > - > - if (0 == *count) > - *count = 32; > - if (*size * *count > vid_limit * 1024 * 1024) > - *count = (vid_limit * 1024 * 1024) / *size; > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > > - return 0; > + return container_of(vbuf, struct pxa_buffer, vbuf); > } > > -static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf) > +static struct device *pcdev_to_dev(struct pxa_camera_dev *pcdev) > { > - struct soc_camera_device *icd = vq->priv_data; > - struct videobuf_dmabuf *dma = videobuf_to_dma(&buf->vb); > - int i; > - > - BUG_ON(in_interrupt()); > - > - dev_dbg(icd->parent, "%s (vb=0x%p) 0x%08lx %d\n", __func__, > - &buf->vb, buf->vb.baddr, buf->vb.bsize); > - > - /* > - * This waits until this buffer is out of danger, i.e., until it is no > - * longer in STATE_QUEUED or STATE_ACTIVE > - */ > - videobuf_waiton(vq, &buf->vb, 0, 0); > - > - for (i = 0; i < 3 && buf->descs[i]; i++) { > - dmaengine_desc_free(buf->descs[i]); > - kfree(buf->sg[i]); > - buf->descs[i] = NULL; > - buf->sg[i] = NULL; > - buf->sg_len[i] = 0; > - } > - videobuf_dma_unmap(vq->dev, dma); > - videobuf_dma_free(dma); > - > - buf->vb.state = VIDEOBUF_NEEDS_INIT; > - > - dev_dbg(icd->parent, "%s end (vb=0x%p) 0x%08lx %d\n", __func__, > - &buf->vb, buf->vb.baddr, buf->vb.bsize); > + return pcdev->soc_host.v4l2_dev.dev; > } > > static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, > @@ -312,31 +279,26 @@ static void pxa_camera_dma_irq_v(void *data) > /** > * pxa_init_dma_channel - init dma descriptors > * @pcdev: pxa camera device > - * @buf: pxa buffer to find pxa dma channel > + * @vb: videobuffer2 buffer > * @dma: dma video buffer > * @channel: dma channel (0 => 'Y', 1 => 'U', 2 => 'V') > * @cibr: camera Receive Buffer Register > - * @size: bytes to transfer > - * @offset: offset in videobuffer of the first byte to transfer > * > * Prepares the pxa dma descriptors to transfer one camera channel. > * > * Returns 0 if success or -ENOMEM if no memory is available > */ > static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, > - struct pxa_buffer *buf, > - struct videobuf_dmabuf *dma, int channel, > - int cibr, int size, int offset) > + struct pxa_buffer *buf, int channel, > + struct scatterlist *sg, int sglen) > { > struct dma_chan *dma_chan = pcdev->dma_chans[channel]; > - struct scatterlist *sg = buf->sg[channel]; > - int sglen = buf->sg_len[channel]; > struct dma_async_tx_descriptor *tx; > > tx = dmaengine_prep_slave_sg(dma_chan, sg, sglen, DMA_DEV_TO_MEM, > DMA_PREP_INTERRUPT | DMA_CTRL_REUSE); > if (!tx) { > - dev_err(pcdev->soc_host.v4l2_dev.dev, > + dev_err(pcdev_to_dev(pcdev), > "dmaengine_prep_slave_sg failed\n"); > goto fail; > } > @@ -357,11 +319,9 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, > buf->descs[channel] = tx; > return 0; > fail: > - kfree(sg); > - > - dev_dbg(pcdev->soc_host.v4l2_dev.dev, > - "%s (vb=0x%p) dma_tx=%p\n", > - __func__, &buf->vb, tx); > + dev_dbg(pcdev_to_dev(pcdev), > + "%s (vb=%p) dma_tx=%p\n", > + __func__, buf, tx); > > return -ENOMEM; > } > @@ -374,129 +334,6 @@ static void pxa_videobuf_set_actdma(struct pxa_camera_dev *pcdev, > buf->active_dma |= DMA_U | DMA_V; > } > > -/* > - * Please check the DMA prepared buffer structure in : > - * Documentation/video4linux/pxa_camera.txt > - * Please check also in pxa_camera_check_link_miss() to understand why DMA chain > - * modification while DMA chain is running will work anyway. > - */ > -static int pxa_videobuf_prepare(struct videobuf_queue *vq, > - struct videobuf_buffer *vb, enum v4l2_field field) > -{ > - struct soc_camera_device *icd = vq->priv_data; > - struct soc_camera_host *ici = to_soc_camera_host(icd->parent); > - struct pxa_camera_dev *pcdev = ici->priv; > - struct device *dev = pcdev->soc_host.v4l2_dev.dev; > - struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb); > - int ret; > - int size_y, size_u = 0, size_v = 0; > - size_t sizes[3]; > - > - dev_dbg(dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, > - vb, vb->baddr, vb->bsize); > - > - /* Added list head initialization on alloc */ > - WARN_ON(!list_empty(&vb->queue)); > - > -#ifdef DEBUG > - /* > - * This can be useful if you want to see if we actually fill > - * the buffer with something > - */ > - memset((void *)vb->baddr, 0xaa, vb->bsize); > -#endif > - > - BUG_ON(NULL == icd->current_fmt); > - > - /* > - * I think, in buf_prepare you only have to protect global data, > - * the actual buffer is yours > - */ > - buf->inwork = 1; > - > - if (buf->code != icd->current_fmt->code || > - vb->width != icd->user_width || > - vb->height != icd->user_height || > - vb->field != field) { > - buf->code = icd->current_fmt->code; > - vb->width = icd->user_width; > - vb->height = icd->user_height; > - vb->field = field; > - vb->state = VIDEOBUF_NEEDS_INIT; > - } > - > - vb->size = icd->sizeimage; > - if (0 != vb->baddr && vb->bsize < vb->size) { > - ret = -EINVAL; > - goto out; > - } > - > - if (vb->state == VIDEOBUF_NEEDS_INIT) { > - int size = vb->size; > - struct videobuf_dmabuf *dma = videobuf_to_dma(vb); > - > - ret = videobuf_iolock(vq, vb, NULL); > - if (ret) > - goto out; > - > - if (pcdev->channels == 3) { > - size_y = size / 2; > - size_u = size_v = size / 4; > - } else { > - size_y = size; > - } > - > - sizes[0] = size_y; > - sizes[1] = size_u; > - sizes[2] = size_v; > - ret = sg_split(dma->sglist, dma->sglen, 0, pcdev->channels, > - sizes, buf->sg, buf->sg_len, GFP_KERNEL); > - if (ret < 0) { > - dev_err(dev, "sg_split failed: %d\n", ret); > - goto fail; > - } > - > - /* init DMA for Y channel */ > - ret = pxa_init_dma_channel(pcdev, buf, dma, 0, CIBR0, > - size_y, 0); > - if (ret) { > - dev_err(dev, "DMA initialization for Y/RGB failed\n"); > - goto fail; > - } > - > - /* init DMA for U channel */ > - if (size_u) > - ret = pxa_init_dma_channel(pcdev, buf, dma, 1, CIBR1, > - size_u, size_y); > - if (ret) { > - dev_err(dev, "DMA initialization for U failed\n"); > - goto fail; > - } > - > - /* init DMA for V channel */ > - if (size_v) > - ret = pxa_init_dma_channel(pcdev, buf, dma, 2, CIBR2, > - size_v, size_y + size_u); > - if (ret) { > - dev_err(dev, "DMA initialization for V failed\n"); > - goto fail; > - } > - > - vb->state = VIDEOBUF_PREPARED; > - } > - > - buf->inwork = 0; > - pxa_videobuf_set_actdma(pcdev, buf); > - > - return 0; > - > -fail: > - free_buffer(vq, buf); > -out: > - buf->inwork = 0; > - return ret; > -} > - > /** > * pxa_dma_start_channels - start DMA channel for active buffer > * @pcdev: pxa camera device > @@ -512,7 +349,7 @@ static void pxa_dma_start_channels(struct pxa_camera_dev *pcdev) > active = pcdev->active; > > for (i = 0; i < pcdev->channels; i++) { > - dev_dbg(pcdev->soc_host.v4l2_dev.dev, > + dev_dbg(pcdev_to_dev(pcdev), > "%s (channel=%d)\n", __func__, i); > dma_async_issue_pending(pcdev->dma_chans[i]); > } > @@ -523,7 +360,7 @@ static void pxa_dma_stop_channels(struct pxa_camera_dev *pcdev) > int i; > > for (i = 0; i < pcdev->channels; i++) { > - dev_dbg(pcdev->soc_host.v4l2_dev.dev, > + dev_dbg(pcdev_to_dev(pcdev), > "%s (channel=%d)\n", __func__, i); > dmaengine_terminate_all(pcdev->dma_chans[i]); > } > @@ -536,7 +373,7 @@ static void pxa_dma_add_tail_buf(struct pxa_camera_dev *pcdev, > > for (i = 0; i < pcdev->channels; i++) { > buf->cookie[i] = dmaengine_submit(buf->descs[i]); > - dev_dbg(pcdev->soc_host.v4l2_dev.dev, > + dev_dbg(pcdev_to_dev(pcdev), > "%s (channel=%d) : submit vb=%p cookie=%d\n", > __func__, i, buf, buf->descs[i]->cookie); > } > @@ -554,7 +391,7 @@ static void pxa_camera_start_capture(struct pxa_camera_dev *pcdev) > { > unsigned long cicr0; > > - dev_dbg(pcdev->soc_host.v4l2_dev.dev, "%s\n", __func__); > + dev_dbg(pcdev_to_dev(pcdev), "%s\n", __func__); > __raw_writel(__raw_readl(pcdev->base + CISR), pcdev->base + CISR); > /* Enable End-Of-Frame Interrupt */ > cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_ENB; > @@ -572,72 +409,20 @@ static void pxa_camera_stop_capture(struct pxa_camera_dev *pcdev) > __raw_writel(cicr0, pcdev->base + CICR0); > > pcdev->active = NULL; > - dev_dbg(pcdev->soc_host.v4l2_dev.dev, "%s\n", __func__); > -} > - > -/* Called under spinlock_irqsave(&pcdev->lock, ...) */ > -static void pxa_videobuf_queue(struct videobuf_queue *vq, > - struct videobuf_buffer *vb) > -{ > - struct soc_camera_device *icd = vq->priv_data; > - struct soc_camera_host *ici = to_soc_camera_host(icd->parent); > - struct pxa_camera_dev *pcdev = ici->priv; > - struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb); > - > - dev_dbg(icd->parent, "%s (vb=0x%p) 0x%08lx %d active=%p\n", > - __func__, vb, vb->baddr, vb->bsize, pcdev->active); > - > - list_add_tail(&vb->queue, &pcdev->capture); > - > - vb->state = VIDEOBUF_ACTIVE; > - pxa_dma_add_tail_buf(pcdev, buf); > - > - if (!pcdev->active) > - pxa_camera_start_capture(pcdev); > -} > - > -static void pxa_videobuf_release(struct videobuf_queue *vq, > - struct videobuf_buffer *vb) > -{ > - struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb); > -#ifdef DEBUG > - struct soc_camera_device *icd = vq->priv_data; > - struct device *dev = icd->parent; > - > - dev_dbg(dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, > - vb, vb->baddr, vb->bsize); > - > - switch (vb->state) { > - case VIDEOBUF_ACTIVE: > - dev_dbg(dev, "%s (active)\n", __func__); > - break; > - case VIDEOBUF_QUEUED: > - dev_dbg(dev, "%s (queued)\n", __func__); > - break; > - case VIDEOBUF_PREPARED: > - dev_dbg(dev, "%s (prepared)\n", __func__); > - break; > - default: > - dev_dbg(dev, "%s (unknown)\n", __func__); > - break; > - } > -#endif > - > - free_buffer(vq, buf); > + dev_dbg(pcdev_to_dev(pcdev), "%s\n", __func__); > } > > static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev, > - struct videobuf_buffer *vb, > struct pxa_buffer *buf) > { > + struct vb2_buffer *vb = &buf->vbuf.vb2_buf; > + > /* _init is used to debug races, see comment in pxa_camera_reqbufs() */ > - list_del_init(&vb->queue); > - vb->state = VIDEOBUF_DONE; > - v4l2_get_timestamp(&vb->ts); > - vb->field_count++; > - wake_up(&vb->done); > - dev_dbg(pcdev->soc_host.v4l2_dev.dev, "%s dequeud buffer (vb=0x%p)\n", > - __func__, vb); > + list_del_init(&buf->queue); > + vb->timestamp = ktime_get_ns(); > + vb2_buffer_done(vb, VB2_BUF_STATE_DONE); > + dev_dbg(pcdev_to_dev(pcdev), "%s dequeud buffer (buf=0x%p)\n", > + __func__, buf); > > if (list_empty(&pcdev->capture)) { > pxa_camera_stop_capture(pcdev); > @@ -645,7 +430,7 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev, > } > > pcdev->active = list_entry(pcdev->capture.next, > - struct pxa_buffer, vb.queue); > + struct pxa_buffer, queue); > } > > /** > @@ -670,7 +455,7 @@ static void pxa_camera_check_link_miss(struct pxa_camera_dev *pcdev, > { > bool is_dma_stopped = last_submitted != last_issued; > > - dev_dbg(pcdev->soc_host.v4l2_dev.dev, > + dev_dbg(pcdev_to_dev(pcdev), > "%s : top queued buffer=%p, is_dma_stopped=%d\n", > __func__, pcdev->active, is_dma_stopped); > > @@ -681,12 +466,11 @@ static void pxa_camera_check_link_miss(struct pxa_camera_dev *pcdev, > static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, > enum pxa_camera_active_dma act_dma) > { > - struct device *dev = pcdev->soc_host.v4l2_dev.dev; > + struct device *dev = pcdev_to_dev(pcdev); > struct pxa_buffer *buf, *last_buf; > unsigned long flags; > u32 camera_status, overrun; > int chan; > - struct videobuf_buffer *vb; > enum dma_status last_status; > dma_cookie_t last_issued; > > @@ -714,9 +498,8 @@ static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, > if (!pcdev->active) > goto out; > > - vb = &pcdev->active->vb; > - buf = container_of(vb, struct pxa_buffer, vb); > - WARN_ON(buf->inwork || list_empty(&vb->queue)); > + buf = pcdev->active; > + WARN_ON(buf->inwork || list_empty(&buf->queue)); > > /* > * It's normal if the last frame creates an overrun, as there > @@ -734,7 +517,7 @@ static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, > break; > } > last_buf = list_entry(pcdev->capture.prev, > - struct pxa_buffer, vb.queue); > + struct pxa_buffer, queue); > last_status = dma_async_is_tx_complete(pcdev->dma_chans[chan], > last_buf->cookie[chan], > NULL, &last_issued); > @@ -743,14 +526,14 @@ static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, > dev_dbg(dev, "FIFO overrun! CISR: %x\n", > camera_status); > pxa_camera_stop_capture(pcdev); > - list_for_each_entry(buf, &pcdev->capture, vb.queue) > + list_for_each_entry(buf, &pcdev->capture, queue) > pxa_dma_add_tail_buf(pcdev, buf); > pxa_camera_start_capture(pcdev); > goto out; > } > buf->active_dma &= ~act_dma; > if (!buf->active_dma) { > - pxa_camera_wakeup(pcdev, vb, buf); > + pxa_camera_wakeup(pcdev, buf); > pxa_camera_check_link_miss(pcdev, last_buf->cookie[chan], > last_issued); > } > @@ -759,26 +542,250 @@ out: > spin_unlock_irqrestore(&pcdev->lock, flags); > } > > -static struct videobuf_queue_ops pxa_videobuf_ops = { > - .buf_setup = pxa_videobuf_setup, > - .buf_prepare = pxa_videobuf_prepare, > - .buf_queue = pxa_videobuf_queue, > - .buf_release = pxa_videobuf_release, > +static void pxa_buffer_cleanup(struct pxa_buffer *buf) > +{ > + int i; > + > + for (i = 0; i < 3 && buf->descs[i]; i++) { > + dmaengine_desc_free(buf->descs[i]); > + kfree(buf->sg[i]); > + buf->descs[i] = NULL; > + buf->sg[i] = NULL; > + buf->sg_len[i] = 0; > + buf->plane_sizes[i] = 0; > + } > + buf->nb_planes = 0; > +} > + > +static int pxa_buffer_init(struct pxa_camera_dev *pcdev, > + struct pxa_buffer *buf) > +{ > + struct vb2_buffer *vb = &buf->vbuf.vb2_buf; > + struct sg_table *sgt = vb2_dma_sg_plane_desc(vb, 0); > + int nb_channels = pcdev->channels; > + int i, ret = 0; > + unsigned long size = vb2_get_plane_payload(vb, 0); I wouldn't use payload here but icd->sizeimage instead. You set the payload in the prepare function, no need to do that here. > + > + switch (nb_channels) { > + case 1: > + buf->plane_sizes[0] = size; > + break; > + case 3: > + buf->plane_sizes[0] = size / 2; > + buf->plane_sizes[1] = size / 4; > + buf->plane_sizes[2] = size / 4; > + break; > + default: > + return -EINVAL; > + }; > + buf->nb_planes = nb_channels; > + > + ret = sg_split(sgt->sgl, sgt->nents, 0, nb_channels, > + buf->plane_sizes, buf->sg, buf->sg_len, GFP_KERNEL); > + if (ret < 0) { > + dev_err(pcdev_to_dev(pcdev), > + "sg_split failed: %d\n", ret); > + return ret; > + } > + for (i = 0; i < nb_channels; i++) { > + ret = pxa_init_dma_channel(pcdev, buf, i, > + buf->sg[i], buf->sg_len[i]); > + if (ret) { > + pxa_buffer_cleanup(buf); > + return ret; > + } > + } > + INIT_LIST_HEAD(&buf->queue); > + > + return ret; > +} > + > +static void pxa_vb2_cleanup(struct vb2_buffer *vb) > +{ > + struct pxa_buffer *buf = vb2_to_pxa_buffer(vb); > + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vb->vb2_queue); > + > + dev_dbg(pcdev_to_dev(pcdev), > + "%s(vb=%p)\n", __func__, vb); > + pxa_buffer_cleanup(buf); > +} > + > +static void pxa_vb2_queue(struct vb2_buffer *vb) > +{ > + struct pxa_buffer *buf = vb2_to_pxa_buffer(vb); > + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vb->vb2_queue); > + > + dev_dbg(pcdev_to_dev(pcdev), > + "%s(vb=%p) nb_channels=%d size=%lu active=%p\n", > + __func__, vb, pcdev->channels, vb2_get_plane_payload(vb, 0), > + pcdev->active); > + > + list_add_tail(&buf->queue, &pcdev->capture); > + > + pxa_dma_add_tail_buf(pcdev, buf); > + > + if (!pcdev->active) > + pxa_camera_start_capture(pcdev); > +} > + > +/* > + * Please check the DMA prepared buffer structure in : > + * Documentation/video4linux/pxa_camera.txt > + * Please check also in pxa_camera_check_link_miss() to understand why DMA chain > + * modification while DMA chain is running will work anyway. > + */ > +static int pxa_vb2_prepare(struct vb2_buffer *vb) > +{ > + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vb->vb2_queue); > + struct pxa_buffer *buf = vb2_to_pxa_buffer(vb); > + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue); > + int i, ret = 0; > + > + switch (pcdev->channels) { > + case 1: > + case 3: > + vb2_set_plane_payload(vb, 0, icd->sizeimage); > + break; > + default: > + return -EINVAL; > + } > + > + dev_dbg(pcdev_to_dev(pcdev), > + "%s (vb=%p) nb_channels=%d size=%lu\n", > + __func__, vb, pcdev->channels, vb2_get_plane_payload(vb, 0)); > + > + WARN_ON(!icd->current_fmt); > + > +#ifdef DEBUG > + /* > + * This can be useful if you want to see if we actually fill > + * the buffer with something > + */ > + for (i = 0; i < vb->num_planes; i++) > + memset((void *)vb2_plane_vaddr(vb, i), > + 0xaa, vb2_get_plane_payload(vb, i)); > +#endif > + > + for (i = 0; i < pcdev->channels && vb2_plane_vaddr(vb, i); i++) > + if (vb2_get_plane_payload(vb, i) > vb2_plane_size(vb, i)) > + return -EINVAL; No need for this check, this can't happen. This is checked when the buffers are allocated, and once allocated icd->sizeimage can no longer change. > + > + if ((pcdev->channels != buf->nb_planes) || > + (vb2_get_plane_payload(vb, 0) != buf->plane_sizes[0])) { > + pxa_buffer_cleanup(buf); > + ret = pxa_buffer_init(pcdev, buf); > + if (ret) > + return ret; > + } Ditto, this can't happen on the fly. > + /* > + * I think, in buf_prepare you only have to protect global data, > + * the actual buffer is yours > + */ > + buf->inwork = 0; > + pxa_videobuf_set_actdma(pcdev, buf); > + > + return ret; > +} > + > +static int pxa_vb2_init(struct vb2_buffer *vb) > +{ > + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vb->vb2_queue); > + struct pxa_buffer *buf = vb2_to_pxa_buffer(vb); > + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue); > + > + dev_dbg(pcdev_to_dev(pcdev), > + "%s(nb_channels=%d size=%u)\n", > + __func__, pcdev->channels, icd->sizeimage); > + > + switch (pcdev->channels) { > + case 1: > + case 3: > + vb2_set_plane_payload(vb, 0, icd->sizeimage); > + break; > + default: > + return -EINVAL; > + } I would remove this code. Setting the payload makes no sense for this init function, that happens in the prepare function. > + > + return pxa_buffer_init(pcdev, buf); > +} > + > +static int pxa_vb2_queue_setup(struct vb2_queue *vq, > + unsigned int *nbufs, > + unsigned int *num_planes, unsigned int sizes[], > + void *alloc_ctxs[]) > +{ > + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vq); > + struct soc_camera_device *icd = soc_camera_from_vb2q(vq); > + int size = icd->sizeimage; > + > + dev_dbg(pcdev_to_dev(pcdev), > + "%s(vq=%p nbufs=%d num_planes=%d)\n", > + __func__, vq, *nbufs, *num_planes); > + /* > + * Called from VIDIOC_REQBUFS or in compatibility mode For YUV422P > + * format, even if there are 3 planes Y, U and V, we reply there is only > + * one plane, containing Y, U and V data, one after the other. > + */ > + if (!*num_planes) { > + switch (pcdev->channels) { > + case 1: > + case 3: > + sizes[0] = icd->sizeimage; > + break; > + default: > + return -EINVAL; > + } > + *num_planes = 1; > + } Missing case when called from VIDIOC_CREATE_BUFS: in that case check that the buffer is large enough to store the current format. } else { return sizes[0] < icd->sizeimage ? -EINVAL : 0; } > + > + alloc_ctxs[0] = pcdev->alloc_ctx; > + > + if (!*nbufs) > + *nbufs = 1; > + > + if (size * *nbufs > vid_limit * 1024 * 1024) > + *nbufs = (vid_limit * 1024 * 1024) / size; Is this a real hardware limit or an artificial software limit? If the latter, then just remove it. If you don't have the memory to allocate the buffers, then reqbufs will just return ENOMEM. I never saw a reason for such checks. > + > + return 0; > +} > + > +static void pxa_vb2_stop_streaming(struct vb2_queue *vq) > +{ > + vb2_wait_for_all_buffers(vq); This is normally where the DMA is shut off. Is that not needed for this hardware? > +} > + > +static struct vb2_ops pxa_vb2_ops = { > + .queue_setup = pxa_vb2_queue_setup, > + .buf_init = pxa_vb2_init, > + .buf_prepare = pxa_vb2_prepare, > + .buf_queue = pxa_vb2_queue, > + .buf_cleanup = pxa_vb2_cleanup, > + .stop_streaming = pxa_vb2_stop_streaming, > + .wait_prepare = vb2_ops_wait_prepare, > + .wait_finish = vb2_ops_wait_finish, > }; > > -static void pxa_camera_init_videobuf(struct videobuf_queue *q, > - struct soc_camera_device *icd) > +static int pxa_camera_init_videobuf2(struct vb2_queue *vq, > + struct soc_camera_device *icd) > { > struct soc_camera_host *ici = to_soc_camera_host(icd->parent); > struct pxa_camera_dev *pcdev = ici->priv; > + int ret; > > - /* > - * We must pass NULL as dev pointer, then all pci_* dma operations > - * transform to normal dma_* ones. > - */ > - videobuf_queue_sg_init(q, &pxa_videobuf_ops, NULL, &pcdev->lock, > - V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FIELD_NONE, > - sizeof(struct pxa_buffer), icd, &ici->host_lock); > + vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > + vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ; > + vq->drv_priv = pcdev; > + vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > + vq->buf_struct_size = sizeof(struct pxa_buffer); > + > + vq->ops = &pxa_vb2_ops; > + vq->mem_ops = &vb2_dma_sg_memops; > + > + ret = vb2_queue_init(vq); > + dev_dbg(pcdev_to_dev(pcdev), > + "vb2_queue_init(vq=%p): %d\n", vq, ret); > + > + return ret; > } > > static u32 mclk_get_divisor(struct platform_device *pdev, > @@ -860,9 +867,8 @@ static void pxa_camera_eof(unsigned long arg) > struct pxa_camera_dev *pcdev = (struct pxa_camera_dev *)arg; > unsigned long cifr; > struct pxa_buffer *buf; > - struct videobuf_buffer *vb; > > - dev_dbg(pcdev->soc_host.v4l2_dev.dev, > + dev_dbg(pcdev_to_dev(pcdev), > "Camera interrupt status 0x%x\n", > __raw_readl(pcdev->base + CISR)); > > @@ -871,9 +877,8 @@ static void pxa_camera_eof(unsigned long arg) > __raw_writel(cifr, pcdev->base + CIFR); > > pcdev->active = list_first_entry(&pcdev->capture, > - struct pxa_buffer, vb.queue); > - vb = &pcdev->active->vb; > - buf = container_of(vb, struct pxa_buffer, vb); > + struct pxa_buffer, queue); > + buf = pcdev->active; > pxa_videobuf_set_actdma(pcdev, buf); > > pxa_dma_start_channels(pcdev); > @@ -885,7 +890,7 @@ static irqreturn_t pxa_camera_irq(int irq, void *data) > unsigned long status, cicr0; > > status = __raw_readl(pcdev->base + CISR); > - dev_dbg(pcdev->soc_host.v4l2_dev.dev, > + dev_dbg(pcdev_to_dev(pcdev), > "Camera interrupt status 0x%lx\n", status); > > if (!status) > @@ -1489,42 +1494,11 @@ static int pxa_camera_try_fmt(struct soc_camera_device *icd, > return ret; > } > > -static int pxa_camera_reqbufs(struct soc_camera_device *icd, > - struct v4l2_requestbuffers *p) > -{ > - int i; > - > - /* > - * This is for locking debugging only. I removed spinlocks and now I > - * check whether .prepare is ever called on a linked buffer, or whether > - * a dma IRQ can occur for an in-work or unlinked buffer. Until now > - * it hadn't triggered > - */ > - for (i = 0; i < p->count; i++) { > - struct pxa_buffer *buf = container_of(icd->vb_vidq.bufs[i], > - struct pxa_buffer, vb); > - buf->inwork = 0; > - INIT_LIST_HEAD(&buf->vb.queue); > - } > - > - return 0; > -} > - > static unsigned int pxa_camera_poll(struct file *file, poll_table *pt) > { > struct soc_camera_device *icd = file->private_data; > - struct pxa_buffer *buf; > - > - buf = list_entry(icd->vb_vidq.stream.next, struct pxa_buffer, > - vb.stream); > - > - poll_wait(file, &buf->vb.done, pt); > > - if (buf->vb.state == VIDEOBUF_DONE || > - buf->vb.state == VIDEOBUF_ERROR) > - return POLLIN|POLLRDNORM; > - > - return 0; > + return vb2_poll(&icd->vb2_vidq, file, pt); > } > > static int pxa_camera_querycap(struct soc_camera_host *ici, > @@ -1597,8 +1571,7 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = { > .put_formats = pxa_camera_put_formats, > .set_fmt = pxa_camera_set_fmt, > .try_fmt = pxa_camera_try_fmt, > - .init_videobuf = pxa_camera_init_videobuf, > - .reqbufs = pxa_camera_reqbufs, > + .init_videobuf2 = pxa_camera_init_videobuf2, > .poll = pxa_camera_poll, > .querycap = pxa_camera_querycap, > .set_bus_param = pxa_camera_set_bus_param, > @@ -1692,6 +1665,10 @@ static int pxa_camera_probe(struct platform_device *pdev) > return -ENOMEM; > } > > + pcdev->alloc_ctx = vb2_dma_sg_init_ctx(&pdev->dev); > + if (IS_ERR(pcdev->alloc_ctx)) > + return PTR_ERR(pcdev->alloc_ctx); > + > pcdev->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(pcdev->clk)) > return PTR_ERR(pcdev->clk); > @@ -1828,6 +1805,7 @@ static int pxa_camera_remove(struct platform_device *pdev) > dma_release_channel(pcdev->dma_chans[0]); > dma_release_channel(pcdev->dma_chans[1]); > dma_release_channel(pcdev->dma_chans[2]); > + vb2_dma_sg_cleanup_ctx(pcdev->alloc_ctx); > > soc_camera_host_unregister(soc_host); > > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hans Verkuil <hverkuil@xs4all.nl> writes: > Hi Robert, > > A quick review below. > > I assume this is the first step to moving the pxa_camera driver out of > soc-camera? Hi Hans, It probably is. The next step would be the soc_camera adherence removal. I already began the work, but it's still in very early draft ugly state. But if I end up duplicating 50% or more of soc_camera code, we should revisit the approach, as each driver will duplicate that same code over and over. A small nitpick for the next reviews, I'd like you to quote only subparts of the submission, not the full patch where there are no comments. That makes mails smaller and easier to follow up. >> +static int pxa_buffer_init(struct pxa_camera_dev *pcdev, >> + struct pxa_buffer *buf) >> +{ >> + struct vb2_buffer *vb = &buf->vbuf.vb2_buf; >> + struct sg_table *sgt = vb2_dma_sg_plane_desc(vb, 0); >> + int nb_channels = pcdev->channels; >> + int i, ret = 0; >> + unsigned long size = vb2_get_plane_payload(vb, 0); > > I wouldn't use payload here but icd->sizeimage instead. > > You set the payload in the prepare function, no need to do that here. Ah, that's a special case we need to discuss. I've written in the commit message a chapter about a "special port of this code". This is it. This usecase is when a user does the following : - set format to 1280x1024, RGB565 - REQBUF for MMAP buffers - QBUF, capture, DQBUF - then set format to 640x480, RGB565 => here the new format fits in the previously allocated video buffer - QBUF => the test in pxa_vb2_prepare() detects this, and calls pxa_buffer_init() again Now if this usecase is impossible, then I'll do as you say to simplify the code : use icd->sizeimage, remove the code in pxa_vb2_prepare(), etc ... >> + for (i = 0; i < pcdev->channels && vb2_plane_vaddr(vb, i); i++) >> + if (vb2_get_plane_payload(vb, i) > vb2_plane_size(vb, i)) >> + return -EINVAL; > > No need for this check, this can't happen. This is checked when the buffers are > allocated, and once allocated icd->sizeimage can no longer change. Okay, that's good. Simpler is better, I'll remove it. >> + >> + if ((pcdev->channels != buf->nb_planes) || >> + (vb2_get_plane_payload(vb, 0) != buf->plane_sizes[0])) { >> + pxa_buffer_cleanup(buf); >> + ret = pxa_buffer_init(pcdev, buf); >> + if (ret) >> + return ret; >> + } > > Ditto, this can't happen on the fly. That's part of the discussion above. I'll remove it if I get confirmation the usecase I described above is impossible by construction of the videobuf2 API. >> +static int pxa_vb2_queue_setup(struct vb2_queue *vq, >> + unsigned int *nbufs, >> + unsigned int *num_planes, unsigned int sizes[], >> + void *alloc_ctxs[]) >> +{ >> + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vq); >> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq); >> + int size = icd->sizeimage; >> + >> + dev_dbg(pcdev_to_dev(pcdev), >> + "%s(vq=%p nbufs=%d num_planes=%d)\n", >> + __func__, vq, *nbufs, *num_planes); >> + /* >> + * Called from VIDIOC_REQBUFS or in compatibility mode For YUV422P >> + * format, even if there are 3 planes Y, U and V, we reply there is only >> + * one plane, containing Y, U and V data, one after the other. >> + */ >> + if (!*num_planes) { >> + switch (pcdev->channels) { >> + case 1: >> + case 3: >> + sizes[0] = icd->sizeimage; >> + break; >> + default: >> + return -EINVAL; >> + } >> + *num_planes = 1; >> + } > > Missing case when called from VIDIOC_CREATE_BUFS: in that case check that the > buffer is large enough to store the current format. > > } else { > return sizes[0] < icd->sizeimage ? -EINVAL : 0; > } Okay, will add that for v2. >> + >> + alloc_ctxs[0] = pcdev->alloc_ctx; >> + >> + if (!*nbufs) >> + *nbufs = 1; >> + >> + if (size * *nbufs > vid_limit * 1024 * 1024) >> + *nbufs = (vid_limit * 1024 * 1024) / size; > > Is this a real hardware limit or an artificial software limit? Artificial limit. > If the latter, then just remove it. If you don't have the memory to allocate > the buffers, then reqbufs will just return ENOMEM. I never saw a reason for > such checks. Okay, that was to be consistent with former driver behavior. This was from the beginning in this driver. If Guennadi doesn't care, then I'll remove that, as he is the original author of this limitation. >> +static void pxa_vb2_stop_streaming(struct vb2_queue *vq) >> +{ >> + vb2_wait_for_all_buffers(vq); > > This is normally where the DMA is shut off. Is that not needed for this > hardware? Well, this is keeping the legacy driver's behavior : wait for all DMAs to have finished, ie. stopped, which is guaranteed by waiting for all vb2_done() calls which in turn happen only when DMAs are finished, and then return. I'm intending in an incremental patch to have the dmaengine_terminate_sync() call used here, but for the conversion I'll be keeping the legacy behavior. Thanks for the review.
On 03/11/2016 02:41 PM, Robert Jarzmik wrote: > Hans Verkuil <hverkuil@xs4all.nl> writes: > >> Hi Robert, >> >> A quick review below. >> >> I assume this is the first step to moving the pxa_camera driver out of >> soc-camera? > Hi Hans, > > It probably is. The next step would be the soc_camera adherence removal. I > already began the work, but it's still in very early draft ugly state. > > But if I end up duplicating 50% or more of soc_camera code, we should revisit > the approach, as each driver will duplicate that same code over and over. There shouldn't be much duplication. I'm doing the same for sh_mobile_ceu and I think the end result will actually be a lot simpler. And certainly a lot more readable. The problem with soc-camera is that it tries to support lots of different devices, which makes it more complex than it needs to be. Which means that it is harder to debug and maintain. There was no alternative when it was originally created, but today we have better ways of doing this. One area where I would like to see some helper functions is with respect to format/media bus processing. I played with this a little bit but it is surprisingly hard to do. A lot of devices have all sorts of weird and wonderful exceptions that make this quite problematic. > A small nitpick for the next reviews, I'd like you to quote only subparts of the > submission, not the full patch where there are no comments. That makes mails > smaller and easier to follow up. Sometimes I do, sometimes I don't, depending on how much time I have :-) I'll try to do this next time. > >>> +static int pxa_buffer_init(struct pxa_camera_dev *pcdev, >>> + struct pxa_buffer *buf) >>> +{ >>> + struct vb2_buffer *vb = &buf->vbuf.vb2_buf; >>> + struct sg_table *sgt = vb2_dma_sg_plane_desc(vb, 0); >>> + int nb_channels = pcdev->channels; >>> + int i, ret = 0; >>> + unsigned long size = vb2_get_plane_payload(vb, 0); >> >> I wouldn't use payload here but icd->sizeimage instead. >> >> You set the payload in the prepare function, no need to do that here. > Ah, that's a special case we need to discuss. > I've written in the commit message a chapter about a "special port of this > code". This is it. > > This usecase is when a user does the following : > - set format to 1280x1024, RGB565 > - REQBUF for MMAP buffers > - QBUF, capture, DQBUF > > - then set format to 640x480, RGB565 > => here the new format fits in the previously allocated video buffer > - QBUF > => the test in pxa_vb2_prepare() detects this, and calls pxa_buffer_init() > again > > Now if this usecase is impossible, then I'll do as you say to simplify the code > : use icd->sizeimage, remove the code in pxa_vb2_prepare(), etc ... Does this actually work with soc-camera? As far as I can see soc-camera returns -EBUSY in soc_camera_s_fmt_vid_cap() if you attempt to change the format while streaming. We theorized about this use-case, but nobody actually implemented it. As far as I can see this use-case isn't supported today, so I would certainly not implement it for this vb2 conversion. > >>> + for (i = 0; i < pcdev->channels && vb2_plane_vaddr(vb, i); i++) >>> + if (vb2_get_plane_payload(vb, i) > vb2_plane_size(vb, i)) >>> + return -EINVAL; >> >> No need for this check, this can't happen. This is checked when the buffers are >> allocated, and once allocated icd->sizeimage can no longer change. > Okay, that's good. Simpler is better, I'll remove it. > >>> + >>> + if ((pcdev->channels != buf->nb_planes) || >>> + (vb2_get_plane_payload(vb, 0) != buf->plane_sizes[0])) { >>> + pxa_buffer_cleanup(buf); >>> + ret = pxa_buffer_init(pcdev, buf); >>> + if (ret) >>> + return ret; >>> + } >> >> Ditto, this can't happen on the fly. > That's part of the discussion above. I'll remove it if I get confirmation the > usecase I described above is impossible by construction of the videobuf2 API. > >>> +static int pxa_vb2_queue_setup(struct vb2_queue *vq, >>> + unsigned int *nbufs, >>> + unsigned int *num_planes, unsigned int sizes[], >>> + void *alloc_ctxs[]) >>> +{ >>> + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vq); >>> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq); >>> + int size = icd->sizeimage; >>> + >>> + dev_dbg(pcdev_to_dev(pcdev), >>> + "%s(vq=%p nbufs=%d num_planes=%d)\n", >>> + __func__, vq, *nbufs, *num_planes); >>> + /* >>> + * Called from VIDIOC_REQBUFS or in compatibility mode For YUV422P >>> + * format, even if there are 3 planes Y, U and V, we reply there is only >>> + * one plane, containing Y, U and V data, one after the other. >>> + */ >>> + if (!*num_planes) { >>> + switch (pcdev->channels) { >>> + case 1: >>> + case 3: >>> + sizes[0] = icd->sizeimage; >>> + break; >>> + default: >>> + return -EINVAL; >>> + } >>> + *num_planes = 1; >>> + } >> >> Missing case when called from VIDIOC_CREATE_BUFS: in that case check that the >> buffer is large enough to store the current format. >> >> } else { >> return sizes[0] < icd->sizeimage ? -EINVAL : 0; >> } > Okay, will add that for v2. > >>> + >>> + alloc_ctxs[0] = pcdev->alloc_ctx; >>> + >>> + if (!*nbufs) >>> + *nbufs = 1; >>> + >>> + if (size * *nbufs > vid_limit * 1024 * 1024) >>> + *nbufs = (vid_limit * 1024 * 1024) / size; >> >> Is this a real hardware limit or an artificial software limit? > Artificial limit. > >> If the latter, then just remove it. If you don't have the memory to allocate >> the buffers, then reqbufs will just return ENOMEM. I never saw a reason for >> such checks. > Okay, that was to be consistent with former driver behavior. This was from the > beginning in this driver. If Guennadi doesn't care, then I'll remove that, as he > is the original author of this limitation. I've removed it from other drivers in the past, nobody complained :-) > >>> +static void pxa_vb2_stop_streaming(struct vb2_queue *vq) >>> +{ >>> + vb2_wait_for_all_buffers(vq); >> >> This is normally where the DMA is shut off. Is that not needed for this >> hardware? > Well, this is keeping the legacy driver's behavior : wait for all DMAs to have > finished, ie. stopped, which is guaranteed by waiting for all vb2_done() calls > which in turn happen only when DMAs are finished, and then return. > > I'm intending in an incremental patch to have the dmaengine_terminate_sync() > call used here, but for the conversion I'll be keeping the legacy behavior. OK, I was just checking because it is unusual. > > Thanks for the review. > No problem! Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hans Verkuil <hverkuil@xs4all.nl> writes: > On 03/11/2016 02:41 PM, Robert Jarzmik wrote: >> Hans Verkuil <hverkuil@xs4all.nl> writes: > One area where I would like to see some helper functions is with respect to > format/media bus processing. I played with this a little bit but it is surprisingly > hard to do. A lot of devices have all sorts of weird and wonderful exceptions > that make this quite problematic. I'm also worried about the initial probing, where the subdevice, be that an I2C sensor or something else has to be available, ie. the v4l2_async_notifier and its implications. >> Ah, that's a special case we need to discuss. >> I've written in the commit message a chapter about a "special port of this >> code". This is it. >> >> This usecase is when a user does the following : >> - set format to 1280x1024, RGB565 >> - REQBUF for MMAP buffers >> - QBUF, capture, DQBUF >> >> - then set format to 640x480, RGB565 >> => here the new format fits in the previously allocated video buffer >> - QBUF >> => the test in pxa_vb2_prepare() detects this, and calls pxa_buffer_init() >> again >> >> Now if this usecase is impossible, then I'll do as you say to simplify the code >> : use icd->sizeimage, remove the code in pxa_vb2_prepare(), etc ... > > Does this actually work with soc-camera? As far as I can see soc-camera returns > -EBUSY in soc_camera_s_fmt_vid_cap() if you attempt to change the format while > streaming. It's not "while streaming" in the described usecase, it's after streaming is finished actually. I should have added in the third dash VIDIOC_STREAMON before "capture" and VIDIOC_STREAMOFF after DQBUF. I think it's working, even if I had not tried recently. I certainly don't care that much about the usecase, and I won't feel sad dropping it :) > We theorized about this use-case, but nobody actually implemented it. > As far as I can see this use-case isn't supported today, so I would certainly not > implement it for this vb2 conversion. Fair enough, simpler is better, I'll remove it for v2. >>> If the latter, then just remove it. If you don't have the memory to allocate >>> the buffers, then reqbufs will just return ENOMEM. I never saw a reason for >>> such checks. >> Okay, that was to be consistent with former driver behavior. This was from the >> beginning in this driver. If Guennadi doesn't care, then I'll remove that, as he >> is the original author of this limitation. > > I've removed it from other drivers in the past, nobody complained :-) Good, let's do that here then. Cheers.
On 03/11/2016 04:40 PM, Robert Jarzmik wrote: > Hans Verkuil <hverkuil@xs4all.nl> writes: > >> On 03/11/2016 02:41 PM, Robert Jarzmik wrote: >>> Hans Verkuil <hverkuil@xs4all.nl> writes: >> One area where I would like to see some helper functions is with respect to >> format/media bus processing. I played with this a little bit but it is surprisingly >> hard to do. A lot of devices have all sorts of weird and wonderful exceptions >> that make this quite problematic. > > I'm also worried about the initial probing, where the subdevice, be that an I2C > sensor or something else has to be available, ie. the v4l2_async_notifier and > its implications. > >>> Ah, that's a special case we need to discuss. >>> I've written in the commit message a chapter about a "special port of this >>> code". This is it. >>> >>> This usecase is when a user does the following : >>> - set format to 1280x1024, RGB565 >>> - REQBUF for MMAP buffers >>> - QBUF, capture, DQBUF >>> >>> - then set format to 640x480, RGB565 >>> => here the new format fits in the previously allocated video buffer >>> - QBUF >>> => the test in pxa_vb2_prepare() detects this, and calls pxa_buffer_init() >>> again >>> >>> Now if this usecase is impossible, then I'll do as you say to simplify the code >>> : use icd->sizeimage, remove the code in pxa_vb2_prepare(), etc ... >> >> Does this actually work with soc-camera? As far as I can see soc-camera returns >> -EBUSY in soc_camera_s_fmt_vid_cap() if you attempt to change the format while >> streaming. > > It's not "while streaming" in the described usecase, it's after streaming is > finished actually. I should have added in the third dash VIDIOC_STREAMON before > "capture" and VIDIOC_STREAMOFF after DQBUF. I think it's working, even if I had > not tried recently. I certainly don't care that much about the usecase, and I > won't feel sad dropping it :) Ah, OK. This use-case will probably work with soc-camera. What I would prefer is to remove the feature for this vb2 conversion and the following soc-camera removal. If you decide that the feature is desirable, then add it back in a final patch. The reason for this is that it is much easier for me to review this once the driver is no longer dependent on soc-camera. I don't have to jump from the pxa driver to the soc-camera framework and back just to trace if there are no corner cases that were forgotten. This feature is unusual (very few drivers support it) and so I would like to take a close look at it, ensuring everything is done correctly. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hans Verkuil <hverkuil@xs4all.nl> writes: Hi Hans, Working further on the pxa_camera conversion out of soc_camera, I hit a problem of dma being very long, in running v4l2-compliance -f. After digging a bit, I realized that the video buffer queued was a 640x480 format, while the s_fmt_vid_cap() lastly called was a 48x32 one. Digging a bit further, I remembered this conversation we had : >> +/* >> + * Please check the DMA prepared buffer structure in : >> + * Documentation/video4linux/pxa_camera.txt >> + * Please check also in pxa_camera_check_link_miss() to understand why DMA chain >> + * modification while DMA chain is running will work anyway. >> + */ >> +static int pxa_vb2_prepare(struct vb2_buffer *vb) ...zip... >> + for (i = 0; i < pcdev->channels && vb2_plane_vaddr(vb, i); i++) >> + if (vb2_get_plane_payload(vb, i) > vb2_plane_size(vb, i)) >> + return -EINVAL; > > No need for this check, this can't happen. This is checked when the buffers are > allocated, and once allocated icd->sizeimage can no longer change. > >> + >> + if ((pcdev->channels != buf->nb_planes) || >> + (vb2_get_plane_payload(vb, 0) != buf->plane_sizes[0])) { >> + pxa_buffer_cleanup(buf); >> + ret = pxa_buffer_init(pcdev, buf); >> + if (ret) >> + return ret; >> + } > > Ditto, this can't happen on the fly. This last assumption is incorrect I think, at least that's what my tests show me. As a proof, as attached in [1] the logs of pxa_camera. You will see that a 640x480 is initialized, but when the buffer is prepared and queued, the capture format had been changed to 48x32. The consequence in my case is that the scatter-gather prepared for the DMA is still 640x480x2 bytes long and not 48x32x2 !!! That means I will capture many many frames before declaring the video-buffer done. The previous test was recalculating the scatter-gather, which would have taken care of this case. Unless I'm wrong, I will re-add the above test. Cheers. -- Robert [1] Logs of pxa_camera under v4l2-compliance -f I put "RJK" annotation for speed reading [ 1509.751011] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue_setup(vq=c312f290 nbufs=2 num_planes=0 size=614400) [ 1509.754861] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1) [ 1509.754989] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0 dir=2 [ 1509.755024] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41 [ 1509.755206] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6400[0] flags=0x41 [ 1509.758558] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1) [ 1509.758603] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0 dir=2 [ 1509.758634] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41 [ 1509.758797] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6000[0] flags=0x41 [ 1509.759296] pxa27x-camera pxa27x-camera.0: pxac_vb2_cleanup(vb=c30e6600) [ 1509.759392] pxa-dma pxa-dma.0: vchan c39b0c30: txd c30e6400[0]: freeing [ 1509.759530] pxa27x-camera pxa27x-camera.0: pxac_vb2_cleanup(vb=c30e6a00) [ 1509.759560] pxa-dma pxa-dma.0: vchan c39b0c30: txd c30e6000[0]: freeing [ 1509.760730] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=640x480:50424752) [ 1509.764971] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x50424752 [ 1509.765077] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=640x480:50424752) [ 1509.768771] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x50424752 [ 1509.768945] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x50424752 [ 1509.769009] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=48x32:56595559) [ 1509.772931] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x56595559 [ 1509.773051] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=48x32:56595559) [ 1509.777213] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x56595559 RJK: Here we switch to 48x32 format [ 1509.777386] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue_setup(vq=c312f290 nbufs=3 num_planes=0 size=614400) [ 1509.780789] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1) [ 1509.780869] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0 dir=2 [ 1509.780902] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41 [ 1509.781088] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6600[0] flags=0x41 [ 1509.784948] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1) [ 1509.785065] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0 dir=2 [ 1509.785100] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41 [ 1509.785281] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6400[0] flags=0x41 [ 1509.788611] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1) [ 1509.788658] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0 dir=2 [ 1509.788689] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41 [ 1509.788853] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6200[0] flags=0x41 [ 1509.802079] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6a00) nb_channels=1 size=614400 RJK: Here we prepare a 614400 bytes buffer for 48x32 format The scatter-gather chain is way too big for a 48x32x2 bytes capture [ 1509.802207] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6000) nb_channels=1 size=614400 [ 1509.802248] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6c00) nb_channels=1 size=614400 [ 1509.802512] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6a00) nb_channels=1 size=614400 active= (null) [ 1509.802564] dma dma0chan2: pxad_tx_submit(): txd c30e6600[7]: submitted (not linked) [ 1509.802600] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6a00 cookie=7 [ 1509.802823] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6000) nb_channels=1 size=614400 active= (null) [ 1509.802857] dma dma0chan2: pxad_tx_submit(): txd c30e6400[8]: submitted (cold linked) [ 1509.802889] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6000 cookie=8 [ 1509.803110] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6c00) nb_channels=1 size=614400 active= (null) [ 1509.803142] dma dma0chan2: pxad_tx_submit(): txd c30e6200[9]: submitted (cold linked) [ 1509.803172] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6c00 cookie=9 [ 1509.803205] pxa27x-camera pxa27x-camera.0: pxac_vb2_start_streaming(count=3) active= (null) [ 1509.803236] pxa27x-camera pxa27x-camera.0: pxa_camera_start_capture [ 1509.976619] pxa27x-camera pxa27x-camera.0: Camera interrupt status 0x9119 [ 1509.976723] pxa27x-camera pxa27x-camera.0: Camera interrupt status 0x0 [ 1509.976760] pxa27x-camera pxa27x-camera.0: pxa_dma_start_channels (channel=0) [ 1509.976793] dma dma0chan2: pxad_issue_pending(): txd c30e6600[7] [ 1509.976823] dma dma0chan2: pxad_launch_chan(): desc=c30e6600 [ 1509.976856] dma dma0chan2: lookup_phy(): phy=c3a7f810(0) [ 1509.976885] dma dma0chan2: phy_enable(); phy=c3a7f810(0) misaligned=0 [ 1592.777757] dma dma0chan2: pxad_chan_handler(): checking txd c30e6600[7]: completed=1 [ 1592.777834] dma dma0chan2: pxad_chan_handler(): checking txd c30e6400[8]: completed=0 [ 1592.777913] pxa27x-camera pxa27x-camera.0: camera dma irq, cisr=0x8318 dma=1 [ 1592.778057] dma dma0chan2: pxad_residue(): txd c30e6200[9] sw_desc=c30e6200: 614400 [ 1592.778216] pxa27x-camera pxa27x-camera.0: pxa_camera_wakeup dequeud buffer (buf=0xc30e6a00) [ 1592.778253] pxa27x-camera pxa27x-camera.0: pxa_camera_check_link_miss : top queued buffer=c30e6000, is_dma_stopped=0 [ 1592.779099] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6a00) nb_channels=1 size=614400 [ 1592.779403] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6a00) nb_channels=1 size=614400 active=c30e6000 [ 1592.779455] dma dma0chan2: pxad_tx_submit(): txd c30e6600[a]: submitted (hot linked) [ 1592.779490] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6a00 cookie=10 [ 1675.855782] dma dma0chan2: pxad_chan_handler(): checking txd c30e6400[8]: completed=1 [ 1675.855861] dma dma0chan2: pxad_chan_handler(): checking txd c30e6200[9]: completed=0 [ 1675.855953] pxa27x-camera pxa27x-camera.0: camera dma irq, cisr=0x8318 dma=1 [ 1675.856097] dma dma0chan2: pxad_residue(): txd c30e6600[a] sw_desc=c30e6600: 614400 [ 1675.856251] pxa27x-camera pxa27x-camera.0: pxa_camera_wakeup dequeud buffer (buf=0xc30e6000) [ 1675.856286] pxa27x-camera pxa27x-camera.0: pxa_camera_check_link_miss : top queued buffer=c30e6c00, is_dma_stopped=0 [ 1675.857074] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6000) nb_channels=1 size=614400 [ 1675.857376] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6000) nb_channels=1 size=614400 active=c30e6c00 [ 1675.857428] dma dma0chan2: pxad_tx_submit(): txd c30e6400[b]: submitted (hot linked) [ 1675.857464] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6000 cookie=11 [ 1758.933819] dma dma0chan2: pxad_chan_handler(): checking txd c30e6200[9]: completed=1 [ 1758.933896] dma dma0chan2: pxad_chan_handler(): checking txd c30e6600[a]: completed=0 [ 1758.933971] pxa27x-camera pxa27x-camera.0: camera dma irq, cisr=0x8318 dma=1 [ 1758.934114] dma dma0chan2: pxad_residue(): txd c30e6400[b] sw_desc=c30e6400: 614400 [ 1758.934271] pxa27x-camera pxa27x-camera.0: pxa_camera_wakeup dequeud buffer (buf=0xc30e6c00) [ 1758.934307] pxa27x-camera pxa27x-camera.0: pxa_camera_check_link_miss : top queued buffer=c30e6a00, is_dma_stopped=0 [ 1758.935111] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6c00) nb_channels=1 size=614400 [ 1758.935414] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6c00) nb_channels=1 size=614400 active=c30e6a00 [ 1758.935467] dma dma0chan2: pxad_tx_submit(): txd c30e6200[c]: submitted (hot linked) [ 1758.935503] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6c00 cookie=12 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/02/2016 08:03 PM, Robert Jarzmik wrote: > Hans Verkuil <hverkuil@xs4all.nl> writes: > > Hi Hans, > > Working further on the pxa_camera conversion out of soc_camera, I hit a problem > of dma being very long, in running v4l2-compliance -f. > > After digging a bit, I realized that the video buffer queued was a 640x480 > format, while the s_fmt_vid_cap() lastly called was a 48x32 one. > > Digging a bit further, I remembered this conversation we had : >>> +/* >>> + * Please check the DMA prepared buffer structure in : >>> + * Documentation/video4linux/pxa_camera.txt >>> + * Please check also in pxa_camera_check_link_miss() to understand why DMA chain >>> + * modification while DMA chain is running will work anyway. >>> + */ >>> +static int pxa_vb2_prepare(struct vb2_buffer *vb) > ...zip... >>> + for (i = 0; i < pcdev->channels && vb2_plane_vaddr(vb, i); i++) >>> + if (vb2_get_plane_payload(vb, i) > vb2_plane_size(vb, i)) >>> + return -EINVAL; >> >> No need for this check, this can't happen. This is checked when the buffers are >> allocated, and once allocated icd->sizeimage can no longer change. >> >>> + >>> + if ((pcdev->channels != buf->nb_planes) || >>> + (vb2_get_plane_payload(vb, 0) != buf->plane_sizes[0])) { >>> + pxa_buffer_cleanup(buf); >>> + ret = pxa_buffer_init(pcdev, buf); >>> + if (ret) >>> + return ret; >>> + } >> >> Ditto, this can't happen on the fly. > > This last assumption is incorrect I think, at least that's what my tests show > me. As a proof, as attached in [1] the logs of pxa_camera. You will see that a > 640x480 is initialized, but when the buffer is prepared and queued, the capture > format had been changed to 48x32. > > The consequence in my case is that the scatter-gather prepared for the DMA is > still 640x480x2 bytes long and not 48x32x2 !!! That means I will capture many > many frames before declaring the video-buffer done. > The previous test was recalculating the scatter-gather, which would have taken > care of this case. > > Unless I'm wrong, I will re-add the above test. > > Cheers. > > -- > Robert > > [1] Logs of pxa_camera under v4l2-compliance -f > I put "RJK" annotation for speed reading > > [ 1509.751011] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue_setup(vq=c312f290 nbufs=2 num_planes=0 size=614400) > [ 1509.754861] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1) > [ 1509.754989] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0 dir=2 > [ 1509.755024] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41 > [ 1509.755206] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6400[0] flags=0x41 > [ 1509.758558] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1) > [ 1509.758603] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0 dir=2 > [ 1509.758634] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41 > [ 1509.758797] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6000[0] flags=0x41 > [ 1509.759296] pxa27x-camera pxa27x-camera.0: pxac_vb2_cleanup(vb=c30e6600) > [ 1509.759392] pxa-dma pxa-dma.0: vchan c39b0c30: txd c30e6400[0]: freeing > [ 1509.759530] pxa27x-camera pxa27x-camera.0: pxac_vb2_cleanup(vb=c30e6a00) > [ 1509.759560] pxa-dma pxa-dma.0: vchan c39b0c30: txd c30e6000[0]: freeing > [ 1509.760730] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=640x480:50424752) > [ 1509.764971] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x50424752 > [ 1509.765077] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=640x480:50424752) > [ 1509.768771] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x50424752 > [ 1509.768945] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x50424752 > [ 1509.769009] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=48x32:56595559) > [ 1509.772931] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x56595559 > [ 1509.773051] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=48x32:56595559) > [ 1509.777213] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x56595559 > RJK: Here we switch to 48x32 format > > [ 1509.777386] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue_setup(vq=c312f290 nbufs=3 num_planes=0 size=614400) But this debug line indicates that pcdev->current_pix.sizeimage is still the old value: this should have been updated by the S_FMT call. You'd have to debug that a bit more, it looks like there is a bug somewhere in the driver. I suspect this line in pxac_vidioc_try_fmt_vid_cap: pix->sizeimage = max_t(u32, pix->sizeimage, ret); This should just be pix->sizeimage = ret. Regards, Hans > [ 1509.780789] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1) > [ 1509.780869] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0 dir=2 > [ 1509.780902] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41 > [ 1509.781088] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6600[0] flags=0x41 > [ 1509.784948] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1) > [ 1509.785065] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0 dir=2 > [ 1509.785100] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41 > [ 1509.785281] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6400[0] flags=0x41 > [ 1509.788611] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1) > [ 1509.788658] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0 dir=2 > [ 1509.788689] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41 > [ 1509.788853] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6200[0] flags=0x41 > [ 1509.802079] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6a00) nb_channels=1 size=614400 > RJK: Here we prepare a 614400 bytes buffer for 48x32 format > The scatter-gather chain is way too big for a 48x32x2 bytes capture > > [ 1509.802207] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6000) nb_channels=1 size=614400 > [ 1509.802248] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6c00) nb_channels=1 size=614400 > [ 1509.802512] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6a00) nb_channels=1 size=614400 active= (null) > [ 1509.802564] dma dma0chan2: pxad_tx_submit(): txd c30e6600[7]: submitted (not linked) > [ 1509.802600] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6a00 cookie=7 > [ 1509.802823] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6000) nb_channels=1 size=614400 active= (null) > [ 1509.802857] dma dma0chan2: pxad_tx_submit(): txd c30e6400[8]: submitted (cold linked) > [ 1509.802889] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6000 cookie=8 > [ 1509.803110] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6c00) nb_channels=1 size=614400 active= (null) > [ 1509.803142] dma dma0chan2: pxad_tx_submit(): txd c30e6200[9]: submitted (cold linked) > [ 1509.803172] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6c00 cookie=9 > [ 1509.803205] pxa27x-camera pxa27x-camera.0: pxac_vb2_start_streaming(count=3) active= (null) > [ 1509.803236] pxa27x-camera pxa27x-camera.0: pxa_camera_start_capture > [ 1509.976619] pxa27x-camera pxa27x-camera.0: Camera interrupt status 0x9119 > [ 1509.976723] pxa27x-camera pxa27x-camera.0: Camera interrupt status 0x0 > [ 1509.976760] pxa27x-camera pxa27x-camera.0: pxa_dma_start_channels (channel=0) > [ 1509.976793] dma dma0chan2: pxad_issue_pending(): txd c30e6600[7] > [ 1509.976823] dma dma0chan2: pxad_launch_chan(): desc=c30e6600 > [ 1509.976856] dma dma0chan2: lookup_phy(): phy=c3a7f810(0) > [ 1509.976885] dma dma0chan2: phy_enable(); phy=c3a7f810(0) misaligned=0 > [ 1592.777757] dma dma0chan2: pxad_chan_handler(): checking txd c30e6600[7]: completed=1 > [ 1592.777834] dma dma0chan2: pxad_chan_handler(): checking txd c30e6400[8]: completed=0 > [ 1592.777913] pxa27x-camera pxa27x-camera.0: camera dma irq, cisr=0x8318 dma=1 > [ 1592.778057] dma dma0chan2: pxad_residue(): txd c30e6200[9] sw_desc=c30e6200: 614400 > [ 1592.778216] pxa27x-camera pxa27x-camera.0: pxa_camera_wakeup dequeud buffer (buf=0xc30e6a00) > [ 1592.778253] pxa27x-camera pxa27x-camera.0: pxa_camera_check_link_miss : top queued buffer=c30e6000, is_dma_stopped=0 > [ 1592.779099] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6a00) nb_channels=1 size=614400 > [ 1592.779403] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6a00) nb_channels=1 size=614400 active=c30e6000 > [ 1592.779455] dma dma0chan2: pxad_tx_submit(): txd c30e6600[a]: submitted (hot linked) > [ 1592.779490] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6a00 cookie=10 > [ 1675.855782] dma dma0chan2: pxad_chan_handler(): checking txd c30e6400[8]: completed=1 > [ 1675.855861] dma dma0chan2: pxad_chan_handler(): checking txd c30e6200[9]: completed=0 > [ 1675.855953] pxa27x-camera pxa27x-camera.0: camera dma irq, cisr=0x8318 dma=1 > [ 1675.856097] dma dma0chan2: pxad_residue(): txd c30e6600[a] sw_desc=c30e6600: 614400 > [ 1675.856251] pxa27x-camera pxa27x-camera.0: pxa_camera_wakeup dequeud buffer (buf=0xc30e6000) > [ 1675.856286] pxa27x-camera pxa27x-camera.0: pxa_camera_check_link_miss : top queued buffer=c30e6c00, is_dma_stopped=0 > [ 1675.857074] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6000) nb_channels=1 size=614400 > [ 1675.857376] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6000) nb_channels=1 size=614400 active=c30e6c00 > [ 1675.857428] dma dma0chan2: pxad_tx_submit(): txd c30e6400[b]: submitted (hot linked) > [ 1675.857464] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6000 cookie=11 > [ 1758.933819] dma dma0chan2: pxad_chan_handler(): checking txd c30e6200[9]: completed=1 > [ 1758.933896] dma dma0chan2: pxad_chan_handler(): checking txd c30e6600[a]: completed=0 > [ 1758.933971] pxa27x-camera pxa27x-camera.0: camera dma irq, cisr=0x8318 dma=1 > [ 1758.934114] dma dma0chan2: pxad_residue(): txd c30e6400[b] sw_desc=c30e6400: 614400 > [ 1758.934271] pxa27x-camera pxa27x-camera.0: pxa_camera_wakeup dequeud buffer (buf=0xc30e6c00) > [ 1758.934307] pxa27x-camera pxa27x-camera.0: pxa_camera_check_link_miss : top queued buffer=c30e6a00, is_dma_stopped=0 > [ 1758.935111] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6c00) nb_channels=1 size=614400 > [ 1758.935414] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6c00) nb_channels=1 size=614400 active=c30e6a00 > [ 1758.935467] dma dma0chan2: pxad_tx_submit(): txd c30e6200[c]: submitted (hot linked) > [ 1758.935503] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6c00 cookie=12 > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hans Verkuil <hverkuil@xs4all.nl> writes: > On 08/02/2016 08:03 PM, Robert Jarzmik wrote: >> Hans Verkuil <hverkuil@xs4all.nl> writes: >> [ 1509.773051] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=48x32:56595559) >> [ 1509.777213] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x56595559 >> RJK: Here we switch to 48x32 format >> >> [ 1509.777386] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue_setup(vq=c312f290 nbufs=3 num_planes=0 size=614400) > > But this debug line indicates that pcdev->current_pix.sizeimage is still the old value: this > should have been updated by the S_FMT call. You'd have to debug that a bit more, it looks like > there is a bug somewhere in the driver. > > I suspect this line in pxac_vidioc_try_fmt_vid_cap: > pix->sizeimage = max_t(u32, pix->sizeimage, ret); > > This should just be pix->sizeimage = ret. Hi Hans and Laurent, Thanks for pinpointing this one, that's exactly where the problem comes from. I'm rerunning my capture tests as well as v4l2-compliance -s and v4l2-compliance -f to be sure. That leads me to a question for Laurent : - in the commit bed8d8033037 ("[media] soc-camera: Honor user-requested bytesperline and sizeimage") - in the hunk @@ -177,22 +178,22 @@ static int soc_camera_try_fmt(struct soc_camera_device *icd, - you added this: pix->sizeimage = max_t(u32, pix->sizeimage, ret); As I blindly copied it from soc_camera.c to pxa_camera.c, I didn't pay attention to it. I'd like to understand what this is for, why using the maximum of the computed image size and imagesize instead of using directly the computed imagesize, and if it is applicable to the pxa_camera case. Thanks in advance. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/platform/soc_camera/Kconfig b/drivers/media/platform/soc_camera/Kconfig index e5e2d6cf6638..39fd9f2d3ca8 100644 --- a/drivers/media/platform/soc_camera/Kconfig +++ b/drivers/media/platform/soc_camera/Kconfig @@ -28,8 +28,8 @@ config VIDEO_MX3 config VIDEO_PXA27x tristate "PXA27x Quick Capture Interface driver" - depends on VIDEO_DEV && PXA27x && SOC_CAMERA - select VIDEOBUF_DMA_SG + depends on VIDEO_DEV && PXA27x && SOC_CAMERA && HAS_DMA + select VIDEOBUF2_DMA_SG select SG_SPLIT ---help--- This is a v4l2 driver for the PXA27x Quick Capture Interface diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c index 2aaf4a8f71a0..141a5ba603a8 100644 --- a/drivers/media/platform/soc_camera/pxa_camera.c +++ b/drivers/media/platform/soc_camera/pxa_camera.c @@ -34,7 +34,7 @@ #include <media/v4l2-common.h> #include <media/v4l2-dev.h> -#include <media/videobuf-dma-sg.h> +#include <media/videobuf2-dma-sg.h> #include <media/soc_camera.h> #include <media/drv-intf/soc_mediabus.h> #include <media/v4l2-of.h> @@ -180,13 +180,16 @@ enum pxa_camera_active_dma { /* buffer for one video frame */ struct pxa_buffer { /* common v4l buffer stuff -- must be first */ - struct videobuf_buffer vb; + struct vb2_v4l2_buffer vbuf; + struct list_head queue; u32 code; + int nb_planes; /* our descriptor lists for Y, U and V channels */ struct dma_async_tx_descriptor *descs[3]; dma_cookie_t cookie[3]; struct scatterlist *sg[3]; int sg_len[3]; + size_t plane_sizes[3]; int inwork; enum pxa_camera_active_dma active_dma; }; @@ -222,6 +225,8 @@ struct pxa_camera_dev { struct tasklet_struct task_eof; u32 save_cicr[5]; + + void *alloc_ctx; }; struct pxa_cam { @@ -235,54 +240,16 @@ static unsigned int vid_limit = 16; /* Video memory limit, in Mb */ /* * Videobuf operations */ -static int pxa_videobuf_setup(struct videobuf_queue *vq, unsigned int *count, - unsigned int *size) +static struct pxa_buffer *vb2_to_pxa_buffer(struct vb2_buffer *vb) { - struct soc_camera_device *icd = vq->priv_data; - - dev_dbg(icd->parent, "count=%d, size=%d\n", *count, *size); - - *size = icd->sizeimage; - - if (0 == *count) - *count = 32; - if (*size * *count > vid_limit * 1024 * 1024) - *count = (vid_limit * 1024 * 1024) / *size; + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); - return 0; + return container_of(vbuf, struct pxa_buffer, vbuf); } -static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf) +static struct device *pcdev_to_dev(struct pxa_camera_dev *pcdev) { - struct soc_camera_device *icd = vq->priv_data; - struct videobuf_dmabuf *dma = videobuf_to_dma(&buf->vb); - int i; - - BUG_ON(in_interrupt()); - - dev_dbg(icd->parent, "%s (vb=0x%p) 0x%08lx %d\n", __func__, - &buf->vb, buf->vb.baddr, buf->vb.bsize); - - /* - * This waits until this buffer is out of danger, i.e., until it is no - * longer in STATE_QUEUED or STATE_ACTIVE - */ - videobuf_waiton(vq, &buf->vb, 0, 0); - - for (i = 0; i < 3 && buf->descs[i]; i++) { - dmaengine_desc_free(buf->descs[i]); - kfree(buf->sg[i]); - buf->descs[i] = NULL; - buf->sg[i] = NULL; - buf->sg_len[i] = 0; - } - videobuf_dma_unmap(vq->dev, dma); - videobuf_dma_free(dma); - - buf->vb.state = VIDEOBUF_NEEDS_INIT; - - dev_dbg(icd->parent, "%s end (vb=0x%p) 0x%08lx %d\n", __func__, - &buf->vb, buf->vb.baddr, buf->vb.bsize); + return pcdev->soc_host.v4l2_dev.dev; } static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, @@ -312,31 +279,26 @@ static void pxa_camera_dma_irq_v(void *data) /** * pxa_init_dma_channel - init dma descriptors * @pcdev: pxa camera device - * @buf: pxa buffer to find pxa dma channel + * @vb: videobuffer2 buffer * @dma: dma video buffer * @channel: dma channel (0 => 'Y', 1 => 'U', 2 => 'V') * @cibr: camera Receive Buffer Register - * @size: bytes to transfer - * @offset: offset in videobuffer of the first byte to transfer * * Prepares the pxa dma descriptors to transfer one camera channel. * * Returns 0 if success or -ENOMEM if no memory is available */ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, - struct pxa_buffer *buf, - struct videobuf_dmabuf *dma, int channel, - int cibr, int size, int offset) + struct pxa_buffer *buf, int channel, + struct scatterlist *sg, int sglen) { struct dma_chan *dma_chan = pcdev->dma_chans[channel]; - struct scatterlist *sg = buf->sg[channel]; - int sglen = buf->sg_len[channel]; struct dma_async_tx_descriptor *tx; tx = dmaengine_prep_slave_sg(dma_chan, sg, sglen, DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT | DMA_CTRL_REUSE); if (!tx) { - dev_err(pcdev->soc_host.v4l2_dev.dev, + dev_err(pcdev_to_dev(pcdev), "dmaengine_prep_slave_sg failed\n"); goto fail; } @@ -357,11 +319,9 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, buf->descs[channel] = tx; return 0; fail: - kfree(sg); - - dev_dbg(pcdev->soc_host.v4l2_dev.dev, - "%s (vb=0x%p) dma_tx=%p\n", - __func__, &buf->vb, tx); + dev_dbg(pcdev_to_dev(pcdev), + "%s (vb=%p) dma_tx=%p\n", + __func__, buf, tx); return -ENOMEM; } @@ -374,129 +334,6 @@ static void pxa_videobuf_set_actdma(struct pxa_camera_dev *pcdev, buf->active_dma |= DMA_U | DMA_V; } -/* - * Please check the DMA prepared buffer structure in : - * Documentation/video4linux/pxa_camera.txt - * Please check also in pxa_camera_check_link_miss() to understand why DMA chain - * modification while DMA chain is running will work anyway. - */ -static int pxa_videobuf_prepare(struct videobuf_queue *vq, - struct videobuf_buffer *vb, enum v4l2_field field) -{ - struct soc_camera_device *icd = vq->priv_data; - struct soc_camera_host *ici = to_soc_camera_host(icd->parent); - struct pxa_camera_dev *pcdev = ici->priv; - struct device *dev = pcdev->soc_host.v4l2_dev.dev; - struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb); - int ret; - int size_y, size_u = 0, size_v = 0; - size_t sizes[3]; - - dev_dbg(dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, - vb, vb->baddr, vb->bsize); - - /* Added list head initialization on alloc */ - WARN_ON(!list_empty(&vb->queue)); - -#ifdef DEBUG - /* - * This can be useful if you want to see if we actually fill - * the buffer with something - */ - memset((void *)vb->baddr, 0xaa, vb->bsize); -#endif - - BUG_ON(NULL == icd->current_fmt); - - /* - * I think, in buf_prepare you only have to protect global data, - * the actual buffer is yours - */ - buf->inwork = 1; - - if (buf->code != icd->current_fmt->code || - vb->width != icd->user_width || - vb->height != icd->user_height || - vb->field != field) { - buf->code = icd->current_fmt->code; - vb->width = icd->user_width; - vb->height = icd->user_height; - vb->field = field; - vb->state = VIDEOBUF_NEEDS_INIT; - } - - vb->size = icd->sizeimage; - if (0 != vb->baddr && vb->bsize < vb->size) { - ret = -EINVAL; - goto out; - } - - if (vb->state == VIDEOBUF_NEEDS_INIT) { - int size = vb->size; - struct videobuf_dmabuf *dma = videobuf_to_dma(vb); - - ret = videobuf_iolock(vq, vb, NULL); - if (ret) - goto out; - - if (pcdev->channels == 3) { - size_y = size / 2; - size_u = size_v = size / 4; - } else { - size_y = size; - } - - sizes[0] = size_y; - sizes[1] = size_u; - sizes[2] = size_v; - ret = sg_split(dma->sglist, dma->sglen, 0, pcdev->channels, - sizes, buf->sg, buf->sg_len, GFP_KERNEL); - if (ret < 0) { - dev_err(dev, "sg_split failed: %d\n", ret); - goto fail; - } - - /* init DMA for Y channel */ - ret = pxa_init_dma_channel(pcdev, buf, dma, 0, CIBR0, - size_y, 0); - if (ret) { - dev_err(dev, "DMA initialization for Y/RGB failed\n"); - goto fail; - } - - /* init DMA for U channel */ - if (size_u) - ret = pxa_init_dma_channel(pcdev, buf, dma, 1, CIBR1, - size_u, size_y); - if (ret) { - dev_err(dev, "DMA initialization for U failed\n"); - goto fail; - } - - /* init DMA for V channel */ - if (size_v) - ret = pxa_init_dma_channel(pcdev, buf, dma, 2, CIBR2, - size_v, size_y + size_u); - if (ret) { - dev_err(dev, "DMA initialization for V failed\n"); - goto fail; - } - - vb->state = VIDEOBUF_PREPARED; - } - - buf->inwork = 0; - pxa_videobuf_set_actdma(pcdev, buf); - - return 0; - -fail: - free_buffer(vq, buf); -out: - buf->inwork = 0; - return ret; -} - /** * pxa_dma_start_channels - start DMA channel for active buffer * @pcdev: pxa camera device @@ -512,7 +349,7 @@ static void pxa_dma_start_channels(struct pxa_camera_dev *pcdev) active = pcdev->active; for (i = 0; i < pcdev->channels; i++) { - dev_dbg(pcdev->soc_host.v4l2_dev.dev, + dev_dbg(pcdev_to_dev(pcdev), "%s (channel=%d)\n", __func__, i); dma_async_issue_pending(pcdev->dma_chans[i]); } @@ -523,7 +360,7 @@ static void pxa_dma_stop_channels(struct pxa_camera_dev *pcdev) int i; for (i = 0; i < pcdev->channels; i++) { - dev_dbg(pcdev->soc_host.v4l2_dev.dev, + dev_dbg(pcdev_to_dev(pcdev), "%s (channel=%d)\n", __func__, i); dmaengine_terminate_all(pcdev->dma_chans[i]); } @@ -536,7 +373,7 @@ static void pxa_dma_add_tail_buf(struct pxa_camera_dev *pcdev, for (i = 0; i < pcdev->channels; i++) { buf->cookie[i] = dmaengine_submit(buf->descs[i]); - dev_dbg(pcdev->soc_host.v4l2_dev.dev, + dev_dbg(pcdev_to_dev(pcdev), "%s (channel=%d) : submit vb=%p cookie=%d\n", __func__, i, buf, buf->descs[i]->cookie); } @@ -554,7 +391,7 @@ static void pxa_camera_start_capture(struct pxa_camera_dev *pcdev) { unsigned long cicr0; - dev_dbg(pcdev->soc_host.v4l2_dev.dev, "%s\n", __func__); + dev_dbg(pcdev_to_dev(pcdev), "%s\n", __func__); __raw_writel(__raw_readl(pcdev->base + CISR), pcdev->base + CISR); /* Enable End-Of-Frame Interrupt */ cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_ENB; @@ -572,72 +409,20 @@ static void pxa_camera_stop_capture(struct pxa_camera_dev *pcdev) __raw_writel(cicr0, pcdev->base + CICR0); pcdev->active = NULL; - dev_dbg(pcdev->soc_host.v4l2_dev.dev, "%s\n", __func__); -} - -/* Called under spinlock_irqsave(&pcdev->lock, ...) */ -static void pxa_videobuf_queue(struct videobuf_queue *vq, - struct videobuf_buffer *vb) -{ - struct soc_camera_device *icd = vq->priv_data; - struct soc_camera_host *ici = to_soc_camera_host(icd->parent); - struct pxa_camera_dev *pcdev = ici->priv; - struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb); - - dev_dbg(icd->parent, "%s (vb=0x%p) 0x%08lx %d active=%p\n", - __func__, vb, vb->baddr, vb->bsize, pcdev->active); - - list_add_tail(&vb->queue, &pcdev->capture); - - vb->state = VIDEOBUF_ACTIVE; - pxa_dma_add_tail_buf(pcdev, buf); - - if (!pcdev->active) - pxa_camera_start_capture(pcdev); -} - -static void pxa_videobuf_release(struct videobuf_queue *vq, - struct videobuf_buffer *vb) -{ - struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb); -#ifdef DEBUG - struct soc_camera_device *icd = vq->priv_data; - struct device *dev = icd->parent; - - dev_dbg(dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, - vb, vb->baddr, vb->bsize); - - switch (vb->state) { - case VIDEOBUF_ACTIVE: - dev_dbg(dev, "%s (active)\n", __func__); - break; - case VIDEOBUF_QUEUED: - dev_dbg(dev, "%s (queued)\n", __func__); - break; - case VIDEOBUF_PREPARED: - dev_dbg(dev, "%s (prepared)\n", __func__); - break; - default: - dev_dbg(dev, "%s (unknown)\n", __func__); - break; - } -#endif - - free_buffer(vq, buf); + dev_dbg(pcdev_to_dev(pcdev), "%s\n", __func__); } static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev, - struct videobuf_buffer *vb, struct pxa_buffer *buf) { + struct vb2_buffer *vb = &buf->vbuf.vb2_buf; + /* _init is used to debug races, see comment in pxa_camera_reqbufs() */ - list_del_init(&vb->queue); - vb->state = VIDEOBUF_DONE; - v4l2_get_timestamp(&vb->ts); - vb->field_count++; - wake_up(&vb->done); - dev_dbg(pcdev->soc_host.v4l2_dev.dev, "%s dequeud buffer (vb=0x%p)\n", - __func__, vb); + list_del_init(&buf->queue); + vb->timestamp = ktime_get_ns(); + vb2_buffer_done(vb, VB2_BUF_STATE_DONE); + dev_dbg(pcdev_to_dev(pcdev), "%s dequeud buffer (buf=0x%p)\n", + __func__, buf); if (list_empty(&pcdev->capture)) { pxa_camera_stop_capture(pcdev); @@ -645,7 +430,7 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev, } pcdev->active = list_entry(pcdev->capture.next, - struct pxa_buffer, vb.queue); + struct pxa_buffer, queue); } /** @@ -670,7 +455,7 @@ static void pxa_camera_check_link_miss(struct pxa_camera_dev *pcdev, { bool is_dma_stopped = last_submitted != last_issued; - dev_dbg(pcdev->soc_host.v4l2_dev.dev, + dev_dbg(pcdev_to_dev(pcdev), "%s : top queued buffer=%p, is_dma_stopped=%d\n", __func__, pcdev->active, is_dma_stopped); @@ -681,12 +466,11 @@ static void pxa_camera_check_link_miss(struct pxa_camera_dev *pcdev, static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, enum pxa_camera_active_dma act_dma) { - struct device *dev = pcdev->soc_host.v4l2_dev.dev; + struct device *dev = pcdev_to_dev(pcdev); struct pxa_buffer *buf, *last_buf; unsigned long flags; u32 camera_status, overrun; int chan; - struct videobuf_buffer *vb; enum dma_status last_status; dma_cookie_t last_issued; @@ -714,9 +498,8 @@ static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, if (!pcdev->active) goto out; - vb = &pcdev->active->vb; - buf = container_of(vb, struct pxa_buffer, vb); - WARN_ON(buf->inwork || list_empty(&vb->queue)); + buf = pcdev->active; + WARN_ON(buf->inwork || list_empty(&buf->queue)); /* * It's normal if the last frame creates an overrun, as there @@ -734,7 +517,7 @@ static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, break; } last_buf = list_entry(pcdev->capture.prev, - struct pxa_buffer, vb.queue); + struct pxa_buffer, queue); last_status = dma_async_is_tx_complete(pcdev->dma_chans[chan], last_buf->cookie[chan], NULL, &last_issued); @@ -743,14 +526,14 @@ static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, dev_dbg(dev, "FIFO overrun! CISR: %x\n", camera_status); pxa_camera_stop_capture(pcdev); - list_for_each_entry(buf, &pcdev->capture, vb.queue) + list_for_each_entry(buf, &pcdev->capture, queue) pxa_dma_add_tail_buf(pcdev, buf); pxa_camera_start_capture(pcdev); goto out; } buf->active_dma &= ~act_dma; if (!buf->active_dma) { - pxa_camera_wakeup(pcdev, vb, buf); + pxa_camera_wakeup(pcdev, buf); pxa_camera_check_link_miss(pcdev, last_buf->cookie[chan], last_issued); } @@ -759,26 +542,250 @@ out: spin_unlock_irqrestore(&pcdev->lock, flags); } -static struct videobuf_queue_ops pxa_videobuf_ops = { - .buf_setup = pxa_videobuf_setup, - .buf_prepare = pxa_videobuf_prepare, - .buf_queue = pxa_videobuf_queue, - .buf_release = pxa_videobuf_release, +static void pxa_buffer_cleanup(struct pxa_buffer *buf) +{ + int i; + + for (i = 0; i < 3 && buf->descs[i]; i++) { + dmaengine_desc_free(buf->descs[i]); + kfree(buf->sg[i]); + buf->descs[i] = NULL; + buf->sg[i] = NULL; + buf->sg_len[i] = 0; + buf->plane_sizes[i] = 0; + } + buf->nb_planes = 0; +} + +static int pxa_buffer_init(struct pxa_camera_dev *pcdev, + struct pxa_buffer *buf) +{ + struct vb2_buffer *vb = &buf->vbuf.vb2_buf; + struct sg_table *sgt = vb2_dma_sg_plane_desc(vb, 0); + int nb_channels = pcdev->channels; + int i, ret = 0; + unsigned long size = vb2_get_plane_payload(vb, 0); + + switch (nb_channels) { + case 1: + buf->plane_sizes[0] = size; + break; + case 3: + buf->plane_sizes[0] = size / 2; + buf->plane_sizes[1] = size / 4; + buf->plane_sizes[2] = size / 4; + break; + default: + return -EINVAL; + }; + buf->nb_planes = nb_channels; + + ret = sg_split(sgt->sgl, sgt->nents, 0, nb_channels, + buf->plane_sizes, buf->sg, buf->sg_len, GFP_KERNEL); + if (ret < 0) { + dev_err(pcdev_to_dev(pcdev), + "sg_split failed: %d\n", ret); + return ret; + } + for (i = 0; i < nb_channels; i++) { + ret = pxa_init_dma_channel(pcdev, buf, i, + buf->sg[i], buf->sg_len[i]); + if (ret) { + pxa_buffer_cleanup(buf); + return ret; + } + } + INIT_LIST_HEAD(&buf->queue); + + return ret; +} + +static void pxa_vb2_cleanup(struct vb2_buffer *vb) +{ + struct pxa_buffer *buf = vb2_to_pxa_buffer(vb); + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vb->vb2_queue); + + dev_dbg(pcdev_to_dev(pcdev), + "%s(vb=%p)\n", __func__, vb); + pxa_buffer_cleanup(buf); +} + +static void pxa_vb2_queue(struct vb2_buffer *vb) +{ + struct pxa_buffer *buf = vb2_to_pxa_buffer(vb); + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vb->vb2_queue); + + dev_dbg(pcdev_to_dev(pcdev), + "%s(vb=%p) nb_channels=%d size=%lu active=%p\n", + __func__, vb, pcdev->channels, vb2_get_plane_payload(vb, 0), + pcdev->active); + + list_add_tail(&buf->queue, &pcdev->capture); + + pxa_dma_add_tail_buf(pcdev, buf); + + if (!pcdev->active) + pxa_camera_start_capture(pcdev); +} + +/* + * Please check the DMA prepared buffer structure in : + * Documentation/video4linux/pxa_camera.txt + * Please check also in pxa_camera_check_link_miss() to understand why DMA chain + * modification while DMA chain is running will work anyway. + */ +static int pxa_vb2_prepare(struct vb2_buffer *vb) +{ + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vb->vb2_queue); + struct pxa_buffer *buf = vb2_to_pxa_buffer(vb); + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue); + int i, ret = 0; + + switch (pcdev->channels) { + case 1: + case 3: + vb2_set_plane_payload(vb, 0, icd->sizeimage); + break; + default: + return -EINVAL; + } + + dev_dbg(pcdev_to_dev(pcdev), + "%s (vb=%p) nb_channels=%d size=%lu\n", + __func__, vb, pcdev->channels, vb2_get_plane_payload(vb, 0)); + + WARN_ON(!icd->current_fmt); + +#ifdef DEBUG + /* + * This can be useful if you want to see if we actually fill + * the buffer with something + */ + for (i = 0; i < vb->num_planes; i++) + memset((void *)vb2_plane_vaddr(vb, i), + 0xaa, vb2_get_plane_payload(vb, i)); +#endif + + for (i = 0; i < pcdev->channels && vb2_plane_vaddr(vb, i); i++) + if (vb2_get_plane_payload(vb, i) > vb2_plane_size(vb, i)) + return -EINVAL; + + if ((pcdev->channels != buf->nb_planes) || + (vb2_get_plane_payload(vb, 0) != buf->plane_sizes[0])) { + pxa_buffer_cleanup(buf); + ret = pxa_buffer_init(pcdev, buf); + if (ret) + return ret; + } + /* + * I think, in buf_prepare you only have to protect global data, + * the actual buffer is yours + */ + buf->inwork = 0; + pxa_videobuf_set_actdma(pcdev, buf); + + return ret; +} + +static int pxa_vb2_init(struct vb2_buffer *vb) +{ + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vb->vb2_queue); + struct pxa_buffer *buf = vb2_to_pxa_buffer(vb); + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue); + + dev_dbg(pcdev_to_dev(pcdev), + "%s(nb_channels=%d size=%u)\n", + __func__, pcdev->channels, icd->sizeimage); + + switch (pcdev->channels) { + case 1: + case 3: + vb2_set_plane_payload(vb, 0, icd->sizeimage); + break; + default: + return -EINVAL; + } + + return pxa_buffer_init(pcdev, buf); +} + +static int pxa_vb2_queue_setup(struct vb2_queue *vq, + unsigned int *nbufs, + unsigned int *num_planes, unsigned int sizes[], + void *alloc_ctxs[]) +{ + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vq); + struct soc_camera_device *icd = soc_camera_from_vb2q(vq); + int size = icd->sizeimage; + + dev_dbg(pcdev_to_dev(pcdev), + "%s(vq=%p nbufs=%d num_planes=%d)\n", + __func__, vq, *nbufs, *num_planes); + /* + * Called from VIDIOC_REQBUFS or in compatibility mode For YUV422P + * format, even if there are 3 planes Y, U and V, we reply there is only + * one plane, containing Y, U and V data, one after the other. + */ + if (!*num_planes) { + switch (pcdev->channels) { + case 1: + case 3: + sizes[0] = icd->sizeimage; + break; + default: + return -EINVAL; + } + *num_planes = 1; + } + + alloc_ctxs[0] = pcdev->alloc_ctx; + + if (!*nbufs) + *nbufs = 1; + + if (size * *nbufs > vid_limit * 1024 * 1024) + *nbufs = (vid_limit * 1024 * 1024) / size; + + return 0; +} + +static void pxa_vb2_stop_streaming(struct vb2_queue *vq) +{ + vb2_wait_for_all_buffers(vq); +} + +static struct vb2_ops pxa_vb2_ops = { + .queue_setup = pxa_vb2_queue_setup, + .buf_init = pxa_vb2_init, + .buf_prepare = pxa_vb2_prepare, + .buf_queue = pxa_vb2_queue, + .buf_cleanup = pxa_vb2_cleanup, + .stop_streaming = pxa_vb2_stop_streaming, + .wait_prepare = vb2_ops_wait_prepare, + .wait_finish = vb2_ops_wait_finish, }; -static void pxa_camera_init_videobuf(struct videobuf_queue *q, - struct soc_camera_device *icd) +static int pxa_camera_init_videobuf2(struct vb2_queue *vq, + struct soc_camera_device *icd) { struct soc_camera_host *ici = to_soc_camera_host(icd->parent); struct pxa_camera_dev *pcdev = ici->priv; + int ret; - /* - * We must pass NULL as dev pointer, then all pci_* dma operations - * transform to normal dma_* ones. - */ - videobuf_queue_sg_init(q, &pxa_videobuf_ops, NULL, &pcdev->lock, - V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FIELD_NONE, - sizeof(struct pxa_buffer), icd, &ici->host_lock); + vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; + vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ; + vq->drv_priv = pcdev; + vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; + vq->buf_struct_size = sizeof(struct pxa_buffer); + + vq->ops = &pxa_vb2_ops; + vq->mem_ops = &vb2_dma_sg_memops; + + ret = vb2_queue_init(vq); + dev_dbg(pcdev_to_dev(pcdev), + "vb2_queue_init(vq=%p): %d\n", vq, ret); + + return ret; } static u32 mclk_get_divisor(struct platform_device *pdev, @@ -860,9 +867,8 @@ static void pxa_camera_eof(unsigned long arg) struct pxa_camera_dev *pcdev = (struct pxa_camera_dev *)arg; unsigned long cifr; struct pxa_buffer *buf; - struct videobuf_buffer *vb; - dev_dbg(pcdev->soc_host.v4l2_dev.dev, + dev_dbg(pcdev_to_dev(pcdev), "Camera interrupt status 0x%x\n", __raw_readl(pcdev->base + CISR)); @@ -871,9 +877,8 @@ static void pxa_camera_eof(unsigned long arg) __raw_writel(cifr, pcdev->base + CIFR); pcdev->active = list_first_entry(&pcdev->capture, - struct pxa_buffer, vb.queue); - vb = &pcdev->active->vb; - buf = container_of(vb, struct pxa_buffer, vb); + struct pxa_buffer, queue); + buf = pcdev->active; pxa_videobuf_set_actdma(pcdev, buf); pxa_dma_start_channels(pcdev); @@ -885,7 +890,7 @@ static irqreturn_t pxa_camera_irq(int irq, void *data) unsigned long status, cicr0; status = __raw_readl(pcdev->base + CISR); - dev_dbg(pcdev->soc_host.v4l2_dev.dev, + dev_dbg(pcdev_to_dev(pcdev), "Camera interrupt status 0x%lx\n", status); if (!status) @@ -1489,42 +1494,11 @@ static int pxa_camera_try_fmt(struct soc_camera_device *icd, return ret; } -static int pxa_camera_reqbufs(struct soc_camera_device *icd, - struct v4l2_requestbuffers *p) -{ - int i; - - /* - * This is for locking debugging only. I removed spinlocks and now I - * check whether .prepare is ever called on a linked buffer, or whether - * a dma IRQ can occur for an in-work or unlinked buffer. Until now - * it hadn't triggered - */ - for (i = 0; i < p->count; i++) { - struct pxa_buffer *buf = container_of(icd->vb_vidq.bufs[i], - struct pxa_buffer, vb); - buf->inwork = 0; - INIT_LIST_HEAD(&buf->vb.queue); - } - - return 0; -} - static unsigned int pxa_camera_poll(struct file *file, poll_table *pt) { struct soc_camera_device *icd = file->private_data; - struct pxa_buffer *buf; - - buf = list_entry(icd->vb_vidq.stream.next, struct pxa_buffer, - vb.stream); - - poll_wait(file, &buf->vb.done, pt); - if (buf->vb.state == VIDEOBUF_DONE || - buf->vb.state == VIDEOBUF_ERROR) - return POLLIN|POLLRDNORM; - - return 0; + return vb2_poll(&icd->vb2_vidq, file, pt); } static int pxa_camera_querycap(struct soc_camera_host *ici, @@ -1597,8 +1571,7 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = { .put_formats = pxa_camera_put_formats, .set_fmt = pxa_camera_set_fmt, .try_fmt = pxa_camera_try_fmt, - .init_videobuf = pxa_camera_init_videobuf, - .reqbufs = pxa_camera_reqbufs, + .init_videobuf2 = pxa_camera_init_videobuf2, .poll = pxa_camera_poll, .querycap = pxa_camera_querycap, .set_bus_param = pxa_camera_set_bus_param, @@ -1692,6 +1665,10 @@ static int pxa_camera_probe(struct platform_device *pdev) return -ENOMEM; } + pcdev->alloc_ctx = vb2_dma_sg_init_ctx(&pdev->dev); + if (IS_ERR(pcdev->alloc_ctx)) + return PTR_ERR(pcdev->alloc_ctx); + pcdev->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(pcdev->clk)) return PTR_ERR(pcdev->clk); @@ -1828,6 +1805,7 @@ static int pxa_camera_remove(struct platform_device *pdev) dma_release_channel(pcdev->dma_chans[0]); dma_release_channel(pcdev->dma_chans[1]); dma_release_channel(pcdev->dma_chans[2]); + vb2_dma_sg_cleanup_ctx(pcdev->alloc_ctx); soc_camera_host_unregister(soc_host);
Convert pxa_camera from videobuf to videobuf2. As the soc_camera was already compatible with videobuf2, the port is quite straightforward. The only special port of this code is that if the vb2 to prepare is "too big" in terms of size for the new capture format, the pxa_camera will accept it anyway, and adapt the dmaengine transfer accordingly. Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> --- I'm not very sure about the locking of the video buffers yet. --- drivers/media/platform/soc_camera/Kconfig | 4 +- drivers/media/platform/soc_camera/pxa_camera.c | 598 ++++++++++++------------- 2 files changed, 290 insertions(+), 312 deletions(-)