[PATCH 41/55] media: rkisp1: csi: Plumb the CSI RX subdev
Dafna Hirschfeld
dafna at fastmail.com
Sat Jun 25 00:45:59 PDT 2022
On 15.06.2022 04:11, Paul Elder wrote:
>From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>Connect the CSI receiver subdevice into the rest of the driver. This
>includes:
To make it clear where you put the csi, i'll add here
'Connect the CSI subdevice between the source
and the isp' or something like that.
Also the sketch 'Media Topology' in rkisp1-dev.c should be updated.
I'd also wonder if we should ignore src configuration of the isp entity
since it must be identical to the sink of the csi.
>- calling the subdevice via the v4l2 subdev API
>- moving the async notifier for the sensor from the ISP to the CSI
> receiver
>- in the ISP, create a media link to the CSI receiver, and remove the
> media link creation to the sensor
>- in the CSI receiver, create a media link to the sensor
>
>Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>---
> .../platform/rockchip/rkisp1/rkisp1-csi.c | 34 ++++++++++++++++--
> .../platform/rockchip/rkisp1/rkisp1-csi.h | 6 ++--
> .../platform/rockchip/rkisp1/rkisp1-dev.c | 36 +++++++++----------
> .../platform/rockchip/rkisp1/rkisp1-isp.c | 21 ++---------
> 4 files changed, 53 insertions(+), 44 deletions(-)
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>index 8182694a6fe0..96712b467dde 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>@@ -43,6 +43,34 @@ rkisp1_csi_get_pad_fmt(struct rkisp1_csi *csi,
> return v4l2_subdev_get_try_format(&csi->sd, &state, pad);
> }
>
>+int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
>+ struct rkisp1_sensor_async *s_asd,
>+ unsigned int source_pad)
>+{
>+ struct rkisp1_csi *csi = &rkisp1->csi;
>+ int ret;
extra space after 'ret;' ?
>+
>+ s_asd->pixel_rate_ctrl = v4l2_ctrl_find(sd->ctrl_handler,
>+ V4L2_CID_PIXEL_RATE);
>+ if (!s_asd->pixel_rate_ctrl) {
>+ dev_err(rkisp1->dev, "No pixel rate control in subdev %s\n",
>+ sd->name);
>+ return -EINVAL;
>+ }
>+
>+ /* Create the link from the sensor to the CSI receiver. */
>+ ret = media_create_pad_link(&sd->entity, source_pad,
>+ &csi->sd.entity, RKISP1_CSI_PAD_SINK,
>+ !s_asd->index ? MEDIA_LNK_FL_ENABLED : 0);
>+ if (ret) {
>+ dev_err(csi->rkisp1->dev, "failed to link src pad of %s\n",
>+ sd->name);
>+ return ret;
>+ }
>+
>+ return 0;
>+}
>+
> static int rkisp1_csi_config(struct rkisp1_csi *csi,
> const struct rkisp1_sensor_async *sensor)
> {
>@@ -118,8 +146,8 @@ static void rkisp1_csi_disable(struct rkisp1_csi *csi)
> val & (~RKISP1_CIF_MIPI_CTRL_OUTPUT_ENA));
> }
>
>-int rkisp1_csi_start(struct rkisp1_csi *csi,
>- const struct rkisp1_sensor_async *sensor)
>+static int rkisp1_csi_start(struct rkisp1_csi *csi,
>+ const struct rkisp1_sensor_async *sensor)
> {
> struct rkisp1_device *rkisp1 = csi->rkisp1;
> union phy_configure_opts opts;
>@@ -155,7 +183,7 @@ int rkisp1_csi_start(struct rkisp1_csi *csi,
> return 0;
> }
>
>-void rkisp1_csi_stop(struct rkisp1_csi *csi)
>+static void rkisp1_csi_stop(struct rkisp1_csi *csi)
> {
> rkisp1_csi_disable(csi);
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
>index ddf8e5e08f55..eadcd24f65fb 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
>@@ -21,8 +21,8 @@ void rkisp1_csi_cleanup(struct rkisp1_device *rkisp1);
> int rkisp1_csi_register(struct rkisp1_device *rkisp1);
> void rkisp1_csi_unregister(struct rkisp1_device *rkisp1);
>
>-int rkisp1_csi_start(struct rkisp1_csi *csi,
>- const struct rkisp1_sensor_async *sensor);
>-void rkisp1_csi_stop(struct rkisp1_csi *csi);
>+int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
>+ struct rkisp1_sensor_async *s_asd,
>+ unsigned int source_pad);
>
> #endif /* _RKISP1_CSI_H */
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>index faf2cd4c8149..a3e182c86bdd 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>@@ -17,6 +17,7 @@
> #include <linux/pinctrl/consumer.h>
> #include <linux/pm_runtime.h>
> #include <media/v4l2-fwnode.h>
>+#include <media/v4l2-mc.h>
>
> #include "rkisp1-common.h"
> #include "rkisp1-csi.h"
>@@ -119,17 +120,8 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> container_of(asd, struct rkisp1_sensor_async, asd);
> int source_pad;
>
>- s_asd->pixel_rate_ctrl = v4l2_ctrl_find(sd->ctrl_handler,
>- V4L2_CID_PIXEL_RATE);
>- if (!s_asd->pixel_rate_ctrl) {
>- dev_err(rkisp1->dev, "No pixel rate control in subdev %s\n",
>- sd->name);
>- return -EINVAL;
>- }
>-
> s_asd->sd = sd;
>
>- /* Create the link to the sensor. */
> source_pad = media_entity_get_fwnode_pad(&sd->entity, s_asd->source_ep,
> MEDIA_PAD_FL_SOURCE);
> if (source_pad < 0) {
>@@ -138,10 +130,7 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> return source_pad;
> }
>
>- return media_create_pad_link(&sd->entity, source_pad,
>- &rkisp1->isp.sd.entity,
>- RKISP1_ISP_PAD_SINK_VIDEO,
>- !s_asd->index ? MEDIA_LNK_FL_ENABLED : 0);
>+ return rkisp1_csi_link_sensor(rkisp1, sd, s_asd, source_pad);
> }
>
> static int rkisp1_subdev_notifier_complete(struct v4l2_async_notifier *notifier)
>@@ -283,6 +272,14 @@ static int rkisp1_create_links(struct rkisp1_device *rkisp1)
> unsigned int i;
> int ret;
>
>+ /* Link the CSI receiver to the ISP. */
>+ ret = media_create_pad_link(&rkisp1->csi.sd.entity, RKISP1_CSI_PAD_SRC,
>+ &rkisp1->isp.sd.entity,
>+ RKISP1_ISP_PAD_SINK_VIDEO,
>+ MEDIA_LNK_FL_ENABLED);
should this also be immutable?
>+ if (ret)
>+ return ret;
>+
> /* create ISP->RSZ->CAP links */
> for (i = 0; i < 2; i++) {
> struct media_entity *resizer =
>@@ -364,13 +361,6 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
> if (ret)
> goto error;
>
>- ret = rkisp1_subdev_notifier_register(rkisp1);
>- if (ret) {
>- dev_err(rkisp1->dev,
>- "Failed to register subdev notifier(%d)\n", ret);
>- goto error;
>- }
>-
> return 0;
>
> error:
>@@ -534,10 +524,16 @@ static int rkisp1_probe(struct platform_device *pdev)
> if (ret)
> goto err_cleanup_csi;
>
>+ ret = rkisp1_subdev_notifier_register(rkisp1);
>+ if (ret)
>+ goto err_unreg_entities;
>+
> rkisp1_debug_init(rkisp1);
>
> return 0;
>
>+err_unreg_entities:
>+ rkisp1_entities_unregister(rkisp1);
> err_cleanup_csi:
> rkisp1_csi_cleanup(rkisp1);
> err_unreg_media_dev:
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>index 5afb8be311c7..260c9ce0dca4 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>@@ -16,7 +16,6 @@
> #include <media/v4l2-event.h>
>
> #include "rkisp1-common.h"
>-#include "rkisp1-csi.h"
>
> #define RKISP1_DEF_SINK_PAD_FMT MEDIA_BUS_FMT_SRGGB10_1X10
> #define RKISP1_DEF_SRC_PAD_FMT MEDIA_BUS_FMT_YUYV8_2X8
>@@ -728,16 +727,12 @@ 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;
> int ret;
>
> if (!enable) {
> v4l2_subdev_call(rkisp1->source, video, s_stream, false);
>-
>- rkisp1_csi_stop(&rkisp1->csi);
> rkisp1_isp_stop(isp);
>-
> return 0;
> }
>
>@@ -754,30 +749,20 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
> return -EPIPE;
> }
>
>- asd = container_of(rkisp1->source->asd, struct rkisp1_sensor_async,
>- asd);
>-
>- if (asd->mbus_type != V4L2_MBUS_CSI2_DPHY)
>- return -EINVAL;
>+ if (rkisp1->source != &rkisp1->csi.sd)
>+ return -EPIPE;
This is not very clear, 'source' should point to the remote source (the one outside this driver)
so why should it be 'csi.sd' ?
Or is it that now 'rkips1->source' might be either the remote source or csi? if so I think it is a bit confusing.
>
> isp->frame_sequence = -1;
> mutex_lock(&isp->ops_lock);
>- ret = rkisp1_config_cif(isp, asd->mbus_type, asd->mbus_flags);
>+ ret = rkisp1_config_cif(isp, V4L2_MBUS_CSI2_DPHY, 0);
> if (ret)
> goto mutex_unlock;
>
> rkisp1_isp_start(isp);
>
>- ret = rkisp1_csi_start(&rkisp1->csi, asd);
>- if (ret) {
>- rkisp1_isp_stop(isp);
>- goto mutex_unlock;
>- }
>-
> ret = v4l2_subdev_call(rkisp1->source, video, s_stream, true);
Now that isp should call 's_stream' for 'rkisp1->csi->sd' and not 'rkisp1->source'.
thanks,
Dafna
> if (ret) {
> rkisp1_isp_stop(isp);
>- rkisp1_csi_stop(&rkisp1->csi);
> goto mutex_unlock;
> }
>
>--
>2.30.2
>
More information about the Linux-rockchip
mailing list