On Mon, Sep 17, 2018 at 5:20 AM Zhi, Yong <yong.zhi@xxxxxxxxx> wrote: > > 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. Just to make sure, the suggestion also includes starting from IMGU_NODE_IN (not + 1), i.e. for (node = IMGU_NODE_IN; node != IMGU_NODE_IN || imgu_queue_getbuf(imgu, IMGU_NODE_IN); node = (node + 1) % IMGU_NODE_NUM) { // ... } > > > +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. > I guess that means that v7 is going to have the workqueue back? :) Best regards, Tomasz
