LKML Archive on lore.kernel.org help / color / mirror / Atom feed
From: Sakari Ailus <firstname.lastname@example.org> To: Paul Kocialkowski <email@example.com> Cc: Maxime Ripard <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, Yong Deng <email@example.com>, Mauro Carvalho Chehab <firstname.lastname@example.org>, Rob Herring <email@example.com>, Hans Verkuil <firstname.lastname@example.org>, Chen-Yu Tsai <email@example.com>, Jernej Skrabec <firstname.lastname@example.org>, Greg Kroah-Hartman <email@example.com>, Helen Koike <firstname.lastname@example.org>, Laurent Pinchart <email@example.com>, Thomas Petazzoni <firstname.lastname@example.org> Subject: Re: [PATCH 15/22] media: sunxi: Remove the sun6i-csi driver implementation Date: Wed, 15 Sep 2021 22:51:53 +0300 [thread overview] Message-ID: <YUJO2cG/46vOkuBe@paasikivi.fi.intel.com> (raw) In-Reply-To: <YUBXiSrQjccLoa8b@aptenodytes> Hi Paul, Maxime, On Tue, Sep 14, 2021 at 10:04:25AM +0200, Paul Kocialkowski wrote: > Hi, > > On Mon 13 Sep 21, 10:17, Maxime Ripard wrote: > > On Fri, Sep 10, 2021 at 08:41:40PM +0200, Paul Kocialkowski wrote: > > > As described in the commit adding support for the new sun6i-csi driver, > > > a complete rewrite was necessary to support the Allwinner A31 ISP as > > > well as fix a number of issues with the current implementation. > > > > > > Farewell and thanks for all the pixels! > > > > > > Signed-off-by: Paul Kocialkowski <email@example.com> > > > > For completeness, this is what the other commit log mentions: > > > > > While adapting the sun6i-csi driver for MIPI CSI-2 support was > > > possible, it became clear that adding support for the ISP required > > > very heavy changes to the driver which were quite hard to break down > > > into a series of subsequent changes. > > > > > The first major difficulty comes from the lack of v4l2 subdev that > > > acts a bridge, separate from the video node representing the DMA > > > engine. To support the ISP, only parts of the hardware must be > > > configured (excluding aspects related to the DMA output), which made > > > the separation a hard requirement. > > > > > Another significant difficulty was the specific dance that is required > > > to have both the ISP and CSI device be part of the same media device. > > > Because the ISP and CSI are two different hardware blocks, they have > > > two distinct drivers that will each try to register their own v4l2 > > > and media devices, resulting in two distinct pipelines. When the ISP > > > is in use, we actually want the CSI driver to register with the ISP's > > > v4l2 and media devices while keeping the ability to register its own > > > when the ISP is not in use. This is done by: > > > 1. Having the CSI driver check whether the ISP is available, using > > > sun6i_csi_isp_detect(); > > > 2. If not, it can register when its own async subdevs are ready, using > > > sun6i_csi_v4l2_complete(); > > > 3. If so, it will register its bridge as an async subdev which will > > > be picked-up by the ISP driver (from the fwnode graph link); > > > 4. When the subdev becomes bound to the ISP's v4l2 device, we can > > > then access that device (and the associated media device) to > > > complete registration of the capture video node, using > > > sun6i_csi_isp_complete(); > > > Besides the logic rework, other issues were identified and resolved: > > > - The sync mechanism for buffer flipping was based on the frame done > > > interrupt, which is too late (next frame is already being processed). > > > This lead to requiring 3 buffers to start and writing two addresses > > > when starting. Using vsync as a sync point seems to be the correct > > > approach and allows using only two buffers without tearing; > > > - Using devm_regmap_init_mmio_clk was incorrect since the reset also > > > comes into play; > > > - Some register definitions were inverted compared to their actual > > > effect (which was inherited from the Allwinner documentation and > > > code): comments were added where relevant; > > > - The deprecated v4l2_async_notifier_parse_fwnode_endpoints() helper > > > is no longer used by the driver; > > > > With that being said, NAK. > > > > Having heavy changes to a driver is completely fine, and is kind of > > expected really with such a big change. Breaking all possibility of > > bisection and throwing away years of stabilization and maintenance > > isn't. > > > > And all those small bug fixes you mention at the end are just that: > > small bug fixes that can be done on the current driver just fine too. > > I understand that this looks like we're trashing all the work that was > done previously by removing the current driver and adding the new one > but the logic for deciding what to write into registers was carefully > preserved from the original driver to make sure that the works of > stabilization and maintenance are not lost. > > However I would understand that my good promise on this is not enough, > so perhaps I could provide a combinatory verification that the same set > of mbus/pixel formats end up with the same thing being written into > registers. > > In addition I understand that it will be necessary to split the changes > up into small commits to clarify the transition path between the two > drivers. So I will do my best to split things up. > > Does that seem like an agreeable plan or do you see other things that > would be blockers? Please do refactor the patches into reviewable chunks that make sense on their own. I'd see the result being the same driver but with additional patches fixing bugs, doing some or more refactoring and adding new functionality. Please use -C100 -M100 if there's a need to rename files, and preferrably do so in separate patches. See e.g. patches to the smiapp driver that turned it into a CCS driver: git log 2db8166f739e75c1269d7e8afe8da68e70098810..b24cc2a18c50e4e315abc76a86b26b4c49652f79~ -- drivers/media/i2c/smiapp git log drivers/media/i2c/ccs Usually bugfixes are best put first. > > My initial thought was that it would be much easier to review the driver as a > rewrite, but I'm not too surprised I was wrong. To be honest it was nearly > impossible to actually have the initial development happen as sequential steps > and I preferred to allocate my time on other tasks than splitting the changes > into these sequential steps. This isn't really unusual when you're changing an existing driver: sometimes you have to implement what you want to achieve in whole, and only then figure out how to split it into something that can be reviewed. Often the end result will look different than what you arrived with on the first time. -- Kind regards, Sakari Ailus
next prev parent reply other threads:[~2021-09-15 19:52 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-10 18:41 [PATCH 00/22] Allwinner A31/A83T MIPI CSI-2 Support and A31 ISP Support Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 01/22] clk: sunxi-ng: v3s: Make the ISP PLL clock public Paul Kocialkowski 2021-09-13 7:54 ` Maxime Ripard 2021-09-13 8:53 ` Paul Kocialkowski 2021-09-16 16:30 ` Maxime Ripard 2021-09-10 18:41 ` [PATCH 02/22] ARM: dts: sun8i: v3s: Parent the CSI module clock to the ISP PLL Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 03/22] dt-bindings: sun6i-a31-mipi-dphy: Add optional direction property Paul Kocialkowski 2021-09-13 8:00 ` Maxime Ripard 2021-09-14 7:39 ` Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 04/22] phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI CSI-2 Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 05/22] dt-bindings: media: sun6i-a31-csi: Add MIPI CSI-2 input port Paul Kocialkowski 2021-09-13 8:09 ` Maxime Ripard 2021-09-14 7:43 ` Paul Kocialkowski 2021-09-14 12:06 ` Maxime Ripard 2021-09-10 18:41 ` [PATCH 06/22] dt-bindings: media: Add Allwinner A31 MIPI CSI-2 bindings documentation Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 07/22] media: sunxi: Add support for the A31 MIPI CSI-2 controller Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 08/22] MAINTAINERS: Add entry for the Allwinner A31 MIPI CSI-2 bridge driver Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 09/22] ARM: dts: sun8i: v3s: Add nodes for MIPI CSI-2 support Paul Kocialkowski 2021-09-11 2:32 ` Samuel Holland 2021-09-13 7:44 ` Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 10/22] dt-bindings: media: Add Allwinner A83T MIPI CSI-2 bindings documentation Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 11/22] media: sunxi: Add support for the A83T MIPI CSI-2 controller Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 12/22] MAINTAINERS: Add entry for the Allwinner A83T MIPI CSI-2 bridge Paul Kocialkowski 2021-09-10 18:41 ` [PATCH NOT FOR MERGE 13/22] ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node Paul Kocialkowski 2021-09-11 2:53 ` Chen-Yu Tsai 2021-09-13 7:45 ` Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 14/22] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865 Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 15/22] media: sunxi: Remove the sun6i-csi driver implementation Paul Kocialkowski 2021-09-13 8:17 ` Maxime Ripard 2021-09-14 8:04 ` Paul Kocialkowski 2021-09-15 19:51 ` Sakari Ailus [this message] 2021-09-10 18:41 ` [PATCH 16/22] media: sunxi: Introduce a rewritten sun6i-csi driver Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 17/22] dt-bindings: media: Add Allwinner A31 ISP bindings documentation Paul Kocialkowski 2021-09-13 8:18 ` Maxime Ripard 2021-09-14 7:44 ` Paul Kocialkowski 2021-09-14 12:07 ` Maxime Ripard 2021-09-10 18:41 ` [PATCH 18/22] dt-bindings: media: sun6i-a31-csi: Add ISP output port Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 19/22] soc: sunxi: mbus: Add A31 ISP compatibles to the list Paul Kocialkowski 2021-09-11 2:36 ` Samuel Holland 2021-09-13 7:45 ` Paul Kocialkowski 2021-09-13 8:32 ` Maxime Ripard 2021-09-10 18:41 ` [PATCH 20/22] staging: media: Add support for the Allwinner A31 ISP Paul Kocialkowski 2021-09-13 8:31 ` Maxime Ripard 2021-09-14 7:50 ` Paul Kocialkowski 2021-09-14 11:11 ` Laurent Pinchart 2021-09-14 11:48 ` Maxime Ripard 2021-09-10 18:41 ` [PATCH 21/22] MAINTAINERS: Add entry for the Allwinner A31 ISP driver Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 22/22] ARM: dts: sun8i: v3s: Add support for the ISP Paul Kocialkowski
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YUJO2cG/46vOkuBe@paasikivi.fi.intel.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).