| Message ID | 20220519072844.31792-1-ming.qian@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: amphion: return error if format is unsupported by vpu | expand |
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 > > 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(-) > > 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)
> 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 > > 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. Ming > > > > 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(-) > > > > 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)
Le vendredi 20 mai 2022 à 01:25 +0000, Ming Qian a écrit : > > 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 > > > > 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 > > Ming > > > > > > > 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(-) > > > > > > 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) >
> From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca] > Sent: Saturday, May 21, 2022 2:09 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: 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@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 > > > > > > 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@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(-) > > > > > > > > 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) > >
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)
return error if format is unsupported by vpu, otherwise the vpu will be stalled at decoding 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(-)