Re: [PATCH 6/7] media: mc: Expand MUST_CONNECT flag to always require an enabled link

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Jan 15, 2024 at 10:59:14AM +0000, Sakari Ailus wrote:
> On Mon, Jan 15, 2024 at 12:55:25PM +0200, Laurent Pinchart wrote:
> > On Mon, Jan 15, 2024 at 10:43:56AM +0000, Sakari Ailus wrote:
> > > Hi Laurent,
> > > 
> > > Many thanks for working on this.
> > 
> > You're welcome. It was somehow fun.
> > 
> > > On Mon, Jan 15, 2024 at 12:30:28PM +0200, Laurent Pinchart wrote:
> > > > The MEDIA_PAD_FL_MUST_CONNECT flag indicates that the pad requires an
> > > > enabled link to stream, but only if it has any link at all. This makes
> > > > little sense, as if a pad is part of a pipeline, there are very few use
> > > > cases for an active link to be mandatory only if links exist at all. A
> > > > review of in-tree drivers confirms they all need an enabled link for
> > > > pads marked with the MEDIA_PAD_FL_MUST_CONNECT flag.
> > > > 
> > > > Expand the scope of the flag by rejecting pads that have no links at
> > > > all. This requires modifying the pipeline build code to add those pads
> > > > to the pipeline.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  .../media/mediactl/media-types.rst            | 11 ++--
> > > >  drivers/media/mc/mc-entity.c                  | 53 +++++++++++++++----
> > > >  2 files changed, 48 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst
> > > > index 0ffeece1e0c8..1ce87c0b705f 100644
> > > > --- a/Documentation/userspace-api/media/mediactl/media-types.rst
> > > > +++ b/Documentation/userspace-api/media/mediactl/media-types.rst
> > > > @@ -375,12 +375,11 @@ Types and flags used to represent the media graph elements
> > > >  	  are origins of links.
> > > >  
> > > >      *  -  ``MEDIA_PAD_FL_MUST_CONNECT``
> > > > -       -  If this flag is set and the pad is linked to any other pad, then
> > > > -	  at least one of those links must be enabled for the entity to be
> > > > -	  able to stream. There could be temporary reasons (e.g. device
> > > > -	  configuration dependent) for the pad to need enabled links even
> > > > -	  when this flag isn't set; the absence of the flag doesn't imply
> > > > -	  there is none.
> > > > +       -  If this flag, then at least one link connected to the pad must be
> > > > +          enabled for the pad to be able to stream. There could be temporary
> > > > +          reasons (e.g. device configuration dependent) for the pad to need
> > > > +          enabled links even when this flag isn't set; the absence of the flag
> > > > +          doesn't imply there is none.
> > > 
> > > Shoudln't you indent by tabs first here?
> > 
> > My text editor picked the option it liked best. I'll fix indentation to
> > avoid switching from tabs to spaces.
> > 
> > > Would it be possible to backport this, too? I'm pretty sure there will be
> > > NULL pointer dereferences due to this, even previous to the graph walk
> > > rework.
> > 
> > Patches 1/7 and 2/7 should be simple to backport (hopefully). Patch 3/7
> > should as well, which will fix the crash in the imx8-isi driver. Patches
> > 4/7 to 6/7 may be more difficult to backport as they could generate more
> > conflicts, it depends how far back you want to go. That would be my
> > preferred option though.
> > 
> > > It may require reworking this to apply it to the pre-rework implementation
> > > and that's outside the scope of this set obviously.
> > 
> > The rework (v6.1) predates the imx8-isi driver (v6.4), so from an
> > imx8-isi point of view, we don't have to backport this earlier than
> > v6.4.
> 
> How certain are you this only affects the imx8-isi driver? There are many
> drivers using the MUST_CONNECT flag and I'm not sure all those have the
> necessary checks in place. There of course could be drivers just missing
> the flag, too, and that's a different issue.

The imx8-isi was not using the flag, so it's a different issue. The ISI
is special in the sense that it has an input crossbar switch that can
have Asome non-connected inputs in valid configurations.

For drivers using the flag, in most cases there should be at least one
link connected to MUST_CONNECT pads. Many drivers are for 1-sink,
1-source subdevs, and those should not get registered in the first place
if they have nothing connected to their source.

One exception may be the ipu3-cio2 driver which has multiple independent
CSI-2 RXs. Some of them will not have any sensor connected. This could
be addressed in the ipu3-cio2 driver to not register those CSI-2 RXs
(and the corresponding video nodes). Alternatively I'm fine if you try
to backport this series before v6.1, but I won't :-)

-- 
Regards,

Laurent Pinchart