> 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, Rob/Paul. My working kernel is still 3.14 based with drm related code up-to-date. It seems that I have to pick the latest kernel for this change, at least to pass the compilation. > >> >> Thanks, >> >> >> Paul Bolle >> > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
