From: Xia Jiang <xia.jiang@mediatek.com>
To: Tomasz Figa <tfiga@chromium.org>
Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
Rob Herring <robh+dt@kernel.org>,
"Matthias Brugger" <matthias.bgg@gmail.com>,
Rick Chang <rick.chang@mediatek.com>,
<linux-media@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-mediatek@lists.infradead.org>,
Marek Szyprowski <m.szyprowski@samsung.com>,
<srv_heupstream@mediatek.com>
Subject: Re: [PATCH v6 1/5] media: platform: Fix jpeg dec driver bug and improve code quality
Date: Mon, 24 Feb 2020 17:46:09 +0800 [thread overview]
Message-ID: <1582537569.859.14.camel@mhfsdcap03> (raw)
In-Reply-To: <20200214093506.GA193786@chromium.org>
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@mediatek.com>
> > ---
> > 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
>
next prev parent reply other threads:[~2020-02-24 9:46 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-21 9:53 [PATCH v6 0/5] Add support for mt2701 JPEG ENC support Xia Jiang
2020-01-21 9:53 ` [PATCH v6 1/5] media: platform: Fix jpeg dec driver bug and improve code quality Xia Jiang
2020-02-14 9:35 ` Tomasz Figa
2020-02-24 9:46 ` Xia Jiang [this message]
2020-01-21 9:53 ` [PATCH v6 2/5] media: dt-bindings: Add jpeg enc device tree node document Xia Jiang
2020-01-21 9:53 ` [PATCH v6 3/5] arm: dts: Add jpeg enc device tree node Xia Jiang
2020-01-21 9:53 ` [PATCH v6 4/5] media: platform: Rename jpeg dec file name Xia Jiang
2020-01-21 9:53 ` [PATCH v6 5/5] media: platform: Add jpeg dec/enc feature Xia Jiang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1582537569.859.14.camel@mhfsdcap03 \
--to=xia.jiang@mediatek.com \
--cc=devicetree@vger.kernel.org \
--cc=hverkuil-cisco@xs4all.nl \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=m.szyprowski@samsung.com \
--cc=matthias.bgg@gmail.com \
--cc=mchehab+samsung@kernel.org \
--cc=rick.chang@mediatek.com \
--cc=robh+dt@kernel.org \
--cc=srv_heupstream@mediatek.com \
--cc=tfiga@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).