Re: [PATCH v8 36/38] media: v4l: Add V4L2_SUBDEV_ROUTE_FL_IMMUTABLE sub-device routing flag

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sakari,

On Tue, Apr 09, 2024 at 01:21:16PM +0000, Sakari Ailus wrote:
> On Thu, Mar 21, 2024 at 07:03:08PM +0200, Laurent Pinchart wrote:
> > On Wed, Mar 13, 2024 at 07:39:17AM +0000, Sakari Ailus wrote:
> > > On Wed, Mar 13, 2024 at 09:34:13AM +0200, Tomi Valkeinen wrote:
> > > > On 13/03/2024 09:25, Sakari Ailus wrote:
> > > > > Add a flag to denote immutable routes, V4L2_SUBDEV_ROUTE_FL_IMMUTABLE.
> > > > > Such routes cannot be changed and they're always active.
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > >   Documentation/userspace-api/media/v4l/dev-subdev.rst         | 3 ++-
> > > > >   .../userspace-api/media/v4l/vidioc-subdev-g-routing.rst      | 5 +++++
> > > > >   include/uapi/linux/v4l2-subdev.h                             | 5 +++++
> > > > >   3 files changed, 12 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > index 08495cc6f4a6..2f2423f676cf 100644
> > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > @@ -572,7 +572,8 @@ internal pad always has a single stream only (0).
> > > > >   Routes from an internal source pad to an external source pad are typically not
> > > > >   modifiable but they can be activated and deactivated using the
> > > > >   :ref:`V4L2_SUBDEV_ROUTE_FL_ACTIVE <v4l2-subdev-routing-flags>` flag, depending
> > > > > -on driver capabilities.
> > > > > +on driver capabilities. This capatibility is indicated by the
> > > > > +:ref:`V4L2_SUBDEV_ROUTE_FL_IMMUTABLE <v4l2-subdev-routing-flags>` flag.
> > 
> > That's not very clear, it sounds like V4L2_SUBDEV_ROUTE_FL_IMMUTABLE
> > indicates that the route can be enabled/disabled. I'd rewrite this.
> 
> I'll use instead:
> 
> The :ref:`V4L2_SUBDEV_ROUTE_FL_IMMUTABLE <v4l2-subdev-routing-flags>` flag
> indicates that the ``V4L2_SUBDEV_ROUTE_FLAG_ACTIVE`` of the route may not
> be unset.

Sounds good to me.

> > > > >   Interaction between routes, streams, formats and selections
> > > > >   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> > > > > index 08b8d17cef3f..cd7735f9104e 100644
> > > > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> > > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> > > > > @@ -139,6 +139,11 @@ Also ``VIDIOC_SUBDEV_S_ROUTING`` may return more route than the user provided in
> > > > >       * - V4L2_SUBDEV_ROUTE_FL_ACTIVE
> > > > >         - 0x0001
> > > > >         - The route is enabled. Set by applications.
> > > > > +    * - V4L2_SUBDEV_ROUTE_FL_IMMUTABLE
> > > > > +      - 0x0002
> > > > > +      - The route is immutable. Set by the driver. The
> > > > > +	``V4L2_SUBDEV_ROUTE_FL_ACTIVE`` flag of an immutable route may not be
> > > > > +	changed.
> > 
> > May not be changed and will always be set ?
> 
> How about "may not be unset"?

Even better :-)

> > > > >   Return Value
> > > > >   ============
> > > > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> > > > > index ca543982460c..7e501cb45e4e 100644
> > > > > --- a/include/uapi/linux/v4l2-subdev.h
> > > > > +++ b/include/uapi/linux/v4l2-subdev.h
> > > > > @@ -200,6 +200,11 @@ struct v4l2_subdev_capability {
> > > > >    * on a video node.
> > > > >    */
> > > > >   #define V4L2_SUBDEV_ROUTE_FL_ACTIVE		(1U << 0)
> > > > > +/*
> > > > > + * Is the route immutable. The ACTIVE flag of an immutable route may not be
> > > > > + * changed.
> > > > > + */
> > > > > +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE		(1U << 1)
> > > > >   /**
> > > > >    * struct v4l2_subdev_route - A route inside a subdev
> > > > 
> > > > Is the route fully immutable? The sink/source stream ID cannot be changed
> > > > (or any new fields we might come up with in the future)?
> > > 
> > > I think the new fields should be considered separately when they're added.
> > > This also applies to the stream IDs, I'll add this to the documentation.
> > > 
> > > The naming of the flag is aligned with MC link flag with a similar purpose.
> > > 
> > > > Hmm, or would a route with different stream IDs be a, well, different
> > > > route...
> > > > 
> > > > The docs here only talk about the ACTIVE flag. Would
> > > > V4L2_SUBDEV_ROUTE_FL_ALWAYS_ACTIVE be a better name, to be more explicit on
> > > > the meaning?
> > > 
> > > I prefer immutable. I wonder what Laurent and Hans think.
> > 
> > I'm fine with either. IMMUTABLE is shorter, if that makes a difference.

-- 
Regards,

Laurent Pinchart




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux