[PATCH v2] media: rkisp1: rkisp1-params.c: Fix typos
s/when the camera active/when the camera is active/
s/thus not isr protection/therefore there is no need to acquire a lock/
Signed-off-by: Sebastian Fricke
---
drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index b6beddd988d0..529c6e21815f 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -1258,7 +1258,10 @@ void rkisp1_params_configure(struct rkisp1_params
*params,
rkisp1_params_config_parameter(params);
}
-/* Not called when the camera active, thus not isr protection. */
+/*
+ * Not called when the camera is active, therefore there is no need to acquire
+ * a lock.
+ */
void rkisp1_params_disable(struct rkisp1_params *params)
{
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPCC_MODE,
--
2.25.1
[PATCH] media: rkisp1: rkisp1-params.c: Fix typos
s/when the camera active/when the camera is active/
s/thus not isr protection/thus no ISR protection/
Signed-off-by: Sebastian Fricke
---
drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index b6beddd988d0..ead948a2d01e 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -1258,7 +1258,7 @@ void rkisp1_params_configure(struct rkisp1_params *params,
rkisp1_params_config_parameter(params);
}
-/* Not called when the camera active, thus not isr protection. */
+/* Not called when the camera is active, thus no ISR protection. */
void rkisp1_params_disable(struct rkisp1_params *params)
{
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPCC_MODE,
--
2.25.1
[PATCH] media: rkisp1: rksip1-capture.c: Improve comments and fix typos
Improve the wording of the function description to increase readability.
Fix three typos:
s/during processing a frame/while processing a frame/
s/it also update/it also updates/
s/there's not buf in shadow/there's no buffer in a shadow register/
Replace the abbreviation 'buf' with the full word buffer, the
abbreviation 'config' with the verb configure, and 'regs' with registers.
The goal of this change is to ease the reading flow of the comment.
Signed-off-by: Sebastian Fricke
---
Side-note:
I also believe there should be a description for the abbreviation FE, as
it is not described anywhere. I think it means frame end, right?.
---
.../platform/rockchip/rkisp1/rkisp1-capture.c| 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
index 5f6c9d1623e4..9643bdd05b7b 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
@@ -830,8 +830,8 @@ static void rkisp1_return_all_buffers(struct rkisp1_capture
*cap,
}
/*
- * Most of registers inside rockchip ISP1 have shadow register since
- * they must be not be changed during processing a frame.
+ * Most registers inside the rockchip ISP1 have shadow register since
+ * they must not be changed while processing a frame.
* Usually, each sub-module updates its shadow register after
* processing the last pixel of a frame.
*/
@@ -847,14 +847,14 @@ static void rkisp1_cap_stream_enable(struct
rkisp1_capture *cap)
spin_lock_irq(>buf.lock);
rkisp1_set_next_buf(cap);
cap->ops->enable(cap);
- /* It's safe to config ACTIVE and SHADOW regs for the
+ /* It's safe to configure ACTIVE and SHADOW registers for the
* first stream. While when the second is starting, do NOT
-* force update because it also update the first one.
+* force update because it also updates the first one.
*
-* The latter case would drop one more buf(that is 2) since
-* there's not buf in shadow when the second FE received. This's
-* also required because the second FE maybe corrupt especially
-* when run at 120fps.
+* The latter case would drop one more buffer(that is 2) since
+* there's no buffer in a shadow register when the second FE received.
+* This's also required because the second FE maybe corrupt
+* especially when run at 120fps.
*/
if (!other->is_streaming) {
/* force cfg update */
--
2.25.1
Re: [PATCH] base: power: runtime.c: Remove a unnecessary space
Hey Joe, On 18.04.2021 00:09, Joe Perches wrote: On Sun, 2021-04-18 at 06:08 +, Sebastian Fricke wrote: Remove a redundant space to improve the quality of the comment. I think this patch is not useful. It's not redundant. Thank you, I actually found this pattern a few more times but I wanted to check first if this is a mistake or chosen consciously. Sorry for the noise. Two spaces after a period is commonly used to separate sentences. It's especially common when used with fixed pitch fonts. A trivial grep seems to show it's used about 50K times in comments. Though single space after period may be used about twice as often. $ git grep '^\s*\*.*\. [A-Z]' | wc -l 54439 $ git grep '^\s*\*.*\. [A-Z]' | wc -l 110003 For drivers/base/power/runtime.c, that 2 space after period style is used dozens of times and changing a single instance of it isn't very useful. True and if I understand you correctly you would rather keep it as is right? Greetings, Sebastian --- Side-note: I found this while reading the code, I don't believe it is important but I thought it doesn't hurt to fix it. --- drivers/base/power/runtime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 18b82427d0cb..499434b84171 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -786,7 +786,7 @@ static int rpm_resume(struct device *dev, int rpmflags) } /* -* See if we can skip waking up the parent. This is safe only if +* See if we can skip waking up the parent. This is safe only if * power.no_callbacks is set, because otherwise we don't know whether * the resume will actually succeed. */
[PATCH] base: power: runtime.c: Remove a unnecessary space
Remove a redundant space to improve the quality of the comment. Signed-off-by: Sebastian Fricke --- Side-note: I found this while reading the code, I don't believe it is important but I thought it doesn't hurt to fix it. --- drivers/base/power/runtime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 18b82427d0cb..499434b84171 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -786,7 +786,7 @@ static int rpm_resume(struct device *dev, int rpmflags) } /* -* See if we can skip waking up the parent. This is safe only if +* See if we can skip waking up the parent. This is safe only if * power.no_callbacks is set, because otherwise we don't know whether * the resume will actually succeed. */ -- 2.25.1
Re: [PATCH v2 0/6] Support second Image Signal Processor on rk3399
Hey Heiko, I have tested your series and it successfully fixes the problem with the 2nd camera when HDMI is connected at boot. Besides that the patch looks good and my tests have confirmed that both cameras have the same output quality when I exchange the connected ISP instances. Tested-by: Sebastian Fricke Greetings, Sebastian On 10.02.2021 12:10, Heiko Stuebner wrote: The rk3399 has two ISPs and right now only the first one is usable. The second ISP is connected to the TXRX dphy on the soc. The phy of ISP1 is only accessible through the DSI controller's io-memory, so this series adds support for simply using the dsi controller is a phy if needed. That solution is needed at least on rk3399 and rk3288 but no-one has looked at camera support on rk3288 at all, so right now only implement the rk3399 specifics. changes in v2: - enable grf-clock also for init callback to not break if for example hdmi is connected on boot and disabled the grf clock during its probe - add Sebastian's Tested-by - add Rob's Ack for the phy-cells property Heiko Stuebner (6): drm/rockchip: dsi: add own additional pclk handling dt-bindings: display: rockchip-dsi: add optional #phy-cells property drm/rockchip: dsi: add ability to work as a phy instead of full dsi arm64: dts: rockchip: add #phy-cells to mipi-dsi1 arm64: dts: rockchip: add cif clk-control pinctrl for rk3399 arm64: dts: rockchip: add isp1 node on rk3399 .../display/rockchip/dw_mipi_dsi_rockchip.txt | 1 + arch/arm64/boot/dts/rockchip/rk3399.dtsi | 39 ++ drivers/gpu/drm/rockchip/Kconfig | 2 + .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 349 ++ 4 files changed, 391 insertions(+) -- 2.29.2
Re: [PATCH 0/6] Support second Image Signal Processor on rk3399
Hey Heiko, On 11.02.2021 15:42, Heiko Stübner wrote: Hi Sebastian, Am Donnerstag, 11. Februar 2021, 06:25:15 CET schrieb Sebastian Fricke: Hey Heiko, On 10.02.2021 12:15, Heiko Stübner wrote: >Hi Sebastian, > >Am Freitag, 5. Februar 2021, 15:55:56 CET schrieb Heiko Stübner: >> Hi Sebastian, >> >> I did some tests myself today as well and can confirm your >> hdmi related finding - at least when plugged in on boot. >> >> I tried some combinations of camera vs. hdmi and it seems >> really only when hdmi is plugged in on boot > >as you can see in v2, it should work now even with hdmi >connected on boot. My patch ignored the grf-clock when >doing the grf-based init. > >All clocks are on during boot and I guess the hdmi-driver >did disable it after its probe. The phy_power_on functions >did handle it correctly already, so it was only happening >with hdmi connected on boot. Thank you very much for solving that problem, I've tested the scenarios described below and it works like a charm. (With your V2) > > >Btw. do you plan on submitting your ov13850 driver >soonish? Actually, I have posted the patch already see here: https://patchwork.kernel.org/project/linux-media/patch/20210130182313.32903-2-sebastian.fri...@posteo.net/ very cool to see I currently review the requested changes and questions and will soon post a second version, but I expect quite some time until it is actually merged. could you Cc me on future versions? Sure will do :) Thanks Heiko Sebastian Greetings, Sebastian > > >> >> (1) >> - boot >> - camera >> --> works >> >> (2) >> - boot >> - camera >> - hdmi plugged in >> - hdmi works >> - camera >> --> works >> >> (3) >> - hdmi plugged in >> - boot >> - hdmi works >> - camera >> --> camera doesn't work >> >> (4) >> - boot >> - hdmi plugged in >> - hdmi works >> - camera >> -> camera works >> >> >> With a bit of brute-force [0] it seems the camera also works again even >> with hdmi connected on boot. So conclusion would be that some clock >> is misbehaving. >> >> Now we'll "only" need to find out which one that is. >> >> >> Heiko >> >> >> [0] >> Don't disable any clock gates >> >> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c >> index 070dc47e95a1..8daf1fc3388c 100644 >> --- a/drivers/clk/clk-gate.c >> +++ b/drivers/clk/clk-gate.c >> @@ -61,6 +61,9 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable) >> >> set ^= enable; >> >> +if (!enable) >> +return; >> + >> if (gate->lock) >> spin_lock_irqsave(gate->lock, flags); >> else >> >> >> >> Am Freitag, 5. Februar 2021, 09:15:47 CET schrieb Heiko Stübner: >> > Hi Sebastian, >> > >> > Am Freitag, 5. Februar 2021, 07:43:35 CET schrieb Sebastian Fricke: >> > > On 03.02.2021 20:54, Heiko Stübner wrote: >> > > >Am Mittwoch, 3. Februar 2021, 19:14:22 CET schrieb Sebastian Fricke: >> > > >> I have tested your patch set on my nanoPC-T4, here is a complete log >> > > >> with: >> > > >> - relevant kernel log entries >> > > >> - system information >> > > >> - media ctl output >> > > >> - sysfs entry information >> > > >> >> > > >> https://paste.debian.net/1183874/ >> > > >> >> > > >> Additionally, to your patchset I have applied the following patches: >> > > >> https://github.com/initBasti/Linux_kernel_media_tree_fork/commits/dual_cam_setup >> > > >> >> > > >> And just to not cause confusion the `media_dev` entries come from this >> > > >> unmerged series: >> > > >> https://patchwork.kernel.org/project/linux-media/list/?series=426269 >> > > >> >> > > >> I have actually been able to stream with both of my cameras at the same >> > > >> time using the libcamera cam command. >> > > >> I would like to thank you a lot for making this possible. >> > > > >> > > >Thanks for testing a dual camera setup. On my board I could only test >> > > >the second ISP. And really glad it works for you tool :-) . >> > > > >> > > >Out of curiosity, do you also see that green tint in the images the cameras >> > > >produce? >> > > >&g
Re: [PATCH 0/6] Support second Image Signal Processor on rk3399
Hey Heiko, On 10.02.2021 12:15, Heiko Stübner wrote: Hi Sebastian, Am Freitag, 5. Februar 2021, 15:55:56 CET schrieb Heiko Stübner: Hi Sebastian, I did some tests myself today as well and can confirm your hdmi related finding - at least when plugged in on boot. I tried some combinations of camera vs. hdmi and it seems really only when hdmi is plugged in on boot as you can see in v2, it should work now even with hdmi connected on boot. My patch ignored the grf-clock when doing the grf-based init. All clocks are on during boot and I guess the hdmi-driver did disable it after its probe. The phy_power_on functions did handle it correctly already, so it was only happening with hdmi connected on boot. Thank you very much for solving that problem, I've tested the scenarios described below and it works like a charm. (With your V2) Btw. do you plan on submitting your ov13850 driver soonish? Actually, I have posted the patch already see here: https://patchwork.kernel.org/project/linux-media/patch/20210130182313.32903-2-sebastian.fri...@posteo.net/ I currently review the requested changes and questions and will soon post a second version, but I expect quite some time until it is actually merged. Heiko Greetings, Sebastian (1) - boot - camera --> works (2) - boot - camera - hdmi plugged in - hdmi works - camera --> works (3) - hdmi plugged in - boot - hdmi works - camera --> camera doesn't work (4) - boot - hdmi plugged in - hdmi works - camera -> camera works With a bit of brute-force [0] it seems the camera also works again even with hdmi connected on boot. So conclusion would be that some clock is misbehaving. Now we'll "only" need to find out which one that is. Heiko [0] Don't disable any clock gates diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c index 070dc47e95a1..8daf1fc3388c 100644 --- a/drivers/clk/clk-gate.c +++ b/drivers/clk/clk-gate.c @@ -61,6 +61,9 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable) set ^= enable; +if (!enable) +return; + if (gate->lock) spin_lock_irqsave(gate->lock, flags); else Am Freitag, 5. Februar 2021, 09:15:47 CET schrieb Heiko Stübner: > Hi Sebastian, > > Am Freitag, 5. Februar 2021, 07:43:35 CET schrieb Sebastian Fricke: > > On 03.02.2021 20:54, Heiko Stübner wrote: > > >Am Mittwoch, 3. Februar 2021, 19:14:22 CET schrieb Sebastian Fricke: > > >> I have tested your patch set on my nanoPC-T4, here is a complete log > > >> with: > > >> - relevant kernel log entries > > >> - system information > > >> - media ctl output > > >> - sysfs entry information > > >> > > >> https://paste.debian.net/1183874/ > > >> > > >> Additionally, to your patchset I have applied the following patches: > > >> https://github.com/initBasti/Linux_kernel_media_tree_fork/commits/dual_cam_setup > > >> > > >> And just to not cause confusion the `media_dev` entries come from this > > >> unmerged series: > > >> https://patchwork.kernel.org/project/linux-media/list/?series=426269 > > >> > > >> I have actually been able to stream with both of my cameras at the same > > >> time using the libcamera cam command. > > >> I would like to thank you a lot for making this possible. > > > > > >Thanks for testing a dual camera setup. On my board I could only test > > >the second ISP. And really glad it works for you tool :-) . > > > > > >Out of curiosity, do you also see that green tint in the images the cameras > > >produce? > > > > Yes, I do. Actually, I currently have two forms of a green tint, on my > > OV13850 everything is quite dark and greenish, which is caused by the > > missing 3A algorithms. On my OV4689, I have big patches of the image > > with bright green color and flickering, I investigated if this is > > connected to the 2nd ISP instance, but that doesn't seem to be the case > > as I have the same results when I switch the CSI ports of the cameras. > > > > I have found another issue, while testing I discovered following > > issue: > > When I start the system with an HDMI monitor connected, then the camera > > on the 2nd port doesn't work. This is probably because the RX/TX is > > reserved as a TX. > > But it made me wonder because if the system has an RX, a TX, and > > an RX/TX, why isn't the pure TX used by the monitor and the > > cameras take RX and RX/TX? > > Or do you think that this is maybe a malfunction of this patch? > > I don't think it is an issue with this specific series, but still puzzling. > > I.e. the DPHYs are actually only relevant to
Re: [PATCH 0/6] Support second Image Signal Processor on rk3399
Hey Heiko, On 03.02.2021 20:54, Heiko Stübner wrote: Hi Sebastian, Am Mittwoch, 3. Februar 2021, 19:14:22 CET schrieb Sebastian Fricke: Hey Heiko, I have tested your patch set on my nanoPC-T4, here is a complete log with: - relevant kernel log entries - system information - media ctl output - sysfs entry information https://paste.debian.net/1183874/ Additionally, to your patchset I have applied the following patches: https://github.com/initBasti/Linux_kernel_media_tree_fork/commits/dual_cam_setup And just to not cause confusion the `media_dev` entries come from this unmerged series: https://patchwork.kernel.org/project/linux-media/list/?series=426269 I have actually been able to stream with both of my cameras at the same time using the libcamera cam command. I would like to thank you a lot for making this possible. Thanks for testing a dual camera setup. On my board I could only test the second ISP. And really glad it works for you tool :-) . Out of curiosity, do you also see that green tint in the images the cameras produce? Yes, I do. Actually, I currently have two forms of a green tint, on my OV13850 everything is quite dark and greenish, which is caused by the missing 3A algorithms. On my OV4689, I have big patches of the image with bright green color and flickering, I investigated if this is connected to the 2nd ISP instance, but that doesn't seem to be the case as I have the same results when I switch the CSI ports of the cameras. I have found another issue, while testing I discovered following issue: When I start the system with an HDMI monitor connected, then the camera on the 2nd port doesn't work. This is probably because the RX/TX is reserved as a TX. But it made me wonder because if the system has an RX, a TX, and an RX/TX, why isn't the pure TX used by the monitor and the cameras take RX and RX/TX? Or do you think that this is maybe a malfunction of this patch? Thanks Heiko Greetings, Sebastian If you like to you can add: Tested-by: Sebastian Fricke On 02.02.2021 15:56, Heiko Stuebner wrote: >The rk3399 has two ISPs and right now only the first one is usable. >The second ISP is connected to the TXRX dphy on the soc. > >The phy of ISP1 is only accessible through the DSI controller's >io-memory, so this series adds support for simply using the dsi >controller is a phy if needed. > >That solution is needed at least on rk3399 and rk3288 but no-one >has looked at camera support on rk3288 at all, so right now >only implement the rk3399 specifics. > > >Heiko Stuebner (6): > drm/rockchip: dsi: add own additional pclk handling > dt-bindings: display: rockchip-dsi: add optional #phy-cells property > drm/rockchip: dsi: add ability to work as a phy instead of full dsi > arm64: dts: rockchip: add #phy-cells to mipi-dsi1 > arm64: dts: rockchip: add cif clk-control pinctrl for rk3399 > arm64: dts: rockchip: add isp1 node on rk3399 > > .../display/rockchip/dw_mipi_dsi_rockchip.txt | 1 + > arch/arm64/boot/dts/rockchip/rk3399.dtsi | 39 ++ > drivers/gpu/drm/rockchip/Kconfig | 2 + > .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 342 ++ > 4 files changed, 384 insertions(+) >
Re: [PATCH 0/6] Support second Image Signal Processor on rk3399
Hey Heiko, I have tested your patch set on my nanoPC-T4, here is a complete log with: - relevant kernel log entries - system information - media ctl output - sysfs entry information https://paste.debian.net/1183874/ Additionally, to your patchset I have applied the following patches: https://github.com/initBasti/Linux_kernel_media_tree_fork/commits/dual_cam_setup And just to not cause confusion the `media_dev` entries come from this unmerged series: https://patchwork.kernel.org/project/linux-media/list/?series=426269 I have actually been able to stream with both of my cameras at the same time using the libcamera cam command. I would like to thank you a lot for making this possible. If you like to you can add: Tested-by: Sebastian Fricke On 02.02.2021 15:56, Heiko Stuebner wrote: The rk3399 has two ISPs and right now only the first one is usable. The second ISP is connected to the TXRX dphy on the soc. The phy of ISP1 is only accessible through the DSI controller's io-memory, so this series adds support for simply using the dsi controller is a phy if needed. That solution is needed at least on rk3399 and rk3288 but no-one has looked at camera support on rk3288 at all, so right now only implement the rk3399 specifics. Heiko Stuebner (6): drm/rockchip: dsi: add own additional pclk handling dt-bindings: display: rockchip-dsi: add optional #phy-cells property drm/rockchip: dsi: add ability to work as a phy instead of full dsi arm64: dts: rockchip: add #phy-cells to mipi-dsi1 arm64: dts: rockchip: add cif clk-control pinctrl for rk3399 arm64: dts: rockchip: add isp1 node on rk3399 .../display/rockchip/dw_mipi_dsi_rockchip.txt | 1 + arch/arm64/boot/dts/rockchip/rk3399.dtsi | 39 ++ drivers/gpu/drm/rockchip/Kconfig | 2 + .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 342 ++ 4 files changed, 384 insertions(+) -- 2.29.2
[PATCH] include/linux/miscdevice.h - Fix typo/grammar
Improve the clarity and grammar of descriptive comment on top of the minor number assignments. Fix a typo within 2 comments for macros. s/This helps in eleminating of boilerplate code. /This helps to eliminate boilerplate code./ Signed-off-by: Sebastian Fricke --- include/linux/miscdevice.h | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h index c7a93002a3c1..0676f18093f9 100644 --- a/include/linux/miscdevice.h +++ b/include/linux/miscdevice.h @@ -7,9 +7,9 @@ #include /* - * These allocations are managed by dev...@lanana.org. If you use an - * entry that is not in assigned your entry may well be moved and - * reassigned, or set dynamic if a fixed value is not justified. + * These allocations are managed by dev...@lanana.org. If you need + * an entry that is not assigned here, it can be moved and + * reassigned or dynamically set if a fixed value is not justified. */ #define PSMOUSE_MINOR 1 @@ -93,14 +93,14 @@ extern void misc_deregister(struct miscdevice *misc); /* * Helper macro for drivers that don't do anything special in the initcall. - * This helps in eleminating of boilerplate code. + * This helps to eliminate boilerplate code. */ #define builtin_misc_device(__misc_device) \ builtin_driver(__misc_device, misc_register) /* * Helper macro for drivers that don't do anything special in module init / exit - * call. This helps in eleminating of boilerplate code. + * call. This helps to eliminate boilerplate code. */ #define module_misc_device(__misc_device) \ module_driver(__misc_device, misc_register, misc_deregister) -- 2.20.1
