Re: [PATCH v7 07/27] media: entity: Use pad as the starting point for a pipeline

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

Hi Jacopo,

On Thu, Jul 08, 2021 at 02:36:20PM +0200, Jacopo Mondi wrote:
> Hello Tomi,
>     A few minors and a question below
> 
> On Mon, May 24, 2021 at 01:43:48PM +0300, Tomi Valkeinen wrote:
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > The pipeline has been moved from the entity to the pads; reflect this in
> > the media pipeline function API.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> >  Documentation/driver-api/media/mc-core.rst    |  6 ++--
> >  drivers/media/mc/mc-entity.c                  | 24 ++++++-------
> >  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  6 ++--
> >  .../media/platform/exynos4-is/fimc-capture.c  |  8 ++---
> >  .../platform/exynos4-is/fimc-isp-video.c      |  8 ++---
> >  drivers/media/platform/exynos4-is/fimc-lite.c |  8 ++---
> >  drivers/media/platform/omap3isp/ispvideo.c    |  6 ++--
> >  .../media/platform/qcom/camss/camss-video.c   |  6 ++--
> >  drivers/media/platform/rcar-vin/rcar-dma.c    |  6 ++--
> >  .../platform/rockchip/rkisp1/rkisp1-capture.c |  6 ++--
> >  .../media/platform/s3c-camif/camif-capture.c  |  6 ++--
> >  drivers/media/platform/stm32/stm32-dcmi.c     |  6 ++--
> >  .../platform/sunxi/sun4i-csi/sun4i_dma.c      |  6 ++--
> >  .../platform/sunxi/sun6i-csi/sun6i_video.c    |  6 ++--
> >  drivers/media/platform/ti-vpe/cal-video.c     |  6 ++--
> >  drivers/media/platform/vsp1/vsp1_video.c      |  6 ++--
> >  drivers/media/platform/xilinx/xilinx-dma.c    |  6 ++--
> >  .../media/test-drivers/vimc/vimc-capture.c    |  6 ++--
> >  drivers/media/usb/au0828/au0828-core.c        |  8 ++---
> >  drivers/staging/media/imx/imx-media-utils.c   |  6 ++--
> >  drivers/staging/media/ipu3/ipu3-v4l2.c        |  6 ++--
> >  drivers/staging/media/omap4iss/iss_video.c    |  6 ++--
> >  drivers/staging/media/tegra-video/tegra210.c  |  6 ++--
> >  include/media/media-entity.h                  | 34 +++++++++----------
> >  24 files changed, 98 insertions(+), 100 deletions(-)
> >
> > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst
> > index 8a13640bed56..69a64279a61f 100644
> > --- a/Documentation/driver-api/media/mc-core.rst
> > +++ b/Documentation/driver-api/media/mc-core.rst
> > @@ -213,11 +213,11 @@ When starting streaming, drivers must notify all entities in the pipeline to
> >  prevent link states from being modified during streaming by calling
> >  :c:func:`media_pipeline_start()`.
> >
> > -The function will mark all entities connected to the given entity through
> > -enabled links, either directly or indirectly, as streaming.
> > +The function will mark all entities connected to the given pad through
> 
> As the stream_count counter is now moved to the pads, should this be
> 
> +The function will mark all the pads connected to the given pad through

Yes.

> 
> > +enabled routes and links, either directly or indirectly, as streaming.
> >
> >  The struct media_pipeline instance pointed to by
> > -the pipe argument will be stored in every entity in the pipeline.
> > +the pipe argument will be stored in every pad in the pipeline.
> >  Drivers should embed the struct media_pipeline
> 
> Does this still apply ?

Yes.

> 
> >  in higher-level pipeline structures and can then access the
> >  pipeline through the struct media_entity
> 
> This sentence should probably be changed to
> 
> pipeline through the struct media_pad pipe field.

Yes.

> 
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index ea1cf7f63ae8..e6451903359c 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -404,12 +404,11 @@ EXPORT_SYMBOL_GPL(media_entity_get_fwnode_pad);
> >   * Pipeline management
> >   */
> >
> > -__must_check int __media_pipeline_start(struct media_entity *entity,
> > +__must_check int __media_pipeline_start(struct media_pad *pad,
> >  					struct media_pipeline *pipe)
> >  {
> > -	struct media_device *mdev = entity->graph_obj.mdev;
> > +	struct media_device *mdev = pad->graph_obj.mdev;
> >  	struct media_graph *graph = &pipe->graph;
> > -	struct media_pad *pad = entity->pads;
> >  	struct media_pad *pad_err = pad;
> >  	struct media_link *link;
> >  	int ret;
> > @@ -542,24 +541,23 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> >  }
> >  EXPORT_SYMBOL_GPL(__media_pipeline_start);
> >
> > -__must_check int media_pipeline_start(struct media_entity *entity,
> > +__must_check int media_pipeline_start(struct media_pad *pad,
> >  				      struct media_pipeline *pipe)
> 
> As it seems that even with the full series applied
> media_pipeline_start() is always called with entity->pads as its first
> argument, I wonder if it wouldn't be more linear for a driver to keep
> using entity and have this function here pass the entity's pads to
> __media_pipeline_start().
> 
> Do we expect drivers to actually start the pipeline using a specific
> pad ?

The pipeline is moved to pads with this patch, and therefore it's logical
to do start from pads, too. Should there be more than one, figuring out
which one to use would not be possible.

-- 
Sakari Ailus