Hi, Tomasz, Thanks for the code review. > -----Original Message----- > From: linux-media-owner@xxxxxxxxxxxxxxx [mailto:linux-media- > owner@xxxxxxxxxxxxxxx] On Behalf Of Tomasz Figa > Sent: Monday, July 2, 2018 3:08 AM > To: Zhi, Yong <yong.zhi@xxxxxxxxx> > Cc: Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx>; Sakari Ailus > <sakari.ailus@xxxxxxxxxxxxxxx>; Mani, Rajmohan > <rajmohan.mani@xxxxxxxxx>; Toivonen, Tuukka > <tuukka.toivonen@xxxxxxxxx>; Hu, Jerry W <jerry.w.hu@xxxxxxxxx>; Zheng, > Jian Xu <jian.xu.zheng@xxxxxxxxx> > Subject: Re: [PATCH v6 12/12] intel-ipu3: Add imgu top level pci device > driver > > Hi Yong, > > On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi <yong.zhi@xxxxxxxxx> wrote: > > +/* > > + * Queue as many buffers to CSS as possible. If all buffers don't fit > > +into > > + * CSS buffer queues, they remain unqueued and will be queued later. > > + */ > > +int imgu_queue_buffers(struct imgu_device *imgu, bool initial) { > > + unsigned int node; > > + int r = 0; > > + struct imgu_buffer *ibuf; > > + > > + if (!ipu3_css_is_streaming(&imgu->css)) > > + return 0; > > + > > + mutex_lock(&imgu->lock); > > + > > + /* Buffer set is queued to FW only when input buffer is ready */ > > + if (!imgu_queue_getbuf(imgu, IMGU_NODE_IN)) { > > + mutex_unlock(&imgu->lock); > > + return 0; > > + } > > + for (node = IMGU_NODE_IN + 1; 1; node = (node + 1) % > > + IMGU_NODE_NUM) { > > Shouldn't we make (node != IMGU_NODE_IN || imgu_queue_getbuf(imgu, > IMGU_NODE_IN)) the condition here, rather than 1? > > This would also let us remove the explicit call to imgu_queue_getbuf() > above the loop. > Ack, will make the suggested changes regarding the loop condition evaluation. > > + if (node == IMGU_NODE_VF && > > + (imgu->css.pipe_id == IPU3_CSS_PIPE_ID_CAPTURE || > > + !imgu->nodes[IMGU_NODE_VF].enabled)) { > > + continue; > > + } else if (node == IMGU_NODE_PV && > > + (imgu->css.pipe_id == IPU3_CSS_PIPE_ID_VIDEO || > > + !imgu->nodes[IMGU_NODE_PV].enabled)) { > > + continue; > > + } else if (imgu->queue_enabled[node]) { > > + struct ipu3_css_buffer *buf = > > + imgu_queue_getbuf(imgu, node); > > + int dummy; > > + > > + if (!buf) > > + break; > > + > > + r = ipu3_css_buf_queue(&imgu->css, buf); > > + if (r) > > + break; > > + dummy = imgu_dummybufs_check(imgu, buf); > > + if (!dummy) > > + ibuf = container_of(buf, struct imgu_buffer, > > + css_buf); > > + dev_dbg(&imgu->pci_dev->dev, > > + "queue %s %s buffer %d to css da: 0x%08x\n", > > + dummy ? "dummy" : "user", > > + imgu_node_map[node].name, > > + dummy ? 0 : ibuf->vid_buf.vbb.vb2_buf.index, > > + (u32)buf->daddr); > > + } > > + if (node == IMGU_NODE_IN && > > + !imgu_queue_getbuf(imgu, IMGU_NODE_IN)) > > + break; > > My suggestion to the for loop condition is based on this. > Got it. > > + } > > + mutex_unlock(&imgu->lock); > > + > > + if (r && r != -EBUSY) > > + goto failed; > > + > > + return 0; > > + > > +failed: > > + /* > > + * On error, mark all buffers as failed which are not > > + * yet queued to CSS > > + */ > > + dev_err(&imgu->pci_dev->dev, > > + "failed to queue buffer to CSS on queue %i (%d)\n", > > + node, r); > > + > > + if (initial) > > + /* If we were called from streamon(), no need to finish bufs */ > > + return r; > > + > > + for (node = 0; node < IMGU_NODE_NUM; node++) { > > + struct imgu_buffer *buf, *buf0; > > + > > + if (!imgu->queue_enabled[node]) > > + continue; /* Skip disabled queues */ > > + > > + mutex_lock(&imgu->lock); > > + list_for_each_entry_safe(buf, buf0, &imgu- > >nodes[node].buffers, > > + vid_buf.list) { > > + if (ipu3_css_buf_state(&buf->css_buf) == > > + IPU3_CSS_BUFFER_QUEUED) > > + continue; /* Was already queued, skip */ > > + > > + ipu3_v4l2_buffer_done(&buf->vid_buf.vbb.vb2_buf, > > + VB2_BUF_STATE_ERROR); > > + } > > + mutex_unlock(&imgu->lock); > > + } > > + > > + return r; > > +} > > [snip] > > > +static irqreturn_t imgu_isr_threaded(int irq, void *imgu_ptr) { > > + struct imgu_device *imgu = imgu_ptr; > > + > > + /* Dequeue / queue buffers */ > > + do { > > + u64 ns = ktime_get_ns(); > > + struct ipu3_css_buffer *b; > > + struct imgu_buffer *buf; > > + unsigned int node; > > + bool dummy; > > + > > + do { > > + mutex_lock(&imgu->lock); > > + b = ipu3_css_buf_dequeue(&imgu->css); > > + mutex_unlock(&imgu->lock); > > + } while (PTR_ERR(b) == -EAGAIN); > > + > > + if (IS_ERR_OR_NULL(b)) { > > + if (!b || PTR_ERR(b) == -EBUSY) /* All done */ > > + break; > > + dev_err(&imgu->pci_dev->dev, > > + "failed to dequeue buffers (%ld)\n", > > + PTR_ERR(b)); > > + break; > > + } > > + > > + node = imgu_map_node(imgu, b->queue); > > + dummy = imgu_dummybufs_check(imgu, b); > > + if (!dummy) > > + buf = container_of(b, struct imgu_buffer, css_buf); > > + dev_dbg(&imgu->pci_dev->dev, > > + "dequeue %s %s buffer %d from css\n", > > + dummy ? "dummy" : "user", > > + imgu_node_map[node].name, > > + dummy ? 0 : buf->vid_buf.vbb.vb2_buf.index); > > + > > + if (dummy) > > + /* It was a dummy buffer, skip it */ > > + continue; > > + > > + /* Fill vb2 buffer entries and tell it's ready */ > > + if (!imgu->nodes[node].output) { > > + buf->vid_buf.vbb.vb2_buf.timestamp = ns; > > + buf->vid_buf.vbb.field = V4L2_FIELD_NONE; > > + buf->vid_buf.vbb.sequence = > > + atomic_inc_return(&imgu->nodes[node].sequence); > > + } > > + imgu_buffer_done(imgu, &buf->vid_buf.vbb.vb2_buf, > > + ipu3_css_buf_state(&buf->css_buf) == > > + IPU3_CSS_BUFFER_DONE ? > > + VB2_BUF_STATE_DONE : > > + VB2_BUF_STATE_ERROR); > > + } while (1); > > + > > + /* > > + * Try to queue more buffers for CSS. > > + * qbuf_barrier is used to disable new buffers > > + * to be queued to CSS. > > + */ > > + if (!atomic_read(&imgu->qbuf_barrier)) > > + imgu_queue_buffers(imgu, false); > > + > > + return IRQ_NONE; > > This is a serious bug. An interrupt handler must not return IRQ_NONE > unless it's really sure that the device it handles did not generate any > interrupt. This threaded handler is called as a result of the hardirq handler > actually finding an interrupt to be handled, so we must always return > IRQ_HANDLED here. > > [snip] Ack, thanks for pointing it out. > > > +static int __maybe_unused imgu_suspend(struct device *dev) { > > + struct pci_dev *pci_dev = to_pci_dev(dev); > > + struct imgu_device *imgu = pci_get_drvdata(pci_dev); > > + unsigned long expire; > > + > > + dev_dbg(dev, "enter %s\n", __func__); > > + imgu->suspend_in_stream = ipu3_css_is_streaming(&imgu->css); > > + if (!imgu->suspend_in_stream) > > + goto out; > > + /* Block new buffers to be queued to CSS. */ > > + atomic_set(&imgu->qbuf_barrier, 1); > > + /* > > + * Wait for currently running irq handler to be done so that > > + * no new buffers will be queued to fw later. > > + */ > > + synchronize_irq(pci_dev->irq); > > + /* Wait until all buffers in CSS are done. */ > > + expire = jiffies + msecs_to_jiffies(1000); > > + while (!ipu3_css_queue_empty(&imgu->css)) { > > + if (time_is_before_jiffies(expire)) { > > + dev_err(dev, "wait buffer drain timeout.\n"); > > + break; > > + } > > + } > > Uhm. We struggle to save some power by suspending the device only to > end up with an ugly busy wait that could take even a second here. This > doesn't make any sense. > > We had a working solution using a wait queue in previous revision [1]. > What happened to it? > > [1] https://chromium- > review.googlesource.com/c/chromiumos/third_party/kernel/+/1029594/2 > /drivers/media/pci/intel/ipu3/ipu3.c#b913 > (see the left side) > The code here was based on an old version of patch "ipu3-imgu: Avoid might sleep operations in suspend callback" at submission, so it did have buf_drain_wq, sorry for the confusion. > > + ipu3_css_stop_streaming(&imgu->css); > > + atomic_set(&imgu->qbuf_barrier, 0); > > + imgu_powerdown(imgu); > > + pm_runtime_force_suspend(dev); > > +out: > > + dev_dbg(dev, "leave %s\n", __func__); > > + return 0; > > +} > > Best regards, > Tomasz
