[PATCH v3 0/8] [media] s5p-jpeg: Various fixes and improvements

[PATCH v3 0/8] [media] s5p-jpeg: Various fixes and improvements

[Thierry Escande]

Hi,

This series contains various fixes and improvements for the Samsung
s5p-jpeg driver. Most of these patches come from the Chromium v3.8
kernel tree.

In this v3:
- Remove codec reset patch (Not needed based on documentation and no
  use case described in original patch commit message).
- Check for Exynos5420 variant in stream error handling patch.
- Add use case for resolution change event support in commit message.
- Move subsampling value decoding in a separate function.
- Check Exynos variant for 4:1:1 subsampling support.

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.

Thierry Escande (3):
  [media] s5p-jpeg: Handle parsing error in s5p_jpeg_parse_hdr()
  [media] s5p-jpeg: Don't use temporary structure in s5p_jpeg_buf_queue
  [media] s5p-jpeg: Split s5p_jpeg_parse_hdr()

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 | 186 +++++++++++++++++++++-------
 drivers/media/platform/s5p-jpeg/jpeg-core.h |   7 ++
 2 files changed, 148 insertions(+), 45 deletions(-)

-- 
2.7.4

[PATCH v3 1/8] [media] s5p-jpeg: Call jpeg_bound_align_image after qbuf

[Thierry Escande]

From: Tony K Nadackal <tony.kn@samsung.com>

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 <tony.kn@samsung.com>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 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

Re: [PATCH v3 1/8] [media] s5p-jpeg: Call jpeg_bound_align_image after qbuf

[Andrzej Pietrasiewicz]

W dniu 27.06.2017 o 18:08, Thierry Escande pisze:
> From: Tony K Nadackal <tony.kn@samsung.com>
> 
> 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 <tony.kn@samsung.com>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Acked-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

> ---
>   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);
> 

[PATCH v3 2/8] [media] s5p-jpeg: Correct WARN_ON statement for checking subsampling

[Thierry Escande]

From: Tony K Nadackal <tony.kn@samsung.com>

Corrects the WARN_ON statement for subsampling based on the
JPEG Hardware version.

Signed-off-by: Tony K Nadackal <tony.kn@samsung.com>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 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

Re: [PATCH v3 2/8] [media] s5p-jpeg: Correct WARN_ON statement for checking subsampling

[Andrzej Pietrasiewicz]

W dniu 27.06.2017 o 18:08, Thierry Escande pisze:
> From: Tony K Nadackal <tony.kn@samsung.com>
> 
> Corrects the WARN_ON statement for subsampling based on the
> JPEG Hardware version.
> 
> Signed-off-by: Tony K Nadackal <tony.kn@samsung.com>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Acked-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

> ---
>   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;
>   	}
>   }
> 

[PATCH v3 3/8] [media] s5p-jpeg: Handle parsing error in s5p_jpeg_parse_hdr()

[Thierry Escande]

This patch modifies the s5p_jpeg_parse_hdr() function so it only
modifies the passed s5p_jpeg_q_data structure if the jpeg header parsing
is successful.

Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 38 ++++++++++++++++-------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 0d935f5..df3e5ee 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1206,22 +1206,9 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
 			break;
 		}
 	}
-	result->w = width;
-	result->h = height;
-	result->sos = sos;
-	result->dht.n = n_dht;
-	while (n_dht--) {
-		result->dht.marker[n_dht] = dht[n_dht];
-		result->dht.len[n_dht] = dht_len[n_dht];
-	}
-	result->dqt.n = n_dqt;
-	while (n_dqt--) {
-		result->dqt.marker[n_dqt] = dqt[n_dqt];
-		result->dqt.len[n_dqt] = dqt_len[n_dqt];
-	}
-	result->sof = sof;
-	result->sof_len = sof_len;
-	result->size = result->components = components;
+
+	if (notfound || !sos)
+		return false;
 
 	switch (subsampling) {
 	case 0x11:
@@ -1240,7 +1227,24 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
 		return false;
 	}
 
-	return !notfound && sos;
+	result->w = width;
+	result->h = height;
+	result->sos = sos;
+	result->dht.n = n_dht;
+	while (n_dht--) {
+		result->dht.marker[n_dht] = dht[n_dht];
+		result->dht.len[n_dht] = dht_len[n_dht];
+	}
+	result->dqt.n = n_dqt;
+	while (n_dqt--) {
+		result->dqt.marker[n_dqt] = dqt[n_dqt];
+		result->dqt.len[n_dqt] = dqt_len[n_dqt];
+	}
+	result->sof = sof;
+	result->sof_len = sof_len;
+	result->size = result->components = components;
+
+	return true;
 }
 
 static int s5p_jpeg_querycap(struct file *file, void *priv,
-- 
2.7.4

Re: [PATCH v3 3/8] [media] s5p-jpeg: Handle parsing error in s5p_jpeg_parse_hdr()

[Andrzej Pietrasiewicz]

W dniu 27.06.2017 o 18:08, Thierry Escande pisze:
> This patch modifies the s5p_jpeg_parse_hdr() function so it only
> modifies the passed s5p_jpeg_q_data structure if the jpeg header parsing
> is successful.
> 
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Acked-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

> ---
>   drivers/media/platform/s5p-jpeg/jpeg-core.c | 38 ++++++++++++++++-------------
>   1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 0d935f5..df3e5ee 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -1206,22 +1206,9 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
>   			break;
>   		}
>   	}
> -	result->w = width;
> -	result->h = height;
> -	result->sos = sos;
> -	result->dht.n = n_dht;
> -	while (n_dht--) {
> -		result->dht.marker[n_dht] = dht[n_dht];
> -		result->dht.len[n_dht] = dht_len[n_dht];
> -	}
> -	result->dqt.n = n_dqt;
> -	while (n_dqt--) {
> -		result->dqt.marker[n_dqt] = dqt[n_dqt];
> -		result->dqt.len[n_dqt] = dqt_len[n_dqt];
> -	}
> -	result->sof = sof;
> -	result->sof_len = sof_len;
> -	result->size = result->components = components;
> +
> +	if (notfound || !sos)
> +		return false;
>   
>   	switch (subsampling) {
>   	case 0x11:
> @@ -1240,7 +1227,24 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
>   		return false;
>   	}
>   
> -	return !notfound && sos;
> +	result->w = width;
> +	result->h = height;
> +	result->sos = sos;
> +	result->dht.n = n_dht;
> +	while (n_dht--) {
> +		result->dht.marker[n_dht] = dht[n_dht];
> +		result->dht.len[n_dht] = dht_len[n_dht];
> +	}
> +	result->dqt.n = n_dqt;
> +	while (n_dqt--) {
> +		result->dqt.marker[n_dqt] = dqt[n_dqt];
> +		result->dqt.len[n_dqt] = dqt_len[n_dqt];
> +	}
> +	result->sof = sof;
> +	result->sof_len = sof_len;
> +	result->size = result->components = components;
> +
> +	return true;
>   }
>   
>   static int s5p_jpeg_querycap(struct file *file, void *priv,
> 

[PATCH v3 4/8] [media] s5p-jpeg: Don't use temporary structure in s5p_jpeg_buf_queue

[Thierry Escande]

If s5p_jpeg_parse_hdr() fails to parse the JPEG header, the passed
s5p_jpeg_q_data structure is not modify so there is no need to use a
temporary structure and the field-by-field copy can be avoided.

Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index df3e5ee..1769744 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2500,9 +2500,9 @@ 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;
+		struct s5p_jpeg_q_data *q_data;
 
-		ctx->hdr_parsed = s5p_jpeg_parse_hdr(&tmp,
+		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);
@@ -2511,24 +2511,9 @@ 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;
+		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
-- 
2.7.4

Re: [PATCH v3 4/8] [media] s5p-jpeg: Don't use temporary structure in s5p_jpeg_buf_queue

[Andrzej Pietrasiewicz]

W dniu 27.06.2017 o 18:08, Thierry Escande pisze:
> If s5p_jpeg_parse_hdr() fails to parse the JPEG header, the passed
> s5p_jpeg_q_data structure is not modify so there is no need to use a
> temporary structure and the field-by-field copy can be avoided.
> 
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Acked-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

> ---
>   drivers/media/platform/s5p-jpeg/jpeg-core.c | 23 ++++-------------------
>   1 file changed, 4 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index df3e5ee..1769744 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -2500,9 +2500,9 @@ 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;
> +		struct s5p_jpeg_q_data *q_data;
>   
> -		ctx->hdr_parsed = s5p_jpeg_parse_hdr(&tmp,
> +		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);
> @@ -2511,24 +2511,9 @@ 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;
> +		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
> 

Re: [PATCH v3 4/8] [media] s5p-jpeg: Don't use temporary structure in s5p_jpeg_buf_queue

[Jacek Anaszewski]

Hi Thierry,

On 06/27/2017 06:08 PM, Thierry Escande wrote:
> If s5p_jpeg_parse_hdr() fails to parse the JPEG header, the passed
> s5p_jpeg_q_data structure is not modify so there is no need to use a

s/modify/modified/

> temporary structure and the field-by-field copy can be avoided.
> 
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/media/platform/s5p-jpeg/jpeg-core.c | 23 ++++-------------------
>  1 file changed, 4 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index df3e5ee..1769744 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -2500,9 +2500,9 @@ 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;
> +		struct s5p_jpeg_q_data *q_data;
>  
> -		ctx->hdr_parsed = s5p_jpeg_parse_hdr(&tmp,
> +		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);
> @@ -2511,24 +2511,9 @@ 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;
> +		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
> 

-- 
Best regards,
Jacek Anaszewski

[PATCH v3 5/8] [media] s5p-jpeg: Split s5p_jpeg_parse_hdr()

[Thierry Escande]

This patch moves the subsampling value decoding read from the jpeg
header into its own function. This new function is called
s5p_jpeg_subsampling_decode() and returns true if it successfully
decodes the subsampling value, false otherwise.

Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 42 ++++++++++++++++-------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 1769744..0783809 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1096,6 +1096,29 @@ static void skip(struct s5p_jpeg_buffer *buf, long len)
 		get_byte(buf);
 }
 
+static bool s5p_jpeg_subsampling_decode(struct s5p_jpeg_ctx *ctx,
+					unsigned int subsampling)
+{
+	switch (subsampling) {
+	case 0x11:
+		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_444;
+		break;
+	case 0x21:
+		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_422;
+		break;
+	case 0x22:
+		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_420;
+		break;
+	case 0x33:
+		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY;
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
 static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
 			       unsigned long buffer, unsigned long size,
 			       struct s5p_jpeg_ctx *ctx)
@@ -1207,26 +1230,9 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
 		}
 	}
 
-	if (notfound || !sos)
+	if (notfound || !sos || !s5p_jpeg_subsampling_decode(ctx, subsampling))
 		return false;
 
-	switch (subsampling) {
-	case 0x11:
-		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_444;
-		break;
-	case 0x21:
-		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_422;
-		break;
-	case 0x22:
-		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_420;
-		break;
-	case 0x33:
-		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY;
-		break;
-	default:
-		return false;
-	}
-
 	result->w = width;
 	result->h = height;
 	result->sos = sos;
-- 
2.7.4

Re: [PATCH v3 5/8] [media] s5p-jpeg: Split s5p_jpeg_parse_hdr()

[Andrzej Pietrasiewicz]

W dniu 27.06.2017 o 18:08, Thierry Escande pisze:
> This patch moves the subsampling value decoding read from the jpeg
> header into its own function. This new function is called
> s5p_jpeg_subsampling_decode() and returns true if it successfully
> decodes the subsampling value, false otherwise.
> 
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Acked-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

> ---
>   drivers/media/platform/s5p-jpeg/jpeg-core.c | 42 ++++++++++++++++-------------
>   1 file changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 1769744..0783809 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -1096,6 +1096,29 @@ static void skip(struct s5p_jpeg_buffer *buf, long len)
>   		get_byte(buf);
>   }
>   
> +static bool s5p_jpeg_subsampling_decode(struct s5p_jpeg_ctx *ctx,
> +					unsigned int subsampling)
> +{
> +	switch (subsampling) {
> +	case 0x11:
> +		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_444;
> +		break;
> +	case 0x21:
> +		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_422;
> +		break;
> +	case 0x22:
> +		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_420;
> +		break;
> +	case 0x33:
> +		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY;
> +		break;
> +	default:
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>   static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
>   			       unsigned long buffer, unsigned long size,
>   			       struct s5p_jpeg_ctx *ctx)
> @@ -1207,26 +1230,9 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
>   		}
>   	}
>   
> -	if (notfound || !sos)
> +	if (notfound || !sos || !s5p_jpeg_subsampling_decode(ctx, subsampling))
>   		return false;
>   
> -	switch (subsampling) {
> -	case 0x11:
> -		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_444;
> -		break;
> -	case 0x21:
> -		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_422;
> -		break;
> -	case 0x22:
> -		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_420;
> -		break;
> -	case 0x33:
> -		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY;
> -		break;
> -	default:
> -		return false;
> -	}
> -
>   	result->w = width;
>   	result->h = height;
>   	result->sos = sos;
> 

[PATCH v3 6/8] [media] s5p-jpeg: Decode 4:1:1 chroma subsampling format

[Thierry Escande]

From: Tony K Nadackal <tony.kn@samsung.com>

This patch adds support for decoding 4:1:1 chroma subsampling in the
jpeg header parsing function.

Signed-off-by: Tony K Nadackal <tony.kn@samsung.com>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 0783809..cca0fb8 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1099,6 +1099,8 @@ static void skip(struct s5p_jpeg_buffer *buf, long len)
 static bool s5p_jpeg_subsampling_decode(struct s5p_jpeg_ctx *ctx,
 					unsigned int subsampling)
 {
+	unsigned int version;
+
 	switch (subsampling) {
 	case 0x11:
 		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_444;
@@ -1112,6 +1114,19 @@ static bool s5p_jpeg_subsampling_decode(struct s5p_jpeg_ctx *ctx,
 	case 0x33:
 		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY;
 		break;
+	case 0x41:
+		/*
+		 * 4:1:1 subsampling only supported by 3250, 5420, and 5433
+		 * variants
+		 */
+		version = ctx->jpeg->variant->version;
+		if (version != SJPEG_EXYNOS3250 &&
+		    version != SJPEG_EXYNOS5420 &&
+		    version != SJPEG_EXYNOS5433)
+			return false;
+
+		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_411;
+		break;
 	default:
 		return false;
 	}
-- 
2.7.4

Re: [PATCH v3 6/8] [media] s5p-jpeg: Decode 4:1:1 chroma subsampling format

[Andrzej Pietrasiewicz]

W dniu 27.06.2017 o 18:08, Thierry Escande pisze:
> From: Tony K Nadackal <tony.kn@samsung.com>
> 
> This patch adds support for decoding 4:1:1 chroma subsampling in the
> jpeg header parsing function.
> 
> Signed-off-by: Tony K Nadackal <tony.kn@samsung.com>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
Acked-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

> ---
>   drivers/media/platform/s5p-jpeg/jpeg-core.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 0783809..cca0fb8 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -1099,6 +1099,8 @@ static void skip(struct s5p_jpeg_buffer *buf, long len)
>   static bool s5p_jpeg_subsampling_decode(struct s5p_jpeg_ctx *ctx,
>   					unsigned int subsampling)
>   {
> +	unsigned int version;
> +
>   	switch (subsampling) {
>   	case 0x11:
>   		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_444;
> @@ -1112,6 +1114,19 @@ static bool s5p_jpeg_subsampling_decode(struct s5p_jpeg_ctx *ctx,
>   	case 0x33:
>   		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY;
>   		break;
> +	case 0x41:
> +		/*
> +		 * 4:1:1 subsampling only supported by 3250, 5420, and 5433
> +		 * variants
> +		 */
> +		version = ctx->jpeg->variant->version;
> +		if (version != SJPEG_EXYNOS3250 &&
> +		    version != SJPEG_EXYNOS5420 &&
> +		    version != SJPEG_EXYNOS5433)
> +			return false;
> +
> +		ctx->subsampling = V4L2_JPEG_CHROMA_SUBSAMPLING_411;
> +		break;
>   	default:
>   		return false;
>   	}
> 

[PATCH v3 7/8] [media] s5p-jpeg: Add support for resolution change event

[Thierry Escande]

From: henryhsu <henryhsu@chromium.org>

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.

This event is used in the Chromium browser project by the V4L2 JPEG
Decode Accelerator (V4L2JDA) to allocate output buffer.

Signed-off-by: Henry-Ruey Hsu <henryhsu@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 106 +++++++++++++++++++++-------
 drivers/media/platform/s5p-jpeg/jpeg-core.h |   7 ++
 2 files changed, 89 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index cca0fb8..5ad3d43 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>
@@ -1633,8 +1634,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
@@ -1642,6 +1641,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,
@@ -1717,6 +1718,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)
 {
@@ -2042,6 +2052,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,
 };
 
 /*
@@ -2434,8 +2447,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;
 }
 
@@ -2514,6 +2536,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);
@@ -2521,7 +2567,18 @@ 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 *q_data;
+		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),
@@ -2532,28 +2589,18 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
 			return;
 		}
 
-		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.
+		 * 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);
@@ -2573,6 +2620,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

Re: [PATCH v3 7/8] [media] s5p-jpeg: Add support for resolution change event

[Andrzej Pietrasiewicz]

W dniu 27.06.2017 o 18:08, Thierry Escande pisze:
> From: henryhsu <henryhsu@chromium.org>
> 
> 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.
> 
> This event is used in the Chromium browser project by the V4L2 JPEG
> Decode Accelerator (V4L2JDA) to allocate output buffer.
> 
> Signed-off-by: Henry-Ruey Hsu <henryhsu@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Acked-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

> ---
>   drivers/media/platform/s5p-jpeg/jpeg-core.c | 106 +++++++++++++++++++++-------
>   drivers/media/platform/s5p-jpeg/jpeg-core.h |   7 ++
>   2 files changed, 89 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index cca0fb8..5ad3d43 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>
> @@ -1633,8 +1634,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
> @@ -1642,6 +1641,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,
> @@ -1717,6 +1718,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)
>   {
> @@ -2042,6 +2052,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,
>   };
>   
>   /*
> @@ -2434,8 +2447,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;
>   }
>   
> @@ -2514,6 +2536,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);
> @@ -2521,7 +2567,18 @@ 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 *q_data;
> +		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),
> @@ -2532,28 +2589,18 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
>   			return;
>   		}
>   
> -		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.
> +		 * 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);
> @@ -2573,6 +2620,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;
>   };
>   
>   /**
> 

[PATCH v3 8/8] [media] s5p-jpeg: Add stream error handling for Exynos5420

[Thierry Escande]

From: henryhsu <henryhsu@chromium.org>

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 <henryhsu@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 5ad3d43..c35d169 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2812,6 +2812,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);
@@ -2828,6 +2829,12 @@ static irqreturn_t exynos3250_jpeg_irq(int irq, void *dev_id)
 
 	jpeg->irq_status |= irq_status;
 
+	if (jpeg->variant->version == SJPEG_EXYNOS5420 &&
+	    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)
@@ -2844,7 +2851,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

Re: [PATCH v3 8/8] [media] s5p-jpeg: Add stream error handling for Exynos5420

[Andrzej Pietrasiewicz]

W dniu 27.06.2017 o 18:08, Thierry Escande pisze:
> From: henryhsu <henryhsu@chromium.org>
> 
> 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 <henryhsu@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>

Acked-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

> ---
>   drivers/media/platform/s5p-jpeg/jpeg-core.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 5ad3d43..c35d169 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -2812,6 +2812,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);
> @@ -2828,6 +2829,12 @@ static irqreturn_t exynos3250_jpeg_irq(int irq, void *dev_id)
>   
>   	jpeg->irq_status |= irq_status;
>   
> +	if (jpeg->variant->version == SJPEG_EXYNOS5420 &&
> +	    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)
> @@ -2844,7 +2851,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;
> 

Re: [PATCH v3 0/8] [media] s5p-jpeg: Various fixes and improvements

[Andrzej Pietrasiewicz]

Hi Thierry,

W dniu 27.06.2017 o 18:08, Thierry Escande pisze:
> Hi,
> 
> This series contains various fixes and improvements for the Samsung
> s5p-jpeg driver. Most of these patches come from the Chromium v3.8
> kernel tree.
> 

Thank you for the series. It looks good to me.

Andrzej

Re: [PATCH v3 0/8] [media] s5p-jpeg: Various fixes and improvements

[Jacek Anaszewski]

Hi Thierry,

For the whole series:

Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

Best regards,
Jacek Anaszewski

On 06/27/2017 06:08 PM, Thierry Escande wrote:
> Hi,
> 
> This series contains various fixes and improvements for the Samsung
> s5p-jpeg driver. Most of these patches come from the Chromium v3.8
> kernel tree.
> 
> In this v3:
> - Remove codec reset patch (Not needed based on documentation and no
>   use case described in original patch commit message).
> - Check for Exynos5420 variant in stream error handling patch.
> - Add use case for resolution change event support in commit message.
> - Move subsampling value decoding in a separate function.
> - Check Exynos variant for 4:1:1 subsampling support.
> 
> 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.
> 
> Thierry Escande (3):
>   [media] s5p-jpeg: Handle parsing error in s5p_jpeg_parse_hdr()
>   [media] s5p-jpeg: Don't use temporary structure in s5p_jpeg_buf_queue
>   [media] s5p-jpeg: Split s5p_jpeg_parse_hdr()
> 
> 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 | 186 +++++++++++++++++++++-------
>  drivers/media/platform/s5p-jpeg/jpeg-core.h |   7 ++
>  2 files changed, 148 insertions(+), 45 deletions(-)
>