Re: [PATCH 1/2] OMAP2/3 V4L2: Add support for OMAP2/3 V4L2 driver on top of DSS2
hvaib...@ti.com wrote:
From: Vaibhav Hiremathhvaib...@ti.com
Features Supported -
1. Provides V4L2 user interface for the video pipelines of DSS
2. Basic streaming working on LCD, DVI and TV.
3. Works on latest DSS2 library from Tomi
4. Support for various pixel formats like YUV, UYVY, RGB32, RGB24,
RGB565
5. Supports Alpha blending.
6. Supports Color keying both source and destination.
7. Supports rotation.
8. Supports cropping.
9. Supports Background color setting.
10. Allocated buffers to only needed size
Signed-off-by: Vaibhav Hiremathhvaib...@ti.com
snip
diff --git a/drivers/media/video/omap/omap_voutlib.c
b/drivers/media/video/omap/omap_voutlib.c
new file mode 100644
index 000..05c0e17
--- /dev/null
+++ b/drivers/media/video/omap/omap_voutlib.c
@@ -0,0 +1,258 @@
+/*
+ * omap_voutlib.c
+ *
+ * Copyright (C) 2005-2010 Texas Instruments.
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed as is without any warranty of any
+ * kind, whether express or implied.
+ *
+ * Based on the OMAP2 camera driver
+ * Video-for-Linux (Version 2) camera capture driver for
+ * the OMAP24xx camera controller.
+ *
+ * Author: Andy Lowe (sou...@mvista.com)
+ *
+ * Copyright (C) 2004 MontaVista Software, Inc.
+ * Copyright (C) 2010 Texas Instruments.
+ *
+ */
+
+#includelinux/module.h
+#includelinux/errno.h
+#includelinux/kernel.h
+#includelinux/types.h
+#includelinux/videodev2.h
+
+MODULE_AUTHOR(Texas Instruments);
+MODULE_DESCRIPTION(OMAP Video library);
+MODULE_LICENSE(GPL);
+
+/* Return the default overlay cropping rectangle in crop given the image
+ * size in pix and the video display size in fbuf. The default
+ * cropping rectangle is the largest rectangle no larger than the capture size
+ * that will fit on the display. The default cropping rectangle is centered in
+ * the image. All dimensions and offsets are rounded down to even numbers.
+ */
+void omap_vout_default_crop(struct v4l2_pix_format *pix,
+ struct v4l2_framebuffer *fbuf, struct v4l2_rect *crop)
+{
+ crop-width = (pix-width fbuf-fmt.width) ?
+ pix-width : fbuf-fmt.width;
+ crop-height = (pix-height fbuf-fmt.height) ?
+ pix-height : fbuf-fmt.height;
+ crop-width= ~1;
+ crop-height= ~1;
+ crop-left = ((pix-width - crop-width) 1) ~1;
+ crop-top = ((pix-height - crop-height) 1) ~1;
+}
+EXPORT_SYMBOL_GPL(omap_vout_default_crop);
+
+/* Given a new render window in new_win, adjust the window to the
+ * nearest supported configuration. The adjusted window parameters are
+ * returned in new_win.
+ * Returns zero if succesful, or -EINVAL if the requested window is
+ * impossible and cannot reasonably be adjusted.
+ */
+int omap_vout_try_window(struct v4l2_framebuffer *fbuf,
+ struct v4l2_window *new_win)
+{
+ struct v4l2_rect try_win;
+
+ /* make a working copy of the new_win rectangle */
+ try_win = new_win-w;
+
+ /* adjust the preview window so it fits on the display by clipping any
+* offscreen areas
+*/
+ if (try_win.left 0) {
+ try_win.width += try_win.left;
+ try_win.left = 0;
+ }
+ if (try_win.top 0) {
+ try_win.height += try_win.top;
+ try_win.top = 0;
+ }
+ try_win.width = (try_win.width fbuf-fmt.width) ?
+ try_win.width : fbuf-fmt.width;
+ try_win.height = (try_win.height fbuf-fmt.height) ?
+ try_win.height : fbuf-fmt.height;
+ if (try_win.left + try_win.width fbuf-fmt.width)
+ try_win.width = fbuf-fmt.width - try_win.left;
+ if (try_win.top + try_win.height fbuf-fmt.height)
+ try_win.height = fbuf-fmt.height - try_win.top;
+ try_win.width= ~1;
+ try_win.height= ~1;
+
+ if (try_win.width= 0 || try_win.height= 0)
+ return -EINVAL;
+
+ /* We now have a valid preview window, so go with it */
+ new_win-w = try_win;
+ new_win-field = V4L2_FIELD_ANY;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(omap_vout_try_window);
+
+/* Given a new render window in new_win, adjust the window to the
+ * nearest supported configuration. The image cropping window in crop
+ * will also be adjusted if necessary. Preference is given to keeping the
+ * the window as close to the requested configuration as possible. If
+ * successful, new_win, vout-win, and crop are updated.
+ * Returns zero if succesful, or -EINVAL if the requested preview window is
+ * impossible and cannot reasonably be adjusted.
+ */
+int omap_vout_new_window(struct v4l2_rect *crop,
+ struct v4l2_window *win, struct v4l2_framebuffer *fbuf,
+ struct v4l2_window *new_win)
+{
+ int err;
+
+ err = omap_vout_try_window(fbuf, new_win);
+ if
RE: [PATCH 1/2] OMAP2/3 V4L2: Add support for OMAP2/3 V4L2 driver on top of DSS2
-Original Message- From: Hans Verkuil [mailto:hverk...@xs4all.nl] Sent: Tuesday, April 06, 2010 11:59 PM To: Hiremath, Vaibhav Cc: linux-media@vger.kernel.org; Karicheri, Muralidharan; mche...@redhat.com; linux-o...@vger.kernel.org; t...@atomide.com Subject: Re: [PATCH 1/2] OMAP2/3 V4L2: Add support for OMAP2/3 V4L2 driver on top of DSS2 Hi Vaibhav, Here is my review. I've tried to avoid repeating the points Murali made. [Hiremath, Vaibhav] Thanks Hans, I do agree with all of them. I have already incorporated all of the changes mentioned by you and Murali and posted it to list. Thanks, Vaibhav snip -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] OMAP2/3 V4L2: Add support for OMAP2/3 V4L2 driver on top of DSS2
-Original Message-
From: Karicheri, Muralidharan
Sent: Tuesday, April 06, 2010 11:56 PM
To: Hiremath, Vaibhav; Muralidharan Karicheri
Cc: linux-media@vger.kernel.org; mche...@redhat.com; linux-
o...@vger.kernel.org; t...@atomide.com
Subject: RE: [PATCH 1/2] OMAP2/3 V4L2: Add support for OMAP2/3 V4L2 driver
on top of DSS2
Vaibhav,
[Hiremath, Vaibhav] Thanks Murali, I really appreciate your comments here.
Please find response below -
You had responded only to some comments. Can I assume that you are taking
care of the other comments as well?
[Hiremath, Vaibhav] Yesterday I was replying from home so replied on only
important comments. Today I went through all of the comments, most of them are
valid comments and I accept most of them except 2 -
+static u32 omap_vout_uservirt_to_phys(u32 virtp)
+{
+ unsigned long physp = 0;
+ struct vm_area_struct *vma;
+ struct mm_struct *mm = current-mm;
+
+ vma = find_vma(mm, virtp);
+ /* For kernel direct-mapped memory, take the easy way */
+ if (virtp = PAGE_OFFSET) {
+ physp = virt_to_phys((void *) virtp);
+ } else if (vma (vma-vm_flags VM_IO)
+ vma-vm_pgoff) {
+ /* this will catch, kernel-allocated,
+ mmaped-to-usermode addresses */
+ physp = (vma-vm_pgoff PAGE_SHIFT) + (virtp -
vma-vm_start);
+ } else {
+ /* otherwise, use get_user_pages() for general userland pages
*/
+ int res, nr_pages = 1;
+ struct page *pages;
+ down_read(current-mm-mmap_sem);
+
+ res = get_user_pages(current, current-mm, virtp, nr_pages,
+ 1, 0, pages, NULL);
+ up_read(current-mm-mmap_sem);
+
+ if (res == nr_pages) {
+ physp = __pa(page_address(pages[0]) +
+ (virtp ~PAGE_MASK));
+ } else {
+ printk(KERN_WARNING VOUT_NAME
+ get_user_pages failed\n);
+ return 0;
+ }
+ }
+
+ return physp;
+}
[Murali] Shouldn't we remove omap_vout_uservirt_to_phys() and use
videobuf_iolock() instead as we have done in vpfe_capture.c?
As mentioned before, in my opinion we can address this in sub-sequent patch
series, and should not block this patch in getting to main-line.
+/*
+ * Convert V4L2 pixel format to DSS pixel format
+ */
+static enum omap_color_mode video_mode_to_dss_mode(struct omap_vout_device
+ *vout)
+{
+ struct omap_overlay *ovl;
+ struct omapvideo_info *ovid;
+ struct v4l2_pix_format *pix = vout-pix;
+
+ ovid = vout-vid_info;
+ ovl = ovid-overlays[0];
+
+ switch (pix-pixelformat) {
+ case 0:
+ break;
+ case V4L2_PIX_FMT_YUYV:
+ return OMAP_DSS_COLOR_YUV2;
+
+ case V4L2_PIX_FMT_UYVY:
+ return OMAP_DSS_COLOR_UYVY;
+
+ case V4L2_PIX_FMT_RGB565:
+ return OMAP_DSS_COLOR_RGB16;
+
+ case V4L2_PIX_FMT_RGB24:
+ return OMAP_DSS_COLOR_RGB24P;
+
+ case V4L2_PIX_FMT_RGB32:
+ return (ovl-id == OMAP_DSS_VIDEO1) ?
+ OMAP_DSS_COLOR_RGB24U : OMAP_DSS_COLOR_ARGB32;
+ case V4L2_PIX_FMT_BGR32:
+ return OMAP_DSS_COLOR_RGBX32;
+
+ default:
+ return -EINVAL;
+ }
+ return -EINVAL;
[Murali] Also return type is enum and you are returning a negative number here
???
I think yes it is acceptable, since ultimately enum is of type integer
constant. You can return the value which fits into this size.
I have also asked Hans to provide
his comments since this is a new driver that he might have to approve. Did
he review the code in the past?
[Hiremath, Vaibhav] Yes he reviewed it some time back; anyway he has already
provided few more comments now which I have already fixed. The patch is
following this reply.
Thanks,
Vaibhav
-Murali
snip
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] OMAP2/3 V4L2: Add support for OMAP2/3 V4L2 driver on top of DSS2
Vaibhav,
[Murali] Shouldn't we remove omap_vout_uservirt_to_phys() and use
videobuf_iolock() instead as we have done in vpfe_capture.c?
As mentioned before, in my opinion we can address this in sub-sequent patch
series, and should not block this patch in getting to main-line.
+/*
+ * Convert V4L2 pixel format to DSS pixel format
+ */
+static enum omap_color_mode video_mode_to_dss_mode(struct
omap_vout_device
+ *vout)
+{
+ struct omap_overlay *ovl;
+ struct omapvideo_info *ovid;
+ struct v4l2_pix_format *pix = vout-pix;
+
+ ovid = vout-vid_info;
+ ovl = ovid-overlays[0];
+
+ switch (pix-pixelformat) {
+ case 0:
+ break;
+ case V4L2_PIX_FMT_YUYV:
+ return OMAP_DSS_COLOR_YUV2;
+
+ case V4L2_PIX_FMT_UYVY:
+ return OMAP_DSS_COLOR_UYVY;
+
+ case V4L2_PIX_FMT_RGB565:
+ return OMAP_DSS_COLOR_RGB16;
+
+ case V4L2_PIX_FMT_RGB24:
+ return OMAP_DSS_COLOR_RGB24P;
+
+ case V4L2_PIX_FMT_RGB32:
+ return (ovl-id == OMAP_DSS_VIDEO1) ?
+ OMAP_DSS_COLOR_RGB24U : OMAP_DSS_COLOR_ARGB32;
+ case V4L2_PIX_FMT_BGR32:
+ return OMAP_DSS_COLOR_RGBX32;
+
+ default:
+ return -EINVAL;
+ }
+ return -EINVAL;
[Murali] Also return type is enum and you are returning a negative number
here ???
I think yes it is acceptable, since ultimately enum is of type integer
constant. You can return the value which fits into this size.
[MK] I did some research into this and this code can behave differently
with different compilers. So if compiler treats the enum as int, it would
work fine, but not otherwise. IMO, we shouldn't write code that are dependant
on the tool chain (rather write portable code). So suggest you
change the return type to int instead of enum. Doesn't this code give
you a warning?
I have also asked Hans to provide
his comments since this is a new driver that he might have to approve.
Did
he review the code in the past?
[Hiremath, Vaibhav] Yes he reviewed it some time back; anyway he has
already provided few more comments now which I have already fixed. The
patch is following this reply.
Thanks,
Vaibhav
-Murali
snip
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] OMAP2/3 V4L2: Add support for OMAP2/3 V4L2 driver on top of DSS2
-Original Message-
From: Muralidharan Karicheri [mailto:mkarich...@gmail.com]
Sent: Saturday, April 03, 2010 1:33 AM
To: Hiremath, Vaibhav
Cc: linux-media@vger.kernel.org; Karicheri, Muralidharan;
mche...@redhat.com; linux-o...@vger.kernel.org; t...@atomide.com
Subject: Re: [PATCH 1/2] OMAP2/3 V4L2: Add support for OMAP2/3 V4L2 driver
on top of DSS2
Vaibhav,
I have some comments on this patch. Please address them.
[Hiremath, Vaibhav] Thanks Murali, I really appreciate your comments here.
Please find response below -
+
+#include asm/processor.h
Add a line here??
+#include plat/dma.h
+#include plat/vram.h
+#include plat/vrfb.h
+#include plat/display.h
+
+#include omap_voutlib.h
+#include omap_voutdef.h
+
+MODULE_AUTHOR(Texas Instruments);
+MODULE_DESCRIPTION(OMAP Video for Linux Video out driver);
+MODULE_LICENSE(GPL);
+
+
+/* Driver Configuration macros */
+#define VOUT_NAME omap_vout
+
+enum omap_vout_channels {
+ OMAP_VIDEO1 = 0,
Why do we have to initialize this to 0. It always start with a value 0
by default.
+ OMAP_VIDEO2,
+};
+
+enum dma_channel_state {
+ DMA_CHAN_NOT_ALLOTED = 0,
Ditto.
+ DMA_CHAN_ALLOTED,
+};
+
+#define QQVGA_WIDTH160
+#define QQVGA_HEIGHT 120
+
+/* Max Resolution supported by the driver */
+#define VID_MAX_WIDTH 1280/* Largest width */
+#define VID_MAX_HEIGHT 720 /* Largest height */
+
-
+
+module_param(debug, bool, S_IRUGO);
+MODULE_PARM_DESC(debug, Debug level (0-1));
+
+/* Local Helper functions */
+static void omap_vout_isr(void *arg, unsigned int irqstatus);
+static void omap_vout_cleanup_device(struct omap_vout_device *vout);
+
Is there a reason why you need these prototypes? I think we could
remove these prototypes and move the function ahead in the file before
it is called.
[Hiremath, Vaibhav] Do you see any harm with this? I think its only
implementation part. But still I will incorporate in next version.
+/* list of image formats supported by OMAP2 video pipelines */
+const static struct v4l2_fmtdesc omap_formats[] = {
+ {
+ /* Note: V4L2 defines RGB565 as:
+*
+* Byte 0Byte 1
+* g2 g1 g0 r4 r3 r2 r1 r0 b4 b3 b2 b1 b0 g5 g4 g3
+*
+* We interpret RGB565 as:
+*
+* Byte 0Byte 1
+* g2 g1 g0 b4 b3 b2 b1 b0 r4 r3 r2 r1 r0 g5 g4 g3
+*/
+ .description = RGB565, le,
+ .pixelformat = V4L2_PIX_FMT_RGB565,
+ },
+ {
+ /* Note: V4L2 defines RGB32 as: RGB-8-8-8-8 we use
+* this for RGB24 unpack mode, the last 8 bits are
ignored
+* */
+ .description = RGB32, le,
+ .pixelformat = V4L2_PIX_FMT_RGB32,
+ },
+ {
+ /* Note: V4L2 defines RGB24 as: RGB-8-8-8 we use
+*this for RGB24 packed mode
+*
+*/
+ .description = RGB24, le,
+ .pixelformat = V4L2_PIX_FMT_RGB24,
+ },
+ {
+ .description = YUYV (YUV 4:2:2), packed,
+ .pixelformat = V4L2_PIX_FMT_YUYV,
+ },
+ {
+ .description = UYVY, packed,
+ .pixelformat = V4L2_PIX_FMT_UYVY,
+ },
+};
+
+#define NUM_OUTPUT_FORMATS (ARRAY_SIZE(omap_formats))
+
+/*
+ * Allocate buffers
+ */
--
+
+/*
+ * omap_vout_uservirt_to_phys: This inline function is used to convert
user
+ * space virtual address to physical address.
+ */
+static u32 omap_vout_uservirt_to_phys(u32 virtp)
+{
+ unsigned long physp = 0;
+ struct vm_area_struct *vma;
+ struct mm_struct *mm = current-mm;
+
+ vma = find_vma(mm, virtp);
+ /* For kernel direct-mapped memory, take the easy way */
+ if (virtp = PAGE_OFFSET) {
+ physp = virt_to_phys((void *) virtp);
+ } else if (vma (vma-vm_flags VM_IO)
+vma-vm_pgoff) {
+ /* this will catch, kernel-allocated,
+ mmaped-to-usermode addresses */
+ physp = (vma-vm_pgoff PAGE_SHIFT) + (virtp - vma-
vm_start);
+ } else {
+ /* otherwise, use get_user_pages() for general userland
pages */
+ int res, nr_pages = 1;
+ struct page *pages;
+ down_read(current-mm-mmap_sem);
+
+ res = get_user_pages(current, current-mm, virtp,
nr_pages,
+ 1, 0, pages, NULL);
+
RE: [PATCH 1/2] OMAP2/3 V4L2: Add support for OMAP2/3 V4L2 driver on top of DSS2
Vaibhav,
[Hiremath, Vaibhav] Thanks Murali, I really appreciate your comments here.
Please find response below -
You had responded only to some comments. Can I assume that you are taking care
of the other comments as well? I have also asked Hans to provide
his comments since this is a new driver that he might have to approve. Did he
review the code in the past?
-Murali
+
+#include asm/processor.h
Add a line here??
+#include plat/dma.h
+#include plat/vram.h
+#include plat/vrfb.h
+#include plat/display.h
+
+#include omap_voutlib.h
+#include omap_voutdef.h
+
+MODULE_AUTHOR(Texas Instruments);
+MODULE_DESCRIPTION(OMAP Video for Linux Video out driver);
+MODULE_LICENSE(GPL);
+
+
+/* Driver Configuration macros */
+#define VOUT_NAME omap_vout
+
+enum omap_vout_channels {
+ OMAP_VIDEO1 = 0,
Why do we have to initialize this to 0. It always start with a value 0
by default.
+ OMAP_VIDEO2,
+};
+
+enum dma_channel_state {
+ DMA_CHAN_NOT_ALLOTED = 0,
Ditto.
+ DMA_CHAN_ALLOTED,
+};
+
+#define QQVGA_WIDTH160
+#define QQVGA_HEIGHT 120
+
+/* Max Resolution supported by the driver */
+#define VID_MAX_WIDTH 1280/* Largest width */
+#define VID_MAX_HEIGHT 720 /* Largest height */
+
-
+
+module_param(debug, bool, S_IRUGO);
+MODULE_PARM_DESC(debug, Debug level (0-1));
+
+/* Local Helper functions */
+static void omap_vout_isr(void *arg, unsigned int irqstatus);
+static void omap_vout_cleanup_device(struct omap_vout_device *vout);
+
Is there a reason why you need these prototypes? I think we could
remove these prototypes and move the function ahead in the file before
it is called.
[Hiremath, Vaibhav] Do you see any harm with this? I think its only
implementation part. But still I will incorporate in next version.
+/* list of image formats supported by OMAP2 video pipelines */
+const static struct v4l2_fmtdesc omap_formats[] = {
+ {
+ /* Note: V4L2 defines RGB565 as:
+*
+* Byte 0Byte 1
+* g2 g1 g0 r4 r3 r2 r1 r0 b4 b3 b2 b1 b0 g5 g4
g3
+*
+* We interpret RGB565 as:
+*
+* Byte 0Byte 1
+* g2 g1 g0 b4 b3 b2 b1 b0 r4 r3 r2 r1 r0 g5 g4
g3
+*/
+ .description = RGB565, le,
+ .pixelformat = V4L2_PIX_FMT_RGB565,
+ },
+ {
+ /* Note: V4L2 defines RGB32 as: RGB-8-8-8-8 we use
+* this for RGB24 unpack mode, the last 8 bits are
ignored
+* */
+ .description = RGB32, le,
+ .pixelformat = V4L2_PIX_FMT_RGB32,
+ },
+ {
+ /* Note: V4L2 defines RGB24 as: RGB-8-8-8 we use
+*this for RGB24 packed mode
+*
+*/
+ .description = RGB24, le,
+ .pixelformat = V4L2_PIX_FMT_RGB24,
+ },
+ {
+ .description = YUYV (YUV 4:2:2), packed,
+ .pixelformat = V4L2_PIX_FMT_YUYV,
+ },
+ {
+ .description = UYVY, packed,
+ .pixelformat = V4L2_PIX_FMT_UYVY,
+ },
+};
+
+#define NUM_OUTPUT_FORMATS (ARRAY_SIZE(omap_formats))
+
+/*
+ * Allocate buffers
+ */
--
+
+/*
+ * omap_vout_uservirt_to_phys: This inline function is used to convert
user
+ * space virtual address to physical address.
+ */
+static u32 omap_vout_uservirt_to_phys(u32 virtp)
+{
+ unsigned long physp = 0;
+ struct vm_area_struct *vma;
+ struct mm_struct *mm = current-mm;
+
+ vma = find_vma(mm, virtp);
+ /* For kernel direct-mapped memory, take the easy way */
+ if (virtp = PAGE_OFFSET) {
+ physp = virt_to_phys((void *) virtp);
+ } else if (vma (vma-vm_flags VM_IO)
+vma-vm_pgoff) {
+ /* this will catch, kernel-allocated,
+ mmaped-to-usermode addresses */
+ physp = (vma-vm_pgoff PAGE_SHIFT) + (virtp - vma-
vm_start);
+ } else {
+ /* otherwise, use get_user_pages() for general userland
pages */
+ int res, nr_pages = 1;
+ struct page *pages;
+ down_read(current-mm-mmap_sem);
+
+ res = get_user_pages(current, current-mm, virtp,
nr_pages,
+ 1, 0, pages, NULL);
+ up_read(current-mm-mmap_sem);
+
+ if (res == nr_pages) {
+ physp = __pa(page_address(pages[0]) +
+
