Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6187a0514757dd50734175ceebcc0061d8eb6c5b.camel@ndufresne.ca>
Subject: Re: [EXT] Re: [PATCH] media: amphion: return error if format is
 unsupported by vpu
From:   Nicolas Dufresne <nicolas@ndufresne.ca>
To:     Ming Qian <ming.qian@nxp.com>,
        "mchehab@kernel.org" <mchehab@kernel.org>,
        "hverkuil-cisco@xs4all.nl" <hverkuil-cisco@xs4all.nl>
Cc:     "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>,
        "festevam@gmail.com" <festevam@gmail.com>,
        dl-linux-imx <linux-imx@nxp.com>,
        "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
Date:   Fri, 20 May 2022 14:09:22 -0400
In-Reply-To: <DB8PR04MB634682E34667F804608E3018E7D39@DB8PR04MB6346.eurprd04.prod.outlook.com>
References: <20220519072844.31792-1-ming.qian@nxp.com>
         <e03669333c20c07d40d0b1ce05eb771f5dbd4139.camel@ndufresne.ca>
         <DB8PR04MB634682E34667F804608E3018E7D39@DB8PR04MB6346.eurprd04.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.1 (3.44.1-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

Le vendredi 20 mai 2022 =C3=A0 01:25 +0000, Ming Qian a =C3=A9crit=C2=A0:
> > From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca]
> > Sent: Friday, May 20, 2022 2:13 AM
> > To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
> > hverkuil-cisco@xs4all.nl
> > Cc: shawnguo@kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de;
> > kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx
> > <linux-imx@nxp.com>; linux-media@vger.kernel.org;
> > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > Subject: [EXT] Re: [PATCH] media: amphion: return error if format is
> > unsupported by vpu
> >=20
> > Caution: EXT Email
> >=20
> > Hi Ming Qian,
> >=20
> > Le jeudi 19 mai 2022 =C3=A0 15:28 +0800, Ming Qian a =C3=A9crit :
> > > return error if format is unsupported by vpu, otherwise the vpu will
> > > be stalled at decoding
> >=20
> > I have a reasonable doubt about this patch. I don't think such a case s=
hould
> > be
> > reachable by users. Normally, calls to S_FMT should ensure the driver f=
ormat
> > state is valid on both ends but modifying the relevant structures. As a=
n
> > example, for decoders, setting the CODEC (OUTPUT queue) format, may
> > change the raw format (CAPTURE queue) implicitly to prevent this situat=
ion.
> > Are we certain this change isn't papering around some missing format
> > propagation ?
> >=20
> > regards,
> > Nicolas
> >=20
>=20
> Hi Nicolas,
> =C2=A0=C2=A0=C2=A0=C2=A0You're right, it's not reachable currently.
> =C2=A0=C2=A0=C2=A0=C2=A0And there are some formats supported by VPU, but =
I didn't add support in
> driver, as they are not defined in kernel yet.
> So if someone wants to enable them in future, and if he only adds a forma=
t
> into vdec_formats[] without modifying the vpu_malone part , then he can
> enum_fmt and set_fmt successfully, but meet vpu hang without any error
> message.
> 	I think driver should report an error in case of the new format is
> not implemented fully.

Fair point, but it should be bug_on or at least an error trace.

regards,
Nicolas

>=20
> Ming
>=20
> > >=20
> > > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > > ---
> > > =C2=A0drivers/media/platform/amphion/vpu_malone.c | 2 ++
> > > =C2=A0drivers/media/platform/amphion/vpu_v4l2.c   | 4 ++--
> > > =C2=A02 files changed, 4 insertions(+), 2 deletions(-)
> > >=20
> > > diff --git a/drivers/media/platform/amphion/vpu_malone.c
> > > b/drivers/media/platform/amphion/vpu_malone.c
> > > index f29c223eefce..0930b6ba8c42 100644
> > > --- a/drivers/media/platform/amphion/vpu_malone.c
> > > +++ b/drivers/media/platform/amphion/vpu_malone.c
> > > @@ -610,6 +610,8 @@ static int vpu_malone_set_params(struct
> > vpu_shared_addr *shared,
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0enum vpu_malone_format malone_for=
mat;
> > >=20
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0malone_format =3D
> > vpu_malone_format_remap(params->codec_format);
> > > +     if (malone_format =3D=3D MALONE_FMT_NULL)
> > > +             return -EINVAL;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0iface->udata_buffer[instance].bas=
e =3D params->udata.base;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0iface->udata_buffer[instance].slo=
t_size =3D params->udata.size;
> > >=20
> > > diff --git a/drivers/media/platform/amphion/vpu_v4l2.c
> > > b/drivers/media/platform/amphion/vpu_v4l2.c
> > > index 446f07d09d0b..89b88e063e45 100644
> > > --- a/drivers/media/platform/amphion/vpu_v4l2.c
> > > +++ b/drivers/media/platform/amphion/vpu_v4l2.c
> > > @@ -500,10 +500,10 @@ static int vpu_vb2_start_streaming(struct
> > vb2_queue *q, unsigned int count)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0fmt->sizeimage[1], fmt->bytesperline[1],
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0fmt->sizeimage[2], fmt->bytesperline[2],
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0q->num_buffers);
> > > -     call_void_vop(inst, start, q->type);
> > > +     ret =3D call_vop(inst, start, q->type);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vb2_clear_last_buffer_dequeued(q)=
;
> > >=20
> > > -     return 0;
> > > +     return ret;
> > > =C2=A0}
> > >=20
> > > =C2=A0static void vpu_vb2_stop_streaming(struct vb2_queue *q)
>=20