| Message ID | 1550221729-29240-1-git-send-email-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media:staging/intel-ipu3: parameter buffer refactoring | expand |
Hi Bingbu, On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: > > From: Bingbu Cao <bingbu.cao@intel.com> > > Current ImgU driver processes and releases the parameter buffer > immediately after queued from user. This does not align with other > image buffers which are grouped in sets and used for the same frame. > If user queues multiple parameter buffers continuously, only the last > one will take effect. > To make consistent buffers usage, this patch changes the parameter > buffer handling and group parameter buffer with other image buffers > for each frame. Thanks for the patch. Please see my comments inline. > > Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > --- > drivers/staging/media/ipu3/ipu3-css.c | 5 ----- > drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- > drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ > 3 files changed, 33 insertions(+), 37 deletions(-) > > diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c > index b9354d2bb692..bcb1d436bc98 100644 > --- a/drivers/staging/media/ipu3/ipu3-css.c > +++ b/drivers/staging/media/ipu3/ipu3-css.c > @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, > obgrid_size = ipu3_css_fw_obgrid_size(bi); > stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; > > - /* > - * TODO(b/118782861): If userspace queues more than 4 buffers, the > - * parameters from previous buffers will be overwritten. Fix the driver > - * not to allow this. > - */ Wouldn't this still happen even with current patch? imgu_queue_buffers() supposedly queues "as many buffers to CSS as possible". This means that if the userspace queues more than 4 complete frames, we still end up overwriting the parameter buffers in the pool. Please correct me if I'm wrong. > ipu3_css_pool_get(&css_pipe->pool.parameter_set_info); > param_set = ipu3_css_pool_last(&css_pipe->pool.parameter_set_info, > 0)->vaddr; > diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c > index e758a650ad2b..7812c7324893 100644 > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c > @@ -341,8 +341,9 @@ static void ipu3_vb2_buf_queue(struct vb2_buffer *vb) > struct imgu_video_device *node = > container_of(vb->vb2_queue, struct imgu_video_device, vbq); > unsigned int queue = imgu_node_to_queue(node->id); > + struct imgu_buffer *buf = container_of(vb, struct imgu_buffer, > + vid_buf.vbb.vb2_buf); > unsigned long need_bytes; > - unsigned int pipe = node->pipe; > > if (vb->vb2_queue->type == V4L2_BUF_TYPE_META_CAPTURE || > vb->vb2_queue->type == V4L2_BUF_TYPE_META_OUTPUT) > @@ -350,42 +351,18 @@ static void ipu3_vb2_buf_queue(struct vb2_buffer *vb) Looks like this might need a rebase. This function seems to be called imgu_vb2_buf_queue() in current linux-next. Other functions seem to have been changed from ipu3_ to imgu_ as well. > else > need_bytes = node->vdev_fmt.fmt.pix_mp.plane_fmt[0].sizeimage; > > - if (queue == IPU3_CSS_QUEUE_PARAMS) { > - unsigned long payload = vb2_get_plane_payload(vb, 0); > - struct vb2_v4l2_buffer *buf = > - container_of(vb, struct vb2_v4l2_buffer, vb2_buf); > - int r = -EINVAL; > - > - if (payload == 0) { > - payload = need_bytes; > - vb2_set_plane_payload(vb, 0, payload); > - } > - if (payload >= need_bytes) > - r = ipu3_css_set_parameters(&imgu->css, pipe, > - vb2_plane_vaddr(vb, 0)); > - buf->flags = V4L2_BUF_FLAG_DONE; > - vb2_buffer_done(vb, r == 0 ? VB2_BUF_STATE_DONE > - : VB2_BUF_STATE_ERROR); This patch removes the check of the buffer contents. We should probably check the payload size in the vb2 .buf_prepare callback and fail if it doesn't match the parameter struct size. > - > - } else { > - struct imgu_buffer *buf = container_of(vb, struct imgu_buffer, > - vid_buf.vbb.vb2_buf); > - > - mutex_lock(&imgu->lock); > + mutex_lock(&imgu->lock); > + if (queue != IPU3_CSS_QUEUE_PARAMS) > ipu3_css_buf_init(&buf->css_buf, queue, buf->map.daddr); > - list_add_tail(&buf->vid_buf.list, > - &node->buffers); > - mutex_unlock(&imgu->lock); > + list_add_tail(&buf->vid_buf.list, &node->buffers); > + mutex_unlock(&imgu->lock); > > - vb2_set_plane_payload(&buf->vid_buf.vbb.vb2_buf, 0, need_bytes); > - > - if (imgu->streaming) > - imgu_queue_buffers(imgu, false, pipe); > - } > + vb2_set_plane_payload(vb, 0, need_bytes); Uhm, the driver is expected to only set the payload for CAPTURE buffers. > + if (imgu->streaming) > + imgu_queue_buffers(imgu, false, node->pipe); > > dev_dbg(&imgu->pci_dev->dev, "%s for pipe %d node %d", __func__, > node->pipe, node->id); > - > } > > static int ipu3_vb2_queue_setup(struct vb2_queue *vq, > diff --git a/drivers/staging/media/ipu3/ipu3.c b/drivers/staging/media/ipu3/ipu3.c > index 839d9398f8e9..25e121eebee2 100644 > --- a/drivers/staging/media/ipu3/ipu3.c > +++ b/drivers/staging/media/ipu3/ipu3.c > @@ -246,6 +246,30 @@ int imgu_queue_buffers(struct imgu_device *imgu, bool initial, unsigned int pipe > dev_warn(&imgu->pci_dev->dev, > "Vf not enabled, ignore queue"); > continue; > + } else if (node == IMGU_NODE_PARAMS && > + imgu_pipe->nodes[node].enabled) { > + struct vb2_buffer *vb; > + struct ipu3_vb2_buffer *ivb; > + > + if (list_empty(&imgu_pipe->nodes[node].buffers)) > + /* No parameters for this frame. */ > + continue; > + ivb = list_first_entry(&imgu_pipe->nodes[node].buffers, > + struct ipu3_vb2_buffer, list); > + vb = &ivb->vbb.vb2_buf; > + r = ipu3_css_set_parameters(&imgu->css, pipe, > + vb2_plane_vaddr(vb, 0)); > + if (r) { > + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); > + dev_err(&imgu->pci_dev->dev, > + "set parameters failed.\n"); > + } else { > + vb2_buffer_done(vb, VB2_BUF_STATE_DONE); > + dev_dbg(&imgu->pci_dev->dev, > + "queue user parameters %d to css.\n", > + vb->index); > + } > + list_del(&ivb->list); nit: I'd rewrite the code as below, to maintain the kernel convention of using ifs to handle special cases and keep the regular code flow at the same level of indentation. r = ipu3_css_set_parameters(&imgu->css, pipe, vb2_plane_vaddr(vb, 0)); list_del(&ivb->list); if (r) { vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); dev_err(&imgu->pci_dev->dev, "set parameters failed.\n"); continue; } vb2_buffer_done(vb, VB2_BUF_STATE_DONE); dev_dbg(&imgu->pci_dev->dev, "queue user parameters %d to css.\n", vb->index); Best regards, Tomasz
On 03/12/2019 01:33 PM, Tomasz Figa wrote: > Hi Bingbu, > > On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: >> From: Bingbu Cao <bingbu.cao@intel.com> >> >> Current ImgU driver processes and releases the parameter buffer >> immediately after queued from user. This does not align with other >> image buffers which are grouped in sets and used for the same frame. >> If user queues multiple parameter buffers continuously, only the last >> one will take effect. >> To make consistent buffers usage, this patch changes the parameter >> buffer handling and group parameter buffer with other image buffers >> for each frame. > Thanks for the patch. Please see my comments inline. > >> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >> --- >> drivers/staging/media/ipu3/ipu3-css.c | 5 ----- >> drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- >> drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ >> 3 files changed, 33 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c >> index b9354d2bb692..bcb1d436bc98 100644 >> --- a/drivers/staging/media/ipu3/ipu3-css.c >> +++ b/drivers/staging/media/ipu3/ipu3-css.c >> @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, >> obgrid_size = ipu3_css_fw_obgrid_size(bi); >> stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; >> >> - /* >> - * TODO(b/118782861): If userspace queues more than 4 buffers, the >> - * parameters from previous buffers will be overwritten. Fix the driver >> - * not to allow this. >> - */ > Wouldn't this still happen even with current patch? > imgu_queue_buffers() supposedly queues "as many buffers to CSS as > possible". This means that if the userspace queues more than 4 > complete frames, we still end up overwriting the parameter buffers in > the pool. Please correct me if I'm wrong. The parameter buffers are queued to CSS sequentially and queue one parameter along with one input buffer once ready, all the data and parameter buffers are tied together to queue to the CSS. If userspace queue more parameter buffers then input buffer, they are pending on the buffer list. The parameters pool actually was just used to keep the last valid parameter, if necessary it will be used to copy and combine with latest parameter to queue. When driver reach at ipu3_css_set_parameters(), driver will queue the parameter soon, no buffers pending as one patch before change the pool mechanism. > >> ipu3_css_pool_get(&css_pipe->pool.parameter_set_info); >> param_set = ipu3_css_pool_last(&css_pipe->pool.parameter_set_info, >> 0)->vaddr; >> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c >> index e758a650ad2b..7812c7324893 100644 >> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c >> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c >> @@ -341,8 +341,9 @@ static void ipu3_vb2_buf_queue(struct vb2_buffer *vb) >> struct imgu_video_device *node = >> container_of(vb->vb2_queue, struct imgu_video_device, vbq); >> unsigned int queue = imgu_node_to_queue(node->id); >> + struct imgu_buffer *buf = container_of(vb, struct imgu_buffer, >> + vid_buf.vbb.vb2_buf); >> unsigned long need_bytes; >> - unsigned int pipe = node->pipe; >> >> if (vb->vb2_queue->type == V4L2_BUF_TYPE_META_CAPTURE || >> vb->vb2_queue->type == V4L2_BUF_TYPE_META_OUTPUT) >> @@ -350,42 +351,18 @@ static void ipu3_vb2_buf_queue(struct vb2_buffer *vb) > Looks like this might need a rebase. This function seems to be called > imgu_vb2_buf_queue() in current linux-next. Other functions seem to > have been changed from ipu3_ to imgu_ as well. Thanks, I will rebase it. > >> else >> need_bytes = node->vdev_fmt.fmt.pix_mp.plane_fmt[0].sizeimage; >> >> - if (queue == IPU3_CSS_QUEUE_PARAMS) { >> - unsigned long payload = vb2_get_plane_payload(vb, 0); >> - struct vb2_v4l2_buffer *buf = >> - container_of(vb, struct vb2_v4l2_buffer, vb2_buf); >> - int r = -EINVAL; >> - >> - if (payload == 0) { >> - payload = need_bytes; >> - vb2_set_plane_payload(vb, 0, payload); >> - } >> - if (payload >= need_bytes) >> - r = ipu3_css_set_parameters(&imgu->css, pipe, >> - vb2_plane_vaddr(vb, 0)); >> - buf->flags = V4L2_BUF_FLAG_DONE; >> - vb2_buffer_done(vb, r == 0 ? VB2_BUF_STATE_DONE >> - : VB2_BUF_STATE_ERROR); > This patch removes the check of the buffer contents. We should > probably check the payload size in the vb2 .buf_prepare callback and > fail if it doesn't match the parameter struct size. Thanks, will add the check in next patch. > >> - >> - } else { >> - struct imgu_buffer *buf = container_of(vb, struct imgu_buffer, >> - vid_buf.vbb.vb2_buf); >> - >> - mutex_lock(&imgu->lock); >> + mutex_lock(&imgu->lock); >> + if (queue != IPU3_CSS_QUEUE_PARAMS) >> ipu3_css_buf_init(&buf->css_buf, queue, buf->map.daddr); >> - list_add_tail(&buf->vid_buf.list, >> - &node->buffers); >> - mutex_unlock(&imgu->lock); >> + list_add_tail(&buf->vid_buf.list, &node->buffers); >> + mutex_unlock(&imgu->lock); >> >> - vb2_set_plane_payload(&buf->vid_buf.vbb.vb2_buf, 0, need_bytes); >> - >> - if (imgu->streaming) >> - imgu_queue_buffers(imgu, false, pipe); >> - } >> + vb2_set_plane_payload(vb, 0, need_bytes); > Uhm, the driver is expected to only set the payload for CAPTURE buffers. Ack. > >> + if (imgu->streaming) >> + imgu_queue_buffers(imgu, false, node->pipe); >> >> dev_dbg(&imgu->pci_dev->dev, "%s for pipe %d node %d", __func__, >> node->pipe, node->id); >> - >> } >> >> static int ipu3_vb2_queue_setup(struct vb2_queue *vq, >> diff --git a/drivers/staging/media/ipu3/ipu3.c b/drivers/staging/media/ipu3/ipu3.c >> index 839d9398f8e9..25e121eebee2 100644 >> --- a/drivers/staging/media/ipu3/ipu3.c >> +++ b/drivers/staging/media/ipu3/ipu3.c >> @@ -246,6 +246,30 @@ int imgu_queue_buffers(struct imgu_device *imgu, bool initial, unsigned int pipe >> dev_warn(&imgu->pci_dev->dev, >> "Vf not enabled, ignore queue"); >> continue; >> + } else if (node == IMGU_NODE_PARAMS && >> + imgu_pipe->nodes[node].enabled) { >> + struct vb2_buffer *vb; >> + struct ipu3_vb2_buffer *ivb; >> + >> + if (list_empty(&imgu_pipe->nodes[node].buffers)) >> + /* No parameters for this frame. */ >> + continue; >> + ivb = list_first_entry(&imgu_pipe->nodes[node].buffers, >> + struct ipu3_vb2_buffer, list); >> + vb = &ivb->vbb.vb2_buf; >> + r = ipu3_css_set_parameters(&imgu->css, pipe, >> + vb2_plane_vaddr(vb, 0)); >> + if (r) { >> + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); >> + dev_err(&imgu->pci_dev->dev, >> + "set parameters failed.\n"); >> + } else { >> + vb2_buffer_done(vb, VB2_BUF_STATE_DONE); >> + dev_dbg(&imgu->pci_dev->dev, >> + "queue user parameters %d to css.\n", >> + vb->index); >> + } >> + list_del(&ivb->list); > nit: I'd rewrite the code as below, to maintain the kernel convention > of using ifs to handle special cases and keep the regular code flow at > the same level of indentation. > > r = ipu3_css_set_parameters(&imgu->css, pipe, > vb2_plane_vaddr(vb, 0)); > list_del(&ivb->list); > if (r) { > vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); > dev_err(&imgu->pci_dev->dev, > "set parameters failed.\n"); > continue; > } > > vb2_buffer_done(vb, VB2_BUF_STATE_DONE); > dev_dbg(&imgu->pci_dev->dev, > "queue user parameters %d to css.\n", > vb->index); Ack. > > Best regards, > Tomasz >
On Tue, Mar 12, 2019 at 3:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > > > > On 03/12/2019 01:33 PM, Tomasz Figa wrote: > > Hi Bingbu, > > > > On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: > >> From: Bingbu Cao <bingbu.cao@intel.com> > >> > >> Current ImgU driver processes and releases the parameter buffer > >> immediately after queued from user. This does not align with other > >> image buffers which are grouped in sets and used for the same frame. > >> If user queues multiple parameter buffers continuously, only the last > >> one will take effect. > >> To make consistent buffers usage, this patch changes the parameter > >> buffer handling and group parameter buffer with other image buffers > >> for each frame. > > Thanks for the patch. Please see my comments inline. > > > >> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> > >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > >> --- > >> drivers/staging/media/ipu3/ipu3-css.c | 5 ----- > >> drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- > >> drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ > >> 3 files changed, 33 insertions(+), 37 deletions(-) > >> > >> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c > >> index b9354d2bb692..bcb1d436bc98 100644 > >> --- a/drivers/staging/media/ipu3/ipu3-css.c > >> +++ b/drivers/staging/media/ipu3/ipu3-css.c > >> @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, > >> obgrid_size = ipu3_css_fw_obgrid_size(bi); > >> stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; > >> > >> - /* > >> - * TODO(b/118782861): If userspace queues more than 4 buffers, the > >> - * parameters from previous buffers will be overwritten. Fix the driver > >> - * not to allow this. > >> - */ > > Wouldn't this still happen even with current patch? > > imgu_queue_buffers() supposedly queues "as many buffers to CSS as > > possible". This means that if the userspace queues more than 4 > > complete frames, we still end up overwriting the parameter buffers in > > the pool. Please correct me if I'm wrong. > The parameter buffers are queued to CSS sequentially and queue one > parameter along with one input buffer once ready, all the data and > parameter buffers are tied together to queue to the CSS. If userspace > queue more parameter buffers then input buffer, they are pending on the > buffer list. It doesn't seem to be what the code does. I'm talking about the following example: Queue OUT buffer 1 Queue PARAM buffer 1 Queue IN buffer 1 Queue OUT buffer 2 Queue PARAM buffer 2 Queue IN buffer 2 Queue OUT buffer 3 Queue PARAM buffer 3 Queue IN buffer 3 Queue OUT buffer 4 Queue PARAM buffer 4 Queue IN buffer 4 Queue OUT buffer 5 Queue PARAM buffer 5 Queue IN buffer 5 All the operations happening exactly one after each other. How would the code prevent the 5th PARAM buffer to be queued to the IMGU, after the 5th IN buffer is queued? As I said, imgu_queue_buffers() just queues as many buffers of each type as there are IN buffers available. Best regards, Tomasz
On Tue, Mar 12, 2019 at 5:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > > > > On 03/12/2019 03:43 PM, Tomasz Figa wrote: > > On Tue, Mar 12, 2019 at 3:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >> > >> > >> On 03/12/2019 01:33 PM, Tomasz Figa wrote: > >>> Hi Bingbu, > >>> > >>> On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: > >>>> From: Bingbu Cao <bingbu.cao@intel.com> > >>>> > >>>> Current ImgU driver processes and releases the parameter buffer > >>>> immediately after queued from user. This does not align with other > >>>> image buffers which are grouped in sets and used for the same frame. > >>>> If user queues multiple parameter buffers continuously, only the last > >>>> one will take effect. > >>>> To make consistent buffers usage, this patch changes the parameter > >>>> buffer handling and group parameter buffer with other image buffers > >>>> for each frame. > >>> Thanks for the patch. Please see my comments inline. > >>> > >>>> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> > >>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > >>>> --- > >>>> drivers/staging/media/ipu3/ipu3-css.c | 5 ----- > >>>> drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- > >>>> drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ > >>>> 3 files changed, 33 insertions(+), 37 deletions(-) > >>>> > >>>> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c > >>>> index b9354d2bb692..bcb1d436bc98 100644 > >>>> --- a/drivers/staging/media/ipu3/ipu3-css.c > >>>> +++ b/drivers/staging/media/ipu3/ipu3-css.c > >>>> @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, > >>>> obgrid_size = ipu3_css_fw_obgrid_size(bi); > >>>> stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; > >>>> > >>>> - /* > >>>> - * TODO(b/118782861): If userspace queues more than 4 buffers, the > >>>> - * parameters from previous buffers will be overwritten. Fix the driver > >>>> - * not to allow this. > >>>> - */ > >>> Wouldn't this still happen even with current patch? > >>> imgu_queue_buffers() supposedly queues "as many buffers to CSS as > >>> possible". This means that if the userspace queues more than 4 > >>> complete frames, we still end up overwriting the parameter buffers in > >>> the pool. Please correct me if I'm wrong. > >> The parameter buffers are queued to CSS sequentially and queue one > >> parameter along with one input buffer once ready, all the data and > >> parameter buffers are tied together to queue to the CSS. If userspace > >> queue more parameter buffers then input buffer, they are pending on the > >> buffer list. > > It doesn't seem to be what the code does. I'm talking about the > > following example: > > > > Queue OUT buffer 1 > > Queue PARAM buffer 1 > > Queue IN buffer 1 > > Queue OUT buffer 2 > > Queue PARAM buffer 2 > > Queue IN buffer 2 > > Queue OUT buffer 3 > > Queue PARAM buffer 3 > > Queue IN buffer 3 > > Queue OUT buffer 4 > > Queue PARAM buffer 4 > > Queue IN buffer 4 > > Queue OUT buffer 5 > > Queue PARAM buffer 5 > > Queue IN buffer 5 > > > > All the operations happening exactly one after each other. How would > > the code prevent the 5th PARAM buffer to be queued to the IMGU, after > > the 5th IN buffer is queued? As I said, imgu_queue_buffers() just > > queues as many buffers of each type as there are IN buffers available. > So the parameter pool now is only used as record last valid parameter not > used as a list or cached, all the parameters will be queued to CSS as soon as > possible(if queue for CSS is not full). > As the size of pool now is a bit confusing, I think we can shrink the its value > for each pipe to 2. I don't follow. Don't we take one buffer from the pool, fill in the parameters in hardware format there and then queue that buffer from the pool to the ISP? The ISP wouldn't read those parameters from the buffer until the previous frame is processed, would it? > > The buffer queue size is limited for CSS, if massive buffers from > user pass down and the css queue is full, driver will get -EBUSY > and return the buffers to user with error. > That's not a correct behavior. If the VB2 queue has N buffers, the userspace needs to be able to queue all the N buffers. The driver should account the queued buffers internally and queue them to the hardware/firmware only as much as the hardware/firmware allows. Best regards, Tomasz
On 03/12/2019 03:43 PM, Tomasz Figa wrote: > On Tue, Mar 12, 2019 at 3:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >> >> >> On 03/12/2019 01:33 PM, Tomasz Figa wrote: >>> Hi Bingbu, >>> >>> On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: >>>> From: Bingbu Cao <bingbu.cao@intel.com> >>>> >>>> Current ImgU driver processes and releases the parameter buffer >>>> immediately after queued from user. This does not align with other >>>> image buffers which are grouped in sets and used for the same frame. >>>> If user queues multiple parameter buffers continuously, only the last >>>> one will take effect. >>>> To make consistent buffers usage, this patch changes the parameter >>>> buffer handling and group parameter buffer with other image buffers >>>> for each frame. >>> Thanks for the patch. Please see my comments inline. >>> >>>> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> >>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >>>> --- >>>> drivers/staging/media/ipu3/ipu3-css.c | 5 ----- >>>> drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- >>>> drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ >>>> 3 files changed, 33 insertions(+), 37 deletions(-) >>>> >>>> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c >>>> index b9354d2bb692..bcb1d436bc98 100644 >>>> --- a/drivers/staging/media/ipu3/ipu3-css.c >>>> +++ b/drivers/staging/media/ipu3/ipu3-css.c >>>> @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, >>>> obgrid_size = ipu3_css_fw_obgrid_size(bi); >>>> stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; >>>> >>>> - /* >>>> - * TODO(b/118782861): If userspace queues more than 4 buffers, the >>>> - * parameters from previous buffers will be overwritten. Fix the driver >>>> - * not to allow this. >>>> - */ >>> Wouldn't this still happen even with current patch? >>> imgu_queue_buffers() supposedly queues "as many buffers to CSS as >>> possible". This means that if the userspace queues more than 4 >>> complete frames, we still end up overwriting the parameter buffers in >>> the pool. Please correct me if I'm wrong. >> The parameter buffers are queued to CSS sequentially and queue one >> parameter along with one input buffer once ready, all the data and >> parameter buffers are tied together to queue to the CSS. If userspace >> queue more parameter buffers then input buffer, they are pending on the >> buffer list. > It doesn't seem to be what the code does. I'm talking about the > following example: > > Queue OUT buffer 1 > Queue PARAM buffer 1 > Queue IN buffer 1 > Queue OUT buffer 2 > Queue PARAM buffer 2 > Queue IN buffer 2 > Queue OUT buffer 3 > Queue PARAM buffer 3 > Queue IN buffer 3 > Queue OUT buffer 4 > Queue PARAM buffer 4 > Queue IN buffer 4 > Queue OUT buffer 5 > Queue PARAM buffer 5 > Queue IN buffer 5 > > All the operations happening exactly one after each other. How would > the code prevent the 5th PARAM buffer to be queued to the IMGU, after > the 5th IN buffer is queued? As I said, imgu_queue_buffers() just > queues as many buffers of each type as there are IN buffers available. So the parameter pool now is only used as record last valid parameter not used as a list or cached, all the parameters will be queued to CSS as soon as possible(if queue for CSS is not full). As the size of pool now is a bit confusing, I think we can shrink the its value for each pipe to 2. The buffer queue size is limited for CSS, if massive buffers from user pass down and the css queue is full, driver will get -EBUSY and return the buffers to user with error. > > Best regards, > Tomasz >
On 03/12/2019 04:54 PM, Tomasz Figa wrote: > On Tue, Mar 12, 2019 at 5:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >> >> >> On 03/12/2019 03:43 PM, Tomasz Figa wrote: >>> On Tue, Mar 12, 2019 at 3:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >>>> >>>> On 03/12/2019 01:33 PM, Tomasz Figa wrote: >>>>> Hi Bingbu, >>>>> >>>>> On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: >>>>>> From: Bingbu Cao <bingbu.cao@intel.com> >>>>>> >>>>>> Current ImgU driver processes and releases the parameter buffer >>>>>> immediately after queued from user. This does not align with other >>>>>> image buffers which are grouped in sets and used for the same frame. >>>>>> If user queues multiple parameter buffers continuously, only the last >>>>>> one will take effect. >>>>>> To make consistent buffers usage, this patch changes the parameter >>>>>> buffer handling and group parameter buffer with other image buffers >>>>>> for each frame. >>>>> Thanks for the patch. Please see my comments inline. >>>>> >>>>>> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> >>>>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >>>>>> --- >>>>>> drivers/staging/media/ipu3/ipu3-css.c | 5 ----- >>>>>> drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- >>>>>> drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ >>>>>> 3 files changed, 33 insertions(+), 37 deletions(-) >>>>>> >>>>>> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c >>>>>> index b9354d2bb692..bcb1d436bc98 100644 >>>>>> --- a/drivers/staging/media/ipu3/ipu3-css.c >>>>>> +++ b/drivers/staging/media/ipu3/ipu3-css.c >>>>>> @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, >>>>>> obgrid_size = ipu3_css_fw_obgrid_size(bi); >>>>>> stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; >>>>>> >>>>>> - /* >>>>>> - * TODO(b/118782861): If userspace queues more than 4 buffers, the >>>>>> - * parameters from previous buffers will be overwritten. Fix the driver >>>>>> - * not to allow this. >>>>>> - */ >>>>> Wouldn't this still happen even with current patch? >>>>> imgu_queue_buffers() supposedly queues "as many buffers to CSS as >>>>> possible". This means that if the userspace queues more than 4 >>>>> complete frames, we still end up overwriting the parameter buffers in >>>>> the pool. Please correct me if I'm wrong. >>>> The parameter buffers are queued to CSS sequentially and queue one >>>> parameter along with one input buffer once ready, all the data and >>>> parameter buffers are tied together to queue to the CSS. If userspace >>>> queue more parameter buffers then input buffer, they are pending on the >>>> buffer list. >>> It doesn't seem to be what the code does. I'm talking about the >>> following example: >>> >>> Queue OUT buffer 1 >>> Queue PARAM buffer 1 >>> Queue IN buffer 1 >>> Queue OUT buffer 2 >>> Queue PARAM buffer 2 >>> Queue IN buffer 2 >>> Queue OUT buffer 3 >>> Queue PARAM buffer 3 >>> Queue IN buffer 3 >>> Queue OUT buffer 4 >>> Queue PARAM buffer 4 >>> Queue IN buffer 4 >>> Queue OUT buffer 5 >>> Queue PARAM buffer 5 >>> Queue IN buffer 5 >>> >>> All the operations happening exactly one after each other. How would >>> the code prevent the 5th PARAM buffer to be queued to the IMGU, after >>> the 5th IN buffer is queued? As I said, imgu_queue_buffers() just >>> queues as many buffers of each type as there are IN buffers available. >> So the parameter pool now is only used as record last valid parameter not >> used as a list or cached, all the parameters will be queued to CSS as soon as >> possible(if queue for CSS is not full). >> As the size of pool now is a bit confusing, I think we can shrink the its value >> for each pipe to 2. > I don't follow. Don't we take one buffer from the pool, fill in the > parameters in hardware format there and then queue that buffer from > the pool to the ISP? The ISP wouldn't read those parameters from the > buffer until the previous frame is processed, would it? Hi, Tomasz, Currently, driver did not take the buffer from pool to queue to ISP, it just queue the parameter buffer along with input frame buffer depends on each user queue request. You are right, if user queue massive buffers one time, it will cause the firmware queue full. Driver should keep the buffer in its list instead of returning back to user irresponsibly. We are thinking about queue one group of buffers(input, output and params) to ISP one time and wait the pipeline finished and then queue next group of buffers. All the buffers are pending on the buffer list. What do you think about this behavior? > >> The buffer queue size is limited for CSS, if massive buffers from >> user pass down and the css queue is full, driver will get -EBUSY >> and return the buffers to user with error. >> > That's not a correct behavior. If the VB2 queue has N buffers, the > userspace needs to be able to queue all the N buffers. The driver > should account the queued buffers internally and queue them to the > hardware/firmware only as much as the hardware/firmware allows. Got it, thanks. > > Best regards, > Tomasz >
Hi Bingbu, On Wed, Mar 13, 2019 at 1:25 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > > > > On 03/12/2019 04:54 PM, Tomasz Figa wrote: > > On Tue, Mar 12, 2019 at 5:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >> > >> > >> On 03/12/2019 03:43 PM, Tomasz Figa wrote: > >>> On Tue, Mar 12, 2019 at 3:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >>>> > >>>> On 03/12/2019 01:33 PM, Tomasz Figa wrote: > >>>>> Hi Bingbu, > >>>>> > >>>>> On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: > >>>>>> From: Bingbu Cao <bingbu.cao@intel.com> > >>>>>> > >>>>>> Current ImgU driver processes and releases the parameter buffer > >>>>>> immediately after queued from user. This does not align with other > >>>>>> image buffers which are grouped in sets and used for the same frame. > >>>>>> If user queues multiple parameter buffers continuously, only the last > >>>>>> one will take effect. > >>>>>> To make consistent buffers usage, this patch changes the parameter > >>>>>> buffer handling and group parameter buffer with other image buffers > >>>>>> for each frame. > >>>>> Thanks for the patch. Please see my comments inline. > >>>>> > >>>>>> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> > >>>>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > >>>>>> --- > >>>>>> drivers/staging/media/ipu3/ipu3-css.c | 5 ----- > >>>>>> drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- > >>>>>> drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ > >>>>>> 3 files changed, 33 insertions(+), 37 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c > >>>>>> index b9354d2bb692..bcb1d436bc98 100644 > >>>>>> --- a/drivers/staging/media/ipu3/ipu3-css.c > >>>>>> +++ b/drivers/staging/media/ipu3/ipu3-css.c > >>>>>> @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, > >>>>>> obgrid_size = ipu3_css_fw_obgrid_size(bi); > >>>>>> stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; > >>>>>> > >>>>>> - /* > >>>>>> - * TODO(b/118782861): If userspace queues more than 4 buffers, the > >>>>>> - * parameters from previous buffers will be overwritten. Fix the driver > >>>>>> - * not to allow this. > >>>>>> - */ > >>>>> Wouldn't this still happen even with current patch? > >>>>> imgu_queue_buffers() supposedly queues "as many buffers to CSS as > >>>>> possible". This means that if the userspace queues more than 4 > >>>>> complete frames, we still end up overwriting the parameter buffers in > >>>>> the pool. Please correct me if I'm wrong. > >>>> The parameter buffers are queued to CSS sequentially and queue one > >>>> parameter along with one input buffer once ready, all the data and > >>>> parameter buffers are tied together to queue to the CSS. If userspace > >>>> queue more parameter buffers then input buffer, they are pending on the > >>>> buffer list. > >>> It doesn't seem to be what the code does. I'm talking about the > >>> following example: > >>> > >>> Queue OUT buffer 1 > >>> Queue PARAM buffer 1 > >>> Queue IN buffer 1 > >>> Queue OUT buffer 2 > >>> Queue PARAM buffer 2 > >>> Queue IN buffer 2 > >>> Queue OUT buffer 3 > >>> Queue PARAM buffer 3 > >>> Queue IN buffer 3 > >>> Queue OUT buffer 4 > >>> Queue PARAM buffer 4 > >>> Queue IN buffer 4 > >>> Queue OUT buffer 5 > >>> Queue PARAM buffer 5 > >>> Queue IN buffer 5 > >>> > >>> All the operations happening exactly one after each other. How would > >>> the code prevent the 5th PARAM buffer to be queued to the IMGU, after > >>> the 5th IN buffer is queued? As I said, imgu_queue_buffers() just > >>> queues as many buffers of each type as there are IN buffers available. > >> So the parameter pool now is only used as record last valid parameter not > >> used as a list or cached, all the parameters will be queued to CSS as soon as > >> possible(if queue for CSS is not full). > >> As the size of pool now is a bit confusing, I think we can shrink the its value > >> for each pipe to 2. > > I don't follow. Don't we take one buffer from the pool, fill in the > > parameters in hardware format there and then queue that buffer from > > the pool to the ISP? The ISP wouldn't read those parameters from the > > buffer until the previous frame is processed, would it? > Hi, Tomasz, > > Currently, driver did not take the buffer from pool to queue to ISP, > it just queue the parameter buffer along with input frame buffer depends > on each user queue request. > > You are right, if user queue massive buffers one time, it will cause > the firmware queue full. Driver should keep the buffer in its list > instead of returning back to user irresponsibly. > > We are thinking about queue one group of buffers(input, output and params) > to ISP one time and wait the pipeline finished and then queue next group > of buffers. All the buffers are pending on the buffer list. > What do you think about this behavior? Sorry, I was sure I replied to your email, but apparently I didn't. Yes, that would certainly work, but wouldn't it introduce pipeline bubbles, potentially affecting the performance? Best regards, Tomasz
On Mon, Mar 25, 2019 at 1:12 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > > > > On 3/25/19 11:16 AM, Tomasz Figa wrote: > > Hi Bingbu, > > > > On Wed, Mar 13, 2019 at 1:25 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >> > >> > >> > >> On 03/12/2019 04:54 PM, Tomasz Figa wrote: > >>> On Tue, Mar 12, 2019 at 5:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >>>> > >>>> > >>>> On 03/12/2019 03:43 PM, Tomasz Figa wrote: > >>>>> On Tue, Mar 12, 2019 at 3:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >>>>>> > >>>>>> On 03/12/2019 01:33 PM, Tomasz Figa wrote: > >>>>>>> Hi Bingbu, > >>>>>>> > >>>>>>> On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: > >>>>>>>> From: Bingbu Cao <bingbu.cao@intel.com> > >>>>>>>> > >>>>>>>> Current ImgU driver processes and releases the parameter buffer > >>>>>>>> immediately after queued from user. This does not align with other > >>>>>>>> image buffers which are grouped in sets and used for the same frame. > >>>>>>>> If user queues multiple parameter buffers continuously, only the last > >>>>>>>> one will take effect. > >>>>>>>> To make consistent buffers usage, this patch changes the parameter > >>>>>>>> buffer handling and group parameter buffer with other image buffers > >>>>>>>> for each frame. > >>>>>>> Thanks for the patch. Please see my comments inline. > >>>>>>> > >>>>>>>> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> > >>>>>>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > >>>>>>>> --- > >>>>>>>> drivers/staging/media/ipu3/ipu3-css.c | 5 ----- > >>>>>>>> drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- > >>>>>>>> drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ > >>>>>>>> 3 files changed, 33 insertions(+), 37 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c > >>>>>>>> index b9354d2bb692..bcb1d436bc98 100644 > >>>>>>>> --- a/drivers/staging/media/ipu3/ipu3-css.c > >>>>>>>> +++ b/drivers/staging/media/ipu3/ipu3-css.c > >>>>>>>> @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, > >>>>>>>> obgrid_size = ipu3_css_fw_obgrid_size(bi); > >>>>>>>> stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; > >>>>>>>> > >>>>>>>> - /* > >>>>>>>> - * TODO(b/118782861): If userspace queues more than 4 buffers, the > >>>>>>>> - * parameters from previous buffers will be overwritten. Fix the driver > >>>>>>>> - * not to allow this. > >>>>>>>> - */ > >>>>>>> Wouldn't this still happen even with current patch? > >>>>>>> imgu_queue_buffers() supposedly queues "as many buffers to CSS as > >>>>>>> possible". This means that if the userspace queues more than 4 > >>>>>>> complete frames, we still end up overwriting the parameter buffers in > >>>>>>> the pool. Please correct me if I'm wrong. > >>>>>> The parameter buffers are queued to CSS sequentially and queue one > >>>>>> parameter along with one input buffer once ready, all the data and > >>>>>> parameter buffers are tied together to queue to the CSS. If userspace > >>>>>> queue more parameter buffers then input buffer, they are pending on the > >>>>>> buffer list. > >>>>> It doesn't seem to be what the code does. I'm talking about the > >>>>> following example: > >>>>> > >>>>> Queue OUT buffer 1 > >>>>> Queue PARAM buffer 1 > >>>>> Queue IN buffer 1 > >>>>> Queue OUT buffer 2 > >>>>> Queue PARAM buffer 2 > >>>>> Queue IN buffer 2 > >>>>> Queue OUT buffer 3 > >>>>> Queue PARAM buffer 3 > >>>>> Queue IN buffer 3 > >>>>> Queue OUT buffer 4 > >>>>> Queue PARAM buffer 4 > >>>>> Queue IN buffer 4 > >>>>> Queue OUT buffer 5 > >>>>> Queue PARAM buffer 5 > >>>>> Queue IN buffer 5 > >>>>> > >>>>> All the operations happening exactly one after each other. How would > >>>>> the code prevent the 5th PARAM buffer to be queued to the IMGU, after > >>>>> the 5th IN buffer is queued? As I said, imgu_queue_buffers() just > >>>>> queues as many buffers of each type as there are IN buffers available. > >>>> So the parameter pool now is only used as record last valid parameter not > >>>> used as a list or cached, all the parameters will be queued to CSS as soon as > >>>> possible(if queue for CSS is not full). > >>>> As the size of pool now is a bit confusing, I think we can shrink the its value > >>>> for each pipe to 2. > >>> I don't follow. Don't we take one buffer from the pool, fill in the > >>> parameters in hardware format there and then queue that buffer from > >>> the pool to the ISP? The ISP wouldn't read those parameters from the > >>> buffer until the previous frame is processed, would it? > >> Hi, Tomasz, > >> > >> Currently, driver did not take the buffer from pool to queue to ISP, > >> it just queue the parameter buffer along with input frame buffer depends > >> on each user queue request. > >> > >> You are right, if user queue massive buffers one time, it will cause > >> the firmware queue full. Driver should keep the buffer in its list > >> instead of returning back to user irresponsibly. > >> > >> We are thinking about queue one group of buffers(input, output and params) > >> to ISP one time and wait the pipeline finished and then queue next group > >> of buffers. All the buffers are pending on the buffer list. > >> What do you think about this behavior? > > > > Sorry, I was sure I replied to your email, but apparently I didn't. > > > > Yes, that would certainly work, but wouldn't it introduce pipeline > > bubbles, potentially affecting the performance? > Hi, Tomasz, > > Thanks for your reply. > > The driver will queue the buffers to CSS immediately after previous > pipeline finished which is invoked in imgu_isr_threaded. > > The bubbles compared from before should be very small since current > camera HAL implementation in production will queue new buffers IFF all > the buffers dequeued from driver, as I know. If the firmware has a queue depth of 4, I think it would still be much better to use it. Would it really make the code much more complicated? I think you could just maintain a counter of queued buffers and keep refilling the queue whenever it's less than 4 and there are any buffers to queue.
On 3/25/19 12:20 PM, Tomasz Figa wrote: > On Mon, Mar 25, 2019 at 1:12 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >> >> >> >> On 3/25/19 11:16 AM, Tomasz Figa wrote: >>> Hi Bingbu, >>> >>> On Wed, Mar 13, 2019 at 1:25 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >>>> >>>> >>>> >>>> On 03/12/2019 04:54 PM, Tomasz Figa wrote: >>>>> On Tue, Mar 12, 2019 at 5:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >>>>>> >>>>>> >>>>>> On 03/12/2019 03:43 PM, Tomasz Figa wrote: >>>>>>> On Tue, Mar 12, 2019 at 3:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >>>>>>>> >>>>>>>> On 03/12/2019 01:33 PM, Tomasz Figa wrote: >>>>>>>>> Hi Bingbu, >>>>>>>>> >>>>>>>>> On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: >>>>>>>>>> From: Bingbu Cao <bingbu.cao@intel.com> >>>>>>>>>> >>>>>>>>>> Current ImgU driver processes and releases the parameter buffer >>>>>>>>>> immediately after queued from user. This does not align with other >>>>>>>>>> image buffers which are grouped in sets and used for the same frame. >>>>>>>>>> If user queues multiple parameter buffers continuously, only the last >>>>>>>>>> one will take effect. >>>>>>>>>> To make consistent buffers usage, this patch changes the parameter >>>>>>>>>> buffer handling and group parameter buffer with other image buffers >>>>>>>>>> for each frame. >>>>>>>>> Thanks for the patch. Please see my comments inline. >>>>>>>>> >>>>>>>>>> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> >>>>>>>>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >>>>>>>>>> --- >>>>>>>>>> drivers/staging/media/ipu3/ipu3-css.c | 5 ----- >>>>>>>>>> drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- >>>>>>>>>> drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ >>>>>>>>>> 3 files changed, 33 insertions(+), 37 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c >>>>>>>>>> index b9354d2bb692..bcb1d436bc98 100644 >>>>>>>>>> --- a/drivers/staging/media/ipu3/ipu3-css.c >>>>>>>>>> +++ b/drivers/staging/media/ipu3/ipu3-css.c >>>>>>>>>> @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, >>>>>>>>>> obgrid_size = ipu3_css_fw_obgrid_size(bi); >>>>>>>>>> stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; >>>>>>>>>> >>>>>>>>>> - /* >>>>>>>>>> - * TODO(b/118782861): If userspace queues more than 4 buffers, the >>>>>>>>>> - * parameters from previous buffers will be overwritten. Fix the driver >>>>>>>>>> - * not to allow this. >>>>>>>>>> - */ >>>>>>>>> Wouldn't this still happen even with current patch? >>>>>>>>> imgu_queue_buffers() supposedly queues "as many buffers to CSS as >>>>>>>>> possible". This means that if the userspace queues more than 4 >>>>>>>>> complete frames, we still end up overwriting the parameter buffers in >>>>>>>>> the pool. Please correct me if I'm wrong. >>>>>>>> The parameter buffers are queued to CSS sequentially and queue one >>>>>>>> parameter along with one input buffer once ready, all the data and >>>>>>>> parameter buffers are tied together to queue to the CSS. If userspace >>>>>>>> queue more parameter buffers then input buffer, they are pending on the >>>>>>>> buffer list. >>>>>>> It doesn't seem to be what the code does. I'm talking about the >>>>>>> following example: >>>>>>> >>>>>>> Queue OUT buffer 1 >>>>>>> Queue PARAM buffer 1 >>>>>>> Queue IN buffer 1 >>>>>>> Queue OUT buffer 2 >>>>>>> Queue PARAM buffer 2 >>>>>>> Queue IN buffer 2 >>>>>>> Queue OUT buffer 3 >>>>>>> Queue PARAM buffer 3 >>>>>>> Queue IN buffer 3 >>>>>>> Queue OUT buffer 4 >>>>>>> Queue PARAM buffer 4 >>>>>>> Queue IN buffer 4 >>>>>>> Queue OUT buffer 5 >>>>>>> Queue PARAM buffer 5 >>>>>>> Queue IN buffer 5 >>>>>>> >>>>>>> All the operations happening exactly one after each other. How would >>>>>>> the code prevent the 5th PARAM buffer to be queued to the IMGU, after >>>>>>> the 5th IN buffer is queued? As I said, imgu_queue_buffers() just >>>>>>> queues as many buffers of each type as there are IN buffers available. >>>>>> So the parameter pool now is only used as record last valid parameter not >>>>>> used as a list or cached, all the parameters will be queued to CSS as soon as >>>>>> possible(if queue for CSS is not full). >>>>>> As the size of pool now is a bit confusing, I think we can shrink the its value >>>>>> for each pipe to 2. >>>>> I don't follow. Don't we take one buffer from the pool, fill in the >>>>> parameters in hardware format there and then queue that buffer from >>>>> the pool to the ISP? The ISP wouldn't read those parameters from the >>>>> buffer until the previous frame is processed, would it? >>>> Hi, Tomasz, >>>> >>>> Currently, driver did not take the buffer from pool to queue to ISP, >>>> it just queue the parameter buffer along with input frame buffer depends >>>> on each user queue request. >>>> >>>> You are right, if user queue massive buffers one time, it will cause >>>> the firmware queue full. Driver should keep the buffer in its list >>>> instead of returning back to user irresponsibly. >>>> >>>> We are thinking about queue one group of buffers(input, output and params) >>>> to ISP one time and wait the pipeline finished and then queue next group >>>> of buffers. All the buffers are pending on the buffer list. >>>> What do you think about this behavior? >>> >>> Sorry, I was sure I replied to your email, but apparently I didn't. >>> >>> Yes, that would certainly work, but wouldn't it introduce pipeline >>> bubbles, potentially affecting the performance? >> Hi, Tomasz, >> >> Thanks for your reply. >> >> The driver will queue the buffers to CSS immediately after previous >> pipeline finished which is invoked in imgu_isr_threaded. >> >> The bubbles compared from before should be very small since current >> camera HAL implementation in production will queue new buffers IFF all >> the buffers dequeued from driver, as I know. > > If the firmware has a queue depth of 4, I think it would still be much > better to use it. Would it really make the code much more complicated? > I think you could just maintain a counter of queued buffers and keep > refilling the queue whenever it's less than 4 and there are any > buffers to queue. Actually, firmware will use latest parameter queued and apply to frame, they are not consumed frame by frame. The parameter buffers are not used same way as frame buffers, so the pool in driver is just used for storing previous parameter and refilling fields within new coming parameter from user and combine with previous ones into a whole parameter. >
On Mon, Mar 25, 2019 at 6:52 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > > > > On 3/25/19 12:20 PM, Tomasz Figa wrote: > > On Mon, Mar 25, 2019 at 1:12 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >> > >> > >> > >> On 3/25/19 11:16 AM, Tomasz Figa wrote: > >>> Hi Bingbu, > >>> > >>> On Wed, Mar 13, 2019 at 1:25 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >>>> > >>>> > >>>> > >>>> On 03/12/2019 04:54 PM, Tomasz Figa wrote: > >>>>> On Tue, Mar 12, 2019 at 5:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >>>>>> > >>>>>> > >>>>>> On 03/12/2019 03:43 PM, Tomasz Figa wrote: > >>>>>>> On Tue, Mar 12, 2019 at 3:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >>>>>>>> > >>>>>>>> On 03/12/2019 01:33 PM, Tomasz Figa wrote: > >>>>>>>>> Hi Bingbu, > >>>>>>>>> > >>>>>>>>> On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: > >>>>>>>>>> From: Bingbu Cao <bingbu.cao@intel.com> > >>>>>>>>>> > >>>>>>>>>> Current ImgU driver processes and releases the parameter buffer > >>>>>>>>>> immediately after queued from user. This does not align with other > >>>>>>>>>> image buffers which are grouped in sets and used for the same frame. > >>>>>>>>>> If user queues multiple parameter buffers continuously, only the last > >>>>>>>>>> one will take effect. > >>>>>>>>>> To make consistent buffers usage, this patch changes the parameter > >>>>>>>>>> buffer handling and group parameter buffer with other image buffers > >>>>>>>>>> for each frame. > >>>>>>>>> Thanks for the patch. Please see my comments inline. > >>>>>>>>> > >>>>>>>>>> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> > >>>>>>>>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > >>>>>>>>>> --- > >>>>>>>>>> drivers/staging/media/ipu3/ipu3-css.c | 5 ----- > >>>>>>>>>> drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- > >>>>>>>>>> drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ > >>>>>>>>>> 3 files changed, 33 insertions(+), 37 deletions(-) > >>>>>>>>>> > >>>>>>>>>> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c > >>>>>>>>>> index b9354d2bb692..bcb1d436bc98 100644 > >>>>>>>>>> --- a/drivers/staging/media/ipu3/ipu3-css.c > >>>>>>>>>> +++ b/drivers/staging/media/ipu3/ipu3-css.c > >>>>>>>>>> @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, > >>>>>>>>>> obgrid_size = ipu3_css_fw_obgrid_size(bi); > >>>>>>>>>> stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; > >>>>>>>>>> > >>>>>>>>>> - /* > >>>>>>>>>> - * TODO(b/118782861): If userspace queues more than 4 buffers, the > >>>>>>>>>> - * parameters from previous buffers will be overwritten. Fix the driver > >>>>>>>>>> - * not to allow this. > >>>>>>>>>> - */ > >>>>>>>>> Wouldn't this still happen even with current patch? > >>>>>>>>> imgu_queue_buffers() supposedly queues "as many buffers to CSS as > >>>>>>>>> possible". This means that if the userspace queues more than 4 > >>>>>>>>> complete frames, we still end up overwriting the parameter buffers in > >>>>>>>>> the pool. Please correct me if I'm wrong. > >>>>>>>> The parameter buffers are queued to CSS sequentially and queue one > >>>>>>>> parameter along with one input buffer once ready, all the data and > >>>>>>>> parameter buffers are tied together to queue to the CSS. If userspace > >>>>>>>> queue more parameter buffers then input buffer, they are pending on the > >>>>>>>> buffer list. > >>>>>>> It doesn't seem to be what the code does. I'm talking about the > >>>>>>> following example: > >>>>>>> > >>>>>>> Queue OUT buffer 1 > >>>>>>> Queue PARAM buffer 1 > >>>>>>> Queue IN buffer 1 > >>>>>>> Queue OUT buffer 2 > >>>>>>> Queue PARAM buffer 2 > >>>>>>> Queue IN buffer 2 > >>>>>>> Queue OUT buffer 3 > >>>>>>> Queue PARAM buffer 3 > >>>>>>> Queue IN buffer 3 > >>>>>>> Queue OUT buffer 4 > >>>>>>> Queue PARAM buffer 4 > >>>>>>> Queue IN buffer 4 > >>>>>>> Queue OUT buffer 5 > >>>>>>> Queue PARAM buffer 5 > >>>>>>> Queue IN buffer 5 > >>>>>>> > >>>>>>> All the operations happening exactly one after each other. How would > >>>>>>> the code prevent the 5th PARAM buffer to be queued to the IMGU, after > >>>>>>> the 5th IN buffer is queued? As I said, imgu_queue_buffers() just > >>>>>>> queues as many buffers of each type as there are IN buffers available. > >>>>>> So the parameter pool now is only used as record last valid parameter not > >>>>>> used as a list or cached, all the parameters will be queued to CSS as soon as > >>>>>> possible(if queue for CSS is not full). > >>>>>> As the size of pool now is a bit confusing, I think we can shrink the its value > >>>>>> for each pipe to 2. > >>>>> I don't follow. Don't we take one buffer from the pool, fill in the > >>>>> parameters in hardware format there and then queue that buffer from > >>>>> the pool to the ISP? The ISP wouldn't read those parameters from the > >>>>> buffer until the previous frame is processed, would it? > >>>> Hi, Tomasz, > >>>> > >>>> Currently, driver did not take the buffer from pool to queue to ISP, > >>>> it just queue the parameter buffer along with input frame buffer depends > >>>> on each user queue request. > >>>> > >>>> You are right, if user queue massive buffers one time, it will cause > >>>> the firmware queue full. Driver should keep the buffer in its list > >>>> instead of returning back to user irresponsibly. > >>>> > >>>> We are thinking about queue one group of buffers(input, output and params) > >>>> to ISP one time and wait the pipeline finished and then queue next group > >>>> of buffers. All the buffers are pending on the buffer list. > >>>> What do you think about this behavior? > >>> > >>> Sorry, I was sure I replied to your email, but apparently I didn't. > >>> > >>> Yes, that would certainly work, but wouldn't it introduce pipeline > >>> bubbles, potentially affecting the performance? > >> Hi, Tomasz, > >> > >> Thanks for your reply. > >> > >> The driver will queue the buffers to CSS immediately after previous > >> pipeline finished which is invoked in imgu_isr_threaded. > >> > >> The bubbles compared from before should be very small since current > >> camera HAL implementation in production will queue new buffers IFF all > >> the buffers dequeued from driver, as I know. > > > > If the firmware has a queue depth of 4, I think it would still be much > > better to use it. Would it really make the code much more complicated? > > I think you could just maintain a counter of queued buffers and keep > > refilling the queue whenever it's less than 4 and there are any > > buffers to queue. > Actually, firmware will use latest parameter queued and apply to frame, > they are not consumed frame by frame. > The parameter buffers are not used same way as frame buffers, so the > pool in driver is just used for storing previous parameter and refilling > fields within new coming parameter from user and combine with previous > ones into a whole parameter. Hmm, that's a rather strange design. Well, in that case we can't queue more than 1 frame indeed, as otherwise we wouldn't be able to synchronize the parameters with the right frames. Best regards, Tomasz
On 3/26/19 12:03 PM, Tomasz Figa wrote: > On Mon, Mar 25, 2019 at 6:52 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >> >> >> >> On 3/25/19 12:20 PM, Tomasz Figa wrote: >>> On Mon, Mar 25, 2019 at 1:12 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >>>> >>>> >>>> >>>> On 3/25/19 11:16 AM, Tomasz Figa wrote: >>>>> Hi Bingbu, >>>>> >>>>> On Wed, Mar 13, 2019 at 1:25 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 03/12/2019 04:54 PM, Tomasz Figa wrote: >>>>>>> On Tue, Mar 12, 2019 at 5:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 03/12/2019 03:43 PM, Tomasz Figa wrote: >>>>>>>>> On Tue, Mar 12, 2019 at 3:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: >>>>>>>>>> >>>>>>>>>> On 03/12/2019 01:33 PM, Tomasz Figa wrote: >>>>>>>>>>> Hi Bingbu, >>>>>>>>>>> >>>>>>>>>>> On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: >>>>>>>>>>>> From: Bingbu Cao <bingbu.cao@intel.com> >>>>>>>>>>>> >>>>>>>>>>>> Current ImgU driver processes and releases the parameter buffer >>>>>>>>>>>> immediately after queued from user. This does not align with other >>>>>>>>>>>> image buffers which are grouped in sets and used for the same frame. >>>>>>>>>>>> If user queues multiple parameter buffers continuously, only the last >>>>>>>>>>>> one will take effect. >>>>>>>>>>>> To make consistent buffers usage, this patch changes the parameter >>>>>>>>>>>> buffer handling and group parameter buffer with other image buffers >>>>>>>>>>>> for each frame. >>>>>>>>>>> Thanks for the patch. Please see my comments inline. >>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> >>>>>>>>>>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >>>>>>>>>>>> --- >>>>>>>>>>>> drivers/staging/media/ipu3/ipu3-css.c | 5 ----- >>>>>>>>>>>> drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- >>>>>>>>>>>> drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ >>>>>>>>>>>> 3 files changed, 33 insertions(+), 37 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c >>>>>>>>>>>> index b9354d2bb692..bcb1d436bc98 100644 >>>>>>>>>>>> --- a/drivers/staging/media/ipu3/ipu3-css.c >>>>>>>>>>>> +++ b/drivers/staging/media/ipu3/ipu3-css.c >>>>>>>>>>>> @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, >>>>>>>>>>>> obgrid_size = ipu3_css_fw_obgrid_size(bi); >>>>>>>>>>>> stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; >>>>>>>>>>>> >>>>>>>>>>>> - /* >>>>>>>>>>>> - * TODO(b/118782861): If userspace queues more than 4 buffers, the >>>>>>>>>>>> - * parameters from previous buffers will be overwritten. Fix the driver >>>>>>>>>>>> - * not to allow this. >>>>>>>>>>>> - */ >>>>>>>>>>> Wouldn't this still happen even with current patch? >>>>>>>>>>> imgu_queue_buffers() supposedly queues "as many buffers to CSS as >>>>>>>>>>> possible". This means that if the userspace queues more than 4 >>>>>>>>>>> complete frames, we still end up overwriting the parameter buffers in >>>>>>>>>>> the pool. Please correct me if I'm wrong. >>>>>>>>>> The parameter buffers are queued to CSS sequentially and queue one >>>>>>>>>> parameter along with one input buffer once ready, all the data and >>>>>>>>>> parameter buffers are tied together to queue to the CSS. If userspace >>>>>>>>>> queue more parameter buffers then input buffer, they are pending on the >>>>>>>>>> buffer list. >>>>>>>>> It doesn't seem to be what the code does. I'm talking about the >>>>>>>>> following example: >>>>>>>>> >>>>>>>>> Queue OUT buffer 1 >>>>>>>>> Queue PARAM buffer 1 >>>>>>>>> Queue IN buffer 1 >>>>>>>>> Queue OUT buffer 2 >>>>>>>>> Queue PARAM buffer 2 >>>>>>>>> Queue IN buffer 2 >>>>>>>>> Queue OUT buffer 3 >>>>>>>>> Queue PARAM buffer 3 >>>>>>>>> Queue IN buffer 3 >>>>>>>>> Queue OUT buffer 4 >>>>>>>>> Queue PARAM buffer 4 >>>>>>>>> Queue IN buffer 4 >>>>>>>>> Queue OUT buffer 5 >>>>>>>>> Queue PARAM buffer 5 >>>>>>>>> Queue IN buffer 5 >>>>>>>>> >>>>>>>>> All the operations happening exactly one after each other. How would >>>>>>>>> the code prevent the 5th PARAM buffer to be queued to the IMGU, after >>>>>>>>> the 5th IN buffer is queued? As I said, imgu_queue_buffers() just >>>>>>>>> queues as many buffers of each type as there are IN buffers available. >>>>>>>> So the parameter pool now is only used as record last valid parameter not >>>>>>>> used as a list or cached, all the parameters will be queued to CSS as soon as >>>>>>>> possible(if queue for CSS is not full). >>>>>>>> As the size of pool now is a bit confusing, I think we can shrink the its value >>>>>>>> for each pipe to 2. >>>>>>> I don't follow. Don't we take one buffer from the pool, fill in the >>>>>>> parameters in hardware format there and then queue that buffer from >>>>>>> the pool to the ISP? The ISP wouldn't read those parameters from the >>>>>>> buffer until the previous frame is processed, would it? >>>>>> Hi, Tomasz, >>>>>> >>>>>> Currently, driver did not take the buffer from pool to queue to ISP, >>>>>> it just queue the parameter buffer along with input frame buffer depends >>>>>> on each user queue request. >>>>>> >>>>>> You are right, if user queue massive buffers one time, it will cause >>>>>> the firmware queue full. Driver should keep the buffer in its list >>>>>> instead of returning back to user irresponsibly. >>>>>> >>>>>> We are thinking about queue one group of buffers(input, output and params) >>>>>> to ISP one time and wait the pipeline finished and then queue next group >>>>>> of buffers. All the buffers are pending on the buffer list. >>>>>> What do you think about this behavior? >>>>> >>>>> Sorry, I was sure I replied to your email, but apparently I didn't. >>>>> >>>>> Yes, that would certainly work, but wouldn't it introduce pipeline >>>>> bubbles, potentially affecting the performance? >>>> Hi, Tomasz, >>>> >>>> Thanks for your reply. >>>> >>>> The driver will queue the buffers to CSS immediately after previous >>>> pipeline finished which is invoked in imgu_isr_threaded. >>>> >>>> The bubbles compared from before should be very small since current >>>> camera HAL implementation in production will queue new buffers IFF all >>>> the buffers dequeued from driver, as I know. >>> >>> If the firmware has a queue depth of 4, I think it would still be much >>> better to use it. Would it really make the code much more complicated? >>> I think you could just maintain a counter of queued buffers and keep >>> refilling the queue whenever it's less than 4 and there are any >>> buffers to queue. >> Actually, firmware will use latest parameter queued and apply to frame, >> they are not consumed frame by frame. >> The parameter buffers are not used same way as frame buffers, so the >> pool in driver is just used for storing previous parameter and refilling >> fields within new coming parameter from user and combine with previous >> ones into a whole parameter. > > Hmm, that's a rather strange design. > > Well, in that case we can't queue more than 1 frame indeed, as > otherwise we wouldn't be able to synchronize the parameters with the > right frames. Hi, Tomasz Yes, the parameter queue handling is a little different from other queues. Group the buffers together is one way to bind the parameter to frame. Do you have any other ideas? > > Best regards, > Tomasz >
On Mon, Apr 8, 2019 at 3:04 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > > > > On 3/26/19 12:03 PM, Tomasz Figa wrote: > > On Mon, Mar 25, 2019 at 6:52 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >> > >> > >> > >> On 3/25/19 12:20 PM, Tomasz Figa wrote: > >>> On Mon, Mar 25, 2019 at 1:12 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >>>> > >>>> > >>>> > >>>> On 3/25/19 11:16 AM, Tomasz Figa wrote: > >>>>> Hi Bingbu, > >>>>> > >>>>> On Wed, Mar 13, 2019 at 1:25 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 03/12/2019 04:54 PM, Tomasz Figa wrote: > >>>>>>> On Tue, Mar 12, 2019 at 5:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> On 03/12/2019 03:43 PM, Tomasz Figa wrote: > >>>>>>>>> On Tue, Mar 12, 2019 at 3:48 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote: > >>>>>>>>>> > >>>>>>>>>> On 03/12/2019 01:33 PM, Tomasz Figa wrote: > >>>>>>>>>>> Hi Bingbu, > >>>>>>>>>>> > >>>>>>>>>>> On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@intel.com> wrote: > >>>>>>>>>>>> From: Bingbu Cao <bingbu.cao@intel.com> > >>>>>>>>>>>> > >>>>>>>>>>>> Current ImgU driver processes and releases the parameter buffer > >>>>>>>>>>>> immediately after queued from user. This does not align with other > >>>>>>>>>>>> image buffers which are grouped in sets and used for the same frame. > >>>>>>>>>>>> If user queues multiple parameter buffers continuously, only the last > >>>>>>>>>>>> one will take effect. > >>>>>>>>>>>> To make consistent buffers usage, this patch changes the parameter > >>>>>>>>>>>> buffer handling and group parameter buffer with other image buffers > >>>>>>>>>>>> for each frame. > >>>>>>>>>>> Thanks for the patch. Please see my comments inline. > >>>>>>>>>>> > >>>>>>>>>>>> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> > >>>>>>>>>>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > >>>>>>>>>>>> --- > >>>>>>>>>>>> drivers/staging/media/ipu3/ipu3-css.c | 5 ----- > >>>>>>>>>>>> drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++-------------------------- > >>>>>>>>>>>> drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++ > >>>>>>>>>>>> 3 files changed, 33 insertions(+), 37 deletions(-) > >>>>>>>>>>>> > >>>>>>>>>>>> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c > >>>>>>>>>>>> index b9354d2bb692..bcb1d436bc98 100644 > >>>>>>>>>>>> --- a/drivers/staging/media/ipu3/ipu3-css.c > >>>>>>>>>>>> +++ b/drivers/staging/media/ipu3/ipu3-css.c > >>>>>>>>>>>> @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, > >>>>>>>>>>>> obgrid_size = ipu3_css_fw_obgrid_size(bi); > >>>>>>>>>>>> stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; > >>>>>>>>>>>> > >>>>>>>>>>>> - /* > >>>>>>>>>>>> - * TODO(b/118782861): If userspace queues more than 4 buffers, the > >>>>>>>>>>>> - * parameters from previous buffers will be overwritten. Fix the driver > >>>>>>>>>>>> - * not to allow this. > >>>>>>>>>>>> - */ > >>>>>>>>>>> Wouldn't this still happen even with current patch? > >>>>>>>>>>> imgu_queue_buffers() supposedly queues "as many buffers to CSS as > >>>>>>>>>>> possible". This means that if the userspace queues more than 4 > >>>>>>>>>>> complete frames, we still end up overwriting the parameter buffers in > >>>>>>>>>>> the pool. Please correct me if I'm wrong. > >>>>>>>>>> The parameter buffers are queued to CSS sequentially and queue one > >>>>>>>>>> parameter along with one input buffer once ready, all the data and > >>>>>>>>>> parameter buffers are tied together to queue to the CSS. If userspace > >>>>>>>>>> queue more parameter buffers then input buffer, they are pending on the > >>>>>>>>>> buffer list. > >>>>>>>>> It doesn't seem to be what the code does. I'm talking about the > >>>>>>>>> following example: > >>>>>>>>> > >>>>>>>>> Queue OUT buffer 1 > >>>>>>>>> Queue PARAM buffer 1 > >>>>>>>>> Queue IN buffer 1 > >>>>>>>>> Queue OUT buffer 2 > >>>>>>>>> Queue PARAM buffer 2 > >>>>>>>>> Queue IN buffer 2 > >>>>>>>>> Queue OUT buffer 3 > >>>>>>>>> Queue PARAM buffer 3 > >>>>>>>>> Queue IN buffer 3 > >>>>>>>>> Queue OUT buffer 4 > >>>>>>>>> Queue PARAM buffer 4 > >>>>>>>>> Queue IN buffer 4 > >>>>>>>>> Queue OUT buffer 5 > >>>>>>>>> Queue PARAM buffer 5 > >>>>>>>>> Queue IN buffer 5 > >>>>>>>>> > >>>>>>>>> All the operations happening exactly one after each other. How would > >>>>>>>>> the code prevent the 5th PARAM buffer to be queued to the IMGU, after > >>>>>>>>> the 5th IN buffer is queued? As I said, imgu_queue_buffers() just > >>>>>>>>> queues as many buffers of each type as there are IN buffers available. > >>>>>>>> So the parameter pool now is only used as record last valid parameter not > >>>>>>>> used as a list or cached, all the parameters will be queued to CSS as soon as > >>>>>>>> possible(if queue for CSS is not full). > >>>>>>>> As the size of pool now is a bit confusing, I think we can shrink the its value > >>>>>>>> for each pipe to 2. > >>>>>>> I don't follow. Don't we take one buffer from the pool, fill in the > >>>>>>> parameters in hardware format there and then queue that buffer from > >>>>>>> the pool to the ISP? The ISP wouldn't read those parameters from the > >>>>>>> buffer until the previous frame is processed, would it? > >>>>>> Hi, Tomasz, > >>>>>> > >>>>>> Currently, driver did not take the buffer from pool to queue to ISP, > >>>>>> it just queue the parameter buffer along with input frame buffer depends > >>>>>> on each user queue request. > >>>>>> > >>>>>> You are right, if user queue massive buffers one time, it will cause > >>>>>> the firmware queue full. Driver should keep the buffer in its list > >>>>>> instead of returning back to user irresponsibly. > >>>>>> > >>>>>> We are thinking about queue one group of buffers(input, output and params) > >>>>>> to ISP one time and wait the pipeline finished and then queue next group > >>>>>> of buffers. All the buffers are pending on the buffer list. > >>>>>> What do you think about this behavior? > >>>>> > >>>>> Sorry, I was sure I replied to your email, but apparently I didn't. > >>>>> > >>>>> Yes, that would certainly work, but wouldn't it introduce pipeline > >>>>> bubbles, potentially affecting the performance? > >>>> Hi, Tomasz, > >>>> > >>>> Thanks for your reply. > >>>> > >>>> The driver will queue the buffers to CSS immediately after previous > >>>> pipeline finished which is invoked in imgu_isr_threaded. > >>>> > >>>> The bubbles compared from before should be very small since current > >>>> camera HAL implementation in production will queue new buffers IFF all > >>>> the buffers dequeued from driver, as I know. > >>> > >>> If the firmware has a queue depth of 4, I think it would still be much > >>> better to use it. Would it really make the code much more complicated? > >>> I think you could just maintain a counter of queued buffers and keep > >>> refilling the queue whenever it's less than 4 and there are any > >>> buffers to queue. > >> Actually, firmware will use latest parameter queued and apply to frame, > >> they are not consumed frame by frame. > >> The parameter buffers are not used same way as frame buffers, so the > >> pool in driver is just used for storing previous parameter and refilling > >> fields within new coming parameter from user and combine with previous > >> ones into a whole parameter. > > > > Hmm, that's a rather strange design. > > > > Well, in that case we can't queue more than 1 frame indeed, as > > otherwise we wouldn't be able to synchronize the parameters with the > > right frames. > Hi, Tomasz > > Yes, the parameter queue handling is a little different from other > queues. Group the buffers together is one way to bind the parameter to > frame. Do you have any other ideas? Nope. Please go ahead with what you originally suggested. Sorry, I still didn't have a chance to review your v2. Does it implement that approach? Best regards, Tomasz
diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c index b9354d2bb692..bcb1d436bc98 100644 --- a/drivers/staging/media/ipu3/ipu3-css.c +++ b/drivers/staging/media/ipu3/ipu3-css.c @@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe, obgrid_size = ipu3_css_fw_obgrid_size(bi); stripes = bi->info.isp.sp.iterator.num_stripes ? : 1; - /* - * TODO(b/118782861): If userspace queues more than 4 buffers, the - * parameters from previous buffers will be overwritten. Fix the driver - * not to allow this. - */ ipu3_css_pool_get(&css_pipe->pool.parameter_set_info); param_set = ipu3_css_pool_last(&css_pipe->pool.parameter_set_info, 0)->vaddr; diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c index e758a650ad2b..7812c7324893 100644 --- a/drivers/staging/media/ipu3/ipu3-v4l2.c +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c @@ -341,8 +341,9 @@ static void ipu3_vb2_buf_queue(struct vb2_buffer *vb) struct imgu_video_device *node = container_of(vb->vb2_queue, struct imgu_video_device, vbq); unsigned int queue = imgu_node_to_queue(node->id); + struct imgu_buffer *buf = container_of(vb, struct imgu_buffer, + vid_buf.vbb.vb2_buf); unsigned long need_bytes; - unsigned int pipe = node->pipe; if (vb->vb2_queue->type == V4L2_BUF_TYPE_META_CAPTURE || vb->vb2_queue->type == V4L2_BUF_TYPE_META_OUTPUT) @@ -350,42 +351,18 @@ static void ipu3_vb2_buf_queue(struct vb2_buffer *vb) else need_bytes = node->vdev_fmt.fmt.pix_mp.plane_fmt[0].sizeimage; - if (queue == IPU3_CSS_QUEUE_PARAMS) { - unsigned long payload = vb2_get_plane_payload(vb, 0); - struct vb2_v4l2_buffer *buf = - container_of(vb, struct vb2_v4l2_buffer, vb2_buf); - int r = -EINVAL; - - if (payload == 0) { - payload = need_bytes; - vb2_set_plane_payload(vb, 0, payload); - } - if (payload >= need_bytes) - r = ipu3_css_set_parameters(&imgu->css, pipe, - vb2_plane_vaddr(vb, 0)); - buf->flags = V4L2_BUF_FLAG_DONE; - vb2_buffer_done(vb, r == 0 ? VB2_BUF_STATE_DONE - : VB2_BUF_STATE_ERROR); - - } else { - struct imgu_buffer *buf = container_of(vb, struct imgu_buffer, - vid_buf.vbb.vb2_buf); - - mutex_lock(&imgu->lock); + mutex_lock(&imgu->lock); + if (queue != IPU3_CSS_QUEUE_PARAMS) ipu3_css_buf_init(&buf->css_buf, queue, buf->map.daddr); - list_add_tail(&buf->vid_buf.list, - &node->buffers); - mutex_unlock(&imgu->lock); + list_add_tail(&buf->vid_buf.list, &node->buffers); + mutex_unlock(&imgu->lock); - vb2_set_plane_payload(&buf->vid_buf.vbb.vb2_buf, 0, need_bytes); - - if (imgu->streaming) - imgu_queue_buffers(imgu, false, pipe); - } + vb2_set_plane_payload(vb, 0, need_bytes); + if (imgu->streaming) + imgu_queue_buffers(imgu, false, node->pipe); dev_dbg(&imgu->pci_dev->dev, "%s for pipe %d node %d", __func__, node->pipe, node->id); - } static int ipu3_vb2_queue_setup(struct vb2_queue *vq, diff --git a/drivers/staging/media/ipu3/ipu3.c b/drivers/staging/media/ipu3/ipu3.c index 839d9398f8e9..25e121eebee2 100644 --- a/drivers/staging/media/ipu3/ipu3.c +++ b/drivers/staging/media/ipu3/ipu3.c @@ -246,6 +246,30 @@ int imgu_queue_buffers(struct imgu_device *imgu, bool initial, unsigned int pipe dev_warn(&imgu->pci_dev->dev, "Vf not enabled, ignore queue"); continue; + } else if (node == IMGU_NODE_PARAMS && + imgu_pipe->nodes[node].enabled) { + struct vb2_buffer *vb; + struct ipu3_vb2_buffer *ivb; + + if (list_empty(&imgu_pipe->nodes[node].buffers)) + /* No parameters for this frame. */ + continue; + ivb = list_first_entry(&imgu_pipe->nodes[node].buffers, + struct ipu3_vb2_buffer, list); + vb = &ivb->vbb.vb2_buf; + r = ipu3_css_set_parameters(&imgu->css, pipe, + vb2_plane_vaddr(vb, 0)); + if (r) { + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); + dev_err(&imgu->pci_dev->dev, + "set parameters failed.\n"); + } else { + vb2_buffer_done(vb, VB2_BUF_STATE_DONE); + dev_dbg(&imgu->pci_dev->dev, + "queue user parameters %d to css.\n", + vb->index); + } + list_del(&ivb->list); } else if (imgu_pipe->queue_enabled[node]) { struct ipu3_css_buffer *buf = imgu_queue_getbuf(imgu, node, pipe);