Hi,
This series contains various fixes and improvements for the Samsung
s5p-jpeg driver. All these patches come from the Chromium v3.8 kernel
tree.
In this v2:
- Remove IOMMU support patch (mapping now created automatically for
single JPEG CODEC device).
- Remove "Change sclk_jpeg to 166MHz" patch (can be set through DT
properties).
- Remove support for multi-planar APIs (Not needed).
- Add comment regarding call to jpeg_bound_align_image() after qbuf.
- Remove unrelated code from resolution change event support patch.
Regards,
Thierry
Abhilash Kesavan (1):
[media] s5p-jpeg: Reset the Codec before doing a soft reset
Tony K Nadackal (3):
[media] s5p-jpeg: Call jpeg_bound_align_image after qbuf
[media] s5p-jpeg: Correct WARN_ON statement for checking subsampling
[media] s5p-jpeg: Decode 4:1:1 chroma subsampling format
henryhsu (2):
[media] s5p-jpeg: Add support for resolution change event
[media] s5p-jpeg: Add stream error handling for Exynos5420
drivers/media/platform/s5p-jpeg/jpeg-core.c | 140 +++++++++++++++++-----
drivers/media/platform/s5p-jpeg/jpeg-core.h | 7 ++
drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c | 4 +
3 files changed, 122 insertions(+), 29 deletions(-)
--
2.7.4
From: Tony K Nadackal <[email protected]>
When queuing an OUTPUT buffer for decoder, s5p_jpeg_parse_hdr()
function parses the input jpeg file and takes the width and height
parameters from its header. These new width/height values will be used
for the calculation of stride. HX_JPEG Hardware needs the width and
height values aligned on a 16 bits boundary. This width/height alignment
is handled in the s5p_jpeg_s_fmt_vid_cap() function during the S_FMT
ioctl call.
But if user space calls the QBUF of OUTPUT buffer after the S_FMT of
CAPTURE buffer, these aligned values will be replaced by the values in
jpeg header. If the width/height values of jpeg are not aligned, the
decoder output will be corrupted. So in this patch we call
jpeg_bound_align_image() to align the width/height values of Capture
buffer in s5p_jpeg_buf_queue().
Signed-off-by: Tony K Nadackal <[email protected]>
Signed-off-by: Thierry Escande <[email protected]>
---
drivers/media/platform/s5p-jpeg/jpeg-core.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 52dc794..623508d 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2523,6 +2523,25 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
q_data = &ctx->cap_q;
q_data->w = tmp.w;
q_data->h = tmp.h;
+
+ /*
+ * This call to jpeg_bound_align_image() takes care of width and
+ * height values alignment when user space calls the QBUF of
+ * OUTPUT buffer after the S_FMT of CAPTURE buffer.
+ * Please note that on Exynos4x12 SoCs, resigning from executing
+ * S_FMT on capture buffer for each JPEG image can result in a
+ * hardware hangup if subsampling is lower than the one of input
+ * JPEG.
+ */
+ jpeg_bound_align_image(ctx,
+ &q_data->w,
+ S5P_JPEG_MIN_WIDTH, S5P_JPEG_MAX_WIDTH,
+ q_data->fmt->h_align,
+ &q_data->h,
+ S5P_JPEG_MIN_HEIGHT, S5P_JPEG_MAX_HEIGHT,
+ q_data->fmt->v_align);
+
+ q_data->size = q_data->w * q_data->h * q_data->fmt->depth >> 3;
}
v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
--
2.7.4
From: henryhsu <[email protected]>
This patch adds support for resolution change event to notify clients so
they can prepare correct output buffer. When resolution change happened,
G_FMT for CAPTURE should return old resolution and format before CAPTURE
queues streamoff.
Signed-off-by: Henry-Ruey Hsu <[email protected]>
Signed-off-by: Thierry Escande <[email protected]>
---
drivers/media/platform/s5p-jpeg/jpeg-core.c | 125 +++++++++++++++++++---------
drivers/media/platform/s5p-jpeg/jpeg-core.h | 7 ++
2 files changed, 91 insertions(+), 41 deletions(-)
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 7ef7173..3d90a63 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -24,6 +24,7 @@
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/string.h>
+#include <media/v4l2-event.h>
#include <media/v4l2-mem2mem.h>
#include <media/v4l2-ioctl.h>
#include <media/videobuf2-v4l2.h>
@@ -1611,8 +1612,6 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct v4l2_format *f)
FMT_TYPE_OUTPUT : FMT_TYPE_CAPTURE;
q_data->fmt = s5p_jpeg_find_format(ct, pix->pixelformat, f_type);
- q_data->w = pix->width;
- q_data->h = pix->height;
if (q_data->fmt->fourcc != V4L2_PIX_FMT_JPEG) {
/*
* During encoding Exynos4x12 SoCs access wider memory area
@@ -1620,6 +1619,8 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct v4l2_format *f)
* the JPEG_IMAGE_SIZE register. In order to avoid sysmmu
* page fault calculate proper buffer size in such a case.
*/
+ q_data->w = pix->width;
+ q_data->h = pix->height;
if (ct->jpeg->variant->hw_ex4_compat &&
f_type == FMT_TYPE_OUTPUT && ct->mode == S5P_JPEG_ENCODE)
q_data->size = exynos4_jpeg_get_output_buffer_size(ct,
@@ -1695,6 +1696,15 @@ static int s5p_jpeg_s_fmt_vid_out(struct file *file, void *priv,
return s5p_jpeg_s_fmt(fh_to_ctx(priv), f);
}
+static int s5p_jpeg_subscribe_event(struct v4l2_fh *fh,
+ const struct v4l2_event_subscription *sub)
+{
+ if (sub->type == V4L2_EVENT_SOURCE_CHANGE)
+ return v4l2_src_change_event_subscribe(fh, sub);
+
+ return -EINVAL;
+}
+
static int exynos3250_jpeg_try_downscale(struct s5p_jpeg_ctx *ctx,
struct v4l2_rect *r)
{
@@ -2020,6 +2030,9 @@ static const struct v4l2_ioctl_ops s5p_jpeg_ioctl_ops = {
.vidioc_g_selection = s5p_jpeg_g_selection,
.vidioc_s_selection = s5p_jpeg_s_selection,
+
+ .vidioc_subscribe_event = s5p_jpeg_subscribe_event,
+ .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
};
/*
@@ -2412,8 +2425,17 @@ static int s5p_jpeg_job_ready(void *priv)
{
struct s5p_jpeg_ctx *ctx = priv;
- if (ctx->mode == S5P_JPEG_DECODE)
+ if (ctx->mode == S5P_JPEG_DECODE) {
+ /*
+ * We have only one input buffer and one output buffer. If there
+ * is a resolution change event, no need to continue decoding.
+ */
+ if (ctx->state == JPEGCTX_RESOLUTION_CHANGE)
+ return 0;
+
return ctx->hdr_parsed;
+ }
+
return 1;
}
@@ -2492,6 +2514,30 @@ static int s5p_jpeg_buf_prepare(struct vb2_buffer *vb)
return 0;
}
+static void s5p_jpeg_set_capture_queue_data(struct s5p_jpeg_ctx *ctx)
+{
+ struct s5p_jpeg_q_data *q_data = &ctx->cap_q;
+
+ q_data->w = ctx->out_q.w;
+ q_data->h = ctx->out_q.h;
+
+ /*
+ * This call to jpeg_bound_align_image() takes care of width and
+ * height values alignment when user space calls the QBUF of
+ * OUTPUT buffer after the S_FMT of CAPTURE buffer.
+ * Please note that on Exynos4x12 SoCs, resigning from executing
+ * S_FMT on capture buffer for each JPEG image can result in a
+ * hardware hangup if subsampling is lower than the one of input
+ * JPEG.
+ */
+ jpeg_bound_align_image(ctx, &q_data->w, S5P_JPEG_MIN_WIDTH,
+ S5P_JPEG_MAX_WIDTH, q_data->fmt->h_align,
+ &q_data->h, S5P_JPEG_MIN_HEIGHT,
+ S5P_JPEG_MAX_HEIGHT, q_data->fmt->v_align);
+
+ q_data->size = q_data->w * q_data->h * q_data->fmt->depth >> 3;
+}
+
static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
{
struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
@@ -2499,9 +2545,20 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
if (ctx->mode == S5P_JPEG_DECODE &&
vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
- struct s5p_jpeg_q_data tmp, *q_data;
-
- ctx->hdr_parsed = s5p_jpeg_parse_hdr(&tmp,
+ static const struct v4l2_event ev_src_ch = {
+ .type = V4L2_EVENT_SOURCE_CHANGE,
+ .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
+ };
+ struct vb2_queue *dst_vq;
+ u32 ori_w;
+ u32 ori_h;
+
+ dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
+ V4L2_BUF_TYPE_VIDEO_CAPTURE);
+ ori_w = ctx->out_q.w;
+ ori_h = ctx->out_q.h;
+
+ ctx->hdr_parsed = s5p_jpeg_parse_hdr(&ctx->out_q,
(unsigned long)vb2_plane_vaddr(vb, 0),
min((unsigned long)ctx->out_q.size,
vb2_get_plane_payload(vb, 0)), ctx);
@@ -2510,43 +2567,18 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
return;
}
- q_data = &ctx->out_q;
- q_data->w = tmp.w;
- q_data->h = tmp.h;
- q_data->sos = tmp.sos;
- memcpy(q_data->dht.marker, tmp.dht.marker,
- sizeof(tmp.dht.marker));
- memcpy(q_data->dht.len, tmp.dht.len, sizeof(tmp.dht.len));
- q_data->dht.n = tmp.dht.n;
- memcpy(q_data->dqt.marker, tmp.dqt.marker,
- sizeof(tmp.dqt.marker));
- memcpy(q_data->dqt.len, tmp.dqt.len, sizeof(tmp.dqt.len));
- q_data->dqt.n = tmp.dqt.n;
- q_data->sof = tmp.sof;
- q_data->sof_len = tmp.sof_len;
-
- q_data = &ctx->cap_q;
- q_data->w = tmp.w;
- q_data->h = tmp.h;
-
/*
- * This call to jpeg_bound_align_image() takes care of width and
- * height values alignment when user space calls the QBUF of
- * OUTPUT buffer after the S_FMT of CAPTURE buffer.
- * Please note that on Exynos4x12 SoCs, resigning from executing
- * S_FMT on capture buffer for each JPEG image can result in a
- * hardware hangup if subsampling is lower than the one of input
- * JPEG.
+ * If there is a resolution change event, only update capture
+ * queue when it is not streaming. Otherwise, update it in
+ * STREAMOFF. See s5p_jpeg_stop_streaming for detail.
*/
- jpeg_bound_align_image(ctx,
- &q_data->w,
- S5P_JPEG_MIN_WIDTH, S5P_JPEG_MAX_WIDTH,
- q_data->fmt->h_align,
- &q_data->h,
- S5P_JPEG_MIN_HEIGHT, S5P_JPEG_MAX_HEIGHT,
- q_data->fmt->v_align);
-
- q_data->size = q_data->w * q_data->h * q_data->fmt->depth >> 3;
+ if (ctx->out_q.w != ori_w || ctx->out_q.h != ori_h) {
+ v4l2_event_queue_fh(&ctx->fh, &ev_src_ch);
+ if (vb2_is_streaming(dst_vq))
+ ctx->state = JPEGCTX_RESOLUTION_CHANGE;
+ else
+ s5p_jpeg_set_capture_queue_data(ctx);
+ }
}
v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
@@ -2566,6 +2598,17 @@ static void s5p_jpeg_stop_streaming(struct vb2_queue *q)
{
struct s5p_jpeg_ctx *ctx = vb2_get_drv_priv(q);
+ /*
+ * STREAMOFF is an acknowledgment for resolution change event.
+ * Before STREAMOFF, we still have to return the old resolution and
+ * subsampling. Update capture queue when the stream is off.
+ */
+ if (ctx->state == JPEGCTX_RESOLUTION_CHANGE &&
+ q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+ s5p_jpeg_set_capture_queue_data(ctx);
+ ctx->state = JPEGCTX_RUNNING;
+ }
+
pm_runtime_put(ctx->jpeg->dev);
}
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.h b/drivers/media/platform/s5p-jpeg/jpeg-core.h
index 4492a35..9aa26bd 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.h
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.h
@@ -98,6 +98,11 @@ enum exynos4_jpeg_img_quality_level {
QUALITY_LEVEL_4, /* low */
};
+enum s5p_jpeg_ctx_state {
+ JPEGCTX_RUNNING = 0,
+ JPEGCTX_RESOLUTION_CHANGE,
+};
+
/**
* struct s5p_jpeg - JPEG IP abstraction
* @lock: the mutex protecting this structure
@@ -220,6 +225,7 @@ struct s5p_jpeg_q_data {
* @hdr_parsed: set if header has been parsed during decompression
* @crop_altered: set if crop rectangle has been altered by the user space
* @ctrl_handler: controls handler
+ * @state: state of the context
*/
struct s5p_jpeg_ctx {
struct s5p_jpeg *jpeg;
@@ -235,6 +241,7 @@ struct s5p_jpeg_ctx {
bool hdr_parsed;
bool crop_altered;
struct v4l2_ctrl_handler ctrl_handler;
+ enum s5p_jpeg_ctx_state state;
};
/**
--
2.7.4
From: Tony K Nadackal <[email protected]>
This patch adds support for decoding 4:1:1 chroma subsampling in the
jpeg header parsing function.
Signed-off-by: Tony K Nadackal <[email protected]>
Signed-off-by: Thierry Escande <[email protected]>
---
drivers/media/platform/s5p-jpeg/jpeg-core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 0d935f5..7ef7173 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1236,6 +1236,9 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
case 0x33:
ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY;
break;
+ case 0x41:
+ ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_411;
+ break;
default:
return false;
}
--
2.7.4
From: Abhilash Kesavan <[email protected]>
This patch resets the encoding and decoding register bits before doing a
soft reset.
Signed-off-by: Tony K Nadackal <[email protected]>
Signed-off-by: Thierry Escande <[email protected]>
---
drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c
index a1d823a..9ad8f6d 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c
@@ -21,6 +21,10 @@ void exynos4_jpeg_sw_reset(void __iomem *base)
unsigned int reg;
reg = readl(base + EXYNOS4_JPEG_CNTL_REG);
+ writel(reg & ~(EXYNOS4_DEC_MODE | EXYNOS4_ENC_MODE),
+ base + EXYNOS4_JPEG_CNTL_REG);
+
+ reg = readl(base + EXYNOS4_JPEG_CNTL_REG);
writel(reg & ~EXYNOS4_SOFT_RESET_HI, base + EXYNOS4_JPEG_CNTL_REG);
udelay(100);
--
2.7.4
From: Tony K Nadackal <[email protected]>
Corrects the WARN_ON statement for subsampling based on the
JPEG Hardware version.
Signed-off-by: Tony K Nadackal <[email protected]>
Signed-off-by: Thierry Escande <[email protected]>
---
drivers/media/platform/s5p-jpeg/jpeg-core.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 623508d..0d935f5 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -614,24 +614,26 @@ static inline struct s5p_jpeg_ctx *fh_to_ctx(struct v4l2_fh *fh)
static int s5p_jpeg_to_user_subsampling(struct s5p_jpeg_ctx *ctx)
{
- WARN_ON(ctx->subsampling > 3);
-
switch (ctx->jpeg->variant->version) {
case SJPEG_S5P:
+ WARN_ON(ctx->subsampling > 3);
if (ctx->subsampling > 2)
return V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY;
return ctx->subsampling;
case SJPEG_EXYNOS3250:
case SJPEG_EXYNOS5420:
+ WARN_ON(ctx->subsampling > 6);
if (ctx->subsampling > 3)
return V4L2_JPEG_CHROMA_SUBSAMPLING_411;
return exynos3250_decoded_subsampling[ctx->subsampling];
case SJPEG_EXYNOS4:
case SJPEG_EXYNOS5433:
+ WARN_ON(ctx->subsampling > 3);
if (ctx->subsampling > 2)
return V4L2_JPEG_CHROMA_SUBSAMPLING_420;
return exynos4x12_decoded_subsampling[ctx->subsampling];
default:
+ WARN_ON(ctx->subsampling > 3);
return V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY;
}
}
--
2.7.4
From: henryhsu <[email protected]>
On Exynos5420, the STREAM_STAT bit raised on the JPGINTST register means
there is a syntax error or an unrecoverable error on compressed file
when ERR_INT_EN is set to 1.
Fix this case and report BUF_STATE_ERROR to videobuf2.
Signed-off-by: Henry-Ruey Hsu <[email protected]>
Signed-off-by: Thierry Escande <[email protected]>
---
drivers/media/platform/s5p-jpeg/jpeg-core.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 3d90a63..1a07a82 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2790,6 +2790,7 @@ static irqreturn_t exynos3250_jpeg_irq(int irq, void *dev_id)
unsigned long payload_size = 0;
enum vb2_buffer_state state = VB2_BUF_STATE_DONE;
bool interrupt_timeout = false;
+ bool stream_error = false;
u32 irq_status;
spin_lock(&jpeg->slock);
@@ -2806,6 +2807,11 @@ static irqreturn_t exynos3250_jpeg_irq(int irq, void *dev_id)
jpeg->irq_status |= irq_status;
+ if (irq_status & EXYNOS3250_STREAM_STAT) {
+ stream_error = true;
+ dev_err(jpeg->dev, "Syntax error or unrecoverable error occurred.\n");
+ }
+
curr_ctx = v4l2_m2m_get_curr_priv(jpeg->m2m_dev);
if (!curr_ctx)
@@ -2822,7 +2828,7 @@ static irqreturn_t exynos3250_jpeg_irq(int irq, void *dev_id)
EXYNOS3250_RDMA_DONE |
EXYNOS3250_RESULT_STAT))
payload_size = exynos3250_jpeg_compressed_size(jpeg->regs);
- else if (interrupt_timeout)
+ else if (interrupt_timeout || stream_error)
state = VB2_BUF_STATE_ERROR;
else
goto exit_unlock;
--
2.7.4
Hi Thierry,
Thank you for the patch. Please see inline.
W dniu 12.06.2017 o 19:13, Thierry Escande pisze:
> From: henryhsu <[email protected]>
>
> On Exynos5420, the STREAM_STAT bit raised on the JPGINTST register means
> there is a syntax error or an unrecoverable error on compressed file
> when ERR_INT_EN is set to 1.
>
> Fix this case and report BUF_STATE_ERROR to videobuf2.
>
> Signed-off-by: Henry-Ruey Hsu <[email protected]>
> Signed-off-by: Thierry Escande <[email protected]>
> ---
> drivers/media/platform/s5p-jpeg/jpeg-core.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 3d90a63..1a07a82 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -2790,6 +2790,7 @@ static irqreturn_t exynos3250_jpeg_irq(int irq, void *dev_id)
> unsigned long payload_size = 0;
> enum vb2_buffer_state state = VB2_BUF_STATE_DONE;
> bool interrupt_timeout = false;
> + bool stream_error = false;
> u32 irq_status;
>
> spin_lock(&jpeg->slock);
> @@ -2806,6 +2807,11 @@ static irqreturn_t exynos3250_jpeg_irq(int irq, void *dev_id)
>
> jpeg->irq_status |= irq_status;
>
> + if (irq_status & EXYNOS3250_STREAM_STAT) {
If the problem which is supposed to be fixed happens on 5420,
then why the 3250 variant is also affected by this patch?
Shouldn't jpeg->variant->version be checked and equal SJPEG_EXYNOS5420?
Andrzej
Hi Thierry,
Thank you for the patch.
Can you give a use case for resolution change event?
Also plase see inline.
W dniu 12.06.2017 o 19:13, Thierry Escande pisze:
> From: henryhsu <[email protected]>
>
> This patch adds support for resolution change event to notify clients so
> they can prepare correct output buffer. When resolution change happened,
> G_FMT for CAPTURE should return old resolution and format before CAPTURE
> queues streamoff.
>
> Signed-off-by: Henry-Ruey Hsu <[email protected]>
> Signed-off-by: Thierry Escande <[email protected]>
> ---
> drivers/media/platform/s5p-jpeg/jpeg-core.c | 125 +++++++++++++++++++---------
> drivers/media/platform/s5p-jpeg/jpeg-core.h | 7 ++
> 2 files changed, 91 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 7ef7173..3d90a63 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -24,6 +24,7 @@
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/string.h>
> +#include <media/v4l2-event.h>
> #include <media/v4l2-mem2mem.h>
> #include <media/v4l2-ioctl.h>
> #include <media/videobuf2-v4l2.h>
> @@ -1611,8 +1612,6 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct v4l2_format *f)
> FMT_TYPE_OUTPUT : FMT_TYPE_CAPTURE;
>
> q_data->fmt = s5p_jpeg_find_format(ct, pix->pixelformat, f_type);
> - q_data->w = pix->width;
> - q_data->h = pix->height;
> if (q_data->fmt->fourcc != V4L2_PIX_FMT_JPEG) {
> /*
> * During encoding Exynos4x12 SoCs access wider memory area
> @@ -1620,6 +1619,8 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct v4l2_format *f)
> * the JPEG_IMAGE_SIZE register. In order to avoid sysmmu
> * page fault calculate proper buffer size in such a case.
> */
> + q_data->w = pix->width;
> + q_data->h = pix->height;
Is this change related to what the patch is supposed to be doing?
> if (ct->jpeg->variant->hw_ex4_compat &&
> f_type == FMT_TYPE_OUTPUT && ct->mode == S5P_JPEG_ENCODE)
> q_data->size = exynos4_jpeg_get_output_buffer_size(ct,
> @@ -1695,6 +1696,15 @@ static int s5p_jpeg_s_fmt_vid_out(struct file *file, void *priv,
> return s5p_jpeg_s_fmt(fh_to_ctx(priv), f);
> }
>
> +static int s5p_jpeg_subscribe_event(struct v4l2_fh *fh,
> + const struct v4l2_event_subscription *sub)
> +{
> + if (sub->type == V4L2_EVENT_SOURCE_CHANGE)
> + return v4l2_src_change_event_subscribe(fh, sub);
> +
> + return -EINVAL;
> +}
> +
> static int exynos3250_jpeg_try_downscale(struct s5p_jpeg_ctx *ctx,
> struct v4l2_rect *r)
> {
> @@ -2020,6 +2030,9 @@ static const struct v4l2_ioctl_ops s5p_jpeg_ioctl_ops = {
>
> .vidioc_g_selection = s5p_jpeg_g_selection,
> .vidioc_s_selection = s5p_jpeg_s_selection,
> +
> + .vidioc_subscribe_event = s5p_jpeg_subscribe_event,
> + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> };
>
> /*
> @@ -2412,8 +2425,17 @@ static int s5p_jpeg_job_ready(void *priv)
> {
> struct s5p_jpeg_ctx *ctx = priv;
>
> - if (ctx->mode == S5P_JPEG_DECODE)
> + if (ctx->mode == S5P_JPEG_DECODE) {
> + /*
> + * We have only one input buffer and one output buffer. If there
> + * is a resolution change event, no need to continue decoding.
> + */
> + if (ctx->state == JPEGCTX_RESOLUTION_CHANGE)
> + return 0;
> +
> return ctx->hdr_parsed;
> + }
> +
> return 1;
> }
>
> @@ -2492,6 +2514,30 @@ static int s5p_jpeg_buf_prepare(struct vb2_buffer *vb)
> return 0;
> }
>
> +static void s5p_jpeg_set_capture_queue_data(struct s5p_jpeg_ctx *ctx)
> +{
> + struct s5p_jpeg_q_data *q_data = &ctx->cap_q;
> +
> + q_data->w = ctx->out_q.w;
> + q_data->h = ctx->out_q.h;
> +
> + /*
> + * This call to jpeg_bound_align_image() takes care of width and
> + * height values alignment when user space calls the QBUF of
> + * OUTPUT buffer after the S_FMT of CAPTURE buffer.
> + * Please note that on Exynos4x12 SoCs, resigning from executing
> + * S_FMT on capture buffer for each JPEG image can result in a
> + * hardware hangup if subsampling is lower than the one of input
> + * JPEG.
> + */
> + jpeg_bound_align_image(ctx, &q_data->w, S5P_JPEG_MIN_WIDTH,
> + S5P_JPEG_MAX_WIDTH, q_data->fmt->h_align,
> + &q_data->h, S5P_JPEG_MIN_HEIGHT,
> + S5P_JPEG_MAX_HEIGHT, q_data->fmt->v_align);
> +
> + q_data->size = q_data->w * q_data->h * q_data->fmt->depth >> 3;
> +}
> +
> static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> @@ -2499,9 +2545,20 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
>
> if (ctx->mode == S5P_JPEG_DECODE &&
> vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> - struct s5p_jpeg_q_data tmp, *q_data;
> -
> - ctx->hdr_parsed = s5p_jpeg_parse_hdr(&tmp,
> + static const struct v4l2_event ev_src_ch = {
> + .type = V4L2_EVENT_SOURCE_CHANGE,
> + .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
> + };
> + struct vb2_queue *dst_vq;
> + u32 ori_w;
> + u32 ori_h;
> +
> + dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> + V4L2_BUF_TYPE_VIDEO_CAPTURE);
> + ori_w = ctx->out_q.w;
> + ori_h = ctx->out_q.h;
> +
> + ctx->hdr_parsed = s5p_jpeg_parse_hdr(&ctx->out_q,
> (unsigned long)vb2_plane_vaddr(vb, 0),
> min((unsigned long)ctx->out_q.size,
> vb2_get_plane_payload(vb, 0)), ctx);
> @@ -2510,43 +2567,18 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
> return;
> }
>
> - q_data = &ctx->out_q;
> - q_data->w = tmp.w;
> - q_data->h = tmp.h;
> - q_data->sos = tmp.sos;
> - memcpy(q_data->dht.marker, tmp.dht.marker,
> - sizeof(tmp.dht.marker));
> - memcpy(q_data->dht.len, tmp.dht.len, sizeof(tmp.dht.len));
> - q_data->dht.n = tmp.dht.n;
> - memcpy(q_data->dqt.marker, tmp.dqt.marker,
> - sizeof(tmp.dqt.marker));
> - memcpy(q_data->dqt.len, tmp.dqt.len, sizeof(tmp.dqt.len));
> - q_data->dqt.n = tmp.dqt.n;
> - q_data->sof = tmp.sof;
> - q_data->sof_len = tmp.sof_len;
> -
> - q_data = &ctx->cap_q;
> - q_data->w = tmp.w;
> - q_data->h = tmp.h;
Why is this part removed?
Andrzej
Hi Andrzej,
On 16/06/2017 17:38, Andrzej Pietrasiewicz wrote:
> Hi Thierry,
>
> Thank you for the patch.
>
> Can you give a use case for resolution change event?
Unfortunately, the original commit does not mention any clear use case.
I've asked to the patch author for more information.
> Also plase see inline.
>
> W dniu 12.06.2017 o 19:13, Thierry Escande pisze:
>> From: henryhsu <[email protected]>
>> @@ -1611,8 +1612,6 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx
>> *ct, struct v4l2_format *f)
>> FMT_TYPE_OUTPUT : FMT_TYPE_CAPTURE;
>> q_data->fmt = s5p_jpeg_find_format(ct, pix->pixelformat, f_type);
>> - q_data->w = pix->width;
>> - q_data->h = pix->height;
>> if (q_data->fmt->fourcc != V4L2_PIX_FMT_JPEG) {
>> /*
>> * During encoding Exynos4x12 SoCs access wider memory area
>> @@ -1620,6 +1619,8 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx
>> *ct, struct v4l2_format *f)
>> * the JPEG_IMAGE_SIZE register. In order to avoid sysmmu
>> * page fault calculate proper buffer size in such a case.
>> */
>> + q_data->w = pix->width;
>> + q_data->h = pix->height;
>
> Is this change related to what the patch is supposed to be doing?
Yes actually. From the author comments to the same question:
"We want to send a resolution change in the first frame. Without this,
q_data->w and h will be updated by s_fmt. Then we won't know the
resolution is changed from (0, 0) to (w, h) in qbuf function."
>> static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
>> {
>> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> @@ -2499,9 +2545,20 @@ static void s5p_jpeg_buf_queue(struct
>> vb2_buffer *vb)
>> if (ctx->mode == S5P_JPEG_DECODE &&
>> vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
>> - struct s5p_jpeg_q_data tmp, *q_data;
>> -
>> - ctx->hdr_parsed = s5p_jpeg_parse_hdr(&tmp,
>> + static const struct v4l2_event ev_src_ch = {
>> + .type = V4L2_EVENT_SOURCE_CHANGE,
>> + .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
>> + };
>> + struct vb2_queue *dst_vq;
>> + u32 ori_w;
>> + u32 ori_h;
>> +
>> + dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
>> + V4L2_BUF_TYPE_VIDEO_CAPTURE);
>> + ori_w = ctx->out_q.w;
>> + ori_h = ctx->out_q.h;
>> +
>> + ctx->hdr_parsed = s5p_jpeg_parse_hdr(&ctx->out_q,
>> (unsigned long)vb2_plane_vaddr(vb, 0),
>> min((unsigned long)ctx->out_q.size,
>> vb2_get_plane_payload(vb, 0)), ctx);
>> @@ -2510,43 +2567,18 @@ static void s5p_jpeg_buf_queue(struct
>> vb2_buffer *vb)
>> return;
>> }
>> - q_data = &ctx->out_q;
>> - q_data->w = tmp.w;
>> - q_data->h = tmp.h;
>> - q_data->sos = tmp.sos;
>> - memcpy(q_data->dht.marker, tmp.dht.marker,
>> - sizeof(tmp.dht.marker));
>> - memcpy(q_data->dht.len, tmp.dht.len, sizeof(tmp.dht.len));
>> - q_data->dht.n = tmp.dht.n;
>> - memcpy(q_data->dqt.marker, tmp.dqt.marker,
>> - sizeof(tmp.dqt.marker));
>> - memcpy(q_data->dqt.len, tmp.dqt.len, sizeof(tmp.dqt.len));
>> - q_data->dqt.n = tmp.dqt.n;
>> - q_data->sof = tmp.sof;
>> - q_data->sof_len = tmp.sof_len;
>> -
>> - q_data = &ctx->cap_q;
>> - q_data->w = tmp.w;
>> - q_data->h = tmp.h;
>
>
> Why is this part removed?
This has not been removed.
The &tmp s5p_jpeg_q_data struct was passed to s5p_jpeg_parse_hdr() and
then copied field-by-field into ctx->out_q (through q_data pointer).
With this change ctx->out_q is passed to s5p_jpeg_parse_hdr() and this
avoids the copy.
Then ctx->cap_q width & height copy is done in
s5p_jpeg_set_capture_queue_data().
Regards,
Thierry
Hi Thierry,
W dniu 19.06.2017 o 15:50, Thierry Escande pisze:
> Hi Andrzej,
>
> On 16/06/2017 17:38, Andrzej Pietrasiewicz wrote:
>> Hi Thierry,
>>
>> Thank you for the patch.
>>
>> Can you give a use case for resolution change event?
> Unfortunately, the original commit does not mention any clear use case.
> I've asked to the patch author for more information.
Can you please share what you learn about it if the author gets back to you?
Now that we don't know why to apply a patch I guess we should not do it.
>
<snip>
>>> @@ -2510,43 +2567,18 @@ static void s5p_jpeg_buf_queue(struct
>>> vb2_buffer *vb)
>>> return;
>>> }
>>> - q_data = &ctx->out_q;
>>> - q_data->w = tmp.w;
>>> - q_data->h = tmp.h;
>>> - q_data->sos = tmp.sos;
>>> - memcpy(q_data->dht.marker, tmp.dht.marker,
>>> - sizeof(tmp.dht.marker));
>>> - memcpy(q_data->dht.len, tmp.dht.len, sizeof(tmp.dht.len));
>>> - q_data->dht.n = tmp.dht.n;
>>> - memcpy(q_data->dqt.marker, tmp.dqt.marker,
>>> - sizeof(tmp.dqt.marker));
>>> - memcpy(q_data->dqt.len, tmp.dqt.len, sizeof(tmp.dqt.len));
>>> - q_data->dqt.n = tmp.dqt.n;
>>> - q_data->sof = tmp.sof;
>>> - q_data->sof_len = tmp.sof_len;
>>> -
>>> - q_data = &ctx->cap_q;
>>> - q_data->w = tmp.w;
>>> - q_data->h = tmp.h;
>>
>>
>> Why is this part removed?
> This has not been removed.
> The &tmp s5p_jpeg_q_data struct was passed to s5p_jpeg_parse_hdr() and
> then copied field-by-field into ctx->out_q (through q_data pointer).
> With this change ctx->out_q is passed to s5p_jpeg_parse_hdr() and this
> avoids the copy.
It seems that changing field-by-field copying to passing a pointer
directly to s5p_jpeg_parse_hdr() is an unrelated change and as such
should be in a separate patch.
Andrzej
Hi Thierry,
W dniu 12.06.2017 o 19:13, Thierry Escande pisze:
> From: Tony K Nadackal <[email protected]>
>
> This patch adds support for decoding 4:1:1 chroma subsampling in the
> jpeg header parsing function.
>
> Signed-off-by: Tony K Nadackal <[email protected]>
> Signed-off-by: Thierry Escande <[email protected]>
> ---
> drivers/media/platform/s5p-jpeg/jpeg-core.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 0d935f5..7ef7173 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -1236,6 +1236,9 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
> case 0x33:
> ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY;
> break;
> + case 0x41:
> + ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_411;
> + break;
Merely parsing 4:1:1 subsampling is not enough.
Now the s5p_jpeg_parse_hdr() sometimes returns false, among others
it does so when unsupported subsampling is encountered in the header.
As far as I know 4:1:1 is supported only on some variants (3250, 5420, 5433)
of the hardware, so the kind of change intended by the patch author
must take hardware variants into account. In the above function
ctx is available, so accessing hardware variant information is possible.
The s5p_jpeg_parse_hdr() is a lengthy function, so probably the
switch (subsampling) part should be factored out to a separate
function and extended appropriately.
Andrzej
Hi Andrzej,
On 20/06/2017 12:51, Andrzej Pietrasiewicz wrote:
> Hi Thierry,
>
> W dniu 19.06.2017 o 15:50, Thierry Escande pisze:
>> Hi Andrzej,
>>
>> On 16/06/2017 17:38, Andrzej Pietrasiewicz wrote:
>>> Hi Thierry,
>>>
>>> Thank you for the patch.
>>>
>>> Can you give a use case for resolution change event?
>> Unfortunately, the original commit does not mention any clear use case.
>> I've asked to the patch author for more information.
>
> Can you please share what you learn about it if the author gets back to
> you?
> Now that we don't know why to apply a patch I guess we should not do it.
This event is used in Chromium by the V4L2 jpeg decode accelerator to
allocate output buffer. Please see:
https://cs.chromium.org/chromium/src/media/gpu/v4l2_jpeg_decode_accelerator.cc?rcl=91793c6ef94f05e93d258db8c7f3cad59819c6b8&l=585
I'll add a note in the commit message.
>
> <snip>
>
>>>> @@ -2510,43 +2567,18 @@ static void s5p_jpeg_buf_queue(struct
>>>> vb2_buffer *vb)
>>>> return;
>>>> }
>>>> - q_data = &ctx->out_q;
>>>> - q_data->w = tmp.w;
>>>> - q_data->h = tmp.h;
>>>> - q_data->sos = tmp.sos;
>>>> - memcpy(q_data->dht.marker, tmp.dht.marker,
>>>> - sizeof(tmp.dht.marker));
>>>> - memcpy(q_data->dht.len, tmp.dht.len, sizeof(tmp.dht.len));
>>>> - q_data->dht.n = tmp.dht.n;
>>>> - memcpy(q_data->dqt.marker, tmp.dqt.marker,
>>>> - sizeof(tmp.dqt.marker));
>>>> - memcpy(q_data->dqt.len, tmp.dqt.len, sizeof(tmp.dqt.len));
>>>> - q_data->dqt.n = tmp.dqt.n;
>>>> - q_data->sof = tmp.sof;
>>>> - q_data->sof_len = tmp.sof_len;
>>>> -
>>>> - q_data = &ctx->cap_q;
>>>> - q_data->w = tmp.w;
>>>> - q_data->h = tmp.h;
>>>
>>>
>>> Why is this part removed?
>> This has not been removed.
>> The &tmp s5p_jpeg_q_data struct was passed to s5p_jpeg_parse_hdr() and
>> then copied field-by-field into ctx->out_q (through q_data pointer).
>> With this change ctx->out_q is passed to s5p_jpeg_parse_hdr() and this
>> avoids the copy.
>
> It seems that changing field-by-field copying to passing a pointer
> directly to s5p_jpeg_parse_hdr() is an unrelated change and as such
> should be in a separate patch.
Will do.
Regards,
Thierry
Hi Thierry,
W dniu 12.06.2017 o 19:13, Thierry Escande pisze:
> From: Abhilash Kesavan <[email protected]>
>
> This patch resets the encoding and decoding register bits before doing a
> soft reset.
Here are my thoughts after consulting the documentation:
>
> Signed-off-by: Tony K Nadackal <[email protected]>
> Signed-off-by: Thierry Escande <[email protected]>
> ---
> drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c
> index a1d823a..9ad8f6d 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c
> @@ -21,6 +21,10 @@ void exynos4_jpeg_sw_reset(void __iomem *base)
> unsigned int reg;
>
> reg = readl(base + EXYNOS4_JPEG_CNTL_REG);
> + writel(reg & ~(EXYNOS4_DEC_MODE | EXYNOS4_ENC_MODE),
> + base + EXYNOS4_JPEG_CNTL_REG);
Indeed, if encoding/decoding "back-to-back", the bits this patch touches
should be reset.
The doc also says, that "Soft reset is asserted to all registers
of JPEG except soft reset bit itself", so, theoretically speaking,
the changes in this patch are redundant. Instead, the doc says,
these bits have to be reset after servicing the interrupt for current image
and before programming the hardware to perform the next en/decoding.
And indeed, the first thing that both ENCODE and DECODE paths
of exynos4_jpeg_device_run() do is calling sw reset.
If, however, you can show that the changes in the patch discussed here
are in fact necessary (that's the very difference between theory and practise...),
I will readily ack it.
Andrzej