Re: [PATCH] media: uvcvideo: query pan/tilt position from the device on every read

[Michael Jordan]

Hi Ricardo,

On Fri, 31 Jul 2026 10:33:02 +0200, Ricardo Ribalda <ribalda@xxxxxxxxxxxx> wrote:
> Dear Michael Jordan
>
> (I always wanted to start an email like that :P)

Happy to have been able to help with that.

> I am not against the idea proposed by this patch, but I would like to
> understand your use case a bit better. Do you want to know where the
> camera is because:
> - You want to do something continuously with that info (e.g., tagging
> frames with the live position)?
> - Or because you just want to know when the camera has actually
> finished moving?

The former. The application computes framing adjustments from the
current pose -- "move 10 degrees left of wherever you are now" needs
to know where "now" is -- and shows live position while the gimbal
moves. A single end-of-move notification would not be enough even for
the commanded-move case, and it does not exist at all for the
autonomous one, which brings me to:

> You also mentioned that the camera can reposition itself. Do you mean
> that the camera could be tracking something and decide to move on its
> own?

Yes, exactly that. This camera (and similar PTZ conference cameras)
has on-device AI tracking: once enabled, the gimbal follows a person
continuously, for minutes at a time, with no host involvement at all.
While tracking is active the position is a continuously varying
quantity with no SET_CUR in flight and no "end of movement" to
report. That is the case that made me give up on every event-shaped
mechanism:

> If it is the latter, you could use V4L2 control events. That should
> work perfectly for signalling the end of a move:

Three separate things go wrong, in increasing order of stubbornness:

- uvcvideo's device-originated control events come from exactly one
  place, uvc_ctrl_status_event(), fed by the Control Change
  interrupt, and the spec only promises that interrupt "at the end of
  the movement" (4.2.2.1.15). So even a perfect device yields one
  event per move, not a trajectory.

- This device never emits the interrupt at all. GET_INFO on
  CT_PANTILT_ABSOLUTE_CONTROL returns 0x03 -- neither D3 (Autoupdate)
  nor D4 (Asynchronous) -- so the event path has nothing to deliver.
  (Arguably a firmware defect per 2.4.4, but it looks systemic: the
  value is identical across every CT control on this device, so the
  firmware is not computing it per control.)

- Under autonomous tracking there is no end of movement, so even
  compliant firmware would have nothing to hook the event to.

> - Because your patch uses UVC_CTRL_DATA_CURRENT for the
> read-modify-write path, the control will send (100, 300). That will
> unintentionally move both axes, yanking the PAN back to 100 and
> fighting the camera's autonomous tracking.

The behaviour is real, but the patch does not introduce it: the write
path is byte-identical before and after. What the RMW merges against
today is decided by the device's GET_INFO bits, and both outcomes you
are weighing already ship:

- On a device whose GET_INFO clears AUTO_UPDATE (this one),
  ctrl->loaded stays set after the commit, so the tilt write merges
  against the cached setpoint and sends (100, 300) -- your scenario,
  reproduced exactly, on an unpatched kernel.

- On a device that reports Autoupdate, uvc_ctrl_commit_entity()
  clears ctrl->loaded, so the tilt write does a fresh GET_CUR and
  merges against live data, sending (200, 300) -- kind to the
  tracker, but this is the variant that cancels a move the *user*
  commanded: issue two single-axis writes a few tens of milliseconds
  apart, and the second merges against a live reading in which the
  first axis has barely left its old position, and commits it right
  back.

I have measured that second failure on real hardware (this camera
keeps AUTO_UPDATE when probed while asleep, since it does not answer
GET_INFO in that state -- its own small horror story): S_CTRL(pan),
then S_CTRL(tilt) ~20ms later, and pan is yanked back to where it
started. Same hardware with AUTO_UPDATE cleared, both axes reach
their targets. Both merge sources are in the tree today; the patch
adds neither and removes neither.

If anything the patch improves your scenario slightly on Autoupdate
devices: reads no longer set ctrl->loaded, so the write path can no
longer merge against a stale sample frozen in the cache by an
earlier G_CTRL.

> What I think we should do instead is manage the cache validity based
> on who "owns" the control. For ASYNCHRONOUS controls that have an
> active handle (i.e., a user-commanded move that has not completed), we
> mark them as "loaded". But when they are owned/updated by the device
> autonomously, we never mark them as loaded.

I think ownership is the right frame -- for the write path. Which
merge source is correct genuinely depends on who owns the control:
while a user move is in flight, the setpoint (live data cancels the
move); while the device owns it, live data (the setpoint fights the
tracker). That is a real pre-existing problem, your scenario is a
good demonstration of one horn of it, and I would be glad to help fix
it. But I do not think it can replace this patch, for the reason you
already identified:

> This specific logic still doesn't support polling the live mid-flight
> position of a user-commanded move, but maybe V4L2 events can cover
> your actual use case?

Right -- and the mid-flight (or mid-tracking) position is the entire
use case. However the write path picks its merge source, G_CTRL still
has to report the actual position, which is what this patch does
without touching the merge at all. The two compose: an ownership-
based write path plus a live read path leaves nothing stale anywhere.

Two practical notes on the pseudo-code:

> - ctrl->loaded = 1;
> + if (!FLAG_AUTO_UPDATE)
> + ctrl->loaded = 1;

__uvc_ctrl_load_cur() fills UVC_CTRL_DATA_CURRENT, which is the RMW
source, so with this hunk every load puts live data where the write
path merges -- the user-move cancellation above, now unconditional on
every Autoupdate device. Keeping the live value in a buffer of its
own is precisely how the patch avoids creating that. Separately,
anything keyed on AUTO_UPDATE or ASYNCHRONOUS will not trigger on
this device, since GET_INFO reports neither -- one reason the patch
uses a static table flag that uvc_ctrl_get_flags() does not rewrite.
The ownership logic would also need a completion signal for "the move
has finished", and the only candidate is the Control Change interrupt
this firmware never sends.

Happy to respin if you would like any of this argued in the commit
message itself -- the autonomous-tracking use case in particular is
probably worth stating there rather than in a reply.

Thanks again for the review.

Best regards,
Michael Jordan