mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
To: Hans Verkuil <hverkuil+cisco@kernel.org>,
	"Ming Qian(OSS)" <ming.qian@oss.nxp.com>
Cc: "sashiko-reviews@lists.linux.dev"
	<sashiko-reviews@lists.linux.dev>,
	 "hverkuil@kernel.org"	 <hverkuil@kernel.org>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	 "imx@lists.linux.dev"	 <imx@lists.linux.dev>,
	"Frank.Li@kernel.org" <Frank.Li@kernel.org>,
	 "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	Eagle Zhou <eagle.zhou@nxp.com>,
	Elliot Chen	 <elliot.chen@nxp.com>, Frank Li <frank.li@nxp.com>,
	 "linux-kernel@vger.kernel.org"	 <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC 1/2] media: docs: Clarify V4L2_FMT_FLAG_DYN_RESOLUTION usage
Date: Wed, 10 Jun 2026 10:00:51 -0400	[thread overview]
Message-ID: <a199bf51ac0d7123ba360bec5f47bf3f1ad5a3e3.camel@collabora.com> (raw)
In-Reply-To: <349d6d48-9996-4791-833c-5c01e07883fd@kernel.org>

[-- Attachment #1: Type: text/plain, Size: 4814 bytes --]

Le mercredi 10 juin 2026 à 08:15 +0200, Hans Verkuil a écrit :
> > The more natural fix would be to add V4L2_EVENT_SOURCE_CHANGE for the
> > initial resolution case as well, then properly report
> > V4L2_FMT_FLAG_DYN_RESOLUTION through VIDIOC_ENUM_FMT. If all existing
> > drivers that support mid-stream DRC can similarly be updated to fire the
> > event for initial resolution, then perhaps the spec change to split the
> > flag isn't strictly necessary - drivers just need to be fixed to comply
> > with the current definition.
> > 
> > I'd appreciate hearing others' thoughts on whether this unified approach
> > is practical across all existing drivers.
> 
> Just a quick reply to mention that the s5p-mfc driver predates a lot of the
> newer uAPIs for stateful codecs. It's why it behaves in a not-quite-standard
> way. So there is history here. The resolution change support was added in
> commit 0520e4cc3bb9d back in 2014, three years after the driver was first
> added to the kernel. The stateful decoder spec as we have today was only
> introduced in 2019.

Fair point. Now, some clarification about the initial event. When this was re-
implemented in GStreamer, we explicitly don't prepare the capture queue (no
REQBUFS/CREATE_BUF, its not streaming). So effectively, the driver is forced to
start with only the output queue, and that was assumed to force the initial
event. The src_ch event indicates that the driver is stuck until the capture
queue has been reconfigured (or configured in this cased).

I think you make a good point that this is not very clearly stated, but it was
also intentional to use that flag as a boundary before expecting this very
strict behaviour. From a HW stand point, it does make quite some sense, since
the parsing of header and discovery should happen separately from the capture
buffer requirement. This is the main difference between stateless and stateful.
In userspace, this workflow allowed to better decouple the output and capture
threads, which very typically is a independent threads (true for GStreamer and
Chromium at minimum).

In the legacy path in gstreamer, the capture queue format is guessed, allocated
and set to streaming. In that case, the driver only have to emit the initial
event if the capture queue configuration miss-match its requirements. I believe
that was done so it aligned with some legacy flow. In theory this should work
with earlier implementation of SOURCE_CHANGE, but remains a best effort. CODA960
being the only legacy implementation I kept testing over time. With CODA today,
if the guessed allocation failed, everything fails, which seems to be a bug or
limitation from you analyses. Fixing it can be tricky, as we need to ensure we
maintain backward compatibility with older userspace. Specially for CODA which
is massively deployed. A lot of strangeness in CODA comes from its reversed
engineered nature. Note that, I do have spec for this one these days, and can
answer questions. I do know that we actually get a subset of the SPS, which
should fully cover the needs for the DYN_RESOLUTION.

The thing that also comes with V4L2_FMT_FLAG_DYN_RESOLUTION, is also the drain
flow. There was just no drain flow prior to this and the matching decoder spec.
So to drain the capture queue on SOURCE_CHANGE, we had to track the state of the
queue, and avoid going back into polling once the queue was drained (since
polling previously blocked). That tracking have simply never been implemented in
GStreamer. The only legacy draining flow we had, was the infamous zero sized
buffer, which was ambiguous, since there was 3 possibilities instead of 2:

- Failed decode
- Failed decode and drained
- Drained

If a decode failed at entropy decode, the resulting failed buffer would be
signalled and confused with the drain. And so the other infamous LAST flag was
added. But then MFC signals the drain state independently, not at the same time
as the last buffer, and so EPIPE on DQBUF was introduced. With the new spec, we
forced this drain flow onto both draining on end-of-stream (CMD_STOP) and to
happen somewhere after any SOURCE_CHANGE, and if you use
V4L2_FMT_FLAG_DYN_RESOLUTION you must follow the DRC flow from the new spec.

I don't know if sharing the drain mechanism for DRC and actual CMD_STOP was the
best idea in the world, but it is what is it now, and for sure, old or
unmaintained drivers endup with highly inconsistent state. I'm happy with the
idea to improve the spec, and to improve the drivers concistency too, as long as
we can actually test, since it too complex to change without testing. I think,
if something could help, it would be to implement couple of the legacy flow in
vivid driver, so we could test virtually.

Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

  reply	other threads:[~2026-06-10 14:00 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-08  3:56 [PATCH RFC 0/2] media: amphion: Clarify and restore V4L2_FMT_FLAG_DYN_RESOLUTION for VC1L Ming Qian
2026-06-08  3:56 ` [PATCH RFC 1/2] media: docs: Clarify V4L2_FMT_FLAG_DYN_RESOLUTION usage Ming Qian
     [not found]   ` <20260608040616.1A3EE1F00893@smtp.kernel.org>
2026-06-08  6:26     ` Ming Qian (OSS)
2026-06-09 18:00       ` Nicolas Dufresne
2026-06-10  2:49         ` Ming Qian(OSS)
2026-06-10  3:36           ` Ming Qian(OSS)
2026-06-10  6:15             ` Hans Verkuil
2026-06-10 14:00               ` Nicolas Dufresne [this message]
2026-06-10 13:20           ` Nicolas Dufresne
2026-06-11  4:32             ` Ming Qian(OSS)
2026-06-08  3:56 ` [PATCH RFC 2/2] media: amphion: Add V4L2_FMT_FLAG_DYN_RESOLUTION for VC1L Ming Qian

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a199bf51ac0d7123ba360bec5f47bf3f1ad5a3e3.camel@collabora.com \
    --to=nicolas.dufresne@collabora.com \
    --cc=Frank.Li@kernel.org \
    --cc=eagle.zhou@nxp.com \
    --cc=elliot.chen@nxp.com \
    --cc=frank.li@nxp.com \
    --cc=hverkuil+cisco@kernel.org \
    --cc=hverkuil@kernel.org \
    --cc=imx@lists.linux.dev \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=ming.qian@oss.nxp.com \
    --cc=sashiko-reviews@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®