linux-kernel - Re: [PATCH v2 4/6] media: rockchip: add a driver for the rockchip camera interface (cif)

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] [day] [month] [year] [list]

Message-ID: <Z4D4a0GZ88sqc-rg@kekkonen.localdomain>
Date: Fri, 10 Jan 2025 10:37:31 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Michael Riesch <michael.riesch@...fvision.net>
Cc: Mehdi Djait <mehdi.djait@...ux.intel.com>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Théo Lebrun <theo.lebrun@...tlin.com>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Kever Yang <kever.yang@...k-chips.com>,
	Nicolas Dufresne <nicolas@...fresne.ca>,
	Sebastian Fricke <sebastian.fricke@...labora.com>,
	Alexander Shiyan <eagle.alexander923@...il.com>,
	Val Packett <val@...kett.cool>, Rob Herring <robh@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>, linux-media@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org,
	Mehdi Djait <mehdi.djait@...tlin.com>,
	Gerald Loacker <gerald.loacker@...fvision.net>
Subject: Re: [PATCH v2 4/6] media: rockchip: add a driver for the rockchip
 camera interface (cif)

Hi Michael,

On Fri, Jan 10, 2025 at 10:12:29AM +0100, Michael Riesch wrote:
...
> >>>> +static int cif_stream_enum_framesizes(struct file *file, void *fh,
> >>>> +				      struct v4l2_frmsizeenum *fsize)
> >>>> +{
> >>>> +	struct cif_stream *stream = video_drvdata(file);
> >>>> +	struct v4l2_subdev_frame_size_enum fse = {
> >>>> +		.index = fsize->index,
> >>>> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >>>> +	};
> >>>> +	struct v4l2_subdev *sd = stream->remote->sd;
> >>>> +	const struct cif_output_fmt *fmt;
> >>>> +	int ret;
> >>>> +
> >>>> +	fmt = cif_stream_find_output_fmt(stream, fsize->pixel_format);
> >>>> +	if (!fmt)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	fse.code = fmt->mbus_code;
> >>>> +
> >>>> +	ret = v4l2_subdev_call(sd, pad, enum_frame_size, NULL, &fse);
> >>>
> >>> You shouldn't get this information from the sensor driver but just convey
> >>> what this device supports.
> >>
> >> OK, but what does this device support? In principle, there is a
> >> continuous range of frame sizes up to a certain maximum size. But in
> >> practice, it depends on the subdevice as there is no scaler in the DVP
> >> path. Thus, every frame size but the one that the subdevice delivers
> >> will result in a broken stream?
> > 
> > Could you use CIF_MAX_WIDTH and CIF_MAX_HEIGHT?
> > 
> >>
> >> If this does not return the only possible frame size, is this callback
> >> useful at all?
> > 
> > In order not to configure an output size for the sensor that can't be
> > received by this block?
> 
> Right... Forgot about this case. This means user space needs to
> determine the possible frame sizes of each V4L2 device and subdevice in
> the pipeline and find a size that matches, right?

Correct.

Ideally this would be available on sub-device nodes, not video devices, but
I guess v4l2-compliance requires it on video devices.

> >> And would that apply to all the method and struct names in this driver
> >> or to the driver as well (location, file names)?
> >>
> >> The name has been discussed several times during the 13 iterations of
> >> the PX30 VIP series and I believe it changed from (cif//rkcif_) in
> >> downstream -> (vip//vip_) in Maximes work -> (cif/cif-/cif_) in Mehdis
> >> work, where the tuple is (driver directory/filename prefix/function and
> >> structs prefix).
> >>
> >> Hence I am a bit reluctant to change the naming convention yet again.
> >> That said, I'll be prepared to change it JUST ONE MORE TIME to any tuple
> >> you suggest -- but this really should be the end of the name bikeshedding.
> > 
> > I don't care about the internal naming but the global symbols. Using a
> > namespace is another option.
> > 
> 
> I would suggest the tuple (rkcif/rkcif-/rkcif_) then. This is in
> alignment with the Rockchip ISP driver (rkisp1/rkisp1-/rkisp1_).
> Unfortunately, the Rockchip RGA differs here (but with the tuple
> (rga/rga-/rga_) it uses the same prefix for everything at least).
> 
> There seems to be even less alignment when looking at other
> drivers/media/platform/ drivers, therefore I can only try to maximize
> the alignment within drivers/media/platform/rockchip/.
> 
> What do you think?

The rkcif prefix for anything global seems good to me.

-- 
Kind regards,

Sakari Ailus