Hi Mikhail,
Thank you for the patch!
I have one high-level comment: please rename the source to r-car-jpu.c. It's
good practice to start with the SoC name since 'jpu' by itself is not very
descriptive.
I have a few more comments (mostly easy ones) below:
On 04/29/2015 11:53 PM, Mikhail Ulyanov wrote:
> Here's the the driver for the Renesas R-Car JPEG processing unit driver.
>
> The driver is implemented within the V4L2 framework as a mem-to-mem device. It
> presents two video nodes to userspace, one for the encoding part, and one for
> the decoding part.
>
> It was found that the only working mode for encoding is no markers output, so we
> generate it with software. In current version of driver we also use software
> JPEG header parsing because with hardware parsing performance is lower then
> desired.
>
> From a userspace point of view the encoding process is typical (S_FMT, REQBUF,
> optionally QUERYBUF, QBUF, STREAMON, DQBUF) for both the source and destination
> queues. The decoding process requires that the source queue performs S_FMT,
> REQBUF, (QUERYBUF), QBUF and STREAMON. After STREAMON on the source queue, it is
> possible to perform G_FMT on the destination queue to find out the processed
> image width and height in order to be able to allocate an appropriate buffer -
> it is assumed that the user does not pass the compressed image width and height
> but instead this information is parsed from the JPEG input. This is done in
> kernel. Then REQBUF, QBUF and STREAMON on the destination queue complete the
> decoding and it is possible to DQBUF from both queues and finish the operation.
>
> During encoding the available formats are: V4L2_PIX_FMT_NV12M and
> V4L2_PIX_FMT_NV16M for source and V4L2_PIX_FMT_JPEG for destination.
>
> During decoding the available formats are: V4L2_PIX_FMT_JPEG for source and
> V4L2_PIX_FMT_NV12M and V4L2_PIX_FMT_NV16M for destination.
>
> Performance of current version:
> 1280x800 NV12 image encoding/decoding
> decoding ~121 FPS
> encoding ~190 FPS
>
> Signed-off-by: Mikhail Ulyanov <mikhail.ulyanov@xxxxxxxxxxxxxxxxxx>
> ---
> Changes since v2:
> - Kconfig entry reordered
> - unnecessary clk_disable_unprepare(jpu->clk) removed
> - ref_count fixed in jpu_resume
> - enable DMABUF in src_vq->io_modes
> - remove jpu_s_priority jpu_g_priority
> - jpu_g_selection fixed
> - timeout in jpu_reset added and hardware reset reworked
> - remove unused macros
> - JPEG header parsing now is software because of performance issues
> based on s5p-jpu code
> - JPEG header generation redesigned:
> JPEG header(s) pre-generated and memcpy'ed on encoding
> we only fill the necessary fields
> more "transparent" header format description
> - S_FMT, G_FMT and TRY_FMT hooks redesigned
> partially inspired by VSP1 driver code
> - some code was reformatted
> - image formats handling redesigned
> - multi-planar V4L2 API now in use
> - now passes v4l2-compliance tool check
>
> 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
>
> drivers/media/platform/Kconfig | 11 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/jpu.c | 1724 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1736 insertions(+)
> create mode 100644 drivers/media/platform/jpu.c
>
<snip>
> diff --git a/drivers/media/platform/jpu.c b/drivers/media/platform/jpu.c
> new file mode 100644
> index 0000000..6c658cc
> --- /dev/null
> +++ b/drivers/media/platform/jpu.c
<snip>
> +/**
> + * struct jpu - JPEG IP abstraction
> + * @mutex: the mutex protecting this structure
> + * @lock: spinlock protecting the device contexts
> + * @v4l2_dev: v4l2 device for mem2mem mode
> + * @vfd_encoder: video device node for encoder mem2mem mode
> + * @vfd_decoder: video device node for decoder mem2mem mode
> + * @m2m_dev: v4l2 mem2mem device data
> + * @regs: JPEG IP registers mapping
> + * @irq: JPEG IP irq
> + * @clk: JPEG IP clock
> + * @dev: JPEG IP struct device
> + * @alloc_ctx: videobuf2 memory allocator's context
> + * @ref_counter: reference counter
> + */
> +struct jpu {
> + struct mutex mutex;
> + spinlock_t lock;
> + struct v4l2_device v4l2_dev;
> + struct video_device *vfd_encoder;
> + struct video_device *vfd_decoder;
Please just embed these video_device structs (so remove the '*'). This means
that the release callback of each video_device should be set to
video_device_release_empty() and that the video_device_alloc/release can be
removed.
The use of video_device_alloc/release is deprecated and will eventually
disappear.
> + struct v4l2_m2m_dev *m2m_dev;
> +
> + void __iomem *regs;
> + unsigned int irq;
> + struct clk *clk;
> + struct device *dev;
> + void *alloc_ctx;
> + int ref_count;
> +};
> +
<snip>
> +static struct jpu_fmt jpu_formats[] = {
> + { "JPEG JFIF", V4L2_PIX_FMT_JPEG, V4L2_COLORSPACE_JPEG,
> + {0, 0}, 0, 0, 0, 1, JPU_ENC_CAPTURE | JPU_DEC_OUTPUT },
> + { "YUV 4:2:2 planar, Y/CbCr", V4L2_PIX_FMT_NV16M, V4L2_COLORSPACE_SRGB,
> + {8, 16}, 2, 2, JPU_JPEG_422, 2, JPU_ENC_OUTPUT | JPU_DEC_CAPTURE },
> + { "YUV 4:2:0 planar, Y/CbCr", V4L2_PIX_FMT_NV12M, V4L2_COLORSPACE_SRGB,
> + {8, 16}, 2, 2, JPU_JPEG_420, 2, JPU_ENC_OUTPUT | JPU_DEC_CAPTURE }
> +};
Drop the 'name' field: the v4l2 core will now fill in the format description
based on the fourcc value. This change was merged a few days ago in the media_tree
repo.
<snip>
> +static int jpu_enum_fmt(struct v4l2_fmtdesc *f, u32 type)
> +{
> + unsigned int i, num = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(jpu_formats); ++i) {
> + if (jpu_formats[i].types & type) {
> + if (num == f->index)
> + break;
> + ++num;
> + }
> + }
> +
> + if (i >= ARRAY_SIZE(jpu_formats))
> + return -EINVAL;
> +
> + strlcpy(f->description, jpu_formats[i].name, sizeof(f->description));
So this line can be dropped as well.
> + f->pixelformat = jpu_formats[i].fourcc;
> +
> + return 0;
> +}
<snip>
> +static int jpu_g_selection(struct file *file, void *priv,
> + struct v4l2_selection *s)
> +{
> + struct jpu_ctx *ctx = fh_to_ctx(priv);
> +
> + switch (s->target) {
> + case V4L2_SEL_TGT_CROP:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + case V4L2_SEL_TGT_COMPOSE:
> + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> + s->r.width = ctx->out_q.format.width;
> + s->r.height = ctx->out_q.format.height;
> + break;
> + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> + case V4L2_SEL_TGT_COMPOSE_PADDED:
> + s->r.width = ctx->cap_q.format.width;
> + s->r.height = ctx->cap_q.format.height;
> + break;
> + default:
> + return -EINVAL;
> + }
> + s->r.left = 0;
> + s->r.top = 0;
> + return 0;
> +}
Why do you implement g_selection? You cannot crop or compose, so what's the point?
The code is wrong anyway since it does not check s->type. CROP for a CAPTURE vs an
OUTPUT buffer should result in different values.
<snip>
> +static int jpu_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> + return 0;
> +}
> +
> +static void jpu_stop_streaming(struct vb2_queue *q)
> +{
> +}
You can drop these empty start/stop functions.
> +
> +static struct vb2_ops jpu_qops = {
> + .queue_setup = jpu_queue_setup,
> + .buf_prepare = jpu_buf_prepare,
> + .buf_queue = jpu_buf_queue,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> + .start_streaming = jpu_start_streaming,
> + .stop_streaming = jpu_stop_streaming,
> +};
> +
<snip>
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
