From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
To: jernej.skrabec@gmail.com
Cc: Samuel Holland <samuel@sholland.org>,
linux-sunxi@googlegroups.com, Maxime Ripard <mripard@kernel.org>,
Chen-Yu Tsai <wens@csie.org>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-media <linux-media@vger.kernel.org>,
Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Subject: Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM
Date: Tue, 21 Apr 2020 00:51:19 -0300 [thread overview]
Message-ID: <CAAEAJfCjUY3JeZ1dmVwkQaumoCbv8OR0FTP9Prdg=RS=gh_NUg@mail.gmail.com> (raw)
In-Reply-To: <5590139.lOV4Wx5bFT@jernej-laptop>
Hi Jernej, Paul:
On Mon, 20 Apr 2020 at 13:41, Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> Dne ponedeljek, 20. april 2020 ob 17:10:10 CEST je Paul Kocialkowski
> napisal(a):
> > Hi,
> >
> > On Sun 19 Apr 20, 15:28, Samuel Holland wrote:
> > > On 4/8/20 11:01 AM, Jernej Škrabec wrote:
> > > > Hi Samuel!
> > > >
> > > > Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisal(a):
> > > >> This allows the VE clocks and PLL_VE to be disabled most of the time.
> > > >>
> > > >> Since the device is stateless, each frame gets a separate runtime PM
> > > >> reference. Enable autosuspend so the PM callbacks are not run before
> > > >> and
> > > >> after every frame.
> > > >>
> > > >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> > > >> ---
> > > >>
> > > >> I tested this with v4l2-request-test. I don't have the setup to do
> > > >> anything more complicated at the moment.
> > > >>
> > > >> ---
> > > >>
> > > >> drivers/staging/media/sunxi/cedrus/cedrus.c | 7 ++
> > > >> .../staging/media/sunxi/cedrus/cedrus_hw.c | 115 ++++++++++++------
> > > >> .../staging/media/sunxi/cedrus/cedrus_hw.h | 3 +
> > > >> 3 files changed, 88 insertions(+), 37 deletions(-)
> > >
> > > [snip]
> > >
> > > > Reset above causes problem. When format is set in cedrus_s_fmt_vid_cap()
> > > > a
> > > > function is called, which sets few registers in HW. Of course, there is
> > > > no
> > > > guarantee that someone will start decoding immediately after capture
> > > > format is set. So, if the driver puts VPU to sleep in the mean time,
> > > > reset will clear those registers and decoded video will be in different
> > > > format than expected. It could be even argued that registers should not
> > > > be set in that function and that this is design issue or bug in driver.
> > >
> > > You're right. I didn't see that cedrus_dst_format_set() was called outside
> > > cedrus_engine_enable/disable().
> >
> > This might indeed be an issue with multiple decoding contexts in parallel,
> > with potentially different formats. For that reason, it looks like the
> > right thing to do would be to set the format at each decoding run based on
> > the format set in the context by s_fmt.
>
> So you are suggesting that cedrus_dst_format_set() should be moved to
> cedrus_device_run(), right? This way all registers are set at each run, which
> is then truly stateless.
>
BTW, this is how the Hantro and Rockchip's Rkvdec
drivers are implemented. One of the main reasons is
to have simultaneous decoding support.
Thanks,
Ezequiel
next prev parent reply other threads:[~2020-04-21 3:51 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-08 1:02 [PATCH] media: cedrus: Implement runtime PM Samuel Holland
2020-04-08 9:41 ` Maxime Ripard
2020-04-08 11:37 ` Paul Kocialkowski
2020-04-08 14:59 ` [linux-sunxi] " Jernej Škrabec
2020-04-08 16:01 ` Jernej Škrabec
2020-04-19 20:28 ` Samuel Holland
2020-04-20 15:10 ` Paul Kocialkowski
2020-04-20 16:41 ` Jernej Škrabec
2020-04-20 17:56 ` Paul Kocialkowski
2020-04-20 18:09 ` Jernej Škrabec
2020-04-21 9:00 ` Paul Kocialkowski
2020-04-21 3:51 ` Ezequiel Garcia [this message]
2020-04-21 9:01 ` Paul Kocialkowski
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='CAAEAJfCjUY3JeZ1dmVwkQaumoCbv8OR0FTP9Prdg=RS=gh_NUg@mail.gmail.com' \
--to=ezequiel@vanguardiasur.com.ar \
--cc=gregkh@linuxfoundation.org \
--cc=jernej.skrabec@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-sunxi@googlegroups.com \
--cc=mchehab@kernel.org \
--cc=mripard@kernel.org \
--cc=paul.kocialkowski@bootlin.com \
--cc=samuel@sholland.org \
--cc=wens@csie.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).