On Thu, Apr 2, 2015 at 2:24 PM, Paul Bolle <pebolle@xxxxxxxxxx> wrote: > Hi Jilai, > > On Thu, 2015-04-02 at 17:58 +0000, jilaiw@xxxxxxxxxxxxxx wrote: >> Thanks Paul. Some comments embedded and for the rest I will update the >> code accordingly. > > Most of my comments appear to be ill informed, so I wouldn't mind if > you'd specify which updates you actually plan to do. > >> >> --- a/drivers/gpu/drm/msm/Makefile >> >> +++ b/drivers/gpu/drm/msm/Makefile >> > >> >> +msm-$(CONFIG_DRM_MSM_WB) += \ >> >> + mdp/mdp5/mdp5_wb_encoder.o \ >> >> + mdp/mdp_wb/mdp_wb.o \ >> >> + mdp/mdp_wb/mdp_wb_connector.o \ >> >> + mdp/mdp_wb/mdp_wb_v4l2.o >> > >> > so mdp_wb_v4l2.o will never be part of a module. >> If CONFIG-DRM_MSM_WB is y, then all wb related files (including >> mdp_wb_v4l2.o) will be added to msm-y, then be linked to msm.o >> obj-$(CONFIG_DRM_MSM) += msm.o > > (A tell tale was that I didn't quote that line.) > >> Refer to document http://lwn.net/Articles/21835/ (section 3.3), >> it should be able to be built-in to kernel or as a module. > > It's hard typing with a brown paper bag for headware: I still have > trouble speaking Makefile, even after all these years, but I'm afraid > you're right. > >> >> --- /dev/null >> >> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb_v4l2.c >> > >> >> +#include <linux/module.h> >> > >> > This include is needed mostly for module_param(), right? >> > >> >> +#define MSM_WB_MODULE_NAME "msm_wb" >> > >> > MSM_WB_DRIVER_NAME? But see below. >> > >> >> +static unsigned debug; >> >> +module_param(debug, uint, 0644); >> > >> > debug is basically a boolean type of flag. Would >> > static bool debug; >> > module_param(debug, bool, 0644); >> > >> > work? >> > >> >> +MODULE_PARM_DESC(debug, "activates debug info"); >> > >> > No one is ever going to see this description. >> > >> >> +#define dprintk(dev, level, fmt, arg...) \ >> >> + v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg) >> > >> > All instances of dprintk() that I found had level set to 1, so the above >> > could be simplified a bit: >> > #define dprintk(dev, fmt, arg...) \ >> > v4l2_dbg(1, debug, &dev->v4l2_dev, fmt, ## arg) >> > >> > But perhaps pr_debug() and the dynamic debug facility could be used >> > instead of module_param(), dprintk() and v4l2_dbg(). Not sure which >> > approach is easier. >> > >> >> +static const struct v4l2_file_operations msm_wb_v4l2_fops = { >> >> + .owner = THIS_MODULE, >> > >> > THIS_MODULE will basically be equivalent to NULL. >> > >> >> + .open = v4l2_fh_open, >> >> + .release = vb2_fop_release, >> >> + .poll = vb2_fop_poll, >> >> + .unlocked_ioctl = video_ioctl2, >> >> +}; >> > >> >> +int msm_wb_v4l2_init(struct msm_wb *wb) >> >> +{ >> >> + struct msm_wb_v4l2_dev *dev; >> >> + struct video_device *vfd; >> >> + struct vb2_queue *q; >> >> + int ret; >> >> + >> >> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >> >> + if (!dev) >> >> + return -ENOMEM; >> >> + >> >> + snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), >> >> + "%s", MSM_WB_MODULE_NAME); >> > >> > It looks like you can actually drop the #define and merge the last two >> > arguments to snprintf() into just "msm_wb". >> > >> >> + ret = v4l2_device_register(NULL, &dev->v4l2_dev); >> >> + if (ret) >> >> + goto free_dev; >> >> + >> >> + /* default ARGB8888 640x480 */ >> >> + dev->fmt = get_format(V4L2_PIX_FMT_RGB32); >> >> + dev->width = 640; >> >> + dev->height = 480; >> >> + >> >> + /* initialize queue */ >> >> + q = &dev->vb_vidq; >> >> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; >> >> + q->io_modes = VB2_DMABUF; >> >> + q->drv_priv = dev; >> >> + q->buf_struct_size = sizeof(struct msm_wb_v4l2_buffer); >> >> + q->ops = &msm_wb_vb2_ops; >> >> + q->mem_ops = &msm_wb_vb2_mem_ops; >> >> + q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; >> >> + >> >> + ret = vb2_queue_init(q); >> >> + if (ret) >> >> + goto unreg_dev; >> >> + >> >> + mutex_init(&dev->mutex); >> >> + >> >> + vfd = &dev->vdev; >> >> + *vfd = msm_wb_v4l2_template; >> >> + vfd->debug = debug; >> > >> > I couldn't find a member of struct video_device named debug. Where does >> > that come from? Because, as far as I can see, this won't compile. >> yes, it's there: >> http://lxr.free-electrons.com/source/include/media/v4l2-dev.h#L127 > > Probably in some tree I'm not aware of. I only did: > $ git cat-file blob v4.0-rc6:include/media/v4l2-dev.h | grep debug > /* Internal device debug flags, not for use by drivers */ > int dev_debug; > > and > $ git cat-file blob next-20150402:include/media/v4l2-dev.h | grep debug > /* Internal device debug flags, not for use by drivers */ > int dev_debug; > > Which tree does debug come from? fwiw, looks like 17028cd renamed debug -> dev_debug BR, -R > > Thanks, > > > Paul Bolle > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
