Re: [PATCH v2 1/6] V4L2: Add Renesas R-Car JPEG codec driver.

2014-09-26 Thread Laurent Pinchart
Hi Mikhail,

Thank you for the patch.

On Monday 25 August 2014 16:29:47 Mikhail Ulyanov wrote:
 This patch contains driver for Renesas R-Car JPEG codec.
 
 Cnanges since v1:
 - s/g_fmt function simplified
 - default format for queues added
 - dumb vidioc functions added to be in compliance with standard api:
 jpu_s_priority, jpu_g_priority
 - standard v4l2_ctrl_subscribe_event and v4l2_event_unsubscribe
   now in use by the same reason
 
 Signed-off-by: Mikhail Ulyanov mikhail.ulya...@cogentembedded.com
 ---
  drivers/media/platform/Kconfig  |   11 +
  drivers/media/platform/Makefile |2 +
  drivers/media/platform/jpu.c| 1628 
  3 files changed, 1641 insertions(+)
  create mode 100644 drivers/media/platform/jpu.c
 
 diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
 index 6d86646..1b8c846 100644
 --- a/drivers/media/platform/Kconfig
 +++ b/drivers/media/platform/Kconfig
 @@ -220,6 +220,17 @@ config VIDEO_RENESAS_VSP1
 To compile this driver as a module, choose M here: the module
 will be called vsp1.
 
 +config VIDEO_RENESAS_JPU
 + tristate Renesas JPEG Processing Unit
 + depends on VIDEO_DEV  VIDEO_V4L2
 + select VIDEOBUF2_DMA_CONTIG
 + select V4L2_MEM2MEM_DEV
 + ---help---
 +   This is a V4L2 driver for the Renesas JPEG Processing Unit.
 +
 +   To compile this driver as a module, choose M here: the module
 +   will be called jpu.
 +

You could move this above VIDEO_RENESAS_VSP1 to keep the file somehow sorted. 
Same comment for the Makefile.

  config VIDEO_TI_VPE
   tristate TI VPE (Video Processing Engine) driver
   depends on VIDEO_DEV  VIDEO_V4L2  SOC_DRA7XX
 diff --git a/drivers/media/platform/Makefile
 b/drivers/media/platform/Makefile index e5269da..e438534 100644
 --- a/drivers/media/platform/Makefile
 +++ b/drivers/media/platform/Makefile
 @@ -47,6 +47,8 @@ obj-$(CONFIG_SOC_CAMERA)+= soc_camera/
 
  obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1/
 
 +obj-$(CONFIG_VIDEO_RENESAS_JPU)  += jpu.o
 +
  obj-y+= davinci/
 
  obj-$(CONFIG_ARCH_OMAP)  += omap/
 diff --git a/drivers/media/platform/jpu.c b/drivers/media/platform/jpu.c
 new file mode 100644
 index 000..da70491
 --- /dev/null
 +++ b/drivers/media/platform/jpu.c
 @@ -0,0 +1,1628 @@
 +/*
 + * Author: Mikhail Ulyanov  sou...@cogentembedded.com
 + * Copyright (C) 2014 Cogent Embedded, Inc.
 + * Copyright (C) 2014 Renesas Electronics Corporation
 + *
 + * This is based on the drivers/media/platform/s5p-jpu driver by
 + * Andrzej Pietrasiewicz and Jacek Anaszewski.
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + */
 +
 +#include linux/clk.h
 +#include linux/err.h
 +#include linux/gfp.h

Do you really gfp.h ? slab.h should be enough.

 +#include linux/interrupt.h
 +#include linux/io.h
 +#include linux/kernel.h
 +#include linux/module.h
 +#include linux/of.h
 +#include linux/of_device.h
 +#include linux/platform_device.h
 +#include linux/slab.h
 +#include linux/spinlock.h
 +#include linux/string.h
 +#include linux/videodev2.h
 +#include media/v4l2-ctrls.h
 +#include media/v4l2-device.h
 +#include media/v4l2-event.h
 +#include media/v4l2-fh.h
 +#include media/v4l2-mem2mem.h
 +#include media/v4l2-ioctl.h
 +#include media/videobuf2-core.h
 +#include media/videobuf2-dma-contig.h
 +
 +
 +#define JPU_M2M_NAME jpu
 +
 +#define JPU_WIDTH_MIN16
 +#define JPU_HEIGHT_MIN   16
 +#define JPU_WIDTH_MAX4096
 +#define JPU_HEIGHT_MAX   4096
 +#define JPU_DEFAULT_WIDTH640
 +#define JPU_DEFAULT_HEIGHT   480
 +
 +#define JPU_ENCODE   0
 +#define JPU_DECODE   1

How about making this an enum, or turning the ctx-mode field into a boolean 
compress (or encoder, decode, decompress...) field ?

 +/* Flags that indicate a format can be used for capture/output */
 +#define JPU_FMT_TYPE_OUTPUT  0
 +#define JPU_FMT_TYPE_CAPTURE 1
 +#define JPU_ENC_CAPTURE  (1  0)
 +#define JPU_ENC_OUTPUT   (1  1)
 +#define JPU_DEC_CAPTURE  (1  2)
 +#define JPU_DEC_OUTPUT   (1  3)
 +
 +/*
 + * JPEG registers and bits
 + */
 +
 +/* JPEG code mode register */
 +#define JCMOD0x00
 +#define JCMOD_SOI_DISABLE(1  8)
 +#define JCMOD_SOI_ENABLE (0  8)
 +#define JCMOD_PCTR   (1  7)
 +#define JCMOD_MSKIP_DISABLE  (0  5)
 +#define JCMOD_MSKIP_ENABLE   (1  5)
 +#define JCMOD_DSP_ENC(0  3)
 +#define JCMOD_DSP_DEC(1  3)
 +#define JCMOD_REDU   (7  0)
 +#define JCMOD_REDU_422   (1  0)
 +#define JCMOD_REDU_420   (2  0)
 +
 +/* JPEG code command register */
 +#define JCCMD0x04
 +#define JCCMD_SRST   (1  12)
 +#define JCCMD_BRST   (1  7)
 +#define JCCMD_JEND   (1  2)

Re: [PATCH v2 1/6] V4L2: Add Renesas R-Car JPEG codec driver.

2014-09-25 Thread Sergei Shtylyov

Hello.

On 9/23/2014 5:31 PM, Kamil Debski wrote:


From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
ow...@vger.kernel.org] On Behalf Of Mikhail Ulyanov
Sent: Monday, August 25, 2014 2:30 PM



This patch contains driver for Renesas R-Car JPEG codec.



Cnanges since v1:
 - s/g_fmt function simplified
 - default format for queues added
 - dumb vidioc functions added to be in compliance with standard
api:
 jpu_s_priority, jpu_g_priority
 - standard v4l2_ctrl_subscribe_event and v4l2_event_unsubscribe
   now in use by the same reason



The patch looks good to me. However, I would suggest using the BIT macro


   I'd prefer not using it since the driver #define's not only bits but also bit
values and multi-bit fields. Using BIT() would look inconsistent in this 
situation.



and making some short functions inline.


   I think the current trend is to use *inline* only in the headers, and let 
gcc figure it out itself in the .c files.



Signed-off-by: Mikhail Ulyanov mikhail.ulya...@cogentembedded.com


[...]

diff --git a/drivers/media/platform/jpu.c
b/drivers/media/platform/jpu.c new file mode 100644 index
000..da70491
--- /dev/null
+++ b/drivers/media/platform/jpu.c
@@ -0,0 +1,1628 @@

[...]


Best wishes,


WBR, Sergei

--
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 v2 1/6] V4L2: Add Renesas R-Car JPEG codec driver.

2014-09-23 Thread Kamil Debski
Hi Mikhail,

 From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
 ow...@vger.kernel.org] On Behalf Of Mikhail Ulyanov
 Sent: Monday, August 25, 2014 2:30 PM
 
 This patch contains driver for Renesas R-Car JPEG codec.
 
 Cnanges since v1:
 - s/g_fmt function simplified
 - default format for queues added
 - dumb vidioc functions added to be in compliance with standard
 api:
 jpu_s_priority, jpu_g_priority
 - standard v4l2_ctrl_subscribe_event and v4l2_event_unsubscribe
   now in use by the same reason

The patch looks good to me. However, I would suggest using the BIT macro
and making some short functions inline.
 
 Signed-off-by: Mikhail Ulyanov mikhail.ulya...@cogentembedded.com
 ---
  drivers/media/platform/Kconfig  |   11 +
  drivers/media/platform/Makefile |2 +
  drivers/media/platform/jpu.c | 1628
 ++
  3 files changed, 1641 insertions(+)
  create mode 100644 drivers/media/platform/jpu.c
 
 diff --git a/drivers/media/platform/Kconfig
 b/drivers/media/platform/Kconfig index 6d86646..1b8c846 100644
 --- a/drivers/media/platform/Kconfig
 +++ b/drivers/media/platform/Kconfig
 @@ -220,6 +220,17 @@ config VIDEO_RENESAS_VSP1
 To compile this driver as a module, choose M here: the module
 will be called vsp1.
 
 +config VIDEO_RENESAS_JPU
 + tristate Renesas JPEG Processing Unit
 + depends on VIDEO_DEV  VIDEO_V4L2
 + select VIDEOBUF2_DMA_CONTIG
 + select V4L2_MEM2MEM_DEV
 + ---help---
 +   This is a V4L2 driver for the Renesas JPEG Processing Unit.
 +
 +   To compile this driver as a module, choose M here: the module
 +   will be called jpu.
 +
  config VIDEO_TI_VPE
   tristate TI VPE (Video Processing Engine) driver
   depends on VIDEO_DEV  VIDEO_V4L2  SOC_DRA7XX diff --git
 a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
 index e5269da..e438534 100644
 --- a/drivers/media/platform/Makefile
 +++ b/drivers/media/platform/Makefile
 @@ -47,6 +47,8 @@ obj-$(CONFIG_SOC_CAMERA)+= soc_camera/
 
  obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1/
 
 +obj-$(CONFIG_VIDEO_RENESAS_JPU)  += jpu.o
 +
  obj-y+= davinci/
 
  obj-$(CONFIG_ARCH_OMAP)  += omap/
 diff --git a/drivers/media/platform/jpu.c
 b/drivers/media/platform/jpu.c new file mode 100644 index
 000..da70491
 --- /dev/null
 +++ b/drivers/media/platform/jpu.c
 @@ -0,0 +1,1628 @@
 +/*
 + * Author: Mikhail Ulyanov  sou...@cogentembedded.com
 + * Copyright (C) 2014 Cogent Embedded, Inc.
 + * Copyright (C) 2014 Renesas Electronics Corporation
 + *
 + * This is based on the drivers/media/platform/s5p-jpu driver by
 + * Andrzej Pietrasiewicz and Jacek Anaszewski.
 + *
 + * This program is free software; you can redistribute it and/or
 modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + */
 +
 +#include linux/clk.h
 +#include linux/err.h
 +#include linux/gfp.h
 +#include linux/interrupt.h
 +#include linux/io.h
 +#include linux/kernel.h
 +#include linux/module.h
 +#include linux/of.h
 +#include linux/of_device.h
 +#include linux/platform_device.h
 +#include linux/slab.h
 +#include linux/spinlock.h
 +#include linux/string.h
 +#include linux/videodev2.h
 +#include media/v4l2-ctrls.h
 +#include media/v4l2-device.h
 +#include media/v4l2-event.h
 +#include media/v4l2-fh.h
 +#include media/v4l2-mem2mem.h
 +#include media/v4l2-ioctl.h
 +#include media/videobuf2-core.h
 +#include media/videobuf2-dma-contig.h
 +
 +
 +#define JPU_M2M_NAME jpu
 +
 +#define JPU_WIDTH_MIN16
 +#define JPU_HEIGHT_MIN   16
 +#define JPU_WIDTH_MAX4096
 +#define JPU_HEIGHT_MAX   4096
 +#define JPU_DEFAULT_WIDTH640
 +#define JPU_DEFAULT_HEIGHT   480
 +
 +#define JPU_ENCODE   0
 +#define JPU_DECODE   1
 +
 +/* Flags that indicate a format can be used for capture/output */
 +#define JPU_FMT_TYPE_OUTPUT  0
 +#define JPU_FMT_TYPE_CAPTURE 1
 +#define JPU_ENC_CAPTURE  (1  0)
 +#define JPU_ENC_OUTPUT   (1  1)
 +#define JPU_DEC_CAPTURE  (1  2)
 +#define JPU_DEC_OUTPUT   (1  3)

The BIT macro could be used here.

 +
 +/*
 + * JPEG registers and bits
 + */
 +
 +/* JPEG code mode register */
 +#define JCMOD0x00
 +#define JCMOD_SOI_DISABLE(1  8)
 +#define JCMOD_SOI_ENABLE (0  8)
 +#define JCMOD_PCTR   (1  7)
 +#define JCMOD_MSKIP_DISABLE  (0  5)
 +#define JCMOD_MSKIP_ENABLE   (1  5)
 +#define JCMOD_DSP_ENC(0  3)
 +#define JCMOD_DSP_DEC(1  3)
 +#define JCMOD_REDU   (7  0)
 +#define JCMOD_REDU_422   (1  0)
 +#define JCMOD_REDU_420   (2  0)
 +
 +/* JPEG code command register */
 +#define JCCMD0x04
 +#define JCCMD_SRST   (1  12)
 +#define JCCMD_BRST   (1  7)
 +#define JCCMD_JEND   (1  2)
 +#define JCCMD_JSRT   (1  0)
 +
 +/* JPEG code 

Re: [PATCH v2 1/6] V4L2: Add Renesas R-Car JPEG codec driver.

2014-09-21 Thread Sergei Shtylyov

Hello.

On 08/25/2014 04:49 PM, Hans Verkuil wrote:


This patch contains driver for Renesas R-Car JPEG codec.



Cnanges since v1:
 - s/g_fmt function simplified
 - default format for queues added
 - dumb vidioc functions added to be in compliance with standard api:
 jpu_s_priority, jpu_g_priority



Oops, that's a bug elsewhere. Don't add these empty prio ops, this needs to be
solved in the v4l2 core.



I'll post a patch for this.



After some thought I've decided to allow prio handling for m2m devices. It is
actually useful if some application wants exclusive access to the m2m hardware.



So I will change v4l2-compliance instead.


   I take it we don't need to change the driver? Asking because the driver 
seems stuck for nearly a month now.
   I'm myself still seeing a place for improvement (register macro naming of 
the top of my head). Perhaps it's time to take this driver into my own hands...



Regards,
Hans


WBR, Sergei

--
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 v2 1/6] V4L2: Add Renesas R-Car JPEG codec driver.

2014-09-21 Thread Sergei Shtylyov

Hello.

On 08/25/2014 04:49 PM, Hans Verkuil wrote:


This patch contains driver for Renesas R-Car JPEG codec.



Cnanges since v1:
 - s/g_fmt function simplified
 - default format for queues added
 - dumb vidioc functions added to be in compliance with standard api:
 jpu_s_priority, jpu_g_priority



Oops, that's a bug elsewhere. Don't add these empty prio ops, this needs to be
solved in the v4l2 core.



I'll post a patch for this.



After some thought I've decided to allow prio handling for m2m devices. It is
actually useful if some application wants exclusive access to the m2m hardware.



So I will change v4l2-compliance instead.


   I take it we don't need to change the driver? Asking because the driver 
seems stuck for nearly a months now.
   I'm myself still seeing a place for improvement (register macro naming of 
the top of my head). Perhaps it's time to take this driver into my own hands...



Regards,
Hans


WBR, Sergei

--
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 v2 1/6] V4L2: Add Renesas R-Car JPEG codec driver.

2014-08-25 Thread Hans Verkuil
On 08/25/2014 02:29 PM, Mikhail Ulyanov wrote:
 This patch contains driver for Renesas R-Car JPEG codec.
 
 Cnanges since v1:
 - s/g_fmt function simplified
 - default format for queues added
 - dumb vidioc functions added to be in compliance with standard api:
 jpu_s_priority, jpu_g_priority

Oops, that's a bug elsewhere. Don't add these empty prio ops, this needs to be
solved in the v4l2 core.

I'll post a patch for this.

Regards,

Hans

 - standard v4l2_ctrl_subscribe_event and v4l2_event_unsubscribe
   now in use by the same reason

--
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 v2 1/6] V4L2: Add Renesas R-Car JPEG codec driver.

2014-08-25 Thread Hans Verkuil
On 08/25/2014 02:39 PM, Hans Verkuil wrote:
 On 08/25/2014 02:29 PM, Mikhail Ulyanov wrote:
 This patch contains driver for Renesas R-Car JPEG codec.

 Cnanges since v1:
 - s/g_fmt function simplified
 - default format for queues added
 - dumb vidioc functions added to be in compliance with standard api:
 jpu_s_priority, jpu_g_priority
 
 Oops, that's a bug elsewhere. Don't add these empty prio ops, this needs to be
 solved in the v4l2 core.
 
 I'll post a patch for this.

After some thought I've decided to allow prio handling for m2m devices. It is
actually useful if some application wants exclusive access to the m2m hardware.

So I will change v4l2-compliance instead.

Regards,

Hans
--
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