Re: [PATCH v6 1/5] media: platform: Fix jpeg dec driver bug and improve code quality

[Xia Jiang]

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
>