Re: [RFC PATCH v2 11/18] media: tegra-video: Add support for external sensor capture

[Sowjanya Komatineni]

On 7/7/20 2:51 AM, Hans Verkuil wrote:
> On 07/07/2020 11:40, Sowjanya Komatineni wrote:
> > On 7/6/20 2:10 AM, Hans Verkuil wrote:
> > > > +static int tegra_vi_graph_notify_complete(struct v4l2_async_notifier *notifier)
> > > > +{
> > > > +	struct tegra_vi_graph_entity *entity;
> > > > +	struct v4l2_async_subdev *asd;
> > > > +	struct v4l2_subdev *subdev;
> > > > +	struct tegra_vi_channel *chan;
> > > > +	struct tegra_vi *vi;
> > > > +	int ret;
> > > > +
> > > > +	chan = container_of(notifier, struct tegra_vi_channel, notifier);
> > > > +	vi = chan->vi;
> > > > +
> > > > +	dev_dbg(vi->dev, "notify complete, all subdevs registered\n");
> > > > +
> > > > +	ret = video_register_device(&chan->video, VFL_TYPE_VIDEO, -1);
> > > This should be done last after the tegra_vi_graph_build and tegra_channel_setup_ctrl_handler
> > > calls. The video device is immediately accessible after this call, so don't
> > > call it until everything is setup (i.e. until just before the 'return 0;' below).
> > >
> > > > +	if (ret < 0) {
> > > > +		dev_err(vi->dev,
> > > > +			"failed to register video device: %d\n", ret);
> > > > +		goto unregister_video;
> > > > +	}
> > > > +
> > > > +	/* create links between the entities */
> > > > +	list_for_each_entry(asd, &chan->notifier.asd_list, asd_list) {
> > > > +		entity = to_tegra_vi_graph_entity(asd);
> > > > +		ret = tegra_vi_graph_build(chan, entity);
> > > > +		if (ret < 0)
> > > > +			goto unregister_video;
> > > > +	}
> > > > +
> > Hi Hans,
> >
> > Currently Tegra video driver sets v4l2_dev->mdev prior to graph parse and building links to let media_device_register_entity() to happen
> > during video_register_device() -> video_register_media_controller() and media_device_unregister_entity() to happen during v4l2_device_release()
> >
> > TPG also does the same of letting media entity register/unregister to happen during video device register and release callback.
> >
> > So, registering video device prior to media links creation as media_device_register_entity() will happen during video_register_device()
> >
> > To register video device after creating media links, it need to change for both TPG and Non-TPG to not set v4l2_dev->mdev and Tegra video
> > driver should explicitly take care of media_device_register_entity() and media_device_unregister_entity().
> >
> > Prior to making this change to both TPG and Non-TPG, would like to understand on possibility of using video device node prior to finishing
> > complete driver probe()
> >
> > As video device register happens during async notifier complete callback, and all the device graph build happens during video driver probe()
> > what exactly will be the issue of having video device node prior to creating media links?
> It's not the 'create links between the entities' bit that's the problem, it is what follows:
>
> +	ret = tegra_channel_setup_ctrl_handler(chan);
> +	if (ret < 0) {
> +		dev_err(vi->dev,
> +			"failed to setup channel controls: %d\n", ret);
> +		goto unregister_video;
> +	}
> +
> +	vi_fmts_bitmap_init(chan);
> +	subdev = tegra_channel_get_remote_subdev(chan, false);
> +	v4l2_set_subdev_hostdata(subdev, chan);
>
> That should be done before the video_register_device call.

v4l2_subdev is retrieved in tegra_channel_get_remote_subdev() through 
media pad entity links which are setup only during media_create_pad_link()

Currently as driver lets media_device_register_entity() to happen during 
video_device_register(), media_create_pad_link() is done after 
video_register_device.

media_create_pad_link() need to happen prior to using 
tegra_channel_get_remote_subdev() with current implementation.


Probably I can move tegra_channel_setup_ctrl_handler() and 
vi_fmts_bitmap_init() into subdev bound notifier callback to invoke them 
for subdev entity not MEDIA_ENT_F_VID_IF_BRIDGE.

With this by the time video_register_device happens during complete 
callback, ctrl handler and bitmaps both will be setup/initialized.

> Because otherwise the /dev/videoX doesn't have the full set of controls, and
> I am also not certain which ioctls might use the subdev hostdata.

csi driver gets hostdata as it needs channel pg_mode selection value and 
this is used only during streaming.


> The core problem is really that video_register_device should have been split
> into an init function and a register function, so it is possible to set
> everything up before registering the video device. Oh well...
>
> Regards,
>
> 	Hans
>
> > I see some other drivers also doing the same order of registering video device prior to creating media links and also we are doing the same
> > in L4T driver as well.
> >
> > Thanks
> >
> > Sowjanya