> From: Nicolas Dufresne [mailto:nicolas@xxxxxxxxxxxx] > Sent: Saturday, May 21, 2022 2:09 AM > To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx; > hverkuil-cisco@xxxxxxxxx > Cc: shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; > kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx > <linux-imx@xxxxxxx>; linux-media@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > Subject: Re: [EXT] Re: [PATCH] media: amphion: return error if format is > unsupported by vpu > > Caution: EXT Email > > Le vendredi 20 mai 2022 à 01:25 +0000, Ming Qian a écrit : > > > From: Nicolas Dufresne [mailto:nicolas@xxxxxxxxxxxx] > > > Sent: Friday, May 20, 2022 2:13 AM > > > To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx; > > > hverkuil-cisco@xxxxxxxxx > > > Cc: shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; > > > kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx > > > <linux-imx@xxxxxxx>; linux-media@xxxxxxxxxxxxxxx; > > > linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > > > Subject: [EXT] Re: [PATCH] media: amphion: return error if format is > > > unsupported by vpu > > > > > > Caution: EXT Email > > > > > > Hi Ming Qian, > > > > > > Le jeudi 19 mai 2022 à 15:28 +0800, Ming Qian a écrit : > > > > 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 should be reachable by users. Normally, calls to S_FMT should > > > ensure the driver format 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 certain this change isn't papering around some missing format > > > propagation ? > > > > > > regards, > > > Nicolas > > > > > > > Hi Nicolas, > > You're right, it's not reachable currently. > > And 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 > > format 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 > I'll add some error trace in v2 > > > > Ming > > > > > > > > > > Signed-off-by: Ming Qian <ming.qian@xxxxxxx> > > > > --- > > > > drivers/media/platform/amphion/vpu_malone.c | 2 ++ > > > > drivers/media/platform/amphion/vpu_v4l2.c | 4 ++-- > > > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > > > > > 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, > > > > enum vpu_malone_format malone_format; > > > > > > > > malone_format = > > > vpu_malone_format_remap(params->codec_format); > > > > + if (malone_format == MALONE_FMT_NULL) > > > > + return -EINVAL; > > > > iface->udata_buffer[instance].base = params->udata.base; > > > > iface->udata_buffer[instance].slot_size = > > > > params->udata.size; > > > > > > > > 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) > > > > fmt->sizeimage[1], fmt->bytesperline[1], > > > > fmt->sizeimage[2], fmt->bytesperline[2], > > > > q->num_buffers); > > > > - call_void_vop(inst, start, q->type); > > > > + ret = call_vop(inst, start, q->type); > > > > vb2_clear_last_buffer_dequeued(q); > > > > > > > > - return 0; > > > > + return ret; > > > > } > > > > > > > > static void vpu_vb2_stop_streaming(struct vb2_queue *q) > >
