From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej2-f12.google.com (mail-ej2-f12.google.com [74.125.228.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 67D74448D00 for ; Fri, 11 Sep 2026 11:40:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.228.140 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789126837; cv=none; b=qyF3FQNeGb8DwLbU3KOO66ql3la9272Pb9QU82YKGOoxFuXTxK3yG/WBys5aYakWq5j0bspUpsURbxXpgsXvk5ODctdQO0ESbjuV+5w4IbccGr1bUmCrD/aWNvakuBZW/vhz0L1j/PHTs2VUCA+YkUWvRLPNtZHX/5AOl5+IsxU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789126837; c=relaxed/simple; bh=+yy7zkyplAsXUOfAB5SEcudaNXZlKAffdf+ehBYvb9g=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=dHsEEag+ikNQRLGPwZkW8S/UYf14KjanXAvEZXkaYtnR/DJNK7qP1CM3F64soDwKQIpMA8hJ5Aqe7KoF2uoLMG4Qjj4CaaEdKgMDsfIJH7oRhvqG1jgoj+apyZ1XRP/sE7d/RfhMkWYZM7Pq98yDlRdVSydOqTmf/KQKwCWEKQ4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=uFjEQ0RM; arc=none smtp.client-ip=74.125.228.140 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="uFjEQ0RM" Received: by mail-ej2-f12.google.com with SMTP id a640c23a62f3a-c254f9f0b20so134929766b.3 for ; Fri, 11 Sep 2026 04:40:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1789126832; x=1789731632; darn=vger.kernel.org; h=content-transfer-encoding:content-type:in-reply-to:content-language :from:references:cc:to:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to:content-type; bh=Ym4xpArbzkJOXp9qnGSR8FW0V7EguEQKezXI/VRiW1k=; b=uFjEQ0RMmN26zVQVWcA9uRyoRnOZBuXd2tvLBAUIth/o51Ytu6y7Zyeg5RAJJ8v1wI K39hPH+YvS8kBNdqyefJunp01lAh5W4T4h023pSLzBAt5q2Y60JC90g/9bIKBhMaX1WF +/FpSf3V5/6fRsaLVC6iDPu4F7EZvcl+1dA+zn2AaIVvn48g0ue9cgnGIUevG3MbFbzU nVQh38RVd2bS7r5CiEA1O56ztvB7Yp88rG98xf19lm9Ghhb1KN9czxz4mpWZIlsDtR+A NDYjS7uKLAIetZL5/nMZYvHFzXcO+WfSf2sA/uJmP8if1KIDYn+1+3g2tWnFJG4Fqo7V 56hg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1789126832; x=1789731632; h=content-transfer-encoding:content-type:in-reply-to:content-language :from:references:cc:to:subject:user-agent:mime-version:date :message-id:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=Ym4xpArbzkJOXp9qnGSR8FW0V7EguEQKezXI/VRiW1k=; b=cCOhkC+WQzjDmsP+Z/cC6pU/gpBOiddMdQNhY/lctiQ6LQbWb7XAaZGhczFniCxfgS EgiuAIVmEIs3pqaag6lnkON/lnygkxluqbI3yklEvrbrrg4Ay50j/DLUDqW2J7zrzdW6 FbjPieH7Dn/t/blkHDo/2SlLiCDhtIW6TWv6LUWYxn0zdLoNr47zz/+AZ32QiKWvK8u/ ESl4DULcHlPjl4/yo08n9b7s0TfhDhoDRq0gTkYK/pArcg/lLXFoXt0ETIMOVYnlIqNo XSLlYmtDqFnmGTuMvbtSsZICE0jolnEPHYc0oYLrclvL54zqr0GoixOq3YxhisfX00xX i4wA== X-Forwarded-Encrypted: i=1; AKwUvBw9Ja/BYCCMQszugaILj9c8i87xFtBjq62q3zZDhkH7kzHftxBKoO2ZFljcQo5p+woOcBswHzEvDE4/Bg==@vger.kernel.org X-Gm-Message-State: AFuF++lV95y9LwiX2Ovuf5Cr2P2SQauBos1+OCI9IQlBQWInHCbGbhjl 54IMpXSQY1JJQ1cUxQ4J53HJ6nH+ES80QT/ptKKYdVaUPwsYNUlyjZR0jxKtbx5QQn+/Ipp8mva dvZC4 X-Gm-Gg: AYBFou3f3m+bx5zAtxzPuMcZk0/G9oeA+8yqO5/lRzYz/KFFzjye/tkdKqxUAdxxT+E KIRTByNE5GZWKtNEguBWZNTolIOVsQTfs5KMapE47U4KuA6jfMRGqdH+exxw++oF5OMfLn2+zBv dSE5Cguxj0Ai9CCkSQjBqM8Vz3HNwVNTbyiLpseMa1fEjptkVfoC6g4qZsiCFHm9xKfHCfkG/N3 7fDMZKZpzXuIjYfWEX9UmN8KC8RaAQDqDU0SmyZyb/p3hE7QEtcBGnUqZ2A8vzSGkfSKC7wqR9s qyGwMpl7TQetwYmqUkzJBPBQzo+ba/qHFvsRsh4cHUUmxRi3tUr2pQ0jqAs9tfbp5tf2HzHUpjx b5vTfxpEfsLEquJwFd5H7Bgfr5tomtxhTrZWJ7f620VqomUDPLMOTDrlIHeBhadKGs+uAWqdicU I0BZT0ltLwQz6ulzFBEIVbESsibM9WOuvMrVo7vqbzcuk2uVUUGS1KExH6QfI6pMGnChEqxICdq relbf4ihWjYAQ== X-Received: by 2002:a17:907:9619:b0:c25:8e53:45ca with SMTP id a640c23a62f3a-c29667578e3mr304602466b.21.1789126502728; Fri, 11 Sep 2026 04:35:02 -0700 (PDT) Received: from [192.168.0.167] ([109.76.225.241]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-c296613ae93sm68275066b.58.2026.09.11.04.35.01 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 11 Sep 2026 04:35:02 -0700 (PDT) Message-ID: Date: Fri, 11 Sep 2026 12:35:01 +0100 Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 4/8] media: qcom: camss: Add streams API support in CSID subdevice To: Gjorgji.Rosikopulos.gjorgji.rosikopulos@oss.qualcomm.com, Mauro Carvalho Chehab Cc: Vladimir Zapolskiy , Loic Poulain , Dmitry Baryshkov , Atanas Filipov , Jigarkumar Zala , linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, Gjorgji Rosikopulos References: <20260911062213.195007-1-gjorgji.rosikopulos@oss.qualcomm.com> <20260911062213.195007-5-gjorgji.rosikopulos@oss.qualcomm.com> From: Bryan O'Donoghue Content-Language: en-GB In-Reply-To: <20260911062213.195007-5-gjorgji.rosikopulos@oss.qualcomm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 11/09/2026 07:22, Gjorgji.Rosikopulos.gjorgji.rosikopulos@oss.qualcomm.com wrote: > From: Gjorgji Rosikopulos > > Add the V4L2 subdev streams API to the CSID driver: per-source-pad > enable/disable_streams pad ops, VC/DT discovery via get_frame_desc on > the remote sink pad, and a routing table that routes a single incoming > sink stream to every source pad by default (remappable via > set_routing for multi-VC sensors). > > Active streams are tracked per pad via a per-pad enabled_streams[] > bitmask, so that enable/disable_streams correctly propagates to the > CSIPHY only on first arrival/last departure of a sink stream, and > multiple source-pad consumers can share a single propagated sink > stream without redundant or colliding propagation. > > csid_init_state() caps the number of default routes to > MSM_CSID_MAX_SRC_STREAMS - 1 for non-lite CSIDs, matching the 3 usable > RDI pads on full-IFE CSIDs (the 4th/pix pad is non-functional). That makes sense. > msm_csid_register_entity() mirrors the same is_lite check for the pad > count itself, so non-lite CSIDs no longer register a pix source pad > that no route ever targets. Nope. I'll send a different solution. Fixing the non-functional and incorrectly mapped pix is a Fixes: level thing not a workaround it inline thing. Just drop the workaround and assume any CSID you are working with actually works and is valid. > csid_pad_enable_streams() rejects enabling with -ENOLINK when there is > no remote sink link and the test generator is disabled, matching the > equivalent check the legacy csid_set_stream() already performs. > > msm_csid_register_entity() assigns the ctrl handler's lock as the > subdev's state_lock before v4l2_subdev_init_finalize(), when the test > pattern control is present. Without this, the test-pattern S_CTRL > handler and the streams API's active-state accessors serialize on two > independent locks despite both touching csid->testgen.enabled, > allowing a concurrent S_CTRL(TEST_PATTERN) and stream enable/disable to > race. Actually this reminds me of the first go at VCs in CAMSS which ended up getting rolled back. How will this be tested ? Is the TPG capable of generating different VCs ? _That_ would be very useful. > > This is opt-in per CSID instance via the new streams_enable resource > flag; no platform sets it yet, so CSIDs continue to use the legacy > non-streams subdev ops unchanged. > > Signed-off-by: Gjorgji Rosikopulos > --- > .../media/platform/qcom/camss/camss-csid.c | 494 +++++++++++++++++- > .../media/platform/qcom/camss/camss-csid.h | 2 + > 2 files changed, 491 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c > index 48459b46a981..ce4b07c0c1c2 100644 > --- a/drivers/media/platform/qcom/camss/camss-csid.c > +++ b/drivers/media/platform/qcom/camss/camss-csid.c > @@ -842,7 +842,7 @@ static void csid_try_format(struct csid_device *csid, > > break; > > - case MSM_CSID_PAD_SRC: > + default: Why subtract the case ? Just add the default: Also I don't think that code change is anything todo with adding streams support to CSID. Separate patches for separate things. > if (csid->testgen.nmodes == CSID_PAYLOAD_MODE_DISABLED || > csid->testgen_mode->cur.val == 0) { > /* Test generator is disabled, */ > @@ -1338,10 +1338,476 @@ static const struct v4l2_subdev_ops csid_v4l2_ops = { > .pad = &csid_pad_ops, > }; > > +/* > + * csid_get_stream_csi2_desc - Discover the virtual channel and data type > + * used by a given sink stream, from an > + * already-fetched frame descriptor > + * @frame_desc: Frame descriptor fetched via .get_frame_desc from the remote > + * subdev linked on the sink pad > + * @sink_stream: Sink-side stream number to look up > + * @desc_csi2: Returns the discovered virtual channel/data type on success > + * > + * A frame descriptor with a single entry means the remote only exposes one > + * stream (e.g. a single-VC sensor), which feeds every CSID source pad, so > + * that entry is used regardless of @sink_stream. > + * > + * Return true if a matching entry was found, false otherwise > + */ > +static bool csid_get_stream_csi2_desc(struct v4l2_mbus_frame_desc *frame_desc, > + u32 sink_stream, > + struct v4l2_mbus_frame_desc_entry_csi2 *desc_csi2) > +{ > + unsigned int i; > + > + if (frame_desc->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2 || !frame_desc->num_entries) > + return false; > + > + if (frame_desc->num_entries == 1) { > + *desc_csi2 = frame_desc->entry[0].bus.csi2; > + return true; > + } You can drop that check entirely, the below loop will do exactly the same thing for num_entires == 1.. > + > + for (i = 0; i < frame_desc->num_entries; i++) { > + if (frame_desc->entry[i].stream == sink_stream) { > + *desc_csi2 = frame_desc->entry[i].bus.csi2; > + return true; > + } > + } > + > + return false; > +} > + > +/* > + * csid_get_stream_vc_dt - Discover the virtual channel and data type to > + * program for a given sink pad/stream, falling back > + * to @format_dt when no frame descriptor is available > + * @csid: CSID device > + * @state: V4L2 subdevice state > + * @remote_pad: Remote pad linked on the CSID sink pad, or NULL if unlinked > + * @pad: Source pad number the caller is enabling a stream on > + * @format_dt: Data type derived from the sink format, used as a fallback > + * and sanity-checked against the discovered data type > + * > + * Return the discovered virtual channel/data type, or {0, @format_dt} if > + * not discovered > + */ > +static struct v4l2_mbus_frame_desc_entry_csi2 > +csid_get_stream_vc_dt(struct csid_device *csid, struct v4l2_subdev_state *state, > + struct media_pad *remote_pad, u32 pad, u8 format_dt) > +{ > + struct v4l2_mbus_frame_desc_entry_csi2 desc_csi2 = { .dt = format_dt }; > + struct v4l2_mbus_frame_desc fd = { }; > + u32 sink_stream; > + > + if (!remote_pad || > + v4l2_subdev_call(media_entity_to_v4l2_subdev(remote_pad->entity), > + pad, get_frame_desc, remote_pad->index, &fd)) > + return desc_csi2; > + if (thing || some_other_thing) { return desc_csi2; } > + if (v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0, NULL, &sink_stream)) > + return desc_csi2; > + > + if (!csid_get_stream_csi2_desc(&fd, sink_stream, &desc_csi2)) { > + dev_warn(csid->camss->dev, > + "Failed to find CSI2 descriptor for sink stream %u, using vc=%u dt=%u\n", > + sink_stream, desc_csi2.vc, desc_csi2.dt); > + return desc_csi2; Is this an error it seems like it should be ? > + } > + > + if (desc_csi2.dt != format_dt) > + dev_warn(csid->camss->dev, > + "Sink stream %u frame desc dt=%u differs from format dt=%u, using dt=%u\n", > + sink_stream, desc_csi2.dt, format_dt, desc_csi2.dt); > + > + return desc_csi2; I'd return a pointer here. > +} > + > +/* > + * csid_pad_enable_streams - Enable one or more streams on a source pad > + * @sd: CSID V4L2 subdevice > + * @state: V4L2 subdevice state > + * @pad: Pad number > + * @streams_mask: Bitmask of v4l2 streams to enable > + * > + * The v4l2 core only calls this on a source pad (v4l2_subdev_enable_streams() > + * rejects sink pads with -EOPNOTSUPP before reaching the driver), so @pad is > + * not checked here. Each source pad only ever carries stream 0. > + * > + * The shared sink stream(s) are propagated upstream only once, on the > + * transition from no active sink streams to at least one, so that a second > + * consumer of the same shared sink stream never triggers a second, redundant > + * propagation to the sensor. The Rx front-end is likewise only configured > + * once, on that same transition. > + * > + * Return 0 on success, -ENOLINK if there is no remote sink link and the test > + * generator is disabled, or another negative error code otherwise > + */ > +static int csid_pad_enable_streams(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + u32 pad, u64 streams_mask) > +{ > + struct csid_device *csid = v4l2_get_subdevdata(sd); > + const struct csid_hw_ops *hw_ops = csid->res->hw_ops; > + struct media_pad *remote_pad = media_pad_remote_pad_first(&csid->pads[MSM_CSID_PAD_SINK]); > + unsigned int hw_port = pad - MSM_CSID_PAD_FIRST_SRC; > + const struct csid_format_info *format; > + struct v4l2_mbus_frame_desc_entry_csi2 desc_csi2; > + u64 sink_streams, propagate_mask; > + int ret; > + > + if (!csid->testgen.enabled && !remote_pad) > + return -ENOLINK; > + > + sink_streams = v4l2_subdev_state_xlate_streams(state, pad, MSM_CSID_PAD_SINK, > + &streams_mask); > + > + if (!csid->enabled_streams[MSM_CSID_PAD_SINK]) { > + if (csid->testgen.nmodes != CSID_PAYLOAD_MODE_DISABLED) { > + /* > + * sd->state_lock is aliased to csid->ctrls.lock, and is > + * already held here by the v4l2_subdev_enable_streams() > + * caller, so use the lock-free variant to avoid > + * self-deadlocking on the same mutex. > + */ > + ret = __v4l2_ctrl_handler_setup(&csid->ctrls); > + if (ret < 0) { > + dev_err(csid->camss->dev, > + "could not sync v4l2 controls: %d\n", ret); > + return ret; > + } > + } > + > + hw_ops->configure_rx(csid); > + } > + > + /* Sink streams already active elsewhere don't need re-propagating. */ > + propagate_mask = sink_streams & ~csid->enabled_streams[MSM_CSID_PAD_SINK]; > + csid->enabled_streams[MSM_CSID_PAD_SINK] |= sink_streams; > + csid->enabled_streams[pad] |= streams_mask; > + > + format = csid_get_fmt_entry(csid->res->formats->formats, > + csid->res->formats->nformats, > + csid->fmt[pad].code); > + desc_csi2 = csid_get_stream_vc_dt(csid, state, remote_pad, pad, format->data_type); You're doing an implict memcpy() here - just return a pointer. And I really wonder based on the activity within the method why returning an error isn't part of this ? > + > + hw_ops->enable_stream(csid, hw_port, desc_csi2.vc, desc_csi2.dt); I commented elsewhere should this be void or int ? I'm not suggesting either more asking rhetorically. > + > + if (propagate_mask && remote_pad) { > + ret = v4l2_subdev_enable_streams(media_entity_to_v4l2_subdev(remote_pad->entity), > + remote_pad->index, propagate_mask); > + if (ret) { > + csid->enabled_streams[MSM_CSID_PAD_SINK] &= ~propagate_mask; > + csid->enabled_streams[pad] &= ~streams_mask; > + > + hw_ops->disable_stream(csid, hw_port); > + > + return ret; > + } > + } > + > + return 0; > +} > + > +/* > + * csid_sink_streams_in_use - Compute the subset of sink streams still > + * referenced by a source pad other than @pad > + * @csid: CSID device > + * @state: V4L2 subdevice state > + * @pad: Source pad to exclude from the check > + * @sink_streams: Candidate sink streams to check > + * > + * Return the subset of @sink_streams still referenced by some other source > + * pad > + */ > +static u64 csid_sink_streams_in_use(struct csid_device *csid, struct v4l2_subdev_state *state, > + u32 pad, u64 sink_streams) > +{ > + u64 in_use = 0; > + unsigned int i; > + > + for (i = MSM_CSID_PAD_FIRST_SRC; i < MSM_CSID_PADS_NUM; i++) { > + u64 other_streams = csid->enabled_streams[i]; > + u64 other_sink_streams; > + > + if (i == pad) > + continue; > + > + other_sink_streams = v4l2_subdev_state_xlate_streams(state, i, MSM_CSID_PAD_SINK, > + &other_streams); > + in_use |= sink_streams & other_sink_streams; > + } > + > + return in_use; > +} > + > +/* > + * csid_pad_disable_streams - Disable one or more streams on a source pad > + * @sd: CSID V4L2 subdevice > + * @state: V4L2 subdevice state > + * @pad: Pad number > + * @streams_mask: Bitmask of v4l2 streams to disable > + * > + * The v4l2 core only calls this on a source pad (v4l2_subdev_disable_streams() > + * rejects sink pads with -EOPNOTSUPP before reaching the driver), so @pad is > + * not checked here. Each source pad only ever carries stream 0. > + * > + * A sink stream is only disabled, and propagated upstream to disable it there > + * too, once no source pad references it any more. > + * > + * Return 0 on success or a negative error code otherwise > + */ > +static int csid_pad_disable_streams(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + u32 pad, u64 streams_mask) > +{ > + struct csid_device *csid = v4l2_get_subdevdata(sd); > + const struct csid_hw_ops *hw_ops = csid->res->hw_ops; > + struct media_pad *remote_pad = media_pad_remote_pad_first(&csid->pads[MSM_CSID_PAD_SINK]); > + unsigned int hw_port = pad - MSM_CSID_PAD_FIRST_SRC; > + u64 sink_streams, disable_sink_streams; > + int ret = 0; > + > + sink_streams = v4l2_subdev_state_xlate_streams(state, pad, MSM_CSID_PAD_SINK, > + &streams_mask); > + > + /* Keep a sink stream active as long as any other source pad still uses it. */ > + disable_sink_streams = sink_streams & > + ~csid_sink_streams_in_use(csid, state, pad, sink_streams); > + > + if (disable_sink_streams && remote_pad) { > + ret = v4l2_subdev_disable_streams(media_entity_to_v4l2_subdev(remote_pad->entity), > + remote_pad->index, disable_sink_streams); > + if (ret) > + dev_err(csid->camss->dev, > + "Failed to disable stream on remote pad: %d\n", ret); > + } > + > + hw_ops->disable_stream(csid, hw_port); > + > + csid->enabled_streams[pad] &= ~streams_mask; > + csid->enabled_streams[MSM_CSID_PAD_SINK] &= ~disable_sink_streams; > + > + return ret; > +} > + > +static const struct v4l2_mbus_framefmt csid_default_format = { > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > + .width = 1920, > + .height = 1080, > + .field = V4L2_FIELD_NONE, > + .colorspace = V4L2_COLORSPACE_SRGB, > +}; > + > +/* > + * csid_set_routing - Handle setting of routing table > + * @sd: CSID V4L2 subdevice > + * @state: V4L2 subdevice state > + * @which: TRY or ACTIVE routing > + * @routing: Routing table to set > + * > + * Return 0 on success or a negative error code otherwise > + */ > +static int csid_set_routing(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + enum v4l2_subdev_format_whence which, > + struct v4l2_subdev_krouting *routing) > +{ > + struct csid_device *csid = v4l2_get_subdevdata(sd); > + unsigned int i; > + int ret; > + > + if (which == V4L2_SUBDEV_FORMAT_ACTIVE && csid->enabled_streams[MSM_CSID_PAD_SINK]) > + return -EBUSY; > + > + for (i = 0; i < routing->num_routes; i++) > + if (routing->routes[i].source_stream != 0) > + return -EINVAL; > + > + ret = v4l2_subdev_routing_validate(sd, routing, > + V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX | > + V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING | > + V4L2_SUBDEV_ROUTING_NO_N_TO_1); > + if (ret) > + return ret; > + > + return v4l2_subdev_set_routing_with_fmt(sd, state, routing, &csid_default_format); > +} > + > +/* > + * __csid_get_stream_format - Get pointer to per-stream format structure > + * @csid: CSID device > + * @sd_state: V4L2 subdev state > + * @pad: pad from which format is requested > + * @stream: stream from which format is requested > + * @which: TRY or ACTIVE format > + * > + * Same as __csid_get_format(), but honors @stream for TRY-state lookups. > + * For ACTIVE state, csid->fmt[] is indexed by pad + stream. @stream is > + * always 0 and @pad selects the RDI channel (0-3). > + * > + * Return pointer to TRY or ACTIVE format structure > + */ > +static struct v4l2_mbus_framefmt * > +__csid_get_stream_format(struct csid_device *csid, > + struct v4l2_subdev_state *sd_state, > + unsigned int pad, u32 stream, > + enum v4l2_subdev_format_whence which) > +{ > + if (which == V4L2_SUBDEV_FORMAT_TRY) > + return v4l2_subdev_state_get_format(sd_state, pad, stream); > + > + if (pad == MSM_CSID_PAD_SINK) > + return &csid->fmt[MSM_CSID_PAD_SINK]; > + > + return &csid->fmt[pad + stream]; > +} > + > +/* > + * csid_streams_get_format - Handle get format by pads subdev method > + * @sd: CSID V4L2 subdevice > + * @sd_state: V4L2 subdev state > + * @fmt: pointer to v4l2 subdev format structure > + * > + * Return -EINVAL or zero on success > + */ > +static int csid_streams_get_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_format *fmt) > +{ > + struct csid_device *csid = v4l2_get_subdevdata(sd); > + struct v4l2_mbus_framefmt *format; > + > + format = __csid_get_stream_format(csid, sd_state, fmt->pad, fmt->stream, fmt->which); > + if (!format) > + return -EINVAL; > + > + fmt->format = *format; > + > + return 0; > +} > + > +/* > + * csid_streams_set_format - Handle set format by pads subdev method > + * @sd: CSID V4L2 subdevice > + * @sd_state: V4L2 subdev state > + * @fmt: pointer to v4l2 subdev format structure > + * > + * Return -EINVAL or zero on success > + */ > +static int csid_streams_set_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_format *fmt) > +{ > + struct csid_device *csid = v4l2_get_subdevdata(sd); > + struct v4l2_mbus_framefmt *format; > + struct v4l2_subdev_route *route; > + > + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && csid->enabled_streams[MSM_CSID_PAD_SINK]) > + return -EBUSY; > + > + format = __csid_get_stream_format(csid, sd_state, fmt->pad, fmt->stream, fmt->which); > + if (!format) > + return -EINVAL; > + > + csid_try_format(csid, sd_state, fmt->pad, &fmt->format, fmt->which); > + *format = fmt->format; > + > + /* Propagate the format from the sink stream to every source stream it feeds */ > + for_each_active_route(&sd_state->routing, route) { > + struct v4l2_mbus_framefmt *src_format; > + > + if (route->sink_pad != fmt->pad || route->sink_stream != fmt->stream) > + continue; > + > + src_format = __csid_get_stream_format(csid, sd_state, route->source_pad, > + route->source_stream, fmt->which); > + if (!src_format) > + continue; > + > + *src_format = fmt->format; > + csid_try_format(csid, sd_state, route->source_pad, src_format, fmt->which); > + } > + > + return 0; > +} Why do we need a full new set of get-format and set-format ? "Feels" like this could wrapper the existing code. > + > +static const struct v4l2_subdev_pad_ops csid_streams_pad_ops = { > + .enum_mbus_code = csid_enum_mbus_code, > + .enum_frame_size = csid_enum_frame_size, > + .get_fmt = csid_streams_get_format, > + .set_fmt = csid_streams_set_format, > + .set_routing = csid_set_routing, > + .enable_streams = csid_pad_enable_streams, > + .disable_streams = csid_pad_disable_streams, > +}; > + > +static const struct v4l2_subdev_video_ops csid_streams_video_ops = { > + .s_stream = v4l2_subdev_s_stream_helper, > +}; > + > +static const struct v4l2_subdev_ops csid_streams_v4l2_ops = { > + .core = &csid_core_ops, > + .pad = &csid_streams_pad_ops, > + .video = &csid_streams_video_ops, > +}; > + > +/* > + * csid_init_state - Initialize the routing table for the streams API subdev > + * @sd: CSID V4L2 subdevice > + * @state: V4L2 subdev state > + * > + * source_stream is always 0: each source pad MSM_CSID_PAD_FIRST_SRC + i > + * links to its own independent downstream subdev, and a link's sink side is > + * validated against the implicit stream 0 exposed by any subdev without > + * V4L2_SUBDEV_FL_STREAMS (see v4l2_link_validate_get_streams()) — every > + * downstream VFE line is such a subdev. > + * > + * All source pads route from sink_stream 0 by default, fanning the single > + * incoming stream out to every port; a multi-VC source is supported by > + * remapping each route's sink_stream via .set_routing, leaving > + * source_pad/source_stream untouched. > + * > + * Return 0 on success or a negative error code otherwise > + */ > +static int csid_init_state(struct v4l2_subdev *sd, struct v4l2_subdev_state *state) > +{ > + struct csid_device *csid = v4l2_get_subdevdata(sd); > + struct v4l2_subdev_route routes[MSM_CSID_MAX_SRC_STREAMS]; > + struct v4l2_subdev_krouting routing = { }; > + unsigned int num_routes; > + int i, ret; > + > + /* The full IFE has only 3 rdi's and pix output is not functional */ > + if (csid_is_lite(csid)) > + num_routes = MSM_CSID_MAX_SRC_STREAMS; > + else > + num_routes = MSM_CSID_MAX_SRC_STREAMS - 1; No. Don't code around this here. I'll make a Fixes: patch for the pix stuff - I have it in tree. I don't want to add work-arounds in code. > + > + for (i = 0; i < num_routes; i++) { > + routes[i].sink_pad = MSM_CSID_PAD_SINK; > + routes[i].sink_stream = 0; > + routes[i].source_pad = MSM_CSID_PAD_FIRST_SRC + i; > + routes[i].source_stream = 0; > + routes[i].flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE; > + } > + > + routing.num_routes = num_routes; > + routing.routes = routes; > + ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing, &csid_default_format); > + if (ret) > + dev_err(csid->camss->dev, "Failed to set routing: %d\n", ret); > + > + return ret; > +} > + > static const struct v4l2_subdev_internal_ops csid_v4l2_internal_ops = { > .open = csid_init_formats, > }; > > +static const struct v4l2_subdev_internal_ops csid_streams_internal_ops = { > + .init_state = csid_init_state, > +}; > + > static const struct media_entity_operations csid_media_ops = { > .link_setup = csid_link_setup, > .link_validate = v4l2_subdev_link_validate, > @@ -1360,13 +1826,18 @@ int msm_csid_register_entity(struct csid_device *csid, > struct v4l2_subdev *sd = &csid->subdev; > struct media_pad *pads = csid->pads; > struct device *dev = csid->camss->dev; > + bool streams_api = csid->res->streams_enable; As I stated elsewhere there's no need to have this flag copy/pasted. Just move it one level up does this SoC support streams, in camss->supports_streams; > + unsigned int num_pads = csid_is_lite(csid) ? MSM_CSID_PADS_NUM : MSM_CSID_PADS_NUM - 1; > int i; > int ret; > > - v4l2_subdev_init(sd, &csid_v4l2_ops); > - sd->internal_ops = &csid_v4l2_internal_ops; > + v4l2_subdev_init(sd, streams_api ? &csid_streams_v4l2_ops : &csid_v4l2_ops); > + sd->internal_ops = streams_api ? &csid_streams_internal_ops > + : &csid_v4l2_internal_ops; > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > V4L2_SUBDEV_FL_HAS_EVENTS; > + if (streams_api) > + sd->flags |= V4L2_SUBDEV_FL_STREAMS; > snprintf(sd->name, ARRAY_SIZE(sd->name), "%s%d", > MSM_CSID_NAME, csid->id); > v4l2_set_subdevdata(sd, csid); > @@ -1400,17 +1871,28 @@ int msm_csid_register_entity(struct csid_device *csid, > } > > pads[MSM_CSID_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > - for (i = MSM_CSID_PAD_FIRST_SRC; i < MSM_CSID_PADS_NUM; ++i) > + for (i = MSM_CSID_PAD_FIRST_SRC; i < num_pads; ++i) > pads[i].flags = MEDIA_PAD_FL_SOURCE; > > sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; > sd->entity.ops = &csid_media_ops; > - ret = media_entity_pads_init(&sd->entity, MSM_CSID_PADS_NUM, pads); > + ret = media_entity_pads_init(&sd->entity, num_pads, pads); > if (ret < 0) { > dev_err(dev, "Failed to init media entity: %d\n", ret); > goto free_ctrl; > } > > + if (streams_api) { > + if (csid->testgen.nmodes != CSID_PAYLOAD_MODE_DISABLED) > + sd->state_lock = csid->ctrls.lock; > + > + ret = v4l2_subdev_init_finalize(sd); > + if (ret) { > + dev_err(dev, "Failed to finalize subdev: %d\n", ret); > + goto media_cleanup; > + } > + } > + > ret = v4l2_device_register_subdev(v4l2_dev, sd); > if (ret < 0) { > dev_err(dev, "Failed to register subdev: %d\n", ret); > @@ -1420,6 +1902,7 @@ int msm_csid_register_entity(struct csid_device *csid, > return 0; > > media_cleanup: > + v4l2_subdev_cleanup(sd); > media_entity_cleanup(&sd->entity); > free_ctrl: > if (csid->testgen.nmodes != CSID_PAYLOAD_MODE_DISABLED) > @@ -1435,6 +1918,7 @@ int msm_csid_register_entity(struct csid_device *csid, > void msm_csid_unregister_entity(struct csid_device *csid) > { > v4l2_device_unregister_subdev(&csid->subdev); > + v4l2_subdev_cleanup(&csid->subdev); > media_entity_cleanup(&csid->subdev.entity); > if (csid->testgen.nmodes != CSID_PAYLOAD_MODE_DISABLED) > v4l2_ctrl_handler_free(&csid->ctrls); > diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h > index 90ee611b9092..a312103cf86d 100644 > --- a/drivers/media/platform/qcom/camss/camss-csid.h > +++ b/drivers/media/platform/qcom/camss/camss-csid.h > @@ -184,6 +184,7 @@ struct csid_hw_ops { > > struct csid_subdev_resources { > bool is_lite; > + bool streams_enable; > const struct csid_hw_ops *hw_ops; > const struct parent_dev_ops *parent_dev_ops; > const struct csid_formats *formats; > @@ -209,6 +210,7 @@ struct csid_device { > struct v4l2_mbus_framefmt fmt[MSM_CSID_PADS_NUM]; > struct v4l2_ctrl_handler ctrls; > struct v4l2_ctrl *testgen_mode; > + u64 enabled_streams[MSM_CSID_PADS_NUM]; > const struct csid_subdev_resources *res; > }; >