On Sun, Sep 27, 2020 at 12:25 PM Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx> wrote: > > Hi, > > Am 25.09.20 um 22:16 schrieb Tomasz Figa: > > Hi Dafna, > > > > On Tue, Sep 22, 2020 at 01:33:54PM +0200, Dafna Hirschfeld wrote: > >> Currently, the first buffer queued in the params node is returned > >> immediately to userspace and a copy of it is saved in the field > >> 'cur_params'. The copy is later used for the first configuration > >> when the stream is initiated by one of selfpath/mainpath capture nodes. > > > > Thank you for the patch. Please see my comments inline. > > > >> > >> There are 3 problems with this implementation: > >> - The first params buffer is applied and returned to userspace even if > >> userspace never calls to streamon on the params node. > > > > How is this possible? VB2 doesn't call the .buf_queue callback until streaming is started. > > To my knowledge, userspace can first queue the buffers and after that start > the stream by calling 'streamon'. > Yes, that's how the UAPI works, but not the videobuf2 kAPI. It is exactly designed in a way to prevent proliferation of similar checks across the drivers. Check the implementation of vb2_core_qbuf() [1]. [1] https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-core.c#L1624 > > > >> - If the first params buffer is queued after the stream started on the > >> params node then it will return to userspace but will never be used. > > > > Why? > > The first params buffer is ignored if it is queued after > selfpath/mainpath already started to stream. > This is because the buffer 'params->cur_params' is applied in function > 'rkisp1_params_config_parameter' which is called when streaming > from mainpath/selfpath starts. Fair enough, thanks. It would make sense to include this in the commit message. > > > > >> - The frame_sequence of the first buffer is set to -1 if the main/selfpath > >> did not start streaming. > > > > Indeed, this is invalid. The sequence number should correspond to the > > sequence number of the frame that the parameters were applied to, i.e. the > > parameter buffer A and the video buffer B dequeued from the CAPTURE node > > which contains the first frame processed with the parameters from buffer A > > should have the same sequence number. > > > >> > >> A correct implementation is to apply the first params buffer when stream > >> is started from mainpath/selfpath and only if params is also streaming. > >> > >> The patch adds a new function 'rkisp1_params_apply_params_cfg' which takes > >> a buffer from the buffers queue, apply it and returns it to userspace. > >> The function is called from the irq handler and when main/selfpath stream > >> starts - in the function 'rkisp1_params_config_parameter' > >> > >> Also remove the fields 'cur_params', 'is_first_params' which are no > >> more needed. > >> > >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx> > >> Reported-by: kernel test robot <lkp@xxxxxxxxx> > >> Acked-by: Helen Koike <helen.koike@xxxxxxxxxxxxx> > >> --- > >> changes since v2: > >> declare function 'rkisp1_params_apply_params_cfg' as static > >> to fix a warning reported by 'kernel test robot <lkp@xxxxxxxxx>' > >> --- > >> drivers/staging/media/rkisp1/rkisp1-common.h | 4 -- > >> drivers/staging/media/rkisp1/rkisp1-params.c | 50 ++++++++------------ > >> 2 files changed, 20 insertions(+), 34 deletions(-) > >> > >> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h > >> index 992d8ec4c448..232bee92d0eb 100644 > >> --- a/drivers/staging/media/rkisp1/rkisp1-common.h > >> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h > >> @@ -262,10 +262,8 @@ struct rkisp1_stats { > >> * @rkisp1: pointer to the rkisp1 device > >> * @config_lock: locks the buffer list 'params' and 'is_streaming' > >> * @params: queue of rkisp1_buffer > >> - * @cur_params: the first params values from userspace > >> * @vdev_fmt: v4l2_format of the metadata format > >> * @is_streaming: device is streaming > >> - * @is_first_params: the first params should take effect immediately > >> * @quantization: the quantization configured on the isp's src pad > >> * @raw_type: the bayer pattern on the isp video sink pad > >> */ > >> @@ -275,10 +273,8 @@ struct rkisp1_params { > >> > >> spinlock_t config_lock; /* locks the buffers list 'params' and 'is_streaming' */ > >> struct list_head params; > >> - struct rkisp1_params_cfg cur_params; > >> struct v4l2_format vdev_fmt; > >> bool is_streaming; > >> - bool is_first_params; > >> > >> enum v4l2_quantization quantization; > >> enum rkisp1_fmt_raw_pat_type raw_type; > >> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c > >> index ab2deb57b1eb..e8049a50575f 100644 > >> --- a/drivers/staging/media/rkisp1/rkisp1-params.c > >> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c > >> @@ -1185,23 +1185,14 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params, > >> } > >> } > >> > >> -void rkisp1_params_isr(struct rkisp1_device *rkisp1) > >> +static void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, > >> + unsigned int frame_sequence) > > > > Should we call this _locked() since it is expected that the config_lock is > > held when called? > > okey, I'll change the name. > > > > > To signify such condition, the __must_hold sparse annotation can be used. > > > >> { > >> - unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence); > >> - struct rkisp1_params *params = &rkisp1->params; > >> struct rkisp1_params_cfg *new_params; > >> struct rkisp1_buffer *cur_buf = NULL; > >> > >> - spin_lock(¶ms->config_lock); > >> - if (!params->is_streaming) { > >> - spin_unlock(¶ms->config_lock); > >> - return; > >> - } > >> - > >> - if (list_empty(¶ms->params)) { > >> - spin_unlock(¶ms->config_lock); > >> + if (list_empty(¶ms->params)) > >> return; > >> - } > >> > >> cur_buf = list_first_entry(¶ms->params, > >> struct rkisp1_buffer, queue); > >> @@ -1218,6 +1209,20 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1) > >> > >> cur_buf->vb.sequence = frame_sequence; > >> vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE); > >> +} > >> + > >> +void rkisp1_params_isr(struct rkisp1_device *rkisp1) > >> +{ > >> + unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence); > >> + struct rkisp1_params *params = &rkisp1->params; > >> + > >> + spin_lock(¶ms->config_lock); > >> + if (!params->is_streaming) { > > > > Do we need this check? Wouldn't the queue be empty if params is not > > streaming? > > Hi, > userspace can queue buffers before start streaming or without ever start streaming, > so if there are buffers in the queue it does not mean that the params is streaming. > See above. Best regards, Tomasz > > > > > > Also, if we decide that we need the check, we could replace the private > > is_streaming flag with vb2_is_streaming(). > > right, > > > Thanks, > Dafna > > > > > Best regards, > > Tomasz > >
