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. > - 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 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? 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? Also, if we decide that we need the check, we could replace the private is_streaming flag with vb2_is_streaming(). Best regards, Tomasz