On Mon, Apr 8, 2019 at 3:04 PM Bingbu Cao <bingbu.cao@xxxxxxxxxxxxxxx> wrote: > > > > On 3/26/19 12:03 PM, Tomasz Figa wrote: > > On Mon, Mar 25, 2019 at 6:52 PM Bingbu Cao <bingbu.cao@xxxxxxxxxxxxxxx> wrote: > >> > >> > >> > >> On 3/25/19 12:20 PM, Tomasz Figa wrote: > >>> 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. > >> 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
