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

From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Ezequiel Garcia <ezequiel@collabora.com>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
	linux-kernel@vger.kernel.org
Cc: Tomasz Figa <tfiga@chromium.org>,
	kernel@collabora.com, Jonas Karlman <jonas@kwiboo.se>,
	Heiko Stuebner <heiko@sntech.de>,
	Alexandre Courbot <acourbot@chromium.org>,
	Jeffrey Kardatzke <jkardatzke@chromium.org>,
	Rob Herring <robh@kernel.org>
Subject: Re: [PATCH v2 3/8] hantro: Use v4l2_m2m_buf_done_and_job_finish
Date: Thu, 26 Mar 2020 14:30:01 -0400	[thread overview]
Message-ID: <3fb1dcdbbf54051d9a8fee1d1498583c3a79cecd.camel@ndufresne.ca> (raw)
In-Reply-To: <648c8411353071a7e1ffd3576d268b01177ab678.camel@collabora.com>

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