On Mon, Mar 25, 2019 at 1:12 PM Bingbu Cao <bingbu.cao@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote: > >> > >> > >> > >> On 03/12/2019 04:54 PM, Tomasz Figa wrote: > >>> On Tue, Mar 12, 2019 at 5:48 PM Bingbu Cao <bingbu.cao@xxxxxxxxxxxxxxx> wrote: > >>>> > >>>> > >>>> On 03/12/2019 03:43 PM, Tomasz Figa wrote: > >>>>> On Tue, Mar 12, 2019 at 3:48 PM Bingbu Cao <bingbu.cao@xxxxxxxxxxxxxxx> wrote: > >>>>>> > >>>>>> On 03/12/2019 01:33 PM, Tomasz Figa wrote: > >>>>>>> Hi Bingbu, > >>>>>>> > >>>>>>> On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@xxxxxxxxx> wrote: > >>>>>>>> From: Bingbu Cao <bingbu.cao@xxxxxxxxx> > >>>>>>>> > >>>>>>>> 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@xxxxxxxxx> > >>>>>>>> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx> > >>>>>>>> --- > >>>>>>>> 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.
