RE: [PATCH v6 11/12] intel-ipu3: Add v4l2 driver based on media framework

["Zhi, Yong" <yong.zhi@xxxxxxxxx>]

Hi, Tomasz,

Sorry for the delay in responding to your review.

> -----Original Message-----
> From: linux-media-owner@xxxxxxxxxxxxxxx [mailto:linux-media-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Tomasz Figa
> Sent: Monday, July 2, 2018 2:50 AM
> To: Zhi, Yong <yong.zhi@xxxxxxxxx>
> Cc: Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx>; Sakari Ailus
> <sakari.ailus@xxxxxxxxxxxxxxx>; Mani, Rajmohan
> <rajmohan.mani@xxxxxxxxx>; Toivonen, Tuukka
> <tuukka.toivonen@xxxxxxxxx>; Hu, Jerry W <jerry.w.hu@xxxxxxxxx>; Zheng,
> Jian Xu <jian.xu.zheng@xxxxxxxxx>; Vijaykumar, Ramya
> <ramya.vijaykumar@xxxxxxxxx>
> Subject: Re: [PATCH v6 11/12] intel-ipu3: Add v4l2 driver based on media
> framework
> 
> Hi Yong,
> 
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi <yong.zhi@xxxxxxxxx> wrote:
> [snip]
> > +static int ipu3_vidioc_enum_input(struct file *file, void *fh,
> > +                                 struct v4l2_input *input) {
> > +       if (input->index > 0)
> > +               return -EINVAL;
> > +       strlcpy(input->name, "camera", sizeof(input->name));
> > +       input->type = V4L2_INPUT_TYPE_CAMERA;
> > +
> > +       return 0;
> > +}
> > +
> > +static int ipu3_vidioc_g_input(struct file *file, void *fh, unsigned
> > +int *input) {
> > +       *input = 0;
> > +
> > +       return 0;
> > +}
> > +
> > +static int ipu3_vidioc_s_input(struct file *file, void *fh, unsigned
> > +int input) {
> > +       return input == 0 ? 0 : -EINVAL; }
> > +
> > +static int ipu3_vidioc_enum_output(struct file *file, void *fh,
> > +                                  struct v4l2_output *output) {
> > +       if (output->index > 0)
> > +               return -EINVAL;
> > +       strlcpy(output->name, "camera", sizeof(output->name));
> > +       output->type = V4L2_INPUT_TYPE_CAMERA;
> > +
> > +       return 0;
> > +}
> > +
> > +static int ipu3_vidioc_g_output(struct file *file, void *fh,
> > +                               unsigned int *output) {
> > +       *output = 0;
> > +
> > +       return 0;
> > +}
> > +
> > +static int ipu3_vidioc_s_output(struct file *file, void *fh,
> > +                               unsigned int output) {
> > +       return output == 0 ? 0 : -EINVAL; }
> 
> Do we really need to implement the 6 functions above? They don't seem to
> be doing anything useful.
> 

They are here to pass v4l2-compliance test. I can add a note in next update for their purpose.  We can remove them in the future when defaults callbacks are available for those ops.

> [snip]
> 
> > +int ipu3_v4l2_register(struct imgu_device *imgu) {
> > +       struct v4l2_mbus_framefmt def_bus_fmt = { 0 };
> > +       struct v4l2_pix_format_mplane def_pix_fmt = { 0 };
> > +
> > +       int i, r;
> > +
> > +       /* Initialize miscellaneous variables */
> > +       imgu->streaming = false;
> > +
> > +       /* Set up media device */
> > +       imgu->media_dev.dev = &imgu->pci_dev->dev;
> > +       strlcpy(imgu->media_dev.model, IMGU_NAME,
> > +               sizeof(imgu->media_dev.model));
> > +       snprintf(imgu->media_dev.bus_info, sizeof(imgu-
> >media_dev.bus_info),
> > +                "%s", dev_name(&imgu->pci_dev->dev));
> > +       imgu->media_dev.hw_revision = 0;
> > +       media_device_init(&imgu->media_dev);
> > +       r = media_device_register(&imgu->media_dev);
> > +       if (r) {
> > +               dev_err(&imgu->pci_dev->dev,
> > +                       "failed to register media device (%d)\n", r);
> > +               return r;
> > +       }
> 
> Shouldn't we register the media device at the end, after all video nodes are
> registered below? Otherwise, since media_device_register() exposes the
> media node to userspace, we risk a race, when userspace opens the media
> device before all the entities are created and linked.
> 

Make sense, will change the call order in v7.

> [snip]
> 
> > +int ipu3_v4l2_unregister(struct imgu_device *imgu) {
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < IMGU_NODE_NUM; i++) {
> > +               video_unregister_device(&imgu->nodes[i].vdev);
> > +               media_entity_cleanup(&imgu->nodes[i].vdev.entity);
> > +               mutex_destroy(&imgu->nodes[i].lock);
> > +       }
> > +
> > +       v4l2_device_unregister_subdev(&imgu->subdev);
> > +       media_entity_cleanup(&imgu->subdev.entity);
> > +       kfree(imgu->subdev_pads);
> > +       v4l2_device_unregister(&imgu->v4l2_dev);
> > +       media_device_unregister(&imgu->media_dev);
> 
> Should unregister media device at the beginning, so that it cannot be used
> when we continue to clean up the entities.
> 

Agree, thanks for the review.

> > +       media_device_cleanup(&imgu->media_dev);
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(ipu3_v4l2_unregister);
> 
> Best regards,
> Tomasz