| 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: <1d1ffeacb66f0a24212f83bbe86996d9.squirrel@www.codeaurora.org>
Date: Thu, 2 Apr 2015 17:58:58 -0000
From: jilaiw@...eaurora.org
To: "Paul Bolle" <pebolle@...cali.nl>
Cc: "Jilai Wang" <jilaiw@...eaurora.org>,
dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org, robdclark@...il.com
Subject: Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support
Thanks Paul. Some comments embedded and for the rest I will update the
code accordingly.
> A few nits follow.
>
> On Wed, 2015-04-01 at 17:12 -0400, Jilai Wang wrote:
>> --- a/drivers/gpu/drm/msm/Kconfig
>> +++ b/drivers/gpu/drm/msm/Kconfig
>
>> +config DRM_MSM_WB
>> + bool "Enable writeback support for MSM modesetting driver"
>> + depends on DRM_MSM
>> + depends on VIDEO_V4L2
>> + select VIDEOBUF2_CORE
>> + default y
>> + help
>> + Choose this option if you have a need to support writeback
>> + connector.
>
> DRM_MSM_WB is a bool symbol...
>
>> --- 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
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.
>
>> --- /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
>
>> + vfd->v4l2_dev = &dev->v4l2_dev;
>> + vfd->queue = q;
>> +
>> + /*
>> + * Provide a mutex to v4l2 core. It will be used to protect
>> + * all fops and v4l2 ioctls.
>> + */
>> + vfd->lock = &dev->mutex;
>> + video_set_drvdata(vfd, dev);
>> +
>> + ret = video_register_device(vfd, VFL_TYPE_GRABBER, -1);
>> + if (ret < 0)
>> + goto unreg_dev;
>> +
>> + dev->wb = wb;
>> + wb->wb_v4l2 = dev;
>> + v4l2_info(&dev->v4l2_dev, "V4L2 device registered as %s\n",
>> + video_device_node_name(vfd));
>> +
>> + return 0;
>> +
>> +unreg_dev:
>> + v4l2_device_unregister(&dev->v4l2_dev);
>> +free_dev:
>> + kfree(dev);
>> + return ret;
>> +}
>
>
> 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