On Fri, 2020-02-14 at 18:35 +0900, Tomasz Figa wrote: > Hi Xia, > > On Tue, Jan 21, 2020 at 05:53:17PM +0800, Xia Jiang wrote: > > Fix v4l2-compliance test bug and improve code quality of jpeg decode > > driver, because the jpeg encode driver will base on it. > > > > Signed-off-by: Xia Jiang <xia.jiang@xxxxxxxxxxxx> > > --- > > v6: alignment 'MTK_JPEG_DCTSIZE' match open parenthesis. > > > > v5: Use clamp()to replace mtk_jpeg_bound_align_image() and round_up() > > to replace mtk_jpeg_align(). > > Get correct compose value in mtk_jpeg_selection(). > > Cancel spin lock and unlock operation in device run function. > > Change register offset hex numberals from upercase to lowercase. > > > > v4: new add patch for v4l2-compliance test bug fix. > > Thanks for the patch. The changes look good to me, but each of the > unrelated changes should be split into its own patch, with proper > explanation in its commit message. Especially the ones that introduce > behavior changes, such as the S_SELECTION or locking change. > > Also please see one comment inline. > > [snip] > > > @@ -801,7 +778,6 @@ static void mtk_jpeg_device_run(void *priv) > > struct mtk_jpeg_dev *jpeg = ctx->jpeg; > > struct vb2_v4l2_buffer *src_buf, *dst_buf; > > enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR; > > - unsigned long flags; > > struct mtk_jpeg_src_buf *jpeg_src_buf; > > struct mtk_jpeg_bs bs; > > struct mtk_jpeg_fb fb; > > @@ -829,13 +805,11 @@ static void mtk_jpeg_device_run(void *priv) > > if (mtk_jpeg_set_dec_dst(ctx, &jpeg_src_buf->dec_param, &dst_buf->vb2_buf, &fb)) > > goto dec_end; > > > > - spin_lock_irqsave(&jpeg->hw_lock, flags); > > Why is it safe to remove the locking here? Dear Tomasz, I will split unrelated changes into different patches. My opinion about remove locking is following(after deep thinking): The device_run function can be only called once for one instance. For multi-instance,is there any posssibility of hw override if removing locking?I think so. What about your further opinion? Best Regards, Xia Jiang > > mtk_jpeg_dec_reset(jpeg->dec_reg_base); > > mtk_jpeg_dec_set_config(jpeg->dec_reg_base, > > &jpeg_src_buf->dec_param, &bs, &fb); > > > > mtk_jpeg_dec_start(jpeg->dec_reg_base); > > - spin_unlock_irqrestore(&jpeg->hw_lock, flags); > > return; > > > > dec_end: > > Best regards, > Tomasz >
