Messages in this thread | | | Subject | Re: [PATCH v2 3/8] hantro: Use v4l2_m2m_buf_done_and_job_finish | From | Ezequiel Garcia <> | Date | Wed, 25 Mar 2020 17:30:32 -0300 |
| |
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
| |