linux-kernel - Re: [RFC PATCH v2 4/7] media: bcm2835-unicam: Add support for for CCP2/CSI2 camera interface

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 26 Jan 2022 16:03:38 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     Jean-Michel Hautbois <jeanmichel.hautbois@...asonboard.com>
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        dave.stevenson@...pberrypi.com, devicetree@...r.kernel.org,
        kernel-list@...pberrypi.com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        linux-rpi-kernel@...ts.infradead.org, lukasz@...y.st,
        mchehab@...nel.org, naush@...pberrypi.com, robh@...nel.org,
        tomi.valkeinen@...asonboard.com
Subject: Re: [RFC PATCH v2 4/7] media: bcm2835-unicam: Add support for for
 CCP2/CSI2 camera interface

Hi,

On Wed, Jan 26, 2022 at 03:21:40PM +0100, Jean-Michel Hautbois wrote:
> On 23/01/2022 00:26, Laurent Pinchart wrote:
> > > +struct unicam_device {
> > > +	struct kref kref;
> > > +
> > > +	/* V4l2 specific parameters */
> > > +	struct v4l2_async_subdev asd;
> > > +
> > > +	/* peripheral base address */
> > > +	void __iomem *base;
> > > +	/* clock gating base address */
> > > +	void __iomem *clk_gate_base;
> > > +	/* lp clock handle */
> > > +	struct clk *clock;
> > > +	/* vpu clock handle */
> > > +	struct clk *vpu_clock;
> > > +	/* vpu clock request */
> > > +	struct clk_request *vpu_req;
> > 
> > Not used (and that may be a problem).
> 
> In the original linux-rpi tree, there is this portion of code in
> unicam_start_streaming:
> 
> dev->vpu_req = clk_request_start(dev->vpu_clock, MIN_VPU_CLOCK_RATE);
> if (!dev->vpu_req) {
> 	unicam_err(dev, "failed to set up VPU clock\n");
> 	goto error_pipeline;
> }
> 
> ret = clk_prepare_enable(dev->vpu_clock);
> if (ret) {
> 	unicam_err(dev, "Failed to enable VPU clock: %d\n", ret);
> 	goto error_pipeline;
> }
> 
> And this is used as this because it depends on the non-merged series "clk:
> [PATCH v2 0/3] clk: Implement a clock request API" [1]
> 
> [1]: https://lore.kernel.org/all/20210914093515.260031-1-maxime@cerno.tech/
> 
> That's why I modified the code and called:
> clk_set_min_rate(dev->vpu_clock, UNICAM_MIN_VPU_CLOCK_RATE);

I assume this would depend on the framerate and resolution though?

> Dave, is it ok or do we need absolutely the clock request API ?

The main issue is that clk_set_min_rate will never scale the clock back
if you (or the HVS) don't have those constraints anymore. So you
eventually make the clock run at the maximum you'll ever need all the
time (which would be around 500MHz in our case).

The clock request API was an attempt at making the clock scale back to
its minimum when we no longer needed it. The current discussion points
towards changing the behavior of clk_set_min_rate:

https://lore.kernel.org/linux-clk/20220125141549.747889-1-maxime@cerno.tech/

So it looks like we won't need the clk_request API after all.

Maxime

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