Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Tomasz Figa <tfiga@chromium.org>
To: Frederic Chen <frederic.chen@mediatek.com>
Cc: "Matthias Brugger" <matthias.bgg@gmail.com>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	yuzhao@chromium.org, zwisler@chromium.org,
	"moderated list:ARM/Mediatek SoC support"
	<linux-mediatek@lists.infradead.org>,
	"list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,"
	<linux-arm-kernel@lists.infradead.org>,
	"Sean Cheng (鄭昇弘)" <Sean.Cheng@mediatek.com>,
	"Sj Huang" <sj.huang@mediatek.com>,
	"Christie Yu (游雅惠)" <christie.yu@mediatek.com>,
	"Holmes Chiou (邱挺)" <holmes.chiou@mediatek.com>,
	"Jerry-ch Chen" <Jerry-ch.Chen@mediatek.com>,
	"Jungo Lin (林明俊)" <jungo.lin@mediatek.com>,
	"Rynn Wu (吳育恩)" <Rynn.Wu@mediatek.com>,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	srv_heupstream <srv_heupstream@mediatek.com>,
	devicetree@vger.kernel.org, "Shik Chen" <shik@chromium.org>,
	suleiman@chromium.org,
	"Laurent Pinchart" <laurent.pinchart+renesas@ideasonboard.com>,
	"Hans Verkuil" <hans.verkuil@cisco.com>
Subject: Re: [RFC PATCH V1 6/6] platform: mtk-isp: Add Mediatek DIP driver
Date: Wed, 29 May 2019 12:38:11 +0900
Message-ID: <CAAFQd5BbrfhjGbKaUi4p6fJJNOKvkZb4_47gw-W8n5fEmaf5XQ@mail.gmail.com> (raw)
In-Reply-To: <1558619189.7995.27.camel@mtksdccf07>

On Thu, May 23, 2019 at 10:46 PM Frederic Chen
<frederic.chen@mediatek.com> wrote:
>
> Dear Tomasz,
>
> Thank you for your comments.
>
>
> On Wed, 2019-05-22 at 19:25 +0900, Tomasz Figa wrote:
> > Hi Frederic,
> >
> > On Wed, May 22, 2019 at 03:14:15AM +0800, Frederic Chen wrote:
> > > Dear Tomasz,
> > >
> > > I appreciate your comment. It is very helpful for us.
> > >
> >
> > You're welcome. Thanks for replying to all the comments. I'll skip those
> > resolved in my reply to keep the message shorter.
> >
> > >
> > > On Thu, 2019-05-09 at 18:48 +0900, Tomasz Figa wrote:
> > > > Hi Frederic,
> > > >
> > > > On Wed, Apr 17, 2019 at 7:45 PM Frederic Chen <frederic.chen@mediatek.com> wrote:
[snip]
> > > > Also a general note - a work can be queued only once. This means that
> > > > current code races when two dip_works are attempted to be queued very
> > > > quickly one after another (or even at the same time from different threads).
> > > >
> > > > I can think of two potential options for fixing this:
> > > >
> > > > 1) Loop in the work function until there is nothing to queue to the hardware
> > > >    anymore - but this needs tricky synchronization, because there is still
> > > >    short time at the end of the work function when a new dip_work could be
> > > >    added.
> > > >
> > > > 2) Change this to a kthread that just keeps running in a loop waiting for
> > > >    some available dip_work to show up and then sending it to the firmware.
> > > >    This should be simpler, as the kthread shouldn't have a chance to miss
> > > >    any dip_work queued.
> > > >
> > > > I'm personally in favor of option 2, as it should simplify the
> > > > synchronization.
> > > >
> > >
> > > I would like to re-design this part with a kthread in the next patch.
> >
> > Actually I missed another option. We could have 1 work_struct for 1
> > request and then we could keep using a workqueue. Perhaps that could be
> > simpler than a kthread.
> >
> > Actually, similar approach could be used for the dip_runner_func.
> > Instead of having a kthread looping, we could just have another
> > workqueue and 1 dip_runner_work per 1 request. Then we wouldn't need to
> > do the waiting loop ourselves anymore.
> >
> > Does it make sense?
>
> Yes, it make sense. Let me summarize the modification about the flow.
>
> First, we will have two work_struct in mtk_dip_request.
>
> struct mtk_dip_request {
>         struct media_request request;
>         //...
>         /* Prepare DIP part hardware configurtion */
>         struct mtk_dip_hw_submit_work submit_work;
>         /* Replace dip_running thread jobs*/
>         struct mtk_dip_hw_composing_work composing_work;
>         /* Only for composing error handling */
>         struct mtk_dip_hw_mdpcb_timeout_work timeout_work;
> };
>
> Second, the overall flow of handling each request is :
>
> 1. mtk_dip_hw_enqueue calls queue_work() to put submit_work into its
>    workqueue
> 2. submit_work sends IMG_IPI_FRAME command to SCP to prepare DIP
>    hardware configuration
> 3. dip_scp_handler receives the IMG_IPI_FRAME result from SCP
> 4. dip_scp_handler calls queue_work() to put composing_work (instead
>    of original dip_running thread jobs) into its workqueue
> 5. composing_work calls dip_mdp_cmdq_send() to finish the mdp part tasks
> 6. dip_mdp_cb_func() trigged by MDP driver calls vb2_buffer_done to
>    return the buffer (no workqueue required here)
>

Sounds good to me, but actually then simply making the workqueues
freezable doesn't solve the suspend/resume problem, because the work
functions wouldn't wait for the firmware/hardware completion anymore.
That's also okay, but in this case we need to add some code to suspend
to wait for any pending operations to complete.

Best regards,
Tomasz

  reply index

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-17 10:45 [RFC PATCH V1 0/6] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC Frederic Chen
2019-04-17 10:45 ` [RFC PATCH V1 1/6] dt-bindings: mt8183: Add binding for DIP shared memory Frederic Chen
2019-04-30  1:15   ` Rob Herring
2019-05-07 14:22     ` Frederic Chen
2019-05-14 16:19       ` Rob Herring
2019-05-16  6:12         ` Tomasz Figa
2019-05-17 22:22           ` Rob Herring
2019-04-17 10:45 ` [RFC PATCH V1 2/6] dts: arm64: mt8183: Add DIP shared memory node Frederic Chen
2019-04-17 10:45 ` [RFC PATCH V1 3/6] dt-bindings: mt8183: Added DIP dt-bindings Frederic Chen
2019-04-30  1:16   ` Rob Herring
2019-05-07 14:16     ` Frederic Chen
2019-05-14 16:14       ` Rob Herring
2019-04-17 10:45 ` [RFC PATCH V1 4/6] dts: arm64: mt8183: Add DIP nodes Frederic Chen
2019-04-17 10:45 ` [RFC PATCH V1 5/6] media: platform: Add Mediatek DIP driver KConfig Frederic Chen
2019-04-17 10:45 ` [RFC PATCH V1 6/6] platform: mtk-isp: Add Mediatek DIP driver Frederic Chen
2019-05-09  9:48   ` Tomasz Figa
2019-05-21 19:14     ` Frederic Chen
2019-05-22 10:25       ` Tomasz Figa
2019-05-23 13:46         ` Frederic Chen
2019-05-29  3:38           ` Tomasz Figa [this message]
2019-06-11  8:48       ` Frederic Chen
2019-06-11  8:59         ` Tomasz Figa
2019-06-11 10:07           ` Frederic Chen
2019-06-11 10:13             ` Tomasz Figa
2019-06-25 11:35               ` Frederic Chen
2019-06-25 12:16     ` Frederic Chen
2019-06-26  4:24       ` Tomasz Figa
2019-05-22  9:51   ` Shik Chen
2019-05-23 14:24     ` Frederic Chen

Reply instructions:

You may reply publically 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=CAAFQd5BbrfhjGbKaUi4p6fJJNOKvkZb4_47gw-W8n5fEmaf5XQ@mail.gmail.com \
    --to=tfiga@chromium.org \
    --cc=Jerry-ch.Chen@mediatek.com \
    --cc=Rynn.Wu@mediatek.com \
    --cc=Sean.Cheng@mediatek.com \
    --cc=christie.yu@mediatek.com \
    --cc=devicetree@vger.kernel.org \
    --cc=frederic.chen@mediatek.com \
    --cc=hans.verkuil@cisco.com \
    --cc=holmes.chiou@mediatek.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=joro@8bytes.org \
    --cc=jungo.lin@mediatek.com \
    --cc=laurent.pinchart+renesas@ideasonboard.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=mchehab@kernel.org \
    --cc=shik@chromium.org \
    --cc=sj.huang@mediatek.com \
    --cc=srv_heupstream@mediatek.com \
    --cc=suleiman@chromium.org \
    --cc=yuzhao@chromium.org \
    --cc=zwisler@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

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git