Re: [PATCH v2 3/8] hantro: Use v4l2_m2m_buf_done_and_job_finish

[Ezequiel Garcia]

   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@xxxxxxxxx>
> > 
> > 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