RE: [PATCH v6 12/12] intel-ipu3: Add imgu top level pci device driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux