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

[Sakari Ailus]

Hi Paul,

On Thu, Apr 28, 2022 at 04:30:11PM +0200, Paul Kocialkowski wrote:
> 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!

You're welcome!

...

> > 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.

This also means it's not possible to associate a capture buffer to a
parameter buffer by other means than timing --- which is unreliable. The
request API would allow that but it's not free of issues either.

Alternatively, I think in this case you could always require the capture
buffer and grab a parameter buffer when it's available. As ISPs are
generally requiring device specific control software, this shouldn't be a
problem really.

I wonder what Laurent thinks.

> 
> 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.

I think that would be nice already, especially if you know these are going
to be different. Otherwise macros could be an option.

...

> > > +	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.

Now that this is an MC-enabled driver, the subdev controls should be
accessed through the subdev nodes only. Adding them to the video device's
control handler is quite hackish and not guaranteed to even work (as e.g.
multiple subdevs can have the same control).

...

> > > +{
> > > +	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.

To that I suggested adding runtime PM support for the affected sensors.
This doesn't seem to get done otherwise. E.g. ipu3-cio2 driver does not
call s_power() on sensor subdevs.

...

> > > +	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.

You can get that information using media-ctl -e 'entity name'.

I guess this could be also added to video_register_device() on debug level.

> > > +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.

I'm fine leaving these here. Just wondering. Up to you.

-- 
Kind regards,

Sakari Ailus