| Message ID | 1553253285-16023-1-git-send-email-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [resend,v2] media:staging/intel-ipu3: parameter buffer refactoring | expand |
Hi Bingbu, Thanks for the patch. On Fri, Mar 22, 2019 at 07:14:45PM +0800, 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. > Each time driver will queue one more group of buffers when previous > frame processed and buffers consumed by css. > > Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > --- > changes since v1: > - add payload check for parameter buffer > - queue buffer only when previous buffer consumed > --- > drivers/staging/media/ipu3/ipu3-css.c | 5 ---- > drivers/staging/media/ipu3/ipu3-v4l2.c | 46 ++++++++++++---------------------- > drivers/staging/media/ipu3/ipu3.c | 30 ++++++++++++++++++++++ > 3 files changed, 46 insertions(+), 35 deletions(-) > > diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c > index 15ab77e4b766..018f5a266c42 100644 > --- a/drivers/staging/media/ipu3/ipu3-css.c > +++ b/drivers/staging/media/ipu3/ipu3-css.c > @@ -2160,11 +2160,6 @@ int imgu_css_set_parameters(struct imgu_css *css, unsigned int pipe, > obgrid_size = imgu_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. > - */ > imgu_css_pool_get(&css_pipe->pool.parameter_set_info); > param_set = imgu_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 9c0352b193a7..87a7919c12f0 100644 > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c > @@ -341,8 +341,10 @@ static void imgu_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; > + unsigned long payload = vb2_get_plane_payload(vb, 0); > > if (vb->vb2_queue->type == V4L2_BUF_TYPE_META_CAPTURE || > vb->vb2_queue->type == V4L2_BUF_TYPE_META_OUTPUT) > @@ -350,42 +352,26 @@ static void imgu_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 = imgu_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); > + if (queue == IPU3_CSS_QUEUE_PARAMS && payload && payload < need_bytes) { > + dev_err(&imgu->pci_dev->dev, "invalid data size for params."); > + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); This would better be done in buf_prepare callback --- you can return an error right away then. But it could well be another patch. > + return; > + } > > - mutex_lock(&imgu->lock); > + mutex_lock(&imgu->lock); > + if (queue != IPU3_CSS_QUEUE_PARAMS) > imgu_css_buf_init(&buf->css_buf, queue, buf->map.daddr); > - 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); > + list_add_tail(&buf->vid_buf.list, &node->buffers); > + mutex_unlock(&imgu->lock); > > - 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 imgu_vb2_queue_setup(struct vb2_queue *vq, > diff --git a/drivers/staging/media/ipu3/ipu3.c b/drivers/staging/media/ipu3/ipu3.c > index d575ac78c8f0..f1045072d535 100644 > --- a/drivers/staging/media/ipu3/ipu3.c > +++ b/drivers/staging/media/ipu3/ipu3.c > @@ -236,6 +236,11 @@ int imgu_queue_buffers(struct imgu_device *imgu, bool initial, unsigned int pipe > dev_dbg(&imgu->pci_dev->dev, "Queue buffers to pipe %d", pipe); > mutex_lock(&imgu->lock); > > + if (!imgu_css_pipe_queue_empty(&imgu->css, pipe)) { > + mutex_unlock(&imgu->lock); > + return 0; > + } > + > /* Buffer set is queued to FW only when input buffer is ready */ > for (node = IMGU_NODE_NUM - 1; > imgu_queue_getbuf(imgu, IMGU_NODE_IN, pipe); > @@ -246,6 +251,31 @@ 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 imgu_vb2_buffer *ivb; > + > + /* No parameters for this frame */ > + if (list_empty(&imgu_pipe->nodes[node].buffers)) > + continue; > + > + ivb = list_first_entry(&imgu_pipe->nodes[node].buffers, > + struct imgu_vb2_buffer, list); > + vb = &ivb->vbb.vb2_buf; > + r = imgu_css_set_parameters(&imgu->css, pipe, > + vb2_plane_vaddr(vb, 0)); > + if (r) { Ideally the parameters should be validated earlier but I guess it'd take significant changes to the code handling them, and there are more important things to do. Just a note. > + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); > + dev_warn(&imgu->pci_dev->dev, > + "set parameters failed."); > + continue; > + } > + > + vb2_buffer_done(vb, VB2_BUF_STATE_DONE); > + dev_dbg(&imgu->pci_dev->dev, > + "queue user parameters %d to css.", vb->index); %d -> %u. I can fix that while applying the patch. > + list_del(&ivb->list); > } else if (imgu_pipe->queue_enabled[node]) { > struct imgu_css_buffer *buf = > imgu_queue_getbuf(imgu, node, pipe);
On 4/8/19 8:26 PM, Sakari Ailus wrote: > Hi Bingbu, > > Thanks for the patch. > > On Fri, Mar 22, 2019 at 07:14:45PM +0800, 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. >> Each time driver will queue one more group of buffers when previous >> frame processed and buffers consumed by css. >> >> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >> --- >> changes since v1: >> - add payload check for parameter buffer >> - queue buffer only when previous buffer consumed >> --- >> drivers/staging/media/ipu3/ipu3-css.c | 5 ---- >> drivers/staging/media/ipu3/ipu3-v4l2.c | 46 ++++++++++++---------------------- >> drivers/staging/media/ipu3/ipu3.c | 30 ++++++++++++++++++++++ >> 3 files changed, 46 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c >> index 15ab77e4b766..018f5a266c42 100644 >> --- a/drivers/staging/media/ipu3/ipu3-css.c >> +++ b/drivers/staging/media/ipu3/ipu3-css.c >> @@ -2160,11 +2160,6 @@ int imgu_css_set_parameters(struct imgu_css *css, unsigned int pipe, >> obgrid_size = imgu_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. >> - */ >> imgu_css_pool_get(&css_pipe->pool.parameter_set_info); >> param_set = imgu_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 9c0352b193a7..87a7919c12f0 100644 >> --- a/drivers/staging/media/ipu3/ipu3-v4l2.cguess it'd take significant changes to the code handling them, and there are more important things to do. Just a note. >> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c >> @@ -341,8 +341,10 @@ static void imgu_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; >> + unsigned long payload = vb2_get_plane_payload(vb, 0); >> >> if (vb->vb2_queue->type == V4L2_BUF_TYPE_META_CAPTURE || >> vb->vb2_queue->type == V4L2_BUF_TYPE_META_OUTPUT) >> @@ -350,42 +352,26 @@ static void imgu_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) {guess it'd take significant changes to the code handling them, and there are more important things to do. Just a note. >> - 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 = imgu_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); >> + if (queue == IPU3_CSS_QUEUE_PARAMS && payload && payload < need_bytes) { >> + dev_err(&imgu->pci_dev->dev, "invalid data size for params."); >> + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); > > This would better be done in buf_prepare callback --- you can return an > error right away then. But it could well be another patch. OK, thanks, I will add this into my TODO list. > >> + return; >> + } >> >> - mutex_lock(&imgu->lock); >> + mutex_lock(&imgu->lock); >> + if (queue != IPU3_CSS_QUEUE_PARAMS) >> imgu_css_buf_init(&buf->css_buf, queue, buf->map.daddr); >> - 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); >> + list_add_tail(&buf->vid_buf.list, &node->buffers); >> + mutex_unlock(&imgu->lock); >> >> - 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 imgu_vb2_queue_setup(struct vb2_queue *vq, >> diff --git a/drivers/staging/media/ipu3/ipu3.c b/drivers/staging/media/ipu3/ipu3.c >> index d575ac78c8f0..f1045072d535 100644 >> --- a/drivers/staging/media/ipu3/ipu3.c >> +++ b/drivers/staging/media/ipu3/ipu3.c >> @@ -236,6 +236,11 @@ int imgu_queue_buffers(struct imgu_device *imgu, bool initial, unsigned int pipe >> dev_dbg(&imgu->pci_dev->dev, "Queue buffers to pipe %d", pipe); >> mutex_lock(&imgu->lock); >> >> + if (!imgu_css_pipe_queue_empty(&imgu->css, pipe)) { >> + mutex_unlock(&imgu->lock); >> + return 0; >> + } >> + >> /* Buffer set is queued to FW only when input buffer is ready */ >> for (node = IMGU_NODE_NUM - 1; >> imgu_queue_getbuf(imgu, IMGU_NODE_IN, pipe); >> @@ -246,6 +251,31 @@ 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 imgu_vb2_buffer *ivb; >> + >> + /* No parameters for this frame */ >> + if (list_empty(&imgu_pipe->nodes[node].buffers)) >> + continue; >> + >> + ivb = list_first_entry(&imgu_pipe->nodes[node].buffers, >> + struct imgu_vb2_buffer, list); >> + vb = &ivb->vbb.vb2_buf; >> + r = imgu_css_set_parameters(&imgu->css, pipe, >> + vb2_plane_vaddr(vb, 0)); >> + if (r) { > > Ideally the parameters should be validated earlier but I guess it'd take > significant changes to the code handling them, and there are more important > things to do. Just a note. > >> + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); >> + dev_warn(&imgu->pci_dev->dev, >> + "set parameters failed."); >> + continue; >> + } >> + >> + vb2_buffer_done(vb, VB2_BUF_STATE_DONE); >> + dev_dbg(&imgu->pci_dev->dev, >> + "queue user parameters %d to css.", vb->index); > > %d -> %u. > > I can fix that while applying the patch. Thanks. > >> + list_del(&ivb->list); >> } else if (imgu_pipe->queue_enabled[node]) { >> struct imgu_css_buffer *buf = >> imgu_queue_getbuf(imgu, node, pipe); >
diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c index 15ab77e4b766..018f5a266c42 100644 --- a/drivers/staging/media/ipu3/ipu3-css.c +++ b/drivers/staging/media/ipu3/ipu3-css.c @@ -2160,11 +2160,6 @@ int imgu_css_set_parameters(struct imgu_css *css, unsigned int pipe, obgrid_size = imgu_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. - */ imgu_css_pool_get(&css_pipe->pool.parameter_set_info); param_set = imgu_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 9c0352b193a7..87a7919c12f0 100644 --- a/drivers/staging/media/ipu3/ipu3-v4l2.c +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c @@ -341,8 +341,10 @@ static void imgu_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; + unsigned long payload = vb2_get_plane_payload(vb, 0); if (vb->vb2_queue->type == V4L2_BUF_TYPE_META_CAPTURE || vb->vb2_queue->type == V4L2_BUF_TYPE_META_OUTPUT) @@ -350,42 +352,26 @@ static void imgu_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 = imgu_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); + if (queue == IPU3_CSS_QUEUE_PARAMS && payload && payload < need_bytes) { + dev_err(&imgu->pci_dev->dev, "invalid data size for params."); + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); + return; + } - mutex_lock(&imgu->lock); + mutex_lock(&imgu->lock); + if (queue != IPU3_CSS_QUEUE_PARAMS) imgu_css_buf_init(&buf->css_buf, queue, buf->map.daddr); - 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); + list_add_tail(&buf->vid_buf.list, &node->buffers); + mutex_unlock(&imgu->lock); - 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 imgu_vb2_queue_setup(struct vb2_queue *vq, diff --git a/drivers/staging/media/ipu3/ipu3.c b/drivers/staging/media/ipu3/ipu3.c index d575ac78c8f0..f1045072d535 100644 --- a/drivers/staging/media/ipu3/ipu3.c +++ b/drivers/staging/media/ipu3/ipu3.c @@ -236,6 +236,11 @@ int imgu_queue_buffers(struct imgu_device *imgu, bool initial, unsigned int pipe dev_dbg(&imgu->pci_dev->dev, "Queue buffers to pipe %d", pipe); mutex_lock(&imgu->lock); + if (!imgu_css_pipe_queue_empty(&imgu->css, pipe)) { + mutex_unlock(&imgu->lock); + return 0; + } + /* Buffer set is queued to FW only when input buffer is ready */ for (node = IMGU_NODE_NUM - 1; imgu_queue_getbuf(imgu, IMGU_NODE_IN, pipe); @@ -246,6 +251,31 @@ 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 imgu_vb2_buffer *ivb; + + /* No parameters for this frame */ + if (list_empty(&imgu_pipe->nodes[node].buffers)) + continue; + + ivb = list_first_entry(&imgu_pipe->nodes[node].buffers, + struct imgu_vb2_buffer, list); + vb = &ivb->vbb.vb2_buf; + r = imgu_css_set_parameters(&imgu->css, pipe, + vb2_plane_vaddr(vb, 0)); + if (r) { + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); + dev_warn(&imgu->pci_dev->dev, + "set parameters failed."); + continue; + } + + vb2_buffer_done(vb, VB2_BUF_STATE_DONE); + dev_dbg(&imgu->pci_dev->dev, + "queue user parameters %d to css.", vb->index); + list_del(&ivb->list); } else if (imgu_pipe->queue_enabled[node]) { struct imgu_css_buffer *buf = imgu_queue_getbuf(imgu, node, pipe);