Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200408010232.48432-1-samuel@sholland.org> <244922ec-ed3a-eca7-6640-49de7ad9c605@sholland.org>
 <20200420151010.GL125838@aptenodytes> <5590139.lOV4Wx5bFT@jernej-laptop>
In-Reply-To: <5590139.lOV4Wx5bFT@jernej-laptop>
From:   Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Date:   Tue, 21 Apr 2020 00:51:19 -0300
Message-ID: <CAAEAJfCjUY3JeZ1dmVwkQaumoCbv8OR0FTP9Prdg=RS=gh_NUg@mail.gmail.com>
Subject: Re: [linux-sunxi] [PATCH] media: cedrus: Implement runtime PM
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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jernej, Paul:

On Mon, 20 Apr 2020 at 13:41, Jernej =C5=A0krabec <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 =C5=A0krabec wrote:
> > > > Hi Samuel!
> > > >
> > > > Dne sreda, 08. april 2020 ob 03:02:32 CEST je Samuel Holland napisa=
l(a):
> > > >> This allows the VE clocks and PLL_VE to be disabled most of the ti=
me.
> > > >>
> > > >> Since the device is stateless, each frame gets a separate runtime =
PM
> > > >> reference. Enable autosuspend so the PM callbacks are not run befo=
re
> > > >> 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, ther=
e is
> > > > no
> > > > guarantee that someone will start decoding immediately after captur=
e
> > > > 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 diffe=
rent
> > > > 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 dri=
ver.
> > >
> > > You're right. I didn't see that cedrus_dst_format_set() was called ou=
tside
> > > cedrus_engine_enable/disable().
> >
> > This might indeed be an issue with multiple decoding contexts in parall=
el,
> > 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, w=
hich
> 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