| Message ID | 20220614191127.3420492-1-paul.elder@ideasonboard.com |
|---|---|
| Headers | show |
| Series | media: rkisp1: Cleanups and add support for i.MX8MP | expand |
+ Sakari and Hans This patch is needed by this series, but I think it could also be reviewed and merged standalone already. On Wed, Jun 15, 2022 at 04:10:48AM +0900, Paul Elder wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Drivers typically extend the v4l2_async_subdev structure by embedding it > in a driver-specific structure, to store per-subdev custom data. The > v4l2_async_subdev instances are freed by the v4l2-async framework, which > makes this mechanism cumbersome to use safely when custom data needs > special treatment to be destroyed (such as freeing additional memory, or > releasing references to kernel objects). > > To ease this, add a .destroy() operation to the > v4l2_async_notifier_operations structure. The operation is called right > before the v4l2_async_subdev is freed, giving drivers a chance to > destroy data if needed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/driver-api/media/v4l2-subdev.rst | 6 ++++++ > drivers/media/v4l2-core/v4l2-async.c | 10 ++++++++++ > include/media/v4l2-async.h | 2 ++ > 3 files changed, 18 insertions(+) > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > index cf3b52bdbfb9..6f8d79926aa5 100644 > --- a/Documentation/driver-api/media/v4l2-subdev.rst > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > @@ -243,6 +243,12 @@ notifier callback is called. After all subdevices have been located the > .complete() callback is called. When a subdevice is removed from the > system the .unbind() method is called. All three callbacks are optional. > > +Drivers can store any type of custom data in their driver-specific > +:c:type:`v4l2_async_subdev` wrapper. If any of that data requires special > +handling when the structure is freed, drivers must implement the ``.destroy()`` > +notifier callback. The framework will call it right before freeing the > +:c:type:`v4l2_async_subdev`. > + > Calling subdev operations > ~~~~~~~~~~~~~~~~~~~~~~~~~ > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index c6995718237a..735dede624b8 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -52,6 +52,15 @@ static int v4l2_async_nf_call_complete(struct v4l2_async_notifier *n) > return n->ops->complete(n); > } > > +static void v4l2_async_nf_call_destroy(struct v4l2_async_notifier *n, > + struct v4l2_async_subdev *asd) > +{ > + if (!n->ops || !n->ops->destroy) > + return; > + > + n->ops->destroy(asd); > +} > + > static bool match_i2c(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > { > @@ -626,6 +635,7 @@ static void __v4l2_async_nf_cleanup(struct v4l2_async_notifier *notifier) > } > > list_del(&asd->asd_list); > + v4l2_async_nf_call_destroy(notifier, asd); > kfree(asd); > } > } > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index 13ff3ad948f4..25eb1d138c06 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -81,6 +81,7 @@ struct v4l2_async_subdev { > * @complete: All subdevices have been probed successfully. The complete > * callback is only executed for the root notifier. > * @unbind: a subdevice is leaving > + * @destroy: the asd is about to be freed > */ > struct v4l2_async_notifier_operations { > int (*bound)(struct v4l2_async_notifier *notifier, > @@ -90,6 +91,7 @@ struct v4l2_async_notifier_operations { > void (*unbind)(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *subdev, > struct v4l2_async_subdev *asd); > + void (*destroy)(struct v4l2_async_subdev *asd); > }; > > /**
Hi Paul, Thank you for the patch. On Wed, Jun 15, 2022 at 04:11:13AM +0900, Paul Elder wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Connect the CSI receiver subdevice into the rest of the driver. This > includes: > - 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@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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; There's trailing whitespace here. > + > + 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); > + 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; > > 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); > if (ret) { > rkisp1_isp_stop(isp); > - rkisp1_csi_stop(&rkisp1->csi); > goto mutex_unlock; > } > > -- > 2.30.2 >
Hi Paul, Thank you for the patches. On Wed, Jun 15, 2022 at 04:10:32AM +0900, Paul Elder wrote: > Hello, > > This series depends on v4 of "media: rkisp1: Misc bug fixes and cleanups" [1]. > > The ISP integrated in the i.MX8MP is nearly the same as the rkisp1, and > so we can reuse the rkisp1 driver for it. > > This series does some cleanup and refactoring of the rkisp1 driver, > (patches 1/55 to 46/55), and then adds support for the i.MX8MP (patches > 47/55 to 55/55). I have a few questions about the i.MX8MP support (the main one being about the version numbering scheme), which I'll ask in replies to the corresponding patches. Patches 01/55 to 46/55 could be merged already, once reviewed of course. As the series is large, let's see if we can motivate any reviewer :-) > [1] https://lore.kernel.org/linux-media/Ymbxs2p9Tuf331qM@pendragon.ideasonboard.com/T/ > > Laurent Pinchart (38): > media: rkisp1: Enable compilation on ARCH_MXC > media: rkisp1: Disable runtime PM in probe error path > media: rkisp1: Read the ID register at probe time instead of streamon > media: rkisp1: Rename rkisp1_match_data to rkisp1_info > media: rkisp1: Access ISP version from info pointer > media: rkisp1: cap: Print debug message on failed link validation > media: rkisp1: Move sensor .s_stream() call to ISP > media: rkisp1: Reject sensors without pixel rate control at bound time > media: rkisp1: Create link from sensor to ISP at notifier bound time > media: rkisp1: Create internal links at probe time > media: rkisp1: Rename rkisp1_subdev_notifier() to > rkisp1_subdev_notifier_register() > media: v4l2-async: Add notifier operation to destroy asd instances > media: rkisp1: Fix sensor source pad retrieval at bound time > media: rkisp1: isp: Start CSI-2 receiver before ISP > media: rkisp1: csi: Handle CSI-2 RX configuration fully in > rkisp1-csi.c > media: rkisp1: csi: Rename CSI functions with a common rkisp1_csi > prefix > media: rkisp1: csi: Move start delay to rkisp1_csi_start() > media: rkisp1: csi: Pass sensor pointer to rkisp1_csi_config() > media: rkisp1: csi: Constify argument to rkisp1_csi_start() > media: rkisp1: isp: Don't initialize ret to 0 in rkisp1_isp_s_stream() > media: rkisp1: isp: Pass mbus type and flags to rkisp1_config_cif() > media: rkisp1: isp: Rename rkisp1_device.active_sensor to source > media: rkisp1: isp: Add container_of wrapper to cast subdev to > rkisp1_isp > media: rkisp1: isp: Add rkisp1_device backpointer to rkisp1_isp > media: rkisp1: isp: Pass rkisp1_isp pointer to internal ISP functions > media: rkisp1: isp: Move input configuration to rkisp1_config_isp() > media: rkisp1: isp: Merge ISP_ACQ_PROP configuration in single > variable > media: rkisp1: isp: Initialize some variables at declaration time > media: rkisp1: isp: Fix whitespace issues > media: rkisp1: isp: Constify various local variables > media: rkisp1: isp: Rename rkisp1_get_remote_source() > media: mc-entity: Add a new helper function to get a remote pad > media: mc-entity: Add a new helper function to get a remote pad for a > pad > media: rkisp1: isp: Disallow multiple active sources > media: rkisp1: csi: Plumb the CSI RX subdev > media: rkisp1: Add infrastructure to support ISP features > media: rkisp1: Make the internal CSI-2 receiver optional > media: rkisp1: Configure gasket on i.MX8MP > > Paul Elder (17): > media: rkisp1: debug: Add dump file in debugfs for MI buffer registers > media: rkisp1: debug: Add debugfs files to monitor MI and ISP > interrupts > media: rkisp1: Save info pointer in rkisp1_device > media: rkisp1: Make rkisp1_isp_mbus_info common > media: rkisp1: Split CSI handling to separate file > media: rkisp1: csi: Implement a V4L2 subdev for the CSI receiver > media: rkisp1: Use fwnode_graph_for_each_endpoint > dt-bindings: media: rkisp1: Add port for parallel interface > media: rkisp1: Support the ISP parallel input > dt-bindings: media: rkisp1: Add i.MX8MP ISP to compatible > media: rkisp1: Add match data for i.MX8MP ISP > media: rkisp1: Add and set registers for crop for i.MX8MP > media: rkisp1: Add and set registers for output size config on i.MX8MP > media: rkisp1: Add i.MX8MP-specific registers for MI and resizer > media: rkisp1: Shift DMA buffer addresses on i.MX8MP > media: rkisp1: Add register definitions for the test pattern generator > media: rkisp1: Fix RSZ_CTRL bits for i.MX8MP > > .../bindings/media/rockchip-isp1.yaml | 30 +- > Documentation/driver-api/media/mc-core.rst | 4 +- > .../driver-api/media/v4l2-subdev.rst | 6 + > drivers/media/mc/mc-entity.c | 69 ++ > .../media/platform/rockchip/rkisp1/Kconfig | 2 +- > .../media/platform/rockchip/rkisp1/Makefile | 1 + > .../platform/rockchip/rkisp1/rkisp1-capture.c | 49 +- > .../platform/rockchip/rkisp1/rkisp1-common.c | 148 ++++ > .../platform/rockchip/rkisp1/rkisp1-common.h | 130 +++- > .../platform/rockchip/rkisp1/rkisp1-csi.c | 525 ++++++++++++++ > .../platform/rockchip/rkisp1/rkisp1-csi.h | 28 + > .../platform/rockchip/rkisp1/rkisp1-debug.c | 55 +- > .../platform/rockchip/rkisp1/rkisp1-dev.c | 440 +++++++----- > .../platform/rockchip/rkisp1/rkisp1-isp.c | 679 +++++++----------- > .../platform/rockchip/rkisp1/rkisp1-params.c | 2 +- > .../platform/rockchip/rkisp1/rkisp1-regs.h | 87 +++ > .../platform/rockchip/rkisp1/rkisp1-resizer.c | 43 +- > .../platform/rockchip/rkisp1/rkisp1-stats.c | 4 +- > drivers/media/v4l2-core/v4l2-async.c | 10 + > include/media/media-entity.h | 63 ++ > include/media/v4l2-async.h | 2 + > include/uapi/linux/rkisp1-config.h | 3 + > 22 files changed, 1735 insertions(+), 645 deletions(-) > create mode 100644 drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c > create mode 100644 drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
Hi Paul, Thank you for the patch. On Wed, Jun 15, 2022 at 04:10:41AM +0900, Paul Elder wrote: > The upcoming CSI receiver split from the ISP to a separate source file > will need to be able to access the list of formats supported by the > driver. Move it out of the ISP's header and into the common header, and > add helper functions for accessing it so that the format list doesn't > need to be stored in the header. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > .../platform/rockchip/rkisp1/rkisp1-common.c | 148 +++++++++++++++ > .../platform/rockchip/rkisp1/rkisp1-common.h | 28 ++- > .../platform/rockchip/rkisp1/rkisp1-isp.c | 168 ++---------------- > .../platform/rockchip/rkisp1/rkisp1-resizer.c | 14 +- > .../platform/rockchip/rkisp1/rkisp1-stats.c | 2 +- > 5 files changed, 193 insertions(+), 167 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c > index cf889666e166..bb0ea20118e1 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c > @@ -5,10 +5,158 @@ > * Copyright (C) 2019 Collabora, Ltd. > */ > > +#include <media/mipi-csi2.h> > #include <media/v4l2-rect.h> > > #include "rkisp1-common.h" > > +static const struct rkisp1_mbus_info rkisp1_formats[] = { > + { > + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8, > + .pixel_enc = V4L2_PIXEL_ENC_YUV, > + .direction = RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW10, > + .bayer_pat = RKISP1_RAW_RGGB, > + .bus_width = 10, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW10, > + .bayer_pat = RKISP1_RAW_BGGR, > + .bus_width = 10, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW10, > + .bayer_pat = RKISP1_RAW_GBRG, > + .bus_width = 10, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW10, > + .bayer_pat = RKISP1_RAW_GRBG, > + .bus_width = 10, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW12, > + .bayer_pat = RKISP1_RAW_RGGB, > + .bus_width = 12, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW12, > + .bayer_pat = RKISP1_RAW_BGGR, > + .bus_width = 12, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW12, > + .bayer_pat = RKISP1_RAW_GBRG, > + .bus_width = 12, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW12, > + .bayer_pat = RKISP1_RAW_GRBG, > + .bus_width = 12, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW8, > + .bayer_pat = RKISP1_RAW_RGGB, > + .bus_width = 8, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW8, > + .bayer_pat = RKISP1_RAW_BGGR, > + .bus_width = 8, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW8, > + .bayer_pat = RKISP1_RAW_GBRG, > + .bus_width = 8, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8, > + .pixel_enc = V4L2_PIXEL_ENC_BAYER, > + .mipi_dt = MIPI_CSI2_DT_RAW8, > + .bayer_pat = RKISP1_RAW_GRBG, > + .bus_width = 8, > + .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > + }, { > + .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16, > + .pixel_enc = V4L2_PIXEL_ENC_YUV, > + .mipi_dt = MIPI_CSI2_DT_YUV422_8B, > + .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_YCBYCR, > + .bus_width = 16, > + .direction = RKISP1_ISP_SD_SINK, > + }, { > + .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16, > + .pixel_enc = V4L2_PIXEL_ENC_YUV, > + .mipi_dt = MIPI_CSI2_DT_YUV422_8B, > + .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_YCRYCB, > + .bus_width = 16, > + .direction = RKISP1_ISP_SD_SINK, > + }, { > + .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16, > + .pixel_enc = V4L2_PIXEL_ENC_YUV, > + .mipi_dt = MIPI_CSI2_DT_YUV422_8B, > + .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_CBYCRY, > + .bus_width = 16, > + .direction = RKISP1_ISP_SD_SINK, > + }, { > + .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16, > + .pixel_enc = V4L2_PIXEL_ENC_YUV, > + .mipi_dt = MIPI_CSI2_DT_YUV422_8B, > + .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_CRYCBY, > + .bus_width = 16, > + .direction = RKISP1_ISP_SD_SINK, > + }, > +}; > + > +unsigned int rkisp1_mbus_info_length(void) > +{ > + return ARRAY_SIZE(rkisp1_formats); > +} > + > +const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_index(unsigned int index) > +{ > + if (index >= rkisp1_mbus_info_length()) if (index >= ARRAY_SIZE(rkisp1_formats)) to match the code below (and see also my comment further down) > + return NULL; > + > + return &rkisp1_formats[index]; > +} > + > +const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_code(u32 mbus_code) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(rkisp1_formats); i++) { > + const struct rkisp1_mbus_info *fmt = &rkisp1_formats[i]; > + > + if (fmt->mbus_code == mbus_code) > + return fmt; > + } > + > + return NULL; > +} > + > static const struct v4l2_rect rkisp1_sd_min_crop = { > .width = RKISP1_ISP_MIN_WIDTH, > .height = RKISP1_ISP_MIN_HEIGHT, > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > index 50d31a254b03..c7d5c57607bd 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > @@ -26,7 +26,7 @@ > struct dentry; > > /* > - * flags on the 'direction' field in struct 'rkisp1_isp_mbus_info' that indicate > + * flags on the 'direction' field in struct rkisp1_mbus_info' that indicate > * on which pad the media bus format is supported > */ > #define RKISP1_ISP_SD_SRC BIT(0) > @@ -150,8 +150,8 @@ struct rkisp1_isp { > struct v4l2_subdev sd; > struct media_pad pads[RKISP1_ISP_PAD_MAX]; > struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX]; > - const struct rkisp1_isp_mbus_info *sink_fmt; > - const struct rkisp1_isp_mbus_info *src_fmt; > + const struct rkisp1_mbus_info *sink_fmt; > + const struct rkisp1_mbus_info *src_fmt; > struct mutex ops_lock; /* serialize the subdevice ops */ > bool is_dphy_errctrl_disabled; > __u32 frame_sequence; > @@ -438,8 +438,8 @@ struct rkisp1_device { > }; > > /* > - * struct rkisp1_isp_mbus_info - ISP media bus info, Translates media bus code to hardware > - * format values > + * struct rkisp1_mbus_info - ISP media bus info, Translates media bus code to hardware > + * format values > * > * @mbus_code: media bus code > * @pixel_enc: pixel encoding > @@ -449,7 +449,7 @@ struct rkisp1_device { > * @bayer_pat: bayer pattern > * @direction: a bitmask of the flags indicating on which pad the format is supported on > */ > -struct rkisp1_isp_mbus_info { > +struct rkisp1_mbus_info { > u32 mbus_code; > enum v4l2_pixel_encoding pixel_enc; > u32 mipi_dt; > @@ -481,6 +481,18 @@ static inline u32 rkisp1_read(struct rkisp1_device *rkisp1, unsigned int addr) > int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap, > struct v4l2_subdev_mbus_code_enum *code); > > +/* > + * rkisp1_mbus_info_length - Return the number of supported mbus codes > + */ > +unsigned int rkisp1_mbus_info_length(void); > + > +/* > + * rkisp1_mbus_info_get_by_index - Retrieve the ith supported mbus info > + * > + * @index: index of the mbus info to fetch > + */ > +const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_index(unsigned int index); > + > /* > * rkisp1_sd_adjust_crop_rect - adjust a rectangle to fit into another rectangle. > * > @@ -500,11 +512,11 @@ void rkisp1_sd_adjust_crop(struct v4l2_rect *crop, > const struct v4l2_mbus_framefmt *bounds); > > /* > - * rkisp1_isp_mbus_info - get the isp info of the media bus code > + * rkisp1_mbus_info_get_by_code - get the isp info of the media bus code > * > * @mbus_code: the media bus code > */ > -const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code); > +const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_code(u32 mbus_code); > > /* rkisp1_params_configure - configure the params when stream starts. > * This function is called by the isp entity upon stream starts. > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > index 328e8fec14e9..89577119b571 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c > @@ -15,7 +15,6 @@ > #include <linux/videodev2.h> > #include <linux/vmalloc.h> > > -#include <media/mipi-csi2.h> > #include <media/v4l2-event.h> > > #include "rkisp1-common.h" > @@ -56,144 +55,10 @@ > * +---------------------------------------------------------+ > */ > > -static const struct rkisp1_isp_mbus_info rkisp1_isp_formats[] = { > - { > - .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8, > - .pixel_enc = V4L2_PIXEL_ENC_YUV, > - .direction = RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW10, > - .bayer_pat = RKISP1_RAW_RGGB, > - .bus_width = 10, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW10, > - .bayer_pat = RKISP1_RAW_BGGR, > - .bus_width = 10, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW10, > - .bayer_pat = RKISP1_RAW_GBRG, > - .bus_width = 10, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW10, > - .bayer_pat = RKISP1_RAW_GRBG, > - .bus_width = 10, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW12, > - .bayer_pat = RKISP1_RAW_RGGB, > - .bus_width = 12, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW12, > - .bayer_pat = RKISP1_RAW_BGGR, > - .bus_width = 12, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW12, > - .bayer_pat = RKISP1_RAW_GBRG, > - .bus_width = 12, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW12, > - .bayer_pat = RKISP1_RAW_GRBG, > - .bus_width = 12, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW8, > - .bayer_pat = RKISP1_RAW_RGGB, > - .bus_width = 8, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW8, > - .bayer_pat = RKISP1_RAW_BGGR, > - .bus_width = 8, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW8, > - .bayer_pat = RKISP1_RAW_GBRG, > - .bus_width = 8, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8, > - .pixel_enc = V4L2_PIXEL_ENC_BAYER, > - .mipi_dt = MIPI_CSI2_DT_RAW8, > - .bayer_pat = RKISP1_RAW_GRBG, > - .bus_width = 8, > - .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, > - }, { > - .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16, > - .pixel_enc = V4L2_PIXEL_ENC_YUV, > - .mipi_dt = MIPI_CSI2_DT_YUV422_8B, > - .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_YCBYCR, > - .bus_width = 16, > - .direction = RKISP1_ISP_SD_SINK, > - }, { > - .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16, > - .pixel_enc = V4L2_PIXEL_ENC_YUV, > - .mipi_dt = MIPI_CSI2_DT_YUV422_8B, > - .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_YCRYCB, > - .bus_width = 16, > - .direction = RKISP1_ISP_SD_SINK, > - }, { > - .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16, > - .pixel_enc = V4L2_PIXEL_ENC_YUV, > - .mipi_dt = MIPI_CSI2_DT_YUV422_8B, > - .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_CBYCRY, > - .bus_width = 16, > - .direction = RKISP1_ISP_SD_SINK, > - }, { > - .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16, > - .pixel_enc = V4L2_PIXEL_ENC_YUV, > - .mipi_dt = MIPI_CSI2_DT_YUV422_8B, > - .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_CRYCBY, > - .bus_width = 16, > - .direction = RKISP1_ISP_SD_SINK, > - }, > -}; > - > /* ---------------------------------------------------------------------------- > * Helpers > */ > > -const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code) > -{ > - unsigned int i; > - > - for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) { > - const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i]; > - > - if (fmt->mbus_code == mbus_code) > - return fmt; > - } > - > - return NULL; > -} > - > static struct v4l2_subdev *rkisp1_get_remote_sensor(struct v4l2_subdev *sd) > { > struct media_pad *local, *remote; > @@ -275,7 +140,7 @@ static void rkisp1_config_ism(struct rkisp1_device *rkisp1) > static int rkisp1_config_isp(struct rkisp1_device *rkisp1) > { > u32 isp_ctrl = 0, irq_mask = 0, acq_mult = 0, signal = 0; > - const struct rkisp1_isp_mbus_info *src_fmt, *sink_fmt; > + const struct rkisp1_mbus_info *src_fmt, *sink_fmt; > struct rkisp1_sensor_async *sensor; > struct v4l2_mbus_framefmt *sink_frm; > struct v4l2_rect *sink_crop; > @@ -376,7 +241,7 @@ static int rkisp1_config_isp(struct rkisp1_device *rkisp1) > > static int rkisp1_config_dvp(struct rkisp1_device *rkisp1) > { > - const struct rkisp1_isp_mbus_info *sink_fmt = rkisp1->isp.sink_fmt; > + const struct rkisp1_mbus_info *sink_fmt = rkisp1->isp.sink_fmt; > u32 val, input_sel; > > switch (sink_fmt->bus_width) { > @@ -402,7 +267,7 @@ static int rkisp1_config_dvp(struct rkisp1_device *rkisp1) > > static int rkisp1_config_mipi(struct rkisp1_device *rkisp1) > { > - const struct rkisp1_isp_mbus_info *sink_fmt = rkisp1->isp.sink_fmt; > + const struct rkisp1_mbus_info *sink_fmt = rkisp1->isp.sink_fmt; > unsigned int lanes = rkisp1->active_sensor->lanes; > u32 mipi_ctrl; > > @@ -593,11 +458,12 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd, > return 0; > } > > - if (code->index >= ARRAY_SIZE(rkisp1_isp_formats)) > + if (code->index >= rkisp1_mbus_info_length()) > return -EINVAL; > > - for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) { > - const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i]; > + for (i = 0; i < rkisp1_mbus_info_length(); i++) { > + const struct rkisp1_mbus_info *fmt = > + rkisp1_mbus_info_get_by_index(i); You could write this as for (i = 0; ; i++) { const struct rkisp1_mbus_info *fmt = rkisp1_mbus_info_get_by_index(i); if (!fmt) return -EINVAL; and drop the rkisp1_mbus_info_length() function. Up to you. Either way, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > if (fmt->direction & dir) > pos++; > @@ -619,7 +485,7 @@ static int rkisp1_isp_enum_frame_size(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_size_enum *fse) > { > - const struct rkisp1_isp_mbus_info *mbus_info; > + const struct rkisp1_mbus_info *mbus_info; > > if (fse->pad == RKISP1_ISP_PAD_SINK_PARAMS || > fse->pad == RKISP1_ISP_PAD_SOURCE_STATS) > @@ -628,7 +494,7 @@ static int rkisp1_isp_enum_frame_size(struct v4l2_subdev *sd, > if (fse->index > 0) > return -EINVAL; > > - mbus_info = rkisp1_isp_mbus_info_get(fse->code); > + mbus_info = rkisp1_mbus_info_get_by_code(fse->code); > if (!mbus_info) > return -EINVAL; > > @@ -695,7 +561,7 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, > struct v4l2_mbus_framefmt *format, > unsigned int which) > { > - const struct rkisp1_isp_mbus_info *mbus_info; > + const struct rkisp1_mbus_info *mbus_info; > struct v4l2_mbus_framefmt *src_fmt; > const struct v4l2_rect *src_crop; > > @@ -705,10 +571,10 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, > RKISP1_ISP_PAD_SOURCE_VIDEO, which); > > src_fmt->code = format->code; > - mbus_info = rkisp1_isp_mbus_info_get(src_fmt->code); > + mbus_info = rkisp1_mbus_info_get_by_code(src_fmt->code); > if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) { > src_fmt->code = RKISP1_DEF_SRC_PAD_FMT; > - mbus_info = rkisp1_isp_mbus_info_get(src_fmt->code); > + mbus_info = rkisp1_mbus_info_get_by_code(src_fmt->code); > } > if (which == V4L2_SUBDEV_FORMAT_ACTIVE) > isp->src_fmt = mbus_info; > @@ -793,7 +659,7 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp, > struct v4l2_mbus_framefmt *format, > unsigned int which) > { > - const struct rkisp1_isp_mbus_info *mbus_info; > + const struct rkisp1_mbus_info *mbus_info; > struct v4l2_mbus_framefmt *sink_fmt; > struct v4l2_rect *sink_crop; > > @@ -801,10 +667,10 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp, > RKISP1_ISP_PAD_SINK_VIDEO, > which); > sink_fmt->code = format->code; > - mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); > + mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SINK)) { > sink_fmt->code = RKISP1_DEF_SINK_PAD_FMT; > - mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); > + mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > } > if (which == V4L2_SUBDEV_FORMAT_ACTIVE) > isp->sink_fmt = mbus_info; > @@ -1080,8 +946,8 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1) > pads[RKISP1_ISP_PAD_SOURCE_VIDEO].flags = MEDIA_PAD_FL_SOURCE; > pads[RKISP1_ISP_PAD_SOURCE_STATS].flags = MEDIA_PAD_FL_SOURCE; > > - isp->sink_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SINK_PAD_FMT); > - isp->src_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SRC_PAD_FMT); > + isp->sink_fmt = rkisp1_mbus_info_get_by_code(RKISP1_DEF_SINK_PAD_FMT); > + isp->src_fmt = rkisp1_mbus_info_get_by_code(RKISP1_DEF_SRC_PAD_FMT); > > mutex_init(&isp->ops_lock); > ret = media_entity_pads_init(&sd->entity, RKISP1_ISP_PAD_MAX, pads); > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > index 1c07985c810d..f4caa8f684aa 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > @@ -433,14 +433,14 @@ static void rkisp1_rsz_set_src_fmt(struct rkisp1_resizer *rsz, > struct v4l2_mbus_framefmt *format, > unsigned int which) > { > - const struct rkisp1_isp_mbus_info *sink_mbus_info; > + const struct rkisp1_mbus_info *sink_mbus_info; > struct v4l2_mbus_framefmt *src_fmt, *sink_fmt; > > sink_fmt = rkisp1_rsz_get_pad_fmt(rsz, sd_state, RKISP1_RSZ_PAD_SINK, > which); > src_fmt = rkisp1_rsz_get_pad_fmt(rsz, sd_state, RKISP1_RSZ_PAD_SRC, > which); > - sink_mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); > + sink_mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > > /* for YUV formats, userspace can change the mbus code on the src pad if it is supported */ > if (sink_mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV && > @@ -462,7 +462,7 @@ static void rkisp1_rsz_set_sink_crop(struct rkisp1_resizer *rsz, > struct v4l2_rect *r, > unsigned int which) > { > - const struct rkisp1_isp_mbus_info *mbus_info; > + const struct rkisp1_mbus_info *mbus_info; > struct v4l2_mbus_framefmt *sink_fmt; > struct v4l2_rect *sink_crop; > > @@ -473,7 +473,7 @@ static void rkisp1_rsz_set_sink_crop(struct rkisp1_resizer *rsz, > which); > > /* Not crop for MP bayer raw data */ > - mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); > + mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > > if (rsz->id == RKISP1_MAINPATH && > mbus_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) { > @@ -500,7 +500,7 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz, > struct v4l2_mbus_framefmt *format, > unsigned int which) > { > - const struct rkisp1_isp_mbus_info *mbus_info; > + const struct rkisp1_mbus_info *mbus_info; > struct v4l2_mbus_framefmt *sink_fmt, *src_fmt; > struct v4l2_rect *sink_crop; > > @@ -516,10 +516,10 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz, > else > sink_fmt->code = format->code; > > - mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); > + mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) { > sink_fmt->code = RKISP1_DEF_FMT; > - mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); > + mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > } > if (which == V4L2_SUBDEV_FORMAT_ACTIVE) > rsz->pixel_enc = mbus_info->pixel_enc; > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c > index 7d82356b5345..2795eef91bdd 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c > @@ -305,7 +305,7 @@ static void rkisp1_stats_get_bls_meas(struct rkisp1_stats *stats, > struct rkisp1_stat_buffer *pbuf) > { > struct rkisp1_device *rkisp1 = stats->rkisp1; > - const struct rkisp1_isp_mbus_info *in_fmt = rkisp1->isp.sink_fmt; > + const struct rkisp1_mbus_info *in_fmt = rkisp1->isp.sink_fmt; > struct rkisp1_cif_isp_bls_meas_val *bls_val; > > bls_val = &pbuf->params.ae.bls_val;
Hello Paul, thanks for the patch. Am Dienstag, 14. Juni 2022, 21:11:27 CEST schrieb Paul Elder: > The ISP that is integrated in the i.MX8MP uses different bits in the > MRSZ_CTRL and SRSZ_CTRL registers for updating the configuration > compared to the on in the RK3399. In addition, it adds a new bit for > enabling crop. Add new definitions for these bits for i.MX8MP devices, > and update where they are set. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h | 4 ++++ > .../media/platform/rockchip/rkisp1/rkisp1-resizer.c | 10 ++++++++-- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h index > 34f4fe09c88d..24ad2ccec2a3 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > @@ -168,6 +168,10 @@ > #define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO BIT(9) > #define RKISP1_CIF_RSZ_SCALER_FACTOR BIT(16) > > +#define RKISP1_CIF_RSZ_CTRL_CROP_ENABLE_IMX BIT(8) > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX BIT(9) > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX BIT(10) > + Does it make sense to move this kind of information into struct rkisp1_info? This way you can skip the if (isp_ver == ...) thing. Best regards, Alexander > /* RSZ_CROP_[XY]_DIR */ > #define RKISP1_CIF_RSZ_CROP_XY_DIR(start, end) ((end) << 16 | (start) << > 0) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c index > 08bf3aa8088f..29a31b18a082 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > @@ -209,9 +209,15 @@ static void rkisp1_rsz_update_shadow(struct > rkisp1_resizer *rsz, u32 ctrl_cfg = rkisp1_rsz_read(rsz, > RKISP1_CIF_RSZ_CTRL); > > if (when == RKISP1_SHADOW_REGS_ASYNC) > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX; > + else > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > else > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX; > + else > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > > rkisp1_rsz_write(rsz, RKISP1_CIF_RSZ_CTRL, ctrl_cfg); > }
On 6/14/22 21:11, Paul Elder wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The media_entity_remote_pad() helper function returns the first remote > pad it find connected to a given pad. Beside being possibly ill-named find -> finds > (as it operates on a pad, not an entity) and non-deterministic (as it > stops at the first enabled link), the fact that it returns the first > match makes it unsuitable for drivers that need to guarantee that a > single link is enabled, for instance when an entity can process data > from one of multiple sources at a time. > > For those use cases, add a new helper function, > media_entity_remote_pad_unique(), that operates on an entity and returns > a remote pad, with a guarantee that only one link is enabled. To ease > its use in drivers, also add an inline wrapper that locates source pads > specifically. A wrapper that locates sink pads can easily be added when > needed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/driver-api/media/mc-core.rst | 4 +- > drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > include/media/media-entity.h | 45 ++++++++++++++++++++++ > 3 files changed, 85 insertions(+), 2 deletions(-) > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > index 02481a2513b9..a2d1e32e3abb 100644 > --- a/Documentation/driver-api/media/mc-core.rst > +++ b/Documentation/driver-api/media/mc-core.rst > @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > > Helper functions can be used to find a link between two given pads, or a pad > connected to another pad through an enabled link > -:c:func:`media_entity_find_link()` and > -:c:func:`media_entity_remote_pad()`. > +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > +:c:func:`media_entity_remote_source_pad()`). > > Use count and power handling > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index 11f5207f73aa..1febf5a86be6 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -9,6 +9,7 @@ > */ > > #include <linux/bitmap.h> > +#include <linux/list.h> > #include <linux/property.h> > #include <linux/slab.h> > #include <media/media-entity.h> > @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > } > EXPORT_SYMBOL_GPL(media_entity_remote_pad); > > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type) > +{ > + struct media_pad *pad = NULL; > + struct media_link *link; > + > + list_for_each_entry(link, &entity->links, list) { > + struct media_pad *local_pad; > + struct media_pad *remote_pad; > + > + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > + continue; > + > + if (type == MEDIA_PAD_FL_SOURCE) { > + local_pad = link->sink; > + remote_pad = link->source; > + } else { > + local_pad = link->source; > + remote_pad = link->sink; > + } > + > + if (local_pad->entity == entity) { > + if (pad) > + return ERR_PTR(-ENOTUNIQ); > + > + pad = remote_pad; > + } > + } > + > + if (!pad) > + return ERR_PTR(-ENOLINK); > + > + return pad; > +} > +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > + > static void media_interface_init(struct media_device *mdev, > struct media_interface *intf, > u32 gobj_type, > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index a9a1c0ec5d1c..33d5f52719a0 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > */ > struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > > +/** > + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > + * @entity: The entity > + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > + * > + * Search for and return a remote pad of @type connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source or sink at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type); > + > +/** > + * media_entity_remote_source_pad - Find a remote source pad connected to an entity Shouldn't this be called media_entity_remote_source_pad_unique? After all, it has the uniqueness constraint, but that's not reflected in the name. With that change you can add my: Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, Hans > + * @entity: The entity > + * > + * Search for and return a remote source pad connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +static inline struct media_pad * > +media_entity_remote_source_pad(const struct media_entity *entity) > +{ > + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > +} > + > /** > * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > * @entity: The entity
On 6/14/22 21:11, Paul Elder wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The media_entity_remote_pad() helper function returns the first remote > pad it find connected to a given pad. Beside being possibly ill-named > (as it operates on a pad, not an entity) and non-deterministic (as it > stops at the first enabled link), the fact that it returns the first > match makes it unsuitable for drivers that need to guarantee that a > single link is enabled, for instance when an entity can process data > from one of multiple sources at a time. Question: of all the callers of this function, are there any that really need media_entity_remote_pad() instead of media_pad_remote_pad_unique()? Would it be possible to replace all callers of the old function with the new function? If that's the case, then the _unique suffix can be dropped, since that would effectively be the default. And if a function is needed to handle the case where there are multiple enabled links, then a new function should be created. Also, media_entity_remote_pad() should really be renamed to media_pad_remote_pad_first() or something like that, right? I'm not saying you should, but that's really what it does, as I understand it. Regards, Hans > > For those use cases, add a new helper function, > media_entity_remote_pad_unique(), that operates on an entity and returns > a remote pad, with a guarantee that only one link is enabled. To ease > its use in drivers, also add an inline wrapper that locates source pads > specifically. A wrapper that locates sink pads can easily be added when > needed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/driver-api/media/mc-core.rst | 4 +- > drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > include/media/media-entity.h | 45 ++++++++++++++++++++++ > 3 files changed, 85 insertions(+), 2 deletions(-) > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > index 02481a2513b9..a2d1e32e3abb 100644 > --- a/Documentation/driver-api/media/mc-core.rst > +++ b/Documentation/driver-api/media/mc-core.rst > @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > > Helper functions can be used to find a link between two given pads, or a pad > connected to another pad through an enabled link > -:c:func:`media_entity_find_link()` and > -:c:func:`media_entity_remote_pad()`. > +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > +:c:func:`media_entity_remote_source_pad()`). > > Use count and power handling > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index 11f5207f73aa..1febf5a86be6 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -9,6 +9,7 @@ > */ > > #include <linux/bitmap.h> > +#include <linux/list.h> > #include <linux/property.h> > #include <linux/slab.h> > #include <media/media-entity.h> > @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > } > EXPORT_SYMBOL_GPL(media_entity_remote_pad); > > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type) > +{ > + struct media_pad *pad = NULL; > + struct media_link *link; > + > + list_for_each_entry(link, &entity->links, list) { > + struct media_pad *local_pad; > + struct media_pad *remote_pad; > + > + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > + continue; > + > + if (type == MEDIA_PAD_FL_SOURCE) { > + local_pad = link->sink; > + remote_pad = link->source; > + } else { > + local_pad = link->source; > + remote_pad = link->sink; > + } > + > + if (local_pad->entity == entity) { > + if (pad) > + return ERR_PTR(-ENOTUNIQ); > + > + pad = remote_pad; > + } > + } > + > + if (!pad) > + return ERR_PTR(-ENOLINK); > + > + return pad; > +} > +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > + > static void media_interface_init(struct media_device *mdev, > struct media_interface *intf, > u32 gobj_type, > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index a9a1c0ec5d1c..33d5f52719a0 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > */ > struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > > +/** > + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > + * @entity: The entity > + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > + * > + * Search for and return a remote pad of @type connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source or sink at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type); > + > +/** > + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > + * @entity: The entity > + * > + * Search for and return a remote source pad connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +static inline struct media_pad * > +media_entity_remote_source_pad(const struct media_entity *entity) > +{ > + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > +} > + > /** > * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > * @entity: The entity
Hello all On 14/06/2022 20:11, Paul Elder wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The media_entity_remote_pad() helper function returns the first remote > pad it find connected to a given pad. Beside being possibly ill-named > (as it operates on a pad, not an entity) and non-deterministic (as it > stops at the first enabled link), the fact that it returns the first > match makes it unsuitable for drivers that need to guarantee that a > single link is enabled, for instance when an entity can process data > from one of multiple sources at a time. > > For those use cases, add a new helper function, > media_entity_remote_pad_unique(), that operates on an entity and returns > a remote pad, with a guarantee that only one link is enabled. To ease > its use in drivers, also add an inline wrapper that locates source pads > specifically. A wrapper that locates sink pads can easily be added when > needed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/driver-api/media/mc-core.rst | 4 +- > drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > include/media/media-entity.h | 45 ++++++++++++++++++++++ > 3 files changed, 85 insertions(+), 2 deletions(-) > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > index 02481a2513b9..a2d1e32e3abb 100644 > --- a/Documentation/driver-api/media/mc-core.rst > +++ b/Documentation/driver-api/media/mc-core.rst > @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > > Helper functions can be used to find a link between two given pads, or a pad > connected to another pad through an enabled link > -:c:func:`media_entity_find_link()` and > -:c:func:`media_entity_remote_pad()`. > +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > +:c:func:`media_entity_remote_source_pad()`). > > Use count and power handling > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index 11f5207f73aa..1febf5a86be6 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -9,6 +9,7 @@ > */ > > #include <linux/bitmap.h> > +#include <linux/list.h> > #include <linux/property.h> > #include <linux/slab.h> > #include <media/media-entity.h> > @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > } > EXPORT_SYMBOL_GPL(media_entity_remote_pad); > > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type) > +{ > + struct media_pad *pad = NULL; > + struct media_link *link; > + > + list_for_each_entry(link, &entity->links, list) { > + struct media_pad *local_pad; > + struct media_pad *remote_pad; > + > + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > + continue; Does this need another guard here to make sure the link isn't an ancillary link? Only likely to happen at least in the immediate future where the entity represents a camera sensor, so possibly not applicable here - I couldn't find where the new function is used in the series to check. I _think_ it would actually work ok regardless...media_gobj type-punning makes my brain ache, but I think the local_pad->entity == entity comparison would actually compare the entity->name member of the entity at the end of an ancillary link to the entity parameter, not find a match and so continue the loop without failing, but that feels a bit sub-optimal. > + > + if (type == MEDIA_PAD_FL_SOURCE) { > + local_pad = link->sink; > + remote_pad = link->source; > + } else { > + local_pad = link->source; > + remote_pad = link->sink; > + } > + > + if (local_pad->entity == entity) { > + if (pad) > + return ERR_PTR(-ENOTUNIQ); > + > + pad = remote_pad; > + } > + } > + > + if (!pad) > + return ERR_PTR(-ENOLINK); > + > + return pad; > +} > +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > + > static void media_interface_init(struct media_device *mdev, > struct media_interface *intf, > u32 gobj_type, > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index a9a1c0ec5d1c..33d5f52719a0 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > */ > struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > > +/** > + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > + * @entity: The entity > + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > + * > + * Search for and return a remote pad of @type connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source or sink at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type); > + > +/** > + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > + * @entity: The entity > + * > + * Search for and return a remote source pad connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +static inline struct media_pad * > +media_entity_remote_source_pad(const struct media_entity *entity) > +{ > + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > +} > + > /** > * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > * @entity: The entity
On 17/06/2022 22:34, Daniel Scally wrote: > Hello all > > On 14/06/2022 20:11, Paul Elder wrote: >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> The media_entity_remote_pad() helper function returns the first remote >> pad it find connected to a given pad. Beside being possibly ill-named >> (as it operates on a pad, not an entity) and non-deterministic (as it >> stops at the first enabled link), the fact that it returns the first >> match makes it unsuitable for drivers that need to guarantee that a >> single link is enabled, for instance when an entity can process data >> from one of multiple sources at a time. >> >> For those use cases, add a new helper function, >> media_entity_remote_pad_unique(), that operates on an entity and returns >> a remote pad, with a guarantee that only one link is enabled. To ease >> its use in drivers, also add an inline wrapper that locates source pads >> specifically. A wrapper that locates sink pads can easily be added when >> needed. >> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> Documentation/driver-api/media/mc-core.rst | 4 +- >> drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ >> include/media/media-entity.h | 45 ++++++++++++++++++++++ >> 3 files changed, 85 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst >> index 02481a2513b9..a2d1e32e3abb 100644 >> --- a/Documentation/driver-api/media/mc-core.rst >> +++ b/Documentation/driver-api/media/mc-core.rst >> @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. >> >> Helper functions can be used to find a link between two given pads, or a pad >> connected to another pad through an enabled link >> -:c:func:`media_entity_find_link()` and >> -:c:func:`media_entity_remote_pad()`. >> +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and >> +:c:func:`media_entity_remote_source_pad()`). >> >> Use count and power handling >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c >> index 11f5207f73aa..1febf5a86be6 100644 >> --- a/drivers/media/mc/mc-entity.c >> +++ b/drivers/media/mc/mc-entity.c >> @@ -9,6 +9,7 @@ >> */ >> >> #include <linux/bitmap.h> >> +#include <linux/list.h> >> #include <linux/property.h> >> #include <linux/slab.h> >> #include <media/media-entity.h> >> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) >> } >> EXPORT_SYMBOL_GPL(media_entity_remote_pad); >> >> +struct media_pad * >> +media_entity_remote_pad_unique(const struct media_entity *entity, >> + unsigned int type) >> +{ >> + struct media_pad *pad = NULL; >> + struct media_link *link; >> + >> + list_for_each_entry(link, &entity->links, list) { >> + struct media_pad *local_pad; >> + struct media_pad *remote_pad; >> + >> + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) >> + continue; > Does this need another guard here to make sure the link isn't an > ancillary link? Only likely to happen at least in the immediate future > where the entity represents a camera sensor, so possibly not applicable > here - I couldn't find where the new function is used in the series to > check. I _think_ it would actually work ok regardless...media_gobj > type-punning makes my brain ache, but I think the local_pad->entity == > entity comparison would actually compare the entity->name member of the > entity at the end of an ancillary link to the entity parameter, not find > a match and so continue the loop without failing, but that feels a bit > sub-optimal. > Or perhaps a better approach would be to provide something like a "list_for_each_data_link()" iterator - the potential problem here (as with Quentin's recent issue with the rkisp1 driver) is the assumption that all links are data links, so maybe it's best to just guarantee that if we can. > >> + >> + if (type == MEDIA_PAD_FL_SOURCE) { >> + local_pad = link->sink; >> + remote_pad = link->source; >> + } else { >> + local_pad = link->source; >> + remote_pad = link->sink; >> + } >> + >> + if (local_pad->entity == entity) { >> + if (pad) >> + return ERR_PTR(-ENOTUNIQ); >> + >> + pad = remote_pad; >> + } >> + } >> + >> + if (!pad) >> + return ERR_PTR(-ENOLINK); >> + >> + return pad; >> +} >> +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); >> + >> static void media_interface_init(struct media_device *mdev, >> struct media_interface *intf, >> u32 gobj_type, >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h >> index a9a1c0ec5d1c..33d5f52719a0 100644 >> --- a/include/media/media-entity.h >> +++ b/include/media/media-entity.h >> @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, >> */ >> struct media_pad *media_entity_remote_pad(const struct media_pad *pad); >> >> +/** >> + * media_entity_remote_pad_unique - Find a remote pad connected to an entity >> + * @entity: The entity >> + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) >> + * >> + * Search for and return a remote pad of @type connected to @entity through an >> + * enabled link. If multiple (or no) remote pads match these criteria, an error >> + * is returned. >> + * >> + * The uniqueness constraint makes this helper function suitable for entities >> + * that support a single active source or sink at a time. >> + * >> + * Return: A pointer to the remote pad, or one of the following error pointers >> + * if an error occurs: >> + * >> + * * -ENOTUNIQ - Multiple links are enabled >> + * * -ENOLINK - No connected pad found >> + */ >> +struct media_pad * >> +media_entity_remote_pad_unique(const struct media_entity *entity, >> + unsigned int type); >> + >> +/** >> + * media_entity_remote_source_pad - Find a remote source pad connected to an entity >> + * @entity: The entity >> + * >> + * Search for and return a remote source pad connected to @entity through an >> + * enabled link. If multiple (or no) remote pads match these criteria, an error >> + * is returned. >> + * >> + * The uniqueness constraint makes this helper function suitable for entities >> + * that support a single active source at a time. >> + * >> + * Return: A pointer to the remote pad, or one of the following error pointers >> + * if an error occurs: >> + * >> + * * -ENOTUNIQ - Multiple links are enabled >> + * * -ENOLINK - No connected pad found >> + */ >> +static inline struct media_pad * >> +media_entity_remote_source_pad(const struct media_entity *entity) >> +{ >> + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); >> +} >> + >> /** >> * media_entity_is_streaming - Test if an entity is part of a streaming pipeline >> * @entity: The entity
Hi Daniel, On Fri, Jun 17, 2022 at 11:33:03PM +0100, Daniel Scally wrote: > On 17/06/2022 22:34, Daniel Scally wrote: > > On 14/06/2022 20:11, Paul Elder wrote: > >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> > >> The media_entity_remote_pad() helper function returns the first remote > >> pad it find connected to a given pad. Beside being possibly ill-named > >> (as it operates on a pad, not an entity) and non-deterministic (as it > >> stops at the first enabled link), the fact that it returns the first > >> match makes it unsuitable for drivers that need to guarantee that a > >> single link is enabled, for instance when an entity can process data > >> from one of multiple sources at a time. > >> > >> For those use cases, add a new helper function, > >> media_entity_remote_pad_unique(), that operates on an entity and returns > >> a remote pad, with a guarantee that only one link is enabled. To ease > >> its use in drivers, also add an inline wrapper that locates source pads > >> specifically. A wrapper that locates sink pads can easily be added when > >> needed. > >> > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- > >> Documentation/driver-api/media/mc-core.rst | 4 +- > >> drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > >> include/media/media-entity.h | 45 ++++++++++++++++++++++ > >> 3 files changed, 85 insertions(+), 2 deletions(-) > >> > >> diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > >> index 02481a2513b9..a2d1e32e3abb 100644 > >> --- a/Documentation/driver-api/media/mc-core.rst > >> +++ b/Documentation/driver-api/media/mc-core.rst > >> @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > >> > >> Helper functions can be used to find a link between two given pads, or a pad > >> connected to another pad through an enabled link > >> -:c:func:`media_entity_find_link()` and > >> -:c:func:`media_entity_remote_pad()`. > >> +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > >> +:c:func:`media_entity_remote_source_pad()`). > >> > >> Use count and power handling > >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > >> index 11f5207f73aa..1febf5a86be6 100644 > >> --- a/drivers/media/mc/mc-entity.c > >> +++ b/drivers/media/mc/mc-entity.c > >> @@ -9,6 +9,7 @@ > >> */ > >> > >> #include <linux/bitmap.h> > >> +#include <linux/list.h> > >> #include <linux/property.h> > >> #include <linux/slab.h> > >> #include <media/media-entity.h> > >> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > >> } > >> EXPORT_SYMBOL_GPL(media_entity_remote_pad); > >> > >> +struct media_pad * > >> +media_entity_remote_pad_unique(const struct media_entity *entity, > >> + unsigned int type) > >> +{ > >> + struct media_pad *pad = NULL; > >> + struct media_link *link; > >> + > >> + list_for_each_entry(link, &entity->links, list) { > >> + struct media_pad *local_pad; > >> + struct media_pad *remote_pad; > >> + > >> + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > >> + continue; > > > > Does this need another guard here to make sure the link isn't an > > ancillary link? Only likely to happen at least in the immediate future > > where the entity represents a camera sensor, so possibly not applicable > > here - I couldn't find where the new function is used in the series to > > check. I _think_ it would actually work ok regardless...media_gobj > > type-punning makes my brain ache, but I think the local_pad->entity == > > entity comparison would actually compare the entity->name member of the > > entity at the end of an ancillary link to the entity parameter, not find > > a match and so continue the loop without failing, but that feels a bit > > sub-optimal. > > Or perhaps a better approach would be to provide something like a > "list_for_each_data_link()" iterator - the potential problem here (as > with Quentin's recent issue with the rkisp1 driver) is the assumption > that all links are data links, so maybe it's best to just guarantee that > if we can. I agree with you, without a dedicated iterator, we're bound to repeat the mistake over and over. Would you like to submit a patch to add that iterator, or should I ? I'd name it for_each_media_entity_data_link() or something similar. > >> + > >> + if (type == MEDIA_PAD_FL_SOURCE) { > >> + local_pad = link->sink; > >> + remote_pad = link->source; > >> + } else { > >> + local_pad = link->source; > >> + remote_pad = link->sink; > >> + } > >> + > >> + if (local_pad->entity == entity) { > >> + if (pad) > >> + return ERR_PTR(-ENOTUNIQ); > >> + > >> + pad = remote_pad; > >> + } > >> + } > >> + > >> + if (!pad) > >> + return ERR_PTR(-ENOLINK); > >> + > >> + return pad; > >> +} > >> +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > >> + > >> static void media_interface_init(struct media_device *mdev, > >> struct media_interface *intf, > >> u32 gobj_type, > >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h > >> index a9a1c0ec5d1c..33d5f52719a0 100644 > >> --- a/include/media/media-entity.h > >> +++ b/include/media/media-entity.h > >> @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > >> */ > >> struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > >> > >> +/** > >> + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > >> + * @entity: The entity > >> + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > >> + * > >> + * Search for and return a remote pad of @type connected to @entity through an > >> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >> + * is returned. > >> + * > >> + * The uniqueness constraint makes this helper function suitable for entities > >> + * that support a single active source or sink at a time. > >> + * > >> + * Return: A pointer to the remote pad, or one of the following error pointers > >> + * if an error occurs: > >> + * > >> + * * -ENOTUNIQ - Multiple links are enabled > >> + * * -ENOLINK - No connected pad found > >> + */ > >> +struct media_pad * > >> +media_entity_remote_pad_unique(const struct media_entity *entity, > >> + unsigned int type); > >> + > >> +/** > >> + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > >> + * @entity: The entity > >> + * > >> + * Search for and return a remote source pad connected to @entity through an > >> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >> + * is returned. > >> + * > >> + * The uniqueness constraint makes this helper function suitable for entities > >> + * that support a single active source at a time. > >> + * > >> + * Return: A pointer to the remote pad, or one of the following error pointers > >> + * if an error occurs: > >> + * > >> + * * -ENOTUNIQ - Multiple links are enabled > >> + * * -ENOLINK - No connected pad found > >> + */ > >> +static inline struct media_pad * > >> +media_entity_remote_source_pad(const struct media_entity *entity) > >> +{ > >> + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > >> +} > >> + > >> /** > >> * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > >> * @entity: The entity
Hi Alexander, On Thu, Jun 16, 2022 at 10:05:06AM +0200, Alexander Stein wrote: > Am Dienstag, 14. Juni 2022, 21:11:27 CEST schrieb Paul Elder: > > The ISP that is integrated in the i.MX8MP uses different bits in the > > MRSZ_CTRL and SRSZ_CTRL registers for updating the configuration > > compared to the on in the RK3399. In addition, it adds a new bit for > > enabling crop. Add new definitions for these bits for i.MX8MP devices, > > and update where they are set. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h | 4 ++++ > > .../media/platform/rockchip/rkisp1/rkisp1-resizer.c | 10 ++++++++-- > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h index > > 34f4fe09c88d..24ad2ccec2a3 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > @@ -168,6 +168,10 @@ > > #define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO BIT(9) > > #define RKISP1_CIF_RSZ_SCALER_FACTOR BIT(16) > > > > +#define RKISP1_CIF_RSZ_CTRL_CROP_ENABLE_IMX BIT(8) > > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX BIT(9) > > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX BIT(10) > > + > > Does it make sense to move this kind of information into struct rkisp1_info? > This way you can skip the if (isp_ver == ...) thing. Good question. Paul, what do you think ? If it doesn't get moved to the structure, I think I'd condition it by the RKISP1_FEATURE_RSZ_CROP feature bit instead of a version check, as it seems closely related. I'm actually leaning towards the latter. > > /* RSZ_CROP_[XY]_DIR */ > > #define RKISP1_CIF_RSZ_CROP_XY_DIR(start, end) ((end) << 16 | (start) << 0) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c index > > 08bf3aa8088f..29a31b18a082 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > @@ -209,9 +209,15 @@ static void rkisp1_rsz_update_shadow(struct > > rkisp1_resizer *rsz, u32 ctrl_cfg = rkisp1_rsz_read(rsz, > > RKISP1_CIF_RSZ_CTRL); > > > > if (when == RKISP1_SHADOW_REGS_ASYNC) > > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX; > > + else > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > > else > > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX; > > + else > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > > > > rkisp1_rsz_write(rsz, RKISP1_CIF_RSZ_CTRL, ctrl_cfg); > > }
Hi Paul, Thank you for the patch. On Wed, Jun 15, 2022 at 04:11:15AM +0900, Paul Elder wrote: > The rkisp1 can take an input on the parallel interface. Add a port for > it, and update the required field. At least one port is required, and > both may be specified. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../bindings/media/rockchip-isp1.yaml | 23 +++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > index d1489b177331..b3661d7d4357 100644 > --- a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > +++ b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > @@ -84,8 +84,27 @@ properties: > minItems: 1 > maxItems: 4 > > - required: > - - port@0 > + port@1: > + $ref: /schemas/graph.yaml#/$defs/port-base > + unevaluatedProperties: false > + description: connection point for input on the parallel interface > + > + properties: > + bus-type: > + enum: [5, 6] > + > + endpoint: > + $ref: video-interfaces.yaml# > + unevaluatedProperties: false > + > + required: > + - bus-type > + > + anyOf: > + - required: > + - port@0 > + - required: > + - port@1 > > required: > - compatible
Hi Paul, Thank you for the patch. On Wed, Jun 15, 2022 at 04:11:19AM +0900, Paul Elder wrote: > The i.MX8MP ISP is compatbile with the rkisp1 driver. Add it to the list > of compatible strings. While at it, expand on the description of the > clocks to make it clear which clock in the i.MX8MP ISP they map to, > based on the names from the datasheet (which are confusing). > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/devicetree/bindings/media/rockchip-isp1.yaml | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > index b3661d7d4357..95cf945f7ac5 100644 > --- a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > +++ b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > @@ -16,6 +16,7 @@ description: | > properties: > compatible: > enum: > + - fsl,imx8mp-isp > - rockchip,px30-cif-isp > - rockchip,rk3399-cif-isp > > @@ -36,9 +37,9 @@ properties: > minItems: 3 > items: > # isp0 and isp1 > - - description: ISP clock > - - description: ISP AXI clock > - - description: ISP AHB clock > + - description: ISP clock (for imx8mp, clk) > + - description: ISP AXI clock (for imx8mp, m_hclk) > + - description: ISP AHB clock (for imx8mp, hclk) > # only for isp1 > - description: ISP Pixel clock >
Morning Laurent On 17/06/2022 23:40, Laurent Pinchart wrote: > Hi Daniel, > > On Fri, Jun 17, 2022 at 11:33:03PM +0100, Daniel Scally wrote: >> On 17/06/2022 22:34, Daniel Scally wrote: >>> On 14/06/2022 20:11, Paul Elder wrote: >>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> >>>> The media_entity_remote_pad() helper function returns the first remote >>>> pad it find connected to a given pad. Beside being possibly ill-named >>>> (as it operates on a pad, not an entity) and non-deterministic (as it >>>> stops at the first enabled link), the fact that it returns the first >>>> match makes it unsuitable for drivers that need to guarantee that a >>>> single link is enabled, for instance when an entity can process data >>>> from one of multiple sources at a time. >>>> >>>> For those use cases, add a new helper function, >>>> media_entity_remote_pad_unique(), that operates on an entity and returns >>>> a remote pad, with a guarantee that only one link is enabled. To ease >>>> its use in drivers, also add an inline wrapper that locates source pads >>>> specifically. A wrapper that locates sink pads can easily be added when >>>> needed. >>>> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> --- >>>> Documentation/driver-api/media/mc-core.rst | 4 +- >>>> drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ >>>> include/media/media-entity.h | 45 ++++++++++++++++++++++ >>>> 3 files changed, 85 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst >>>> index 02481a2513b9..a2d1e32e3abb 100644 >>>> --- a/Documentation/driver-api/media/mc-core.rst >>>> +++ b/Documentation/driver-api/media/mc-core.rst >>>> @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. >>>> >>>> Helper functions can be used to find a link between two given pads, or a pad >>>> connected to another pad through an enabled link >>>> -:c:func:`media_entity_find_link()` and >>>> -:c:func:`media_entity_remote_pad()`. >>>> +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and >>>> +:c:func:`media_entity_remote_source_pad()`). >>>> >>>> Use count and power handling >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c >>>> index 11f5207f73aa..1febf5a86be6 100644 >>>> --- a/drivers/media/mc/mc-entity.c >>>> +++ b/drivers/media/mc/mc-entity.c >>>> @@ -9,6 +9,7 @@ >>>> */ >>>> >>>> #include <linux/bitmap.h> >>>> +#include <linux/list.h> >>>> #include <linux/property.h> >>>> #include <linux/slab.h> >>>> #include <media/media-entity.h> >>>> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) >>>> } >>>> EXPORT_SYMBOL_GPL(media_entity_remote_pad); >>>> >>>> +struct media_pad * >>>> +media_entity_remote_pad_unique(const struct media_entity *entity, >>>> + unsigned int type) >>>> +{ >>>> + struct media_pad *pad = NULL; >>>> + struct media_link *link; >>>> + >>>> + list_for_each_entry(link, &entity->links, list) { >>>> + struct media_pad *local_pad; >>>> + struct media_pad *remote_pad; >>>> + >>>> + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) >>>> + continue; >>> Does this need another guard here to make sure the link isn't an >>> ancillary link? Only likely to happen at least in the immediate future >>> where the entity represents a camera sensor, so possibly not applicable >>> here - I couldn't find where the new function is used in the series to >>> check. I _think_ it would actually work ok regardless...media_gobj >>> type-punning makes my brain ache, but I think the local_pad->entity == >>> entity comparison would actually compare the entity->name member of the >>> entity at the end of an ancillary link to the entity parameter, not find >>> a match and so continue the loop without failing, but that feels a bit >>> sub-optimal. >> Or perhaps a better approach would be to provide something like a >> "list_for_each_data_link()" iterator - the potential problem here (as >> with Quentin's recent issue with the rkisp1 driver) is the assumption >> that all links are data links, so maybe it's best to just guarantee that >> if we can. > I agree with you, without a dedicated iterator, we're bound to repeat > the mistake over and over. > > Would you like to submit a patch to add that iterator, or should I ? I'd > name it for_each_media_entity_data_link() or something similar. I've started one - I'll send it later (using your function name though, naming things was never my strong suit!) > >>>> + >>>> + if (type == MEDIA_PAD_FL_SOURCE) { >>>> + local_pad = link->sink; >>>> + remote_pad = link->source; >>>> + } else { >>>> + local_pad = link->source; >>>> + remote_pad = link->sink; >>>> + } >>>> + >>>> + if (local_pad->entity == entity) { >>>> + if (pad) >>>> + return ERR_PTR(-ENOTUNIQ); >>>> + >>>> + pad = remote_pad; >>>> + } >>>> + } >>>> + >>>> + if (!pad) >>>> + return ERR_PTR(-ENOLINK); >>>> + >>>> + return pad; >>>> +} >>>> +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); >>>> + >>>> static void media_interface_init(struct media_device *mdev, >>>> struct media_interface *intf, >>>> u32 gobj_type, >>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h >>>> index a9a1c0ec5d1c..33d5f52719a0 100644 >>>> --- a/include/media/media-entity.h >>>> +++ b/include/media/media-entity.h >>>> @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, >>>> */ >>>> struct media_pad *media_entity_remote_pad(const struct media_pad *pad); >>>> >>>> +/** >>>> + * media_entity_remote_pad_unique - Find a remote pad connected to an entity >>>> + * @entity: The entity >>>> + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) >>>> + * >>>> + * Search for and return a remote pad of @type connected to @entity through an >>>> + * enabled link. If multiple (or no) remote pads match these criteria, an error >>>> + * is returned. >>>> + * >>>> + * The uniqueness constraint makes this helper function suitable for entities >>>> + * that support a single active source or sink at a time. >>>> + * >>>> + * Return: A pointer to the remote pad, or one of the following error pointers >>>> + * if an error occurs: >>>> + * >>>> + * * -ENOTUNIQ - Multiple links are enabled >>>> + * * -ENOLINK - No connected pad found >>>> + */ >>>> +struct media_pad * >>>> +media_entity_remote_pad_unique(const struct media_entity *entity, >>>> + unsigned int type); >>>> + >>>> +/** >>>> + * media_entity_remote_source_pad - Find a remote source pad connected to an entity >>>> + * @entity: The entity >>>> + * >>>> + * Search for and return a remote source pad connected to @entity through an >>>> + * enabled link. If multiple (or no) remote pads match these criteria, an error >>>> + * is returned. >>>> + * >>>> + * The uniqueness constraint makes this helper function suitable for entities >>>> + * that support a single active source at a time. >>>> + * >>>> + * Return: A pointer to the remote pad, or one of the following error pointers >>>> + * if an error occurs: >>>> + * >>>> + * * -ENOTUNIQ - Multiple links are enabled >>>> + * * -ENOLINK - No connected pad found >>>> + */ >>>> +static inline struct media_pad * >>>> +media_entity_remote_source_pad(const struct media_entity *entity) >>>> +{ >>>> + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); >>>> +} >>>> + >>>> /** >>>> * media_entity_is_streaming - Test if an entity is part of a streaming pipeline >>>> * @entity: The entity
On 6/14/22 21:10, Paul Elder wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Drivers typically extend the v4l2_async_subdev structure by embedding it > in a driver-specific structure, to store per-subdev custom data. The > v4l2_async_subdev instances are freed by the v4l2-async framework, which > makes this mechanism cumbersome to use safely when custom data needs > special treatment to be destroyed (such as freeing additional memory, or > releasing references to kernel objects). > > To ease this, add a .destroy() operation to the > v4l2_async_notifier_operations structure. The operation is called right > before the v4l2_async_subdev is freed, giving drivers a chance to > destroy data if needed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Looks good! Regards, Hans > --- > Documentation/driver-api/media/v4l2-subdev.rst | 6 ++++++ > drivers/media/v4l2-core/v4l2-async.c | 10 ++++++++++ > include/media/v4l2-async.h | 2 ++ > 3 files changed, 18 insertions(+) > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > index cf3b52bdbfb9..6f8d79926aa5 100644 > --- a/Documentation/driver-api/media/v4l2-subdev.rst > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > @@ -243,6 +243,12 @@ notifier callback is called. After all subdevices have been located the > .complete() callback is called. When a subdevice is removed from the > system the .unbind() method is called. All three callbacks are optional. > > +Drivers can store any type of custom data in their driver-specific > +:c:type:`v4l2_async_subdev` wrapper. If any of that data requires special > +handling when the structure is freed, drivers must implement the ``.destroy()`` > +notifier callback. The framework will call it right before freeing the > +:c:type:`v4l2_async_subdev`. > + > Calling subdev operations > ~~~~~~~~~~~~~~~~~~~~~~~~~ > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index c6995718237a..735dede624b8 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -52,6 +52,15 @@ static int v4l2_async_nf_call_complete(struct v4l2_async_notifier *n) > return n->ops->complete(n); > } > > +static void v4l2_async_nf_call_destroy(struct v4l2_async_notifier *n, > + struct v4l2_async_subdev *asd) > +{ > + if (!n->ops || !n->ops->destroy) > + return; > + > + n->ops->destroy(asd); > +} > + > static bool match_i2c(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > { > @@ -626,6 +635,7 @@ static void __v4l2_async_nf_cleanup(struct v4l2_async_notifier *notifier) > } > > list_del(&asd->asd_list); > + v4l2_async_nf_call_destroy(notifier, asd); > kfree(asd); > } > } > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index 13ff3ad948f4..25eb1d138c06 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -81,6 +81,7 @@ struct v4l2_async_subdev { > * @complete: All subdevices have been probed successfully. The complete > * callback is only executed for the root notifier. > * @unbind: a subdevice is leaving > + * @destroy: the asd is about to be freed > */ > struct v4l2_async_notifier_operations { > int (*bound)(struct v4l2_async_notifier *notifier, > @@ -90,6 +91,7 @@ struct v4l2_async_notifier_operations { > void (*unbind)(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *subdev, > struct v4l2_async_subdev *asd); > + void (*destroy)(struct v4l2_async_subdev *asd); > }; > > /**
On 15.06.2022 04:10, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >If the v4l2_device_register() call fails, runtime PM is left enabled. >Fix it. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com> >--- > drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >index 97d569968285..248f0172ca62 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >@@ -523,7 +523,7 @@ static int rkisp1_probe(struct platform_device *pdev) > > ret = v4l2_device_register(rkisp1->dev, &rkisp1->v4l2_dev); > if (ret) >- return ret; >+ goto err_pm_runtime_disable; > > ret = media_device_register(&rkisp1->media_dev); > if (ret) { >@@ -543,6 +543,7 @@ static int rkisp1_probe(struct platform_device *pdev) > media_device_unregister(&rkisp1->media_dev); > err_unreg_v4l2_dev: > v4l2_device_unregister(&rkisp1->v4l2_dev); >+err_pm_runtime_disable: > pm_runtime_disable(&pdev->dev); > return ret; > } >-- >2.30.2 >
On 15.06.2022 04:10, Paul Elder wrote: >The upcoming CSI receiver split from the ISP to a separate source file >will need to be able to access the list of formats supported by the >driver. Move it out of the ISP's header and into the common header, and >add helper functions for accessing it so that the format list doesn't >need to be stored in the header. > >Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by Dafna Hirschfeld <dafna@fastmail.com> >--- > .../platform/rockchip/rkisp1/rkisp1-common.c | 148 +++++++++++++++ > .../platform/rockchip/rkisp1/rkisp1-common.h | 28 ++- > .../platform/rockchip/rkisp1/rkisp1-isp.c | 168 ++---------------- > .../platform/rockchip/rkisp1/rkisp1-resizer.c | 14 +- > .../platform/rockchip/rkisp1/rkisp1-stats.c | 2 +- > 5 files changed, 193 insertions(+), 167 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c >index cf889666e166..bb0ea20118e1 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c >@@ -5,10 +5,158 @@ > * Copyright (C) 2019 Collabora, Ltd. > */ > >+#include <media/mipi-csi2.h> > #include <media/v4l2-rect.h> > > #include "rkisp1-common.h" > >+static const struct rkisp1_mbus_info rkisp1_formats[] = { >+ { >+ .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8, >+ .pixel_enc = V4L2_PIXEL_ENC_YUV, >+ .direction = RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW10, >+ .bayer_pat = RKISP1_RAW_RGGB, >+ .bus_width = 10, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW10, >+ .bayer_pat = RKISP1_RAW_BGGR, >+ .bus_width = 10, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW10, >+ .bayer_pat = RKISP1_RAW_GBRG, >+ .bus_width = 10, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW10, >+ .bayer_pat = RKISP1_RAW_GRBG, >+ .bus_width = 10, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW12, >+ .bayer_pat = RKISP1_RAW_RGGB, >+ .bus_width = 12, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW12, >+ .bayer_pat = RKISP1_RAW_BGGR, >+ .bus_width = 12, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW12, >+ .bayer_pat = RKISP1_RAW_GBRG, >+ .bus_width = 12, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW12, >+ .bayer_pat = RKISP1_RAW_GRBG, >+ .bus_width = 12, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW8, >+ .bayer_pat = RKISP1_RAW_RGGB, >+ .bus_width = 8, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW8, >+ .bayer_pat = RKISP1_RAW_BGGR, >+ .bus_width = 8, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW8, >+ .bayer_pat = RKISP1_RAW_GBRG, >+ .bus_width = 8, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8, >+ .pixel_enc = V4L2_PIXEL_ENC_BAYER, >+ .mipi_dt = MIPI_CSI2_DT_RAW8, >+ .bayer_pat = RKISP1_RAW_GRBG, >+ .bus_width = 8, >+ .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16, >+ .pixel_enc = V4L2_PIXEL_ENC_YUV, >+ .mipi_dt = MIPI_CSI2_DT_YUV422_8B, >+ .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_YCBYCR, >+ .bus_width = 16, >+ .direction = RKISP1_ISP_SD_SINK, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16, >+ .pixel_enc = V4L2_PIXEL_ENC_YUV, >+ .mipi_dt = MIPI_CSI2_DT_YUV422_8B, >+ .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_YCRYCB, >+ .bus_width = 16, >+ .direction = RKISP1_ISP_SD_SINK, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16, >+ .pixel_enc = V4L2_PIXEL_ENC_YUV, >+ .mipi_dt = MIPI_CSI2_DT_YUV422_8B, >+ .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_CBYCRY, >+ .bus_width = 16, >+ .direction = RKISP1_ISP_SD_SINK, >+ }, { >+ .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16, >+ .pixel_enc = V4L2_PIXEL_ENC_YUV, >+ .mipi_dt = MIPI_CSI2_DT_YUV422_8B, >+ .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_CRYCBY, >+ .bus_width = 16, >+ .direction = RKISP1_ISP_SD_SINK, >+ }, >+}; >+ >+unsigned int rkisp1_mbus_info_length(void) >+{ >+ return ARRAY_SIZE(rkisp1_formats); >+} >+ >+const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_index(unsigned int index) >+{ >+ if (index >= rkisp1_mbus_info_length()) >+ return NULL; >+ >+ return &rkisp1_formats[index]; >+} >+ >+const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_code(u32 mbus_code) >+{ >+ unsigned int i; >+ >+ for (i = 0; i < ARRAY_SIZE(rkisp1_formats); i++) { >+ const struct rkisp1_mbus_info *fmt = &rkisp1_formats[i]; >+ >+ if (fmt->mbus_code == mbus_code) >+ return fmt; >+ } >+ >+ return NULL; >+} >+ > static const struct v4l2_rect rkisp1_sd_min_crop = { > .width = RKISP1_ISP_MIN_WIDTH, > .height = RKISP1_ISP_MIN_HEIGHT, >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >index 50d31a254b03..c7d5c57607bd 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >@@ -26,7 +26,7 @@ > struct dentry; > > /* >- * flags on the 'direction' field in struct 'rkisp1_isp_mbus_info' that indicate >+ * flags on the 'direction' field in struct rkisp1_mbus_info' that indicate > * on which pad the media bus format is supported > */ > #define RKISP1_ISP_SD_SRC BIT(0) >@@ -150,8 +150,8 @@ struct rkisp1_isp { > struct v4l2_subdev sd; > struct media_pad pads[RKISP1_ISP_PAD_MAX]; > struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX]; >- const struct rkisp1_isp_mbus_info *sink_fmt; >- const struct rkisp1_isp_mbus_info *src_fmt; >+ const struct rkisp1_mbus_info *sink_fmt; >+ const struct rkisp1_mbus_info *src_fmt; > struct mutex ops_lock; /* serialize the subdevice ops */ > bool is_dphy_errctrl_disabled; > __u32 frame_sequence; >@@ -438,8 +438,8 @@ struct rkisp1_device { > }; > > /* >- * struct rkisp1_isp_mbus_info - ISP media bus info, Translates media bus code to hardware >- * format values >+ * struct rkisp1_mbus_info - ISP media bus info, Translates media bus code to hardware >+ * format values > * > * @mbus_code: media bus code > * @pixel_enc: pixel encoding >@@ -449,7 +449,7 @@ struct rkisp1_device { > * @bayer_pat: bayer pattern > * @direction: a bitmask of the flags indicating on which pad the format is supported on > */ >-struct rkisp1_isp_mbus_info { >+struct rkisp1_mbus_info { > u32 mbus_code; > enum v4l2_pixel_encoding pixel_enc; > u32 mipi_dt; >@@ -481,6 +481,18 @@ static inline u32 rkisp1_read(struct rkisp1_device *rkisp1, unsigned int addr) > int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap, > struct v4l2_subdev_mbus_code_enum *code); > >+/* >+ * rkisp1_mbus_info_length - Return the number of supported mbus codes >+ */ >+unsigned int rkisp1_mbus_info_length(void); >+ >+/* >+ * rkisp1_mbus_info_get_by_index - Retrieve the ith supported mbus info >+ * >+ * @index: index of the mbus info to fetch >+ */ >+const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_index(unsigned int index); >+ > /* > * rkisp1_sd_adjust_crop_rect - adjust a rectangle to fit into another rectangle. > * >@@ -500,11 +512,11 @@ void rkisp1_sd_adjust_crop(struct v4l2_rect *crop, > const struct v4l2_mbus_framefmt *bounds); > > /* >- * rkisp1_isp_mbus_info - get the isp info of the media bus code >+ * rkisp1_mbus_info_get_by_code - get the isp info of the media bus code > * > * @mbus_code: the media bus code > */ >-const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code); >+const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_code(u32 mbus_code); > > /* rkisp1_params_configure - configure the params when stream starts. > * This function is called by the isp entity upon stream starts. >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >index 328e8fec14e9..89577119b571 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >@@ -15,7 +15,6 @@ > #include <linux/videodev2.h> > #include <linux/vmalloc.h> > >-#include <media/mipi-csi2.h> > #include <media/v4l2-event.h> > > #include "rkisp1-common.h" >@@ -56,144 +55,10 @@ > * +---------------------------------------------------------+ > */ > >-static const struct rkisp1_isp_mbus_info rkisp1_isp_formats[] = { >- { >- .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8, >- .pixel_enc = V4L2_PIXEL_ENC_YUV, >- .direction = RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW10, >- .bayer_pat = RKISP1_RAW_RGGB, >- .bus_width = 10, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW10, >- .bayer_pat = RKISP1_RAW_BGGR, >- .bus_width = 10, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW10, >- .bayer_pat = RKISP1_RAW_GBRG, >- .bus_width = 10, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW10, >- .bayer_pat = RKISP1_RAW_GRBG, >- .bus_width = 10, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW12, >- .bayer_pat = RKISP1_RAW_RGGB, >- .bus_width = 12, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW12, >- .bayer_pat = RKISP1_RAW_BGGR, >- .bus_width = 12, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW12, >- .bayer_pat = RKISP1_RAW_GBRG, >- .bus_width = 12, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW12, >- .bayer_pat = RKISP1_RAW_GRBG, >- .bus_width = 12, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW8, >- .bayer_pat = RKISP1_RAW_RGGB, >- .bus_width = 8, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW8, >- .bayer_pat = RKISP1_RAW_BGGR, >- .bus_width = 8, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW8, >- .bayer_pat = RKISP1_RAW_GBRG, >- .bus_width = 8, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8, >- .pixel_enc = V4L2_PIXEL_ENC_BAYER, >- .mipi_dt = MIPI_CSI2_DT_RAW8, >- .bayer_pat = RKISP1_RAW_GRBG, >- .bus_width = 8, >- .direction = RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC, >- }, { >- .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16, >- .pixel_enc = V4L2_PIXEL_ENC_YUV, >- .mipi_dt = MIPI_CSI2_DT_YUV422_8B, >- .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_YCBYCR, >- .bus_width = 16, >- .direction = RKISP1_ISP_SD_SINK, >- }, { >- .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16, >- .pixel_enc = V4L2_PIXEL_ENC_YUV, >- .mipi_dt = MIPI_CSI2_DT_YUV422_8B, >- .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_YCRYCB, >- .bus_width = 16, >- .direction = RKISP1_ISP_SD_SINK, >- }, { >- .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16, >- .pixel_enc = V4L2_PIXEL_ENC_YUV, >- .mipi_dt = MIPI_CSI2_DT_YUV422_8B, >- .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_CBYCRY, >- .bus_width = 16, >- .direction = RKISP1_ISP_SD_SINK, >- }, { >- .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16, >- .pixel_enc = V4L2_PIXEL_ENC_YUV, >- .mipi_dt = MIPI_CSI2_DT_YUV422_8B, >- .yuv_seq = RKISP1_CIF_ISP_ACQ_PROP_CRYCBY, >- .bus_width = 16, >- .direction = RKISP1_ISP_SD_SINK, >- }, >-}; >- > /* ---------------------------------------------------------------------------- > * Helpers > */ > >-const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code) >-{ >- unsigned int i; >- >- for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) { >- const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i]; >- >- if (fmt->mbus_code == mbus_code) >- return fmt; >- } >- >- return NULL; >-} >- > static struct v4l2_subdev *rkisp1_get_remote_sensor(struct v4l2_subdev *sd) > { > struct media_pad *local, *remote; >@@ -275,7 +140,7 @@ static void rkisp1_config_ism(struct rkisp1_device *rkisp1) > static int rkisp1_config_isp(struct rkisp1_device *rkisp1) > { > u32 isp_ctrl = 0, irq_mask = 0, acq_mult = 0, signal = 0; >- const struct rkisp1_isp_mbus_info *src_fmt, *sink_fmt; >+ const struct rkisp1_mbus_info *src_fmt, *sink_fmt; > struct rkisp1_sensor_async *sensor; > struct v4l2_mbus_framefmt *sink_frm; > struct v4l2_rect *sink_crop; >@@ -376,7 +241,7 @@ static int rkisp1_config_isp(struct rkisp1_device *rkisp1) > > static int rkisp1_config_dvp(struct rkisp1_device *rkisp1) > { >- const struct rkisp1_isp_mbus_info *sink_fmt = rkisp1->isp.sink_fmt; >+ const struct rkisp1_mbus_info *sink_fmt = rkisp1->isp.sink_fmt; > u32 val, input_sel; > > switch (sink_fmt->bus_width) { >@@ -402,7 +267,7 @@ static int rkisp1_config_dvp(struct rkisp1_device *rkisp1) > > static int rkisp1_config_mipi(struct rkisp1_device *rkisp1) > { >- const struct rkisp1_isp_mbus_info *sink_fmt = rkisp1->isp.sink_fmt; >+ const struct rkisp1_mbus_info *sink_fmt = rkisp1->isp.sink_fmt; > unsigned int lanes = rkisp1->active_sensor->lanes; > u32 mipi_ctrl; > >@@ -593,11 +458,12 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd, > return 0; > } > >- if (code->index >= ARRAY_SIZE(rkisp1_isp_formats)) >+ if (code->index >= rkisp1_mbus_info_length()) > return -EINVAL; > >- for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) { >- const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i]; >+ for (i = 0; i < rkisp1_mbus_info_length(); i++) { >+ const struct rkisp1_mbus_info *fmt = >+ rkisp1_mbus_info_get_by_index(i); > > if (fmt->direction & dir) > pos++; >@@ -619,7 +485,7 @@ static int rkisp1_isp_enum_frame_size(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_size_enum *fse) > { >- const struct rkisp1_isp_mbus_info *mbus_info; >+ const struct rkisp1_mbus_info *mbus_info; > > if (fse->pad == RKISP1_ISP_PAD_SINK_PARAMS || > fse->pad == RKISP1_ISP_PAD_SOURCE_STATS) >@@ -628,7 +494,7 @@ static int rkisp1_isp_enum_frame_size(struct v4l2_subdev *sd, > if (fse->index > 0) > return -EINVAL; > >- mbus_info = rkisp1_isp_mbus_info_get(fse->code); >+ mbus_info = rkisp1_mbus_info_get_by_code(fse->code); > if (!mbus_info) > return -EINVAL; > >@@ -695,7 +561,7 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, > struct v4l2_mbus_framefmt *format, > unsigned int which) > { >- const struct rkisp1_isp_mbus_info *mbus_info; >+ const struct rkisp1_mbus_info *mbus_info; > struct v4l2_mbus_framefmt *src_fmt; > const struct v4l2_rect *src_crop; > >@@ -705,10 +571,10 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, > RKISP1_ISP_PAD_SOURCE_VIDEO, which); > > src_fmt->code = format->code; >- mbus_info = rkisp1_isp_mbus_info_get(src_fmt->code); >+ mbus_info = rkisp1_mbus_info_get_by_code(src_fmt->code); > if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) { > src_fmt->code = RKISP1_DEF_SRC_PAD_FMT; >- mbus_info = rkisp1_isp_mbus_info_get(src_fmt->code); >+ mbus_info = rkisp1_mbus_info_get_by_code(src_fmt->code); > } > if (which == V4L2_SUBDEV_FORMAT_ACTIVE) > isp->src_fmt = mbus_info; >@@ -793,7 +659,7 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp, > struct v4l2_mbus_framefmt *format, > unsigned int which) > { >- const struct rkisp1_isp_mbus_info *mbus_info; >+ const struct rkisp1_mbus_info *mbus_info; > struct v4l2_mbus_framefmt *sink_fmt; > struct v4l2_rect *sink_crop; > >@@ -801,10 +667,10 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp, > RKISP1_ISP_PAD_SINK_VIDEO, > which); > sink_fmt->code = format->code; >- mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); >+ mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SINK)) { > sink_fmt->code = RKISP1_DEF_SINK_PAD_FMT; >- mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); >+ mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > } > if (which == V4L2_SUBDEV_FORMAT_ACTIVE) > isp->sink_fmt = mbus_info; >@@ -1080,8 +946,8 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1) > pads[RKISP1_ISP_PAD_SOURCE_VIDEO].flags = MEDIA_PAD_FL_SOURCE; > pads[RKISP1_ISP_PAD_SOURCE_STATS].flags = MEDIA_PAD_FL_SOURCE; > >- isp->sink_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SINK_PAD_FMT); >- isp->src_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SRC_PAD_FMT); >+ isp->sink_fmt = rkisp1_mbus_info_get_by_code(RKISP1_DEF_SINK_PAD_FMT); >+ isp->src_fmt = rkisp1_mbus_info_get_by_code(RKISP1_DEF_SRC_PAD_FMT); > > mutex_init(&isp->ops_lock); > ret = media_entity_pads_init(&sd->entity, RKISP1_ISP_PAD_MAX, pads); >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c >index 1c07985c810d..f4caa8f684aa 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c >@@ -433,14 +433,14 @@ static void rkisp1_rsz_set_src_fmt(struct rkisp1_resizer *rsz, > struct v4l2_mbus_framefmt *format, > unsigned int which) > { >- const struct rkisp1_isp_mbus_info *sink_mbus_info; >+ const struct rkisp1_mbus_info *sink_mbus_info; > struct v4l2_mbus_framefmt *src_fmt, *sink_fmt; > > sink_fmt = rkisp1_rsz_get_pad_fmt(rsz, sd_state, RKISP1_RSZ_PAD_SINK, > which); > src_fmt = rkisp1_rsz_get_pad_fmt(rsz, sd_state, RKISP1_RSZ_PAD_SRC, > which); >- sink_mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); >+ sink_mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > > /* for YUV formats, userspace can change the mbus code on the src pad if it is supported */ > if (sink_mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV && >@@ -462,7 +462,7 @@ static void rkisp1_rsz_set_sink_crop(struct rkisp1_resizer *rsz, > struct v4l2_rect *r, > unsigned int which) > { >- const struct rkisp1_isp_mbus_info *mbus_info; >+ const struct rkisp1_mbus_info *mbus_info; > struct v4l2_mbus_framefmt *sink_fmt; > struct v4l2_rect *sink_crop; > >@@ -473,7 +473,7 @@ static void rkisp1_rsz_set_sink_crop(struct rkisp1_resizer *rsz, > which); > > /* Not crop for MP bayer raw data */ >- mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); >+ mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > > if (rsz->id == RKISP1_MAINPATH && > mbus_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) { >@@ -500,7 +500,7 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz, > struct v4l2_mbus_framefmt *format, > unsigned int which) > { >- const struct rkisp1_isp_mbus_info *mbus_info; >+ const struct rkisp1_mbus_info *mbus_info; > struct v4l2_mbus_framefmt *sink_fmt, *src_fmt; > struct v4l2_rect *sink_crop; > >@@ -516,10 +516,10 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz, > else > sink_fmt->code = format->code; > >- mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); >+ mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) { > sink_fmt->code = RKISP1_DEF_FMT; >- mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code); >+ mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code); > } > if (which == V4L2_SUBDEV_FORMAT_ACTIVE) > rsz->pixel_enc = mbus_info->pixel_enc; >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c >index 7d82356b5345..2795eef91bdd 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c >@@ -305,7 +305,7 @@ static void rkisp1_stats_get_bls_meas(struct rkisp1_stats *stats, > struct rkisp1_stat_buffer *pbuf) > { > struct rkisp1_device *rkisp1 = stats->rkisp1; >- const struct rkisp1_isp_mbus_info *in_fmt = rkisp1->isp.sink_fmt; >+ const struct rkisp1_mbus_info *in_fmt = rkisp1->isp.sink_fmt; > struct rkisp1_cif_isp_bls_meas_val *bls_val; > > bls_val = &pbuf->params.ae.bls_val; >-- >2.30.2 >
On 15.06.2022 04:10, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >Move the calls to the active sensor's .s_stream() operation to the ISP >subdev's .s_stream(). This groups all handling of the active sensor in >one place, preparing for a rework of that code. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by Dafna Hirschfeld <dafna@fastmail.com> >--- > .../media/platform/rockchip/rkisp1/rkisp1-capture.c | 12 +----------- > drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 11 +++++++++++ > 2 files changed, 12 insertions(+), 11 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >index 94a0d787a312..9edaa95fa44c 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >@@ -926,11 +926,8 @@ static void rkisp1_pipeline_stream_disable(struct rkisp1_capture *cap) > * If the other capture is streaming, isp and sensor nodes shouldn't > * be disabled, skip them. > */ >- if (rkisp1->pipe.streaming_count < 2) { >- v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, >- false); >+ if (rkisp1->pipe.streaming_count < 2) > v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); >- } > > v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, > false); >@@ -966,15 +963,8 @@ static int rkisp1_pipeline_stream_enable(struct rkisp1_capture *cap) > if (ret) > goto err_disable_rsz; > >- ret = v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, >- true); >- if (ret) >- goto err_disable_isp; >- > return 0; > >-err_disable_isp: >- v4l2_subdev_call(&rkisp1->isp.sd, video, s_stream, false); > err_disable_rsz: > v4l2_subdev_call(&rkisp1->resizer_devs[cap->id].sd, video, s_stream, > false); >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >index 89577119b571..58cf6d21f1eb 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >@@ -857,6 +857,9 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > int ret = 0; > > if (!enable) { >+ v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, >+ false); >+ > rkisp1_isp_stop(rkisp1); > rkisp1_mipi_csi2_stop(rkisp1->active_sensor); > return 0; >@@ -886,6 +889,14 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > > rkisp1_isp_start(rkisp1); > >+ ret = v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, >+ true); >+ if (ret) { >+ rkisp1_isp_stop(rkisp1); >+ rkisp1_mipi_csi2_stop(rkisp1->active_sensor); >+ goto mutex_unlock; >+ } >+ > mutex_unlock: > mutex_unlock(&isp->ops_lock); > return ret; >-- >2.30.2 >
On 15.06.2022 04:10, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >Links from sensors to the ISP can be created as sensors are bound. Move >the link creation from rkisp1_create_links() to the bound notifier, and >clean up the rkisp1_create_links() function while at it. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by Dafna Hirschfeld <dafna@fastmail.com> >--- > .../platform/rockchip/rkisp1/rkisp1-common.h | 2 + > .../platform/rockchip/rkisp1/rkisp1-dev.c | 87 ++++++++----------- > 2 files changed, 40 insertions(+), 49 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >index c7d5c57607bd..ba11baf75fa9 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >@@ -116,6 +116,7 @@ struct rkisp1_info { > * of the v4l2-async API > * > * @asd: async_subdev variable for the sensor >+ * @index: index of the sensor (counting sensor found in DT) > * @lanes: number of lanes > * @mbus_type: type of bus (currently only CSI2 is supported) > * @mbus_flags: media bus (V4L2_MBUS_*) flags >@@ -125,6 +126,7 @@ struct rkisp1_info { > */ > struct rkisp1_sensor_async { > struct v4l2_async_subdev asd; >+ unsigned int index; > unsigned int lanes; > enum v4l2_mbus_type mbus_type; > unsigned int mbus_flags; >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >index 7fc617d51f44..4501aea265cb 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >@@ -111,72 +111,46 @@ struct rkisp1_isr_data { > > static int rkisp1_create_links(struct rkisp1_device *rkisp1) > { >- struct media_entity *source, *sink; >- unsigned int flags, source_pad; >- struct v4l2_subdev *sd; > unsigned int i; > int ret; > >- /* sensor links */ >- flags = MEDIA_LNK_FL_ENABLED; >- list_for_each_entry(sd, &rkisp1->v4l2_dev.subdevs, list) { >- if (sd == &rkisp1->isp.sd || >- sd == &rkisp1->resizer_devs[RKISP1_MAINPATH].sd || >- sd == &rkisp1->resizer_devs[RKISP1_SELFPATH].sd) >- continue; >- >- ret = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, >- MEDIA_PAD_FL_SOURCE); >- if (ret < 0) { >- dev_err(rkisp1->dev, "failed to find src pad for %s\n", >- sd->name); >- return ret; >- } >- source_pad = ret; >- >- ret = media_create_pad_link(&sd->entity, source_pad, >- &rkisp1->isp.sd.entity, >- RKISP1_ISP_PAD_SINK_VIDEO, >- flags); >- if (ret) >- return ret; >- >- flags = 0; >- } >- >- flags = MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE; >- > /* create ISP->RSZ->CAP links */ > for (i = 0; i < 2; i++) { >- source = &rkisp1->isp.sd.entity; >- sink = &rkisp1->resizer_devs[i].sd.entity; >- ret = media_create_pad_link(source, RKISP1_ISP_PAD_SOURCE_VIDEO, >- sink, RKISP1_RSZ_PAD_SINK, >+ struct media_entity *resizer = >+ &rkisp1->resizer_devs[i].sd.entity; >+ struct media_entity *capture = >+ &rkisp1->capture_devs[i].vnode.vdev.entity;; >+ >+ ret = media_create_pad_link(&rkisp1->isp.sd.entity, >+ RKISP1_ISP_PAD_SOURCE_VIDEO, >+ resizer, RKISP1_RSZ_PAD_SINK, > MEDIA_LNK_FL_ENABLED); > if (ret) > return ret; > >- source = sink; >- sink = &rkisp1->capture_devs[i].vnode.vdev.entity; >- ret = media_create_pad_link(source, RKISP1_RSZ_PAD_SRC, >- sink, 0, flags); >+ ret = media_create_pad_link(resizer, RKISP1_RSZ_PAD_SRC, >+ capture, 0, >+ MEDIA_LNK_FL_ENABLED | >+ MEDIA_LNK_FL_IMMUTABLE); > if (ret) > return ret; > } > > /* params links */ >- source = &rkisp1->params.vnode.vdev.entity; >- sink = &rkisp1->isp.sd.entity; >- ret = media_create_pad_link(source, 0, sink, >- RKISP1_ISP_PAD_SINK_PARAMS, flags); >+ ret = media_create_pad_link(&rkisp1->params.vnode.vdev.entity, 0, >+ &rkisp1->isp.sd.entity, >+ RKISP1_ISP_PAD_SINK_PARAMS, >+ MEDIA_LNK_FL_ENABLED | >+ MEDIA_LNK_FL_IMMUTABLE); > if (ret) > return ret; > > /* 3A stats links */ >- source = &rkisp1->isp.sd.entity; >- sink = &rkisp1->stats.vnode.vdev.entity; >- return media_create_pad_link(source, RKISP1_ISP_PAD_SOURCE_STATS, >- sink, 0, flags); >+ return media_create_pad_link(&rkisp1->isp.sd.entity, >+ RKISP1_ISP_PAD_SOURCE_STATS, >+ &rkisp1->stats.vnode.vdev.entity, 0, >+ MEDIA_LNK_FL_ENABLED | >+ MEDIA_LNK_FL_IMMUTABLE); > } > > static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier, >@@ -187,6 +161,7 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier, > container_of(notifier, struct rkisp1_device, notifier); > struct rkisp1_sensor_async *s_asd = > 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); >@@ -206,7 +181,19 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier, > > phy_init(s_asd->dphy); > >- return 0; >+ /* Create the link to the sensor. */ >+ source_pad = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, >+ MEDIA_PAD_FL_SOURCE); >+ if (source_pad < 0) { >+ dev_err(rkisp1->dev, "failed to find source pad for %s\n", >+ sd->name); >+ 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); > } > > static void rkisp1_subdev_notifier_unbind(struct v4l2_async_notifier *notifier, >@@ -248,6 +235,7 @@ static int rkisp1_subdev_notifier(struct rkisp1_device *rkisp1) > { > struct v4l2_async_notifier *ntf = &rkisp1->notifier; > unsigned int next_id = 0; >+ unsigned int index = 0; > int ret; > > v4l2_async_nf_init(ntf); >@@ -277,6 +265,7 @@ static int rkisp1_subdev_notifier(struct rkisp1_device *rkisp1) > goto err_parse; > } > >+ rk_asd->index = index++; > rk_asd->mbus_type = vep.bus_type; > rk_asd->mbus_flags = vep.bus.mipi_csi2.flags; > rk_asd->lanes = vep.bus.mipi_csi2.num_data_lanes; >-- >2.30.2 >
On 15.06.2022 04:10, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >Make sure the ISP is ready to receive data before starting the CSI-2 >receiver by starting it first. Similarly, stop the CSI-2 receiver before >the ISP when stopping streaming. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by Dafna Hirschfeld <dafna@fastmail.com> >--- > drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >index 5eabb321e320..0e68c8d53404 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >@@ -771,8 +771,9 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, > false); > >- rkisp1_isp_stop(rkisp1); > rkisp1_mipi_csi2_stop(&rkisp1->csi); >+ rkisp1_isp_stop(rkisp1); >+ > return 0; > } > >@@ -794,11 +795,13 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > if (ret) > goto mutex_unlock; > >+ rkisp1_isp_start(rkisp1); >+ > ret = rkisp1_mipi_csi2_start(&rkisp1->csi, rkisp1->active_sensor); >- if (ret) >+ if (ret) { >+ rkisp1_isp_stop(rkisp1); > goto mutex_unlock; >- >- rkisp1_isp_start(rkisp1); >+ } > > ret = v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, > true); >-- >2.30.2 >
On 15.06.2022 04:10, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >The CSI-related functions are not named consistently. Fix it by using a >common rkisp1_csi prefix. this fixes my comment few patches back > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by Dafna Hirschfeld <dafna@fastmail.com> >--- > .../platform/rockchip/rkisp1/rkisp1-common.h | 2 +- > .../platform/rockchip/rkisp1/rkisp1-csi.c | 20 +++++++++---------- > .../platform/rockchip/rkisp1/rkisp1-csi.h | 6 +++--- > .../platform/rockchip/rkisp1/rkisp1-dev.c | 4 ++-- > .../platform/rockchip/rkisp1/rkisp1-isp.c | 6 +++--- > 5 files changed, 19 insertions(+), 19 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >index a52fa9e0dd66..dbf1baca623a 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >@@ -555,7 +555,7 @@ void rkisp1_params_disable(struct rkisp1_params *params); > > /* irq handlers */ > irqreturn_t rkisp1_isp_isr(int irq, void *ctx); >-irqreturn_t rkisp1_mipi_isr(int irq, void *ctx); >+irqreturn_t rkisp1_csi_isr(int irq, void *ctx); > irqreturn_t rkisp1_capture_isr(int irq, void *ctx); > void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris); > void rkisp1_params_isr(struct rkisp1_device *rkisp1); >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c >index f19513a601e8..c1bb8c05543d 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c >@@ -18,7 +18,7 @@ > #include "rkisp1-common.h" > #include "rkisp1-csi.h" > >-static int rkisp1_config_mipi(struct rkisp1_csi *csi) >+static int rkisp1_csi_config(struct rkisp1_csi *csi) > { > struct rkisp1_device *rkisp1 = csi->rkisp1; > const struct rkisp1_mbus_info *sink_fmt = rkisp1->isp.sink_fmt; >@@ -68,7 +68,7 @@ static int rkisp1_config_mipi(struct rkisp1_csi *csi) > return 0; > } > >-void rkisp1_mipi_start(struct rkisp1_csi *csi) >+static void rkisp1_csi_enable(struct rkisp1_csi *csi) > { > struct rkisp1_device *rkisp1 = csi->rkisp1; > u32 val; >@@ -78,7 +78,7 @@ void rkisp1_mipi_start(struct rkisp1_csi *csi) > val | RKISP1_CIF_MIPI_CTRL_OUTPUT_ENA); > } > >-void rkisp1_mipi_stop(struct rkisp1_csi *csi) >+static void rkisp1_csi_disable(struct rkisp1_csi *csi) > { > struct rkisp1_device *rkisp1 = csi->rkisp1; > u32 val; >@@ -92,8 +92,8 @@ void rkisp1_mipi_stop(struct rkisp1_csi *csi) > val & (~RKISP1_CIF_MIPI_CTRL_OUTPUT_ENA)); > } > >-int rkisp1_mipi_csi2_start(struct rkisp1_csi *csi, >- struct rkisp1_sensor_async *sensor) >+int rkisp1_csi_start(struct rkisp1_csi *csi, >+ struct rkisp1_sensor_async *sensor) > { > struct rkisp1_device *rkisp1 = csi->rkisp1; > union phy_configure_opts opts; >@@ -101,7 +101,7 @@ int rkisp1_mipi_csi2_start(struct rkisp1_csi *csi, > s64 pixel_clock; > int ret; > >- ret = rkisp1_config_mipi(csi); >+ ret = rkisp1_csi_config(csi); > if (ret) > return ret; > >@@ -118,19 +118,19 @@ int rkisp1_mipi_csi2_start(struct rkisp1_csi *csi, > phy_configure(csi->dphy, &opts); > phy_power_on(csi->dphy); > >- rkisp1_mipi_start(csi); >+ rkisp1_csi_enable(csi); > > return 0; > } > >-void rkisp1_mipi_csi2_stop(struct rkisp1_csi *csi) >+void rkisp1_csi_stop(struct rkisp1_csi *csi) > { >- rkisp1_mipi_stop(csi); >+ rkisp1_csi_disable(csi); > > phy_power_off(csi->dphy); > } > >-irqreturn_t rkisp1_mipi_isr(int irq, void *ctx) >+irqreturn_t rkisp1_csi_isr(int irq, void *ctx) > { > struct device *dev = ctx; > struct rkisp1_device *rkisp1 = dev_get_drvdata(dev); >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h >index 1f921d534865..7d3f01cfb49f 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h >@@ -17,8 +17,8 @@ struct rkisp1_sensor_async; > int rkisp1_csi_init(struct rkisp1_device *rkisp1); > void rkisp1_csi_cleanup(struct rkisp1_device *rkisp1); > >-int rkisp1_mipi_csi2_start(struct rkisp1_csi *csi, >- struct rkisp1_sensor_async *sensor); >-void rkisp1_mipi_csi2_stop(struct rkisp1_csi *csi); >+int rkisp1_csi_start(struct rkisp1_csi *csi, >+ struct rkisp1_sensor_async *sensor); >+void rkisp1_csi_stop(struct rkisp1_csi *csi); > > #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 373a1f00c10a..253220188abd 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >@@ -383,7 +383,7 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx) > */ > rkisp1_capture_isr(irq, ctx); > rkisp1_isp_isr(irq, ctx); >- rkisp1_mipi_isr(irq, ctx); >+ rkisp1_csi_isr(irq, ctx); > > return IRQ_HANDLED; > } >@@ -398,7 +398,7 @@ static const char * const px30_isp_clks[] = { > static const struct rkisp1_isr_data px30_isp_isrs[] = { > { "isp", rkisp1_isp_isr }, > { "mi", rkisp1_capture_isr }, >- { "mipi", rkisp1_mipi_isr }, >+ { "mipi", rkisp1_csi_isr }, > }; > > static const struct rkisp1_info px30_isp_info = { >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >index 208c68aa52ea..ecb8ca0ad670 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >@@ -763,7 +763,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, > false); > >- rkisp1_mipi_csi2_stop(&rkisp1->csi); >+ rkisp1_csi_stop(&rkisp1->csi); > rkisp1_isp_stop(rkisp1); > > return 0; >@@ -789,7 +789,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > > rkisp1_isp_start(rkisp1); > >- ret = rkisp1_mipi_csi2_start(&rkisp1->csi, rkisp1->active_sensor); >+ ret = rkisp1_csi_start(&rkisp1->csi, rkisp1->active_sensor); > if (ret) { > rkisp1_isp_stop(rkisp1); > goto mutex_unlock; >@@ -799,7 +799,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > true); > if (ret) { > rkisp1_isp_stop(rkisp1); >- rkisp1_mipi_csi2_stop(&rkisp1->csi); >+ rkisp1_csi_stop(&rkisp1->csi); > goto mutex_unlock; > } > >-- >2.30.2 >
On 15.06.2022 04:10, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >To prepare for the removal of the active_sensor field from the >rkisp1_device structure, pass the sensor pointer to the >rkisp1_csi_config() function instead of accessing it through >active_sensor. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by Dafna Hirschfeld <dafna@fastmail.com> >--- > drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c >index fcaffffd371b..925274b9a3c4 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c >@@ -19,11 +19,12 @@ > #include "rkisp1-common.h" > #include "rkisp1-csi.h" > >-static int rkisp1_csi_config(struct rkisp1_csi *csi) >+static int rkisp1_csi_config(struct rkisp1_csi *csi, >+ struct rkisp1_sensor_async *sensor) > { > struct rkisp1_device *rkisp1 = csi->rkisp1; > const struct rkisp1_mbus_info *sink_fmt = rkisp1->isp.sink_fmt; >- unsigned int lanes = rkisp1->active_sensor->lanes; >+ unsigned int lanes = sensor->lanes; > u32 mipi_ctrl; > > if (lanes < 1 || lanes > 4) >@@ -102,7 +103,7 @@ int rkisp1_csi_start(struct rkisp1_csi *csi, > s64 pixel_clock; > int ret; > >- ret = rkisp1_csi_config(csi); >+ ret = rkisp1_csi_config(csi, sensor); > if (ret) > return ret; > >-- >2.30.2 >
On 15.06.2022 04:10, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >To prepare for the removal of the active_sensor field from the >rkisp1_device structure, pass the media bus type of flag to the >rkisp1_config_cif() function instead of accessing them through >active_sensor. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by Dafna Hirschfeld <dafna@fastmail.com> >--- > .../platform/rockchip/rkisp1/rkisp1-isp.c | 44 +++++++++---------- > 1 file changed, 22 insertions(+), 22 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >index a234cf29ec67..f6d1c93dd99d 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >@@ -136,15 +136,14 @@ static void rkisp1_config_ism(struct rkisp1_device *rkisp1) > /* > * configure ISP blocks with input format, size...... > */ >-static int rkisp1_config_isp(struct rkisp1_device *rkisp1) >+static int rkisp1_config_isp(struct rkisp1_device *rkisp1, >+ enum v4l2_mbus_type mbus_type, u32 mbus_flags) > { > u32 isp_ctrl = 0, irq_mask = 0, acq_mult = 0, signal = 0; > const struct rkisp1_mbus_info *src_fmt, *sink_fmt; >- struct rkisp1_sensor_async *sensor; > struct v4l2_mbus_framefmt *sink_frm; > struct v4l2_rect *sink_crop; > >- sensor = rkisp1->active_sensor; > sink_fmt = rkisp1->isp.sink_fmt; > src_fmt = rkisp1->isp.src_fmt; > sink_frm = rkisp1_isp_get_pad_fmt(&rkisp1->isp, NULL, >@@ -157,7 +156,7 @@ static int rkisp1_config_isp(struct rkisp1_device *rkisp1) > if (sink_fmt->pixel_enc == V4L2_PIXEL_ENC_BAYER) { > acq_mult = 1; > if (src_fmt->pixel_enc == V4L2_PIXEL_ENC_BAYER) { >- if (sensor->mbus_type == V4L2_MBUS_BT656) >+ if (mbus_type == V4L2_MBUS_BT656) > isp_ctrl = RKISP1_CIF_ISP_CTRL_ISP_MODE_RAW_PICT_ITU656; > else > isp_ctrl = RKISP1_CIF_ISP_CTRL_ISP_MODE_RAW_PICT; >@@ -165,17 +164,17 @@ static int rkisp1_config_isp(struct rkisp1_device *rkisp1) > rkisp1_write(rkisp1, RKISP1_CIF_ISP_DEMOSAIC, > RKISP1_CIF_ISP_DEMOSAIC_TH(0xc)); > >- if (sensor->mbus_type == V4L2_MBUS_BT656) >+ if (mbus_type == V4L2_MBUS_BT656) > isp_ctrl = RKISP1_CIF_ISP_CTRL_ISP_MODE_BAYER_ITU656; > else > isp_ctrl = RKISP1_CIF_ISP_CTRL_ISP_MODE_BAYER_ITU601; > } > } else if (sink_fmt->pixel_enc == V4L2_PIXEL_ENC_YUV) { > acq_mult = 2; >- if (sensor->mbus_type == V4L2_MBUS_CSI2_DPHY) { >+ if (mbus_type == V4L2_MBUS_CSI2_DPHY) { > isp_ctrl = RKISP1_CIF_ISP_CTRL_ISP_MODE_ITU601; > } else { >- if (sensor->mbus_type == V4L2_MBUS_BT656) >+ if (mbus_type == V4L2_MBUS_BT656) > isp_ctrl = RKISP1_CIF_ISP_CTRL_ISP_MODE_ITU656; > else > isp_ctrl = RKISP1_CIF_ISP_CTRL_ISP_MODE_ITU601; >@@ -185,17 +184,16 @@ static int rkisp1_config_isp(struct rkisp1_device *rkisp1) > } > > /* Set up input acquisition properties */ >- if (sensor->mbus_type == V4L2_MBUS_BT656 || >- sensor->mbus_type == V4L2_MBUS_PARALLEL) { >- if (sensor->mbus_flags & V4L2_MBUS_PCLK_SAMPLE_RISING) >+ if (mbus_type == V4L2_MBUS_BT656 || mbus_type == V4L2_MBUS_PARALLEL) { >+ if (mbus_flags & V4L2_MBUS_PCLK_SAMPLE_RISING) > signal = RKISP1_CIF_ISP_ACQ_PROP_POS_EDGE; > } > >- if (sensor->mbus_type == V4L2_MBUS_PARALLEL) { >- if (sensor->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) >+ if (mbus_type == V4L2_MBUS_PARALLEL) { >+ if (mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) > signal |= RKISP1_CIF_ISP_ACQ_PROP_VSYNC_LOW; > >- if (sensor->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW) >+ if (mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW) > signal |= RKISP1_CIF_ISP_ACQ_PROP_HSYNC_LOW; > } > >@@ -265,17 +263,17 @@ static int rkisp1_config_dvp(struct rkisp1_device *rkisp1) > } > > /* Configure MUX */ >-static int rkisp1_config_path(struct rkisp1_device *rkisp1) >+static int rkisp1_config_path(struct rkisp1_device *rkisp1, >+ enum v4l2_mbus_type mbus_type) > { >- struct rkisp1_sensor_async *sensor = rkisp1->active_sensor; > u32 dpcl = rkisp1_read(rkisp1, RKISP1_CIF_VI_DPCL); > int ret = 0; > >- if (sensor->mbus_type == V4L2_MBUS_BT656 || >- sensor->mbus_type == V4L2_MBUS_PARALLEL) { >+ if (mbus_type == V4L2_MBUS_BT656 || >+ mbus_type == V4L2_MBUS_PARALLEL) { > ret = rkisp1_config_dvp(rkisp1); > dpcl |= RKISP1_CIF_VI_DPCL_IF_SEL_PARALLEL; >- } else if (sensor->mbus_type == V4L2_MBUS_CSI2_DPHY) { >+ } else if (mbus_type == V4L2_MBUS_CSI2_DPHY) { > dpcl |= RKISP1_CIF_VI_DPCL_IF_SEL_MIPI; > } > >@@ -285,14 +283,15 @@ static int rkisp1_config_path(struct rkisp1_device *rkisp1) > } > > /* Hardware configure Entry */ >-static int rkisp1_config_cif(struct rkisp1_device *rkisp1) >+static int rkisp1_config_cif(struct rkisp1_device *rkisp1, >+ enum v4l2_mbus_type mbus_type, u32 mbus_flags) > { > int ret; > >- ret = rkisp1_config_isp(rkisp1); >+ ret = rkisp1_config_isp(rkisp1, mbus_type, mbus_flags); > if (ret) > return ret; >- ret = rkisp1_config_path(rkisp1); >+ ret = rkisp1_config_path(rkisp1, mbus_type); > if (ret) > return ret; > rkisp1_config_ism(rkisp1); >@@ -777,7 +776,8 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > > rkisp1->isp.frame_sequence = -1; > mutex_lock(&isp->ops_lock); >- ret = rkisp1_config_cif(rkisp1); >+ ret = rkisp1_config_cif(rkisp1, rkisp1->active_sensor->mbus_type, >+ rkisp1->active_sensor->mbus_flags); > if (ret) > goto mutex_unlock; > >-- >2.30.2 >
On 15.06.2022 04:11, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >The rkisp1_isp structure documentation mentions a backpointer field to >rkisp1_device, but the field is missing. Add it, and use it to replace >more complicated constructs using container_of() on the v4l2_device. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by Dafna Hirschfeld <dafna@fastmail.com> >--- > .../media/platform/rockchip/rkisp1/rkisp1-common.h | 1 + > drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 11 +++++------ > 2 files changed, 6 insertions(+), 6 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >index 7a6f55a31bb0..3c55e922346e 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >@@ -162,6 +162,7 @@ struct rkisp1_csi { > */ > struct rkisp1_isp { > struct v4l2_subdev sd; >+ struct rkisp1_device *rkisp1; > struct media_pad pads[RKISP1_ISP_PAD_MAX]; > struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX]; > const struct rkisp1_mbus_info *sink_fmt; >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >index 812fb2ea5323..f8ab1d9cc8cd 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >@@ -704,15 +704,13 @@ static int rkisp1_isp_set_selection(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_selection *sel) > { >- struct rkisp1_device *rkisp1 = >- container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev); > struct rkisp1_isp *isp = to_rkisp1_isp(sd); > int ret = 0; > > if (sel->target != V4L2_SEL_TGT_CROP) > return -EINVAL; > >- dev_dbg(rkisp1->dev, "%s: pad: %d sel(%d,%d)/%dx%d\n", __func__, >+ dev_dbg(isp->rkisp1->dev, "%s: pad: %d sel(%d,%d)/%dx%d\n", __func__, > sel->pad, sel->r.left, sel->r.top, sel->r.width, sel->r.height); > mutex_lock(&isp->ops_lock); > if (sel->pad == RKISP1_ISP_PAD_SINK_VIDEO) >@@ -751,9 +749,8 @@ static const struct v4l2_subdev_pad_ops rkisp1_isp_pad_ops = { > > static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > { >- struct rkisp1_device *rkisp1 = >- container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev); > struct rkisp1_isp *isp = to_rkisp1_isp(sd); >+ struct rkisp1_device *rkisp1 = isp->rkisp1; > struct rkisp1_sensor_async *asd; > int ret; > >@@ -840,12 +837,14 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1) > { > struct v4l2_subdev_state state = { > .pads = rkisp1->isp.pad_cfg >- }; >+ }; > struct rkisp1_isp *isp = &rkisp1->isp; > struct media_pad *pads = isp->pads; > struct v4l2_subdev *sd = &isp->sd; > int ret; > >+ isp->rkisp1 = rkisp1; >+ > v4l2_subdev_init(sd, &rkisp1_isp_ops); > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; > sd->entity.ops = &rkisp1_isp_media_ops; >-- >2.30.2 >
On 15.06.2022 04:11, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >The ISP_ACQ_PROP register is set twice, once in rkisp1_config_isp() for >most of its fields, and once in rkisp1_config_dvp() (called from >rkisp1_config_path()) to configure the input selection field. Move the >latter to rkisp1_config_isp() to write the register once only, and drop >the now empty rkisp1_config_dvp() function. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >--- > .../platform/rockchip/rkisp1/rkisp1-isp.c | 62 +++++++------------ > 1 file changed, 22 insertions(+), 40 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >index e27137b5c33e..f5b8a2e31936 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >@@ -141,7 +141,7 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp, > enum v4l2_mbus_type mbus_type, u32 mbus_flags) > { > struct rkisp1_device *rkisp1 = isp->rkisp1; >- u32 isp_ctrl = 0, irq_mask = 0, acq_mult = 0, signal = 0; >+ u32 isp_ctrl = 0, irq_mask = 0, acq_mult = 0, signal = 0, input_sel = 0; > const struct rkisp1_mbus_info *src_fmt, *sink_fmt; > struct v4l2_mbus_framefmt *sink_frm; > struct v4l2_rect *sink_crop; >@@ -189,6 +189,21 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp, > if (mbus_type == V4L2_MBUS_BT656 || mbus_type == V4L2_MBUS_PARALLEL) { > if (mbus_flags & V4L2_MBUS_PCLK_SAMPLE_RISING) > signal = RKISP1_CIF_ISP_ACQ_PROP_POS_EDGE; >+ >+ switch (sink_fmt->bus_width) { >+ case 8: >+ input_sel = RKISP1_CIF_ISP_ACQ_PROP_IN_SEL_8B_ZERO; >+ break; >+ case 10: >+ input_sel = RKISP1_CIF_ISP_ACQ_PROP_IN_SEL_10B_ZERO; >+ break; >+ case 12: >+ input_sel = RKISP1_CIF_ISP_ACQ_PROP_IN_SEL_12B; >+ break; >+ default: >+ dev_err(rkisp1->dev, "Invalid bus width\n"); maybe also print the bus width here >+ return -EINVAL; >+ } > } > > if (mbus_type == V4L2_MBUS_PARALLEL) { >@@ -201,7 +216,7 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp, > > rkisp1_write(rkisp1, RKISP1_CIF_ISP_CTRL, isp_ctrl); > rkisp1_write(rkisp1, RKISP1_CIF_ISP_ACQ_PROP, >- signal | sink_fmt->yuv_seq | >+ signal | sink_fmt->yuv_seq | input_sel | > RKISP1_CIF_ISP_ACQ_PROP_BAYER_PAT(sink_fmt->bayer_pat) | > RKISP1_CIF_ISP_ACQ_PROP_FIELD_SEL_ALL); > rkisp1_write(rkisp1, RKISP1_CIF_ISP_ACQ_NR_FRAMES, 0); >@@ -238,52 +253,20 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp, > return 0; > } > >-static int rkisp1_config_dvp(struct rkisp1_isp *isp) >-{ >- struct rkisp1_device *rkisp1 = isp->rkisp1; >- const struct rkisp1_mbus_info *sink_fmt = isp->sink_fmt; >- u32 val, input_sel; >- >- switch (sink_fmt->bus_width) { >- case 8: >- input_sel = RKISP1_CIF_ISP_ACQ_PROP_IN_SEL_8B_ZERO; >- break; >- case 10: >- input_sel = RKISP1_CIF_ISP_ACQ_PROP_IN_SEL_10B_ZERO; >- break; >- case 12: >- input_sel = RKISP1_CIF_ISP_ACQ_PROP_IN_SEL_12B; >- break; >- default: >- dev_err(rkisp1->dev, "Invalid bus width\n"); >- return -EINVAL; >- } >- >- val = rkisp1_read(rkisp1, RKISP1_CIF_ISP_ACQ_PROP); >- rkisp1_write(rkisp1, RKISP1_CIF_ISP_ACQ_PROP, val | input_sel); >- >- return 0; >-} >- > /* Configure MUX */ >-static int rkisp1_config_path(struct rkisp1_isp *isp, >- enum v4l2_mbus_type mbus_type) >+static void rkisp1_config_path(struct rkisp1_isp *isp, >+ enum v4l2_mbus_type mbus_type) > { > struct rkisp1_device *rkisp1 = isp->rkisp1; > u32 dpcl = rkisp1_read(rkisp1, RKISP1_CIF_VI_DPCL); >- int ret = 0; > >- if (mbus_type == V4L2_MBUS_BT656 || >- mbus_type == V4L2_MBUS_PARALLEL) { >- ret = rkisp1_config_dvp(isp); >+ if (mbus_type == V4L2_MBUS_BT656 || mbus_type == V4L2_MBUS_PARALLEL) { > dpcl |= RKISP1_CIF_VI_DPCL_IF_SEL_PARALLEL; > } else if (mbus_type == V4L2_MBUS_CSI2_DPHY) { > dpcl |= RKISP1_CIF_VI_DPCL_IF_SEL_MIPI; > } can remove the curly braces thanks, Dafna > > rkisp1_write(rkisp1, RKISP1_CIF_VI_DPCL, dpcl); >- >- return ret; > } > > /* Hardware configure Entry */ >@@ -295,9 +278,8 @@ static int rkisp1_config_cif(struct rkisp1_isp *isp, > ret = rkisp1_config_isp(isp, mbus_type, mbus_flags); > if (ret) > return ret; >- ret = rkisp1_config_path(isp, mbus_type); >- if (ret) >- return ret; >+ >+ rkisp1_config_path(isp, mbus_type); > rkisp1_config_ism(isp); > > return 0; >-- >2.30.2 >
On 15.06.2022 04:11, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >Intialize the src_fmt and sink_fmt variable when declaring them in >rkisp1_config_isp(). > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by Dafna Hirschfeld <dafna@fastmail.com> >--- > drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >index 4496af991c82..bf44f10200f5 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >@@ -142,12 +142,11 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp, > { > struct rkisp1_device *rkisp1 = isp->rkisp1; > u32 isp_ctrl = 0, irq_mask = 0, acq_mult = 0, acq_prop = 0; >- const struct rkisp1_mbus_info *src_fmt, *sink_fmt; >+ const struct rkisp1_mbus_info *sink_fmt = isp->sink_fmt; >+ const struct rkisp1_mbus_info *src_fmt = isp->src_fmt; > struct v4l2_mbus_framefmt *sink_frm; > struct v4l2_rect *sink_crop; > >- sink_fmt = isp->sink_fmt; >- src_fmt = isp->src_fmt; > sink_frm = rkisp1_isp_get_pad_fmt(isp, NULL, > RKISP1_ISP_PAD_SINK_VIDEO, > V4L2_SUBDEV_FORMAT_ACTIVE); >-- >2.30.2 >
On 15.06.2022 04:11, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >A set of local variables point to structure that are not meant to be >modified. Constify them. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by Dafna Hirschfeld <dafna@fastmail.com> >--- > drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >index 86d496855374..91b37f2dca91 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >@@ -113,7 +113,7 @@ rkisp1_isp_get_pad_crop(struct rkisp1_isp *isp, > */ > static void rkisp1_config_ism(struct rkisp1_isp *isp) > { >- struct v4l2_rect *src_crop = >+ const struct v4l2_rect *src_crop = > rkisp1_isp_get_pad_crop(isp, NULL, > RKISP1_ISP_PAD_SOURCE_VIDEO, > V4L2_SUBDEV_FORMAT_ACTIVE); >@@ -146,8 +146,8 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp, > u32 isp_ctrl = 0, irq_mask = 0, acq_mult = 0, acq_prop = 0; > const struct rkisp1_mbus_info *sink_fmt = isp->sink_fmt; > const struct rkisp1_mbus_info *src_fmt = isp->src_fmt; >- struct v4l2_mbus_framefmt *sink_frm; >- struct v4l2_rect *sink_crop; >+ const struct v4l2_mbus_framefmt *sink_frm; >+ const struct v4l2_rect *sink_crop; > > sink_frm = rkisp1_isp_get_pad_fmt(isp, NULL, > RKISP1_ISP_PAD_SINK_VIDEO, >@@ -557,7 +557,7 @@ static void rkisp1_isp_set_sink_crop(struct rkisp1_isp *isp, > struct v4l2_rect *r, unsigned int which) > { > struct v4l2_rect *sink_crop, *src_crop; >- struct v4l2_mbus_framefmt *sink_fmt; >+ const struct v4l2_mbus_framefmt *sink_fmt; > > sink_crop = rkisp1_isp_get_pad_crop(isp, sd_state, > RKISP1_ISP_PAD_SINK_VIDEO, >@@ -742,7 +742,7 @@ 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; >- struct rkisp1_sensor_async *asd; >+ const struct rkisp1_sensor_async *asd; > int ret; > > if (!enable) { >-- >2.30.2 >
On 15.06.2022 04:11, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com> >Signed-off-by: Laurent Pinchart <laurent.pinchart@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 >
Hi Dafna, On Sat, Jun 25, 2022 at 10:45:59AM +0300, Dafna Hirschfeld wrote: > On 15.06.2022 04:11, Paul Elder wrote: > > From: Laurent Pinchart <laurent.pinchart@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. Good point, I'll do that. > 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. What do you mean exactly by ignoring it ? > > - 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@ideasonboard.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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;' ? Oh. Paul, did you get so used to the fact that checkstyle.py is integrated in git hooks in libcamera that you forgot that checkpatch.pl has to be run manually ? :-) > > + > > + 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? I don't think so, because one could connect a parallel sensor directly to the ISP parallel input. This isn't well supported in the driver today though, probably because nobody has cared much about upstreaming support for such a setup, but I think it's a valid hardware option. > > + 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. rkisp1->source is the source of the ISP. it can be the CSI-2 receiver, or if a parallel sensor is connected directly to the ISP, the sensor itself. The check here is meant to catch bugs in the implementation while transitioning, as in this patch external CSI-2 receivers or parallel sensors are not supported yet. Patch "media: rkisp1: Support the ISP parallel input" then changes this code. > > > > 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'. As explained above, I think source is correct here, as rkisp1->csi is the internal CSI-2 receiver, which may or may not be the source of the ISP, depending on link configuration. > > if (ret) { > > rkisp1_isp_stop(isp); > > - rkisp1_csi_stop(&rkisp1->csi); > > goto mutex_unlock; > > } > >
Hi Hans, On Fri, Jun 17, 2022 at 01:48:05PM +0200, Hans Verkuil wrote: > On 6/14/22 21:11, Paul Elder wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > The media_entity_remote_pad() helper function returns the first remote > > pad it find connected to a given pad. Beside being possibly ill-named > > (as it operates on a pad, not an entity) and non-deterministic (as it > > stops at the first enabled link), the fact that it returns the first > > match makes it unsuitable for drivers that need to guarantee that a > > single link is enabled, for instance when an entity can process data > > from one of multiple sources at a time. > > Question: of all the callers of this function, are there any that really > need media_entity_remote_pad() instead of media_pad_remote_pad_unique()? > > Would it be possible to replace all callers of the old function with the > new function? If that's the case, then the _unique suffix can be dropped, > since that would effectively be the default. And if a function is needed > to handle the case where there are multiple enabled links, then a new > function should be created. I don't think so. media_entity_remote_pad() operates on a pad, switching to media_pad_remote_pad_unique() wouldn't work on subdevs that have multiple sink or source pads with one active link each. > Also, media_entity_remote_pad() should really be renamed to > media_pad_remote_pad_first() or something like that, right? I'm not saying > you should, but that's really what it does, as I understand it. Yes, I think that would make sense, and it would freethe media_entity_remote_pad() name, so the new function wouldn't need the _unique suffix. I'll give it a try. > > For those use cases, add a new helper function, > > media_entity_remote_pad_unique(), that operates on an entity and returns > > a remote pad, with a guarantee that only one link is enabled. To ease > > its use in drivers, also add an inline wrapper that locates source pads > > specifically. A wrapper that locates sink pads can easily be added when > > needed. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Documentation/driver-api/media/mc-core.rst | 4 +- > > drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > > include/media/media-entity.h | 45 ++++++++++++++++++++++ > > 3 files changed, 85 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > > index 02481a2513b9..a2d1e32e3abb 100644 > > --- a/Documentation/driver-api/media/mc-core.rst > > +++ b/Documentation/driver-api/media/mc-core.rst > > @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > > > > Helper functions can be used to find a link between two given pads, or a pad > > connected to another pad through an enabled link > > -:c:func:`media_entity_find_link()` and > > -:c:func:`media_entity_remote_pad()`. > > +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > > +:c:func:`media_entity_remote_source_pad()`). > > > > Use count and power handling > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > > index 11f5207f73aa..1febf5a86be6 100644 > > --- a/drivers/media/mc/mc-entity.c > > +++ b/drivers/media/mc/mc-entity.c > > @@ -9,6 +9,7 @@ > > */ > > > > #include <linux/bitmap.h> > > +#include <linux/list.h> > > #include <linux/property.h> > > #include <linux/slab.h> > > #include <media/media-entity.h> > > @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > > } > > EXPORT_SYMBOL_GPL(media_entity_remote_pad); > > > > +struct media_pad * > > +media_entity_remote_pad_unique(const struct media_entity *entity, > > + unsigned int type) > > +{ > > + struct media_pad *pad = NULL; > > + struct media_link *link; > > + > > + list_for_each_entry(link, &entity->links, list) { > > + struct media_pad *local_pad; > > + struct media_pad *remote_pad; > > + > > + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > > + continue; > > + > > + if (type == MEDIA_PAD_FL_SOURCE) { > > + local_pad = link->sink; > > + remote_pad = link->source; > > + } else { > > + local_pad = link->source; > > + remote_pad = link->sink; > > + } > > + > > + if (local_pad->entity == entity) { > > + if (pad) > > + return ERR_PTR(-ENOTUNIQ); > > + > > + pad = remote_pad; > > + } > > + } > > + > > + if (!pad) > > + return ERR_PTR(-ENOLINK); > > + > > + return pad; > > +} > > +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > > + > > static void media_interface_init(struct media_device *mdev, > > struct media_interface *intf, > > u32 gobj_type, > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > > index a9a1c0ec5d1c..33d5f52719a0 100644 > > --- a/include/media/media-entity.h > > +++ b/include/media/media-entity.h > > @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > > */ > > struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > > > > +/** > > + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > > + * @entity: The entity > > + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > > + * > > + * Search for and return a remote pad of @type connected to @entity through an > > + * enabled link. If multiple (or no) remote pads match these criteria, an error > > + * is returned. > > + * > > + * The uniqueness constraint makes this helper function suitable for entities > > + * that support a single active source or sink at a time. > > + * > > + * Return: A pointer to the remote pad, or one of the following error pointers > > + * if an error occurs: > > + * > > + * * -ENOTUNIQ - Multiple links are enabled > > + * * -ENOLINK - No connected pad found > > + */ > > +struct media_pad * > > +media_entity_remote_pad_unique(const struct media_entity *entity, > > + unsigned int type); > > + > > +/** > > + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > > + * @entity: The entity > > + * > > + * Search for and return a remote source pad connected to @entity through an > > + * enabled link. If multiple (or no) remote pads match these criteria, an error > > + * is returned. > > + * > > + * The uniqueness constraint makes this helper function suitable for entities > > + * that support a single active source at a time. > > + * > > + * Return: A pointer to the remote pad, or one of the following error pointers > > + * if an error occurs: > > + * > > + * * -ENOTUNIQ - Multiple links are enabled > > + * * -ENOLINK - No connected pad found > > + */ > > +static inline struct media_pad * > > +media_entity_remote_source_pad(const struct media_entity *entity) > > +{ > > + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > > +} > > + > > /** > > * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > > * @entity: The entity
Hi Hans, On Sat, Jun 25, 2022 at 08:00:51PM +0300, Laurent Pinchart wrote: > On Fri, Jun 17, 2022 at 01:48:05PM +0200, Hans Verkuil wrote: > > On 6/14/22 21:11, Paul Elder wrote: > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > The media_entity_remote_pad() helper function returns the first remote > > > pad it find connected to a given pad. Beside being possibly ill-named > > > (as it operates on a pad, not an entity) and non-deterministic (as it > > > stops at the first enabled link), the fact that it returns the first > > > match makes it unsuitable for drivers that need to guarantee that a > > > single link is enabled, for instance when an entity can process data > > > from one of multiple sources at a time. > > > > Question: of all the callers of this function, are there any that really > > need media_entity_remote_pad() instead of media_pad_remote_pad_unique()? > > > > Would it be possible to replace all callers of the old function with the > > new function? If that's the case, then the _unique suffix can be dropped, > > since that would effectively be the default. And if a function is needed > > to handle the case where there are multiple enabled links, then a new > > function should be created. > > I don't think so. media_entity_remote_pad() operates on a pad, switching > to media_pad_remote_pad_unique() wouldn't work on subdevs that have > multiple sink or source pads with one active link each. > > > Also, media_entity_remote_pad() should really be renamed to > > media_pad_remote_pad_first() or something like that, right? I'm not saying > > you should, but that's really what it does, as I understand it. > > Yes, I think that would make sense, and it would freethe > media_entity_remote_pad() name, so the new function wouldn't need the > _unique suffix. I'll give it a try. The rename was easy enough, but I've decided to keep the media_entity_remote_pad_unique() name and renamed media_entity_remote_source_pad() to media_entity_remote_source_pad_unique() to make the API clearer. If those names end up being too long, we can rename them later. > > > For those use cases, add a new helper function, > > > media_entity_remote_pad_unique(), that operates on an entity and returns > > > a remote pad, with a guarantee that only one link is enabled. To ease > > > its use in drivers, also add an inline wrapper that locates source pads > > > specifically. A wrapper that locates sink pads can easily be added when > > > needed. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > Documentation/driver-api/media/mc-core.rst | 4 +- > > > drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > > > include/media/media-entity.h | 45 ++++++++++++++++++++++ > > > 3 files changed, 85 insertions(+), 2 deletions(-) > > > > > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > > > index 02481a2513b9..a2d1e32e3abb 100644 > > > --- a/Documentation/driver-api/media/mc-core.rst > > > +++ b/Documentation/driver-api/media/mc-core.rst > > > @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > > > > > > Helper functions can be used to find a link between two given pads, or a pad > > > connected to another pad through an enabled link > > > -:c:func:`media_entity_find_link()` and > > > -:c:func:`media_entity_remote_pad()`. > > > +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > > > +:c:func:`media_entity_remote_source_pad()`). > > > > > > Use count and power handling > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > > > index 11f5207f73aa..1febf5a86be6 100644 > > > --- a/drivers/media/mc/mc-entity.c > > > +++ b/drivers/media/mc/mc-entity.c > > > @@ -9,6 +9,7 @@ > > > */ > > > > > > #include <linux/bitmap.h> > > > +#include <linux/list.h> > > > #include <linux/property.h> > > > #include <linux/slab.h> > > > #include <media/media-entity.h> > > > @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > > > } > > > EXPORT_SYMBOL_GPL(media_entity_remote_pad); > > > > > > +struct media_pad * > > > +media_entity_remote_pad_unique(const struct media_entity *entity, > > > + unsigned int type) > > > +{ > > > + struct media_pad *pad = NULL; > > > + struct media_link *link; > > > + > > > + list_for_each_entry(link, &entity->links, list) { > > > + struct media_pad *local_pad; > > > + struct media_pad *remote_pad; > > > + > > > + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > > > + continue; > > > + > > > + if (type == MEDIA_PAD_FL_SOURCE) { > > > + local_pad = link->sink; > > > + remote_pad = link->source; > > > + } else { > > > + local_pad = link->source; > > > + remote_pad = link->sink; > > > + } > > > + > > > + if (local_pad->entity == entity) { > > > + if (pad) > > > + return ERR_PTR(-ENOTUNIQ); > > > + > > > + pad = remote_pad; > > > + } > > > + } > > > + > > > + if (!pad) > > > + return ERR_PTR(-ENOLINK); > > > + > > > + return pad; > > > +} > > > +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > > > + > > > static void media_interface_init(struct media_device *mdev, > > > struct media_interface *intf, > > > u32 gobj_type, > > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > > > index a9a1c0ec5d1c..33d5f52719a0 100644 > > > --- a/include/media/media-entity.h > > > +++ b/include/media/media-entity.h > > > @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > > > */ > > > struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > > > > > > +/** > > > + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > > > + * @entity: The entity > > > + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > > > + * > > > + * Search for and return a remote pad of @type connected to @entity through an > > > + * enabled link. If multiple (or no) remote pads match these criteria, an error > > > + * is returned. > > > + * > > > + * The uniqueness constraint makes this helper function suitable for entities > > > + * that support a single active source or sink at a time. > > > + * > > > + * Return: A pointer to the remote pad, or one of the following error pointers > > > + * if an error occurs: > > > + * > > > + * * -ENOTUNIQ - Multiple links are enabled > > > + * * -ENOLINK - No connected pad found > > > + */ > > > +struct media_pad * > > > +media_entity_remote_pad_unique(const struct media_entity *entity, > > > + unsigned int type); > > > + > > > +/** > > > + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > > > + * @entity: The entity > > > + * > > > + * Search for and return a remote source pad connected to @entity through an > > > + * enabled link. If multiple (or no) remote pads match these criteria, an error > > > + * is returned. > > > + * > > > + * The uniqueness constraint makes this helper function suitable for entities > > > + * that support a single active source at a time. > > > + * > > > + * Return: A pointer to the remote pad, or one of the following error pointers > > > + * if an error occurs: > > > + * > > > + * * -ENOTUNIQ - Multiple links are enabled > > > + * * -ENOLINK - No connected pad found > > > + */ > > > +static inline struct media_pad * > > > +media_entity_remote_source_pad(const struct media_entity *entity) > > > +{ > > > + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > > > +} > > > + > > > /** > > > * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > > > * @entity: The entity
Hi Dan, On Sat, Jun 18, 2022 at 10:35:12AM +0100, Daniel Scally wrote: > On 17/06/2022 23:40, Laurent Pinchart wrote: > > On Fri, Jun 17, 2022 at 11:33:03PM +0100, Daniel Scally wrote: > >> On 17/06/2022 22:34, Daniel Scally wrote: > >>> On 14/06/2022 20:11, Paul Elder wrote: > >>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> > >>>> The media_entity_remote_pad() helper function returns the first remote > >>>> pad it find connected to a given pad. Beside being possibly ill-named > >>>> (as it operates on a pad, not an entity) and non-deterministic (as it > >>>> stops at the first enabled link), the fact that it returns the first > >>>> match makes it unsuitable for drivers that need to guarantee that a > >>>> single link is enabled, for instance when an entity can process data > >>>> from one of multiple sources at a time. > >>>> > >>>> For those use cases, add a new helper function, > >>>> media_entity_remote_pad_unique(), that operates on an entity and returns > >>>> a remote pad, with a guarantee that only one link is enabled. To ease > >>>> its use in drivers, also add an inline wrapper that locates source pads > >>>> specifically. A wrapper that locates sink pads can easily be added when > >>>> needed. > >>>> > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> --- > >>>> Documentation/driver-api/media/mc-core.rst | 4 +- > >>>> drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > >>>> include/media/media-entity.h | 45 ++++++++++++++++++++++ > >>>> 3 files changed, 85 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > >>>> index 02481a2513b9..a2d1e32e3abb 100644 > >>>> --- a/Documentation/driver-api/media/mc-core.rst > >>>> +++ b/Documentation/driver-api/media/mc-core.rst > >>>> @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > >>>> > >>>> Helper functions can be used to find a link between two given pads, or a pad > >>>> connected to another pad through an enabled link > >>>> -:c:func:`media_entity_find_link()` and > >>>> -:c:func:`media_entity_remote_pad()`. > >>>> +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > >>>> +:c:func:`media_entity_remote_source_pad()`). > >>>> > >>>> Use count and power handling > >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >>>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > >>>> index 11f5207f73aa..1febf5a86be6 100644 > >>>> --- a/drivers/media/mc/mc-entity.c > >>>> +++ b/drivers/media/mc/mc-entity.c > >>>> @@ -9,6 +9,7 @@ > >>>> */ > >>>> > >>>> #include <linux/bitmap.h> > >>>> +#include <linux/list.h> > >>>> #include <linux/property.h> > >>>> #include <linux/slab.h> > >>>> #include <media/media-entity.h> > >>>> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > >>>> } > >>>> EXPORT_SYMBOL_GPL(media_entity_remote_pad); > >>>> > >>>> +struct media_pad * > >>>> +media_entity_remote_pad_unique(const struct media_entity *entity, > >>>> + unsigned int type) > >>>> +{ > >>>> + struct media_pad *pad = NULL; > >>>> + struct media_link *link; > >>>> + > >>>> + list_for_each_entry(link, &entity->links, list) { > >>>> + struct media_pad *local_pad; > >>>> + struct media_pad *remote_pad; > >>>> + > >>>> + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > >>>> + continue; > >>> > >>> Does this need another guard here to make sure the link isn't an > >>> ancillary link? Only likely to happen at least in the immediate future > >>> where the entity represents a camera sensor, so possibly not applicable > >>> here - I couldn't find where the new function is used in the series to > >>> check. I _think_ it would actually work ok regardless...media_gobj > >>> type-punning makes my brain ache, but I think the local_pad->entity == > >>> entity comparison would actually compare the entity->name member of the > >>> entity at the end of an ancillary link to the entity parameter, not find > >>> a match and so continue the loop without failing, but that feels a bit > >>> sub-optimal. > >> > >> Or perhaps a better approach would be to provide something like a > >> "list_for_each_data_link()" iterator - the potential problem here (as > >> with Quentin's recent issue with the rkisp1 driver) is the assumption > >> that all links are data links, so maybe it's best to just guarantee that > >> if we can. > >> > > I agree with you, without a dedicated iterator, we're bound to repeat > > the mistake over and over. > > > > Would you like to submit a patch to add that iterator, or should I ? I'd > > name it for_each_media_entity_data_link() or something similar. > > I've started one - I'll send it later (using your function name though, > naming things was never my strong suit!) I've modified this patch to skip non-data links manually, to avoid depending on your series. If the iterator gets merged first, I'll update the media_entity_remote_pad_unique() helper to use it. > >>>> + > >>>> + if (type == MEDIA_PAD_FL_SOURCE) { > >>>> + local_pad = link->sink; > >>>> + remote_pad = link->source; > >>>> + } else { > >>>> + local_pad = link->source; > >>>> + remote_pad = link->sink; > >>>> + } > >>>> + > >>>> + if (local_pad->entity == entity) { > >>>> + if (pad) > >>>> + return ERR_PTR(-ENOTUNIQ); > >>>> + > >>>> + pad = remote_pad; > >>>> + } > >>>> + } > >>>> + > >>>> + if (!pad) > >>>> + return ERR_PTR(-ENOLINK); > >>>> + > >>>> + return pad; > >>>> +} > >>>> +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > >>>> + > >>>> static void media_interface_init(struct media_device *mdev, > >>>> struct media_interface *intf, > >>>> u32 gobj_type, > >>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h > >>>> index a9a1c0ec5d1c..33d5f52719a0 100644 > >>>> --- a/include/media/media-entity.h > >>>> +++ b/include/media/media-entity.h > >>>> @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > >>>> */ > >>>> struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > >>>> > >>>> +/** > >>>> + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > >>>> + * @entity: The entity > >>>> + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > >>>> + * > >>>> + * Search for and return a remote pad of @type connected to @entity through an > >>>> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >>>> + * is returned. > >>>> + * > >>>> + * The uniqueness constraint makes this helper function suitable for entities > >>>> + * that support a single active source or sink at a time. > >>>> + * > >>>> + * Return: A pointer to the remote pad, or one of the following error pointers > >>>> + * if an error occurs: > >>>> + * > >>>> + * * -ENOTUNIQ - Multiple links are enabled > >>>> + * * -ENOLINK - No connected pad found > >>>> + */ > >>>> +struct media_pad * > >>>> +media_entity_remote_pad_unique(const struct media_entity *entity, > >>>> + unsigned int type); > >>>> + > >>>> +/** > >>>> + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > >>>> + * @entity: The entity > >>>> + * > >>>> + * Search for and return a remote source pad connected to @entity through an > >>>> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >>>> + * is returned. > >>>> + * > >>>> + * The uniqueness constraint makes this helper function suitable for entities > >>>> + * that support a single active source at a time. > >>>> + * > >>>> + * Return: A pointer to the remote pad, or one of the following error pointers > >>>> + * if an error occurs: > >>>> + * > >>>> + * * -ENOTUNIQ - Multiple links are enabled > >>>> + * * -ENOLINK - No connected pad found > >>>> + */ > >>>> +static inline struct media_pad * > >>>> +media_entity_remote_source_pad(const struct media_entity *entity) > >>>> +{ > >>>> + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > >>>> +} > >>>> + > >>>> /** > >>>> * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > >>>> * @entity: The entity
Hi Paul, On Thu, Jun 16, 2022 at 01:41:56AM +0300, Laurent Pinchart wrote: > On Wed, Jun 15, 2022 at 04:10:33AM +0900, Paul Elder wrote: > > Add a register dump file in debugfs for some of the buffer-related > > registers in MI, for the base address, the size, and the offset. Also > > dump the appropriate shadow registers. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > .../platform/rockchip/rkisp1/rkisp1-debug.c | 21 +++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c > > index e76dc2b164b6..1a59c00fabdd 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-debug.c > > @@ -121,6 +121,24 @@ static int rkisp1_debug_dump_rsz_regs_show(struct seq_file *m, void *p) > > } > > DEFINE_SHOW_ATTRIBUTE(rkisp1_debug_dump_rsz_regs); > > > > +static int rkisp1_debug_dump_mi_mp_y_offs_cnt_show(struct seq_file *m, void *p) > > +{ > > + static const struct rkisp1_debug_register registers[] = { > > + RKISP1_DEBUG_REG(MI_MP_Y_BASE_AD_INIT), > > + RKISP1_DEBUG_REG(MI_MP_Y_BASE_AD_INIT2), > > + RKISP1_DEBUG_REG(MI_MP_Y_BASE_AD_SHD), > > + RKISP1_DEBUG_REG(MI_MP_Y_SIZE_INIT), > > + RKISP1_DEBUG_REG(MI_MP_Y_SIZE_INIT), > > + RKISP1_DEBUG_REG(MI_MP_Y_SIZE_SHD), > > + RKISP1_DEBUG_REG(MI_MP_Y_OFFS_CNT_SHD), > > + { /* Sentinel */ }, > > + }; > > + struct rkisp1_device *rkisp1 = m->private; > > + > > + return rkisp1_debug_dump_regs(rkisp1, m, 0, registers); > > +} > > +DEFINE_SHOW_ATTRIBUTE(rkisp1_debug_dump_mi_mp_y_offs_cnt); > > + > > #define RKISP1_DEBUG_DATA_COUNT_BINS 32 > > #define RKISP1_DEBUG_DATA_COUNT_STEP (4096 / RKISP1_DEBUG_DATA_COUNT_BINS) > > > > @@ -214,6 +232,9 @@ void rkisp1_debug_init(struct rkisp1_device *rkisp1) > > debugfs_create_file("srsz", 0444, regs_dir, > > &rkisp1->resizer_devs[RKISP1_SELFPATH], > > &rkisp1_debug_dump_rsz_regs_fops); > > + > > + debugfs_create_file("mi_mp_y_bufs", 0444, regs_dir, rkisp1, > > + &rkisp1_debug_dump_mi_mp_y_offs_cnt_fops); > > That's a very specialized file. I wonder if we should call it just > "mi_mp" if it needs to be extended later with other memory interface > registers for the main path. Or maybe even just "mi", to cover the self > path too ? The latter may be a tad too generic. What do you think ? Ping > > } > > > > void rkisp1_debug_cleanup(struct rkisp1_device *rkisp1)
Hi Paul, Thank you for the patch. On Wed, Jun 15, 2022 at 04:11:25AM +0900, Paul Elder wrote: > On the ISP that is integrated in the i.MX8MP, the DMA base addresses are > encoded in 34-bit. Shift them to the left by 2 bits so that they can be I think you meant right, not left. > contained in 32 bits. The important part here is that this is how the address is encoded in the hardware. I suppose it's obvious, otherwise it woudln't work at all, but maybe it could be explained more explicitly ? On the ISP that is integrated in the i.MX8MP, DMA addresses have been extended to 34 bits, with the 32 MSBs stored in the DMA address registers and the 2 LSBs set to 0. Shift the buffer addresses right by 2 on that platform. > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../platform/rockchip/rkisp1/rkisp1-capture.c | 19 +++++++++++-------- > .../platform/rockchip/rkisp1/rkisp1-common.h | 1 + > .../platform/rockchip/rkisp1/rkisp1-dev.c | 3 ++- > 3 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > index 35cec263c563..234b1f8488cb 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > @@ -624,6 +624,9 @@ static void rkisp1_dummy_buf_destroy(struct rkisp1_capture *cap) > > static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > { > + u8 shift = cap->rkisp1->info->features & RKISP1_FEATURE_DMA_34BIT ? > + 2 : 0; > + > cap->buf.curr = cap->buf.next; > cap->buf.next = NULL; > > @@ -636,7 +639,7 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > buff_addr = cap->buf.next->buff_addr; > > rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, > - buff_addr[RKISP1_PLANE_Y]); > + buff_addr[RKISP1_PLANE_Y] >> shift); > /* > * In order to support grey format we capture > * YUV422 planar format from the camera and > @@ -645,17 +648,17 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) { > rkisp1_write(cap->rkisp1, > cap->config->mi.cb_base_ad_init, > - cap->buf.dummy.dma_addr); > + cap->buf.dummy.dma_addr >> shift); > rkisp1_write(cap->rkisp1, > cap->config->mi.cr_base_ad_init, > - cap->buf.dummy.dma_addr); > + cap->buf.dummy.dma_addr >> shift); > } else { > rkisp1_write(cap->rkisp1, > cap->config->mi.cb_base_ad_init, > - buff_addr[RKISP1_PLANE_CB]); > + buff_addr[RKISP1_PLANE_CB] >> shift); > rkisp1_write(cap->rkisp1, > cap->config->mi.cr_base_ad_init, > - buff_addr[RKISP1_PLANE_CR]); > + buff_addr[RKISP1_PLANE_CR] >> shift); > } > } else { > /* > @@ -663,11 +666,11 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) > * throw data if there is no available buffer. > */ > rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, > - cap->buf.dummy.dma_addr); > + cap->buf.dummy.dma_addr >> shift); > rkisp1_write(cap->rkisp1, cap->config->mi.cb_base_ad_init, > - cap->buf.dummy.dma_addr); > + cap->buf.dummy.dma_addr >> shift); > rkisp1_write(cap->rkisp1, cap->config->mi.cr_base_ad_init, > - cap->buf.dummy.dma_addr); > + cap->buf.dummy.dma_addr >> shift); > } > > /* Set plane offsets */ > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > index 96657e55a5b0..0b834579d08c 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > @@ -113,6 +113,7 @@ enum rkisp1_feature { > RKISP1_FEATURE_DUAL_CROP = BIT(1), > RKISP1_FEATURE_RSZ_CROP = BIT(2), > RKISP1_FEATURE_MAIN_STRIDE = BIT(3), > + RKISP1_FEATURE_DMA_34BIT = BIT(4), > }; > > /* > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > index d68a805e8b6b..4c77aa2bc50a 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > @@ -506,7 +506,8 @@ static const struct rkisp1_info imx8mp_isp_info = { > .isr_size = ARRAY_SIZE(imx8mp_isp_isrs), > .isp_ver = IMX8MP_V10, > .features = RKISP1_FEATURE_RSZ_CROP > - | RKISP1_FEATURE_MAIN_STRIDE, > + | RKISP1_FEATURE_MAIN_STRIDE > + | RKISP1_FEATURE_DMA_34BIT, > }; > > static const struct of_device_id rkisp1_of_match[] = {
Hi Paul, Thank you for the patch. On Wed, Jun 15, 2022 at 04:11:23AM +0900, Paul Elder wrote: > The ISP version in the i.MX8MP has a set of registers currently not > handled by the driver for output size configuration. Add a feature flag > to determine if the ISP requires this, and set the registers based on > that. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c | 8 ++++++++ > drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 1 + > drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 3 ++- > drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h | 9 +++++++++ > 4 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > index 9edaa95fa44c..35cec263c563 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > @@ -420,6 +420,14 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap) > rkisp1_write(rkisp1, cap->config->mi.cr_size_init, > rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR)); > > + if (rkisp1->info->features & RKISP1_FEATURE_MAIN_STRIDE) { > + rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_LLENGTH, pixm->width); > + rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_WIDTH, pixm->width); > + rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_HEIGHT, pixm->height); > + rkisp1_write(rkisp1, RKISP1_CIF_MI_MP_Y_PIC_SIZE, > + pixm->width * pixm->height); > + } > + > rkisp1_irq_frame_end_enable(cap); > > /* set uv swapping for semiplanar formats */ > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > index e4f422bed09a..96657e55a5b0 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > @@ -112,6 +112,7 @@ enum rkisp1_feature { > RKISP1_FEATURE_MIPI_CSI2 = BIT(0), > RKISP1_FEATURE_DUAL_CROP = BIT(1), > RKISP1_FEATURE_RSZ_CROP = BIT(2), > + RKISP1_FEATURE_MAIN_STRIDE = BIT(3), > }; > > /* > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > index 5abe33f5fed4..d68a805e8b6b 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > @@ -505,7 +505,8 @@ static const struct rkisp1_info imx8mp_isp_info = { > .isrs = imx8mp_isp_isrs, > .isr_size = ARRAY_SIZE(imx8mp_isp_isrs), > .isp_ver = IMX8MP_V10, > - .features = RKISP1_FEATURE_RSZ_CROP, > + .features = RKISP1_FEATURE_RSZ_CROP > + | RKISP1_FEATURE_MAIN_STRIDE, > }; > > static const struct of_device_id rkisp1_of_match[] = { > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > index 1fc54ab22b6d..5c2195019723 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > @@ -1013,6 +1013,15 @@ > #define RKISP1_CIF_MI_SP_CB_BASE_AD_INIT2 (RKISP1_CIF_MI_BASE + 0x00000140) > #define RKISP1_CIF_MI_SP_CR_BASE_AD_INIT2 (RKISP1_CIF_MI_BASE + 0x00000144) > #define RKISP1_CIF_MI_XTD_FORMAT_CTRL (RKISP1_CIF_MI_BASE + 0x00000148) > +#define RKISP1_CIF_MI_MP_HANDSHAKE_0 (RKISP1_CIF_MI_BASE + 0x0000014C) > +#define RKISP1_CIF_MI_MP_Y_LLENGTH (RKISP1_CIF_MI_BASE + 0x00000150) > +#define RKISP1_CIF_MI_MP_Y_SLICE_OFFSET (RKISP1_CIF_MI_BASE + 0x00000154) > +#define RKISP1_CIF_MI_MP_C_SLICE_OFFSET (RKISP1_CIF_MI_BASE + 0x00000158) > +#define RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT (RKISP1_CIF_MI_BASE + 0x0000015C) > +#define RKISP1_CIF_MI_MP_OUTPUT_FIFO_SIZE (RKISP1_CIF_MI_BASE + 0x00000160) > +#define RKISP1_CIF_MI_MP_Y_PIC_WIDTH (RKISP1_CIF_MI_BASE + 0x00000164) > +#define RKISP1_CIF_MI_MP_Y_PIC_HEIGHT (RKISP1_CIF_MI_BASE + 0x00000168) > +#define RKISP1_CIF_MI_MP_Y_PIC_SIZE (RKISP1_CIF_MI_BASE + 0x0000016C) > > #define RKISP1_CIF_SMIA_BASE 0x00001A00 > #define RKISP1_CIF_SMIA_CTRL (RKISP1_CIF_SMIA_BASE + 0x00000000)
On 15.06.2022 04:10, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >The active_sensor field of the rkisp1_device structure points to the ASD >data for the active source. The source may however not be a sensor, so >the naming is a bit confusing. Furthermore, the driver doesn't need to >access the full ASD from the active_sensor field, only the subdev >pointer is needed, when stopping streaming. > >Rename the field to source, and turn it into a v4l2_subdev pointer. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com> >--- > .../platform/rockchip/rkisp1/rkisp1-common.h | 4 +-- > .../platform/rockchip/rkisp1/rkisp1-isp.c | 27 +++++++++---------- > 2 files changed, 14 insertions(+), 17 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >index dbf1baca623a..7a6f55a31bb0 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >@@ -419,7 +419,7 @@ struct rkisp1_debug { > * @v4l2_dev: v4l2_device variable > * @media_dev: media_device variable > * @notifier: a notifier to register on the v4l2-async API to be notified on the sensor >- * @active_sensor: sensor in-use, set when streaming on >+ * @source: source subdev in-use, set when starting streaming > * @csi: internal CSI-2 receiver > * @isp: ISP sub-device > * @resizer_devs: resizer sub-devices >@@ -439,7 +439,7 @@ struct rkisp1_device { > struct v4l2_device v4l2_dev; > struct media_device media_dev; > struct v4l2_async_notifier notifier; >- struct rkisp1_sensor_async *active_sensor; >+ struct v4l2_subdev *source; > struct rkisp1_csi csi; > struct rkisp1_isp isp; > struct rkisp1_resizer resizer_devs[2]; >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >index f6d1c93dd99d..4f12fc0b7694 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c >@@ -58,7 +58,7 @@ > * Helpers > */ > >-static struct v4l2_subdev *rkisp1_get_remote_sensor(struct v4l2_subdev *sd) >+static struct v4l2_subdev *rkisp1_get_remote_source(struct v4l2_subdev *sd) > { > struct media_pad *local, *remote; > struct media_entity *sensor_me; >@@ -749,12 +749,11 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > struct rkisp1_device *rkisp1 = > container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev); > struct rkisp1_isp *isp = &rkisp1->isp; >- struct v4l2_subdev *sensor_sd; >+ struct rkisp1_sensor_async *asd; > int ret; > > if (!enable) { >- v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, >- false); >+ v4l2_subdev_call(rkisp1->source, video, s_stream, false); > > rkisp1_csi_stop(&rkisp1->csi); > rkisp1_isp_stop(rkisp1); >@@ -762,35 +761,33 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > return 0; > } > >- sensor_sd = rkisp1_get_remote_sensor(sd); >- if (!sensor_sd) { >- dev_warn(rkisp1->dev, "No link between isp and sensor\n"); >+ rkisp1->source = rkisp1_get_remote_source(sd); >+ if (!rkisp1->source) { >+ dev_warn(rkisp1->dev, "No link between isp and source\n"); > return -ENODEV; > } > >- rkisp1->active_sensor = container_of(sensor_sd->asd, >- struct rkisp1_sensor_async, asd); >+ asd = container_of(rkisp1->source->asd, struct rkisp1_sensor_async, >+ asd); > >- if (rkisp1->active_sensor->mbus_type != V4L2_MBUS_CSI2_DPHY) >+ if (asd->mbus_type != V4L2_MBUS_CSI2_DPHY) > return -EINVAL; > > rkisp1->isp.frame_sequence = -1; > mutex_lock(&isp->ops_lock); >- ret = rkisp1_config_cif(rkisp1, rkisp1->active_sensor->mbus_type, >- rkisp1->active_sensor->mbus_flags); >+ ret = rkisp1_config_cif(rkisp1, asd->mbus_type, asd->mbus_flags); > if (ret) > goto mutex_unlock; > > rkisp1_isp_start(rkisp1); > >- ret = rkisp1_csi_start(&rkisp1->csi, rkisp1->active_sensor); >+ ret = rkisp1_csi_start(&rkisp1->csi, asd); > if (ret) { > rkisp1_isp_stop(rkisp1); > goto mutex_unlock; > } > >- ret = v4l2_subdev_call(rkisp1->active_sensor->sd, video, s_stream, >- true); >+ ret = v4l2_subdev_call(rkisp1->source, video, s_stream, true); > if (ret) { > rkisp1_isp_stop(rkisp1); > rkisp1_csi_stop(&rkisp1->csi); >-- >2.30.2 >
On 15.06.2022 04:10, Paul Elder wrote: >From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >When a sensor is bound, its source pad is retrieved in the .bound() >operation with a call to media_entity_get_fwnode_pad(). The function >should be called with the source endpoint fwnode of the sensor, but is >instead called with the sensor's device fwnode. > >Fix this, which involves storing a reference to the source endpoint >fwnode in the rkisp1_sensor_async structure, and thus implementing the >subdev notifier .destroy() operation to release the reference. > >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >--- > .../platform/rockchip/rkisp1/rkisp1-common.h | 2 ++ > .../platform/rockchip/rkisp1/rkisp1-dev.c | 28 ++++++++++++++++--- > 2 files changed, 26 insertions(+), 4 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >index ba11baf75fa9..60c5462e1746 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >@@ -117,6 +117,7 @@ struct rkisp1_info { > * > * @asd: async_subdev variable for the sensor > * @index: index of the sensor (counting sensor found in DT) >+ * @source_ep: fwnode for the sensor source endpoint > * @lanes: number of lanes > * @mbus_type: type of bus (currently only CSI2 is supported) > * @mbus_flags: media bus (V4L2_MBUS_*) flags >@@ -127,6 +128,7 @@ struct rkisp1_info { > struct rkisp1_sensor_async { > struct v4l2_async_subdev asd; > unsigned int index; >+ struct fwnode_handle *source_ep; > unsigned int lanes; > enum v4l2_mbus_type mbus_type; > unsigned int mbus_flags; >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >index 386c1c17aec2..0f3e45cdbf2a 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >@@ -138,7 +138,7 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier, > phy_init(s_asd->dphy); > > /* Create the link to the sensor. */ >- source_pad = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, >+ source_pad = media_entity_get_fwnode_pad(&sd->entity, s_asd->source_ep, > MEDIA_PAD_FL_SOURCE); > if (source_pad < 0) { > dev_err(rkisp1->dev, "failed to find source pad for %s\n", >@@ -170,10 +170,19 @@ static int rkisp1_subdev_notifier_complete(struct v4l2_async_notifier *notifier) > return v4l2_device_register_subdev_nodes(&rkisp1->v4l2_dev); > } > >+static void rkisp1_subdev_notifier_destroy(struct v4l2_async_subdev *asd) >+{ >+ struct rkisp1_sensor_async *rk_asd = >+ container_of(asd, struct rkisp1_sensor_async, asd); >+ >+ fwnode_handle_put(rk_asd->source_ep); >+} >+ > static const struct v4l2_async_notifier_operations rkisp1_subdev_notifier_ops = { > .bound = rkisp1_subdev_notifier_bound, > .unbind = rkisp1_subdev_notifier_unbind, > .complete = rkisp1_subdev_notifier_complete, >+ .destroy = rkisp1_subdev_notifier_destroy, > }; > > static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1) >@@ -190,6 +199,7 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1) > .bus_type = V4L2_MBUS_CSI2_DPHY > }; > struct rkisp1_sensor_async *rk_asd; >+ struct fwnode_handle *source = NULL; > struct fwnode_handle *ep; > > ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(rkisp1->dev), >@@ -202,15 +212,24 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1) > if (ret) > goto err_parse; > >- rk_asd = v4l2_async_nf_add_fwnode_remote(ntf, ep, >- struct >- rkisp1_sensor_async); >+ source = fwnode_graph_get_remote_endpoint(ep); >+ if (!source) { >+ dev_err(rkisp1->dev, >+ "endpoint %pfw has no remote endpoint\n", >+ ep); >+ ret = -ENODEV; >+ goto err_parse; >+ } >+ >+ rk_asd = v4l2_async_nf_add_fwnode(ntf, source, >+ struct rkisp1_sensor_async); > if (IS_ERR(rk_asd)) { > ret = PTR_ERR(rk_asd); > goto err_parse; > } > > rk_asd->index = index++; >+ rk_asd->source_ep = source; here do 'source = NULL', see reason below > rk_asd->mbus_type = vep.bus_type; > rk_asd->mbus_flags = vep.bus.mipi_csi2.flags; > rk_asd->lanes = vep.bus.mipi_csi2.num_data_lanes; >@@ -225,6 +244,7 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1) > continue; > err_parse: > fwnode_handle_put(ep); >+ fwnode_handle_put(source); if v4l2_fwnode_endpoint_parse fails then here you put the source of previous iteration thanks, Dafna > v4l2_async_nf_cleanup(ntf); > return ret; > } >-- >2.30.2 >
On 26.06.2022 14:38, Laurent Pinchart wrote: >Hi Paul, > >Thank you for the patch. > >On Wed, Jun 15, 2022 at 04:11:25AM +0900, Paul Elder wrote: >> On the ISP that is integrated in the i.MX8MP, the DMA base addresses are >> encoded in 34-bit. Shift them to the left by 2 bits so that they can be > >I think you meant right, not left. > >> contained in 32 bits. > >The important part here is that this is how the address is encoded in >the hardware. I suppose it's obvious, otherwise it woudln't work at all, >but maybe it could be explained more explicitly ? > >On the ISP that is integrated in the i.MX8MP, DMA addresses have been >extended to 34 bits, with the 32 MSBs stored in the DMA address >registers and the 2 LSBs set to 0. Shift the buffer addresses right by 2 >on that platform. If the 32 MSB are all stored in the in the dma address then why do we need to shift? > >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > >Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- >> .../platform/rockchip/rkisp1/rkisp1-capture.c | 19 +++++++++++-------- >> .../platform/rockchip/rkisp1/rkisp1-common.h | 1 + >> .../platform/rockchip/rkisp1/rkisp1-dev.c | 3 ++- >> 3 files changed, 14 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >> index 35cec263c563..234b1f8488cb 100644 >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c >> @@ -624,6 +624,9 @@ static void rkisp1_dummy_buf_destroy(struct rkisp1_capture *cap) >> >> static void rkisp1_set_next_buf(struct rkisp1_capture *cap) >> { >> + u8 shift = cap->rkisp1->info->features & RKISP1_FEATURE_DMA_34BIT ? >> + 2 : 0; >> + >> cap->buf.curr = cap->buf.next; >> cap->buf.next = NULL; >> >> @@ -636,7 +639,7 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) >> buff_addr = cap->buf.next->buff_addr; >> >> rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, >> - buff_addr[RKISP1_PLANE_Y]); >> + buff_addr[RKISP1_PLANE_Y] >> shift); >> /* >> * In order to support grey format we capture >> * YUV422 planar format from the camera and >> @@ -645,17 +648,17 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) >> if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) { >> rkisp1_write(cap->rkisp1, >> cap->config->mi.cb_base_ad_init, >> - cap->buf.dummy.dma_addr); >> + cap->buf.dummy.dma_addr >> shift); >> rkisp1_write(cap->rkisp1, >> cap->config->mi.cr_base_ad_init, >> - cap->buf.dummy.dma_addr); >> + cap->buf.dummy.dma_addr >> shift); >> } else { >> rkisp1_write(cap->rkisp1, >> cap->config->mi.cb_base_ad_init, >> - buff_addr[RKISP1_PLANE_CB]); >> + buff_addr[RKISP1_PLANE_CB] >> shift); >> rkisp1_write(cap->rkisp1, >> cap->config->mi.cr_base_ad_init, >> - buff_addr[RKISP1_PLANE_CR]); >> + buff_addr[RKISP1_PLANE_CR] >> shift); >> } >> } else { >> /* >> @@ -663,11 +666,11 @@ static void rkisp1_set_next_buf(struct rkisp1_capture *cap) >> * throw data if there is no available buffer. >> */ >> rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init, >> - cap->buf.dummy.dma_addr); >> + cap->buf.dummy.dma_addr >> shift); >> rkisp1_write(cap->rkisp1, cap->config->mi.cb_base_ad_init, >> - cap->buf.dummy.dma_addr); >> + cap->buf.dummy.dma_addr >> shift); >> rkisp1_write(cap->rkisp1, cap->config->mi.cr_base_ad_init, >> - cap->buf.dummy.dma_addr); >> + cap->buf.dummy.dma_addr >> shift); >> } >> >> /* Set plane offsets */ >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >> index 96657e55a5b0..0b834579d08c 100644 >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >> @@ -113,6 +113,7 @@ enum rkisp1_feature { >> RKISP1_FEATURE_DUAL_CROP = BIT(1), >> RKISP1_FEATURE_RSZ_CROP = BIT(2), >> RKISP1_FEATURE_MAIN_STRIDE = BIT(3), >> + RKISP1_FEATURE_DMA_34BIT = BIT(4), doc this field thanks, Dafna >> }; >> >> /* >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> index d68a805e8b6b..4c77aa2bc50a 100644 >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> @@ -506,7 +506,8 @@ static const struct rkisp1_info imx8mp_isp_info = { >> .isr_size = ARRAY_SIZE(imx8mp_isp_isrs), >> .isp_ver = IMX8MP_V10, >> .features = RKISP1_FEATURE_RSZ_CROP >> - | RKISP1_FEATURE_MAIN_STRIDE, >> + | RKISP1_FEATURE_MAIN_STRIDE >> + | RKISP1_FEATURE_DMA_34BIT, >> }; >> >> static const struct of_device_id rkisp1_of_match[] = { > >-- >Regards, > >Laurent Pinchart
Hi Dafna, On Fri, Jul 01, 2022 at 07:36:41AM +0300, Dafna Hirschfeld wrote: > On 15.06.2022 04:10, Paul Elder wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > When a sensor is bound, its source pad is retrieved in the .bound() > > operation with a call to media_entity_get_fwnode_pad(). The function > > should be called with the source endpoint fwnode of the sensor, but is > > instead called with the sensor's device fwnode. > > > > Fix this, which involves storing a reference to the source endpoint > > fwnode in the rkisp1_sensor_async structure, and thus implementing the > > subdev notifier .destroy() operation to release the reference. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > .../platform/rockchip/rkisp1/rkisp1-common.h | 2 ++ > > .../platform/rockchip/rkisp1/rkisp1-dev.c | 28 ++++++++++++++++--- > > 2 files changed, 26 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > index ba11baf75fa9..60c5462e1746 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > @@ -117,6 +117,7 @@ struct rkisp1_info { > > * > > * @asd: async_subdev variable for the sensor > > * @index: index of the sensor (counting sensor found in DT) > > + * @source_ep: fwnode for the sensor source endpoint > > * @lanes: number of lanes > > * @mbus_type: type of bus (currently only CSI2 is supported) > > * @mbus_flags: media bus (V4L2_MBUS_*) flags > > @@ -127,6 +128,7 @@ struct rkisp1_info { > > struct rkisp1_sensor_async { > > struct v4l2_async_subdev asd; > > unsigned int index; > > + struct fwnode_handle *source_ep; > > unsigned int lanes; > > enum v4l2_mbus_type mbus_type; > > unsigned int mbus_flags; > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > > index 386c1c17aec2..0f3e45cdbf2a 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > > @@ -138,7 +138,7 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier, > > phy_init(s_asd->dphy); > > > > /* Create the link to the sensor. */ > > - source_pad = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, > > + source_pad = media_entity_get_fwnode_pad(&sd->entity, s_asd->source_ep, > > MEDIA_PAD_FL_SOURCE); > > if (source_pad < 0) { > > dev_err(rkisp1->dev, "failed to find source pad for %s\n", > > @@ -170,10 +170,19 @@ static int rkisp1_subdev_notifier_complete(struct v4l2_async_notifier *notifier) > > return v4l2_device_register_subdev_nodes(&rkisp1->v4l2_dev); > > } > > > > +static void rkisp1_subdev_notifier_destroy(struct v4l2_async_subdev *asd) > > +{ > > + struct rkisp1_sensor_async *rk_asd = > > + container_of(asd, struct rkisp1_sensor_async, asd); > > + > > + fwnode_handle_put(rk_asd->source_ep); > > +} > > + > > static const struct v4l2_async_notifier_operations rkisp1_subdev_notifier_ops = { > > .bound = rkisp1_subdev_notifier_bound, > > .unbind = rkisp1_subdev_notifier_unbind, > > .complete = rkisp1_subdev_notifier_complete, > > + .destroy = rkisp1_subdev_notifier_destroy, > > }; > > > > static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1) > > @@ -190,6 +199,7 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1) > > .bus_type = V4L2_MBUS_CSI2_DPHY > > }; > > struct rkisp1_sensor_async *rk_asd; > > + struct fwnode_handle *source = NULL; > > struct fwnode_handle *ep; > > > > ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(rkisp1->dev), > > @@ -202,15 +212,24 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1) > > if (ret) > > goto err_parse; > > > > - rk_asd = v4l2_async_nf_add_fwnode_remote(ntf, ep, > > - struct > > - rkisp1_sensor_async); > > + source = fwnode_graph_get_remote_endpoint(ep); > > + if (!source) { > > + dev_err(rkisp1->dev, > > + "endpoint %pfw has no remote endpoint\n", > > + ep); > > + ret = -ENODEV; > > + goto err_parse; > > + } > > + > > + rk_asd = v4l2_async_nf_add_fwnode(ntf, source, > > + struct rkisp1_sensor_async); > > if (IS_ERR(rk_asd)) { > > ret = PTR_ERR(rk_asd); > > goto err_parse; > > } > > > > rk_asd->index = index++; > > + rk_asd->source_ep = source; > > here do 'source = NULL', see reason below > > > rk_asd->mbus_type = vep.bus_type; > > rk_asd->mbus_flags = vep.bus.mipi_csi2.flags; > > rk_asd->lanes = vep.bus.mipi_csi2.num_data_lanes; > > @@ -225,6 +244,7 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1) > > continue; > > err_parse: > > fwnode_handle_put(ep); > > + fwnode_handle_put(source); > > if v4l2_fwnode_endpoint_parse fails then here you put the source of previous iteration source is a variable local to the loop, and it's initialized to NULL in the declaration, so it will be reset to NULL at every iteration. > > v4l2_async_nf_cleanup(ntf); > > return ret; > > }
Hello, This series depends on v4 of "media: rkisp1: Misc bug fixes and cleanups" [1]. The ISP integrated in the i.MX8MP is nearly the same as the rkisp1, and so we can reuse the rkisp1 driver for it. This series does some cleanup and refactoring of the rkisp1 driver, (patches 1/55 to 46/55), and then adds support for the i.MX8MP (patches 47/55 to 55/55). Paul [1] https://lore.kernel.org/linux-media/Ymbxs2p9Tuf331qM@pendragon.ideasonboard.com/T/ Laurent Pinchart (38): media: rkisp1: Enable compilation on ARCH_MXC media: rkisp1: Disable runtime PM in probe error path media: rkisp1: Read the ID register at probe time instead of streamon media: rkisp1: Rename rkisp1_match_data to rkisp1_info media: rkisp1: Access ISP version from info pointer media: rkisp1: cap: Print debug message on failed link validation media: rkisp1: Move sensor .s_stream() call to ISP media: rkisp1: Reject sensors without pixel rate control at bound time media: rkisp1: Create link from sensor to ISP at notifier bound time media: rkisp1: Create internal links at probe time media: rkisp1: Rename rkisp1_subdev_notifier() to rkisp1_subdev_notifier_register() media: v4l2-async: Add notifier operation to destroy asd instances media: rkisp1: Fix sensor source pad retrieval at bound time media: rkisp1: isp: Start CSI-2 receiver before ISP media: rkisp1: csi: Handle CSI-2 RX configuration fully in rkisp1-csi.c media: rkisp1: csi: Rename CSI functions with a common rkisp1_csi prefix media: rkisp1: csi: Move start delay to rkisp1_csi_start() media: rkisp1: csi: Pass sensor pointer to rkisp1_csi_config() media: rkisp1: csi: Constify argument to rkisp1_csi_start() media: rkisp1: isp: Don't initialize ret to 0 in rkisp1_isp_s_stream() media: rkisp1: isp: Pass mbus type and flags to rkisp1_config_cif() media: rkisp1: isp: Rename rkisp1_device.active_sensor to source media: rkisp1: isp: Add container_of wrapper to cast subdev to rkisp1_isp media: rkisp1: isp: Add rkisp1_device backpointer to rkisp1_isp media: rkisp1: isp: Pass rkisp1_isp pointer to internal ISP functions media: rkisp1: isp: Move input configuration to rkisp1_config_isp() media: rkisp1: isp: Merge ISP_ACQ_PROP configuration in single variable media: rkisp1: isp: Initialize some variables at declaration time media: rkisp1: isp: Fix whitespace issues media: rkisp1: isp: Constify various local variables media: rkisp1: isp: Rename rkisp1_get_remote_source() media: mc-entity: Add a new helper function to get a remote pad media: mc-entity: Add a new helper function to get a remote pad for a pad media: rkisp1: isp: Disallow multiple active sources media: rkisp1: csi: Plumb the CSI RX subdev media: rkisp1: Add infrastructure to support ISP features media: rkisp1: Make the internal CSI-2 receiver optional media: rkisp1: Configure gasket on i.MX8MP Paul Elder (17): media: rkisp1: debug: Add dump file in debugfs for MI buffer registers media: rkisp1: debug: Add debugfs files to monitor MI and ISP interrupts media: rkisp1: Save info pointer in rkisp1_device media: rkisp1: Make rkisp1_isp_mbus_info common media: rkisp1: Split CSI handling to separate file media: rkisp1: csi: Implement a V4L2 subdev for the CSI receiver media: rkisp1: Use fwnode_graph_for_each_endpoint dt-bindings: media: rkisp1: Add port for parallel interface media: rkisp1: Support the ISP parallel input dt-bindings: media: rkisp1: Add i.MX8MP ISP to compatible media: rkisp1: Add match data for i.MX8MP ISP media: rkisp1: Add and set registers for crop for i.MX8MP media: rkisp1: Add and set registers for output size config on i.MX8MP media: rkisp1: Add i.MX8MP-specific registers for MI and resizer media: rkisp1: Shift DMA buffer addresses on i.MX8MP media: rkisp1: Add register definitions for the test pattern generator media: rkisp1: Fix RSZ_CTRL bits for i.MX8MP .../bindings/media/rockchip-isp1.yaml | 30 +- Documentation/driver-api/media/mc-core.rst | 4 +- .../driver-api/media/v4l2-subdev.rst | 6 + drivers/media/mc/mc-entity.c | 69 ++ .../media/platform/rockchip/rkisp1/Kconfig | 2 +- .../media/platform/rockchip/rkisp1/Makefile | 1 + .../platform/rockchip/rkisp1/rkisp1-capture.c | 49 +- .../platform/rockchip/rkisp1/rkisp1-common.c | 148 ++++ .../platform/rockchip/rkisp1/rkisp1-common.h | 130 +++- .../platform/rockchip/rkisp1/rkisp1-csi.c | 525 ++++++++++++++ .../platform/rockchip/rkisp1/rkisp1-csi.h | 28 + .../platform/rockchip/rkisp1/rkisp1-debug.c | 55 +- .../platform/rockchip/rkisp1/rkisp1-dev.c | 440 +++++++----- .../platform/rockchip/rkisp1/rkisp1-isp.c | 679 +++++++----------- .../platform/rockchip/rkisp1/rkisp1-params.c | 2 +- .../platform/rockchip/rkisp1/rkisp1-regs.h | 87 +++ .../platform/rockchip/rkisp1/rkisp1-resizer.c | 43 +- .../platform/rockchip/rkisp1/rkisp1-stats.c | 4 +- drivers/media/v4l2-core/v4l2-async.c | 10 + include/media/media-entity.h | 63 ++ include/media/v4l2-async.h | 2 + include/uapi/linux/rkisp1-config.h | 3 + 22 files changed, 1735 insertions(+), 645 deletions(-) create mode 100644 drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c create mode 100644 drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h