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

[Ricardo Ribalda]

Hi Michael

On Fri, 31 Jul 2026 at 17:15, Michael Jordan <jordan.mymail@xxxxxxxxx> wrote:
>
> 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:

How accurate does the mapping frame/position need to be?

You do not know how big the camera pipeline is. Maybe you are
processing frame NOW-4 when you read position  NOW

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

And we rely on that interrupt to know when the device is done and we
can go to sleep.

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

If the device is not setting the Asynchronous I think we have bigger
problems to deal with.

Without the async, the device will go to sleep as soon as you set the
control (if you are not streaming).

You definitely have to ping the vendor to make sure they fix their firmware.

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

If AUTO_UPDATE is present you would get fresh data when you poll the control.

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

unrelated note: if you need to set the two axis at the set time, you
can use  V4L.VIDIOC_S_EXT_CTRLS
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-g-ext-ctrls.html#c.V4L.VIDIOC_S_EXT_CTRLS

(You probably are aware of it, just want to make sure that you know it exists).

In fact you could use that today to avoid part of the kernel issue.
Always send pan and tilt when you are tracking the movement.

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

Let's wait a bit for Laurent or HansG (or even Hans Verkuil) to comment.

There are different problems here:
1) Your firmware is broken. We need to fix the general case assuming
compliant cameras. (We can introduce a quirk later, for your device)
2) We want different behaviour based on what is happening:
   - The camera is tracking an object autonomously:
                   - Get the live values for pan and tilt and use them
as reference when we change pan or tilt
   - The camera is finalising a movement required by the user
                   - Use the pair of pain/tilt provived by the user as
reference when we change pan or tilt.

note: A firmware that sets the AUTO_UPDATE flag could be used by a
userspace that is aware of who owns the control.

I am very curious what HansV thinks.

Regards

PS: I am planning to be OOO next week so my replies will be slower.


>
> Thanks again for the review.
>
> Best regards,
> Michael Jordan



-- 
Ricardo Ribalda