Re: [PATCH] media: hantro: Check whether reset op is defined before use

From: Marek Vasut <marex@denx.de>
To: Chen-Yu Tsai <wenst@chromium.org>
Cc: Adam Ford <aford173@gmail.com>,
	linux-media@vger.kernel.org,
	Benjamin Gaignard <benjamin.gaignard@collabora.com>,
	Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	linux-rockchip@lists.infradead.org
Subject: Re: [PATCH] media: hantro: Check whether reset op is defined before use
Date: Fri, 25 Aug 2023 10:33:26 +0200	[thread overview]
Message-ID: <f6f55f46-bd6e-4dde-4484-86d1c385c8fe@denx.de> (raw)
In-Reply-To: <CAGXv+5GQFtCAYbfSbEJeHA4Os0O6WuD64V-xcZ9uDD33HbuhhQ@mail.gmail.com>

On 8/25/23 09:09, Chen-Yu Tsai wrote:
> On Thu, Aug 24, 2023 at 9:08 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 8/24/23 12:39, Adam Ford wrote:
>>> On Wed, Aug 23, 2023 at 8:39 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> The i.MX8MM/N/P does not define the .reset op since reset of the VPU is
>>>> done by genpd. Check whether the .reset op is defined before calling it
>>>> to avoid NULL pointer dereference.
>>>>
>>>> Note that the Fixes tag is set to the commit which removed the reset op
>>>> from i.MX8M Hantro G2 implementation, this is because before this commit
>>>> all the implementations did define the .reset op.
>>>
>>> I am surprised I didn't have issues when I was testing the 8MQ and
>>> 8MM, but this makes sense.
>>
>> You need to trigger the VPU watchdog to trigger the crash, that means
>> you have to get the VPU into some weird state where it fails to decode
>> frame. Then it triggers the reset and ... boom.
>>
>> See this patch, that contains a gstreamer invocation to generate such
>> trigger condition input data:
>>
>> [PATCH] media: verisilicon: Do not enable G2 postproc downscale if
>> source is narrower than destination
>>
>> "
>> To generate input test data to trigger this bug, use e.g.:
>> $ gst-launch-1.0 videotestsrc !
>> video/x-raw,width=272,height=256,format=I420 ! \
>>                    vp9enc ! matroskamux ! filesink location=/tmp/test.vp9
>> To trigger the bug upon decoding (note that the NV12 must be forced, as
>> that assures the output data would pass the G2 postproc):
>> $ gst-launch-1.0 filesrc location=/tmp/test.vp9 ! matroskademux !
>> vp9parse ! \
>>                    v4l2slvp9dec ! video/x-raw,format=NV12 ! videoconvert
>> ! fbdevsink
>> "
> 
> Does it completely recover afterwards? In my previous trials the hardware
> ended up in some bizzare state: while decoding succeeds, the output's md5sum
> didn't match up.

Have you got a testcase that triggers this, one I can try ?

I am not entirely sure whether this is happening here as well or not, 
but I can imagine that the power domain went down and back up between 
tests, so the VPU would be power cycled (and therefore reset) that way. 
So, I think it is worth testing that.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip