From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Dafna Hirschfeld <dafna@fastmail.com> Cc: linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org, Heiko Stuebner <heiko@sntech.de>, Helen Koike <helen.koike@collabora.com>, Paul Elder <paul.elder@ideasonboard.com> Subject: Re: [PATCH v3 38/46] media: rkisp1: isp: Disallow multiple active sources Date: Tue, 12 Jul 2022 22:18:27 +0300 [thread overview] Message-ID: <Ys3JA6bTN2TG6uMn@pendragon.ideasonboard.com> (raw) In-Reply-To: <20220712184005.nwnlflsupgie6owf@guri> Hi Dafna, On Tue, Jul 12, 2022 at 09:40:05PM +0300, Dafna Hirschfeld wrote: > On 11.07.2022 15:42, Laurent Pinchart wrote: > > The ISP supports multiple source subdevs, but can only capture from a > > single one at a time. The source is selected through link setup. The > > driver finds the active source in its .s_stream() handler using the > > media_entity_remote_pad() function. This fails to reject invalid > > configurations with multiple active sources. Fix it by using the > > media_pad_remote_pad_unique() helper instead, and inline > > rkisp1_isp_get_source() in rkisp1_isp_s_stream() as the function is > > small and has a single caller. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Changes since v2: > > > > - Update media_pad_remote_pad_unique() function name in commit message > > --- > > .../platform/rockchip/rkisp1/rkisp1-isp.c | 30 ++++++++----------- > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > > index 37623b73b1d9..d7e2802d11f5 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > > @@ -58,20 +58,6 @@ > > * Helpers > > */ > > > > -static struct v4l2_subdev *rkisp1_isp_get_source(struct v4l2_subdev *sd) > > -{ > > - struct media_pad *local, *remote; > > - struct media_entity *sensor_me; > > - > > - local = &sd->entity.pads[RKISP1_ISP_PAD_SINK_VIDEO]; > > - remote = media_pad_remote_pad_first(local); > > - if (!remote) > > - return NULL; > > - > > - sensor_me = remote->entity; > > - return media_entity_to_v4l2_subdev(sensor_me); > > -} > > - > > static struct v4l2_mbus_framefmt * > > rkisp1_isp_get_pad_fmt(struct rkisp1_isp *isp, > > struct v4l2_subdev_state *sd_state, > > @@ -743,6 +729,8 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > > struct rkisp1_isp *isp = to_rkisp1_isp(sd); > > struct rkisp1_device *rkisp1 = isp->rkisp1; > > const struct rkisp1_sensor_async *asd; > > + struct media_pad *source_pad; > > + struct media_pad *sink_pad; > > int ret; > > > > if (!enable) { > > @@ -754,10 +742,18 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > > return 0; > > } > > > > - rkisp1->source = rkisp1_isp_get_source(sd); > > so 'rkisp1->source' is the source of the isp right? and in addition you later > add csi->source for the csi source. Maybe move rkisp1->source field to rkisp1_isp > to make it clear it is the isp source. > Just a suggestion. If you don't feel like, then: That's a very good idea. I'll add a patch on top to do this. > Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com> > > > + sink_pad = &isp->pads[RKISP1_ISP_PAD_SINK_VIDEO]; > > + source_pad = media_pad_remote_pad_unique(sink_pad); > > + if (IS_ERR(source_pad)) { > > + dev_dbg(rkisp1->dev, "Failed to get source for ISP: %ld\n", > > + PTR_ERR(source_pad)); > > + return -EPIPE; > > + } > > + > > + rkisp1->source = media_entity_to_v4l2_subdev(source_pad->entity); > > if (!rkisp1->source) { > > - dev_warn(rkisp1->dev, "No link between isp and source\n"); > > - return -ENODEV; > > + /* This should really not happen, so is not worth a message. */ > > + return -EPIPE; > > } > > > > asd = container_of(rkisp1->source->asd, struct rkisp1_sensor_async, -- Regards, Laurent Pinchart _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Dafna Hirschfeld <dafna@fastmail.com> Cc: linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org, Heiko Stuebner <heiko@sntech.de>, Helen Koike <helen.koike@collabora.com>, Paul Elder <paul.elder@ideasonboard.com> Subject: Re: [PATCH v3 38/46] media: rkisp1: isp: Disallow multiple active sources Date: Tue, 12 Jul 2022 22:18:27 +0300 [thread overview] Message-ID: <Ys3JA6bTN2TG6uMn@pendragon.ideasonboard.com> (raw) In-Reply-To: <20220712184005.nwnlflsupgie6owf@guri> Hi Dafna, On Tue, Jul 12, 2022 at 09:40:05PM +0300, Dafna Hirschfeld wrote: > On 11.07.2022 15:42, Laurent Pinchart wrote: > > The ISP supports multiple source subdevs, but can only capture from a > > single one at a time. The source is selected through link setup. The > > driver finds the active source in its .s_stream() handler using the > > media_entity_remote_pad() function. This fails to reject invalid > > configurations with multiple active sources. Fix it by using the > > media_pad_remote_pad_unique() helper instead, and inline > > rkisp1_isp_get_source() in rkisp1_isp_s_stream() as the function is > > small and has a single caller. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Changes since v2: > > > > - Update media_pad_remote_pad_unique() function name in commit message > > --- > > .../platform/rockchip/rkisp1/rkisp1-isp.c | 30 ++++++++----------- > > 1 file changed, 13 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > > index 37623b73b1d9..d7e2802d11f5 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > > @@ -58,20 +58,6 @@ > > * Helpers > > */ > > > > -static struct v4l2_subdev *rkisp1_isp_get_source(struct v4l2_subdev *sd) > > -{ > > - struct media_pad *local, *remote; > > - struct media_entity *sensor_me; > > - > > - local = &sd->entity.pads[RKISP1_ISP_PAD_SINK_VIDEO]; > > - remote = media_pad_remote_pad_first(local); > > - if (!remote) > > - return NULL; > > - > > - sensor_me = remote->entity; > > - return media_entity_to_v4l2_subdev(sensor_me); > > -} > > - > > static struct v4l2_mbus_framefmt * > > rkisp1_isp_get_pad_fmt(struct rkisp1_isp *isp, > > struct v4l2_subdev_state *sd_state, > > @@ -743,6 +729,8 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > > struct rkisp1_isp *isp = to_rkisp1_isp(sd); > > struct rkisp1_device *rkisp1 = isp->rkisp1; > > const struct rkisp1_sensor_async *asd; > > + struct media_pad *source_pad; > > + struct media_pad *sink_pad; > > int ret; > > > > if (!enable) { > > @@ -754,10 +742,18 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > > return 0; > > } > > > > - rkisp1->source = rkisp1_isp_get_source(sd); > > so 'rkisp1->source' is the source of the isp right? and in addition you later > add csi->source for the csi source. Maybe move rkisp1->source field to rkisp1_isp > to make it clear it is the isp source. > Just a suggestion. If you don't feel like, then: That's a very good idea. I'll add a patch on top to do this. > Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com> > > > + sink_pad = &isp->pads[RKISP1_ISP_PAD_SINK_VIDEO]; > > + source_pad = media_pad_remote_pad_unique(sink_pad); > > + if (IS_ERR(source_pad)) { > > + dev_dbg(rkisp1->dev, "Failed to get source for ISP: %ld\n", > > + PTR_ERR(source_pad)); > > + return -EPIPE; > > + } > > + > > + rkisp1->source = media_entity_to_v4l2_subdev(source_pad->entity); > > if (!rkisp1->source) { > > - dev_warn(rkisp1->dev, "No link between isp and source\n"); > > - return -ENODEV; > > + /* This should really not happen, so is not worth a message. */ > > + return -EPIPE; > > } > > > > asd = container_of(rkisp1->source->asd, struct rkisp1_sensor_async, -- Regards, Laurent Pinchart
next prev parent reply other threads:[~2022-07-12 19:19 UTC|newest] Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-11 12:42 [PATCH v3 00/46] media: rkisp1: Cleanups to prepare for i.MX8MP support Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 01/46] media: v4l2-async: Add notifier operation to destroy asd instances Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 02/46] media: mc-entity: Rename media_entity_remote_pad() to media_pad_remote_pad_first() Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-15 14:41 ` [PATCH v3.1 " Laurent Pinchart 2022-07-15 14:41 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 03/46] media: mc-entity: Add a new helper function to get a remote pad Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 04/46] media: mc-entity: Add a new helper function to get a remote pad for a pad Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 05/46] media: rkisp1: Enable compilation on ARCH_MXC Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 06/46] media: rkisp1: Disable runtime PM in probe error path Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 07/46] media: rkisp1: Read the ID register at probe time instead of streamon Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 08/46] media: rkisp1: Rename rkisp1_match_data to rkisp1_info Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 09/46] media: rkisp1: Save info pointer in rkisp1_device Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 10/46] media: rkisp1: Access ISP version from info pointer Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 11/46] media: rkisp1: Make rkisp1_isp_mbus_info common Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 12/46] media: rkisp1: cap: Print debug message on failed link validation Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 13/46] media: rkisp1: Move sensor .s_stream() call to ISP Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 14/46] media: rkisp1: Reject sensors without pixel rate control at bound time Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 15/46] media: rkisp1: Create link from sensor to ISP at notifier " Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 16/46] media: rkisp1: Create internal links at probe time Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 17/46] media: rkisp1: Rename rkisp1_subdev_notifier() to rkisp1_subdev_notifier_register() Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 18/46] media: rkisp1: Fix sensor source pad retrieval at bound time Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 19/46] media: rkisp1: Split CSI handling to separate file Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 20/46] media: rkisp1: isp: Start CSI-2 receiver before ISP Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 21/46] media: rkisp1: csi: Handle CSI-2 RX configuration fully in rkisp1-csi.c Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-16 19:01 ` Laurent Pinchart 2022-07-16 19:01 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 22/46] media: rkisp1: csi: Rename CSI functions with a common rkisp1_csi prefix Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 23/46] media: rkisp1: csi: Move start delay to rkisp1_csi_start() Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 24/46] media: rkisp1: csi: Pass sensor pointer to rkisp1_csi_config() Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 25/46] media: rkisp1: csi: Constify argument to rkisp1_csi_start() Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 26/46] media: rkisp1: isp: Don't initialize ret to 0 in rkisp1_isp_s_stream() Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 27/46] media: rkisp1: isp: Pass mbus type and flags to rkisp1_config_cif() Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 28/46] media: rkisp1: isp: Rename rkisp1_device.active_sensor to source Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 29/46] media: rkisp1: isp: Add container_of wrapper to cast subdev to rkisp1_isp Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 30/46] media: rkisp1: isp: Add rkisp1_device backpointer " Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 31/46] media: rkisp1: isp: Pass rkisp1_isp pointer to internal ISP functions Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 32/46] media: rkisp1: isp: Move input configuration to rkisp1_config_isp() Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 33/46] media: rkisp1: isp: Merge ISP_ACQ_PROP configuration in single variable Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 34/46] media: rkisp1: isp: Initialize some variables at declaration time Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 35/46] media: rkisp1: isp: Fix whitespace issues Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 36/46] media: rkisp1: isp: Constify various local variables Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 37/46] media: rkisp1: isp: Rename rkisp1_get_remote_source() Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 38/46] media: rkisp1: isp: Disallow multiple active sources Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-12 18:40 ` Dafna Hirschfeld 2022-07-12 18:40 ` Dafna Hirschfeld 2022-07-12 19:18 ` Laurent Pinchart [this message] 2022-07-12 19:18 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 39/46] media: rkisp1: csi: Implement a V4L2 subdev for the CSI receiver Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-12 18:31 ` Dafna Hirschfeld 2022-07-12 18:31 ` Dafna Hirschfeld 2022-07-11 12:42 ` [PATCH v3 40/46] media: rkisp1: csi: Plumb the CSI RX subdev Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 41/46] media: rkisp1: Use fwnode_graph_for_each_endpoint Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 42/46] dt-bindings: media: rkisp1: Add port for parallel interface Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 43/46] media: rkisp1: Support the ISP parallel input Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 44/46] media: rkisp1: Add infrastructure to support ISP features Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 16:37 ` Michael Riesch 2022-07-11 16:37 ` Michael Riesch 2022-07-11 16:50 ` Laurent Pinchart 2022-07-11 16:50 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 45/46] media: rkisp1: Make the internal CSI-2 receiver optional Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-11 12:42 ` [PATCH v3 46/46] media: rkisp1: debug: Add dump file in debugfs for MI main path registers Laurent Pinchart 2022-07-11 12:42 ` Laurent Pinchart 2022-07-12 18:42 ` Dafna Hirschfeld 2022-07-12 18:42 ` Dafna Hirschfeld
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=Ys3JA6bTN2TG6uMn@pendragon.ideasonboard.com \ --to=laurent.pinchart@ideasonboard.com \ --cc=dafna@fastmail.com \ --cc=heiko@sntech.de \ --cc=helen.koike@collabora.com \ --cc=linux-media@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=paul.elder@ideasonboard.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.