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

From: Sowjanya Komatineni
Date: Tue Jul 07 2020 - 06:32:51 EST



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