* [PATCH v2 01/12] media: Documentation: Align numbered list, make it a proper ReST
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 12:51 ` [PATCH v2 02/12] media: ccs: Fix driver quirk struct documentation Sakari Ailus
` (10 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Align lines for numbered list so that Sphinx produces an uniform output
for all list entries. Also indent paragraphs of such list entries for
consistency.
Also use ReST numbered list syntax for the entries.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
.../userspace-api/media/v4l/dev-subdev.rst | 49 +++++++++----------
1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
index a4f1df7093e8..43988516acdd 100644
--- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
+++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
@@ -579,20 +579,19 @@ is started.
There are three steps in configuring the streams:
-1) Set up links. Connect the pads between sub-devices using the :ref:`Media
-Controller API <media_controller>`
+1. Set up links. Connect the pads between sub-devices using the
+ :ref:`Media Controller API <media_controller>`
-2) Streams. Streams are declared and their routing is configured by
-setting the routing table for the sub-device using
-:ref:`VIDIOC_SUBDEV_S_ROUTING <VIDIOC_SUBDEV_G_ROUTING>` ioctl. Note that
-setting the routing table will reset formats and selections in the
-sub-device to default values.
+2. Streams. Streams are declared and their routing is configured by setting the
+ routing table for the sub-device using :ref:`VIDIOC_SUBDEV_S_ROUTING
+ <VIDIOC_SUBDEV_G_ROUTING>` ioctl. Note that setting the routing table will
+ reset formats and selections in the sub-device to default values.
-3) Configure formats and selections. Formats and selections of each stream
-are configured separately as documented for plain sub-devices in
-:ref:`format-propagation`. The stream ID is set to the same stream ID
-associated with either sink or source pads of routes configured using the
-:ref:`VIDIOC_SUBDEV_S_ROUTING <VIDIOC_SUBDEV_G_ROUTING>` ioctl.
+3. Configure formats and selections. Formats and selections of each stream are
+ configured separately as documented for plain sub-devices in
+ :ref:`format-propagation`. The stream ID is set to the same stream ID
+ associated with either sink or source pads of routes configured using the
+ :ref:`VIDIOC_SUBDEV_S_ROUTING <VIDIOC_SUBDEV_G_ROUTING>` ioctl.
Multiplexed streams setup example
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -618,11 +617,11 @@ modeled as V4L2 devices, exposed to userspace via /dev/videoX nodes.
To configure this pipeline, the userspace must take the following steps:
-1) Set up media links between entities: connect the sensors to the bridge,
-bridge to the receiver, and the receiver to the DMA engines. This step does
-not differ from normal non-multiplexed media controller setup.
+1. Set up media links between entities: connect the sensors to the bridge,
+ bridge to the receiver, and the receiver to the DMA engines. This step does
+ not differ from normal non-multiplexed media controller setup.
-2) Configure routing
+2. Configure routing
.. flat-table:: Bridge routing table
:header-rows: 1
@@ -656,14 +655,14 @@ not differ from normal non-multiplexed media controller setup.
- V4L2_SUBDEV_ROUTE_FL_ACTIVE
- Pixel data stream from Sensor B
-3) Configure formats and selections
+3. Configure formats and selections
-After configuring routing, the next step is configuring the formats and
-selections for the streams. This is similar to performing this step without
-streams, with just one exception: the ``stream`` field needs to be assigned
-to the value of the stream ID.
+ After configuring routing, the next step is configuring the formats and
+ selections for the streams. This is similar to performing this step without
+ streams, with just one exception: the ``stream`` field needs to be assigned
+ to the value of the stream ID.
-A common way to accomplish this is to start from the sensors and propagate the
-configurations along the stream towards the receiver,
-using :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls to configure each
-stream endpoint in each sub-device.
+ A common way to accomplish this is to start from the sensors and propagate
+ the configurations along the stream towards the receiver, using
+ :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls to configure each
+ stream endpoint in each sub-device.
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v2 02/12] media: ccs: Fix driver quirk struct documentation
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
2023-09-18 12:51 ` [PATCH v2 01/12] media: Documentation: Align numbered list, make it a proper ReST Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 12:51 ` [PATCH v2 03/12] media: ccs: Correctly initialise try compose rectangle Sakari Ailus
` (9 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Fix documentation for struct ccs_quirk, a device specific struct for
managing deviations from the standard. The flags field was drifted away
from where it should have been.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/i2c/ccs/ccs-quirk.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/media/i2c/ccs/ccs-quirk.h b/drivers/media/i2c/ccs/ccs-quirk.h
index 5838fcda92fd..0b1a64958d71 100644
--- a/drivers/media/i2c/ccs/ccs-quirk.h
+++ b/drivers/media/i2c/ccs/ccs-quirk.h
@@ -32,12 +32,10 @@ struct ccs_sensor;
* @reg: Pointer to the register to access
* @value: Register value, set by the caller on write, or
* by the quirk on read
- *
- * @flags: Quirk flags
- *
* @return: 0 on success, -ENOIOCTLCMD if no register
* access may be done by the caller (default read
* value is zero), else negative error code on error
+ * @flags: Quirk flags
*/
struct ccs_quirk {
int (*limits)(struct ccs_sensor *sensor);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v2 03/12] media: ccs: Correctly initialise try compose rectangle
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
2023-09-18 12:51 ` [PATCH v2 01/12] media: Documentation: Align numbered list, make it a proper ReST Sakari Ailus
2023-09-18 12:51 ` [PATCH v2 02/12] media: ccs: Fix driver quirk struct documentation Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 12:51 ` [PATCH v2 04/12] media: ccs: Correct error handling in ccs_register_subdev Sakari Ailus
` (8 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Initialise the try sink compose rectangle size to the sink compose
rectangle for binner and scaler sub-devices. This was missed due to the
faulty condition that lead to the compose rectangles to be initialised for
the pixel array sub-device where it is not relevant.
Fixes: ccfc97bdb5ae ("[media] smiapp: Add driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/i2c/ccs/ccs-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index 49e0d9a09530..6f8fbd82e21c 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -3097,7 +3097,7 @@ static int ccs_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
try_fmt->code = sensor->internal_csi_format->code;
try_fmt->field = V4L2_FIELD_NONE;
- if (ssd != sensor->pixel_array)
+ if (ssd == sensor->pixel_array)
continue;
try_comp = v4l2_subdev_get_try_compose(sd, fh->state, i);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v2 04/12] media: ccs: Correct error handling in ccs_register_subdev
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
` (2 preceding siblings ...)
2023-09-18 12:51 ` [PATCH v2 03/12] media: ccs: Correctly initialise try compose rectangle Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 12:51 ` [PATCH v2 05/12] media: ccs: Switch to init_cfg Sakari Ailus
` (7 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
ccs_register_subdev() did not clean up the media entity in error case, do
that now. Also switch to goto based error handling.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/i2c/ccs/ccs-core.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index 6f8fbd82e21c..3fed071b65ab 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -2968,7 +2968,7 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev, &ssd->sd);
if (rval) {
dev_err(&client->dev, "v4l2_device_register_subdev failed\n");
- return rval;
+ goto out_media_entity_cleanup;
}
rval = media_create_pad_link(&ssd->sd.entity, source_pad,
@@ -2976,11 +2976,18 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
link_flags);
if (rval) {
dev_err(&client->dev, "media_create_pad_link failed\n");
- v4l2_device_unregister_subdev(&ssd->sd);
- return rval;
+ goto out_v4l2_device_unregister_subdev;
}
return 0;
+
+out_v4l2_device_unregister_subdev:
+ v4l2_device_unregister_subdev(&ssd->sd);
+
+out_media_entity_cleanup:
+ media_entity_cleanup(&ssd->sd.entity);
+
+ return rval;
}
static void ccs_unregistered(struct v4l2_subdev *subdev)
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v2 05/12] media: ccs: Switch to init_cfg
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
` (3 preceding siblings ...)
2023-09-18 12:51 ` [PATCH v2 04/12] media: ccs: Correct error handling in ccs_register_subdev Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 13:53 ` Laurent Pinchart
2023-09-18 12:51 ` [PATCH v2 06/12] media: ccs: Use sub-device active state Sakari Ailus
` (6 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Use init_cfg() instead of manually setting up defaults during probe and
file handle open.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/i2c/ccs/ccs-core.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index 3fed071b65ab..db461b0e49c8 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -2945,7 +2945,6 @@ static int ccs_identify_module(struct ccs_sensor *sensor)
}
static const struct v4l2_subdev_ops ccs_ops;
-static const struct v4l2_subdev_internal_ops ccs_internal_ops;
static const struct media_entity_operations ccs_entity_ops;
static int ccs_register_subdev(struct ccs_sensor *sensor,
@@ -3076,13 +3075,12 @@ static void ccs_create_subdev(struct ccs_sensor *sensor,
if (ssd == sensor->src)
return;
- ssd->sd.internal_ops = &ccs_internal_ops;
ssd->sd.owner = THIS_MODULE;
ssd->sd.dev = &client->dev;
v4l2_set_subdevdata(&ssd->sd, client);
}
-static int ccs_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+static int ccs_init_cfg(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state)
{
struct ccs_subdev *ssd = to_ccs_subdev(sd);
struct ccs_sensor *sensor = ssd->sensor;
@@ -3092,9 +3090,9 @@ static int ccs_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
for (i = 0; i < ssd->npads; i++) {
struct v4l2_mbus_framefmt *try_fmt =
- v4l2_subdev_get_try_format(sd, fh->state, i);
+ v4l2_subdev_get_try_format(sd, sd_state, i);
struct v4l2_rect *try_crop =
- v4l2_subdev_get_try_crop(sd, fh->state, i);
+ v4l2_subdev_get_try_crop(sd, sd_state, i);
struct v4l2_rect *try_comp;
ccs_get_native_size(ssd, try_crop);
@@ -3107,7 +3105,7 @@ static int ccs_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
if (ssd == sensor->pixel_array)
continue;
- try_comp = v4l2_subdev_get_try_compose(sd, fh->state, i);
+ try_comp = v4l2_subdev_get_try_compose(sd, sd_state, i);
*try_comp = *try_crop;
}
@@ -3123,6 +3121,7 @@ static const struct v4l2_subdev_video_ops ccs_video_ops = {
};
static const struct v4l2_subdev_pad_ops ccs_pad_ops = {
+ .init_cfg = ccs_init_cfg,
.enum_mbus_code = ccs_enum_mbus_code,
.get_fmt = ccs_get_format,
.set_fmt = ccs_set_format,
@@ -3148,11 +3147,6 @@ static const struct media_entity_operations ccs_entity_ops = {
static const struct v4l2_subdev_internal_ops ccs_internal_src_ops = {
.registered = ccs_registered,
.unregistered = ccs_unregistered,
- .open = ccs_open,
-};
-
-static const struct v4l2_subdev_internal_ops ccs_internal_ops = {
- .open = ccs_open,
};
/* -----------------------------------------------------------------------------
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 05/12] media: ccs: Switch to init_cfg
2023-09-18 12:51 ` [PATCH v2 05/12] media: ccs: Switch to init_cfg Sakari Ailus
@ 2023-09-18 13:53 ` Laurent Pinchart
0 siblings, 0 replies; 28+ messages in thread
From: Laurent Pinchart @ 2023-09-18 13:53 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Sakari,
Thank you for the patch.
On Mon, Sep 18, 2023 at 03:51:31PM +0300, Sakari Ailus wrote:
> Use init_cfg() instead of manually setting up defaults during probe and
> file handle open.
I don't think you touch probe time in this patch. That's fine, it's
addressed in the next patch, so with the commit message updated,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/i2c/ccs/ccs-core.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index 3fed071b65ab..db461b0e49c8 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -2945,7 +2945,6 @@ static int ccs_identify_module(struct ccs_sensor *sensor)
> }
>
> static const struct v4l2_subdev_ops ccs_ops;
> -static const struct v4l2_subdev_internal_ops ccs_internal_ops;
> static const struct media_entity_operations ccs_entity_ops;
>
> static int ccs_register_subdev(struct ccs_sensor *sensor,
> @@ -3076,13 +3075,12 @@ static void ccs_create_subdev(struct ccs_sensor *sensor,
> if (ssd == sensor->src)
> return;
>
> - ssd->sd.internal_ops = &ccs_internal_ops;
> ssd->sd.owner = THIS_MODULE;
> ssd->sd.dev = &client->dev;
> v4l2_set_subdevdata(&ssd->sd, client);
> }
>
> -static int ccs_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +static int ccs_init_cfg(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state)
> {
> struct ccs_subdev *ssd = to_ccs_subdev(sd);
> struct ccs_sensor *sensor = ssd->sensor;
> @@ -3092,9 +3090,9 @@ static int ccs_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>
> for (i = 0; i < ssd->npads; i++) {
> struct v4l2_mbus_framefmt *try_fmt =
> - v4l2_subdev_get_try_format(sd, fh->state, i);
> + v4l2_subdev_get_try_format(sd, sd_state, i);
> struct v4l2_rect *try_crop =
> - v4l2_subdev_get_try_crop(sd, fh->state, i);
> + v4l2_subdev_get_try_crop(sd, sd_state, i);
> struct v4l2_rect *try_comp;
>
> ccs_get_native_size(ssd, try_crop);
> @@ -3107,7 +3105,7 @@ static int ccs_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> if (ssd == sensor->pixel_array)
> continue;
>
> - try_comp = v4l2_subdev_get_try_compose(sd, fh->state, i);
> + try_comp = v4l2_subdev_get_try_compose(sd, sd_state, i);
> *try_comp = *try_crop;
> }
>
> @@ -3123,6 +3121,7 @@ static const struct v4l2_subdev_video_ops ccs_video_ops = {
> };
>
> static const struct v4l2_subdev_pad_ops ccs_pad_ops = {
> + .init_cfg = ccs_init_cfg,
> .enum_mbus_code = ccs_enum_mbus_code,
> .get_fmt = ccs_get_format,
> .set_fmt = ccs_set_format,
> @@ -3148,11 +3147,6 @@ static const struct media_entity_operations ccs_entity_ops = {
> static const struct v4l2_subdev_internal_ops ccs_internal_src_ops = {
> .registered = ccs_registered,
> .unregistered = ccs_unregistered,
> - .open = ccs_open,
> -};
> -
> -static const struct v4l2_subdev_internal_ops ccs_internal_ops = {
> - .open = ccs_open,
> };
>
> /* -----------------------------------------------------------------------------
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 06/12] media: ccs: Use sub-device active state
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
` (4 preceding siblings ...)
2023-09-18 12:51 ` [PATCH v2 05/12] media: ccs: Switch to init_cfg Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 13:59 ` Laurent Pinchart
2023-09-18 12:51 ` [PATCH v2 07/12] media: ov2740: Enable runtime PM before registering the async subdev Sakari Ailus
` (5 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Make use of sub-device active state. In most cases the effect on need for
acquiring the mutex is non-existent as access to the driver's core data
structure still needs to be serialised.
This still removes a lot of code as the code paths for active and try
state are the same in many cases.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/i2c/ccs/ccs-core.c | 278 ++++++++++++-------------------
drivers/media/i2c/ccs/ccs.h | 4 +-
2 files changed, 103 insertions(+), 179 deletions(-)
diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index db461b0e49c8..efed75b6534c 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -508,9 +508,8 @@ static void __ccs_update_exposure_limits(struct ccs_sensor *sensor)
struct v4l2_ctrl *ctrl = sensor->exposure;
int max;
- max = sensor->pixel_array->crop[CCS_PA_PAD_SRC].height
- + sensor->vblank->val
- - CCS_LIM(sensor, COARSE_INTEGRATION_TIME_MAX_MARGIN);
+ max = sensor->pa_src.height + sensor->vblank->val -
+ CCS_LIM(sensor, COARSE_INTEGRATION_TIME_MAX_MARGIN);
__v4l2_ctrl_modify_range(ctrl, ctrl->minimum, max, ctrl->step, max);
}
@@ -728,15 +727,12 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl)
break;
case V4L2_CID_VBLANK:
rval = ccs_write(sensor, FRAME_LENGTH_LINES,
- sensor->pixel_array->crop[
- CCS_PA_PAD_SRC].height
- + ctrl->val);
+ sensor->pa_src.height + ctrl->val);
break;
case V4L2_CID_HBLANK:
rval = ccs_write(sensor, LINE_LENGTH_PCK,
- sensor->pixel_array->crop[CCS_PA_PAD_SRC].width
- + ctrl->val);
+ sensor->pa_src.width + ctrl->val);
break;
case V4L2_CID_TEST_PATTERN:
@@ -1214,15 +1210,13 @@ static void ccs_update_blanking(struct ccs_sensor *sensor)
min = max_t(int,
CCS_LIM(sensor, MIN_FRAME_BLANKING_LINES),
- min_fll - sensor->pixel_array->crop[CCS_PA_PAD_SRC].height);
- max = max_fll - sensor->pixel_array->crop[CCS_PA_PAD_SRC].height;
+ min_fll - sensor->pa_src.height);
+ max = max_fll - sensor->pa_src.height;
__v4l2_ctrl_modify_range(vblank, min, max, vblank->step, min);
- min = max_t(int,
- min_llp - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width,
- min_lbp);
- max = max_llp - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width;
+ min = max_t(int, min_llp - sensor->pa_src.width, min_lbp);
+ max = max_llp - sensor->pa_src.width;
__v4l2_ctrl_modify_range(hblank, min, max, hblank->step, min);
@@ -1246,10 +1240,8 @@ static int ccs_pll_blanking_update(struct ccs_sensor *sensor)
dev_dbg(&client->dev, "real timeperframe\t100/%d\n",
sensor->pll.pixel_rate_pixel_array /
- ((sensor->pixel_array->crop[CCS_PA_PAD_SRC].width
- + sensor->hblank->val) *
- (sensor->pixel_array->crop[CCS_PA_PAD_SRC].height
- + sensor->vblank->val) / 100));
+ ((sensor->pa_src.width + sensor->hblank->val) *
+ (sensor->pa_src.height + sensor->vblank->val) / 100));
return 0;
}
@@ -1756,28 +1748,22 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
goto out;
/* Analog crop start coordinates */
- rval = ccs_write(sensor, X_ADDR_START,
- sensor->pixel_array->crop[CCS_PA_PAD_SRC].left);
+ rval = ccs_write(sensor, X_ADDR_START, sensor->pa_src.left);
if (rval < 0)
goto out;
- rval = ccs_write(sensor, Y_ADDR_START,
- sensor->pixel_array->crop[CCS_PA_PAD_SRC].top);
+ rval = ccs_write(sensor, Y_ADDR_START, sensor->pa_src.top);
if (rval < 0)
goto out;
/* Analog crop end coordinates */
- rval = ccs_write(
- sensor, X_ADDR_END,
- sensor->pixel_array->crop[CCS_PA_PAD_SRC].left
- + sensor->pixel_array->crop[CCS_PA_PAD_SRC].width - 1);
+ rval = ccs_write(sensor, X_ADDR_END,
+ sensor->pa_src.left + sensor->pa_src.width - 1);
if (rval < 0)
goto out;
- rval = ccs_write(
- sensor, Y_ADDR_END,
- sensor->pixel_array->crop[CCS_PA_PAD_SRC].top
- + sensor->pixel_array->crop[CCS_PA_PAD_SRC].height - 1);
+ rval = ccs_write(sensor, Y_ADDR_END,
+ sensor->pa_src.top + sensor->pa_src.height - 1);
if (rval < 0)
goto out;
@@ -1789,27 +1775,23 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
/* Digital crop */
if (CCS_LIM(sensor, DIGITAL_CROP_CAPABILITY)
== CCS_DIGITAL_CROP_CAPABILITY_INPUT_CROP) {
- rval = ccs_write(
- sensor, DIGITAL_CROP_X_OFFSET,
- sensor->scaler->crop[CCS_PAD_SINK].left);
+ rval = ccs_write(sensor, DIGITAL_CROP_X_OFFSET,
+ sensor->scaler_sink.left);
if (rval < 0)
goto out;
- rval = ccs_write(
- sensor, DIGITAL_CROP_Y_OFFSET,
- sensor->scaler->crop[CCS_PAD_SINK].top);
+ rval = ccs_write(sensor, DIGITAL_CROP_Y_OFFSET,
+ sensor->scaler_sink.top);
if (rval < 0)
goto out;
- rval = ccs_write(
- sensor, DIGITAL_CROP_IMAGE_WIDTH,
- sensor->scaler->crop[CCS_PAD_SINK].width);
+ rval = ccs_write(sensor, DIGITAL_CROP_IMAGE_WIDTH,
+ sensor->scaler_sink.width);
if (rval < 0)
goto out;
- rval = ccs_write(
- sensor, DIGITAL_CROP_IMAGE_HEIGHT,
- sensor->scaler->crop[CCS_PAD_SINK].height);
+ rval = ccs_write(sensor, DIGITAL_CROP_IMAGE_HEIGHT,
+ sensor->scaler_sink.height);
if (rval < 0)
goto out;
}
@@ -1827,12 +1809,10 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
}
/* Output size from sensor */
- rval = ccs_write(sensor, X_OUTPUT_SIZE,
- sensor->src->crop[CCS_PAD_SRC].width);
+ rval = ccs_write(sensor, X_OUTPUT_SIZE, sensor->src_src.width);
if (rval < 0)
goto out;
- rval = ccs_write(sensor, Y_OUTPUT_SIZE,
- sensor->src->crop[CCS_PAD_SRC].height);
+ rval = ccs_write(sensor, Y_OUTPUT_SIZE, sensor->src_src.height);
if (rval < 0)
goto out;
@@ -2053,24 +2033,8 @@ static int __ccs_get_format(struct v4l2_subdev *subdev,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_format *fmt)
{
- struct ccs_subdev *ssd = to_ccs_subdev(subdev);
-
- if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
- fmt->format = *v4l2_subdev_get_try_format(subdev, sd_state,
- fmt->pad);
- } else {
- struct v4l2_rect *r;
-
- if (fmt->pad == ssd->source_pad)
- r = &ssd->crop[ssd->source_pad];
- else
- r = &ssd->sink_fmt;
-
- fmt->format.code = __ccs_get_mbus_code(subdev, fmt->pad);
- fmt->format.width = r->width;
- fmt->format.height = r->height;
- fmt->format.field = V4L2_FIELD_NONE;
- }
+ fmt->format = *v4l2_subdev_get_pad_format(subdev, sd_state, fmt->pad);
+ fmt->format.code = __ccs_get_mbus_code(subdev, fmt->pad);
return 0;
}
@@ -2092,28 +2056,18 @@ static int ccs_get_format(struct v4l2_subdev *subdev,
static void ccs_get_crop_compose(struct v4l2_subdev *subdev,
struct v4l2_subdev_state *sd_state,
struct v4l2_rect **crops,
- struct v4l2_rect **comps, int which)
+ struct v4l2_rect **comps)
{
struct ccs_subdev *ssd = to_ccs_subdev(subdev);
unsigned int i;
- if (which == V4L2_SUBDEV_FORMAT_ACTIVE) {
- if (crops)
- for (i = 0; i < subdev->entity.num_pads; i++)
- crops[i] = &ssd->crop[i];
- if (comps)
- *comps = &ssd->compose;
- } else {
- if (crops) {
- for (i = 0; i < subdev->entity.num_pads; i++)
- crops[i] = v4l2_subdev_get_try_crop(subdev,
- sd_state,
- i);
- }
- if (comps)
- *comps = v4l2_subdev_get_try_compose(subdev, sd_state,
- CCS_PAD_SINK);
- }
+ if (crops)
+ for (i = 0; i < subdev->entity.num_pads; i++)
+ crops[i] =
+ v4l2_subdev_get_pad_crop(subdev, sd_state, i);
+ if (comps)
+ *comps = v4l2_subdev_get_pad_compose(subdev, sd_state,
+ ssd->sink_pad);
}
/* Changes require propagation only on sink pad. */
@@ -2125,7 +2079,7 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
struct ccs_subdev *ssd = to_ccs_subdev(subdev);
struct v4l2_rect *comp, *crops[CCS_PADS];
- ccs_get_crop_compose(subdev, sd_state, crops, &comp, which);
+ ccs_get_crop_compose(subdev, sd_state, crops, &comp);
switch (target) {
case V4L2_SEL_TGT_CROP:
@@ -2136,6 +2090,7 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
sensor->scale_m = CCS_LIM(sensor, SCALER_N_MIN);
sensor->scaling_mode =
CCS_SCALING_MODE_NO_SCALING;
+ sensor->scaler_sink = *comp;
} else if (ssd == sensor->binner) {
sensor->binning_horizontal = 1;
sensor->binning_vertical = 1;
@@ -2144,6 +2099,8 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
fallthrough;
case V4L2_SEL_TGT_COMPOSE:
*crops[CCS_PAD_SRC] = *comp;
+ if (which == V4L2_SUBDEV_FORMAT_ACTIVE && ssd == sensor->src)
+ sensor->src_src = *crops[CCS_PAD_SRC];
break;
default:
WARN_ON_ONCE(1);
@@ -2252,14 +2209,12 @@ static int ccs_set_format(struct v4l2_subdev *subdev,
CCS_LIM(sensor, MIN_Y_OUTPUT_SIZE),
CCS_LIM(sensor, MAX_Y_OUTPUT_SIZE));
- ccs_get_crop_compose(subdev, sd_state, crops, NULL, fmt->which);
+ ccs_get_crop_compose(subdev, sd_state, crops, NULL);
crops[ssd->sink_pad]->left = 0;
crops[ssd->sink_pad]->top = 0;
crops[ssd->sink_pad]->width = fmt->format.width;
crops[ssd->sink_pad]->height = fmt->format.height;
- if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
- ssd->sink_fmt = *crops[ssd->sink_pad];
ccs_propagate(subdev, sd_state, fmt->which, V4L2_SEL_TGT_CROP);
mutex_unlock(&sensor->mutex);
@@ -2482,7 +2437,7 @@ static int ccs_set_compose(struct v4l2_subdev *subdev,
struct ccs_subdev *ssd = to_ccs_subdev(subdev);
struct v4l2_rect *comp, *crops[CCS_PADS];
- ccs_get_crop_compose(subdev, sd_state, crops, &comp, sel->which);
+ ccs_get_crop_compose(subdev, sd_state, crops, &comp);
sel->r.top = 0;
sel->r.left = 0;
@@ -2501,8 +2456,8 @@ static int ccs_set_compose(struct v4l2_subdev *subdev,
return 0;
}
-static int __ccs_sel_supported(struct v4l2_subdev *subdev,
- struct v4l2_subdev_selection *sel)
+static int ccs_sel_supported(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_selection *sel)
{
struct ccs_sensor *sensor = to_ccs_sensor(subdev);
struct ccs_subdev *ssd = to_ccs_subdev(subdev);
@@ -2545,33 +2500,18 @@ static int ccs_set_crop(struct v4l2_subdev *subdev,
{
struct ccs_sensor *sensor = to_ccs_sensor(subdev);
struct ccs_subdev *ssd = to_ccs_subdev(subdev);
- struct v4l2_rect *src_size, *crops[CCS_PADS];
- struct v4l2_rect _r;
+ struct v4l2_rect src_size = { 0 }, *crops[CCS_PADS], *comp;
- ccs_get_crop_compose(subdev, sd_state, crops, NULL, sel->which);
+ ccs_get_crop_compose(subdev, sd_state, crops, &comp);
- if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
- if (sel->pad == ssd->sink_pad)
- src_size = &ssd->sink_fmt;
- else
- src_size = &ssd->compose;
+ if (sel->pad == ssd->sink_pad) {
+ struct v4l2_mbus_framefmt *mfmt =
+ v4l2_subdev_get_pad_format(subdev, sd_state, sel->pad);
+
+ src_size.width = mfmt->width;
+ src_size.height = mfmt->height;
} else {
- if (sel->pad == ssd->sink_pad) {
- _r.left = 0;
- _r.top = 0;
- _r.width = v4l2_subdev_get_try_format(subdev,
- sd_state,
- sel->pad)
- ->width;
- _r.height = v4l2_subdev_get_try_format(subdev,
- sd_state,
- sel->pad)
- ->height;
- src_size = &_r;
- } else {
- src_size = v4l2_subdev_get_try_compose(
- subdev, sd_state, ssd->sink_pad);
- }
+ src_size = *comp;
}
if (ssd == sensor->src && sel->pad == CCS_PAD_SRC) {
@@ -2579,16 +2519,19 @@ static int ccs_set_crop(struct v4l2_subdev *subdev,
sel->r.top = 0;
}
- sel->r.width = min(sel->r.width, src_size->width);
- sel->r.height = min(sel->r.height, src_size->height);
+ sel->r.width = min(sel->r.width, src_size.width);
+ sel->r.height = min(sel->r.height, src_size.height);
- sel->r.left = min_t(int, sel->r.left, src_size->width - sel->r.width);
- sel->r.top = min_t(int, sel->r.top, src_size->height - sel->r.height);
+ sel->r.left = min_t(int, sel->r.left, src_size.width - sel->r.width);
+ sel->r.top = min_t(int, sel->r.top, src_size.height - sel->r.height);
*crops[sel->pad] = sel->r;
if (ssd != sensor->pixel_array && sel->pad == CCS_PAD_SINK)
ccs_propagate(subdev, sd_state, sel->which, V4L2_SEL_TGT_CROP);
+ else if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
+ ssd == sensor->pixel_array)
+ sensor->pa_src = sel->r;
return 0;
}
@@ -2601,44 +2544,36 @@ static void ccs_get_native_size(struct ccs_subdev *ssd, struct v4l2_rect *r)
r->height = CCS_LIM(ssd->sensor, Y_ADDR_MAX) + 1;
}
-static int __ccs_get_selection(struct v4l2_subdev *subdev,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_selection *sel)
+static int ccs_get_selection(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_selection *sel)
{
struct ccs_sensor *sensor = to_ccs_sensor(subdev);
struct ccs_subdev *ssd = to_ccs_subdev(subdev);
struct v4l2_rect *comp, *crops[CCS_PADS];
- struct v4l2_rect sink_fmt;
int ret;
- ret = __ccs_sel_supported(subdev, sel);
+ ret = ccs_sel_supported(subdev, sel);
if (ret)
return ret;
- ccs_get_crop_compose(subdev, sd_state, crops, &comp, sel->which);
-
- if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
- sink_fmt = ssd->sink_fmt;
- } else {
- struct v4l2_mbus_framefmt *fmt =
- v4l2_subdev_get_try_format(subdev, sd_state,
- ssd->sink_pad);
-
- sink_fmt.left = 0;
- sink_fmt.top = 0;
- sink_fmt.width = fmt->width;
- sink_fmt.height = fmt->height;
- }
+ ccs_get_crop_compose(subdev, sd_state, crops, &comp);
switch (sel->target) {
case V4L2_SEL_TGT_CROP_BOUNDS:
case V4L2_SEL_TGT_NATIVE_SIZE:
- if (ssd == sensor->pixel_array)
+ if (ssd == sensor->pixel_array) {
ccs_get_native_size(ssd, &sel->r);
- else if (sel->pad == ssd->sink_pad)
- sel->r = sink_fmt;
- else
+ } else if (sel->pad == ssd->sink_pad) {
+ struct v4l2_mbus_framefmt *sink_fmt =
+ v4l2_subdev_get_pad_format(subdev, sd_state,
+ ssd->sink_pad);
+ sel->r.top = sel->r.left = 0;
+ sel->r.width = sink_fmt->width;
+ sel->r.height = sink_fmt->height;
+ } else {
sel->r = *comp;
+ }
break;
case V4L2_SEL_TGT_CROP:
case V4L2_SEL_TGT_COMPOSE_BOUNDS:
@@ -2652,20 +2587,6 @@ static int __ccs_get_selection(struct v4l2_subdev *subdev,
return 0;
}
-static int ccs_get_selection(struct v4l2_subdev *subdev,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_selection *sel)
-{
- struct ccs_sensor *sensor = to_ccs_sensor(subdev);
- int rval;
-
- mutex_lock(&sensor->mutex);
- rval = __ccs_get_selection(subdev, sd_state, sel);
- mutex_unlock(&sensor->mutex);
-
- return rval;
-}
-
static int ccs_set_selection(struct v4l2_subdev *subdev,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_selection *sel)
@@ -2673,7 +2594,7 @@ static int ccs_set_selection(struct v4l2_subdev *subdev,
struct ccs_sensor *sensor = to_ccs_sensor(subdev);
int ret;
- ret = __ccs_sel_supported(subdev, sel);
+ ret = ccs_sel_supported(subdev, sel);
if (ret)
return ret;
@@ -2964,10 +2885,14 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
return rval;
}
+ rval = v4l2_subdev_init_finalize(&ssd->sd);
+ if (rval)
+ goto out_media_entity_cleanup;
+
rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev, &ssd->sd);
if (rval) {
dev_err(&client->dev, "v4l2_device_register_subdev failed\n");
- goto out_media_entity_cleanup;
+ goto out_v4l2_subdev_cleanup;
}
rval = media_create_pad_link(&ssd->sd.entity, source_pad,
@@ -2983,6 +2908,9 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
out_v4l2_device_unregister_subdev:
v4l2_device_unregister_subdev(&ssd->sd);
+out_v4l2_subdev_cleanup:
+ v4l2_subdev_cleanup(&ssd->sd);
+
out_media_entity_cleanup:
media_entity_cleanup(&ssd->sd.entity);
@@ -3059,16 +2987,9 @@ static void ccs_create_subdev(struct ccs_sensor *sensor,
v4l2_i2c_subdev_set_name(&ssd->sd, client, sensor->minfo.name, name);
- ccs_get_native_size(ssd, &ssd->sink_fmt);
-
- ssd->compose.width = ssd->sink_fmt.width;
- ssd->compose.height = ssd->sink_fmt.height;
- ssd->crop[ssd->source_pad] = ssd->compose;
ssd->pads[ssd->source_pad].flags = MEDIA_PAD_FL_SOURCE;
- if (ssd != sensor->pixel_array) {
- ssd->crop[ssd->sink_pad] = ssd->compose;
+ if (ssd != sensor->pixel_array)
ssd->pads[ssd->sink_pad].flags = MEDIA_PAD_FL_SINK;
- }
ssd->sd.entity.ops = &ccs_entity_ops;
@@ -3089,24 +3010,24 @@ static int ccs_init_cfg(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_sta
mutex_lock(&sensor->mutex);
for (i = 0; i < ssd->npads; i++) {
- struct v4l2_mbus_framefmt *try_fmt =
- v4l2_subdev_get_try_format(sd, sd_state, i);
- struct v4l2_rect *try_crop =
- v4l2_subdev_get_try_crop(sd, sd_state, i);
- struct v4l2_rect *try_comp;
+ struct v4l2_mbus_framefmt *pad_fmt =
+ v4l2_subdev_get_pad_format(sd, sd_state, i);
+ struct v4l2_rect *pad_crop =
+ v4l2_subdev_get_pad_crop(sd, sd_state, i);
+ struct v4l2_rect *pad_comp;
- ccs_get_native_size(ssd, try_crop);
+ ccs_get_native_size(ssd, pad_crop);
- try_fmt->width = try_crop->width;
- try_fmt->height = try_crop->height;
- try_fmt->code = sensor->internal_csi_format->code;
- try_fmt->field = V4L2_FIELD_NONE;
+ pad_fmt->width = pad_crop->width;
+ pad_fmt->height = pad_crop->height;
+ pad_fmt->code = sensor->internal_csi_format->code;
+ pad_fmt->field = V4L2_FIELD_NONE;
if (ssd == sensor->pixel_array)
continue;
- try_comp = v4l2_subdev_get_try_compose(sd, sd_state, i);
- *try_comp = *try_crop;
+ pad_comp = v4l2_subdev_get_pad_compose(sd, sd_state, i);
+ *pad_comp = *pad_crop;
}
mutex_unlock(&sensor->mutex);
@@ -3631,6 +3552,10 @@ static int ccs_probe(struct i2c_client *client)
if (rval < 0)
goto out_media_entity_cleanup;
+ rval = v4l2_subdev_init_finalize(&sensor->src->sd);
+ if (rval)
+ goto out_media_entity_cleanup;
+
rval = ccs_write_msr_regs(sensor);
if (rval)
goto out_media_entity_cleanup;
@@ -3690,6 +3615,7 @@ static void ccs_remove(struct i2c_client *client)
for (i = 0; i < sensor->ssds_used; i++) {
v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
+ v4l2_subdev_cleanup(subdev);
media_entity_cleanup(&sensor->ssds[i].sd.entity);
}
ccs_cleanup(sensor);
diff --git a/drivers/media/i2c/ccs/ccs.h b/drivers/media/i2c/ccs/ccs.h
index a94c796cea48..9c3587b2fbe7 100644
--- a/drivers/media/i2c/ccs/ccs.h
+++ b/drivers/media/i2c/ccs/ccs.h
@@ -182,9 +182,6 @@ struct ccs_binning_subtype {
struct ccs_subdev {
struct v4l2_subdev sd;
struct media_pad pads[CCS_PADS];
- struct v4l2_rect sink_fmt;
- struct v4l2_rect crop[CCS_PADS];
- struct v4l2_rect compose; /* compose on sink */
unsigned short sink_pad;
unsigned short source_pad;
int npads;
@@ -220,6 +217,7 @@ struct ccs_sensor {
u32 mbus_frame_fmts;
const struct ccs_csi_data_format *csi_format;
const struct ccs_csi_data_format *internal_csi_format;
+ struct v4l2_rect pa_src, scaler_sink, src_src;
u32 default_mbus_frame_fmts;
int default_pixel_order;
struct ccs_data_container sdata, mdata;
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 06/12] media: ccs: Use sub-device active state
2023-09-18 12:51 ` [PATCH v2 06/12] media: ccs: Use sub-device active state Sakari Ailus
@ 2023-09-18 13:59 ` Laurent Pinchart
2023-09-19 10:29 ` Sakari Ailus
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2023-09-18 13:59 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Sakari,
Thank you for the patch.
On Mon, Sep 18, 2023 at 03:51:32PM +0300, Sakari Ailus wrote:
> Make use of sub-device active state. In most cases the effect on need for
> acquiring the mutex is non-existent as access to the driver's core data
> structure still needs to be serialised.
>
> This still removes a lot of code as the code paths for active and try
> state are the same in many cases.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/i2c/ccs/ccs-core.c | 278 ++++++++++++-------------------
> drivers/media/i2c/ccs/ccs.h | 4 +-
> 2 files changed, 103 insertions(+), 179 deletions(-)
>
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index db461b0e49c8..efed75b6534c 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -508,9 +508,8 @@ static void __ccs_update_exposure_limits(struct ccs_sensor *sensor)
> struct v4l2_ctrl *ctrl = sensor->exposure;
> int max;
>
> - max = sensor->pixel_array->crop[CCS_PA_PAD_SRC].height
> - + sensor->vblank->val
> - - CCS_LIM(sensor, COARSE_INTEGRATION_TIME_MAX_MARGIN);
> + max = sensor->pa_src.height + sensor->vblank->val -
> + CCS_LIM(sensor, COARSE_INTEGRATION_TIME_MAX_MARGIN);
>
> __v4l2_ctrl_modify_range(ctrl, ctrl->minimum, max, ctrl->step, max);
> }
> @@ -728,15 +727,12 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl)
> break;
> case V4L2_CID_VBLANK:
> rval = ccs_write(sensor, FRAME_LENGTH_LINES,
> - sensor->pixel_array->crop[
> - CCS_PA_PAD_SRC].height
> - + ctrl->val);
> + sensor->pa_src.height + ctrl->val);
>
> break;
> case V4L2_CID_HBLANK:
> rval = ccs_write(sensor, LINE_LENGTH_PCK,
> - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width
> - + ctrl->val);
> + sensor->pa_src.width + ctrl->val);
>
> break;
> case V4L2_CID_TEST_PATTERN:
> @@ -1214,15 +1210,13 @@ static void ccs_update_blanking(struct ccs_sensor *sensor)
>
> min = max_t(int,
> CCS_LIM(sensor, MIN_FRAME_BLANKING_LINES),
> - min_fll - sensor->pixel_array->crop[CCS_PA_PAD_SRC].height);
> - max = max_fll - sensor->pixel_array->crop[CCS_PA_PAD_SRC].height;
> + min_fll - sensor->pa_src.height);
> + max = max_fll - sensor->pa_src.height;
>
> __v4l2_ctrl_modify_range(vblank, min, max, vblank->step, min);
>
> - min = max_t(int,
> - min_llp - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width,
> - min_lbp);
> - max = max_llp - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width;
> + min = max_t(int, min_llp - sensor->pa_src.width, min_lbp);
> + max = max_llp - sensor->pa_src.width;
>
> __v4l2_ctrl_modify_range(hblank, min, max, hblank->step, min);
>
> @@ -1246,10 +1240,8 @@ static int ccs_pll_blanking_update(struct ccs_sensor *sensor)
>
> dev_dbg(&client->dev, "real timeperframe\t100/%d\n",
> sensor->pll.pixel_rate_pixel_array /
> - ((sensor->pixel_array->crop[CCS_PA_PAD_SRC].width
> - + sensor->hblank->val) *
> - (sensor->pixel_array->crop[CCS_PA_PAD_SRC].height
> - + sensor->vblank->val) / 100));
> + ((sensor->pa_src.width + sensor->hblank->val) *
> + (sensor->pa_src.height + sensor->vblank->val) / 100));
>
> return 0;
> }
> @@ -1756,28 +1748,22 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
> goto out;
>
> /* Analog crop start coordinates */
> - rval = ccs_write(sensor, X_ADDR_START,
> - sensor->pixel_array->crop[CCS_PA_PAD_SRC].left);
> + rval = ccs_write(sensor, X_ADDR_START, sensor->pa_src.left);
> if (rval < 0)
> goto out;
>
> - rval = ccs_write(sensor, Y_ADDR_START,
> - sensor->pixel_array->crop[CCS_PA_PAD_SRC].top);
> + rval = ccs_write(sensor, Y_ADDR_START, sensor->pa_src.top);
> if (rval < 0)
> goto out;
>
> /* Analog crop end coordinates */
> - rval = ccs_write(
> - sensor, X_ADDR_END,
> - sensor->pixel_array->crop[CCS_PA_PAD_SRC].left
> - + sensor->pixel_array->crop[CCS_PA_PAD_SRC].width - 1);
> + rval = ccs_write(sensor, X_ADDR_END,
> + sensor->pa_src.left + sensor->pa_src.width - 1);
> if (rval < 0)
> goto out;
>
> - rval = ccs_write(
> - sensor, Y_ADDR_END,
> - sensor->pixel_array->crop[CCS_PA_PAD_SRC].top
> - + sensor->pixel_array->crop[CCS_PA_PAD_SRC].height - 1);
> + rval = ccs_write(sensor, Y_ADDR_END,
> + sensor->pa_src.top + sensor->pa_src.height - 1);
> if (rval < 0)
> goto out;
>
> @@ -1789,27 +1775,23 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
> /* Digital crop */
> if (CCS_LIM(sensor, DIGITAL_CROP_CAPABILITY)
> == CCS_DIGITAL_CROP_CAPABILITY_INPUT_CROP) {
> - rval = ccs_write(
> - sensor, DIGITAL_CROP_X_OFFSET,
> - sensor->scaler->crop[CCS_PAD_SINK].left);
> + rval = ccs_write(sensor, DIGITAL_CROP_X_OFFSET,
> + sensor->scaler_sink.left);
> if (rval < 0)
> goto out;
>
> - rval = ccs_write(
> - sensor, DIGITAL_CROP_Y_OFFSET,
> - sensor->scaler->crop[CCS_PAD_SINK].top);
> + rval = ccs_write(sensor, DIGITAL_CROP_Y_OFFSET,
> + sensor->scaler_sink.top);
> if (rval < 0)
> goto out;
>
> - rval = ccs_write(
> - sensor, DIGITAL_CROP_IMAGE_WIDTH,
> - sensor->scaler->crop[CCS_PAD_SINK].width);
> + rval = ccs_write(sensor, DIGITAL_CROP_IMAGE_WIDTH,
> + sensor->scaler_sink.width);
> if (rval < 0)
> goto out;
>
> - rval = ccs_write(
> - sensor, DIGITAL_CROP_IMAGE_HEIGHT,
> - sensor->scaler->crop[CCS_PAD_SINK].height);
> + rval = ccs_write(sensor, DIGITAL_CROP_IMAGE_HEIGHT,
> + sensor->scaler_sink.height);
> if (rval < 0)
> goto out;
> }
> @@ -1827,12 +1809,10 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
> }
>
> /* Output size from sensor */
> - rval = ccs_write(sensor, X_OUTPUT_SIZE,
> - sensor->src->crop[CCS_PAD_SRC].width);
> + rval = ccs_write(sensor, X_OUTPUT_SIZE, sensor->src_src.width);
> if (rval < 0)
> goto out;
> - rval = ccs_write(sensor, Y_OUTPUT_SIZE,
> - sensor->src->crop[CCS_PAD_SRC].height);
> + rval = ccs_write(sensor, Y_OUTPUT_SIZE, sensor->src_src.height);
> if (rval < 0)
> goto out;
>
> @@ -2053,24 +2033,8 @@ static int __ccs_get_format(struct v4l2_subdev *subdev,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_format *fmt)
> {
> - struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> -
> - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> - fmt->format = *v4l2_subdev_get_try_format(subdev, sd_state,
> - fmt->pad);
> - } else {
> - struct v4l2_rect *r;
> -
> - if (fmt->pad == ssd->source_pad)
> - r = &ssd->crop[ssd->source_pad];
> - else
> - r = &ssd->sink_fmt;
> -
> - fmt->format.code = __ccs_get_mbus_code(subdev, fmt->pad);
> - fmt->format.width = r->width;
> - fmt->format.height = r->height;
> - fmt->format.field = V4L2_FIELD_NONE;
> - }
> + fmt->format = *v4l2_subdev_get_pad_format(subdev, sd_state, fmt->pad);
> + fmt->format.code = __ccs_get_mbus_code(subdev, fmt->pad);
>
> return 0;
> }
> @@ -2092,28 +2056,18 @@ static int ccs_get_format(struct v4l2_subdev *subdev,
Please replace ccs_get_format() with v4l2_subdev_get_fmt(). It's a
drop-in replacement for the .get_fmt() operation, you can drop this
function. The only remaining caller of __ccs_get_format() can then use
v4l2_subdev_get_fmt() too.
> static void ccs_get_crop_compose(struct v4l2_subdev *subdev,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_rect **crops,
> - struct v4l2_rect **comps, int which)
> + struct v4l2_rect **comps)
> {
> struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> unsigned int i;
>
> - if (which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> - if (crops)
> - for (i = 0; i < subdev->entity.num_pads; i++)
> - crops[i] = &ssd->crop[i];
> - if (comps)
> - *comps = &ssd->compose;
> - } else {
> - if (crops) {
> - for (i = 0; i < subdev->entity.num_pads; i++)
> - crops[i] = v4l2_subdev_get_try_crop(subdev,
> - sd_state,
> - i);
> - }
> - if (comps)
> - *comps = v4l2_subdev_get_try_compose(subdev, sd_state,
> - CCS_PAD_SINK);
> - }
> + if (crops)
> + for (i = 0; i < subdev->entity.num_pads; i++)
> + crops[i] =
> + v4l2_subdev_get_pad_crop(subdev, sd_state, i);
> + if (comps)
> + *comps = v4l2_subdev_get_pad_compose(subdev, sd_state,
> + ssd->sink_pad);
> }
>
> /* Changes require propagation only on sink pad. */
> @@ -2125,7 +2079,7 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
> struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> struct v4l2_rect *comp, *crops[CCS_PADS];
>
> - ccs_get_crop_compose(subdev, sd_state, crops, &comp, which);
> + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
>
> switch (target) {
> case V4L2_SEL_TGT_CROP:
> @@ -2136,6 +2090,7 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
> sensor->scale_m = CCS_LIM(sensor, SCALER_N_MIN);
> sensor->scaling_mode =
> CCS_SCALING_MODE_NO_SCALING;
> + sensor->scaler_sink = *comp;
> } else if (ssd == sensor->binner) {
> sensor->binning_horizontal = 1;
> sensor->binning_vertical = 1;
> @@ -2144,6 +2099,8 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
> fallthrough;
> case V4L2_SEL_TGT_COMPOSE:
> *crops[CCS_PAD_SRC] = *comp;
> + if (which == V4L2_SUBDEV_FORMAT_ACTIVE && ssd == sensor->src)
> + sensor->src_src = *crops[CCS_PAD_SRC];
> break;
> default:
> WARN_ON_ONCE(1);
> @@ -2252,14 +2209,12 @@ static int ccs_set_format(struct v4l2_subdev *subdev,
> CCS_LIM(sensor, MIN_Y_OUTPUT_SIZE),
> CCS_LIM(sensor, MAX_Y_OUTPUT_SIZE));
>
> - ccs_get_crop_compose(subdev, sd_state, crops, NULL, fmt->which);
> + ccs_get_crop_compose(subdev, sd_state, crops, NULL);
>
> crops[ssd->sink_pad]->left = 0;
> crops[ssd->sink_pad]->top = 0;
> crops[ssd->sink_pad]->width = fmt->format.width;
> crops[ssd->sink_pad]->height = fmt->format.height;
> - if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> - ssd->sink_fmt = *crops[ssd->sink_pad];
> ccs_propagate(subdev, sd_state, fmt->which, V4L2_SEL_TGT_CROP);
>
> mutex_unlock(&sensor->mutex);
> @@ -2482,7 +2437,7 @@ static int ccs_set_compose(struct v4l2_subdev *subdev,
> struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> struct v4l2_rect *comp, *crops[CCS_PADS];
>
> - ccs_get_crop_compose(subdev, sd_state, crops, &comp, sel->which);
> + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
>
> sel->r.top = 0;
> sel->r.left = 0;
> @@ -2501,8 +2456,8 @@ static int ccs_set_compose(struct v4l2_subdev *subdev,
> return 0;
> }
>
> -static int __ccs_sel_supported(struct v4l2_subdev *subdev,
> - struct v4l2_subdev_selection *sel)
> +static int ccs_sel_supported(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_selection *sel)
> {
> struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> @@ -2545,33 +2500,18 @@ static int ccs_set_crop(struct v4l2_subdev *subdev,
> {
> struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> - struct v4l2_rect *src_size, *crops[CCS_PADS];
> - struct v4l2_rect _r;
> + struct v4l2_rect src_size = { 0 }, *crops[CCS_PADS], *comp;
>
> - ccs_get_crop_compose(subdev, sd_state, crops, NULL, sel->which);
> + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
>
> - if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> - if (sel->pad == ssd->sink_pad)
> - src_size = &ssd->sink_fmt;
> - else
> - src_size = &ssd->compose;
> + if (sel->pad == ssd->sink_pad) {
> + struct v4l2_mbus_framefmt *mfmt =
> + v4l2_subdev_get_pad_format(subdev, sd_state, sel->pad);
> +
> + src_size.width = mfmt->width;
> + src_size.height = mfmt->height;
> } else {
> - if (sel->pad == ssd->sink_pad) {
> - _r.left = 0;
> - _r.top = 0;
> - _r.width = v4l2_subdev_get_try_format(subdev,
> - sd_state,
> - sel->pad)
> - ->width;
> - _r.height = v4l2_subdev_get_try_format(subdev,
> - sd_state,
> - sel->pad)
> - ->height;
> - src_size = &_r;
> - } else {
> - src_size = v4l2_subdev_get_try_compose(
> - subdev, sd_state, ssd->sink_pad);
> - }
> + src_size = *comp;
> }
>
> if (ssd == sensor->src && sel->pad == CCS_PAD_SRC) {
> @@ -2579,16 +2519,19 @@ static int ccs_set_crop(struct v4l2_subdev *subdev,
> sel->r.top = 0;
> }
>
> - sel->r.width = min(sel->r.width, src_size->width);
> - sel->r.height = min(sel->r.height, src_size->height);
> + sel->r.width = min(sel->r.width, src_size.width);
> + sel->r.height = min(sel->r.height, src_size.height);
>
> - sel->r.left = min_t(int, sel->r.left, src_size->width - sel->r.width);
> - sel->r.top = min_t(int, sel->r.top, src_size->height - sel->r.height);
> + sel->r.left = min_t(int, sel->r.left, src_size.width - sel->r.width);
> + sel->r.top = min_t(int, sel->r.top, src_size.height - sel->r.height);
>
> *crops[sel->pad] = sel->r;
>
> if (ssd != sensor->pixel_array && sel->pad == CCS_PAD_SINK)
> ccs_propagate(subdev, sd_state, sel->which, V4L2_SEL_TGT_CROP);
> + else if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
> + ssd == sensor->pixel_array)
> + sensor->pa_src = sel->r;
>
> return 0;
> }
> @@ -2601,44 +2544,36 @@ static void ccs_get_native_size(struct ccs_subdev *ssd, struct v4l2_rect *r)
> r->height = CCS_LIM(ssd->sensor, Y_ADDR_MAX) + 1;
> }
>
> -static int __ccs_get_selection(struct v4l2_subdev *subdev,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_selection *sel)
> +static int ccs_get_selection(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_selection *sel)
> {
> struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> struct v4l2_rect *comp, *crops[CCS_PADS];
> - struct v4l2_rect sink_fmt;
> int ret;
>
> - ret = __ccs_sel_supported(subdev, sel);
> + ret = ccs_sel_supported(subdev, sel);
> if (ret)
> return ret;
>
> - ccs_get_crop_compose(subdev, sd_state, crops, &comp, sel->which);
> -
> - if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> - sink_fmt = ssd->sink_fmt;
> - } else {
> - struct v4l2_mbus_framefmt *fmt =
> - v4l2_subdev_get_try_format(subdev, sd_state,
> - ssd->sink_pad);
> -
> - sink_fmt.left = 0;
> - sink_fmt.top = 0;
> - sink_fmt.width = fmt->width;
> - sink_fmt.height = fmt->height;
> - }
> + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
>
> switch (sel->target) {
> case V4L2_SEL_TGT_CROP_BOUNDS:
> case V4L2_SEL_TGT_NATIVE_SIZE:
> - if (ssd == sensor->pixel_array)
> + if (ssd == sensor->pixel_array) {
> ccs_get_native_size(ssd, &sel->r);
> - else if (sel->pad == ssd->sink_pad)
> - sel->r = sink_fmt;
> - else
> + } else if (sel->pad == ssd->sink_pad) {
> + struct v4l2_mbus_framefmt *sink_fmt =
> + v4l2_subdev_get_pad_format(subdev, sd_state,
> + ssd->sink_pad);
> + sel->r.top = sel->r.left = 0;
> + sel->r.width = sink_fmt->width;
> + sel->r.height = sink_fmt->height;
> + } else {
> sel->r = *comp;
> + }
> break;
> case V4L2_SEL_TGT_CROP:
> case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> @@ -2652,20 +2587,6 @@ static int __ccs_get_selection(struct v4l2_subdev *subdev,
> return 0;
> }
>
> -static int ccs_get_selection(struct v4l2_subdev *subdev,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_selection *sel)
> -{
> - struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> - int rval;
> -
> - mutex_lock(&sensor->mutex);
> - rval = __ccs_get_selection(subdev, sd_state, sel);
> - mutex_unlock(&sensor->mutex);
> -
> - return rval;
> -}
> -
> static int ccs_set_selection(struct v4l2_subdev *subdev,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_selection *sel)
> @@ -2673,7 +2594,7 @@ static int ccs_set_selection(struct v4l2_subdev *subdev,
> struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> int ret;
>
> - ret = __ccs_sel_supported(subdev, sel);
> + ret = ccs_sel_supported(subdev, sel);
> if (ret)
> return ret;
>
> @@ -2964,10 +2885,14 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
> return rval;
> }
>
> + rval = v4l2_subdev_init_finalize(&ssd->sd);
> + if (rval)
> + goto out_media_entity_cleanup;
> +
> rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev, &ssd->sd);
> if (rval) {
> dev_err(&client->dev, "v4l2_device_register_subdev failed\n");
> - goto out_media_entity_cleanup;
> + goto out_v4l2_subdev_cleanup;
> }
>
> rval = media_create_pad_link(&ssd->sd.entity, source_pad,
> @@ -2983,6 +2908,9 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
> out_v4l2_device_unregister_subdev:
> v4l2_device_unregister_subdev(&ssd->sd);
>
> +out_v4l2_subdev_cleanup:
> + v4l2_subdev_cleanup(&ssd->sd);
> +
> out_media_entity_cleanup:
> media_entity_cleanup(&ssd->sd.entity);
>
> @@ -3059,16 +2987,9 @@ static void ccs_create_subdev(struct ccs_sensor *sensor,
>
> v4l2_i2c_subdev_set_name(&ssd->sd, client, sensor->minfo.name, name);
>
> - ccs_get_native_size(ssd, &ssd->sink_fmt);
> -
> - ssd->compose.width = ssd->sink_fmt.width;
> - ssd->compose.height = ssd->sink_fmt.height;
> - ssd->crop[ssd->source_pad] = ssd->compose;
> ssd->pads[ssd->source_pad].flags = MEDIA_PAD_FL_SOURCE;
> - if (ssd != sensor->pixel_array) {
> - ssd->crop[ssd->sink_pad] = ssd->compose;
> + if (ssd != sensor->pixel_array)
> ssd->pads[ssd->sink_pad].flags = MEDIA_PAD_FL_SINK;
> - }
>
> ssd->sd.entity.ops = &ccs_entity_ops;
>
> @@ -3089,24 +3010,24 @@ static int ccs_init_cfg(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_sta
> mutex_lock(&sensor->mutex);
>
> for (i = 0; i < ssd->npads; i++) {
> - struct v4l2_mbus_framefmt *try_fmt =
> - v4l2_subdev_get_try_format(sd, sd_state, i);
> - struct v4l2_rect *try_crop =
> - v4l2_subdev_get_try_crop(sd, sd_state, i);
> - struct v4l2_rect *try_comp;
> + struct v4l2_mbus_framefmt *pad_fmt =
> + v4l2_subdev_get_pad_format(sd, sd_state, i);
> + struct v4l2_rect *pad_crop =
> + v4l2_subdev_get_pad_crop(sd, sd_state, i);
> + struct v4l2_rect *pad_comp;
These can simply be called fmt, crop and comp.
>
> - ccs_get_native_size(ssd, try_crop);
> + ccs_get_native_size(ssd, pad_crop);
>
> - try_fmt->width = try_crop->width;
> - try_fmt->height = try_crop->height;
> - try_fmt->code = sensor->internal_csi_format->code;
> - try_fmt->field = V4L2_FIELD_NONE;
> + pad_fmt->width = pad_crop->width;
> + pad_fmt->height = pad_crop->height;
> + pad_fmt->code = sensor->internal_csi_format->code;
> + pad_fmt->field = V4L2_FIELD_NONE;
>
> if (ssd == sensor->pixel_array)
> continue;
>
> - try_comp = v4l2_subdev_get_try_compose(sd, sd_state, i);
> - *try_comp = *try_crop;
> + pad_comp = v4l2_subdev_get_pad_compose(sd, sd_state, i);
> + *pad_comp = *pad_crop;
> }
>
> mutex_unlock(&sensor->mutex);
> @@ -3631,6 +3552,10 @@ static int ccs_probe(struct i2c_client *client)
> if (rval < 0)
> goto out_media_entity_cleanup;
>
> + rval = v4l2_subdev_init_finalize(&sensor->src->sd);
> + if (rval)
> + goto out_media_entity_cleanup;
> +
> rval = ccs_write_msr_regs(sensor);
> if (rval)
> goto out_media_entity_cleanup;
> @@ -3690,6 +3615,7 @@ static void ccs_remove(struct i2c_client *client)
>
> for (i = 0; i < sensor->ssds_used; i++) {
> v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
> + v4l2_subdev_cleanup(subdev);
> media_entity_cleanup(&sensor->ssds[i].sd.entity);
> }
> ccs_cleanup(sensor);
> diff --git a/drivers/media/i2c/ccs/ccs.h b/drivers/media/i2c/ccs/ccs.h
> index a94c796cea48..9c3587b2fbe7 100644
> --- a/drivers/media/i2c/ccs/ccs.h
> +++ b/drivers/media/i2c/ccs/ccs.h
> @@ -182,9 +182,6 @@ struct ccs_binning_subtype {
> struct ccs_subdev {
> struct v4l2_subdev sd;
> struct media_pad pads[CCS_PADS];
> - struct v4l2_rect sink_fmt;
> - struct v4l2_rect crop[CCS_PADS];
> - struct v4l2_rect compose; /* compose on sink */
> unsigned short sink_pad;
> unsigned short source_pad;
> int npads;
> @@ -220,6 +217,7 @@ struct ccs_sensor {
> u32 mbus_frame_fmts;
> const struct ccs_csi_data_format *csi_format;
> const struct ccs_csi_data_format *internal_csi_format;
> + struct v4l2_rect pa_src, scaler_sink, src_src;
The idea of the active state API is to remove all active state from the
driver private structure. Why do you need these, can't you get them from
the active state where appropriate ?
> u32 default_mbus_frame_fmts;
> int default_pixel_order;
> struct ccs_data_container sdata, mdata;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 06/12] media: ccs: Use sub-device active state
2023-09-18 13:59 ` Laurent Pinchart
@ 2023-09-19 10:29 ` Sakari Ailus
2023-09-19 13:42 ` Laurent Pinchart
0 siblings, 1 reply; 28+ messages in thread
From: Sakari Ailus @ 2023-09-19 10:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Laurent,
On Mon, Sep 18, 2023 at 04:59:33PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
Thanks for the review!
>
> On Mon, Sep 18, 2023 at 03:51:32PM +0300, Sakari Ailus wrote:
> > Make use of sub-device active state. In most cases the effect on need for
> > acquiring the mutex is non-existent as access to the driver's core data
> > structure still needs to be serialised.
> >
> > This still removes a lot of code as the code paths for active and try
> > state are the same in many cases.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/i2c/ccs/ccs-core.c | 278 ++++++++++++-------------------
> > drivers/media/i2c/ccs/ccs.h | 4 +-
> > 2 files changed, 103 insertions(+), 179 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > index db461b0e49c8..efed75b6534c 100644
> > --- a/drivers/media/i2c/ccs/ccs-core.c
> > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > @@ -508,9 +508,8 @@ static void __ccs_update_exposure_limits(struct ccs_sensor *sensor)
> > struct v4l2_ctrl *ctrl = sensor->exposure;
> > int max;
> >
> > - max = sensor->pixel_array->crop[CCS_PA_PAD_SRC].height
> > - + sensor->vblank->val
> > - - CCS_LIM(sensor, COARSE_INTEGRATION_TIME_MAX_MARGIN);
> > + max = sensor->pa_src.height + sensor->vblank->val -
> > + CCS_LIM(sensor, COARSE_INTEGRATION_TIME_MAX_MARGIN);
> >
> > __v4l2_ctrl_modify_range(ctrl, ctrl->minimum, max, ctrl->step, max);
> > }
> > @@ -728,15 +727,12 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl)
> > break;
> > case V4L2_CID_VBLANK:
> > rval = ccs_write(sensor, FRAME_LENGTH_LINES,
> > - sensor->pixel_array->crop[
> > - CCS_PA_PAD_SRC].height
> > - + ctrl->val);
> > + sensor->pa_src.height + ctrl->val);
> >
> > break;
> > case V4L2_CID_HBLANK:
> > rval = ccs_write(sensor, LINE_LENGTH_PCK,
> > - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width
> > - + ctrl->val);
> > + sensor->pa_src.width + ctrl->val);
> >
> > break;
> > case V4L2_CID_TEST_PATTERN:
> > @@ -1214,15 +1210,13 @@ static void ccs_update_blanking(struct ccs_sensor *sensor)
> >
> > min = max_t(int,
> > CCS_LIM(sensor, MIN_FRAME_BLANKING_LINES),
> > - min_fll - sensor->pixel_array->crop[CCS_PA_PAD_SRC].height);
> > - max = max_fll - sensor->pixel_array->crop[CCS_PA_PAD_SRC].height;
> > + min_fll - sensor->pa_src.height);
> > + max = max_fll - sensor->pa_src.height;
> >
> > __v4l2_ctrl_modify_range(vblank, min, max, vblank->step, min);
> >
> > - min = max_t(int,
> > - min_llp - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width,
> > - min_lbp);
> > - max = max_llp - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width;
> > + min = max_t(int, min_llp - sensor->pa_src.width, min_lbp);
> > + max = max_llp - sensor->pa_src.width;
> >
> > __v4l2_ctrl_modify_range(hblank, min, max, hblank->step, min);
> >
> > @@ -1246,10 +1240,8 @@ static int ccs_pll_blanking_update(struct ccs_sensor *sensor)
> >
> > dev_dbg(&client->dev, "real timeperframe\t100/%d\n",
> > sensor->pll.pixel_rate_pixel_array /
> > - ((sensor->pixel_array->crop[CCS_PA_PAD_SRC].width
> > - + sensor->hblank->val) *
> > - (sensor->pixel_array->crop[CCS_PA_PAD_SRC].height
> > - + sensor->vblank->val) / 100));
> > + ((sensor->pa_src.width + sensor->hblank->val) *
> > + (sensor->pa_src.height + sensor->vblank->val) / 100));
> >
> > return 0;
> > }
> > @@ -1756,28 +1748,22 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
> > goto out;
> >
> > /* Analog crop start coordinates */
> > - rval = ccs_write(sensor, X_ADDR_START,
> > - sensor->pixel_array->crop[CCS_PA_PAD_SRC].left);
> > + rval = ccs_write(sensor, X_ADDR_START, sensor->pa_src.left);
> > if (rval < 0)
> > goto out;
> >
> > - rval = ccs_write(sensor, Y_ADDR_START,
> > - sensor->pixel_array->crop[CCS_PA_PAD_SRC].top);
> > + rval = ccs_write(sensor, Y_ADDR_START, sensor->pa_src.top);
> > if (rval < 0)
> > goto out;
> >
> > /* Analog crop end coordinates */
> > - rval = ccs_write(
> > - sensor, X_ADDR_END,
> > - sensor->pixel_array->crop[CCS_PA_PAD_SRC].left
> > - + sensor->pixel_array->crop[CCS_PA_PAD_SRC].width - 1);
> > + rval = ccs_write(sensor, X_ADDR_END,
> > + sensor->pa_src.left + sensor->pa_src.width - 1);
> > if (rval < 0)
> > goto out;
> >
> > - rval = ccs_write(
> > - sensor, Y_ADDR_END,
> > - sensor->pixel_array->crop[CCS_PA_PAD_SRC].top
> > - + sensor->pixel_array->crop[CCS_PA_PAD_SRC].height - 1);
> > + rval = ccs_write(sensor, Y_ADDR_END,
> > + sensor->pa_src.top + sensor->pa_src.height - 1);
> > if (rval < 0)
> > goto out;
> >
> > @@ -1789,27 +1775,23 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
> > /* Digital crop */
> > if (CCS_LIM(sensor, DIGITAL_CROP_CAPABILITY)
> > == CCS_DIGITAL_CROP_CAPABILITY_INPUT_CROP) {
> > - rval = ccs_write(
> > - sensor, DIGITAL_CROP_X_OFFSET,
> > - sensor->scaler->crop[CCS_PAD_SINK].left);
> > + rval = ccs_write(sensor, DIGITAL_CROP_X_OFFSET,
> > + sensor->scaler_sink.left);
> > if (rval < 0)
> > goto out;
> >
> > - rval = ccs_write(
> > - sensor, DIGITAL_CROP_Y_OFFSET,
> > - sensor->scaler->crop[CCS_PAD_SINK].top);
> > + rval = ccs_write(sensor, DIGITAL_CROP_Y_OFFSET,
> > + sensor->scaler_sink.top);
> > if (rval < 0)
> > goto out;
> >
> > - rval = ccs_write(
> > - sensor, DIGITAL_CROP_IMAGE_WIDTH,
> > - sensor->scaler->crop[CCS_PAD_SINK].width);
> > + rval = ccs_write(sensor, DIGITAL_CROP_IMAGE_WIDTH,
> > + sensor->scaler_sink.width);
> > if (rval < 0)
> > goto out;
> >
> > - rval = ccs_write(
> > - sensor, DIGITAL_CROP_IMAGE_HEIGHT,
> > - sensor->scaler->crop[CCS_PAD_SINK].height);
> > + rval = ccs_write(sensor, DIGITAL_CROP_IMAGE_HEIGHT,
> > + sensor->scaler_sink.height);
> > if (rval < 0)
> > goto out;
> > }
> > @@ -1827,12 +1809,10 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
> > }
> >
> > /* Output size from sensor */
> > - rval = ccs_write(sensor, X_OUTPUT_SIZE,
> > - sensor->src->crop[CCS_PAD_SRC].width);
> > + rval = ccs_write(sensor, X_OUTPUT_SIZE, sensor->src_src.width);
> > if (rval < 0)
> > goto out;
> > - rval = ccs_write(sensor, Y_OUTPUT_SIZE,
> > - sensor->src->crop[CCS_PAD_SRC].height);
> > + rval = ccs_write(sensor, Y_OUTPUT_SIZE, sensor->src_src.height);
> > if (rval < 0)
> > goto out;
> >
> > @@ -2053,24 +2033,8 @@ static int __ccs_get_format(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_format *fmt)
> > {
> > - struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > -
> > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > - fmt->format = *v4l2_subdev_get_try_format(subdev, sd_state,
> > - fmt->pad);
> > - } else {
> > - struct v4l2_rect *r;
> > -
> > - if (fmt->pad == ssd->source_pad)
> > - r = &ssd->crop[ssd->source_pad];
> > - else
> > - r = &ssd->sink_fmt;
> > -
> > - fmt->format.code = __ccs_get_mbus_code(subdev, fmt->pad);
> > - fmt->format.width = r->width;
> > - fmt->format.height = r->height;
> > - fmt->format.field = V4L2_FIELD_NONE;
> > - }
> > + fmt->format = *v4l2_subdev_get_pad_format(subdev, sd_state, fmt->pad);
> > + fmt->format.code = __ccs_get_mbus_code(subdev, fmt->pad);
> >
> > return 0;
> > }
> > @@ -2092,28 +2056,18 @@ static int ccs_get_format(struct v4l2_subdev *subdev,
>
> Please replace ccs_get_format() with v4l2_subdev_get_fmt(). It's a
> drop-in replacement for the .get_fmt() operation, you can drop this
> function. The only remaining caller of __ccs_get_format() can then use
> v4l2_subdev_get_fmt() too.
That is my goal but the issue is that it (sligthly) changes UAPI behaviour.
Currently the flipping controls also affect the try formats.
>
> > static void ccs_get_crop_compose(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_rect **crops,
> > - struct v4l2_rect **comps, int which)
> > + struct v4l2_rect **comps)
> > {
> > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > unsigned int i;
> >
> > - if (which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > - if (crops)
> > - for (i = 0; i < subdev->entity.num_pads; i++)
> > - crops[i] = &ssd->crop[i];
> > - if (comps)
> > - *comps = &ssd->compose;
> > - } else {
> > - if (crops) {
> > - for (i = 0; i < subdev->entity.num_pads; i++)
> > - crops[i] = v4l2_subdev_get_try_crop(subdev,
> > - sd_state,
> > - i);
> > - }
> > - if (comps)
> > - *comps = v4l2_subdev_get_try_compose(subdev, sd_state,
> > - CCS_PAD_SINK);
> > - }
> > + if (crops)
> > + for (i = 0; i < subdev->entity.num_pads; i++)
> > + crops[i] =
> > + v4l2_subdev_get_pad_crop(subdev, sd_state, i);
> > + if (comps)
> > + *comps = v4l2_subdev_get_pad_compose(subdev, sd_state,
> > + ssd->sink_pad);
> > }
> >
> > /* Changes require propagation only on sink pad. */
> > @@ -2125,7 +2079,7 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
> > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > struct v4l2_rect *comp, *crops[CCS_PADS];
> >
> > - ccs_get_crop_compose(subdev, sd_state, crops, &comp, which);
> > + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
> >
> > switch (target) {
> > case V4L2_SEL_TGT_CROP:
> > @@ -2136,6 +2090,7 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
> > sensor->scale_m = CCS_LIM(sensor, SCALER_N_MIN);
> > sensor->scaling_mode =
> > CCS_SCALING_MODE_NO_SCALING;
> > + sensor->scaler_sink = *comp;
> > } else if (ssd == sensor->binner) {
> > sensor->binning_horizontal = 1;
> > sensor->binning_vertical = 1;
> > @@ -2144,6 +2099,8 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
> > fallthrough;
> > case V4L2_SEL_TGT_COMPOSE:
> > *crops[CCS_PAD_SRC] = *comp;
> > + if (which == V4L2_SUBDEV_FORMAT_ACTIVE && ssd == sensor->src)
> > + sensor->src_src = *crops[CCS_PAD_SRC];
> > break;
> > default:
> > WARN_ON_ONCE(1);
> > @@ -2252,14 +2209,12 @@ static int ccs_set_format(struct v4l2_subdev *subdev,
> > CCS_LIM(sensor, MIN_Y_OUTPUT_SIZE),
> > CCS_LIM(sensor, MAX_Y_OUTPUT_SIZE));
> >
> > - ccs_get_crop_compose(subdev, sd_state, crops, NULL, fmt->which);
> > + ccs_get_crop_compose(subdev, sd_state, crops, NULL);
> >
> > crops[ssd->sink_pad]->left = 0;
> > crops[ssd->sink_pad]->top = 0;
> > crops[ssd->sink_pad]->width = fmt->format.width;
> > crops[ssd->sink_pad]->height = fmt->format.height;
> > - if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > - ssd->sink_fmt = *crops[ssd->sink_pad];
> > ccs_propagate(subdev, sd_state, fmt->which, V4L2_SEL_TGT_CROP);
> >
> > mutex_unlock(&sensor->mutex);
> > @@ -2482,7 +2437,7 @@ static int ccs_set_compose(struct v4l2_subdev *subdev,
> > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > struct v4l2_rect *comp, *crops[CCS_PADS];
> >
> > - ccs_get_crop_compose(subdev, sd_state, crops, &comp, sel->which);
> > + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
> >
> > sel->r.top = 0;
> > sel->r.left = 0;
> > @@ -2501,8 +2456,8 @@ static int ccs_set_compose(struct v4l2_subdev *subdev,
> > return 0;
> > }
> >
> > -static int __ccs_sel_supported(struct v4l2_subdev *subdev,
> > - struct v4l2_subdev_selection *sel)
> > +static int ccs_sel_supported(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_selection *sel)
> > {
> > struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > @@ -2545,33 +2500,18 @@ static int ccs_set_crop(struct v4l2_subdev *subdev,
> > {
> > struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > - struct v4l2_rect *src_size, *crops[CCS_PADS];
> > - struct v4l2_rect _r;
> > + struct v4l2_rect src_size = { 0 }, *crops[CCS_PADS], *comp;
> >
> > - ccs_get_crop_compose(subdev, sd_state, crops, NULL, sel->which);
> > + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
> >
> > - if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > - if (sel->pad == ssd->sink_pad)
> > - src_size = &ssd->sink_fmt;
> > - else
> > - src_size = &ssd->compose;
> > + if (sel->pad == ssd->sink_pad) {
> > + struct v4l2_mbus_framefmt *mfmt =
> > + v4l2_subdev_get_pad_format(subdev, sd_state, sel->pad);
> > +
> > + src_size.width = mfmt->width;
> > + src_size.height = mfmt->height;
> > } else {
> > - if (sel->pad == ssd->sink_pad) {
> > - _r.left = 0;
> > - _r.top = 0;
> > - _r.width = v4l2_subdev_get_try_format(subdev,
> > - sd_state,
> > - sel->pad)
> > - ->width;
> > - _r.height = v4l2_subdev_get_try_format(subdev,
> > - sd_state,
> > - sel->pad)
> > - ->height;
> > - src_size = &_r;
> > - } else {
> > - src_size = v4l2_subdev_get_try_compose(
> > - subdev, sd_state, ssd->sink_pad);
> > - }
> > + src_size = *comp;
> > }
> >
> > if (ssd == sensor->src && sel->pad == CCS_PAD_SRC) {
> > @@ -2579,16 +2519,19 @@ static int ccs_set_crop(struct v4l2_subdev *subdev,
> > sel->r.top = 0;
> > }
> >
> > - sel->r.width = min(sel->r.width, src_size->width);
> > - sel->r.height = min(sel->r.height, src_size->height);
> > + sel->r.width = min(sel->r.width, src_size.width);
> > + sel->r.height = min(sel->r.height, src_size.height);
> >
> > - sel->r.left = min_t(int, sel->r.left, src_size->width - sel->r.width);
> > - sel->r.top = min_t(int, sel->r.top, src_size->height - sel->r.height);
> > + sel->r.left = min_t(int, sel->r.left, src_size.width - sel->r.width);
> > + sel->r.top = min_t(int, sel->r.top, src_size.height - sel->r.height);
> >
> > *crops[sel->pad] = sel->r;
> >
> > if (ssd != sensor->pixel_array && sel->pad == CCS_PAD_SINK)
> > ccs_propagate(subdev, sd_state, sel->which, V4L2_SEL_TGT_CROP);
> > + else if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
> > + ssd == sensor->pixel_array)
> > + sensor->pa_src = sel->r;
> >
> > return 0;
> > }
> > @@ -2601,44 +2544,36 @@ static void ccs_get_native_size(struct ccs_subdev *ssd, struct v4l2_rect *r)
> > r->height = CCS_LIM(ssd->sensor, Y_ADDR_MAX) + 1;
> > }
> >
> > -static int __ccs_get_selection(struct v4l2_subdev *subdev,
> > - struct v4l2_subdev_state *sd_state,
> > - struct v4l2_subdev_selection *sel)
> > +static int ccs_get_selection(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_selection *sel)
> > {
> > struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > struct v4l2_rect *comp, *crops[CCS_PADS];
> > - struct v4l2_rect sink_fmt;
> > int ret;
> >
> > - ret = __ccs_sel_supported(subdev, sel);
> > + ret = ccs_sel_supported(subdev, sel);
> > if (ret)
> > return ret;
> >
> > - ccs_get_crop_compose(subdev, sd_state, crops, &comp, sel->which);
> > -
> > - if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > - sink_fmt = ssd->sink_fmt;
> > - } else {
> > - struct v4l2_mbus_framefmt *fmt =
> > - v4l2_subdev_get_try_format(subdev, sd_state,
> > - ssd->sink_pad);
> > -
> > - sink_fmt.left = 0;
> > - sink_fmt.top = 0;
> > - sink_fmt.width = fmt->width;
> > - sink_fmt.height = fmt->height;
> > - }
> > + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
> >
> > switch (sel->target) {
> > case V4L2_SEL_TGT_CROP_BOUNDS:
> > case V4L2_SEL_TGT_NATIVE_SIZE:
> > - if (ssd == sensor->pixel_array)
> > + if (ssd == sensor->pixel_array) {
> > ccs_get_native_size(ssd, &sel->r);
> > - else if (sel->pad == ssd->sink_pad)
> > - sel->r = sink_fmt;
> > - else
> > + } else if (sel->pad == ssd->sink_pad) {
> > + struct v4l2_mbus_framefmt *sink_fmt =
> > + v4l2_subdev_get_pad_format(subdev, sd_state,
> > + ssd->sink_pad);
> > + sel->r.top = sel->r.left = 0;
> > + sel->r.width = sink_fmt->width;
> > + sel->r.height = sink_fmt->height;
> > + } else {
> > sel->r = *comp;
> > + }
> > break;
> > case V4L2_SEL_TGT_CROP:
> > case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > @@ -2652,20 +2587,6 @@ static int __ccs_get_selection(struct v4l2_subdev *subdev,
> > return 0;
> > }
> >
> > -static int ccs_get_selection(struct v4l2_subdev *subdev,
> > - struct v4l2_subdev_state *sd_state,
> > - struct v4l2_subdev_selection *sel)
> > -{
> > - struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > - int rval;
> > -
> > - mutex_lock(&sensor->mutex);
> > - rval = __ccs_get_selection(subdev, sd_state, sel);
> > - mutex_unlock(&sensor->mutex);
> > -
> > - return rval;
> > -}
> > -
> > static int ccs_set_selection(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_selection *sel)
> > @@ -2673,7 +2594,7 @@ static int ccs_set_selection(struct v4l2_subdev *subdev,
> > struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > int ret;
> >
> > - ret = __ccs_sel_supported(subdev, sel);
> > + ret = ccs_sel_supported(subdev, sel);
> > if (ret)
> > return ret;
> >
> > @@ -2964,10 +2885,14 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
> > return rval;
> > }
> >
> > + rval = v4l2_subdev_init_finalize(&ssd->sd);
> > + if (rval)
> > + goto out_media_entity_cleanup;
> > +
> > rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev, &ssd->sd);
> > if (rval) {
> > dev_err(&client->dev, "v4l2_device_register_subdev failed\n");
> > - goto out_media_entity_cleanup;
> > + goto out_v4l2_subdev_cleanup;
> > }
> >
> > rval = media_create_pad_link(&ssd->sd.entity, source_pad,
> > @@ -2983,6 +2908,9 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
> > out_v4l2_device_unregister_subdev:
> > v4l2_device_unregister_subdev(&ssd->sd);
> >
> > +out_v4l2_subdev_cleanup:
> > + v4l2_subdev_cleanup(&ssd->sd);
> > +
> > out_media_entity_cleanup:
> > media_entity_cleanup(&ssd->sd.entity);
> >
> > @@ -3059,16 +2987,9 @@ static void ccs_create_subdev(struct ccs_sensor *sensor,
> >
> > v4l2_i2c_subdev_set_name(&ssd->sd, client, sensor->minfo.name, name);
> >
> > - ccs_get_native_size(ssd, &ssd->sink_fmt);
> > -
> > - ssd->compose.width = ssd->sink_fmt.width;
> > - ssd->compose.height = ssd->sink_fmt.height;
> > - ssd->crop[ssd->source_pad] = ssd->compose;
> > ssd->pads[ssd->source_pad].flags = MEDIA_PAD_FL_SOURCE;
> > - if (ssd != sensor->pixel_array) {
> > - ssd->crop[ssd->sink_pad] = ssd->compose;
> > + if (ssd != sensor->pixel_array)
> > ssd->pads[ssd->sink_pad].flags = MEDIA_PAD_FL_SINK;
> > - }
> >
> > ssd->sd.entity.ops = &ccs_entity_ops;
> >
> > @@ -3089,24 +3010,24 @@ static int ccs_init_cfg(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_sta
> > mutex_lock(&sensor->mutex);
> >
> > for (i = 0; i < ssd->npads; i++) {
> > - struct v4l2_mbus_framefmt *try_fmt =
> > - v4l2_subdev_get_try_format(sd, sd_state, i);
> > - struct v4l2_rect *try_crop =
> > - v4l2_subdev_get_try_crop(sd, sd_state, i);
> > - struct v4l2_rect *try_comp;
> > + struct v4l2_mbus_framefmt *pad_fmt =
> > + v4l2_subdev_get_pad_format(sd, sd_state, i);
> > + struct v4l2_rect *pad_crop =
> > + v4l2_subdev_get_pad_crop(sd, sd_state, i);
> > + struct v4l2_rect *pad_comp;
>
> These can simply be called fmt, crop and comp.
Yes.
>
> >
> > - ccs_get_native_size(ssd, try_crop);
> > + ccs_get_native_size(ssd, pad_crop);
> >
> > - try_fmt->width = try_crop->width;
> > - try_fmt->height = try_crop->height;
> > - try_fmt->code = sensor->internal_csi_format->code;
> > - try_fmt->field = V4L2_FIELD_NONE;
> > + pad_fmt->width = pad_crop->width;
> > + pad_fmt->height = pad_crop->height;
> > + pad_fmt->code = sensor->internal_csi_format->code;
> > + pad_fmt->field = V4L2_FIELD_NONE;
> >
> > if (ssd == sensor->pixel_array)
> > continue;
> >
> > - try_comp = v4l2_subdev_get_try_compose(sd, sd_state, i);
> > - *try_comp = *try_crop;
> > + pad_comp = v4l2_subdev_get_pad_compose(sd, sd_state, i);
> > + *pad_comp = *pad_crop;
> > }
> >
> > mutex_unlock(&sensor->mutex);
> > @@ -3631,6 +3552,10 @@ static int ccs_probe(struct i2c_client *client)
> > if (rval < 0)
> > goto out_media_entity_cleanup;
> >
> > + rval = v4l2_subdev_init_finalize(&sensor->src->sd);
> > + if (rval)
> > + goto out_media_entity_cleanup;
> > +
> > rval = ccs_write_msr_regs(sensor);
> > if (rval)
> > goto out_media_entity_cleanup;
> > @@ -3690,6 +3615,7 @@ static void ccs_remove(struct i2c_client *client)
> >
> > for (i = 0; i < sensor->ssds_used; i++) {
> > v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
> > + v4l2_subdev_cleanup(subdev);
> > media_entity_cleanup(&sensor->ssds[i].sd.entity);
> > }
> > ccs_cleanup(sensor);
> > diff --git a/drivers/media/i2c/ccs/ccs.h b/drivers/media/i2c/ccs/ccs.h
> > index a94c796cea48..9c3587b2fbe7 100644
> > --- a/drivers/media/i2c/ccs/ccs.h
> > +++ b/drivers/media/i2c/ccs/ccs.h
> > @@ -182,9 +182,6 @@ struct ccs_binning_subtype {
> > struct ccs_subdev {
> > struct v4l2_subdev sd;
> > struct media_pad pads[CCS_PADS];
> > - struct v4l2_rect sink_fmt;
> > - struct v4l2_rect crop[CCS_PADS];
> > - struct v4l2_rect compose; /* compose on sink */
> > unsigned short sink_pad;
> > unsigned short source_pad;
> > int npads;
> > @@ -220,6 +217,7 @@ struct ccs_sensor {
> > u32 mbus_frame_fmts;
> > const struct ccs_csi_data_format *csi_format;
> > const struct ccs_csi_data_format *internal_csi_format;
> > + struct v4l2_rect pa_src, scaler_sink, src_src;
>
> The idea of the active state API is to remove all active state from the
> driver private structure. Why do you need these, can't you get them from
> the active state where appropriate ?
The issue is the dependency to controls that can't be accessed with the
state lock (only) held.
If we can remove that dependency, with a slight change to UAPI, then this
can be done.
>
> > u32 default_mbus_frame_fmts;
> > int default_pixel_order;
> > struct ccs_data_container sdata, mdata;
>
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 06/12] media: ccs: Use sub-device active state
2023-09-19 10:29 ` Sakari Ailus
@ 2023-09-19 13:42 ` Laurent Pinchart
0 siblings, 0 replies; 28+ messages in thread
From: Laurent Pinchart @ 2023-09-19 13:42 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Sakari,
On Tue, Sep 19, 2023 at 10:29:44AM +0000, Sakari Ailus wrote:
> On Mon, Sep 18, 2023 at 04:59:33PM +0300, Laurent Pinchart wrote:
> > On Mon, Sep 18, 2023 at 03:51:32PM +0300, Sakari Ailus wrote:
> > > Make use of sub-device active state. In most cases the effect on need for
> > > acquiring the mutex is non-existent as access to the driver's core data
> > > structure still needs to be serialised.
> > >
> > > This still removes a lot of code as the code paths for active and try
> > > state are the same in many cases.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > drivers/media/i2c/ccs/ccs-core.c | 278 ++++++++++++-------------------
> > > drivers/media/i2c/ccs/ccs.h | 4 +-
> > > 2 files changed, 103 insertions(+), 179 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > index db461b0e49c8..efed75b6534c 100644
> > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > @@ -508,9 +508,8 @@ static void __ccs_update_exposure_limits(struct ccs_sensor *sensor)
> > > struct v4l2_ctrl *ctrl = sensor->exposure;
> > > int max;
> > >
> > > - max = sensor->pixel_array->crop[CCS_PA_PAD_SRC].height
> > > - + sensor->vblank->val
> > > - - CCS_LIM(sensor, COARSE_INTEGRATION_TIME_MAX_MARGIN);
> > > + max = sensor->pa_src.height + sensor->vblank->val -
> > > + CCS_LIM(sensor, COARSE_INTEGRATION_TIME_MAX_MARGIN);
> > >
> > > __v4l2_ctrl_modify_range(ctrl, ctrl->minimum, max, ctrl->step, max);
> > > }
> > > @@ -728,15 +727,12 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl)
> > > break;
> > > case V4L2_CID_VBLANK:
> > > rval = ccs_write(sensor, FRAME_LENGTH_LINES,
> > > - sensor->pixel_array->crop[
> > > - CCS_PA_PAD_SRC].height
> > > - + ctrl->val);
> > > + sensor->pa_src.height + ctrl->val);
> > >
> > > break;
> > > case V4L2_CID_HBLANK:
> > > rval = ccs_write(sensor, LINE_LENGTH_PCK,
> > > - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width
> > > - + ctrl->val);
> > > + sensor->pa_src.width + ctrl->val);
> > >
> > > break;
> > > case V4L2_CID_TEST_PATTERN:
> > > @@ -1214,15 +1210,13 @@ static void ccs_update_blanking(struct ccs_sensor *sensor)
> > >
> > > min = max_t(int,
> > > CCS_LIM(sensor, MIN_FRAME_BLANKING_LINES),
> > > - min_fll - sensor->pixel_array->crop[CCS_PA_PAD_SRC].height);
> > > - max = max_fll - sensor->pixel_array->crop[CCS_PA_PAD_SRC].height;
> > > + min_fll - sensor->pa_src.height);
> > > + max = max_fll - sensor->pa_src.height;
> > >
> > > __v4l2_ctrl_modify_range(vblank, min, max, vblank->step, min);
> > >
> > > - min = max_t(int,
> > > - min_llp - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width,
> > > - min_lbp);
> > > - max = max_llp - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width;
> > > + min = max_t(int, min_llp - sensor->pa_src.width, min_lbp);
> > > + max = max_llp - sensor->pa_src.width;
> > >
> > > __v4l2_ctrl_modify_range(hblank, min, max, hblank->step, min);
> > >
> > > @@ -1246,10 +1240,8 @@ static int ccs_pll_blanking_update(struct ccs_sensor *sensor)
> > >
> > > dev_dbg(&client->dev, "real timeperframe\t100/%d\n",
> > > sensor->pll.pixel_rate_pixel_array /
> > > - ((sensor->pixel_array->crop[CCS_PA_PAD_SRC].width
> > > - + sensor->hblank->val) *
> > > - (sensor->pixel_array->crop[CCS_PA_PAD_SRC].height
> > > - + sensor->vblank->val) / 100));
> > > + ((sensor->pa_src.width + sensor->hblank->val) *
> > > + (sensor->pa_src.height + sensor->vblank->val) / 100));
> > >
> > > return 0;
> > > }
> > > @@ -1756,28 +1748,22 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
> > > goto out;
> > >
> > > /* Analog crop start coordinates */
> > > - rval = ccs_write(sensor, X_ADDR_START,
> > > - sensor->pixel_array->crop[CCS_PA_PAD_SRC].left);
> > > + rval = ccs_write(sensor, X_ADDR_START, sensor->pa_src.left);
> > > if (rval < 0)
> > > goto out;
> > >
> > > - rval = ccs_write(sensor, Y_ADDR_START,
> > > - sensor->pixel_array->crop[CCS_PA_PAD_SRC].top);
> > > + rval = ccs_write(sensor, Y_ADDR_START, sensor->pa_src.top);
> > > if (rval < 0)
> > > goto out;
> > >
> > > /* Analog crop end coordinates */
> > > - rval = ccs_write(
> > > - sensor, X_ADDR_END,
> > > - sensor->pixel_array->crop[CCS_PA_PAD_SRC].left
> > > - + sensor->pixel_array->crop[CCS_PA_PAD_SRC].width - 1);
> > > + rval = ccs_write(sensor, X_ADDR_END,
> > > + sensor->pa_src.left + sensor->pa_src.width - 1);
> > > if (rval < 0)
> > > goto out;
> > >
> > > - rval = ccs_write(
> > > - sensor, Y_ADDR_END,
> > > - sensor->pixel_array->crop[CCS_PA_PAD_SRC].top
> > > - + sensor->pixel_array->crop[CCS_PA_PAD_SRC].height - 1);
> > > + rval = ccs_write(sensor, Y_ADDR_END,
> > > + sensor->pa_src.top + sensor->pa_src.height - 1);
> > > if (rval < 0)
> > > goto out;
> > >
> > > @@ -1789,27 +1775,23 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
> > > /* Digital crop */
> > > if (CCS_LIM(sensor, DIGITAL_CROP_CAPABILITY)
> > > == CCS_DIGITAL_CROP_CAPABILITY_INPUT_CROP) {
> > > - rval = ccs_write(
> > > - sensor, DIGITAL_CROP_X_OFFSET,
> > > - sensor->scaler->crop[CCS_PAD_SINK].left);
> > > + rval = ccs_write(sensor, DIGITAL_CROP_X_OFFSET,
> > > + sensor->scaler_sink.left);
> > > if (rval < 0)
> > > goto out;
> > >
> > > - rval = ccs_write(
> > > - sensor, DIGITAL_CROP_Y_OFFSET,
> > > - sensor->scaler->crop[CCS_PAD_SINK].top);
> > > + rval = ccs_write(sensor, DIGITAL_CROP_Y_OFFSET,
> > > + sensor->scaler_sink.top);
> > > if (rval < 0)
> > > goto out;
> > >
> > > - rval = ccs_write(
> > > - sensor, DIGITAL_CROP_IMAGE_WIDTH,
> > > - sensor->scaler->crop[CCS_PAD_SINK].width);
> > > + rval = ccs_write(sensor, DIGITAL_CROP_IMAGE_WIDTH,
> > > + sensor->scaler_sink.width);
> > > if (rval < 0)
> > > goto out;
> > >
> > > - rval = ccs_write(
> > > - sensor, DIGITAL_CROP_IMAGE_HEIGHT,
> > > - sensor->scaler->crop[CCS_PAD_SINK].height);
> > > + rval = ccs_write(sensor, DIGITAL_CROP_IMAGE_HEIGHT,
> > > + sensor->scaler_sink.height);
> > > if (rval < 0)
> > > goto out;
> > > }
> > > @@ -1827,12 +1809,10 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
> > > }
> > >
> > > /* Output size from sensor */
> > > - rval = ccs_write(sensor, X_OUTPUT_SIZE,
> > > - sensor->src->crop[CCS_PAD_SRC].width);
> > > + rval = ccs_write(sensor, X_OUTPUT_SIZE, sensor->src_src.width);
> > > if (rval < 0)
> > > goto out;
> > > - rval = ccs_write(sensor, Y_OUTPUT_SIZE,
> > > - sensor->src->crop[CCS_PAD_SRC].height);
> > > + rval = ccs_write(sensor, Y_OUTPUT_SIZE, sensor->src_src.height);
> > > if (rval < 0)
> > > goto out;
> > >
> > > @@ -2053,24 +2033,8 @@ static int __ccs_get_format(struct v4l2_subdev *subdev,
> > > struct v4l2_subdev_state *sd_state,
> > > struct v4l2_subdev_format *fmt)
> > > {
> > > - struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > > -
> > > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > - fmt->format = *v4l2_subdev_get_try_format(subdev, sd_state,
> > > - fmt->pad);
> > > - } else {
> > > - struct v4l2_rect *r;
> > > -
> > > - if (fmt->pad == ssd->source_pad)
> > > - r = &ssd->crop[ssd->source_pad];
> > > - else
> > > - r = &ssd->sink_fmt;
> > > -
> > > - fmt->format.code = __ccs_get_mbus_code(subdev, fmt->pad);
> > > - fmt->format.width = r->width;
> > > - fmt->format.height = r->height;
> > > - fmt->format.field = V4L2_FIELD_NONE;
> > > - }
> > > + fmt->format = *v4l2_subdev_get_pad_format(subdev, sd_state, fmt->pad);
> > > + fmt->format.code = __ccs_get_mbus_code(subdev, fmt->pad);
> > >
> > > return 0;
> > > }
> > > @@ -2092,28 +2056,18 @@ static int ccs_get_format(struct v4l2_subdev *subdev,
> >
> > Please replace ccs_get_format() with v4l2_subdev_get_fmt(). It's a
> > drop-in replacement for the .get_fmt() operation, you can drop this
> > function. The only remaining caller of __ccs_get_format() can then use
> > v4l2_subdev_get_fmt() too.
>
> That is my goal but the issue is that it (sligthly) changes UAPI behaviour.
> Currently the flipping controls also affect the try formats.
Let's fix it sooner than later, different behaviours for different
sensors is very painful for userspace.
> > > static void ccs_get_crop_compose(struct v4l2_subdev *subdev,
> > > struct v4l2_subdev_state *sd_state,
> > > struct v4l2_rect **crops,
> > > - struct v4l2_rect **comps, int which)
> > > + struct v4l2_rect **comps)
> > > {
> > > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > > unsigned int i;
> > >
> > > - if (which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > - if (crops)
> > > - for (i = 0; i < subdev->entity.num_pads; i++)
> > > - crops[i] = &ssd->crop[i];
> > > - if (comps)
> > > - *comps = &ssd->compose;
> > > - } else {
> > > - if (crops) {
> > > - for (i = 0; i < subdev->entity.num_pads; i++)
> > > - crops[i] = v4l2_subdev_get_try_crop(subdev,
> > > - sd_state,
> > > - i);
> > > - }
> > > - if (comps)
> > > - *comps = v4l2_subdev_get_try_compose(subdev, sd_state,
> > > - CCS_PAD_SINK);
> > > - }
> > > + if (crops)
> > > + for (i = 0; i < subdev->entity.num_pads; i++)
> > > + crops[i] =
> > > + v4l2_subdev_get_pad_crop(subdev, sd_state, i);
> > > + if (comps)
> > > + *comps = v4l2_subdev_get_pad_compose(subdev, sd_state,
> > > + ssd->sink_pad);
> > > }
> > >
> > > /* Changes require propagation only on sink pad. */
> > > @@ -2125,7 +2079,7 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
> > > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > > struct v4l2_rect *comp, *crops[CCS_PADS];
> > >
> > > - ccs_get_crop_compose(subdev, sd_state, crops, &comp, which);
> > > + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
> > >
> > > switch (target) {
> > > case V4L2_SEL_TGT_CROP:
> > > @@ -2136,6 +2090,7 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
> > > sensor->scale_m = CCS_LIM(sensor, SCALER_N_MIN);
> > > sensor->scaling_mode =
> > > CCS_SCALING_MODE_NO_SCALING;
> > > + sensor->scaler_sink = *comp;
> > > } else if (ssd == sensor->binner) {
> > > sensor->binning_horizontal = 1;
> > > sensor->binning_vertical = 1;
> > > @@ -2144,6 +2099,8 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
> > > fallthrough;
> > > case V4L2_SEL_TGT_COMPOSE:
> > > *crops[CCS_PAD_SRC] = *comp;
> > > + if (which == V4L2_SUBDEV_FORMAT_ACTIVE && ssd == sensor->src)
> > > + sensor->src_src = *crops[CCS_PAD_SRC];
> > > break;
> > > default:
> > > WARN_ON_ONCE(1);
> > > @@ -2252,14 +2209,12 @@ static int ccs_set_format(struct v4l2_subdev *subdev,
> > > CCS_LIM(sensor, MIN_Y_OUTPUT_SIZE),
> > > CCS_LIM(sensor, MAX_Y_OUTPUT_SIZE));
> > >
> > > - ccs_get_crop_compose(subdev, sd_state, crops, NULL, fmt->which);
> > > + ccs_get_crop_compose(subdev, sd_state, crops, NULL);
> > >
> > > crops[ssd->sink_pad]->left = 0;
> > > crops[ssd->sink_pad]->top = 0;
> > > crops[ssd->sink_pad]->width = fmt->format.width;
> > > crops[ssd->sink_pad]->height = fmt->format.height;
> > > - if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > > - ssd->sink_fmt = *crops[ssd->sink_pad];
> > > ccs_propagate(subdev, sd_state, fmt->which, V4L2_SEL_TGT_CROP);
> > >
> > > mutex_unlock(&sensor->mutex);
> > > @@ -2482,7 +2437,7 @@ static int ccs_set_compose(struct v4l2_subdev *subdev,
> > > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > > struct v4l2_rect *comp, *crops[CCS_PADS];
> > >
> > > - ccs_get_crop_compose(subdev, sd_state, crops, &comp, sel->which);
> > > + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
> > >
> > > sel->r.top = 0;
> > > sel->r.left = 0;
> > > @@ -2501,8 +2456,8 @@ static int ccs_set_compose(struct v4l2_subdev *subdev,
> > > return 0;
> > > }
> > >
> > > -static int __ccs_sel_supported(struct v4l2_subdev *subdev,
> > > - struct v4l2_subdev_selection *sel)
> > > +static int ccs_sel_supported(struct v4l2_subdev *subdev,
> > > + struct v4l2_subdev_selection *sel)
> > > {
> > > struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > > @@ -2545,33 +2500,18 @@ static int ccs_set_crop(struct v4l2_subdev *subdev,
> > > {
> > > struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > > - struct v4l2_rect *src_size, *crops[CCS_PADS];
> > > - struct v4l2_rect _r;
> > > + struct v4l2_rect src_size = { 0 }, *crops[CCS_PADS], *comp;
> > >
> > > - ccs_get_crop_compose(subdev, sd_state, crops, NULL, sel->which);
> > > + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
> > >
> > > - if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > - if (sel->pad == ssd->sink_pad)
> > > - src_size = &ssd->sink_fmt;
> > > - else
> > > - src_size = &ssd->compose;
> > > + if (sel->pad == ssd->sink_pad) {
> > > + struct v4l2_mbus_framefmt *mfmt =
> > > + v4l2_subdev_get_pad_format(subdev, sd_state, sel->pad);
> > > +
> > > + src_size.width = mfmt->width;
> > > + src_size.height = mfmt->height;
> > > } else {
> > > - if (sel->pad == ssd->sink_pad) {
> > > - _r.left = 0;
> > > - _r.top = 0;
> > > - _r.width = v4l2_subdev_get_try_format(subdev,
> > > - sd_state,
> > > - sel->pad)
> > > - ->width;
> > > - _r.height = v4l2_subdev_get_try_format(subdev,
> > > - sd_state,
> > > - sel->pad)
> > > - ->height;
> > > - src_size = &_r;
> > > - } else {
> > > - src_size = v4l2_subdev_get_try_compose(
> > > - subdev, sd_state, ssd->sink_pad);
> > > - }
> > > + src_size = *comp;
> > > }
> > >
> > > if (ssd == sensor->src && sel->pad == CCS_PAD_SRC) {
> > > @@ -2579,16 +2519,19 @@ static int ccs_set_crop(struct v4l2_subdev *subdev,
> > > sel->r.top = 0;
> > > }
> > >
> > > - sel->r.width = min(sel->r.width, src_size->width);
> > > - sel->r.height = min(sel->r.height, src_size->height);
> > > + sel->r.width = min(sel->r.width, src_size.width);
> > > + sel->r.height = min(sel->r.height, src_size.height);
> > >
> > > - sel->r.left = min_t(int, sel->r.left, src_size->width - sel->r.width);
> > > - sel->r.top = min_t(int, sel->r.top, src_size->height - sel->r.height);
> > > + sel->r.left = min_t(int, sel->r.left, src_size.width - sel->r.width);
> > > + sel->r.top = min_t(int, sel->r.top, src_size.height - sel->r.height);
> > >
> > > *crops[sel->pad] = sel->r;
> > >
> > > if (ssd != sensor->pixel_array && sel->pad == CCS_PAD_SINK)
> > > ccs_propagate(subdev, sd_state, sel->which, V4L2_SEL_TGT_CROP);
> > > + else if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
> > > + ssd == sensor->pixel_array)
> > > + sensor->pa_src = sel->r;
> > >
> > > return 0;
> > > }
> > > @@ -2601,44 +2544,36 @@ static void ccs_get_native_size(struct ccs_subdev *ssd, struct v4l2_rect *r)
> > > r->height = CCS_LIM(ssd->sensor, Y_ADDR_MAX) + 1;
> > > }
> > >
> > > -static int __ccs_get_selection(struct v4l2_subdev *subdev,
> > > - struct v4l2_subdev_state *sd_state,
> > > - struct v4l2_subdev_selection *sel)
> > > +static int ccs_get_selection(struct v4l2_subdev *subdev,
> > > + struct v4l2_subdev_state *sd_state,
> > > + struct v4l2_subdev_selection *sel)
> > > {
> > > struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > > struct ccs_subdev *ssd = to_ccs_subdev(subdev);
> > > struct v4l2_rect *comp, *crops[CCS_PADS];
> > > - struct v4l2_rect sink_fmt;
> > > int ret;
> > >
> > > - ret = __ccs_sel_supported(subdev, sel);
> > > + ret = ccs_sel_supported(subdev, sel);
> > > if (ret)
> > > return ret;
> > >
> > > - ccs_get_crop_compose(subdev, sd_state, crops, &comp, sel->which);
> > > -
> > > - if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > - sink_fmt = ssd->sink_fmt;
> > > - } else {
> > > - struct v4l2_mbus_framefmt *fmt =
> > > - v4l2_subdev_get_try_format(subdev, sd_state,
> > > - ssd->sink_pad);
> > > -
> > > - sink_fmt.left = 0;
> > > - sink_fmt.top = 0;
> > > - sink_fmt.width = fmt->width;
> > > - sink_fmt.height = fmt->height;
> > > - }
> > > + ccs_get_crop_compose(subdev, sd_state, crops, &comp);
> > >
> > > switch (sel->target) {
> > > case V4L2_SEL_TGT_CROP_BOUNDS:
> > > case V4L2_SEL_TGT_NATIVE_SIZE:
> > > - if (ssd == sensor->pixel_array)
> > > + if (ssd == sensor->pixel_array) {
> > > ccs_get_native_size(ssd, &sel->r);
> > > - else if (sel->pad == ssd->sink_pad)
> > > - sel->r = sink_fmt;
> > > - else
> > > + } else if (sel->pad == ssd->sink_pad) {
> > > + struct v4l2_mbus_framefmt *sink_fmt =
> > > + v4l2_subdev_get_pad_format(subdev, sd_state,
> > > + ssd->sink_pad);
> > > + sel->r.top = sel->r.left = 0;
> > > + sel->r.width = sink_fmt->width;
> > > + sel->r.height = sink_fmt->height;
> > > + } else {
> > > sel->r = *comp;
> > > + }
> > > break;
> > > case V4L2_SEL_TGT_CROP:
> > > case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > > @@ -2652,20 +2587,6 @@ static int __ccs_get_selection(struct v4l2_subdev *subdev,
> > > return 0;
> > > }
> > >
> > > -static int ccs_get_selection(struct v4l2_subdev *subdev,
> > > - struct v4l2_subdev_state *sd_state,
> > > - struct v4l2_subdev_selection *sel)
> > > -{
> > > - struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > > - int rval;
> > > -
> > > - mutex_lock(&sensor->mutex);
> > > - rval = __ccs_get_selection(subdev, sd_state, sel);
> > > - mutex_unlock(&sensor->mutex);
> > > -
> > > - return rval;
> > > -}
> > > -
> > > static int ccs_set_selection(struct v4l2_subdev *subdev,
> > > struct v4l2_subdev_state *sd_state,
> > > struct v4l2_subdev_selection *sel)
> > > @@ -2673,7 +2594,7 @@ static int ccs_set_selection(struct v4l2_subdev *subdev,
> > > struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > > int ret;
> > >
> > > - ret = __ccs_sel_supported(subdev, sel);
> > > + ret = ccs_sel_supported(subdev, sel);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -2964,10 +2885,14 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
> > > return rval;
> > > }
> > >
> > > + rval = v4l2_subdev_init_finalize(&ssd->sd);
> > > + if (rval)
> > > + goto out_media_entity_cleanup;
> > > +
> > > rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev, &ssd->sd);
> > > if (rval) {
> > > dev_err(&client->dev, "v4l2_device_register_subdev failed\n");
> > > - goto out_media_entity_cleanup;
> > > + goto out_v4l2_subdev_cleanup;
> > > }
> > >
> > > rval = media_create_pad_link(&ssd->sd.entity, source_pad,
> > > @@ -2983,6 +2908,9 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
> > > out_v4l2_device_unregister_subdev:
> > > v4l2_device_unregister_subdev(&ssd->sd);
> > >
> > > +out_v4l2_subdev_cleanup:
> > > + v4l2_subdev_cleanup(&ssd->sd);
> > > +
> > > out_media_entity_cleanup:
> > > media_entity_cleanup(&ssd->sd.entity);
> > >
> > > @@ -3059,16 +2987,9 @@ static void ccs_create_subdev(struct ccs_sensor *sensor,
> > >
> > > v4l2_i2c_subdev_set_name(&ssd->sd, client, sensor->minfo.name, name);
> > >
> > > - ccs_get_native_size(ssd, &ssd->sink_fmt);
> > > -
> > > - ssd->compose.width = ssd->sink_fmt.width;
> > > - ssd->compose.height = ssd->sink_fmt.height;
> > > - ssd->crop[ssd->source_pad] = ssd->compose;
> > > ssd->pads[ssd->source_pad].flags = MEDIA_PAD_FL_SOURCE;
> > > - if (ssd != sensor->pixel_array) {
> > > - ssd->crop[ssd->sink_pad] = ssd->compose;
> > > + if (ssd != sensor->pixel_array)
> > > ssd->pads[ssd->sink_pad].flags = MEDIA_PAD_FL_SINK;
> > > - }
> > >
> > > ssd->sd.entity.ops = &ccs_entity_ops;
> > >
> > > @@ -3089,24 +3010,24 @@ static int ccs_init_cfg(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_sta
> > > mutex_lock(&sensor->mutex);
> > >
> > > for (i = 0; i < ssd->npads; i++) {
> > > - struct v4l2_mbus_framefmt *try_fmt =
> > > - v4l2_subdev_get_try_format(sd, sd_state, i);
> > > - struct v4l2_rect *try_crop =
> > > - v4l2_subdev_get_try_crop(sd, sd_state, i);
> > > - struct v4l2_rect *try_comp;
> > > + struct v4l2_mbus_framefmt *pad_fmt =
> > > + v4l2_subdev_get_pad_format(sd, sd_state, i);
> > > + struct v4l2_rect *pad_crop =
> > > + v4l2_subdev_get_pad_crop(sd, sd_state, i);
> > > + struct v4l2_rect *pad_comp;
> >
> > These can simply be called fmt, crop and comp.
>
> Yes.
>
> > >
> > > - ccs_get_native_size(ssd, try_crop);
> > > + ccs_get_native_size(ssd, pad_crop);
> > >
> > > - try_fmt->width = try_crop->width;
> > > - try_fmt->height = try_crop->height;
> > > - try_fmt->code = sensor->internal_csi_format->code;
> > > - try_fmt->field = V4L2_FIELD_NONE;
> > > + pad_fmt->width = pad_crop->width;
> > > + pad_fmt->height = pad_crop->height;
> > > + pad_fmt->code = sensor->internal_csi_format->code;
> > > + pad_fmt->field = V4L2_FIELD_NONE;
> > >
> > > if (ssd == sensor->pixel_array)
> > > continue;
> > >
> > > - try_comp = v4l2_subdev_get_try_compose(sd, sd_state, i);
> > > - *try_comp = *try_crop;
> > > + pad_comp = v4l2_subdev_get_pad_compose(sd, sd_state, i);
> > > + *pad_comp = *pad_crop;
> > > }
> > >
> > > mutex_unlock(&sensor->mutex);
> > > @@ -3631,6 +3552,10 @@ static int ccs_probe(struct i2c_client *client)
> > > if (rval < 0)
> > > goto out_media_entity_cleanup;
> > >
> > > + rval = v4l2_subdev_init_finalize(&sensor->src->sd);
> > > + if (rval)
> > > + goto out_media_entity_cleanup;
> > > +
> > > rval = ccs_write_msr_regs(sensor);
> > > if (rval)
> > > goto out_media_entity_cleanup;
> > > @@ -3690,6 +3615,7 @@ static void ccs_remove(struct i2c_client *client)
> > >
> > > for (i = 0; i < sensor->ssds_used; i++) {
> > > v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
> > > + v4l2_subdev_cleanup(subdev);
> > > media_entity_cleanup(&sensor->ssds[i].sd.entity);
> > > }
> > > ccs_cleanup(sensor);
> > > diff --git a/drivers/media/i2c/ccs/ccs.h b/drivers/media/i2c/ccs/ccs.h
> > > index a94c796cea48..9c3587b2fbe7 100644
> > > --- a/drivers/media/i2c/ccs/ccs.h
> > > +++ b/drivers/media/i2c/ccs/ccs.h
> > > @@ -182,9 +182,6 @@ struct ccs_binning_subtype {
> > > struct ccs_subdev {
> > > struct v4l2_subdev sd;
> > > struct media_pad pads[CCS_PADS];
> > > - struct v4l2_rect sink_fmt;
> > > - struct v4l2_rect crop[CCS_PADS];
> > > - struct v4l2_rect compose; /* compose on sink */
> > > unsigned short sink_pad;
> > > unsigned short source_pad;
> > > int npads;
> > > @@ -220,6 +217,7 @@ struct ccs_sensor {
> > > u32 mbus_frame_fmts;
> > > const struct ccs_csi_data_format *csi_format;
> > > const struct ccs_csi_data_format *internal_csi_format;
> > > + struct v4l2_rect pa_src, scaler_sink, src_src;
> >
> > The idea of the active state API is to remove all active state from the
> > driver private structure. Why do you need these, can't you get them from
> > the active state where appropriate ?
>
> The issue is the dependency to controls that can't be accessed with the
> state lock (only) held.
>
> If we can remove that dependency, with a slight change to UAPI, then this
> can be done.
Eagerly waiting for patches :-)
> > > u32 default_mbus_frame_fmts;
> > > int default_pixel_order;
> > > struct ccs_data_container sdata, mdata;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 07/12] media: ov2740: Enable runtime PM before registering the async subdev
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
` (5 preceding siblings ...)
2023-09-18 12:51 ` [PATCH v2 06/12] media: ccs: Use sub-device active state Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 13:27 ` Laurent Pinchart
2023-09-18 12:51 ` [PATCH v2 08/12] media: ov2740: Use sub-device active state Sakari Ailus
` (4 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Enable runtime PM before registering the async subdev as the driver UAPI
may become accessible immediately after the registration. Runtime PM needs
to be enabled by that time.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/i2c/ov2740.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 41d4f85470fd..319dc00e47b4 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -1172,6 +1172,12 @@ static int ov2740_probe(struct i2c_client *client)
goto probe_error_v4l2_ctrl_handler_free;
}
+ /* Set the device's state to active if it's in D0 state. */
+ if (full_power)
+ pm_runtime_set_active(&client->dev);
+ pm_runtime_enable(&client->dev);
+ pm_runtime_idle(&client->dev);
+
ret = v4l2_async_register_subdev_sensor(&ov2740->sd);
if (ret < 0) {
dev_err_probe(dev, ret, "failed to register V4L2 subdev\n");
@@ -1182,16 +1188,12 @@ static int ov2740_probe(struct i2c_client *client)
if (ret)
dev_warn(&client->dev, "register nvmem failed, ret %d\n", ret);
- /* Set the device's state to active if it's in D0 state. */
- if (full_power)
- pm_runtime_set_active(&client->dev);
- pm_runtime_enable(&client->dev);
- pm_runtime_idle(&client->dev);
-
return 0;
probe_error_media_entity_cleanup:
media_entity_cleanup(&ov2740->sd.entity);
+ pm_runtime_disable(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
probe_error_v4l2_ctrl_handler_free:
v4l2_ctrl_handler_free(ov2740->sd.ctrl_handler);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 07/12] media: ov2740: Enable runtime PM before registering the async subdev
2023-09-18 12:51 ` [PATCH v2 07/12] media: ov2740: Enable runtime PM before registering the async subdev Sakari Ailus
@ 2023-09-18 13:27 ` Laurent Pinchart
0 siblings, 0 replies; 28+ messages in thread
From: Laurent Pinchart @ 2023-09-18 13:27 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Sakari,
Thank you for the patch.
On Mon, Sep 18, 2023 at 03:51:33PM +0300, Sakari Ailus wrote:
> Enable runtime PM before registering the async subdev as the driver UAPI
> may become accessible immediately after the registration. Runtime PM needs
> to be enabled by that time.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/i2c/ov2740.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index 41d4f85470fd..319dc00e47b4 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -1172,6 +1172,12 @@ static int ov2740_probe(struct i2c_client *client)
> goto probe_error_v4l2_ctrl_handler_free;
> }
>
> + /* Set the device's state to active if it's in D0 state. */
> + if (full_power)
> + pm_runtime_set_active(&client->dev);
> + pm_runtime_enable(&client->dev);
> + pm_runtime_idle(&client->dev);
> +
> ret = v4l2_async_register_subdev_sensor(&ov2740->sd);
> if (ret < 0) {
> dev_err_probe(dev, ret, "failed to register V4L2 subdev\n");
> @@ -1182,16 +1188,12 @@ static int ov2740_probe(struct i2c_client *client)
> if (ret)
> dev_warn(&client->dev, "register nvmem failed, ret %d\n", ret);
>
> - /* Set the device's state to active if it's in D0 state. */
> - if (full_power)
> - pm_runtime_set_active(&client->dev);
> - pm_runtime_enable(&client->dev);
> - pm_runtime_idle(&client->dev);
> -
> return 0;
>
> probe_error_media_entity_cleanup:
> media_entity_cleanup(&ov2740->sd.entity);
> + pm_runtime_disable(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
>
> probe_error_v4l2_ctrl_handler_free:
> v4l2_ctrl_handler_free(ov2740->sd.ctrl_handler);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 08/12] media: ov2740: Use sub-device active state
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
` (6 preceding siblings ...)
2023-09-18 12:51 ` [PATCH v2 07/12] media: ov2740: Enable runtime PM before registering the async subdev Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 12:51 ` [PATCH v2 09/12] media: ov2740: Return -EPROBE_DEFER if no endpoint is found Sakari Ailus
` (3 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Use sub-device active state. Rely on control handler lock to serialise
access to the active state. Also clean up locking on s_stream handler.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/i2c/ov2740.c | 122 +++++++++++++++----------------------
1 file changed, 50 insertions(+), 72 deletions(-)
diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 319dc00e47b4..2c00e653ec47 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -336,9 +336,6 @@ struct ov2740 {
/* Current mode */
const struct ov2740_mode *cur_mode;
- /* To serialize asynchronus callbacks */
- struct mutex mutex;
-
/* Streaming on/off */
bool streaming;
@@ -582,7 +579,6 @@ static int ov2740_init_controls(struct ov2740 *ov2740)
if (ret)
return ret;
- ctrl_hdlr->lock = &ov2740->mutex;
cur_mode = ov2740->cur_mode;
size = ARRAY_SIZE(link_freq_menu_items);
@@ -792,18 +788,18 @@ static int ov2740_set_stream(struct v4l2_subdev *sd, int enable)
{
struct ov2740 *ov2740 = to_ov2740(sd);
struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct v4l2_subdev_state *sd_state;
int ret = 0;
+ sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
+
if (ov2740->streaming == enable)
- return 0;
+ goto out_unlock;
- mutex_lock(&ov2740->mutex);
if (enable) {
ret = pm_runtime_resume_and_get(&client->dev);
- if (ret < 0) {
- mutex_unlock(&ov2740->mutex);
- return ret;
- }
+ if (ret < 0)
+ goto out_unlock;
ret = ov2740_start_streaming(ov2740);
if (ret) {
@@ -817,7 +813,9 @@ static int ov2740_set_stream(struct v4l2_subdev *sd, int enable)
}
ov2740->streaming = enable;
- mutex_unlock(&ov2740->mutex);
+
+out_unlock:
+ v4l2_subdev_unlock_state(sd_state);
return ret;
}
@@ -826,12 +824,13 @@ static int ov2740_suspend(struct device *dev)
{
struct v4l2_subdev *sd = dev_get_drvdata(dev);
struct ov2740 *ov2740 = to_ov2740(sd);
+ struct v4l2_subdev_state *sd_state;
- mutex_lock(&ov2740->mutex);
+ sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
if (ov2740->streaming)
ov2740_stop_streaming(ov2740);
- mutex_unlock(&ov2740->mutex);
+ v4l2_subdev_unlock_state(sd_state);
return 0;
}
@@ -840,9 +839,10 @@ static int ov2740_resume(struct device *dev)
{
struct v4l2_subdev *sd = dev_get_drvdata(dev);
struct ov2740 *ov2740 = to_ov2740(sd);
+ struct v4l2_subdev_state *sd_state;
int ret = 0;
- mutex_lock(&ov2740->mutex);
+ sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
if (!ov2740->streaming)
goto exit;
@@ -853,7 +853,7 @@ static int ov2740_resume(struct device *dev)
}
exit:
- mutex_unlock(&ov2740->mutex);
+ v4l2_subdev_unlock_state(sd_state);
return ret;
}
@@ -870,48 +870,26 @@ static int ov2740_set_format(struct v4l2_subdev *sd,
height, fmt->format.width,
fmt->format.height);
- mutex_lock(&ov2740->mutex);
ov2740_update_pad_format(mode, &fmt->format);
- if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
- *v4l2_subdev_get_try_format(sd, sd_state, fmt->pad) = fmt->format;
- } else {
- ov2740->cur_mode = mode;
- __v4l2_ctrl_s_ctrl(ov2740->link_freq, mode->link_freq_index);
- __v4l2_ctrl_s_ctrl_int64(ov2740->pixel_rate,
- to_pixel_rate(mode->link_freq_index));
-
- /* Update limits and set FPS to default */
- vblank_def = mode->vts_def - mode->height;
- __v4l2_ctrl_modify_range(ov2740->vblank,
- mode->vts_min - mode->height,
- OV2740_VTS_MAX - mode->height, 1,
- vblank_def);
- __v4l2_ctrl_s_ctrl(ov2740->vblank, vblank_def);
- h_blank = to_pixels_per_line(mode->hts, mode->link_freq_index) -
- mode->width;
- __v4l2_ctrl_modify_range(ov2740->hblank, h_blank, h_blank, 1,
- h_blank);
- }
- mutex_unlock(&ov2740->mutex);
-
- return 0;
-}
+ *v4l2_subdev_get_pad_format(sd, sd_state, fmt->pad) = fmt->format;
-static int ov2740_get_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_format *fmt)
-{
- struct ov2740 *ov2740 = to_ov2740(sd);
-
- mutex_lock(&ov2740->mutex);
if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
- fmt->format = *v4l2_subdev_get_try_format(&ov2740->sd,
- sd_state,
- fmt->pad);
- else
- ov2740_update_pad_format(ov2740->cur_mode, &fmt->format);
+ return 0;
- mutex_unlock(&ov2740->mutex);
+ ov2740->cur_mode = mode;
+ __v4l2_ctrl_s_ctrl(ov2740->link_freq, mode->link_freq_index);
+ __v4l2_ctrl_s_ctrl_int64(ov2740->pixel_rate,
+ to_pixel_rate(mode->link_freq_index));
+
+ /* Update limits and set FPS to default */
+ vblank_def = mode->vts_def - mode->height;
+ __v4l2_ctrl_modify_range(ov2740->vblank,
+ mode->vts_min - mode->height,
+ OV2740_VTS_MAX - mode->height, 1, vblank_def);
+ __v4l2_ctrl_s_ctrl(ov2740->vblank, vblank_def);
+ h_blank = to_pixels_per_line(mode->hts, mode->link_freq_index) -
+ mode->width;
+ __v4l2_ctrl_modify_range(ov2740->hblank, h_blank, h_blank, 1, h_blank);
return 0;
}
@@ -946,14 +924,11 @@ static int ov2740_enum_frame_size(struct v4l2_subdev *sd,
return 0;
}
-static int ov2740_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+static int ov2740_init_cfg(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state)
{
- struct ov2740 *ov2740 = to_ov2740(sd);
-
- mutex_lock(&ov2740->mutex);
ov2740_update_pad_format(&supported_modes[0],
- v4l2_subdev_get_try_format(sd, fh->state, 0));
- mutex_unlock(&ov2740->mutex);
+ v4l2_subdev_get_pad_format(sd, sd_state, 0));
return 0;
}
@@ -963,10 +938,11 @@ static const struct v4l2_subdev_video_ops ov2740_video_ops = {
};
static const struct v4l2_subdev_pad_ops ov2740_pad_ops = {
+ .get_fmt = v4l2_subdev_get_fmt,
.set_fmt = ov2740_set_format,
- .get_fmt = ov2740_get_format,
.enum_mbus_code = ov2740_enum_mbus_code,
.enum_frame_size = ov2740_enum_frame_size,
+ .init_cfg = ov2740_init_cfg,
};
static const struct v4l2_subdev_ops ov2740_subdev_ops = {
@@ -978,10 +954,6 @@ static const struct media_entity_operations ov2740_subdev_entity_ops = {
.link_validate = v4l2_subdev_link_validate,
};
-static const struct v4l2_subdev_internal_ops ov2740_internal_ops = {
- .open = ov2740_open,
-};
-
static int ov2740_check_hwcfg(struct device *dev)
{
struct fwnode_handle *ep;
@@ -1047,13 +1019,12 @@ static int ov2740_check_hwcfg(struct device *dev)
static void ov2740_remove(struct i2c_client *client)
{
struct v4l2_subdev *sd = i2c_get_clientdata(client);
- struct ov2740 *ov2740 = to_ov2740(sd);
v4l2_async_unregister_subdev(sd);
media_entity_cleanup(&sd->entity);
+ v4l2_subdev_cleanup(sd);
v4l2_ctrl_handler_free(sd->ctrl_handler);
pm_runtime_disable(&client->dev);
- mutex_destroy(&ov2740->mutex);
}
static int ov2740_nvmem_read(void *priv, unsigned int off, void *val,
@@ -1062,9 +1033,11 @@ static int ov2740_nvmem_read(void *priv, unsigned int off, void *val,
struct nvm_data *nvm = priv;
struct device *dev = regmap_get_device(nvm->regmap);
struct ov2740 *ov2740 = to_ov2740(dev_get_drvdata(dev));
+ struct v4l2_subdev_state *sd_state;
int ret = 0;
- mutex_lock(&ov2740->mutex);
+ /* Serialise sensor access */
+ sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
if (nvm->nvm_buffer) {
memcpy(val, nvm->nvm_buffer + off, count);
@@ -1082,7 +1055,7 @@ static int ov2740_nvmem_read(void *priv, unsigned int off, void *val,
pm_runtime_put(dev);
exit:
- mutex_unlock(&ov2740->mutex);
+ v4l2_subdev_unlock_state(sd_state);
return ret;
}
@@ -1153,7 +1126,6 @@ static int ov2740_probe(struct i2c_client *client)
return dev_err_probe(dev, ret, "failed to find sensor\n");
}
- mutex_init(&ov2740->mutex);
ov2740->cur_mode = &supported_modes[0];
ret = ov2740_init_controls(ov2740);
if (ret) {
@@ -1161,7 +1133,7 @@ static int ov2740_probe(struct i2c_client *client)
goto probe_error_v4l2_ctrl_handler_free;
}
- ov2740->sd.internal_ops = &ov2740_internal_ops;
+ ov2740->sd.state_lock = ov2740->ctrl_handler.lock;
ov2740->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
ov2740->sd.entity.ops = &ov2740_subdev_entity_ops;
ov2740->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
@@ -1172,6 +1144,10 @@ static int ov2740_probe(struct i2c_client *client)
goto probe_error_v4l2_ctrl_handler_free;
}
+ ret = v4l2_subdev_init_finalize(&ov2740->sd);
+ if (ret)
+ goto probe_error_media_entity_cleanup;
+
/* Set the device's state to active if it's in D0 state. */
if (full_power)
pm_runtime_set_active(&client->dev);
@@ -1181,7 +1157,7 @@ static int ov2740_probe(struct i2c_client *client)
ret = v4l2_async_register_subdev_sensor(&ov2740->sd);
if (ret < 0) {
dev_err_probe(dev, ret, "failed to register V4L2 subdev\n");
- goto probe_error_media_entity_cleanup;
+ goto probe_error_v4l2_subdev_cleanup;
}
ret = ov2740_register_nvmem(client, ov2740);
@@ -1190,6 +1166,9 @@ static int ov2740_probe(struct i2c_client *client)
return 0;
+probe_error_v4l2_subdev_cleanup:
+ v4l2_subdev_cleanup(&ov2740->sd);
+
probe_error_media_entity_cleanup:
media_entity_cleanup(&ov2740->sd.entity);
pm_runtime_disable(&client->dev);
@@ -1197,7 +1176,6 @@ static int ov2740_probe(struct i2c_client *client)
probe_error_v4l2_ctrl_handler_free:
v4l2_ctrl_handler_free(ov2740->sd.ctrl_handler);
- mutex_destroy(&ov2740->mutex);
return ret;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v2 09/12] media: ov2740: Return -EPROBE_DEFER if no endpoint is found
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
` (7 preceding siblings ...)
2023-09-18 12:51 ` [PATCH v2 08/12] media: ov2740: Use sub-device active state Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 13:24 ` Laurent Pinchart
2023-09-18 12:51 ` [PATCH v2 10/12] media: v4l: subdev: Clear frame descriptor before get_frame_desc Sakari Ailus
` (2 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
With ipu bridge, endpoints may only be created when ipu bridge has
initialised. This may happen after the sensor driver has first probed.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/i2c/ov2740.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 2c00e653ec47..ccbb15e730ae 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -976,7 +976,7 @@ static int ov2740_check_hwcfg(struct device *dev)
ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
if (!ep)
- return -ENXIO;
+ return -EPROBE_DEFER;
ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
fwnode_handle_put(ep);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 09/12] media: ov2740: Return -EPROBE_DEFER if no endpoint is found
2023-09-18 12:51 ` [PATCH v2 09/12] media: ov2740: Return -EPROBE_DEFER if no endpoint is found Sakari Ailus
@ 2023-09-18 13:24 ` Laurent Pinchart
2023-09-19 10:11 ` Sakari Ailus
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2023-09-18 13:24 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Sakari,
Thank you for the patch.
On Mon, Sep 18, 2023 at 03:51:35PM +0300, Sakari Ailus wrote:
> With ipu bridge, endpoints may only be created when ipu bridge has
> initialised. This may happen after the sensor driver has first probed.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/i2c/ov2740.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index 2c00e653ec47..ccbb15e730ae 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -976,7 +976,7 @@ static int ov2740_check_hwcfg(struct device *dev)
>
> ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> if (!ep)
> - return -ENXIO;
> + return -EPROBE_DEFER;
This is fine as a quick fix, so
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
but I'm thinking that we need better in the long run, for multiple
reasons:
- All sensor drivers that support ACPI should do the same, as they
shouldn't be aware of platform-specific integration details. This
could be done by mass-patching them, with a documentation update to
clearly indicate what error code is appropriate. Another option could
be to return an error pointer from fwnode_graph_get_next_endpoint(),
with the appropriate error being set depending on the platform. Other
solutions may be possible too.
- On OF systems, a DT error will result in probe deferral, which is
harder to debug than probe failures. It would be nice to avoid that.
>
> ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> fwnode_handle_put(ep);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 09/12] media: ov2740: Return -EPROBE_DEFER if no endpoint is found
2023-09-18 13:24 ` Laurent Pinchart
@ 2023-09-19 10:11 ` Sakari Ailus
0 siblings, 0 replies; 28+ messages in thread
From: Sakari Ailus @ 2023-09-19 10:11 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Laurent,
On Mon, Sep 18, 2023 at 04:24:55PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Mon, Sep 18, 2023 at 03:51:35PM +0300, Sakari Ailus wrote:
> > With ipu bridge, endpoints may only be created when ipu bridge has
> > initialised. This may happen after the sensor driver has first probed.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/i2c/ov2740.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > index 2c00e653ec47..ccbb15e730ae 100644
> > --- a/drivers/media/i2c/ov2740.c
> > +++ b/drivers/media/i2c/ov2740.c
> > @@ -976,7 +976,7 @@ static int ov2740_check_hwcfg(struct device *dev)
> >
> > ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > if (!ep)
> > - return -ENXIO;
> > + return -EPROBE_DEFER;
>
> This is fine as a quick fix, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Thank you.
>
> but I'm thinking that we need better in the long run, for multiple
> reasons:
>
> - All sensor drivers that support ACPI should do the same, as they
> shouldn't be aware of platform-specific integration details. This
> could be done by mass-patching them, with a documentation update to
> clearly indicate what error code is appropriate. Another option could
> be to return an error pointer from fwnode_graph_get_next_endpoint(),
> with the appropriate error being set depending on the platform. Other
> solutions may be possible too.
I'd like to see IPU bridge in the ACPI framework but that won't happen
overnight. Then this issue would disappear. The problem with the error is
that callers don't expect it and how do you tell if you're expecting
something you don't know about to appear.
>
> - On OF systems, a DT error will result in probe deferral, which is
> harder to debug than probe failures. It would be nice to avoid that.
Yes.
I change it for this sensor only as I was otherwise working on it.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 10/12] media: v4l: subdev: Clear frame descriptor before get_frame_desc
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
` (8 preceding siblings ...)
2023-09-18 12:51 ` [PATCH v2 09/12] media: ov2740: Return -EPROBE_DEFER if no endpoint is found Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 13:30 ` Laurent Pinchart
2023-09-19 8:04 ` Tomi Valkeinen
2023-09-18 12:51 ` [PATCH v2 11/12] media: v4l: subdev: Print debug information on frame descriptor Sakari Ailus
2023-09-18 12:51 ` [PATCH v2 12/12] media: mc: Check pad flag validity Sakari Ailus
11 siblings, 2 replies; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Clear frame descriptor before calling transmitter's get_frame_desc() op.
Also remove the corresponding memset() calls from drivers.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/i2c/ds90ub913.c | 2 --
drivers/media/i2c/ds90ub953.c | 2 --
drivers/media/i2c/ds90ub960.c | 2 --
drivers/media/platform/nxp/imx-mipi-csis.c | 2 --
drivers/media/v4l2-core/v4l2-subdev.c | 9 +++++++++
5 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index 4bfa3b3cf619..8e9ebed09f64 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -362,8 +362,6 @@ static int ub913_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
if (ret)
return ret;
- memset(fd, 0, sizeof(*fd));
-
fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL;
state = v4l2_subdev_lock_and_get_active_state(sd);
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index dc394e22a42c..644022312833 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -499,8 +499,6 @@ static int ub953_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
if (ret)
return ret;
- memset(fd, 0, sizeof(*fd));
-
fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
state = v4l2_subdev_lock_and_get_active_state(sd);
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 8ba5750f5a23..b8f3e5ca03ef 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -2786,8 +2786,6 @@ static int ub960_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
if (!ub960_pad_is_source(priv, pad))
return -EINVAL;
- memset(fd, 0, sizeof(*fd));
-
fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
state = v4l2_subdev_lock_and_get_active_state(&priv->sd);
diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 16f19a640130..aac9cffe503c 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -1114,8 +1114,6 @@ static int mipi_csis_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL;
fd->num_entries = 1;
- memset(entry, 0, sizeof(*entry));
-
entry->flags = 0;
entry->pixelcode = csis_fmt->code;
entry->bus.csi2.vc = 0;
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index b92348ad61f6..7b087be3ff4f 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -306,6 +306,14 @@ static int call_set_selection(struct v4l2_subdev *sd,
sd->ops->pad->set_selection(sd, state, sel);
}
+static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+ struct v4l2_mbus_frame_desc *fd)
+{
+ memset(fd, 0, sizeof(*fd));
+
+ return sd->ops->pad->get_frame_desc(sd, pad, fd);
+}
+
static inline int check_edid(struct v4l2_subdev *sd,
struct v4l2_subdev_edid *edid)
{
@@ -431,6 +439,7 @@ static const struct v4l2_subdev_pad_ops v4l2_subdev_call_pad_wrappers = {
.set_edid = call_set_edid,
.dv_timings_cap = call_dv_timings_cap,
.enum_dv_timings = call_enum_dv_timings,
+ .get_frame_desc = call_get_frame_desc,
.get_mbus_config = call_get_mbus_config,
};
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 10/12] media: v4l: subdev: Clear frame descriptor before get_frame_desc
2023-09-18 12:51 ` [PATCH v2 10/12] media: v4l: subdev: Clear frame descriptor before get_frame_desc Sakari Ailus
@ 2023-09-18 13:30 ` Laurent Pinchart
2023-09-19 8:04 ` Tomi Valkeinen
1 sibling, 0 replies; 28+ messages in thread
From: Laurent Pinchart @ 2023-09-18 13:30 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Sakari,
Thank you for the patch.
On Mon, Sep 18, 2023 at 03:51:36PM +0300, Sakari Ailus wrote:
> Clear frame descriptor before calling transmitter's get_frame_desc() op.
> Also remove the corresponding memset() calls from drivers.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/i2c/ds90ub913.c | 2 --
> drivers/media/i2c/ds90ub953.c | 2 --
> drivers/media/i2c/ds90ub960.c | 2 --
> drivers/media/platform/nxp/imx-mipi-csis.c | 2 --
> drivers/media/v4l2-core/v4l2-subdev.c | 9 +++++++++
> 5 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
> index 4bfa3b3cf619..8e9ebed09f64 100644
> --- a/drivers/media/i2c/ds90ub913.c
> +++ b/drivers/media/i2c/ds90ub913.c
> @@ -362,8 +362,6 @@ static int ub913_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> if (ret)
> return ret;
>
> - memset(fd, 0, sizeof(*fd));
> -
> fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL;
>
> state = v4l2_subdev_lock_and_get_active_state(sd);
> diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
> index dc394e22a42c..644022312833 100644
> --- a/drivers/media/i2c/ds90ub953.c
> +++ b/drivers/media/i2c/ds90ub953.c
> @@ -499,8 +499,6 @@ static int ub953_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> if (ret)
> return ret;
>
> - memset(fd, 0, sizeof(*fd));
> -
> fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
>
> state = v4l2_subdev_lock_and_get_active_state(sd);
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index 8ba5750f5a23..b8f3e5ca03ef 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -2786,8 +2786,6 @@ static int ub960_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> if (!ub960_pad_is_source(priv, pad))
> return -EINVAL;
>
> - memset(fd, 0, sizeof(*fd));
> -
> fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
>
> state = v4l2_subdev_lock_and_get_active_state(&priv->sd);
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 16f19a640130..aac9cffe503c 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -1114,8 +1114,6 @@ static int mipi_csis_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL;
> fd->num_entries = 1;
>
> - memset(entry, 0, sizeof(*entry));
> -
> entry->flags = 0;
> entry->pixelcode = csis_fmt->code;
> entry->bus.csi2.vc = 0;
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index b92348ad61f6..7b087be3ff4f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -306,6 +306,14 @@ static int call_set_selection(struct v4l2_subdev *sd,
> sd->ops->pad->set_selection(sd, state, sel);
> }
>
> +static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> + struct v4l2_mbus_frame_desc *fd)
> +{
> + memset(fd, 0, sizeof(*fd));
> +
> + return sd->ops->pad->get_frame_desc(sd, pad, fd);
> +}
I like the operation wrappers.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> static inline int check_edid(struct v4l2_subdev *sd,
> struct v4l2_subdev_edid *edid)
> {
> @@ -431,6 +439,7 @@ static const struct v4l2_subdev_pad_ops v4l2_subdev_call_pad_wrappers = {
> .set_edid = call_set_edid,
> .dv_timings_cap = call_dv_timings_cap,
> .enum_dv_timings = call_enum_dv_timings,
> + .get_frame_desc = call_get_frame_desc,
> .get_mbus_config = call_get_mbus_config,
> };
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 10/12] media: v4l: subdev: Clear frame descriptor before get_frame_desc
2023-09-18 12:51 ` [PATCH v2 10/12] media: v4l: subdev: Clear frame descriptor before get_frame_desc Sakari Ailus
2023-09-18 13:30 ` Laurent Pinchart
@ 2023-09-19 8:04 ` Tomi Valkeinen
1 sibling, 0 replies; 28+ messages in thread
From: Tomi Valkeinen @ 2023-09-19 8:04 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Jacopo Mondi,
Rui Miguel Silva, Martin Kepplinger
On 18/09/2023 15:51, Sakari Ailus wrote:
> Clear frame descriptor before calling transmitter's get_frame_desc() op.
> Also remove the corresponding memset() calls from drivers.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/i2c/ds90ub913.c | 2 --
> drivers/media/i2c/ds90ub953.c | 2 --
> drivers/media/i2c/ds90ub960.c | 2 --
> drivers/media/platform/nxp/imx-mipi-csis.c | 2 --
> drivers/media/v4l2-core/v4l2-subdev.c | 9 +++++++++
> 5 files changed, 9 insertions(+), 8 deletions(-)
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tomi
> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
> index 4bfa3b3cf619..8e9ebed09f64 100644
> --- a/drivers/media/i2c/ds90ub913.c
> +++ b/drivers/media/i2c/ds90ub913.c
> @@ -362,8 +362,6 @@ static int ub913_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> if (ret)
> return ret;
>
> - memset(fd, 0, sizeof(*fd));
> -
> fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL;
>
> state = v4l2_subdev_lock_and_get_active_state(sd);
> diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
> index dc394e22a42c..644022312833 100644
> --- a/drivers/media/i2c/ds90ub953.c
> +++ b/drivers/media/i2c/ds90ub953.c
> @@ -499,8 +499,6 @@ static int ub953_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> if (ret)
> return ret;
>
> - memset(fd, 0, sizeof(*fd));
> -
> fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
>
> state = v4l2_subdev_lock_and_get_active_state(sd);
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index 8ba5750f5a23..b8f3e5ca03ef 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -2786,8 +2786,6 @@ static int ub960_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> if (!ub960_pad_is_source(priv, pad))
> return -EINVAL;
>
> - memset(fd, 0, sizeof(*fd));
> -
> fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
>
> state = v4l2_subdev_lock_and_get_active_state(&priv->sd);
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 16f19a640130..aac9cffe503c 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -1114,8 +1114,6 @@ static int mipi_csis_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL;
> fd->num_entries = 1;
>
> - memset(entry, 0, sizeof(*entry));
> -
> entry->flags = 0;
> entry->pixelcode = csis_fmt->code;
> entry->bus.csi2.vc = 0;
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index b92348ad61f6..7b087be3ff4f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -306,6 +306,14 @@ static int call_set_selection(struct v4l2_subdev *sd,
> sd->ops->pad->set_selection(sd, state, sel);
> }
>
> +static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> + struct v4l2_mbus_frame_desc *fd)
> +{
> + memset(fd, 0, sizeof(*fd));
> +
> + return sd->ops->pad->get_frame_desc(sd, pad, fd);
> +}
> +
> static inline int check_edid(struct v4l2_subdev *sd,
> struct v4l2_subdev_edid *edid)
> {
> @@ -431,6 +439,7 @@ static const struct v4l2_subdev_pad_ops v4l2_subdev_call_pad_wrappers = {
> .set_edid = call_set_edid,
> .dv_timings_cap = call_dv_timings_cap,
> .enum_dv_timings = call_enum_dv_timings,
> + .get_frame_desc = call_get_frame_desc,
> .get_mbus_config = call_get_mbus_config,
> };
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 11/12] media: v4l: subdev: Print debug information on frame descriptor
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
` (9 preceding siblings ...)
2023-09-18 12:51 ` [PATCH v2 10/12] media: v4l: subdev: Clear frame descriptor before get_frame_desc Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 13:39 ` Laurent Pinchart
2023-09-18 12:51 ` [PATCH v2 12/12] media: mc: Check pad flag validity Sakari Ailus
11 siblings, 1 reply; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Print debug level information on returned frame descriptors.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 35 ++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 7b087be3ff4f..abd9275febdb 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -309,9 +309,42 @@ static int call_set_selection(struct v4l2_subdev *sd,
static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
struct v4l2_mbus_frame_desc *fd)
{
+ unsigned int i;
+ int ret;
+
memset(fd, 0, sizeof(*fd));
- return sd->ops->pad->get_frame_desc(sd, pad, fd);
+ ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
+ if (ret)
+ return ret;
+
+ dev_dbg(sd->dev, "Frame descriptor\n");
+ dev_dbg(sd->dev, "\ttype %s\n",
+ fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
+ fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
+ "unknown");
+ dev_dbg(sd->dev, "\tentries %u\n", fd->num_entries);
+
+ for (i = 0; i < fd->num_entries; i++) {
+ struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
+
+ dev_dbg(sd->dev, "\tentry %u\n", i);
+ dev_dbg(sd->dev, "\tflags%s%s\n",
+ entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
+ " LEN_MAX" : "",
+ entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
+ " BLOB" : "");
+ dev_dbg(sd->dev, "\t\tstream %u\n", entry->stream);
+ dev_dbg(sd->dev, "\t\tpixelcode 0x%4.4x\n", entry->pixelcode);
+ dev_dbg(sd->dev, "\t\tlength %u\n", entry->length);
+
+ if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
+ dev_dbg(sd->dev, "\t\tvc %u\n", entry->bus.csi2.vc);
+ dev_dbg(sd->dev, "\t\tdt 0x%2.2x\n", entry->bus.csi2.dt);
+ }
+ }
+
+ return 0;
}
static inline int check_edid(struct v4l2_subdev *sd,
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 11/12] media: v4l: subdev: Print debug information on frame descriptor
2023-09-18 12:51 ` [PATCH v2 11/12] media: v4l: subdev: Print debug information on frame descriptor Sakari Ailus
@ 2023-09-18 13:39 ` Laurent Pinchart
2023-09-19 8:19 ` Tomi Valkeinen
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2023-09-18 13:39 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Sakari,
Thank you for the patch.
On Mon, Sep 18, 2023 at 03:51:37PM +0300, Sakari Ailus wrote:
> Print debug level information on returned frame descriptors.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 35 ++++++++++++++++++++++++++-
> 1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 7b087be3ff4f..abd9275febdb 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -309,9 +309,42 @@ static int call_set_selection(struct v4l2_subdev *sd,
> static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> struct v4l2_mbus_frame_desc *fd)
> {
> + unsigned int i;
> + int ret;
> +
> memset(fd, 0, sizeof(*fd));
>
> - return sd->ops->pad->get_frame_desc(sd, pad, fd);
> + ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> + if (ret)
> + return ret;
> +
> + dev_dbg(sd->dev, "Frame descriptor\n");
> + dev_dbg(sd->dev, "\ttype %s\n",
> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> + "unknown");
> + dev_dbg(sd->dev, "\tentries %u\n", fd->num_entries);
You could skip this line, it's implied by the entries that you print
below.
> +
> + for (i = 0; i < fd->num_entries; i++) {
> + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> +
> + dev_dbg(sd->dev, "\tentry %u\n", i);
> + dev_dbg(sd->dev, "\tflags%s%s\n",
> + entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
> + " LEN_MAX" : "",
> + entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
> + " BLOB" : "");
> + dev_dbg(sd->dev, "\t\tstream %u\n", entry->stream);
> + dev_dbg(sd->dev, "\t\tpixelcode 0x%4.4x\n", entry->pixelcode);
> + dev_dbg(sd->dev, "\t\tlength %u\n", entry->length);
> +
> + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> + dev_dbg(sd->dev, "\t\tvc %u\n", entry->bus.csi2.vc);
> + dev_dbg(sd->dev, "\t\tdt 0x%2.2x\n", entry->bus.csi2.dt);
I'd merge those two in a single line.
> + }
> + }
All this is a bit verbose. If it was in a hot path I would be annoyed,
but in this case I suppose it can be useful for debugging and won't
affect runtime too much.
It would be nice if we could have a single check and return early. That
should be possible by using DEFINE_DYNAMIC_DEBUG_METADATA() and
DYNAMIC_DEBUG_BRANCH(), like done in alloc_contig_dump_pages() for
instance. It has the additional upside of being able to control all the
messages with a single flag. I'm not sure it's worth it though, I'll let
you decide.
> +
> + return 0;
> }
>
> static inline int check_edid(struct v4l2_subdev *sd,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 11/12] media: v4l: subdev: Print debug information on frame descriptor
2023-09-18 13:39 ` Laurent Pinchart
@ 2023-09-19 8:19 ` Tomi Valkeinen
2023-09-19 10:21 ` Sakari Ailus
0 siblings, 1 reply; 28+ messages in thread
From: Tomi Valkeinen @ 2023-09-19 8:19 UTC (permalink / raw)
To: Laurent Pinchart, Sakari Ailus
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Jacopo Mondi,
Rui Miguel Silva, Martin Kepplinger
On 18/09/2023 16:39, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Mon, Sep 18, 2023 at 03:51:37PM +0300, Sakari Ailus wrote:
>> Print debug level information on returned frame descriptors.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>> drivers/media/v4l2-core/v4l2-subdev.c | 35 ++++++++++++++++++++++++++-
>> 1 file changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 7b087be3ff4f..abd9275febdb 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -309,9 +309,42 @@ static int call_set_selection(struct v4l2_subdev *sd,
>> static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>> struct v4l2_mbus_frame_desc *fd)
>> {
>> + unsigned int i;
>> + int ret;
>> +
>> memset(fd, 0, sizeof(*fd));
>>
>> - return sd->ops->pad->get_frame_desc(sd, pad, fd);
>> + ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
>> + if (ret)
>> + return ret;
>> +
>> + dev_dbg(sd->dev, "Frame descriptor\n");
>> + dev_dbg(sd->dev, "\ttype %s\n",
>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
>> + "unknown");
>> + dev_dbg(sd->dev, "\tentries %u\n", fd->num_entries);
>
> You could skip this line, it's implied by the entries that you print
> below.
>
>> +
>> + for (i = 0; i < fd->num_entries; i++) {
>> + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
>> +
>> + dev_dbg(sd->dev, "\tentry %u\n", i);
>> + dev_dbg(sd->dev, "\tflags%s%s\n",
>> + entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
>> + " LEN_MAX" : "",
>> + entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
>> + " BLOB" : "");
>> + dev_dbg(sd->dev, "\t\tstream %u\n", entry->stream);
>> + dev_dbg(sd->dev, "\t\tpixelcode 0x%4.4x\n", entry->pixelcode);
>> + dev_dbg(sd->dev, "\t\tlength %u\n", entry->length);
>> +
>> + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
>> + dev_dbg(sd->dev, "\t\tvc %u\n", entry->bus.csi2.vc);
>> + dev_dbg(sd->dev, "\t\tdt 0x%2.2x\n", entry->bus.csi2.dt);
>
> I'd merge those two in a single line.
>
>> + }
>> + }
>
> All this is a bit verbose. If it was in a hot path I would be annoyed,
> but in this case I suppose it can be useful for debugging and won't
> affect runtime too much.
>
> It would be nice if we could have a single check and return early. That
> should be possible by using DEFINE_DYNAMIC_DEBUG_METADATA() and
> DYNAMIC_DEBUG_BRANCH(), like done in alloc_contig_dump_pages() for
> instance. It has the additional upside of being able to control all the
> messages with a single flag. I'm not sure it's worth it though, I'll let
> you decide.
Yes, this is very spammy. Maybe instead of:
Frame descriptor
type CSI-2
entries 2
entry 0
flags LEN_MAX
stream 0
pixelcode 0x3012
length 3194400
vc 0
dt 0x2c
entry 1
flags LEN_MAX
stream 1
pixelcode 0x8003
length 2904
vc 0
dt 0x12
Frame descriptor type CSI-2:
pad 0 stream 0 code 0x3012 len 3194400 vc 0 dt 0x2c LEN_MAX
pad 0 stream 1 code 0x8003 len 2904 vc 0 dt 0x12 LEN_MAX
(note: pad is missing in your patch)
Tomi
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 11/12] media: v4l: subdev: Print debug information on frame descriptor
2023-09-19 8:19 ` Tomi Valkeinen
@ 2023-09-19 10:21 ` Sakari Ailus
0 siblings, 0 replies; 28+ messages in thread
From: Sakari Ailus @ 2023-09-19 10:21 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Laurent Pinchart, linux-media, Tianshu Qiu, Bingbu Cao,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Tomi, Laurent,
Thanks for the review.
On Tue, Sep 19, 2023 at 11:19:36AM +0300, Tomi Valkeinen wrote:
> On 18/09/2023 16:39, Laurent Pinchart wrote:
> > Hi Sakari,
> >
> > Thank you for the patch.
> >
> > On Mon, Sep 18, 2023 at 03:51:37PM +0300, Sakari Ailus wrote:
> > > Print debug level information on returned frame descriptors.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > drivers/media/v4l2-core/v4l2-subdev.c | 35 ++++++++++++++++++++++++++-
> > > 1 file changed, 34 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 7b087be3ff4f..abd9275febdb 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -309,9 +309,42 @@ static int call_set_selection(struct v4l2_subdev *sd,
> > > static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > > struct v4l2_mbus_frame_desc *fd)
> > > {
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > memset(fd, 0, sizeof(*fd));
> > > - return sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > + ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + dev_dbg(sd->dev, "Frame descriptor\n");
> > > + dev_dbg(sd->dev, "\ttype %s\n",
> > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> > > + "unknown");
> > > + dev_dbg(sd->dev, "\tentries %u\n", fd->num_entries);
> >
> > You could skip this line, it's implied by the entries that you print
> > below.
I guess that's reasonable if I switch to the format Tomi proposed. You'd
only need to count lines.
> >
> > > +
> > > + for (i = 0; i < fd->num_entries; i++) {
> > > + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> > > +
> > > + dev_dbg(sd->dev, "\tentry %u\n", i);
> > > + dev_dbg(sd->dev, "\tflags%s%s\n",
> > > + entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
> > > + " LEN_MAX" : "",
> > > + entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
> > > + " BLOB" : "");
> > > + dev_dbg(sd->dev, "\t\tstream %u\n", entry->stream);
> > > + dev_dbg(sd->dev, "\t\tpixelcode 0x%4.4x\n", entry->pixelcode);
> > > + dev_dbg(sd->dev, "\t\tlength %u\n", entry->length);
> > > +
> > > + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> > > + dev_dbg(sd->dev, "\t\tvc %u\n", entry->bus.csi2.vc);
> > > + dev_dbg(sd->dev, "\t\tdt 0x%2.2x\n", entry->bus.csi2.dt);
> >
> > I'd merge those two in a single line.
> >
> > > + }
> > > + }
> >
> > All this is a bit verbose. If it was in a hot path I would be annoyed,
> > but in this case I suppose it can be useful for debugging and won't
> > affect runtime too much.
> >
> > It would be nice if we could have a single check and return early. That
> > should be possible by using DEFINE_DYNAMIC_DEBUG_METADATA() and
> > DYNAMIC_DEBUG_BRANCH(), like done in alloc_contig_dump_pages() for
> > instance. It has the additional upside of being able to control all the
> > messages with a single flag. I'm not sure it's worth it though, I'll let
> > you decide.
>
> Yes, this is very spammy. Maybe instead of:
>
> Frame descriptor
> type CSI-2
> entries 2
> entry 0
> flags LEN_MAX
> stream 0
> pixelcode 0x3012
> length 3194400
> vc 0
> dt 0x2c
> entry 1
> flags LEN_MAX
> stream 1
> pixelcode 0x8003
> length 2904
> vc 0
> dt 0x12
>
>
> Frame descriptor type CSI-2:
> pad 0 stream 0 code 0x3012 len 3194400 vc 0 dt 0x2c LEN_MAX
> pad 0 stream 1 code 0x8003 len 2904 vc 0 dt 0x12 LEN_MAX
>
> (note: pad is missing in your patch)
It's indeed much more condensed but still contains all the relevant
information. I'll switch to this (or something alike).
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 12/12] media: mc: Check pad flag validity
2023-09-18 12:51 [PATCH v2 00/12] Small fixes and cleanups (ov2740 and ccs) Sakari Ailus
` (10 preceding siblings ...)
2023-09-18 12:51 ` [PATCH v2 11/12] media: v4l: subdev: Print debug information on frame descriptor Sakari Ailus
@ 2023-09-18 12:51 ` Sakari Ailus
2023-09-18 13:48 ` Laurent Pinchart
11 siblings, 1 reply; 28+ messages in thread
From: Sakari Ailus @ 2023-09-18 12:51 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Check the validity of pad flags on entity init. Exactly one of the flags
must be set.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/mc/mc-entity.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 83468d4a440b..38d5bbae33d7 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -195,8 +195,9 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
struct media_pad *pads)
{
struct media_device *mdev = entity->graph_obj.mdev;
- struct media_pad *iter;
+ struct media_pad *iter, *iter2;
unsigned int i = 0;
+ int ret = 0;
if (num_pads >= MEDIA_ENTITY_MAX_PADS)
return -E2BIG;
@@ -210,15 +211,30 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
media_entity_for_each_pad(entity, iter) {
iter->entity = entity;
iter->index = i++;
+
+ if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK |
+ MEDIA_PAD_FL_SOURCE)) != 1) {
+ ret = -EINVAL;
+ break;
+ }
+
if (mdev)
media_gobj_create(mdev, MEDIA_GRAPH_PAD,
&iter->graph_obj);
}
+ if (ret && mdev) {
+ media_entity_for_each_pad(entity, iter2) {
+ if (iter2 == iter)
+ break;
+ media_gobj_destroy(&iter->graph_obj);
+ }
+ }
+
if (mdev)
mutex_unlock(&mdev->graph_mutex);
- return 0;
+ return ret;
}
EXPORT_SYMBOL_GPL(media_entity_pads_init);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 12/12] media: mc: Check pad flag validity
2023-09-18 12:51 ` [PATCH v2 12/12] media: mc: Check pad flag validity Sakari Ailus
@ 2023-09-18 13:48 ` Laurent Pinchart
2023-09-19 10:24 ` Sakari Ailus
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2023-09-18 13:48 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Sakari,
Thank you for the patch.
On Mon, Sep 18, 2023 at 03:51:38PM +0300, Sakari Ailus wrote:
> Check the validity of pad flags on entity init. Exactly one of the flags
> must be set.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/mc/mc-entity.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 83468d4a440b..38d5bbae33d7 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -195,8 +195,9 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> struct media_pad *pads)
> {
> struct media_device *mdev = entity->graph_obj.mdev;
> - struct media_pad *iter;
> + struct media_pad *iter, *iter2;
> unsigned int i = 0;
> + int ret = 0;
>
> if (num_pads >= MEDIA_ENTITY_MAX_PADS)
> return -E2BIG;
> @@ -210,15 +211,30 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> media_entity_for_each_pad(entity, iter) {
> iter->entity = entity;
> iter->index = i++;
> +
> + if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK |
> + MEDIA_PAD_FL_SOURCE)) != 1) {
> + ret = -EINVAL;
> + break;
> + }
> +
> if (mdev)
> media_gobj_create(mdev, MEDIA_GRAPH_PAD,
> &iter->graph_obj);
> }
>
> + if (ret && mdev) {
> + media_entity_for_each_pad(entity, iter2) {
> + if (iter2 == iter)
> + break;
> + media_gobj_destroy(&iter->graph_obj);
Wrong iterator.
Instead of using a second iterator, which can be error-prone as shown
here, how about breaking when !iter->graph_obj.mdev, which indicates an
uninitialized object ?
> + }
> + }
> +
> if (mdev)
> mutex_unlock(&mdev->graph_mutex);
>
> - return 0;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(media_entity_pads_init);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 12/12] media: mc: Check pad flag validity
2023-09-18 13:48 ` Laurent Pinchart
@ 2023-09-19 10:24 ` Sakari Ailus
2023-09-19 10:37 ` Laurent Pinchart
0 siblings, 1 reply; 28+ messages in thread
From: Sakari Ailus @ 2023-09-19 10:24 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
Hi Laurent,
Thanks for the review.
On Mon, Sep 18, 2023 at 04:48:02PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Mon, Sep 18, 2023 at 03:51:38PM +0300, Sakari Ailus wrote:
> > Check the validity of pad flags on entity init. Exactly one of the flags
> > must be set.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/mc/mc-entity.c | 20 ++++++++++++++++++--
> > 1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index 83468d4a440b..38d5bbae33d7 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -195,8 +195,9 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> > struct media_pad *pads)
> > {
> > struct media_device *mdev = entity->graph_obj.mdev;
> > - struct media_pad *iter;
> > + struct media_pad *iter, *iter2;
> > unsigned int i = 0;
> > + int ret = 0;
> >
> > if (num_pads >= MEDIA_ENTITY_MAX_PADS)
> > return -E2BIG;
> > @@ -210,15 +211,30 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> > media_entity_for_each_pad(entity, iter) {
> > iter->entity = entity;
> > iter->index = i++;
> > +
> > + if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK |
> > + MEDIA_PAD_FL_SOURCE)) != 1) {
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > if (mdev)
> > media_gobj_create(mdev, MEDIA_GRAPH_PAD,
> > &iter->graph_obj);
> > }
> >
> > + if (ret && mdev) {
> > + media_entity_for_each_pad(entity, iter2) {
> > + if (iter2 == iter)
> > + break;
> > + media_gobj_destroy(&iter->graph_obj);
>
> Wrong iterator.
>
> Instead of using a second iterator, which can be error-prone as shown
> here, how about breaking when !iter->graph_obj.mdev, which indicates an
> uninitialized object ?
media_gobj_destroy() is actually a no-op if it hasn't been initialised
(created).
It'd be still cleaner to make the pad flag checks first and only then call
media_gobj_create() on them. But if someone needs something more in the
future, then that doesn't mix as well anymore.
Both are fine IMO.
>
> > + }
> > + }
> > +
> > if (mdev)
> > mutex_unlock(&mdev->graph_mutex);
> >
> > - return 0;
> > + return ret;
> > }
> > EXPORT_SYMBOL_GPL(media_entity_pads_init);
> >
>
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 12/12] media: mc: Check pad flag validity
2023-09-19 10:24 ` Sakari Ailus
@ 2023-09-19 10:37 ` Laurent Pinchart
0 siblings, 0 replies; 28+ messages in thread
From: Laurent Pinchart @ 2023-09-19 10:37 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tianshu Qiu, Bingbu Cao, Tomi Valkeinen,
Jacopo Mondi, Rui Miguel Silva, Martin Kepplinger
On Tue, Sep 19, 2023 at 10:24:32AM +0000, Sakari Ailus wrote:
> On Mon, Sep 18, 2023 at 04:48:02PM +0300, Laurent Pinchart wrote:
> > On Mon, Sep 18, 2023 at 03:51:38PM +0300, Sakari Ailus wrote:
> > > Check the validity of pad flags on entity init. Exactly one of the flags
> > > must be set.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > drivers/media/mc/mc-entity.c | 20 ++++++++++++++++++--
> > > 1 file changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > > index 83468d4a440b..38d5bbae33d7 100644
> > > --- a/drivers/media/mc/mc-entity.c
> > > +++ b/drivers/media/mc/mc-entity.c
> > > @@ -195,8 +195,9 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> > > struct media_pad *pads)
> > > {
> > > struct media_device *mdev = entity->graph_obj.mdev;
> > > - struct media_pad *iter;
> > > + struct media_pad *iter, *iter2;
> > > unsigned int i = 0;
> > > + int ret = 0;
> > >
> > > if (num_pads >= MEDIA_ENTITY_MAX_PADS)
> > > return -E2BIG;
> > > @@ -210,15 +211,30 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> > > media_entity_for_each_pad(entity, iter) {
> > > iter->entity = entity;
> > > iter->index = i++;
> > > +
> > > + if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK |
> > > + MEDIA_PAD_FL_SOURCE)) != 1) {
> > > + ret = -EINVAL;
> > > + break;
> > > + }
> > > +
> > > if (mdev)
> > > media_gobj_create(mdev, MEDIA_GRAPH_PAD,
> > > &iter->graph_obj);
> > > }
> > >
> > > + if (ret && mdev) {
> > > + media_entity_for_each_pad(entity, iter2) {
> > > + if (iter2 == iter)
> > > + break;
> > > + media_gobj_destroy(&iter->graph_obj);
> >
> > Wrong iterator.
> >
> > Instead of using a second iterator, which can be error-prone as shown
> > here, how about breaking when !iter->graph_obj.mdev, which indicates an
> > uninitialized object ?
>
> media_gobj_destroy() is actually a no-op if it hasn't been initialised
> (created).
I'm fine calling it unconditionally too.
> It'd be still cleaner to make the pad flag checks first and only then call
> media_gobj_create() on them. But if someone needs something more in the
> future, then that doesn't mix as well anymore.
>
> Both are fine IMO.
>
> > > + }
> > > + }
> > > +
> > > if (mdev)
> > > mutex_unlock(&mdev->graph_mutex);
> > >
> > > - return 0;
> > > + return ret;
> > > }
> > > EXPORT_SYMBOL_GPL(media_entity_pads_init);
> > >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread