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

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

 



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




[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