From: Paul Bolle <pebolle@tiscali.nl>
To: jilaiw@codeaurora.org
Cc: dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
linux-kernel@vger.kernel.org, robdclark@gmail.com
Subject: Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support
Date: Thu, 02 Apr 2015 20:24:02 +0200 [thread overview]
Message-ID: <1427999042.10518.60.camel@x220> (raw)
In-Reply-To: <1d1ffeacb66f0a24212f83bbe86996d9.squirrel@www.codeaurora.org>
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?
Thanks,
Paul Bolle
next prev parent reply other threads:[~2015-04-02 18:24 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-01 21:12 [PATCH 2/3] drm:msm: Initial Add Writeback Support Jilai Wang
2015-04-02 9:40 ` Paul Bolle
2015-04-02 17:58 ` jilaiw
2015-04-02 18:24 ` Paul Bolle [this message]
2015-04-02 18:42 ` Rob Clark
2015-04-02 18:54 ` jilaiw
2015-04-02 18:54 ` jilaiw
2015-04-02 11:48 ` Emil Velikov
2015-04-02 18:07 ` jilaiw
2015-04-02 18:19 ` Rob Clark
2015-04-02 22:29 ` Emil Velikov
2015-04-02 14:29 ` Rob Clark
2015-04-07 5:59 ` Daniel Vetter
2015-04-07 15:55 ` jilaiw
2015-04-07 18:09 ` [PATCH 2/3] drm:msm: Initial Add Writeback Support (V2) Jilai Wang
2015-08-19 19:00 ` Rob Clark
2015-08-25 7:05 ` Daniel Vetter
2015-08-25 19:11 ` Rob Clark
2015-08-26 12:06 ` Daniel Vetter
2015-04-08 8:07 ` [PATCH 2/3] drm:msm: Initial Add Writeback Support Daniel Vetter
2015-04-08 14:40 ` jilaiw
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1427999042.10518.60.camel@x220 \
--to=pebolle@tiscali.nl \
--cc=dri-devel@lists.freedesktop.org \
--cc=jilaiw@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robdclark@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).