Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support

[jilaiw@codeaurora.org]

> On Thu, Apr 2, 2015 at 2:24 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
>> Hi Jilai,
>>
>> On Thu, 2015-04-02 at 17:58 +0000, jilaiw@codeaurora.org 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
>>
>