Received-SPF: pass (google.com: domain of linux-kernel+bounces-215622-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sat, 15 Jun 2024 03:47:05 +0300
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Julien Stephan <jstephan@baylibre.com>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>,
	Louis Kuo <louis.kuo@mediatek.com>,
	Phi-bang Nguyen <pnguyen@baylibre.com>,
	Florian Sylvestre <fsylvestre@baylibre.com>,
	Andy Hsieh <andy.hsieh@mediatek.com>,
	Conor Dooley <conor+dt@kernel.org>, devicetree@vger.kernel.org,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-mediatek@lists.infradead.org, linux-media@vger.kernel.org,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Paul Elder <paul.elder@ideasonboard.com>,
	Rob Herring <robh+dt@kernel.org>
Subject: Re: [PATCH v4 3/5] media: platform: mediatek: isp_30: add mediatek
 ISP3.0 sensor interface
Message-ID: <20240615004705.GI9171@pendragon.ideasonboard.com>
References: <20240110141443.364655-1-jstephan@baylibre.com>
 <20240110141443.364655-4-jstephan@baylibre.com>
 <3c2bee40-3792-409c-b42f-f8b013ff641c@collabora.com>
 <CAEHHSvaT_U+HNzWQUoK9EuqGuqEd11+Lu0CLz_rL7uQf0Q5isw@mail.gmail.com>
 <53838e76-bfa4-41f5-a015-a37472e98991@collabora.com>
 <CAEHHSvaRqZM9c8oD05WKkhOHdjKLBkR6tXp2Q1b8OMiDxDsDhQ@mail.gmail.com>
 <20240614123345.GN6019@pendragon.ideasonboard.com>
 <CAEHHSvaWO7m=n5_f0BM7gwuDMfh_GMX=x3DknG28PnmtZbrGQw@mail.gmail.com>
 <20240614144248.GA20136@pendragon.ideasonboard.com>
 <CAEHHSvZPATFV=w451KaaT+e__EK9u3Vc5ORPRQ-Gfa4rJ_o8hA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAEHHSvZPATFV=w451KaaT+e__EK9u3Vc5ORPRQ-Gfa4rJ_o8hA@mail.gmail.com>

On Fri, Jun 14, 2024 at 04:54:47PM +0200, Julien Stephan wrote:
> Le ven. 14 juin 2024 à 16:43, Laurent Pinchart a écrit :
> > On Fri, Jun 14, 2024 at 04:14:52PM +0200, Julien Stephan wrote:
> > > Le ven. 14 juin 2024 à 14:34, Laurent Pinchart a écrit :
> > > > On Fri, Jun 14, 2024 at 12:38:15PM +0200, Julien Stephan wrote:
> > > > > Le mer. 12 juin 2024 à 10:06, AngeloGioacchino Del Regno a écrit :
> > > > > >
> > > > > > Il 10/06/24 16:39, Julien Stephan ha scritto:
> > > > > [...]
> > > > > > >>
> > > > > > >>> +     writel(0x10001, input->base + SENINF_TG1_SEN_CK);
> > > > > > >>
> > > > > > >> Unroll this one... this is the TG1 sensor clock divider.
> > > > > > >>
> > > > > > >> CLKFL GENMASK(5, 0)
> > > > > > >> CLKRS GENMASK(13, 8)
> > > > > > >> CLKCNT GENMASK(21,16)
> > > > > > >>
> > > > > > >> Like this, I don't get what you're trying to set, because you're using a fixed
> > > > > > >> sensor clock rate, meaning that only a handful of camera sensors will be usable.
> > > > > > >>
> > > > > > >> Is this 8Mhz? 16? 24? what? :-)
> > > > > > >>
> > > > > > >> Two hints:
> > > > > > >>    - sensor_clk = clk_get_rate(isp_clk) / (tg1_sen_ck_clkcnt + 1);
> > > > > > >>    - int mtk_seninf_set_sensor_clk(u8 rate_mhz);
> > > > > > >>
> > > > > > >> Please :-)
> > > > > > >
> > > > > > > Hi Angelo,
> > > > > > >
> > > > > > > I think I get your point about not hardcoding the sensor rate, but I
> > > > > > > am not sure how to use
> > > > > > > a mtk_seninf_set_sensor_clk(u8 rate_mhz); function.
> > > > > > >
> > > > > > > Where would it be called? How is it exposed to the user?
> > > > > > >
> > > > > >
> > > > > > As for where: setup, streaming start, resolution change (which may be covered
> > > > > > by streaming start anyway, as a change should be calling stop->start anyway).
> > > > > >
> > > > > > And for the how is it exposed to the user - well, depends what you mean for user,
> > > > > > but it's all standard V4L2 API :-)
> > > > > >
> > > > > > Last but not least, I can give you another hint....
> > > > > >
> > > > > > struct media_entity *entity = (something_here);
> > > > > > struct media_pad *mpad;
> > > > > > struct v4l2_subdev *cam_subdev;
> > > > > > struct v4l2_ctrl *ctl;
> > > > > > s64 link_frequency, pixel_clock;
> > > > > >
> > > > > > if (entity->pads[0].flags & MEDIA_PAD_FL_SINK)
> > > > > >     return -E_NOT_A_CAMERA_SENSOR_WE_IGNORE_THIS_ONE;
> > > > > >
> > > > > > pad = media_pad_remote_pad_first(&entity->pads[0]);
> > > > > > if (!pad)
> > > > > >    return -ENOENT;
> > > > > >
> > > > > > if (!is_media_entity_v4l2_subdev(pad->entity))
> > > > > >    return -ENOENT;
> > > > > >
> > > > > > if (pad->entity->function != MEDIA_ENT_F_CAM_SENSOR)
> > > > > >    return -ENOENT;
> > > > > >
> > > > >
> > > > > Hi Angelo,
> > > > >
> > > > > Thank you for the detailed explanation :)
> > > > > However, I can't make it work because in my case, seninf is connected
> > > > > to an external ISP
> > > > > so pad->entity->function == MEDIA_ENT_F_PROC_VIDEO_ISP.
> > > > >
> > > > > How can I get the pad corresponding to the sensor?
> > > >
> > > > You don't have to. You can drop that check, and get the link frequency
> > > > of the source subdev with v4l2_get_link_freq(), whatever it is.
> > > >
> > > > > > cam_subdev = media_entity_to_v4l2_subdev(pad->entity);
> > > > > > ctl = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > > > >
> > > > > Is this mandatory to implement V4L2_CID_PIXEL_RATE ?
> > > > > Should I return an error if not found?
> > > >
> > > > Does SENINF need to know both the pixel rate and link frequency ?
> > > > V4L2_CID_PIXEL_RATE is very ill-defined, at the moment it only makes
> > > > sense as a value relative to the sensor pixel array, and doesn't really
> > > > apply further down in the pipeline. What information do you need to
> > > > program the SENINF ?
> > >
> > > Hi Laurent,
> > >
> > > I need to know the clock divider for the sensor
> >
> > Could you provide some details on how the SENINF uses that divisor ?
> > What does it control, and what are the constraints ?
> 
> According to the datasheet,  SENINF_TG1_SEN_CK[21:16] :CLKCNT : Sensor
> master clock will be ISP_clock/(CLKCNT+1) where CLKCNT >= 1

I'll need more information. My guess so far is that there's a FIFO
somewhere in the SENINF, with the pixel bus clocked by the CSI-2 clock
before the FIFO, and by the "Sensor master clock" after the FIFO. Is
that right ? If so, the simplest approach would be to use the link
frequency to compute the pixel clock before the FIFO, and make sure that
the sensor master clock will be larger than or equal to that.

A better approach from a power consumption point of view would be to
consider horizontal blanking. The FIFO can fill faster than it gets
emptied during the active portion of the line and then drain during
blanking. This allows for a slower clock on the output side. You will
need to pick an output clock frequency that

- on average is larger than the number of active pixels per line divided
  by the line duration ; and

- ensures the FIFO never overflows during the active portion of the line,
  for cases where the line length is larger than the FIFO size.

> > > > > > /* multiplier is usually bits per pixel, divider is usually num of lanes */
> > > > > > link_frequency = v4l2_get_link_freq(cam_subdev->ctrl_handler, multiplier, divider);
> > > > > > pixel_clock = v4l2_ctrl_g_ctrl_int64(ctl);
> > > > >
> > > > > How to know the sensor clock given link_frequency and pixel_clock?
> > > > > Can you point me to drivers doing something similar?
> > > > >
> > > > > >
> > > > > > ....now you know what the sensor wants, set the seninf sensor clock accordingly.
> > > > > >
> > > > > > Cheers
> > > > > > Angelo
> > > > > >
> > > > > [...]

-- 
Regards,

Laurent Pinchart