| Message ID | 20200318132108.21873-4-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hantro: set of small cleanups and fixes | expand |
On 3/18/20 2:21 PM, Ezequiel Garcia wrote: > Let the core sort out the nuances of returning buffers > to userspace, by using the v4l2_m2m_buf_done_and_job_finish > helper. > > This change also removes usage of buffer sequence fields, > which shouldn't have any meaning for stateless decoders. Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance? Also, while I agree that it is not terribly useful, it doesn't hurt, does it? And the V4L2 spec makes no exception for stateless codecs with respect to the sequence field. Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, Hans > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/staging/media/hantro/hantro_drv.c | 27 ++++++++--------------- > 1 file changed, 9 insertions(+), 18 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > index 0b1200fc0e1a..ec889d755cd6 100644 > --- a/drivers/staging/media/hantro/hantro_drv.c > +++ b/drivers/staging/media/hantro/hantro_drv.c > @@ -94,32 +94,23 @@ static void hantro_job_finish(struct hantro_dev *vpu, > unsigned int bytesused, > enum vb2_buffer_state result) > { > - struct vb2_v4l2_buffer *src, *dst; > int ret; > > pm_runtime_mark_last_busy(vpu->dev); > pm_runtime_put_autosuspend(vpu->dev); > clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks); > > - src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); > - dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); > - > - if (WARN_ON(!src)) > - return; > - if (WARN_ON(!dst)) > - return; > - > - src->sequence = ctx->sequence_out++; > - dst->sequence = ctx->sequence_cap++; > - > - ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused); > - if (ret) > - result = VB2_BUF_STATE_ERROR; > + if (ctx->buf_finish) { > + struct vb2_v4l2_buffer *dst; > > - v4l2_m2m_buf_done(src, result); > - v4l2_m2m_buf_done(dst, result); > + dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > + ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused); > + if (ret) > + result = VB2_BUF_STATE_ERROR; > + } > > - v4l2_m2m_job_finish(vpu->m2m_dev, ctx->fh.m2m_ctx); > + v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx, > + result); > } > > void hantro_irq_done(struct hantro_dev *vpu, unsigned int bytesused, >
Le mercredi 25 mars 2020 à 09:22 +0100, Hans Verkuil a écrit : > On 3/18/20 2:21 PM, Ezequiel Garcia wrote: > > Let the core sort out the nuances of returning buffers > > to userspace, by using the v4l2_m2m_buf_done_and_job_finish > > helper. > > > > This change also removes usage of buffer sequence fields, > > which shouldn't have any meaning for stateless decoders. > > Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance? > Also, while I agree that it is not terribly useful, it doesn't hurt, does it? > > And the V4L2 spec makes no exception for stateless codecs with respect to the > sequence field. > > Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> The spec also does not say what it means either. As an example, you have spec for ALTERNATE interlacing mode that changes the meaning of the sequence, but not for alternate H264 fields (which cannot be part of the format, since this changes often). We also don't have spec for the the sequence behaviour while using HOLD features. I'm worried we are falling into the test driven trap, were people implement features to satisfy a test, while the added complexity don't really make sense. Shouldn't we change our approach and opt-out features for new type of HW, so that we can keep the drivers code saner? > > Regards, > > Hans > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/staging/media/hantro/hantro_drv.c | 27 ++++++++--------------- > > 1 file changed, 9 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > > index 0b1200fc0e1a..ec889d755cd6 100644 > > --- a/drivers/staging/media/hantro/hantro_drv.c > > +++ b/drivers/staging/media/hantro/hantro_drv.c > > @@ -94,32 +94,23 @@ static void hantro_job_finish(struct hantro_dev *vpu, > > unsigned int bytesused, > > enum vb2_buffer_state result) > > { > > - struct vb2_v4l2_buffer *src, *dst; > > int ret; > > > > pm_runtime_mark_last_busy(vpu->dev); > > pm_runtime_put_autosuspend(vpu->dev); > > clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks); > > > > - src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); > > - dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); > > - > > - if (WARN_ON(!src)) > > - return; > > - if (WARN_ON(!dst)) > > - return; > > - > > - src->sequence = ctx->sequence_out++; > > - dst->sequence = ctx->sequence_cap++; > > - > > - ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused); > > - if (ret) > > - result = VB2_BUF_STATE_ERROR; > > + if (ctx->buf_finish) { > > + struct vb2_v4l2_buffer *dst; > > > > - v4l2_m2m_buf_done(src, result); > > - v4l2_m2m_buf_done(dst, result); > > + dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > > + ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused); > > + if (ret) > > + result = VB2_BUF_STATE_ERROR; > > + } > > > > - v4l2_m2m_job_finish(vpu->m2m_dev, ctx->fh.m2m_ctx); > > + v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx, > > + result); > > } > > > > void hantro_irq_done(struct hantro_dev *vpu, unsigned int bytesused, > >
On 3/25/20 3:02 PM, Nicolas Dufresne wrote: > Le mercredi 25 mars 2020 à 09:22 +0100, Hans Verkuil a écrit : >> On 3/18/20 2:21 PM, Ezequiel Garcia wrote: >>> Let the core sort out the nuances of returning buffers >>> to userspace, by using the v4l2_m2m_buf_done_and_job_finish >>> helper. >>> >>> This change also removes usage of buffer sequence fields, >>> which shouldn't have any meaning for stateless decoders. >> >> Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance? >> Also, while I agree that it is not terribly useful, it doesn't hurt, does it? >> >> And the V4L2 spec makes no exception for stateless codecs with respect to the >> sequence field. >> >> Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > The spec also does not say what it means either. As an example, you > have spec for ALTERNATE interlacing mode that changes the meaning of > the sequence, but not for alternate H264 fields (which cannot be part > of the format, since this changes often). We also don't have spec for > the the sequence behaviour while using HOLD features. I hate it that the spec changes the sequence meaning for FIELD_ALTERNATE, I always thought that that made drivers unnecessarily complicated. Unfortunately, this is something we inherited. Currently the spec says for sequence: "Set by the driver, counting the frames (not fields!) in sequence. This field is set for both input and output devices." The only thing missing here is that it should say that for compressed formats this counts the buffers, since one buffer with compressed data may not have a one-to-one mapping with frames. This description for 'sequence' was never updated when compressed data formats were added, so it is a bit out of date. > > I'm worried we are falling into the test driven trap, were people > implement features to satisfy a test, while the added complexity don't > really make sense. Shouldn't we change our approach and opt-out > features for new type of HW, so that we can keep the drivers code > saner? Why wasn't the existing code in this patch sane? Sure, we can change the spec, but then 1) all existing drivers need to be updated as well, and 2) v4l2-compliance needs to be changed to test specifically for this class of drivers and ensure that for those the sequence field is set to 0. Not to mention introducing an exception in the uAPI where the sequence field suddenly isn't used anymore. Frankly, I would prefer that the whole sequence handling is moved to videobuf2-v4l2.c. It really doesn't belong in drivers, with the exception of incrementing the sequence counter in case of dropped frames. I think I suggested it when vb2 was being designed, but at the time the preference was to keep it in the driver. Long time ago, though. And another reason why I want to keep it: I find it actually useful to see a running counter: it helps keeping track of how many buffers you've processed since you started streaming. Finally, the removal of the sequence counter simply does not belong in this patch. Regards, Hans > >> >> Regards, >> >> Hans >> >>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>> --- >>> drivers/staging/media/hantro/hantro_drv.c | 27 ++++++++--------------- >>> 1 file changed, 9 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c >>> index 0b1200fc0e1a..ec889d755cd6 100644 >>> --- a/drivers/staging/media/hantro/hantro_drv.c >>> +++ b/drivers/staging/media/hantro/hantro_drv.c >>> @@ -94,32 +94,23 @@ static void hantro_job_finish(struct hantro_dev *vpu, >>> unsigned int bytesused, >>> enum vb2_buffer_state result) >>> { >>> - struct vb2_v4l2_buffer *src, *dst; >>> int ret; >>> >>> pm_runtime_mark_last_busy(vpu->dev); >>> pm_runtime_put_autosuspend(vpu->dev); >>> clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks); >>> >>> - src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); >>> - dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); >>> - >>> - if (WARN_ON(!src)) >>> - return; >>> - if (WARN_ON(!dst)) >>> - return; >>> - >>> - src->sequence = ctx->sequence_out++; >>> - dst->sequence = ctx->sequence_cap++; >>> - >>> - ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused); >>> - if (ret) >>> - result = VB2_BUF_STATE_ERROR; >>> + if (ctx->buf_finish) { >>> + struct vb2_v4l2_buffer *dst; >>> >>> - v4l2_m2m_buf_done(src, result); >>> - v4l2_m2m_buf_done(dst, result); >>> + dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); >>> + ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused); >>> + if (ret) >>> + result = VB2_BUF_STATE_ERROR; >>> + } >>> >>> - v4l2_m2m_job_finish(vpu->m2m_dev, ctx->fh.m2m_ctx); >>> + v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx, >>> + result); >>> } >>> >>> void hantro_irq_done(struct hantro_dev *vpu, unsigned int bytesused, >>> >
1. On Wed, 2020-03-25 at 16:28 +0100, Hans Verkuil wrote: > On 3/25/20 3:02 PM, Nicolas Dufresne wrote: > > Le mercredi 25 mars 2020 à 09:22 +0100, Hans Verkuil a écrit : > > > On 3/18/20 2:21 PM, Ezequiel Garcia wrote: > > > > Let the core sort out the nuances of returning buffers > > > > to userspace, by using the v4l2_m2m_buf_done_and_job_finish > > > > helper. > > > > > > > > This change also removes usage of buffer sequence fields, > > > > which shouldn't have any meaning for stateless decoders. > > > > > > Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance? > > > Also, while I agree that it is not terribly useful, it doesn't hurt, does it? > > > > > > And the V4L2 spec makes no exception for stateless codecs with respect to the > > > sequence field. > > > > > > Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > > > The spec also does not say what it means either. As an example, you > > have spec for ALTERNATE interlacing mode that changes the meaning of > > the sequence, but not for alternate H264 fields (which cannot be part > > of the format, since this changes often). We also don't have spec for > > the the sequence behaviour while using HOLD features. > > I hate it that the spec changes the sequence meaning for FIELD_ALTERNATE, > I always thought that that made drivers unnecessarily complicated. Unfortunately, > this is something we inherited. > > Currently the spec says for sequence: > > "Set by the driver, counting the frames (not fields!) in sequence. This field is set > for both input and output devices." > > The only thing missing here is that it should say that for compressed formats this > counts the buffers, since one buffer with compressed data may not have a one-to-one > mapping with frames. > > This description for 'sequence' was never updated when compressed data formats were > added, so it is a bit out of date. > > > I'm worried we are falling into the test driven trap, were people > > implement features to satisfy a test, while the added complexity don't > > really make sense. Shouldn't we change our approach and opt-out > > features for new type of HW, so that we can keep the drivers code > > saner? > > Why wasn't the existing code in this patch sane? Sure, we can change the spec, but > then 1) all existing drivers need to be updated as well, and 2) v4l2-compliance needs > to be changed to test specifically for this class of drivers and ensure that for those > the sequence field is set to 0. Not to mention introducing an exception in the uAPI > where the sequence field suddenly isn't used anymore. > > Frankly, I would prefer that the whole sequence handling is moved to videobuf2-v4l2.c. > It really doesn't belong in drivers, with the exception of incrementing the sequence > counter in case of dropped frames. > > I think I suggested it when vb2 was being designed, but at the time the preference > was to keep it in the driver. Long time ago, though. > Do you think we could try to move this to the core? I might be able find some time to try that. > And another reason why I want to keep it: I find it actually useful to see a running > counter: it helps keeping track of how many buffers you've processed since you started > streaming. > +1 > Finally, the removal of the sequence counter simply does not belong in this patch. > Agreed, no complaints on my side. I am actually happy about this feedback. Thanks, Ezequiel
Le mercredi 25 mars 2020 à 17:30 -0300, Ezequiel Garcia a écrit : > 1. On Wed, 2020-03-25 at 16:28 +0100, Hans Verkuil wrote: > > On 3/25/20 3:02 PM, Nicolas Dufresne wrote: > > > Le mercredi 25 mars 2020 à 09:22 +0100, Hans Verkuil a écrit : > > > > On 3/18/20 2:21 PM, Ezequiel Garcia wrote: > > > > > Let the core sort out the nuances of returning buffers > > > > > to userspace, by using the v4l2_m2m_buf_done_and_job_finish > > > > > helper. > > > > > > > > > > This change also removes usage of buffer sequence fields, > > > > > which shouldn't have any meaning for stateless decoders. > > > > > > > > Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance? > > > > Also, while I agree that it is not terribly useful, it doesn't hurt, does it? > > > > > > > > And the V4L2 spec makes no exception for stateless codecs with respect to the > > > > sequence field. > > > > > > > > Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > > > > > The spec also does not say what it means either. As an example, you > > > have spec for ALTERNATE interlacing mode that changes the meaning of > > > the sequence, but not for alternate H264 fields (which cannot be part > > > of the format, since this changes often). We also don't have spec for > > > the the sequence behaviour while using HOLD features. > > > > I hate it that the spec changes the sequence meaning for FIELD_ALTERNATE, > > I always thought that that made drivers unnecessarily complicated. Unfortunately, > > this is something we inherited. > > > > Currently the spec says for sequence: > > > > "Set by the driver, counting the frames (not fields!) in sequence. This field is set > > for both input and output devices." > > > > The only thing missing here is that it should say that for compressed formats this > > counts the buffers, since one buffer with compressed data may not have a one-to-one > > mapping with frames. That's also why I think it's programatically useless in that case, there is no logic for which input/output can be related unless you know what the framing is. > > > > This description for 'sequence' was never updated when compressed data formats were > > added, so it is a bit out of date. > > > > > I'm worried we are falling into the test driven trap, were people > > > implement features to satisfy a test, while the added complexity don't > > > really make sense. Shouldn't we change our approach and opt-out > > > features for new type of HW, so that we can keep the drivers code > > > saner? > > > > Why wasn't the existing code in this patch sane? Sure, we can change the spec, but > > then 1) all existing drivers need to be updated as well, and 2) v4l2-compliance needs > > to be changed to test specifically for this class of drivers and ensure that for those > > the sequence field is set to 0. Not to mention introducing an exception in the uAPI > > where the sequence field suddenly isn't used anymore. > > > > Frankly, I would prefer that the whole sequence handling is moved to videobuf2-v4l2.c. > > It really doesn't belong in drivers, with the exception of incrementing the sequence > > counter in case of dropped frames. > > > > I think I suggested it when vb2 was being designed, but at the time the preference > > was to keep it in the driver. Long time ago, though. > > > > Do you think we could try to move this to the core? I'm also happy as long as drivers stop having to implement this generic statistic. Note, that only applies to existing m2m, we still need that counter to detect driver side frame drops in CAPTURE only devices (like UVC cameras). > > I might be able find some time to try that. > > > And another reason why I want to keep it: I find it actually useful to see a running > > counter: it helps keeping track of how many buffers you've processed since you started > > streaming. > > > > +1 > > > Finally, the removal of the sequence counter simply does not belong in this patch. > > > > Agreed, no complaints on my side. > > I am actually happy about this feedback. > > Thanks, > Ezequiel > >
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c index 0b1200fc0e1a..ec889d755cd6 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c @@ -94,32 +94,23 @@ static void hantro_job_finish(struct hantro_dev *vpu, unsigned int bytesused, enum vb2_buffer_state result) { - struct vb2_v4l2_buffer *src, *dst; int ret; pm_runtime_mark_last_busy(vpu->dev); pm_runtime_put_autosuspend(vpu->dev); clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks); - src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); - dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); - - if (WARN_ON(!src)) - return; - if (WARN_ON(!dst)) - return; - - src->sequence = ctx->sequence_out++; - dst->sequence = ctx->sequence_cap++; - - ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused); - if (ret) - result = VB2_BUF_STATE_ERROR; + if (ctx->buf_finish) { + struct vb2_v4l2_buffer *dst; - v4l2_m2m_buf_done(src, result); - v4l2_m2m_buf_done(dst, result); + dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); + ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused); + if (ret) + result = VB2_BUF_STATE_ERROR; + } - v4l2_m2m_job_finish(vpu->m2m_dev, ctx->fh.m2m_ctx); + v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx, + result); } void hantro_irq_done(struct hantro_dev *vpu, unsigned int bytesused,
Let the core sort out the nuances of returning buffers to userspace, by using the v4l2_m2m_buf_done_and_job_finish helper. This change also removes usage of buffer sequence fields, which shouldn't have any meaning for stateless decoders. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/staging/media/hantro/hantro_drv.c | 27 ++++++++--------------- 1 file changed, 9 insertions(+), 18 deletions(-)