Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
From: Jacopo Mondi <jacopo@jmondi.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: "Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
	"Benoit Parrot" <bparrot@ti.com>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v2 09/30] media: entity: Swap pads if route is checked from source to sink
Date: Wed, 6 Mar 2019 09:29:46 +0100
Message-ID: <20190306082946.bxcf6jnl52nrc6q3@uno.localdomain> (raw)
In-Reply-To: <20190305200458.GK14928@pendragon.ideasonboard.com>

[-- Attachment #1: Type: text/plain, Size: 5718 bytes --]

HI Laurent,

On Tue, Mar 05, 2019 at 10:04:58PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Mar 04, 2019 at 01:35:36PM +0100, Jacopo Mondi wrote:
> > On Fri, Feb 22, 2019 at 02:18:11PM +0200, Laurent Pinchart wrote:
> > > On Mon, Feb 18, 2019 at 10:21:07AM +0100, Jacopo Mondi wrote:
> > >> On Tue, Jan 22, 2019 at 05:20:30PM +0200, Laurent Pinchart wrote:
> > >>> On Tue, Jan 22, 2019 at 05:15:06PM +0200, Sakari Ailus wrote:
> > >>>> On Wed, Jan 16, 2019 at 12:57:43AM +0200, Laurent Pinchart wrote:
> > >>>>>>
> > >>>>>> This way the pads are always passed to the has_route() op sink pad first.
> > >>>>>> Makes sense.
> > >>>>>
> > >>>>> Is there anything in the API that mandates one pad to be a sink and the
> > >>>>> other pad to the a source ? I had designed the operation to allow
> > >>>>> sink-sink and source-source connections to be checked too.
> > >>>>
> > >>>> Do you have a use case in mind for sink--sink or source--source routes? The
> > >>>> routes are about flows of data, so I'd presume only source--sink or
> > >>>> sink--source routes are meaningful.
> > >>>>
> > >>>> If you did, then the driver would have to handle that by itself. This still
> > >>>> simplifies the implementation for drivers that do not.
> > >>>
> > >>> I don't have use cases for such routes, but we use the has_route
> > >>> operation when traversing pipelines, and at that point we need to get
> > >>> all the internally connected pads. In another patch in this series you
> > >>> implement a helper function that handles this, but its implementation
> > >>> isn't complete. I explained in my review of that patch that I fear a
> > >>> correct generic implementation would become quite complex, while the
> > >>> complexity should be easy to handle on the driver side as the code can
> > >>> then be specialized for the case at hand.
> > >>>
> > >>
> > >> As a compromise, in v3 I'm thinking of maintaining support for the
> > >> most common case of two sources connected to the same sink, as
> > >> Sakari's patch does, but let more complex cases be handled by the
> > >> driver implementation of has_route().
> > >>
> > >> Ack?
> > >
> > > I fear this will be confusing for subdevs, as they would have to
> > > implement part of the operation.
> > >
> > > Could it be that the subdev has_route operation isn't the best API for
> > > the job, if it gets that complex ? I wonder if it would be easier to
> > > create another operation that takes a pad index as argument, and returns
> > > the list of pads (possibly as a bitmask ?) or connected pads.
> > > media_entity_has_route() could easily be implemented on top of that, and
> > > these new semantics may be easier for subdevs to implement.
> > >
> >
> > I see, but if subdevs can easily elaborate that list, they could as
> > well easily check if the pad provided as argument is on that list.
>
> Possibly. In any case, if we keep this operation as-is, I wouldn't try
> to split the logic between the subdev drivers and the core, that would
> be asking for trouble. If it gets too complex to implement for subdev
> drivers, then we need a different operation with a different logic in
> the subdev API, and a helper that wraps around it.

In v3 I have removed support for indirect routes from the framework
part. It's all on the subdevice driver for now.
>
> > >>>>> If your goal is to simplify the implementation of the .has_route()
> > >>>>> operation in drivers, I would instead sort pad0 and pad1 by value.
> > >>>>
> > >>>> That'd be another option to make the order deterministic for the driver.
> > >>>> I'm fine with that as well.
> > >>
> > >> In v3 I have taken both suggestions in: try the "sink then source" order
> > >> first, then order by index in case the pads are of the same time. This
> > >> needs to be documented in has_route() operation definition though.
> > >>
> > >> Would that be fine with you?
> > >
> > > I think that's the worst of both worlds from a subdev point of view :-)
> >
> > Possibly :)
> >
> > Should we drop completely the sink-source ordering in favour of
> > ordering by value, and drop [15/30] that adds support for trivial
> > indirect routes?
> >
> > Let's reach consensus so I could send v3.
>
> I would certainly drop 15/30, and I don't think ordering by value would
> help subdev drivers much.

Yes, but sorting by index makes it easier to deal with the sink-sink
and source-source use cases, if the subdevice supports indirect
routes.

I have dropped 15/30 and specified pads are passed by index in v3.

Thanks
  j

>
> > >>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >>>>>> ---
> > >>>>>>  drivers/media/media-entity.c | 4 ++++
> > >>>>>>  1 file changed, 4 insertions(+)
> > >>>>>>
> > >>>>>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > >>>>>> index 3c0e7425c8983b45..33f00e35ccd92c6f 100644
> > >>>>>> --- a/drivers/media/media-entity.c
> > >>>>>> +++ b/drivers/media/media-entity.c
> > >>>>>> @@ -249,6 +249,10 @@ bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
> > >>>>>>  	if (!entity->ops || !entity->ops->has_route)
> > >>>>>>  		return true;
> > >>>>>>
> > >>>>>> +	if (entity->pads[pad0].flags & MEDIA_PAD_FL_SOURCE
> > >>>>>> +	    && entity->pads[pad1].flags & MEDIA_PAD_FL_SINK)
> > >>>>>> +		swap(pad0, pad1);
> > >>>>>> +
> > >>>>>>  	return entity->ops->has_route(entity, pad0, pad1);
> > >>>>>>  }
> > >>>>>>  EXPORT_SYMBOL_GPL(media_entity_has_route);
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply index

Thread overview: 90+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-01 23:31 [PATCH v2 00/30] v4l: add support for multiplexed streams Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 01/30] media: entity: Use pad as a starting point for graph walk Niklas Söderlund
2019-01-15 21:43   ` Laurent Pinchart
2018-11-01 23:31 ` [PATCH v2 02/30] media: entity: Use pads instead of entities in the media graph walk stack Niklas Söderlund
2019-01-15 22:03   ` Laurent Pinchart
2019-01-15 22:13     ` Sakari Ailus
2019-01-15 22:07   ` Laurent Pinchart
2018-11-01 23:31 ` [PATCH v2 03/30] media: entity: Walk the graph based on pads Niklas Söderlund
2019-01-15 22:21   ` Laurent Pinchart
     [not found]     ` <20190115223406.mxgzl36cp54gb7nv@kekkonen.localdomain>
2019-01-15 23:28       ` Laurent Pinchart
2019-01-22 14:50         ` Sakari Ailus
2019-02-14 15:15   ` Jacopo Mondi
2018-11-01 23:31 ` [PATCH v2 04/30] v4l: mc: Start walk from a specific pad in use count calculation Niklas Söderlund
2019-01-15 22:24   ` Laurent Pinchart
2019-01-15 22:36     ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 05/30] media: entity: Move the pipeline from entity to pads Niklas Söderlund
2019-01-15 22:38   ` Laurent Pinchart
2019-01-15 22:48     ` Sakari Ailus
2019-02-14 15:53   ` Jacopo Mondi
2018-11-01 23:31 ` [PATCH v2 06/30] media: entity: Use pad as the starting point for a pipeline Niklas Söderlund
2019-01-15 22:54   ` Laurent Pinchart
2019-01-22 15:31     ` Sakari Ailus
2019-01-22 15:37       ` Laurent Pinchart
2019-01-22 16:16         ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 07/30] media: entity: Add has_route entity operation Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 08/30] media: entity: Add media_has_route() function Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 09/30] media: entity: Swap pads if route is checked from source to sink Niklas Söderlund
2019-01-15 22:57   ` Laurent Pinchart
2019-01-22 15:15     ` Sakari Ailus
2019-01-22 15:20       ` Laurent Pinchart
2019-02-18  9:21         ` Jacopo Mondi
2019-02-22 12:18           ` Laurent Pinchart
2019-03-04 12:35             ` Jacopo Mondi
2019-03-05 20:04               ` Laurent Pinchart
2019-03-06  8:29                 ` Jacopo Mondi [this message]
2018-11-01 23:31 ` [PATCH v2 10/30] media: entity: Use routing information during graph traversal Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 11/30] media: entity: Skip link validation for pads to which there is no route to Niklas Söderlund
2019-01-15 23:13   ` Laurent Pinchart
2018-11-01 23:31 ` [PATCH v2 12/30] media: entity: Add an iterator helper for connected pads Niklas Söderlund
2019-01-15 23:24   ` Laurent Pinchart
2019-01-22 15:36     ` Sakari Ailus
2019-01-22 15:38       ` Laurent Pinchart
2019-01-22 16:21         ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 13/30] media: entity: Add only connected pads to the pipeline Niklas Söderlund
2019-01-15 23:33   ` Laurent Pinchart
2018-11-01 23:31 ` [PATCH v2 14/30] media: entity: Add debug information in graph walk route check Niklas Söderlund
2019-01-15 23:35   ` Laurent Pinchart
2019-01-22 15:38     ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 15/30] media: entity: Look for indirect routes Niklas Söderlund
2019-01-15 23:41   ` Laurent Pinchart
2019-01-22 15:56     ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 16/30] v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations Niklas Söderlund
2019-01-15 23:51   ` Laurent Pinchart
2019-01-22 16:14     ` Sakari Ailus
2019-01-22 17:00       ` Laurent Pinchart
2019-02-21 14:59     ` Jacopo Mondi
2019-02-21 23:49       ` Sakari Ailus
2019-02-22  8:46         ` Jacopo Mondi
2019-02-21 14:39   ` Jacopo Mondi
2019-02-21 22:31     ` Sakari Ailus
2019-02-22  8:40       ` Jacopo Mondi
2019-02-22 11:04         ` Sakari Ailus
2019-02-22 11:17           ` Jacopo Mondi
2019-02-22 11:29             ` Sakari Ailus
2019-02-22 13:37               ` Ian Arkver
2019-02-22 13:50               ` Geert Uytterhoeven
2018-11-01 23:31 ` [PATCH v2 17/30] v4l: subdev: compat: Implement handling for VIDIOC_SUBDEV_[GS]_ROUTING Niklas Söderlund
2019-01-08 10:04   ` Geert Uytterhoeven
2019-01-15 23:53   ` Laurent Pinchart
2019-01-22 15:57     ` Sakari Ailus
2019-02-18 11:21     ` Jacopo Mondi
2019-02-21 23:50       ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 18/30] v4l: subdev: Take routing information into account in link validation Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 19/30] v4l: subdev: Improve link format validation debug messages Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 20/30] v4l: mc: Add an S_ROUTING helper function for power state changes Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 21/30] v4l: Add bus type to frame descriptors Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 22/30] v4l: Add CSI-2 bus configuration " Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 23/30] v4l: Add stream to frame descriptor Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 24/30] adv748x: csi2: add translation from pixelcode to CSI-2 datatype Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 25/30] adv748x: csi2: only allow formats on sink pads Niklas Söderlund
2019-02-21 14:18   ` Jacopo Mondi
2018-11-01 23:31 ` [PATCH v2 26/30] adv748x: csi2: describe the multiplexed stream Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 27/30] adv748x: csi2: add internal routing configuration Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 28/30] adv748x: afe: add routing support Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 29/30] rcar-csi2: use frame description information to configure CSI-2 bus Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 30/30] rcar-csi2: expose the subdevice internal routing Niklas Söderlund
2018-11-14 13:10   ` Nikita Yushchenko
2018-11-14 19:45     ` Niklas Söderlund
2018-12-03 22:16 ` [PATCH v2 00/30] v4l: add support for multiplexed streams Sakari Ailus
2018-12-05 22:09   ` Niklas Söderlund

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190306082946.bxcf6jnl52nrc6q3@uno.localdomain \
    --to=jacopo@jmondi.org \
    --cc=bparrot@ti.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=niklas.soderlund+renesas@ragnatech.se \
    --cc=sakari.ailus@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox