Hi, Tomasz, > -----Original Message----- > From: linux-media-owner@xxxxxxxxxxxxxxx [mailto:linux-media- > owner@xxxxxxxxxxxxxxx] On Behalf Of Tomasz Figa > Sent: Tuesday, September 18, 2018 10:23 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 > > 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) { > // ... > } > Thanks for the clarification. > > > > +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? :) > Yes, that's the plan. > Best regards, > Tomasz
