| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAF6AEGtC+S+tGmnEf__=ex0NBPc7wQtEOfGg8w95mT=UCk-zOw@mail.gmail.com>
Date: Thu, 2 Apr 2015 14:42:47 -0400
From: Rob Clark <robdclark@...il.com>
To: Paul Bolle <pebolle@...cali.nl>
Cc: Jilai Wang <jilaiw@...eaurora.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support
On Thu, Apr 2, 2015 at 2:24 PM, Paul Bolle <pebolle@...cali.nl> wrote:
> Hi Jilai,
>
> On Thu, 2015-04-02 at 17:58 +0000, jilaiw@...eaurora.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,
>
>
> Paul Bolle
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists