The S5PV210 platform only supports device tree based booting where the FIMC variant data is parsed directly from the device tree, hence the now unused static data can be removed. Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> Reviewed-by: Tomasz Figa <tfiga@chromium.org> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- Changes from v1: - Added R-b tags --- drivers/media/platform/exynos4-is/fimc-core.c | 59 ------------------- 1 file changed, 59 deletions(-) diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c index cde60fbb23a8..2258f3bfc929 100644 --- a/drivers/media/platform/exynos4-is/fimc-core.c +++ b/drivers/media/platform/exynos4-is/fimc-core.c @@ -1110,67 +1110,8 @@ static int fimc_remove(struct platform_device *pdev) return 0; } -/* Image pixel limits, similar across several FIMC HW revisions. */ -static const struct fimc_pix_limit s5p_pix_limit[4] = { - [0] = { - .scaler_en_w = 3264, - .scaler_dis_w = 8192, - .out_rot_en_w = 1920, - .out_rot_dis_w = 4224, - }, - [1] = { - .scaler_en_w = 4224, - .scaler_dis_w = 8192, - .out_rot_en_w = 1920, - .out_rot_dis_w = 4224, - }, - [2] = { - .scaler_en_w = 1920, - .scaler_dis_w = 8192, - .out_rot_en_w = 1280, - .out_rot_dis_w = 1920, - }, -}; - -static const struct fimc_variant fimc0_variant_s5pv210 = { - .has_inp_rot = 1, - .has_out_rot = 1, - .has_cam_if = 1, - .min_inp_pixsize = 16, - .min_out_pixsize = 16, - .hor_offs_align = 8, - .min_vsize_align = 16, - .pix_limit = &s5p_pix_limit[1], -}; - -static const struct fimc_variant fimc1_variant_s5pv210 = { - .has_inp_rot = 1, - .has_out_rot = 1, - .has_cam_if = 1, - .has_mainscaler_ext = 1, - .min_inp_pixsize = 16, - .min_out_pixsize = 16, - .hor_offs_align = 1, - .min_vsize_align = 1, - .pix_limit = &s5p_pix_limit[2], -}; - -static const struct fimc_variant fimc2_variant_s5pv210 = { - .has_cam_if = 1, - .min_inp_pixsize = 16, - .min_out_pixsize = 16, - .hor_offs_align = 8, - .min_vsize_align = 16, - .pix_limit = &s5p_pix_limit[2], -}; - /* S5PV210, S5PC110 */ static const struct fimc_drvdata fimc_drvdata_s5pv210 = { - .variant = { - [0] = &fimc0_variant_s5pv210, - [1] = &fimc1_variant_s5pv210, - [2] = &fimc2_variant_s5pv210, - }, .num_entities = 3, .lclk_frequency = 166000000UL, .out_buf_count = 4, -- 2.20.1
From: Tomasz Figa <tomasz.figa@gmail.com> On FIMC variants which don't have writeback channel, there is no need to access system registers. This patch makes the driver request sysreg regmap conditionally depending on whether writeback is supported. Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> Reviewed-by: Tomasz Figa <tfiga@chromium.org> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- Changes from v1: - Added R-b tags --- drivers/media/platform/exynos4-is/fimc-core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c index 2258f3bfc929..08d1f39a914c 100644 --- a/drivers/media/platform/exynos4-is/fimc-core.c +++ b/drivers/media/platform/exynos4-is/fimc-core.c @@ -954,9 +954,11 @@ static int fimc_probe(struct platform_device *pdev) spin_lock_init(&fimc->slock); mutex_init(&fimc->lock); - fimc->sysreg = fimc_get_sysreg_regmap(dev->of_node); - if (IS_ERR(fimc->sysreg)) - return PTR_ERR(fimc->sysreg); + if (fimc->variant->has_isp_wb) { + fimc->sysreg = fimc_get_sysreg_regmap(dev->of_node); + if (IS_ERR(fimc->sysreg)) + return PTR_ERR(fimc->sysreg); + } res = platform_get_resource(pdev, IORESOURCE_MEM, 0); fimc->regs = devm_ioremap_resource(dev, res); -- 2.20.1
Not all devices use the CSIS device, some may use the FIMC directly in which case the CSIS device isn't registered. This leads to a nullptr exception when starting the stream as the CSIS device is always referenced. Instead, if getting the CSIS device fails, try getting the FIMC directly to check if we are using the subdev API Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- Changes from v1: - Added R-b tag --- drivers/media/platform/exynos4-is/media-dev.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c index 9aaf3b8060d5..5c32abc7251b 100644 --- a/drivers/media/platform/exynos4-is/media-dev.c +++ b/drivers/media/platform/exynos4-is/media-dev.c @@ -289,11 +289,26 @@ static int __fimc_pipeline_s_stream(struct exynos_media_pipeline *ep, bool on) { IDX_CSIS, IDX_FLITE, IDX_FIMC, IDX_SENSOR, IDX_IS_ISP }, }; struct fimc_pipeline *p = to_fimc_pipeline(ep); - struct fimc_md *fmd = entity_to_fimc_mdev(&p->subdevs[IDX_CSIS]->entity); enum fimc_subdev_index sd_id; int i, ret = 0; if (p->subdevs[IDX_SENSOR] == NULL) { + struct fimc_md *fmd; + struct v4l2_subdev *sd = p->subdevs[IDX_CSIS]; + + if (!sd) + sd = p->subdevs[IDX_FIMC]; + + if (!sd) { + /* + * If neither CSIS nor FIMC was set up, + * it's impossible to have any sensors + */ + return -ENODEV; + } + + fmd = entity_to_fimc_mdev(&sd->entity); + if (!fmd->user_subdev_api) { /* * Sensor must be already discovered if we -- 2.20.1
Commit bae4500399c4 ("[media] exynos4-is: Add missing entity function initialization") tried to suppress the warnings such as s5p-fimc-md camera: Entity type for entity FIMC.0 was not initialized! However, this missed setting for the subdev. Set it now to avoid the ugly warnings on boot. Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> --- Changes from v1: - Don't remove the function from the vfd entity as it is distinct from the sd entity - Re-word commit message for above change --- drivers/media/platform/exynos4-is/fimc-capture.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c index 705f182330ca..82f051f6b816 100644 --- a/drivers/media/platform/exynos4-is/fimc-capture.c +++ b/drivers/media/platform/exynos4-is/fimc-capture.c @@ -1898,6 +1898,7 @@ int fimc_initialize_capture_subdev(struct fimc_dev *fimc) return ret; sd->entity.ops = &fimc_sd_media_ops; + sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER; sd->internal_ops = &fimc_capture_sd_internal_ops; v4l2_set_subdevdata(sd, fimc); return 0; -- 2.20.1
Commit ee7160e57c98 ("[media] s5p-fimc: Add support for JPEG capture") added support for JPEG capture, but missed setting a register when the parallel port was used rather than the CSIS device. Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- Changes from v1: - Re-wording commit message - Add R-b tag --- drivers/media/platform/exynos4-is/fimc-reg.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/media/platform/exynos4-is/fimc-reg.c b/drivers/media/platform/exynos4-is/fimc-reg.c index 5ce2bdebd424..269a98fca1e8 100644 --- a/drivers/media/platform/exynos4-is/fimc-reg.c +++ b/drivers/media/platform/exynos4-is/fimc-reg.c @@ -606,6 +606,11 @@ int fimc_hw_set_camera_source(struct fimc_dev *fimc, switch (source->fimc_bus_type) { case FIMC_BUS_TYPE_ITU_601: case FIMC_BUS_TYPE_ITU_656: + if (fimc_fmt_is_user_defined(f->fmt->color)) { + cfg |= FIMC_REG_CISRCFMT_ITU601_8BIT; + break; + } + for (i = 0; i < ARRAY_SIZE(pix_desc); i++) { if (vc->ci_fmt.code == pix_desc[i].pixelcode) { cfg = pix_desc[i].cisrcfmt; @@ -707,6 +712,8 @@ int fimc_hw_set_camera_type(struct fimc_dev *fimc, case FIMC_BUS_TYPE_ITU_601...FIMC_BUS_TYPE_ITU_656: if (source->mux_id == 0) /* ITU-A, ITU-B: 0, 1 */ cfg |= FIMC_REG_CIGCTRL_SELCAM_ITU_A; + if (vid_cap->ci_fmt.code == MEDIA_BUS_FMT_JPEG_1X8) + cfg |= FIMC_REG_CIGCTRL_CAM_JPEG; break; case FIMC_BUS_TYPE_LCD_WRITEBACK_A: cfg |= FIMC_REG_CIGCTRL_CAMIF_SELWB; -- 2.20.1
Instead of keeping a local copy of how many sensors we've probed (which may not even properly represent the number of sensors probed if we have a port without a sensor), use the global num_sensors counter that has the actual number used. This will also make it easier to add support for multiple sensors being connected to the same port. Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> --- Changes from v1: - New patch --- drivers/media/platform/exynos4-is/media-dev.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c index 5c32abc7251b..70b0a70840cf 100644 --- a/drivers/media/platform/exynos4-is/media-dev.c +++ b/drivers/media/platform/exynos4-is/media-dev.c @@ -396,9 +396,9 @@ static void fimc_md_pipelines_free(struct fimc_md *fmd) /* Parse port node and register as a sub-device any sensor specified there. */ static int fimc_md_parse_port_node(struct fimc_md *fmd, - struct device_node *port, - unsigned int index) + struct device_node *port) { + int index = fmd->num_sensors; struct fimc_source_info *pd = &fmd->sensor[index].pdata; struct device_node *rem, *ep, *np; struct v4l2_fwnode_endpoint endpoint = { .bus_type = 0 }; @@ -488,7 +488,6 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd) struct device_node *parent = fmd->pdev->dev.of_node; struct device_node *ports = NULL; struct device_node *node; - int index = 0; int ret; /* @@ -515,13 +514,12 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd) if (!port) continue; - ret = fimc_md_parse_port_node(fmd, port, index); + ret = fimc_md_parse_port_node(fmd, port); of_node_put(port); if (ret < 0) { of_node_put(node); goto cleanup; } - index++; } /* Attach sensors listed in the parallel-ports node */ @@ -530,12 +528,11 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd) goto rpm_put; for_each_child_of_node(ports, node) { - ret = fimc_md_parse_port_node(fmd, node, index); + ret = fimc_md_parse_port_node(fmd, node); if (ret < 0) { of_node_put(node); goto cleanup; } - index++; } of_node_put(ports); -- 2.20.1
On some devices, there may be multiple camera sensors attached to the same port. Make sure we probe all of them, not just the first one. Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> --- Changes from v1: - Separated parsing each endpoint into separate func --- drivers/media/platform/exynos4-is/media-dev.c | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c index 70b0a70840cf..6d9b8fe3ac79 100644 --- a/drivers/media/platform/exynos4-is/media-dev.c +++ b/drivers/media/platform/exynos4-is/media-dev.c @@ -394,21 +394,15 @@ static void fimc_md_pipelines_free(struct fimc_md *fmd) } } -/* Parse port node and register as a sub-device any sensor specified there. */ -static int fimc_md_parse_port_node(struct fimc_md *fmd, - struct device_node *port) +static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, + struct device_node *ep) { int index = fmd->num_sensors; struct fimc_source_info *pd = &fmd->sensor[index].pdata; - struct device_node *rem, *ep, *np; + struct device_node *rem, *np; struct v4l2_fwnode_endpoint endpoint = { .bus_type = 0 }; int ret; - /* Assume here a port node can have only one endpoint node. */ - ep = of_get_next_child(port, NULL); - if (!ep) - return 0; - ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &endpoint); if (ret) { of_node_put(ep); @@ -482,6 +476,22 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd, return 0; } +/* Parse port node and register as a sub-device any sensor specified there. */ +static int fimc_md_parse_port_node(struct fimc_md *fmd, + struct device_node *port) +{ + struct device_node *ep; + int ret; + + for_each_child_of_node(port, ep) { + ret = fimc_md_parse_one_endpoint(fmd, ep); + if (ret < 0) + return ret; + } + + return 0; +} + /* Register all SoC external sub-devices */ static int fimc_md_register_sensor_entities(struct fimc_md *fmd) { -- 2.20.1
This is a no-op as it is never set and is a remnant from non-DT days that can be safely removed. Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> Reviewed-by: Tomasz Figa <tfiga@chromium.org> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- Changes from v1: - Added R-b tags --- drivers/media/platform/exynos4-is/fimc-capture.c | 13 +------------ drivers/media/platform/exynos4-is/fimc-core.h | 3 --- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c index 82f051f6b816..e70785d8a809 100644 --- a/drivers/media/platform/exynos4-is/fimc-capture.c +++ b/drivers/media/platform/exynos4-is/fimc-capture.c @@ -492,17 +492,6 @@ static int fimc_capture_open(struct file *file) ret = fimc_pipeline_call(ve, open, &ve->vdev.entity, true); - if (ret == 0 && vc->user_subdev_api && vc->inh_sensor_ctrls) { - /* - * Recreate controls of the the video node to drop - * any controls inherited from the sensor subdev. - */ - fimc_ctrls_delete(vc->ctx); - - ret = fimc_ctrls_create(vc->ctx); - if (ret == 0) - vc->inh_sensor_ctrls = false; - } if (ret == 0) ve->vdev.entity.use_count++; @@ -1408,7 +1397,7 @@ static int fimc_link_setup(struct media_entity *entity, vc->input = sd->grp_id; - if (vc->user_subdev_api || vc->inh_sensor_ctrls) + if (vc->user_subdev_api) return 0; /* Inherit V4L2 controls from the image sensor subdev. */ diff --git a/drivers/media/platform/exynos4-is/fimc-core.h b/drivers/media/platform/exynos4-is/fimc-core.h index d130f664a60b..31f81bcb8483 100644 --- a/drivers/media/platform/exynos4-is/fimc-core.h +++ b/drivers/media/platform/exynos4-is/fimc-core.h @@ -299,8 +299,6 @@ struct fimc_m2m_device { * @input_index: input (camera sensor) index * @input: capture input type, grp_id of the attached subdev * @user_subdev_api: true if subdevs are not configured by the host driver - * @inh_sensor_ctrls: a flag indicating v4l2 controls are inherited from - * an image sensor subdev */ struct fimc_vid_cap { struct fimc_ctx *ctx; @@ -322,7 +320,6 @@ struct fimc_vid_cap { int input_index; u32 input; bool user_subdev_api; - bool inh_sensor_ctrls; }; /** -- 2.20.1
This is no longer used since the conversion to DT Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> Reviewed-by: Tomasz Figa <tfiga@chromium.org> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- Changes from v1: - Added R-b tags --- drivers/media/platform/exynos4-is/fimc-core.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/media/platform/exynos4-is/fimc-core.h b/drivers/media/platform/exynos4-is/fimc-core.h index 31f81bcb8483..e4a56232907a 100644 --- a/drivers/media/platform/exynos4-is/fimc-core.h +++ b/drivers/media/platform/exynos4-is/fimc-core.h @@ -296,7 +296,6 @@ struct fimc_m2m_device { * @buf_index: index for managing the output DMA buffers * @frame_count: the frame counter for statistics * @reqbufs_count: the number of buffers requested in REQBUFS ioctl - * @input_index: input (camera sensor) index * @input: capture input type, grp_id of the attached subdev * @user_subdev_api: true if subdevs are not configured by the host driver */ @@ -317,7 +316,6 @@ struct fimc_vid_cap { unsigned int frame_count; unsigned int reqbufs_count; bool streaming; - int input_index; u32 input; bool user_subdev_api; }; -- 2.20.1
vidioc_streamoff can be called multiple times from userspace, but we should only call media_pipeline_stop when we're actually setup. This became more noticeable after commit 2a2599c66368 ("[media] media: entity: Catch unbalanced media_pipeline_stop calls") was merged as it added a WARN for unbalanced calls to media_pipeline_stop. Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> --- Changes from v1: - re-worded commit message for a better description of the problem --- drivers/media/platform/exynos4-is/fimc-capture.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c index e70785d8a809..887a736b2e0e 100644 --- a/drivers/media/platform/exynos4-is/fimc-capture.c +++ b/drivers/media/platform/exynos4-is/fimc-capture.c @@ -1232,8 +1232,11 @@ static int fimc_cap_streamoff(struct file *file, void *priv, if (ret < 0) return ret; - media_pipeline_stop(&vc->ve.vdev.entity); - vc->streaming = false; + if (vc->streaming) { + media_pipeline_stop(&vc->ve.vdev.entity); + vc->streaming = false; + } + return 0; } -- 2.20.1
The parallel port nodes should be numbered 1 and 2, not 0 and 1 for A and B respectively. The driver has always implemented 1 and 2 and the in-tree Goni DTS uses 1 as port A as well. Update the documentation to match this behaviour. Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> --- Changes from v1: - New patch replacing patch modifying driver to match documentation --- Documentation/devicetree/bindings/media/samsung-fimc.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt index f91b9dc80eb3..20447529c985 100644 --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt @@ -95,7 +95,7 @@ Optional properties: This node should contain child 'port' nodes specifying active parallel video input ports. It includes camera A and camera B inputs. 'reg' property in the -port nodes specifies data input - 0, 1 indicates input A, B respectively. +port nodes specifies data input - 1, 2 indicates input A, B respectively. Optional properties @@ -172,8 +172,8 @@ Example: /* parallel camera ports */ parallel-ports { /* camera A input */ - port@0 { - reg = <0>; + port@1 { + reg = <1>; fimc0_ep: endpoint { remote-endpoint = <&s5k6aa_ep>; bus-width = <8>; -- 2.20.1
On 31.07.2020 01:01, Jonathan Bakker wrote:
> Commit bae4500399c4 ("[media] exynos4-is: Add missing entity function
> initialization") tried to suppress the warnings such as
>
> s5p-fimc-md camera: Entity type for entity FIMC.0 was not initialized!
>
> However, this missed setting for the subdev. Set it now to avoid the
> ugly warnings on boot.
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
On 31.07.2020 01:01, Jonathan Bakker wrote:
> Instead of keeping a local copy of how many sensors we've probed
> (which may not even properly represent the number of sensors
> probed if we have a port without a sensor), use the global
> num_sensors counter that has the actual number used.
>
> This will also make it easier to add support for multiple sensors
> being connected to the same port.
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
On 31.07.2020 01:01, Jonathan Bakker wrote:
> On some devices, there may be multiple camera sensors attached
> to the same port. Make sure we probe all of them, not just the
> first one.
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
On 31.07.2020 01:01, Jonathan Bakker wrote:
> vidioc_streamoff can be called multiple times from userspace, but we
> should only call media_pipeline_stop when we're actually setup.
>
> This became more noticeable after commit 2a2599c66368 ("[media] media:
> entity: Catch unbalanced media_pipeline_stop calls") was merged as it
> added a WARN for unbalanced calls to media_pipeline_stop.
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
On 31.07.2020 01:01, Jonathan Bakker wrote:
> The parallel port nodes should be numbered 1 and 2, not 0 and 1
> for A and B respectively. The driver has always implemented 1
> and 2 and the in-tree Goni DTS uses 1 as port A as well. Update
> the documentation to match this behaviour.
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Thanks for all those improvements, whole series looks good to me.
--
Regards,
Sylwester
On Thu, 30 Jul 2020 16:01:14 -0700, Jonathan Bakker wrote:
> The parallel port nodes should be numbered 1 and 2, not 0 and 1
> for A and B respectively. The driver has always implemented 1
> and 2 and the in-tree Goni DTS uses 1 as port A as well. Update
> the documentation to match this behaviour.
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
> Changes from v1:
> - New patch replacing patch modifying driver to match documentation
> ---
> Documentation/devicetree/bindings/media/samsung-fimc.txt | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
Reviewed-by: Rob Herring <robh@kernel.org>