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: <e03669333c20c07d40d0b1ce05eb771f5dbd4139.camel@ndufresne.ca>
Subject: 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,
        hverkuil-cisco@xs4all.nl
Cc:     shawnguo@kernel.org, robh+dt@kernel.org, s.hauer@pengutronix.de,
        kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Date:   Thu, 19 May 2022 14:12:34 -0400
In-Reply-To: <20220519072844.31792-1-ming.qian@nxp.com>
References: <20220519072844.31792-1-ming.qian@nxp.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

Hi Ming Qian,

Le jeudi 19 mai 2022 =C3=A0 15:28 +0800, Ming Qian a =C3=A9crit=C2=A0:
> return error if format is unsupported by vpu,
> otherwise the vpu will be stalled at decoding

I have a reasonable doubt about this patch. I don't think such a case shoul=
d be
reachable by users. Normally, calls to S_FMT should ensure the driver forma=
t
state is valid on both ends but modifying the relevant structures. As an
example, for decoders, setting the CODEC (OUTPUT queue) format, may change =
the
raw format (CAPTURE queue) implicitly to prevent this situation. Are we cer=
tain
this change isn't papering around some missing format propagation ?

regards,
Nicolas

>=20
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
>  drivers/media/platform/amphion/vpu_malone.c | 2 ++
>  drivers/media/platform/amphion/vpu_v4l2.c   | 4 ++--
>  2 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_ad=
dr *shared,
>  	enum vpu_malone_format malone_format;
> =20
>  	malone_format =3D vpu_malone_format_remap(params->codec_format);
> +	if (malone_format =3D=3D MALONE_FMT_NULL)
> +		return -EINVAL;
>  	iface->udata_buffer[instance].base =3D params->udata.base;
>  	iface->udata_buffer[instance].slot_size =3D params->udata.size;
> =20
> diff --git a/drivers/media/platform/amphion/vpu_v4l2.c b/drivers/media/pl=
atform/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)
>  		  fmt->sizeimage[1], fmt->bytesperline[1],
>  		  fmt->sizeimage[2], fmt->bytesperline[2],
>  		  q->num_buffers);
> -	call_void_vop(inst, start, q->type);
> +	ret =3D call_vop(inst, start, q->type);
>  	vb2_clear_last_buffer_dequeued(q);
> =20
> -	return 0;
> +	return ret;
>  }
> =20
>  static void vpu_vb2_stop_streaming(struct vb2_queue *q)