linux-kernel - Re: [PATCH 15/22] media: sunxi: Remove the sun6i-csi driver implementation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210913081707.3pjcfuwan46pbdep@gilmour>
Date:   Mon, 13 Sep 2021 10:17:07 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
        linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
        linux-clk@...r.kernel.org, linux-staging@...ts.linux.dev,
        Yong Deng <yong.deng@...ewell.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Helen Koike <helen.koike@...labora.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 15/22] media: sunxi: Remove the sun6i-csi driver
 implementation

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 <paul.kocialkowski@...tlin.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.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)