Messages in this thread | | | Subject | Re: [RFC PATCH v2 11/18] media: tegra-video: Add support for external sensor capture | From | Sowjanya Komatineni <> | Date | Tue, 7 Jul 2020 03:32:49 -0700 |
| |
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 >> >>
| |