On Fri, Apr 27, 2018 at 2:22 AM Zhi, Yong <yong.zhi@xxxxxxxxx> wrote: > Hi, Tomasz, > Thanks for the review again. > > -----Original Message----- > > From: Tomasz Figa [mailto:tfiga@xxxxxxxxxxxx] > > Sent: Thursday, April 26, 2018 12:15 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> > > Subject: Re: [PATCH v6 12/12] intel-ipu3: Add imgu top level pci device > > driver > > > > Hi Yong, > > > > On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi <yong.zhi@xxxxxxxxx> wrote: > > [snip] > > > +static int imgu_video_nodes_init(struct imgu_device *imgu) { > > > + struct v4l2_pix_format_mplane *fmts[IPU3_CSS_QUEUES] = { NULL }; > > > + struct v4l2_rect *rects[IPU3_CSS_RECTS] = { NULL }; > > > + unsigned int i; > > > + int r; > > > + > > > + imgu->buf_struct_size = sizeof(struct imgu_buffer); > > > + > > > + for (i = 0; i < IMGU_NODE_NUM; i++) { > > > + imgu->nodes[i].name = imgu_node_map[i].name; > > > + imgu->nodes[i].output = i < IMGU_QUEUE_FIRST_INPUT; > > > + imgu->nodes[i].immutable = false; > > > + imgu->nodes[i].enabled = false; > > > + > > > + if (i != IMGU_NODE_PARAMS && i != IMGU_NODE_STAT_3A) > > > + fmts[imgu_node_map[i].css_queue] = > > > + &imgu->nodes[i].vdev_fmt.fmt.pix_mp; > > > + atomic_set(&imgu->nodes[i].sequence, 0); > > > + } > > > + > > > + /* Master queue is always enabled */ > > > + imgu->nodes[IMGU_QUEUE_MASTER].immutable = true; > > > + imgu->nodes[IMGU_QUEUE_MASTER].enabled = true; > > > + > > > + r = ipu3_v4l2_register(imgu); > > > + if (r) > > > + return r; > > > + > > > + /* Set initial formats and initialize formats of video nodes */ > > > + rects[IPU3_CSS_RECT_EFFECTIVE] = &imgu->rect.eff; > > > + rects[IPU3_CSS_RECT_BDS] = &imgu->rect.bds; > > > + ipu3_css_fmt_set(&imgu->css, fmts, rects); > > > + > > > + /* Pre-allocate dummy buffers */ > > > + r = imgu_dummybufs_preallocate(imgu); > > > + if (r) { > > > + dev_err(&imgu->pci_dev->dev, > > > + "failed to pre-allocate dummy buffers (%d)", r); > > > + imgu_dummybufs_cleanup(imgu); > > > > No need to call ipu3_v4l2_unregister() here? > > > > (That's why I keep suggesting use of single return error path with labels > > named after the first cleanup step that needs to be done, as it makes it > > easier to spot such mistakes.) > > > Good catch, suggestion taken :) Thanks. > Maybe I should move the imgu_dummybufs_preallocate() out from imgu_video_nodes_init(). Either is fine for me. I don't see any big problem in imgu_dummybufs_preallocate() being in imgu_video_nodes_init(). It must be done before exposing video nodes to userspace (video_device_register()0, so it's probably a good place. Best regards, Tomasz
