Hi Laurent, On Mon, Jan 15, 2024 at 12:55:25PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > 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@xxxxxxxxxxxxxxxx> > > > --- > > > .../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. -- Regards, Sakari Ailus
