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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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