Re: [PATCH 1/1] media: subdev: Make get_fmt on INTERNAL pads without STREAMS an error

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

On Wed, Oct 07, 2026 at 12:14:46PM +0200, Hans Verkuil wrote:
> On 07/10/2026 11:48, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Wed, Oct 07, 2026 at 11:43:35AM +0200, Hans Verkuil wrote:
> >> On 06/10/2026 11:29, Sakari Ailus wrote:
> >>> Internal pads should only be accessible to file handles with
> >>> V4L2_SUBDEV_CLIENT_CAP_STREAMS flag set. Return an error otherwise.
> >>>
> >>> Fixes: 49cdf56876d3 ("media: mc: Add INTERNAL pad flag")
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>> ---
> >>>  drivers/media/v4l2-core/v4l2-subdev.c | 7 ++++++-
> >>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> index a07d77e584c3..7cb5a40f0f8c 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> @@ -854,8 +854,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> >>>  	case VIDIOC_SUBDEV_G_FMT: {
> >>>  		struct v4l2_subdev_format *format = arg;
> >>>  
> >>> -		if (!client_supports_streams)
> >>> +		if (!client_supports_streams) {
> >>> +			if (format->pad < sd->entity.num_pads &&
> >>> +			    sd->entity.pads[format->pad].flags & MEDIA_PAD_FL_INTERNAL)
> >>> +				return -EINVAL;
> >>> +
> >>>  			format->stream = 0;
> >>> +		}
> >>>  
> >>>  		memset(format->reserved, 0, sizeof(format->reserved));
> >>>  		memset(format->format.reserved, 0, sizeof(format->format.reserved));
> >>>
> >>
> >> There is no documentation that I can find that says that MEDIA_PAD_FL_INTERNAL is only available
> >> if V4L2_SUBDEV_CLIENT_CAP_STREAMS is set.
> >>
> >> I think this needs some more thought: if internal pads are only available if that cap is set,
> >> then I expect that a lot more ioctls will need this check. In that case the check should become
> >> a helper function.
> >>
> >> But how does this affect e.g. G_TOPOLOGY or ENUM_LINKS? If the cap is not set, should internal
> >> pads still be reported?
> > 
> > We don't have client capabilities on MC side, at least not right now. The
> > INTERNAL pads are intended to be used in very special circumstances and for
> > that we do have sub-device capability flags. They were introduced in this
> > cycle and if we allow wider access to them now, there could be issues
> > blocking that as the interface they expose isn't intended to be used
> > without these capability flags.
> 
> So are the internal pads exposed or not through these MC ioctls? That's not clear
> to me. It's not documented either.

The pads naturally are exposed via MC, like any other pads. Neither the
sink nor source pad documentation discusses whether the pads are
user-visible so I don't thin this explicitly needs to be specified for
internal pads either.

> 
> > 
> > I'll rework this to apply to the rest of the pad-related IOCTLs.
> > 
> >>
> >> And you need checks in v4l2-compliance, ensuring that trying to access internal pads without
> >> that cap will indeed fail.
> > 
> > I can add that.
> > 
> >>
> >> Ideally you would like to have this emulated in vimc as well.
> >>
> >> In other words, I think this needs more work, both in the core and w.r.t. documentation.
> > 
> > The INTERNAL pad flag was introduced as the Maxim serdes driver needed it
> > and we thought it's fine to merge it early; the bulk of the documentation
> > resides in the depths of my metadata series.
> > 
> 
> Ah, that's not something I was aware of (or may have been aware of, but forgotten). Adding
> a new uAPI requires a lot more care: proper documentation, v4l2-compliance tests, if at all
> possible emulation support in one of the test drivers.

Note that the maxim-serdes driver does not expose any functionality that
would depend on the internal pads solely; it requires streams API
enablement that is behind another kernel change. It is thus very, very
unlikely to cause any issues. On the other hand, no functionality is
disabled by the other two patches I posted to hide the INTERNAL flag from
the userspace.

An example of the contrary, though, is fairly recently merged STREAMS
capability flag, which in its current state cannot be taken into use --
we'll have to use another bit, as enabling this flag now will break
libcamera. The problem here is that the flag is changing the behaviour of
existing interfaces but at the time of the introduction of the flag it
wasn't exactly specified how the existing interfaces would be affected.

> 
> Neither was it reviewed by the media maintainers (me or Mauro). I'm really not happy about
> that. uAPI changes have to be reviewed by the media maintainers.
> 
> Before rc1 is released you must have a patch series ready (and reviewed by me) adding proper
> documentation, fixing issues like the one addressed in this patch, and a patch for
> v4l2-compliance.
> 
> Ideally also a patch to emulate this in the test driver (probably vimc), but that will
> likely take more time.
> 
> If it is not properly documented etc., then I might decide to revert this driver + uAPI
> change in 7.4-rcX.
> 
> As media committer it is your responsibility to ensure that the media maintainers have
> reviewed uAPI changes and are OK with it. That clearly didn't happen here.

In retrospect, I agree I should have explictly asked you before merging the
patch. My apologies for that. Still, versions of the patch have been out
for review for more than three years and while the previous version you've
commented is v6 (in 2023), there have been six more versions for review
where you've been cc'd and additional 15 versions of the maxim-serdes
driver patchset containing the same patch (where you haven't been cc'd
though).

My hope is that we'd get the metadata series, which contains multi-stream
support on which work has been done for more than a decade now, merged in
7.5 (or at least not much later than that), with userspace support in
libcamera which Jai has been working on. I'll post a new version of that
once we have rc1.

-- 
Kind regards,

Sakari Ailus