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. Best regards, Tomasz
