All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jacopo Mondi <jacopo@jmondi.org>
To: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>,
	sakari.ailus@linux.intel.com, laurent.pinchart@ideasonboard.com,
	niklas.soderlund+renesas@ragnatech.se,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v3 26/31] adv748x: csi2: add internal routing configuration
Date: Fri, 15 Mar 2019 10:45:38 +0100	[thread overview]
Message-ID: <20190315094538.bs5ecsdzndrxjdbb@uno.localdomain> (raw)
In-Reply-To: <4f5b5763-be90-4040-7d55-986471168de1@lucaceresoli.net>

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

Hi Luca,

On Thu, Mar 14, 2019 at 03:45:27PM +0100, Luca Ceresoli wrote:
> Hi,
>
> begging your pardon for the noob question below...
>

Let a noob help another noob then

> On 05/03/19 19:51, Jacopo Mondi wrote:
> > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > Add support to get and set the internal routing between the adv748x
> > CSI-2 transmitters sink pad and its multiplexed source pad. This routing
> > includes which stream of the multiplexed pad to use, allowing the user
> > to select which CSI-2 virtual channel to use when transmitting the
> > stream.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 65 ++++++++++++++++++++++++
> >  1 file changed, 65 insertions(+)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index d8f7cbee86e7..13454af72c6e 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -14,6 +14,8 @@
> >
> >  #include "adv748x.h"
> >
> > +#define ADV748X_CSI2_ROUTES_MAX 4
> > +
> >  struct adv748x_csi2_format {
> >  	unsigned int code;
> >  	unsigned int datatype;
> > @@ -253,10 +255,73 @@ static int adv748x_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> >  	return 0;
> >  }
> >
> > +static int adv748x_csi2_get_routing(struct v4l2_subdev *sd,
> > +				    struct v4l2_subdev_krouting *routing)
> > +{
> > +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > +	struct v4l2_subdev_route *r = routing->routes;
> > +	unsigned int vc;
> > +
> > +	if (routing->num_routes < ADV748X_CSI2_ROUTES_MAX) {
> > +		routing->num_routes = ADV748X_CSI2_ROUTES_MAX;
> > +		return -ENOSPC;
> > +	}
> > +
> > +	routing->num_routes = ADV748X_CSI2_ROUTES_MAX;
> > +
> > +	for (vc = 0; vc < ADV748X_CSI2_ROUTES_MAX; vc++) {
> > +		r->sink_pad = ADV748X_CSI2_SINK;
> > +		r->sink_stream = 0;
> > +		r->source_pad = ADV748X_CSI2_SOURCE;
> > +		r->source_stream = vc;
> > +		r->flags = vc == tx->vc ? V4L2_SUBDEV_ROUTE_FL_ACTIVE : 0;
> > +		r++;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int adv748x_csi2_set_routing(struct v4l2_subdev *sd,
> > +				    struct v4l2_subdev_krouting *routing)
> > +{
> > +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > +	struct v4l2_subdev_route *r = routing->routes;
> > +	unsigned int i;
> > +	int vc = -1;
> > +
> > +	if (routing->num_routes > ADV748X_CSI2_ROUTES_MAX)
> > +		return -ENOSPC;
> > +
> > +	for (i = 0; i < routing->num_routes; i++) {
> > +		if (r->sink_pad != ADV748X_CSI2_SINK ||
> > +		    r->sink_stream != 0 ||
> > +		    r->source_pad != ADV748X_CSI2_SOURCE ||
> > +		    r->source_stream >= ADV748X_CSI2_ROUTES_MAX)
> > +			return -EINVAL;
> > +
> > +		if (r->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE) {
> > +			if (vc != -1)
> > +				return -EMLINK;
> > +
> > +			vc = r->source_stream;
> > +		}
> > +		r++;
> > +	}
> > +
> > +	if (vc != -1)
> > +		tx->vc = vc;
> > +
> > +	adv748x_csi2_set_virtual_channel(tx, tx->vc);
> > +
> > +	return 0;
> > +}
>
> Not specific to this patch but rather to the set_routing idea as a
> whole: can the set_routing ioctl be called while the stream is running?
>
> If it cannot, I find it a limiting factor for nowadays use cases. I also
> didn't find where the ioctl is rejected.
>

The framework does not make assumptions about that at the moment.

> If it can, then shouldn't this function call s_stream(stop) through the
> sink pad whose route becomes disabled, and a s_stream(start) through the
> one that gets enabled?
>

If I got this right, you're here rightfully pointing out that changing
the routing between pads in an entity migh impact the pipeline as a
whole, and this would require, to activate/deactivate devices that
where not part of the pipeline.

This is probably the wrong patch to use an example, as this one is for
a multiplexed interface, where there is no need to go through an
s_stream() for the two CSI-2 endpoints, but as you pointed out in our
brief offline chat, the AFE->TX routing example for this very device
is a good one: if we change the analogue source that is internally
routed to the CSI-2 output of the adv748x, do we need to s_stream(1)
the now routed entity and s_stream(0) on the not not-anymore-routed
one?

My gut feeling is that this is up to userspace, as it should know
what are the requirements of the devices in the system, but this mean
going through an s_stream(0)/s_stream(1) sequence on the video device,
and that would interrupt the streaming for sure.

At the same time, I don't feel too much at ease with the idea of
s_routing calling s_stream on the entity' remote subdevices, as this
would skip the link format validation that media_pipeline_start()
performs.

So yeah, I understand your point, but I don't have a real answer,
maybe someone else does and want to share his mind?

Thanks
   j

> Thanks,
> --
> Luca

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

  reply	other threads:[~2019-03-15  9:45 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-05 18:51 [PATCH v3 00/31] v4l: add support for multiplexed streams Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 01/31] media: entity: Use pad as a starting point for graph walk Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 02/31] media: entity: Use pads instead of entities in the media graph walk stack Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 03/31] media: entity: Walk the graph based on pads Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 04/31] v4l: mc: Start walk from a specific pad in use count calculation Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 05/31] media: entity: Add iterator helper for entity pads Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 06/31] media: entity: Move the pipeline from entity to pads Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 07/31] media: entity: Use pad as the starting point for a pipeline Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 08/31] media: entity: Add has_route entity operation Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 09/31] media: entity: Add media_has_route() function Jacopo Mondi
2019-03-14 14:45   ` Luca Ceresoli
2019-03-15  9:22     ` Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 10/31] media: entity: Use routing information during graph traversal Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 11/31] media: entity: Skip link validation for pads to which there is no route to Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 12/31] media: entity: Add an iterator helper for connected pads Jacopo Mondi
2019-03-07 10:09   ` Sakari Ailus
2019-03-07 10:27     ` Ian Arkver
2019-03-07 12:38       ` Sakari Ailus
2019-03-07 20:18       ` Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 13/31] media: entity: Add only connected pads to the pipeline Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 14/31] media: entity: Add debug information in graph walk route check Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 15/31] v4l: Add bus type to frame descriptors Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 16/31] v4l: Add CSI-2 bus configuration " Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 17/31] v4l: Add stream to frame descriptor Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 18/31] v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 19/31] media: Documentation: Add GS_ROUTING documentation Jacopo Mondi
2019-03-07 15:19   ` Sakari Ailus
2019-03-08 13:31     ` Jacopo Mondi
2019-03-08 14:14       ` Sakari Ailus
2019-03-14 14:44     ` Luca Ceresoli
2019-03-14 14:43   ` Luca Ceresoli
2020-02-13 13:36   ` Hans Verkuil
2019-03-05 18:51 ` [PATCH v3 20/31] v4l: subdev: Take routing information into account in link validation Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 21/31] v4l: subdev: Improve link format validation debug messages Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 22/31] v4l: mc: Add an S_ROUTING helper function for power state changes Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 23/31] adv748x: csi2: add translation from pixelcode to CSI-2 datatype Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 24/31] adv748x: csi2: only allow formats on sink pads Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 25/31] adv748x: csi2: describe the multiplexed stream Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 26/31] adv748x: csi2: add internal routing configuration Jacopo Mondi
2019-03-14 14:45   ` Luca Ceresoli
2019-03-15  9:45     ` Jacopo Mondi [this message]
2019-03-15 10:06       ` Sakari Ailus
2019-03-16 10:23         ` Luca Ceresoli
2019-03-20 17:14           ` Jacopo Mondi
2019-03-22 16:52             ` Luca Ceresoli
2019-03-28 15:08               ` Jacopo Mondi
2019-03-28 15:17                 ` Luca Ceresoli
2019-03-05 18:51 ` [PATCH v3 27/31] adv748x: afe: add routing support Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 28/31] adv748x: afe: Implement has_route() Jacopo Mondi
2019-03-07 12:49   ` Sakari Ailus
2019-03-14 14:45   ` Luca Ceresoli
2019-03-05 18:51 ` [PATCH v3 29/31] rcar-csi2: use frame description information to configure CSI-2 bus Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 30/31] rcar-csi2: expose the subdevice internal routing Jacopo Mondi
2019-03-05 18:51 ` [PATCH v3 31/31] media: rcar-csi2: Implement has_route() Jacopo Mondi
2019-03-07 12:56   ` Sakari Ailus
2019-03-07 22:28     ` Jacopo Mondi
2019-03-07  9:47 ` [PATCH v3 00/31] v4l: add support for multiplexed streams Sakari Ailus
2019-03-08 13:19   ` Jacopo Mondi
2019-03-08 14:12     ` Sakari Ailus

Reply instructions:

You may reply publicly 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=20190315094538.bs5ecsdzndrxjdbb@uno.localdomain \
    --to=jacopo@jmondi.org \
    --cc=jacopo+renesas@jmondi.org \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=luca@lucaceresoli.net \
    --cc=niklas.soderlund+renesas@ragnatech.se \
    --cc=sakari.ailus@linux.intel.com \
    --subject='Re: [PATCH v3 26/31] adv748x: csi2: add internal routing configuration' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.