Re: [PATCH v3 3/4] staging: media: Add support for the Allwinner A31 ISP

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 10074 bytes --]

Hi Sakari,

On Thu 28 Apr 22, 15:14, Sakari Ailus wrote:
> Hi Paul,
> 
> Thanks for the set.
> 
> A few comments below.

Thanks a lot for your review!

[...]

> >  .../staging/media/sunxi/sun6i-isp/TODO.txt    |   6 +
> 
> This should be called TODO (without .txt).

Understood!

> I understand this is an online ISP. How do you schedule the video buffer
> queues? Say, what happens if it's time to set up buffers for a frame and
> there's a buffer queued in the parameter queue but not in the image data
> queue? Or the other way around?

The ISP works in a quite atypical way, with a DMA buffer that is used to
hold upcoming parameters (including buffer addresses) and a bit in a "direct"
register to schedule the update of the parameters at next vsync.

The update (setting the bit) is triggered whenever new parameters are
submitted via the params video device or whenever there's a capture buffer
available in the capture video device.

So you don't particularly need to have one parameter buffer matching a capture
buffer, the two can be updated independently. Of course, a capture buffer will
only be returned after another buffer becomes active.

I hope this answers your concern!

[...]

> > +static int sun6i_isp_tables_setup(struct sun6i_isp_device *isp_dev)
> > +{
> > +	struct sun6i_isp_tables *tables = &isp_dev->tables;
> > +	int ret;
> > +
> > +	/* Sizes are hardcoded for now but actually depend on the platform. */
> 
> Would it be cleaner to have them defined in a platform-specific way, e.g.
> in a struct you obtain using device_get_match_data()?

Absolutely! I didn't do it at this stage since only one platform is supported
but we could just as well introduce a variant structure already for the table
sizes.

> > +
> > +	tables->load.size = 0x1000;
> > +	ret = sun6i_isp_table_setup(isp_dev, &tables->load);
> > +	if (ret)
> > +		return ret;
> > +
> > +	tables->save.size = 0x1000;
> > +	ret = sun6i_isp_table_setup(isp_dev, &tables->save);
> > +	if (ret)
> > +		return ret;
> > +
> > +	tables->lut.size = 0xe00;
> > +	ret = sun6i_isp_table_setup(isp_dev, &tables->lut);
> > +	if (ret)
> > +		return ret;
> > +
> > +	tables->drc.size = 0x600;
> > +	ret = sun6i_isp_table_setup(isp_dev, &tables->drc);
> > +	if (ret)
> > +		return ret;
> > +
> > +	tables->stats.size = 0x2100;
> > +	ret = sun6i_isp_table_setup(isp_dev, &tables->stats);
> 
> You can return already here.

Sure.

> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static void sun6i_isp_tables_cleanup(struct sun6i_isp_device *isp_dev)
> > +{
> > +	struct sun6i_isp_tables *tables = &isp_dev->tables;
> > +
> > +	sun6i_isp_table_cleanup(isp_dev, &tables->stats);
> > +	sun6i_isp_table_cleanup(isp_dev, &tables->drc);
> > +	sun6i_isp_table_cleanup(isp_dev, &tables->lut);
> > +	sun6i_isp_table_cleanup(isp_dev, &tables->save);
> > +	sun6i_isp_table_cleanup(isp_dev, &tables->load);
> > +}
> > +
> > +/* Media */
> > +
> > +static const struct media_device_ops sun6i_isp_media_ops = {
> > +	.link_notify = v4l2_pipeline_link_notify,
> > +};
> > +
> > +/* V4L2 */
> > +
> > +static int sun6i_isp_v4l2_setup(struct sun6i_isp_device *isp_dev)
> > +{
> > +	struct sun6i_isp_v4l2 *v4l2 = &isp_dev->v4l2;
> > +	struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
> > +	struct media_device *media_dev = &v4l2->media_dev;
> > +	struct device *dev = isp_dev->dev;
> > +	int ret;
> > +
> > +	/* Media Device */
> > +
> > +	strscpy(media_dev->model, SUN6I_ISP_DESCRIPTION,
> > +		sizeof(media_dev->model));
> > +	snprintf(media_dev->bus_info, sizeof(media_dev->bus_info),
> > +		 "platform:%s", dev_name(dev));
> 
> This is no longer needed, see commit b0e38610f40a0f89e34939d2c7420590d67d86a3

Thanks!

> > +	media_dev->ops = &sun6i_isp_media_ops;
> > +	media_dev->hw_revision = 0;
> > +	media_dev->dev = dev;
> > +
> > +	media_device_init(media_dev);
> > +
> > +	ret = media_device_register(media_dev);
> > +	if (ret) {
> > +		dev_err(dev, "failed to register media device\n");
> > +		return ret;
> > +	}
> > +
> > +	/* V4L2 Control Handler */
> > +
> > +	ret = v4l2_ctrl_handler_init(&v4l2->ctrl_handler, 0);
> 
> I suppose you intend to add controls later on?

I might be wrong but I thought this was necessary to expose sensor controls
registered by subdevs that end up attached to this v4l2 device.

I doubt the drivers itself will expose controls otherwise.

[...]

> > +	isp_dev->clock_ram = devm_clk_get(dev, "ram");
> > +	if (IS_ERR(isp_dev->clock_ram)) {
> > +		dev_err(dev, "failed to acquire ram clock\n");
> > +		return PTR_ERR(isp_dev->clock_ram);
> > +	}
> > +
> > +	ret = clk_set_rate_exclusive(isp_dev->clock_mod, 297000000);
> 
> Is this also specific to the model?

There is less certainty that another platform that will use this driver
will need another rate, but I think it would look better to have it in the
variant anyway.

[...]

> > +static int sun6i_isp_capture_open(struct file *file)
> > +{
> > +	struct sun6i_isp_device *isp_dev = video_drvdata(file);
> > +	struct video_device *video_dev = &isp_dev->capture.video_dev;
> > +	struct mutex *lock = &isp_dev->capture.lock;
> > +	int ret;
> > +
> > +	if (mutex_lock_interruptible(lock))
> > +		return -ERESTARTSYS;
> > +
> > +	ret = v4l2_pipeline_pm_get(&video_dev->entity);
> 
> Do you need this?
> 
> Drivers should primarily depend on runtime PM, this is only needed for
> compatibility reasons. Instead I'd like to see sensor drivers being moved
> to runtime PM.

Yes it's still needed to support sensor drivers that don't use rpm yet.

[...]

> > +int sun6i_isp_capture_setup(struct sun6i_isp_device *isp_dev)
> > +{
> > +	struct sun6i_isp_capture *capture = &isp_dev->capture;
> > +	struct sun6i_isp_capture_state *state = &capture->state;
> > +	struct v4l2_device *v4l2_dev = &isp_dev->v4l2.v4l2_dev;
> > +	struct v4l2_subdev *proc_subdev = &isp_dev->proc.subdev;
> > +	struct video_device *video_dev = &capture->video_dev;
> > +	struct vb2_queue *queue = &capture->queue;
> > +	struct media_pad *pad = &capture->pad;
> > +	struct v4l2_format *format = &capture->format;
> > +	struct v4l2_pix_format *pix_format = &format->fmt.pix;
> > +	int ret;
> > +
> > +	/* State */
> > +
> > +	INIT_LIST_HEAD(&state->queue);
> > +	spin_lock_init(&state->lock);
> > +
> > +	/* Media Entity */
> > +
> > +	video_dev->entity.ops = &sun6i_isp_capture_entity_ops;
> > +
> > +	/* Media Pads */
> > +
> > +	pad->flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
> > +
> > +	ret = media_entity_pads_init(&video_dev->entity, 1, pad);
> > +	if (ret)
> > +		goto error_mutex;
> 
> return ret;

Good catch, thanks!

[...]

> > +	ret = video_register_device(video_dev, VFL_TYPE_VIDEO, -1);
> > +	if (ret) {
> > +		v4l2_err(v4l2_dev, "failed to register video device: %d\n",
> > +			 ret);
> > +		goto error_media_entity;
> > +	}
> > +
> > +	v4l2_info(v4l2_dev, "device %s registered as %s\n", video_dev->name,
> > +		  video_device_node_name(video_dev));
> 
> This isn't really driver specific. I'd drop it.

I agree but I see that many drivers are doing it and the information can
actually be quite useful at times.

> > +
> > +	/* Media Pad Link */
> > +
> > +	ret = media_create_pad_link(&proc_subdev->entity,
> > +				    SUN6I_ISP_PROC_PAD_SOURCE,
> > +				    &video_dev->entity, 0,
> > +				    MEDIA_LNK_FL_ENABLED |
> > +				    MEDIA_LNK_FL_IMMUTABLE);
> > +	if (ret < 0) {
> > +		v4l2_err(v4l2_dev, "failed to create %s:%u -> %s:%u link\n",
> > +			 proc_subdev->entity.name, SUN6I_ISP_PROC_PAD_SOURCE,
> > +			 video_dev->entity.name, 0);
> 
> This error message printing would be better to be added to
> media_create_pad_link().

Yeah that makes sense.

[...]

> > +void sun6i_isp_params_configure(struct sun6i_isp_device *isp_dev)
> > +{
> > +	struct sun6i_isp_params_state *state = &isp_dev->params.state;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&state->lock, flags);
> > +
> > +	sun6i_isp_params_configure_base(isp_dev);
> > +
> > +	/* Default config is only applied at the very first stream start. */
> > +	if (state->configured)
> > +		goto complete;
> > +
> > +	 sun6i_isp_params_configure_modules(isp_dev,
> 
> Indentation. Doesn't checkpatch.pl complain?

It doesn't, even with --strict, but that's definitely an issue there.

[...]

> > +static int sun6i_isp_params_querycap(struct file *file, void *private,
> > +				     struct v4l2_capability *capability)
> > +{
> > +	struct sun6i_isp_device *isp_dev = video_drvdata(file);
> > +	struct video_device *video_dev = &isp_dev->params.video_dev;
> > +
> > +	strscpy(capability->driver, SUN6I_ISP_NAME, sizeof(capability->driver));
> > +	strscpy(capability->card, video_dev->name, sizeof(capability->card));
> > +	snprintf(capability->bus_info, sizeof(capability->bus_info),
> > +		 "platform:%s", dev_name(isp_dev->dev));
> 
> This is no longer needed with commit
> 2a792fd5cf669d379d82354a99998d9ae9ff7d14 .

Thanks.

[...]

> > +static const struct v4l2_subdev_pad_ops sun6i_isp_proc_pad_ops = {
> > +	.init_cfg	= sun6i_isp_proc_init_cfg,
> > +	.enum_mbus_code	= sun6i_isp_proc_enum_mbus_code,
> > +	.get_fmt	= sun6i_isp_proc_get_fmt,
> > +	.set_fmt	= sun6i_isp_proc_set_fmt,
> > +};
> > +
> > +const struct v4l2_subdev_ops sun6i_isp_proc_subdev_ops = {
> 
> This can be static, can't it?

Oops, yes and it should be. Same applies to sun6i-csi-bridge actually.

[...]

> > +struct sun6i_isp_params_config_bdnf {
> > +	__u8	in_dis_min; // 8
> > +	__u8	in_dis_max; // 10
> 
> Are these default values or something else? Better documentation was in the
> TODO.txt file already.

Yes that's the default register values, but these comments are and overlook on
my side and should be removed.

Thanks!

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]