* [PATCH] media: meson: vdec: esparser: check parsing state with hardware write pointer @ 2023-01-23 9:43 Christian Hewitt 2023-01-23 9:47 ` Neil Armstrong 2023-01-23 12:04 ` Dan Carpenter 0 siblings, 2 replies; 4+ messages in thread From: Christian Hewitt @ 2023-01-23 9:43 UTC (permalink / raw) To: Neil Armstrong, Mauro Carvalho Chehab, Greg Kroah-Hartman, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-media, linux-amlogic, linux-staging, linux-arm-kernel, linux-kernel Cc: Christian Hewitt, Neil Armstrong From: Neil Armstrong <narmstrong@baylibre.com> Also check the hardware write pointer to check if ES Parser has stalled. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> Signed-off-by: Christian Hewitt <christianshewitt@gmail.com> --- drivers/staging/media/meson/vdec/esparser.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c index df5956c6141d..41b705f999b2 100644 --- a/drivers/staging/media/meson/vdec/esparser.c +++ b/drivers/staging/media/meson/vdec/esparser.c @@ -300,6 +300,7 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf) u32 num_dst_bufs = 0; u32 offset; u32 pad_size; + u32 wp, wp2; /* * When max ref frame is held by VP9, this should be -= 3 to prevent a @@ -354,15 +355,21 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf) } pad_size = esparser_pad_start_code(core, vb, payload_size); + wp = amvdec_read_parser(core, PARSER_VIDEO_WP); ret = esparser_write_data(core, phy, payload_size + pad_size); + wp2 = amvdec_read_parser(core, PARSER_VIDEO_WP); if (ret <= 0) { - dev_warn(core->dev, "esparser: input parsing error\n"); - amvdec_remove_ts(sess, vb->timestamp); - v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); amvdec_write_parser(core, PARSER_FETCH_CMD, 0); - return 0; + if (ret < 0 || wp2 == wp) { + dev_err(core->dev, "esparser: input parsing error ret %d (%x <=> %x)\n", + ret, wp, wp2); + amvdec_remove_ts(sess, vb->timestamp); + v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); + + return 0; + } } atomic_inc(&sess->esparser_queued_bufs); -- 2.34.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] media: meson: vdec: esparser: check parsing state with hardware write pointer 2023-01-23 9:43 [PATCH] media: meson: vdec: esparser: check parsing state with hardware write pointer Christian Hewitt @ 2023-01-23 9:47 ` Neil Armstrong 2023-01-23 12:04 ` Dan Carpenter 1 sibling, 0 replies; 4+ messages in thread From: Neil Armstrong @ 2023-01-23 9:47 UTC (permalink / raw) To: Christian Hewitt, Mauro Carvalho Chehab, Greg Kroah-Hartman, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-media, linux-amlogic, linux-staging, linux-arm-kernel, linux-kernel Cc: Neil Armstrong On 23/01/2023 10:43, Christian Hewitt wrote: > From: Neil Armstrong <narmstrong@baylibre.com> > > Also check the hardware write pointer to check if ES Parser has stalled. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > Signed-off-by: Christian Hewitt <christianshewitt@gmail.com> > --- > drivers/staging/media/meson/vdec/esparser.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c > index df5956c6141d..41b705f999b2 100644 > --- a/drivers/staging/media/meson/vdec/esparser.c > +++ b/drivers/staging/media/meson/vdec/esparser.c > @@ -300,6 +300,7 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf) > u32 num_dst_bufs = 0; > u32 offset; > u32 pad_size; > + u32 wp, wp2; > > /* > * When max ref frame is held by VP9, this should be -= 3 to prevent a > @@ -354,15 +355,21 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf) > } > > pad_size = esparser_pad_start_code(core, vb, payload_size); > + wp = amvdec_read_parser(core, PARSER_VIDEO_WP); > ret = esparser_write_data(core, phy, payload_size + pad_size); > + wp2 = amvdec_read_parser(core, PARSER_VIDEO_WP); > > if (ret <= 0) { > - dev_warn(core->dev, "esparser: input parsing error\n"); > - amvdec_remove_ts(sess, vb->timestamp); > - v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); > amvdec_write_parser(core, PARSER_FETCH_CMD, 0); > > - return 0; > + if (ret < 0 || wp2 == wp) { > + dev_err(core->dev, "esparser: input parsing error ret %d (%x <=> %x)\n", > + ret, wp, wp2); > + amvdec_remove_ts(sess, vb->timestamp); > + v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); > + > + return 0; > + } > } > > atomic_inc(&sess->esparser_queued_bufs); It's weird, but: Acked-by: Neil Armstrong <neil.armstrong@linaro.org> Thanks Christian for handling those changes Neil ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: meson: vdec: esparser: check parsing state with hardware write pointer 2023-01-23 9:43 [PATCH] media: meson: vdec: esparser: check parsing state with hardware write pointer Christian Hewitt 2023-01-23 9:47 ` Neil Armstrong @ 2023-01-23 12:04 ` Dan Carpenter 2023-03-21 13:58 ` Hans Verkuil 1 sibling, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2023-01-23 12:04 UTC (permalink / raw) To: Christian Hewitt Cc: Neil Armstrong, Mauro Carvalho Chehab, Greg Kroah-Hartman, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-media, linux-amlogic, linux-staging, linux-arm-kernel, linux-kernel, Neil Armstrong On Mon, Jan 23, 2023 at 09:43:00AM +0000, Christian Hewitt wrote: > From: Neil Armstrong <narmstrong@baylibre.com> > > Also check the hardware write pointer to check if ES Parser has stalled. > Presumably this is something which has happens in real life? Say a user has this hardware and is wondering if this patch fixes their bug, what does this bug look like to them? Please add a Fixes tag. > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > Signed-off-by: Christian Hewitt <christianshewitt@gmail.com> > --- > drivers/staging/media/meson/vdec/esparser.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c > index df5956c6141d..41b705f999b2 100644 > --- a/drivers/staging/media/meson/vdec/esparser.c > +++ b/drivers/staging/media/meson/vdec/esparser.c > @@ -300,6 +300,7 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf) > u32 num_dst_bufs = 0; > u32 offset; > u32 pad_size; > + u32 wp, wp2; > > /* > * When max ref frame is held by VP9, this should be -= 3 to prevent a > @@ -354,15 +355,21 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf) > } > > pad_size = esparser_pad_start_code(core, vb, payload_size); > + wp = amvdec_read_parser(core, PARSER_VIDEO_WP); > ret = esparser_write_data(core, phy, payload_size + pad_size); > + wp2 = amvdec_read_parser(core, PARSER_VIDEO_WP); > Why is check not done inside the esparser_write_data() function? It really feels like doing a write where nothing happens should be a bug for both callers... The esparser_write_data() function returns > 0 on success, 0 on timeout (I guess timeout is an error-ish) and negative if the user presses CTR-C. There are no comments to explain the unusual returns. Could we clean this up so it just returns negatives on error and zero on success or if not then let's add a comment explaining what's going on? > if (ret <= 0) { > - dev_warn(core->dev, "esparser: input parsing error\n"); > - amvdec_remove_ts(sess, vb->timestamp); > - v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); > amvdec_write_parser(core, PARSER_FETCH_CMD, 0); > > - return 0; > + if (ret < 0 || wp2 == wp) { So in this patch if there is a timeout but something was written then that's kind of a success path? regards, dan carpenter > + dev_err(core->dev, "esparser: input parsing error ret %d (%x <=> %x)\n", > + ret, wp, wp2); > + amvdec_remove_ts(sess, vb->timestamp); > + v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); > + > + return 0; > + } > } > > atomic_inc(&sess->esparser_queued_bufs); ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: meson: vdec: esparser: check parsing state with hardware write pointer 2023-01-23 12:04 ` Dan Carpenter @ 2023-03-21 13:58 ` Hans Verkuil 0 siblings, 0 replies; 4+ messages in thread From: Hans Verkuil @ 2023-03-21 13:58 UTC (permalink / raw) To: Christian Hewitt Cc: Neil Armstrong, Mauro Carvalho Chehab, Greg Kroah-Hartman, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-media, linux-amlogic, linux-staging, linux-arm-kernel, linux-kernel, Neil Armstrong, Dan Carpenter Hi Christian, On 23/01/2023 13:04, Dan Carpenter wrote: > On Mon, Jan 23, 2023 at 09:43:00AM +0000, Christian Hewitt wrote: >> From: Neil Armstrong <narmstrong@baylibre.com> >> >> Also check the hardware write pointer to check if ES Parser has stalled. >> > > Presumably this is something which has happens in real life? Say a user > has this hardware and is wondering if this patch fixes their bug, what > does this bug look like to them? > > Please add a Fixes tag. I'm marking this as Changes Requested in patchwork. Dan has good comments, and in general, if something is 'weird', then it needs to be commented carefully. Regards, Hans > >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> Signed-off-by: Christian Hewitt <christianshewitt@gmail.com> >> --- >> drivers/staging/media/meson/vdec/esparser.c | 15 +++++++++++---- >> 1 file changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c >> index df5956c6141d..41b705f999b2 100644 >> --- a/drivers/staging/media/meson/vdec/esparser.c >> +++ b/drivers/staging/media/meson/vdec/esparser.c >> @@ -300,6 +300,7 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf) >> u32 num_dst_bufs = 0; >> u32 offset; >> u32 pad_size; >> + u32 wp, wp2; >> >> /* >> * When max ref frame is held by VP9, this should be -= 3 to prevent a >> @@ -354,15 +355,21 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf) >> } >> >> pad_size = esparser_pad_start_code(core, vb, payload_size); >> + wp = amvdec_read_parser(core, PARSER_VIDEO_WP); >> ret = esparser_write_data(core, phy, payload_size + pad_size); >> + wp2 = amvdec_read_parser(core, PARSER_VIDEO_WP); >> > > Why is check not done inside the esparser_write_data() function? It > really feels like doing a write where nothing happens should be a bug > for both callers... > > The esparser_write_data() function returns > 0 on success, 0 on timeout > (I guess timeout is an error-ish) and negative if the user presses > CTR-C. There are no comments to explain the unusual returns. Could we > clean this up so it just returns negatives on error and zero on success > or if not then let's add a comment explaining what's going on? > >> if (ret <= 0) { >> - dev_warn(core->dev, "esparser: input parsing error\n"); >> - amvdec_remove_ts(sess, vb->timestamp); >> - v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); >> amvdec_write_parser(core, PARSER_FETCH_CMD, 0); >> >> - return 0; >> + if (ret < 0 || wp2 == wp) { > > So in this patch if there is a timeout but something was written then > that's kind of a success path? > > regards, > dan carpenter > >> + dev_err(core->dev, "esparser: input parsing error ret %d (%x <=> %x)\n", >> + ret, wp, wp2); >> + amvdec_remove_ts(sess, vb->timestamp); >> + v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); >> + >> + return 0; >> + } >> } >> >> atomic_inc(&sess->esparser_queued_bufs); > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-21 13:58 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-23 9:43 [PATCH] media: meson: vdec: esparser: check parsing state with hardware write pointer Christian Hewitt 2023-01-23 9:47 ` Neil Armstrong 2023-01-23 12:04 ` Dan Carpenter 2023-03-21 13:58 ` Hans Verkuil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).