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

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

[Thierry Escande]

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.

Regards,
 Thierry

Abhilash Kesavan (1):
  [media] s5p-jpeg: Reset the Codec before doing a soft reset

Ricky Liang (1):
  [media] s5p-jpeg: Add support for multi-planar APIs

Tony K Nadackal (4):
  [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
  [media] s5p-jpeg: Add IOMMU support

henryhsu (3):
  [media] s5p-jpeg: Add support for resolution change event
  [media] s5p-jpeg: Change sclk_jpeg to 166MHz for Exynos5250
  [media] s5p-jpeg: Add stream error handling for Exynos5420

 drivers/media/platform/s5p-jpeg/jpeg-core.c       | 387 ++++++++++++++++++++--
 drivers/media/platform/s5p-jpeg/jpeg-core.h       |   9 +
 drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c |   4 +
 3 files changed, 368 insertions(+), 32 deletions(-)

-- 
2.7.4

[PATCH 1/9] [media] s5p-jpeg: Reset the Codec before doing a soft reset

[Thierry Escande]

From: Abhilash Kesavan <a.kesavan@samsung.com>

This patch resets the encoding and decoding register bits before doing a
soft reset.

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

Re: [PATCH 1/9] [media] s5p-jpeg: Reset the Codec before doing a soft reset

[Jacek Anaszewski]

Hi Thierry,

On 06/02/2017 06:02 PM, Thierry Escande wrote:
> From: Abhilash Kesavan <a.kesavan@samsung.com>
> 
> This patch resets the encoding and decoding register bits before doing a
> soft reset.
> 
> 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-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);

Why is it required? It would be nice if commit message explained that.

> +	reg = readl(base + EXYNOS4_JPEG_CNTL_REG);
>  	writel(reg & ~EXYNOS4_SOFT_RESET_HI, base + EXYNOS4_JPEG_CNTL_REG);
>  
>  	udelay(100);
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/9] [media] s5p-jpeg: Reset the Codec before doing a soft reset

[Thierry Escande]

Hi Jacek,

On 02/06/2017 21:50, Jacek Anaszewski wrote:
> Hi Thierry,
> 
> On 06/02/2017 06:02 PM, Thierry Escande wrote:
>> From: Abhilash Kesavan <a.kesavan@samsung.com>
>>
>> This patch resets the encoding and decoding register bits before doing a
>> soft reset.
>>
>> 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-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);
> 
> Why is it required? It would be nice if commit message explained that.

Unfortunately the bug entry in the ChromeOS issue tracker does not 
mention more information about that and the patch author is no more 
reachable on that email address.

So unless someone else knows the answer I won't be able to give more 
explanation in the commit message...

Regards,
  Thierry

Re: [PATCH 1/9] [media] s5p-jpeg: Reset the Codec before doing a soft reset

[Jacek Anaszewski]

Hi Thierry,

On 06/07/2017 02:34 PM, Thierry Escande wrote:
> Hi Jacek,
> 
> On 02/06/2017 21:50, Jacek Anaszewski wrote:
>> Hi Thierry,
>>
>> On 06/02/2017 06:02 PM, Thierry Escande wrote:
>>> From: Abhilash Kesavan <a.kesavan@samsung.com>
>>>
>>> This patch resets the encoding and decoding register bits before doing a
>>> soft reset.
>>>
>>> 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-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);
>>
>> Why is it required? It would be nice if commit message explained that.
> 
> Unfortunately the bug entry in the ChromeOS issue tracker does not
> mention more information about that and the patch author is no more
> reachable on that email address.
> 
> So unless someone else knows the answer I won't be able to give more
> explanation in the commit message...

Unfortunately I don't have longer access to the hardware and
can't test these changes. Have you tested them, or just cherry-picked
from the bug tracker?

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/9] [media] s5p-jpeg: Reset the Codec before doing a soft reset

[Andrzej Pietrasiewicz]

Hi,

W dniu 13.06.2017 o 20:46, Jacek Anaszewski pisze:
> Hi Thierry,
> 
> On 06/07/2017 02:34 PM, Thierry Escande wrote:
>> Hi Jacek,
>>
>> On 02/06/2017 21:50, Jacek Anaszewski wrote:
>>> Hi Thierry,
>>>
>>> On 06/02/2017 06:02 PM, Thierry Escande wrote:
>>>> From: Abhilash Kesavan <a.kesavan@samsung.com>
>>>>
>>>> This patch resets the encoding and decoding register bits before doing a
>>>> soft reset.
>>>>
>>>> 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-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);
>>>
>>> Why is it required? It would be nice if commit message explained that.
>>
>> Unfortunately the bug entry in the ChromeOS issue tracker does not
>> mention more information about that and the patch author is no more
>> reachable on that email address.
>>
>> So unless someone else knows the answer I won't be able to give more
>> explanation in the commit message...
> 
> Unfortunately I don't have longer access to the hardware and
> can't test these changes. Have you tested them, or just cherry-picked
> from the bug tracker?
> 

I do have access to the hardware and will look into the series,
however results can be expected next week.

Andrzej

[PATCH 2/9] [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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 52dc794..6fb1ab4 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2523,6 +2523,13 @@ 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;
+
+		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 2/9] [media] s5p-jpeg: Call jpeg_bound_align_image after qbuf

[Jacek Anaszewski]

Hi Thierry,

Thanks for the patch.

On 06/02/2017 06:02 PM, Thierry Escande wrote:
> 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.

I assume that you may want to avoid re-setting the capture buf format
when decoding a stream of JPEGs and you are certain that all of them
have the same subsampling. Nonetheless, please keep in mind that in case
of Exynos4x12 SoCs there is a risk of permanent decoder hangup if you'd
try to decode to a YUV with lower subsampling than the one of input
JPEG. s5p_jpeg_try_fmt_vid_cap() does a suitable adjustment to avoid the
problem.

I'd add a comment over this call to jpeg_bound_align_image() that
resigning from executing S_FMT on capture buf for each JPEG image
can result in a hardware hangup if forbidden decoding will be enforced.

> 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 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 52dc794..6fb1ab4 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -2523,6 +2523,13 @@ 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;
> +
> +		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);
> 

-- 
Best regards,
Jacek Anaszewski

[PATCH 3/9] [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 6fb1ab4..0d83948 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

[PATCH 4/9] [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 | 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 0d83948..770a709 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

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

[Jacek Anaszewski]

Hi Thierry,

On 06/02/2017 06:02 PM, Thierry Escande wrote:
> 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 | 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 0d83948..770a709 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;
>  	}
> 

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

-- 
Best regards,
Jacek Anaszewski

[PATCH 5/9] [media] s5p-jpeg: Add IOMMU support

[Thierry Escande]

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

This patch adds support for IOMMU s5p-jpeg driver if the Exynos IOMMU
and ARM DMA IOMMU configurations are supported. The address space is
created with size limited to 256M and base address set to 0x20000000.

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 | 77 +++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 770a709..5569b99 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -28,6 +28,14 @@
 #include <media/v4l2-ioctl.h>
 #include <media/videobuf2-v4l2.h>
 #include <media/videobuf2-dma-contig.h>
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+#include <asm/dma-iommu.h>
+#include <linux/dma-iommu.h>
+#include <linux/dma-mapping.h>
+#include <linux/iommu.h>
+#include <linux/kref.h>
+#include <linux/of_platform.h>
+#endif
 
 #include "jpeg-core.h"
 #include "jpeg-hw-s5p.h"
@@ -35,6 +43,10 @@
 #include "jpeg-hw-exynos3250.h"
 #include "jpeg-regs.h"
 
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+static struct dma_iommu_mapping *mapping;
+#endif
+
 static struct s5p_jpeg_fmt sjpeg_formats[] = {
 	{
 		.name		= "JPEG JFIF",
@@ -956,6 +968,60 @@ static void exynos4_jpeg_parse_q_tbl(struct s5p_jpeg_ctx *ctx)
 	}
 }
 
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+static int jpeg_iommu_init(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int err;
+
+	mapping = arm_iommu_create_mapping(&platform_bus_type, 0x20000000,
+					   SZ_512M);
+	if (IS_ERR(mapping)) {
+		dev_err(dev, "IOMMU mapping failed\n");
+		return PTR_ERR(mapping);
+	}
+
+	dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), GFP_KERNEL);
+	if (!dev->dma_parms) {
+		err = -ENOMEM;
+		goto error_alloc;
+	}
+
+	err = dma_set_max_seg_size(dev, 0xffffffffu);
+	if (err)
+		goto error;
+
+	err = arm_iommu_attach_device(dev, mapping);
+	if (err)
+		goto error;
+
+	return 0;
+
+error:
+	devm_kfree(dev, dev->dma_parms);
+	dev->dma_parms = NULL;
+
+error_alloc:
+	arm_iommu_release_mapping(mapping);
+	mapping = NULL;
+
+	return err;
+}
+
+static void jpeg_iommu_deinit(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	if (mapping) {
+		arm_iommu_detach_device(dev);
+		devm_kfree(dev, dev->dma_parms);
+		dev->dma_parms = NULL;
+		arm_iommu_release_mapping(mapping);
+		mapping = NULL;
+	}
+}
+#endif
+
 /*
  * ============================================================================
  * Device file operations
@@ -2816,6 +2882,13 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
 	spin_lock_init(&jpeg->slock);
 	jpeg->dev = &pdev->dev;
 
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+	ret = jpeg_iommu_init(pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "IOMMU Initialization failed\n");
+		return ret;
+	}
+#endif
 	/* memory-mapped registers */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
@@ -2962,6 +3035,10 @@ static int s5p_jpeg_remove(struct platform_device *pdev)
 			clk_disable_unprepare(jpeg->clocks[i]);
 	}
 
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+	jpeg_iommu_deinit(pdev);
+#endif
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH 5/9] [media] s5p-jpeg: Add IOMMU support

[Jacek Anaszewski]

Cc Marek Szyprowski.

Marek, could you share your opinion about this patch?

On 06/02/2017 06:02 PM, Thierry Escande wrote:
> From: Tony K Nadackal <tony.kn@samsung.com>
> 
> This patch adds support for IOMMU s5p-jpeg driver if the Exynos IOMMU
> and ARM DMA IOMMU configurations are supported. The address space is
> created with size limited to 256M and base address set to 0x20000000.
> 
> 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 | 77 +++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 770a709..5569b99 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -28,6 +28,14 @@
>  #include <media/v4l2-ioctl.h>
>  #include <media/videobuf2-v4l2.h>
>  #include <media/videobuf2-dma-contig.h>
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +#include <asm/dma-iommu.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/iommu.h>
> +#include <linux/kref.h>
> +#include <linux/of_platform.h>
> +#endif
>  
>  #include "jpeg-core.h"
>  #include "jpeg-hw-s5p.h"
> @@ -35,6 +43,10 @@
>  #include "jpeg-hw-exynos3250.h"
>  #include "jpeg-regs.h"
>  
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +static struct dma_iommu_mapping *mapping;
> +#endif
> +
>  static struct s5p_jpeg_fmt sjpeg_formats[] = {
>  	{
>  		.name		= "JPEG JFIF",
> @@ -956,6 +968,60 @@ static void exynos4_jpeg_parse_q_tbl(struct s5p_jpeg_ctx *ctx)
>  	}
>  }
>  
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +static int jpeg_iommu_init(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int err;
> +
> +	mapping = arm_iommu_create_mapping(&platform_bus_type, 0x20000000,
> +					   SZ_512M);
> +	if (IS_ERR(mapping)) {
> +		dev_err(dev, "IOMMU mapping failed\n");
> +		return PTR_ERR(mapping);
> +	}
> +
> +	dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), GFP_KERNEL);
> +	if (!dev->dma_parms) {
> +		err = -ENOMEM;
> +		goto error_alloc;
> +	}
> +
> +	err = dma_set_max_seg_size(dev, 0xffffffffu);
> +	if (err)
> +		goto error;
> +
> +	err = arm_iommu_attach_device(dev, mapping);
> +	if (err)
> +		goto error;
> +
> +	return 0;
> +
> +error:
> +	devm_kfree(dev, dev->dma_parms);
> +	dev->dma_parms = NULL;
> +
> +error_alloc:
> +	arm_iommu_release_mapping(mapping);
> +	mapping = NULL;
> +
> +	return err;
> +}
> +
> +static void jpeg_iommu_deinit(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	if (mapping) {
> +		arm_iommu_detach_device(dev);
> +		devm_kfree(dev, dev->dma_parms);
> +		dev->dma_parms = NULL;
> +		arm_iommu_release_mapping(mapping);
> +		mapping = NULL;
> +	}
> +}
> +#endif
> +
>  /*
>   * ============================================================================
>   * Device file operations
> @@ -2816,6 +2882,13 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
>  	spin_lock_init(&jpeg->slock);
>  	jpeg->dev = &pdev->dev;
>  
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +	ret = jpeg_iommu_init(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "IOMMU Initialization failed\n");
> +		return ret;
> +	}
> +#endif
>  	/* memory-mapped registers */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  
> @@ -2962,6 +3035,10 @@ static int s5p_jpeg_remove(struct platform_device *pdev)
>  			clk_disable_unprepare(jpeg->clocks[i]);
>  	}
>  
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +	jpeg_iommu_deinit(pdev);
> +#endif
> +
>  	return 0;
>  }
>  
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 5/9] [media] s5p-jpeg: Add IOMMU support

[Marek Szyprowski]

Hi All,

I'm sorry for the late reply, I just got back from holidays.

On 2017-06-02 23:43, Jacek Anaszewski wrote:
> Cc Marek Szyprowski.
>
> Marek, could you share your opinion about this patch?
>
> On 06/02/2017 06:02 PM, Thierry Escande wrote:
>> From: Tony K Nadackal <tony.kn@samsung.com>
>>
>> This patch adds support for IOMMU s5p-jpeg driver if the Exynos IOMMU
>> and ARM DMA IOMMU configurations are supported. The address space is
>> created with size limited to 256M and base address set to 0x20000000.

Could you clarify WHY this patch is needed? IOMMU core configures per-device
IO address space by default and AFAIR JPEG module doesn't have any specific
requirements for the IO address space layout (base or size), so it should
work fine (and works in my tests!) without this patch.

Please drop this patch for now.

>> 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 | 77 +++++++++++++++++++++++++++++
>>   1 file changed, 77 insertions(+)
>>
>> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
>> index 770a709..5569b99 100644
>> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
>> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
>> @@ -28,6 +28,14 @@
>>   #include <media/v4l2-ioctl.h>
>>   #include <media/videobuf2-v4l2.h>
>>   #include <media/videobuf2-dma-contig.h>
>> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
>> +#include <asm/dma-iommu.h>
>> +#include <linux/dma-iommu.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/iommu.h>
>> +#include <linux/kref.h>
>> +#include <linux/of_platform.h>
>> +#endif
>>   
>>   #include "jpeg-core.h"
>>   #include "jpeg-hw-s5p.h"
>> @@ -35,6 +43,10 @@
>>   #include "jpeg-hw-exynos3250.h"
>>   #include "jpeg-regs.h"
>>   
>> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
>> +static struct dma_iommu_mapping *mapping;
>> +#endif
>> +
>>   static struct s5p_jpeg_fmt sjpeg_formats[] = {
>>   	{
>>   		.name		= "JPEG JFIF",
>> @@ -956,6 +968,60 @@ static void exynos4_jpeg_parse_q_tbl(struct s5p_jpeg_ctx *ctx)
>>   	}
>>   }
>>   
>> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
>> +static int jpeg_iommu_init(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int err;
>> +
>> +	mapping = arm_iommu_create_mapping(&platform_bus_type, 0x20000000,
>> +					   SZ_512M);
>> +	if (IS_ERR(mapping)) {
>> +		dev_err(dev, "IOMMU mapping failed\n");
>> +		return PTR_ERR(mapping);
>> +	}
>> +
>> +	dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), GFP_KERNEL);
>> +	if (!dev->dma_parms) {
>> +		err = -ENOMEM;
>> +		goto error_alloc;
>> +	}
>> +
>> +	err = dma_set_max_seg_size(dev, 0xffffffffu);
>> +	if (err)
>> +		goto error;
>> +
>> +	err = arm_iommu_attach_device(dev, mapping);
>> +	if (err)
>> +		goto error;
>> +
>> +	return 0;
>> +
>> +error:
>> +	devm_kfree(dev, dev->dma_parms);
>> +	dev->dma_parms = NULL;
>> +
>> +error_alloc:
>> +	arm_iommu_release_mapping(mapping);
>> +	mapping = NULL;
>> +
>> +	return err;
>> +}
>> +
>> +static void jpeg_iommu_deinit(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +
>> +	if (mapping) {
>> +		arm_iommu_detach_device(dev);
>> +		devm_kfree(dev, dev->dma_parms);
>> +		dev->dma_parms = NULL;
>> +		arm_iommu_release_mapping(mapping);
>> +		mapping = NULL;
>> +	}
>> +}
>> +#endif
>> +
>>   /*
>>    * ============================================================================
>>    * Device file operations
>> @@ -2816,6 +2882,13 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
>>   	spin_lock_init(&jpeg->slock);
>>   	jpeg->dev = &pdev->dev;
>>   
>> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
>> +	ret = jpeg_iommu_init(pdev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "IOMMU Initialization failed\n");
>> +		return ret;
>> +	}
>> +#endif
>>   	/* memory-mapped registers */
>>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>   
>> @@ -2962,6 +3035,10 @@ static int s5p_jpeg_remove(struct platform_device *pdev)
>>   			clk_disable_unprepare(jpeg->clocks[i]);
>>   	}
>>   
>> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
>> +	jpeg_iommu_deinit(pdev);
>> +#endif
>> +
>>   	return 0;
>>   }
>>   
>>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 5/9] [media] s5p-jpeg: Add IOMMU support

[Shuah Khan]

On Fri, Jun 2, 2017 at 10:02 AM, Thierry Escande
<thierry.escande@collabora.com> wrote:
> From: Tony K Nadackal <tony.kn@samsung.com>
>
> This patch adds support for IOMMU s5p-jpeg driver if the Exynos IOMMU
> and ARM DMA IOMMU configurations are supported. The address space is
> created with size limited to 256M and base address set to 0x20000000.
>
> 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 | 77 +++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 770a709..5569b99 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -28,6 +28,14 @@
>  #include <media/v4l2-ioctl.h>
>  #include <media/videobuf2-v4l2.h>
>  #include <media/videobuf2-dma-contig.h>
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +#include <asm/dma-iommu.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/iommu.h>
> +#include <linux/kref.h>
> +#include <linux/of_platform.h>
> +#endif
>
>  #include "jpeg-core.h"
>  #include "jpeg-hw-s5p.h"
> @@ -35,6 +43,10 @@
>  #include "jpeg-hw-exynos3250.h"
>  #include "jpeg-regs.h"
>
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +static struct dma_iommu_mapping *mapping;
> +#endif
> +
>  static struct s5p_jpeg_fmt sjpeg_formats[] = {
>         {
>                 .name           = "JPEG JFIF",
> @@ -956,6 +968,60 @@ static void exynos4_jpeg_parse_q_tbl(struct s5p_jpeg_ctx *ctx)
>         }
>  }
>
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +static int jpeg_iommu_init(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       int err;
> +
> +       mapping = arm_iommu_create_mapping(&platform_bus_type, 0x20000000,
> +                                          SZ_512M);

Change log says 256M??

What happens when another driver uses the same start point?
exynos drm uses the same  looks like

EXYNOS_DEV_ADDR_START   0x20000000

> +       if (IS_ERR(mapping)) {
> +               dev_err(dev, "IOMMU mapping failed\n");
> +               return PTR_ERR(mapping);
> +       }
> +
> +       dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), GFP_KERNEL);
> +       if (!dev->dma_parms) {
> +               err = -ENOMEM;
> +               goto error_alloc;
> +       }
> +
> +       err = dma_set_max_seg_size(dev, 0xffffffffu);

You could use DMA_BIT_MASK(32) instead of 0xffffffffu

> +       if (err)
> +               goto error;
> +
> +       err = arm_iommu_attach_device(dev, mapping);
> +       if (err)
> +               goto error;
> +
> +       return 0;
> +
> +error:
> +       devm_kfree(dev, dev->dma_parms);
> +       dev->dma_parms = NULL;
> +
> +error_alloc:
> +       arm_iommu_release_mapping(mapping);
> +       mapping = NULL;
> +
> +       return err;
> +}
> +
> +static void jpeg_iommu_deinit(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +
> +       if (mapping) {
> +               arm_iommu_detach_device(dev);
> +               devm_kfree(dev, dev->dma_parms);
> +               dev->dma_parms = NULL;
> +               arm_iommu_release_mapping(mapping);
> +               mapping = NULL;
> +       }
> +}
> +#endif
> +
>  /*
>   * ============================================================================
>   * Device file operations
> @@ -2816,6 +2882,13 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
>         spin_lock_init(&jpeg->slock);
>         jpeg->dev = &pdev->dev;
>
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +       ret = jpeg_iommu_init(pdev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "IOMMU Initialization failed\n");
> +               return ret;
> +       }
> +#endif

You might be able to avoid use of ifdefs if you define stubs for !defines case.

>         /* memory-mapped registers */
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> @@ -2962,6 +3035,10 @@ static int s5p_jpeg_remove(struct platform_device *pdev)
>                         clk_disable_unprepare(jpeg->clocks[i]);
>         }
>
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +       jpeg_iommu_deinit(pdev);
> +#endif
> +
>         return 0;
>  }
>
> --
> 2.7.4
>

Re: [PATCH 5/9] [media] s5p-jpeg: Add IOMMU support

[Sylwester Nawrocki]

On 06/02/2017 06:02 PM, Thierry Escande wrote:
> From: Tony K Nadackal <tony.kn@samsung.com>
> 
> This patch adds support for IOMMU s5p-jpeg driver if the Exynos IOMMU
> and ARM DMA IOMMU configurations are supported. The address space is
> created with size limited to 256M and base address set to 0x20000000.

I don't think this patch is needed now, a few things changed in mainline
since v3.8. The mapping is being created automatically now for this single
JPEG CODEC device by the driver core/dma-mapping code AFAICS.
See dma_configure() in drivers/base/dd.c.
I doubt we need a specific CPU address range, but even if we would shouldn't
it be specified through the dma-ranges DT property?

> 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 | 77 +++++++++++++++++++++++++++++
>   1 file changed, 77 insertions(+)

> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +static int jpeg_iommu_init(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int err;
> +
> +	mapping = arm_iommu_create_mapping(&platform_bus_type, 0x20000000,
> +					   SZ_512M);
> +	if (IS_ERR(mapping)) {
> +		dev_err(dev, "IOMMU mapping failed\n");
> +		return PTR_ERR(mapping);
> +	}
> +
> +	dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), GFP_KERNEL);

dev->dma_parms seems to be unused.

> +	if (!dev->dma_parms) {
> +		err = -ENOMEM;
> +		goto error_alloc;
> +	}
> +
> +	err = dma_set_max_seg_size(dev, 0xffffffffu);
> +	if (err)
> +		goto error;
> +
> +	err = arm_iommu_attach_device(dev, mapping);
> +	if (err)
> +		goto error;
> +
> +	return 0;
> +
> +error:
> +	devm_kfree(dev, dev->dma_parms);

There is no need for this devm_kfree() call.

> +	dev->dma_parms = NULL;
> +
> +error_alloc:
> +	arm_iommu_release_mapping(mapping);
> +	mapping = NULL;
> +
> +	return err;
> +}
> +
> +static void jpeg_iommu_deinit(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	if (mapping) {
> +		arm_iommu_detach_device(dev);
> +		devm_kfree(dev, dev->dma_parms);

Ditto.

> +		dev->dma_parms = NULL;
> +		arm_iommu_release_mapping(mapping);
> +		mapping = NULL;
> +	}
> +}

>   /*
>    * ============================================================================
>    * Device file operations
> @@ -2816,6 +2882,13 @@ static int s5p_jpeg_probe(struct platform_device *pdev)

> +	ret = jpeg_iommu_init(pdev);

> @@ -2962,6 +3035,10 @@ static int s5p_jpeg_remove(struct platform_device *pdev)

> +	jpeg_iommu_deinit(pdev);

>   	return 0;
>   }

--
Thanks,
Sylwester

[PATCH 6/9] [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.

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

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 5569b99..7a7acbc 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>
@@ -1416,8 +1417,17 @@ static int s5p_jpeg_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 	q_data = get_q_data(ct, f->type);
 	BUG_ON(q_data == NULL);
 
-	pix->width = q_data->w;
-	pix->height = q_data->h;
+	if ((f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+	     ct->mode == S5P_JPEG_ENCODE) ||
+	    (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
+	     ct->mode == S5P_JPEG_DECODE)) {
+		pix->width = 0;
+		pix->height = 0;
+	} else {
+		pix->width = q_data->w;
+		pix->height = q_data->h;
+	}
+
 	pix->field = V4L2_FIELD_NONE;
 	pix->pixelformat = q_data->fmt->fourcc;
 	pix->bytesperline = 0;
@@ -1677,8 +1687,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
@@ -1686,6 +1694,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,
@@ -1761,6 +1771,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)
 {
@@ -2086,6 +2105,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,
 };
 
 /*
@@ -2478,8 +2500,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;
 }
 
@@ -2558,6 +2589,21 @@ 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;
+
+	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);
@@ -2565,9 +2611,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);
@@ -2576,31 +2633,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;
-
-		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 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.
+		 */
+		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);
@@ -2620,6 +2664,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 6/9] [media] s5p-jpeg: Add support for resolution change event

[Jacek Anaszewski]

Hi Thierry,

On 06/02/2017 06:02 PM, Thierry Escande wrote:
> 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.

Do you have a use case for that?

> 
> 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 | 121 ++++++++++++++++++++--------
>  drivers/media/platform/s5p-jpeg/jpeg-core.h |   7 ++
>  2 files changed, 95 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 5569b99..7a7acbc 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>
> @@ -1416,8 +1417,17 @@ static int s5p_jpeg_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	q_data = get_q_data(ct, f->type);
>  	BUG_ON(q_data == NULL);
>  
> -	pix->width = q_data->w;
> -	pix->height = q_data->h;
> +	if ((f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> +	     ct->mode == S5P_JPEG_ENCODE) ||
> +	    (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> +	     ct->mode == S5P_JPEG_DECODE)) {
> +		pix->width = 0;
> +		pix->height = 0;
> +	} else {
> +		pix->width = q_data->w;
> +		pix->height = q_data->h;
> +	}
> +

Is this change related to the patch subject?

>  	pix->field = V4L2_FIELD_NONE;
>  	pix->pixelformat = q_data->fmt->fourcc;
>  	pix->bytesperline = 0;
> @@ -1677,8 +1687,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
> @@ -1686,6 +1694,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,
> @@ -1761,6 +1771,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)
>  {
> @@ -2086,6 +2105,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,
>  };
>  
>  /*
> @@ -2478,8 +2500,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;
>  }
>  
> @@ -2558,6 +2589,21 @@ 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;
> +
> +	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);
> @@ -2565,9 +2611,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);
> @@ -2576,31 +2633,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;

You're removing here quantization and Huffman table info, is it
intentional?

> -
> -		q_data = &ctx->cap_q;
> -		q_data->w = tmp.w;
> -		q_data->h = tmp.h;
> -
> -		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 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.
> +		 */
> +		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);
> @@ -2620,6 +2664,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;
>  };
>  
>  /**
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 6/9] [media] s5p-jpeg: Add support for resolution change event

[Thierry Escande]

Hi Jacek,

On 02/06/2017 23:53, Jacek Anaszewski wrote:
> Hi Thierry,
> 
> On 06/02/2017 06:02 PM, Thierry Escande wrote:
>> 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.
> 
> Do you have a use case for that?
Sorry but no. Again, the entry in the chromeos bug tracker does not 
mention any use case.

>> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
>> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
>> -	pix->width = q_data->w;
>> -	pix->height = q_data->h;
>> +	if ((f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
>> +	     ct->mode == S5P_JPEG_ENCODE) ||
>> +	    (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
>> +	     ct->mode == S5P_JPEG_DECODE)) {
>> +		pix->width = 0;
>> +		pix->height = 0;
>> +	} else {
>> +		pix->width = q_data->w;
>> +		pix->height = q_data->h;
>> +	}
>> +
> 
> Is this change related to the patch subject?
Hum... Not sure indeed. I'll remove that from the v2.

>> +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;
>> +
>> +	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);
>> @@ -2565,9 +2611,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);
>> @@ -2576,31 +2633,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;
> 
> You're removing here quantization and Huffman table info, is it
> intentional?
ctx->out_q is now passed directly to s5p_jpeg_parse_hdr(). This avoids 
this field-by-field copy already done in s5p_jpeg_parse_hdr().
This do not remove anything unless I'm missing something here...

Regards,
  Thierry

[PATCH 7/9] [media] s5p-jpeg: Change sclk_jpeg to 166MHz for Exynos5250

[Thierry Escande]

From: henryhsu <henryhsu@chromium.org>

The default clock parent of jpeg on Exynos5250 is fin_pll, which is
24MHz. We have to change the clock parent to CPLL, which is 333MHz,
and set sclk_jpeg to 166MHz.

Signed-off-by: Heng-Ruey Hsu <henryhsu@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 47 +++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 7a7acbc..430e925 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -969,6 +969,44 @@ static void exynos4_jpeg_parse_q_tbl(struct s5p_jpeg_ctx *ctx)
 	}
 }
 
+static int exynos4_jpeg_set_sclk_rate(struct s5p_jpeg *jpeg, struct clk *sclk)
+{
+	struct clk *mout_jpeg;
+	struct clk *sclk_cpll;
+	int ret;
+
+	mout_jpeg = clk_get(jpeg->dev, "mout_jpeg");
+	if (IS_ERR(mout_jpeg)) {
+		dev_err(jpeg->dev, "mout_jpeg clock not available: %ld\n",
+			PTR_ERR(mout_jpeg));
+		return PTR_ERR(mout_jpeg);
+	}
+
+	sclk_cpll = clk_get(jpeg->dev, "sclk_cpll");
+	if (IS_ERR(sclk_cpll)) {
+		dev_err(jpeg->dev, "sclk_cpll clock not available: %ld\n",
+			PTR_ERR(sclk_cpll));
+		clk_put(mout_jpeg);
+		return PTR_ERR(sclk_cpll);
+	}
+
+	ret = clk_set_parent(mout_jpeg, sclk_cpll);
+	clk_put(sclk_cpll);
+	clk_put(mout_jpeg);
+	if (ret) {
+		dev_err(jpeg->dev, "clk_set_parent failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_set_rate(sclk, 166500 * 1000);
+	if (ret) {
+		dev_err(jpeg->dev, "clk_set_rate failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 #if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
 static int jpeg_iommu_init(struct platform_device *pdev)
 {
@@ -2974,6 +3012,15 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
 				jpeg->variant->clk_names[i]);
 			return PTR_ERR(jpeg->clocks[i]);
 		}
+
+		if (jpeg->variant->version == SJPEG_EXYNOS4 &&
+		    !strncmp(jpeg->variant->clk_names[i],
+			     "sclk", strlen("sclk"))) {
+			ret = exynos4_jpeg_set_sclk_rate(jpeg,
+							 jpeg->clocks[i]);
+			if (ret)
+				return ret;
+		}
 	}
 
 	/* v4l2 device */
-- 
2.7.4

Re: [PATCH 7/9] [media] s5p-jpeg: Change sclk_jpeg to 166MHz for Exynos5250

[Jacek Anaszewski]

Cc Marek and Sylwester.

On 06/02/2017 06:02 PM, Thierry Escande wrote:
> From: henryhsu <henryhsu@chromium.org>
> 
> The default clock parent of jpeg on Exynos5250 is fin_pll, which is
> 24MHz. We have to change the clock parent to CPLL, which is 333MHz,
> and set sclk_jpeg to 166MHz.
> 
> Signed-off-by: Heng-Ruey Hsu <henryhsu@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/media/platform/s5p-jpeg/jpeg-core.c | 47 +++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 7a7acbc..430e925 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -969,6 +969,44 @@ static void exynos4_jpeg_parse_q_tbl(struct s5p_jpeg_ctx *ctx)
>  	}
>  }
>  
> +static int exynos4_jpeg_set_sclk_rate(struct s5p_jpeg *jpeg, struct clk *sclk)

Why here exynos4 and in the subject Exynos5250?

> +{
> +	struct clk *mout_jpeg;
> +	struct clk *sclk_cpll;
> +	int ret;
> +
> +	mout_jpeg = clk_get(jpeg->dev, "mout_jpeg");
> +	if (IS_ERR(mout_jpeg)) {
> +		dev_err(jpeg->dev, "mout_jpeg clock not available: %ld\n",
> +			PTR_ERR(mout_jpeg));
> +		return PTR_ERR(mout_jpeg);
> +	}
> +
> +	sclk_cpll = clk_get(jpeg->dev, "sclk_cpll");
> +	if (IS_ERR(sclk_cpll)) {
> +		dev_err(jpeg->dev, "sclk_cpll clock not available: %ld\n",
> +			PTR_ERR(sclk_cpll));
> +		clk_put(mout_jpeg);
> +		return PTR_ERR(sclk_cpll);
> +	}
> +
> +	ret = clk_set_parent(mout_jpeg, sclk_cpll);
> +	clk_put(sclk_cpll);
> +	clk_put(mout_jpeg);
> +	if (ret) {
> +		dev_err(jpeg->dev, "clk_set_parent failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_set_rate(sclk, 166500 * 1000);
> +	if (ret) {
> +		dev_err(jpeg->dev, "clk_set_rate failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  #if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
>  static int jpeg_iommu_init(struct platform_device *pdev)
>  {
> @@ -2974,6 +3012,15 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
>  				jpeg->variant->clk_names[i]);
>  			return PTR_ERR(jpeg->clocks[i]);
>  		}
> +
> +		if (jpeg->variant->version == SJPEG_EXYNOS4 &&
> +		    !strncmp(jpeg->variant->clk_names[i],
> +			     "sclk", strlen("sclk"))) {
> +			ret = exynos4_jpeg_set_sclk_rate(jpeg,
> +							 jpeg->clocks[i]);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	/* v4l2 device */
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 7/9] [media] s5p-jpeg: Change sclk_jpeg to 166MHz for Exynos5250

[Sylwester Nawrocki]

On 06/02/2017 11:58 PM, Jacek Anaszewski wrote:
> On 06/02/2017 06:02 PM, Thierry Escande wrote:
>> From: henryhsu<henryhsu@chromium.org>
>>
>> The default clock parent of jpeg on Exynos5250 is fin_pll, which is
>> 24MHz. We have to change the clock parent to CPLL, which is 333MHz,
>> and set sclk_jpeg to 166MHz.

There is no need to patch the driver for these platform specific clock
settings, it can be specified in the device tree with the "assigned-clocks"
properties. There is an example in mainline for exynos3250 SoC already [1].

-- 
Thanks,
Sylwester

[1] 
http://elixir.free-electrons.com/linux/v4.6/source/arch/arm/boot/dts/exynos3250.dtsi#L263

[PATCH 8/9] [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 | 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 430e925..db56135 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2894,6 +2894,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);
@@ -2910,6 +2911,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)
@@ -2926,7 +2932,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

[PATCH 9/9] [media] s5p-jpeg: Add support for multi-planar APIs

[Thierry Escande]

From: Ricky Liang <jcliang@chromium.org>

This patch adds multi-planar APIs to the s5p-jpeg driver. The multi-planar
APIs are identical to the exisiting single-planar APIs except the plane
format info is stored in the v4l2_pixel_format_mplan struct instead
of the v4l2_pixel_format struct.

Signed-off-by: Ricky Liang <jcliang@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 152 +++++++++++++++++++++++++---
 drivers/media/platform/s5p-jpeg/jpeg-core.h |   2 +
 2 files changed, 139 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index db56135..a8fd7ed 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1371,6 +1371,15 @@ static int s5p_jpeg_querycap(struct file *file, void *priv,
 		 dev_name(ctx->jpeg->dev));
 	cap->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_M2M;
 	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
+	/*
+	 * Advertise multi-planar capabilities. The driver supports only
+	 * single-planar pixel format at this moment so all the buffers will
+	 * have only one plane.
+	 */
+	cap->capabilities |= V4L2_CAP_VIDEO_M2M_MPLANE |
+			     V4L2_CAP_VIDEO_CAPTURE_MPLANE |
+			     V4L2_CAP_VIDEO_OUTPUT_MPLANE;
+
 	return 0;
 }
 
@@ -1430,12 +1439,10 @@ static int s5p_jpeg_enum_fmt_vid_out(struct file *file, void *priv,
 static struct s5p_jpeg_q_data *get_q_data(struct s5p_jpeg_ctx *ctx,
 					  enum v4l2_buf_type type)
 {
-	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
+	if (V4L2_TYPE_IS_OUTPUT(type))
 		return &ctx->out_q;
-	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		return &ctx->cap_q;
 
-	return NULL;
+	return &ctx->cap_q;
 }
 
 static int s5p_jpeg_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
@@ -1449,16 +1456,14 @@ static int s5p_jpeg_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 	if (!vq)
 		return -EINVAL;
 
-	if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+	if (!V4L2_TYPE_IS_OUTPUT(f->type) &&
 	    ct->mode == S5P_JPEG_DECODE && !ct->hdr_parsed)
 		return -EINVAL;
 	q_data = get_q_data(ct, f->type);
 	BUG_ON(q_data == NULL);
 
-	if ((f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
-	     ct->mode == S5P_JPEG_ENCODE) ||
-	    (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
-	     ct->mode == S5P_JPEG_DECODE)) {
+	if ((!V4L2_TYPE_IS_OUTPUT(f->type) && ct->mode == S5P_JPEG_ENCODE) ||
+	    (V4L2_TYPE_IS_OUTPUT(f->type) && ct->mode == S5P_JPEG_DECODE)) {
 		pix->width = 0;
 		pix->height = 0;
 	} else {
@@ -1715,6 +1720,8 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct v4l2_format *f)
 
 	q_data = get_q_data(ct, f->type);
 	BUG_ON(q_data == NULL);
+	vq->type = f->type;
+	q_data->type = f->type;
 
 	if (vb2_is_busy(vq)) {
 		v4l2_err(&ct->jpeg->v4l2_dev, "%s queue busy\n", __func__);
@@ -1919,7 +1926,9 @@ static int s5p_jpeg_g_selection(struct file *file, void *priv,
 	struct s5p_jpeg_ctx *ctx = fh_to_ctx(priv);
 
 	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
-	    s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+	    s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+	    s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
+	    s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
 		return -EINVAL;
 
 	/* For JPEG blob active == default == bounds */
@@ -1957,7 +1966,8 @@ static int s5p_jpeg_s_selection(struct file *file, void *fh,
 	struct v4l2_rect *rect = &s->r;
 	int ret = -EINVAL;
 
-	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+	    s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
 		return -EINVAL;
 
 	if (s->target == V4L2_SEL_TGT_COMPOSE) {
@@ -2118,6 +2128,107 @@ static int s5p_jpeg_controls_create(struct s5p_jpeg_ctx *ctx)
 	return ret;
 }
 
+static void v4l2_format_pixmp_to_pix(struct v4l2_format *fmt_pix_mp,
+				     struct v4l2_format *fmt_pix) {
+	struct v4l2_pix_format *pix = &fmt_pix->fmt.pix;
+	struct v4l2_pix_format_mplane *pix_mp = &fmt_pix_mp->fmt.pix_mp;
+
+	fmt_pix->type = fmt_pix_mp->type;
+	pix->width = pix_mp->width;
+	pix->height = pix_mp->height;
+	pix->pixelformat = pix_mp->pixelformat;
+	pix->field = pix_mp->field;
+	pix->colorspace = pix_mp->colorspace;
+	pix->bytesperline = pix_mp->plane_fmt[0].bytesperline;
+	pix->sizeimage = pix_mp->plane_fmt[0].sizeimage;
+}
+
+static void v4l2_format_pixmp_from_pix(struct v4l2_format *fmt_pix_mp,
+				       struct v4l2_format *fmt_pix) {
+	struct v4l2_pix_format *pix = &fmt_pix->fmt.pix;
+	struct v4l2_pix_format_mplane *pix_mp = &fmt_pix_mp->fmt.pix_mp;
+
+	fmt_pix_mp->type = fmt_pix->type;
+	pix_mp->width = pix->width;
+	pix_mp->height = pix->height;
+	pix_mp->pixelformat = pix->pixelformat;
+	pix_mp->field = pix->field;
+	pix_mp->colorspace = pix->colorspace;
+	pix_mp->plane_fmt[0].bytesperline = pix->bytesperline;
+	pix_mp->plane_fmt[0].sizeimage = pix->sizeimage;
+	pix_mp->num_planes = 1;
+}
+
+static int s5p_jpeg_g_fmt_mplane(struct file *file, void *priv,
+				 struct v4l2_format *f)
+{
+	struct v4l2_format tmp;
+	int ret;
+
+	memset(&tmp, 0, sizeof(tmp));
+	v4l2_format_pixmp_to_pix(f, &tmp);
+	ret = s5p_jpeg_g_fmt(file, priv, &tmp);
+	v4l2_format_pixmp_from_pix(f, &tmp);
+
+	return ret;
+}
+
+static int s5p_jpeg_try_fmt_vid_cap_mplane(struct file *file, void *priv,
+					   struct v4l2_format *f)
+{
+	struct v4l2_format tmp;
+	int ret;
+
+	memset(&tmp, 0, sizeof(tmp));
+	v4l2_format_pixmp_to_pix(f, &tmp);
+	ret = s5p_jpeg_try_fmt_vid_cap(file, priv, &tmp);
+	v4l2_format_pixmp_from_pix(f, &tmp);
+
+	return ret;
+}
+
+static int s5p_jpeg_try_fmt_vid_out_mplane(struct file *file, void *priv,
+					   struct v4l2_format *f)
+{
+	struct v4l2_format tmp;
+	int ret;
+
+	memset(&tmp, 0, sizeof(tmp));
+	v4l2_format_pixmp_to_pix(f, &tmp);
+	ret = s5p_jpeg_try_fmt_vid_out(file, priv, &tmp);
+	v4l2_format_pixmp_from_pix(f, &tmp);
+
+	return ret;
+}
+
+static int s5p_jpeg_s_fmt_vid_cap_mplane(struct file *file, void *priv,
+					 struct v4l2_format *f)
+{
+	struct v4l2_format tmp;
+	int ret;
+
+	memset(&tmp, 0, sizeof(tmp));
+	v4l2_format_pixmp_to_pix(f, &tmp);
+	ret = s5p_jpeg_s_fmt_vid_cap(file, priv, &tmp);
+	v4l2_format_pixmp_from_pix(f, &tmp);
+
+	return ret;
+}
+
+static int s5p_jpeg_s_fmt_vid_out_mplane(struct file *file, void *priv,
+					 struct v4l2_format *f)
+{
+	struct v4l2_format tmp;
+	int ret;
+
+	memset(&tmp, 0, sizeof(tmp));
+	v4l2_format_pixmp_to_pix(f, &tmp);
+	ret = s5p_jpeg_s_fmt_vid_out(file, priv, &tmp);
+	v4l2_format_pixmp_from_pix(f, &tmp);
+
+	return ret;
+}
+
 static const struct v4l2_ioctl_ops s5p_jpeg_ioctl_ops = {
 	.vidioc_querycap		= s5p_jpeg_querycap,
 
@@ -2133,6 +2244,18 @@ static const struct v4l2_ioctl_ops s5p_jpeg_ioctl_ops = {
 	.vidioc_s_fmt_vid_cap		= s5p_jpeg_s_fmt_vid_cap,
 	.vidioc_s_fmt_vid_out		= s5p_jpeg_s_fmt_vid_out,
 
+	.vidioc_enum_fmt_vid_cap_mplane	= s5p_jpeg_enum_fmt_vid_cap,
+	.vidioc_enum_fmt_vid_out_mplane	= s5p_jpeg_enum_fmt_vid_out,
+
+	.vidioc_g_fmt_vid_cap_mplane	= s5p_jpeg_g_fmt_mplane,
+	.vidioc_g_fmt_vid_out_mplane	= s5p_jpeg_g_fmt_mplane,
+
+	.vidioc_try_fmt_vid_cap_mplane	= s5p_jpeg_try_fmt_vid_cap_mplane,
+	.vidioc_try_fmt_vid_out_mplane	= s5p_jpeg_try_fmt_vid_out_mplane,
+
+	.vidioc_s_fmt_vid_cap_mplane	= s5p_jpeg_s_fmt_vid_cap_mplane,
+	.vidioc_s_fmt_vid_out_mplane	= s5p_jpeg_s_fmt_vid_out_mplane,
+
 	.vidioc_reqbufs			= v4l2_m2m_ioctl_reqbufs,
 	.vidioc_querybuf		= v4l2_m2m_ioctl_querybuf,
 	.vidioc_qbuf			= v4l2_m2m_ioctl_qbuf,
@@ -2648,7 +2771,7 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
 	struct s5p_jpeg_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
 
 	if (ctx->mode == S5P_JPEG_DECODE &&
-	    vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+	    vb->vb2_queue->type == ctx->out_q.type) {
 		static const struct v4l2_event ev_src_ch = {
 			.type = V4L2_EVENT_SOURCE_CHANGE,
 			.u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
@@ -2657,8 +2780,7 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
 		u32 ori_w;
 		u32 ori_h;
 
-		dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
-					 V4L2_BUF_TYPE_VIDEO_CAPTURE);
+		dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, ctx->cap_q.type);
 		ori_w = ctx->out_q.w;
 		ori_h = ctx->out_q.h;
 
@@ -2708,7 +2830,7 @@ static void s5p_jpeg_stop_streaming(struct vb2_queue *q)
 	 * subsampling. Update capture queue when the stream is off.
 	 */
 	if (ctx->state == JPEGCTX_RESOLUTION_CHANGE &&
-	    q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+	    !V4L2_TYPE_IS_OUTPUT(q->type)) {
 		s5p_jpeg_set_capture_queue_data(ctx);
 		ctx->state = JPEGCTX_RUNNING;
 	}
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.h b/drivers/media/platform/s5p-jpeg/jpeg-core.h
index 9aa26bd..302a297 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.h
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.h
@@ -196,6 +196,7 @@ struct s5p_jpeg_marker {
  * @sof_len:	SOF0 marker's payload length (without length field itself)
  * @components:	number of image components
  * @size:	image buffer size in bytes
+ * @type:	buffer type of the queue (enum v4l2_buf_type)
  */
 struct s5p_jpeg_q_data {
 	struct s5p_jpeg_fmt	*fmt;
@@ -208,6 +209,7 @@ struct s5p_jpeg_q_data {
 	u32			sof_len;
 	u32			components;
 	u32			size;
+	u32			type;
 };
 
 /**
-- 
2.7.4

Re: [PATCH 9/9] [media] s5p-jpeg: Add support for multi-planar APIs

[Jacek Anaszewski]

Hi Thierry,

What is the gain of introducing multiplanar API for this hardware?
AFAIR all the HW implementations store the data in a single contiguous
memory region and use suitable padding between planes.

On 06/02/2017 06:02 PM, Thierry Escande wrote:
> From: Ricky Liang <jcliang@chromium.org>
> 
> This patch adds multi-planar APIs to the s5p-jpeg driver. The multi-planar
> APIs are identical to the exisiting single-planar APIs except the plane
> format info is stored in the v4l2_pixel_format_mplan struct instead
> of the v4l2_pixel_format struct.
> 
> Signed-off-by: Ricky Liang <jcliang@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/media/platform/s5p-jpeg/jpeg-core.c | 152 +++++++++++++++++++++++++---
>  drivers/media/platform/s5p-jpeg/jpeg-core.h |   2 +
>  2 files changed, 139 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index db56135..a8fd7ed 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -1371,6 +1371,15 @@ static int s5p_jpeg_querycap(struct file *file, void *priv,
>  		 dev_name(ctx->jpeg->dev));
>  	cap->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_M2M;
>  	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> +	/*
> +	 * Advertise multi-planar capabilities. The driver supports only
> +	 * single-planar pixel format at this moment so all the buffers will
> +	 * have only one plane.
> +	 */
> +	cap->capabilities |= V4L2_CAP_VIDEO_M2M_MPLANE |
> +			     V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> +			     V4L2_CAP_VIDEO_OUTPUT_MPLANE;
> +
>  	return 0;
>  }
>  
> @@ -1430,12 +1439,10 @@ static int s5p_jpeg_enum_fmt_vid_out(struct file *file, void *priv,
>  static struct s5p_jpeg_q_data *get_q_data(struct s5p_jpeg_ctx *ctx,
>  					  enum v4l2_buf_type type)
>  {
> -	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> +	if (V4L2_TYPE_IS_OUTPUT(type))
>  		return &ctx->out_q;
> -	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> -		return &ctx->cap_q;
>  
> -	return NULL;
> +	return &ctx->cap_q;
>  }
>  
>  static int s5p_jpeg_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
> @@ -1449,16 +1456,14 @@ static int s5p_jpeg_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	if (!vq)
>  		return -EINVAL;
>  
> -	if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> +	if (!V4L2_TYPE_IS_OUTPUT(f->type) &&
>  	    ct->mode == S5P_JPEG_DECODE && !ct->hdr_parsed)
>  		return -EINVAL;
>  	q_data = get_q_data(ct, f->type);
>  	BUG_ON(q_data == NULL);
>  
> -	if ((f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> -	     ct->mode == S5P_JPEG_ENCODE) ||
> -	    (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> -	     ct->mode == S5P_JPEG_DECODE)) {
> +	if ((!V4L2_TYPE_IS_OUTPUT(f->type) && ct->mode == S5P_JPEG_ENCODE) ||
> +	    (V4L2_TYPE_IS_OUTPUT(f->type) && ct->mode == S5P_JPEG_DECODE)) {
>  		pix->width = 0;
>  		pix->height = 0;
>  	} else {
> @@ -1715,6 +1720,8 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct v4l2_format *f)
>  
>  	q_data = get_q_data(ct, f->type);
>  	BUG_ON(q_data == NULL);
> +	vq->type = f->type;
> +	q_data->type = f->type;
>  
>  	if (vb2_is_busy(vq)) {
>  		v4l2_err(&ct->jpeg->v4l2_dev, "%s queue busy\n", __func__);
> @@ -1919,7 +1926,9 @@ static int s5p_jpeg_g_selection(struct file *file, void *priv,
>  	struct s5p_jpeg_ctx *ctx = fh_to_ctx(priv);
>  
>  	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> -	    s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +	    s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> +	    s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
> +	    s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>  		return -EINVAL;
>  
>  	/* For JPEG blob active == default == bounds */
> @@ -1957,7 +1966,8 @@ static int s5p_jpeg_s_selection(struct file *file, void *fh,
>  	struct v4l2_rect *rect = &s->r;
>  	int ret = -EINVAL;
>  
> -	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> +	    s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>  		return -EINVAL;
>  
>  	if (s->target == V4L2_SEL_TGT_COMPOSE) {
> @@ -2118,6 +2128,107 @@ static int s5p_jpeg_controls_create(struct s5p_jpeg_ctx *ctx)
>  	return ret;
>  }
>  
> +static void v4l2_format_pixmp_to_pix(struct v4l2_format *fmt_pix_mp,
> +				     struct v4l2_format *fmt_pix) {
> +	struct v4l2_pix_format *pix = &fmt_pix->fmt.pix;
> +	struct v4l2_pix_format_mplane *pix_mp = &fmt_pix_mp->fmt.pix_mp;
> +
> +	fmt_pix->type = fmt_pix_mp->type;
> +	pix->width = pix_mp->width;
> +	pix->height = pix_mp->height;
> +	pix->pixelformat = pix_mp->pixelformat;
> +	pix->field = pix_mp->field;
> +	pix->colorspace = pix_mp->colorspace;
> +	pix->bytesperline = pix_mp->plane_fmt[0].bytesperline;
> +	pix->sizeimage = pix_mp->plane_fmt[0].sizeimage;
> +}
> +
> +static void v4l2_format_pixmp_from_pix(struct v4l2_format *fmt_pix_mp,
> +				       struct v4l2_format *fmt_pix) {
> +	struct v4l2_pix_format *pix = &fmt_pix->fmt.pix;
> +	struct v4l2_pix_format_mplane *pix_mp = &fmt_pix_mp->fmt.pix_mp;
> +
> +	fmt_pix_mp->type = fmt_pix->type;
> +	pix_mp->width = pix->width;
> +	pix_mp->height = pix->height;
> +	pix_mp->pixelformat = pix->pixelformat;
> +	pix_mp->field = pix->field;
> +	pix_mp->colorspace = pix->colorspace;
> +	pix_mp->plane_fmt[0].bytesperline = pix->bytesperline;
> +	pix_mp->plane_fmt[0].sizeimage = pix->sizeimage;
> +	pix_mp->num_planes = 1;
> +}
> +
> +static int s5p_jpeg_g_fmt_mplane(struct file *file, void *priv,
> +				 struct v4l2_format *f)
> +{
> +	struct v4l2_format tmp;
> +	int ret;
> +
> +	memset(&tmp, 0, sizeof(tmp));
> +	v4l2_format_pixmp_to_pix(f, &tmp);
> +	ret = s5p_jpeg_g_fmt(file, priv, &tmp);
> +	v4l2_format_pixmp_from_pix(f, &tmp);
> +
> +	return ret;
> +}
> +
> +static int s5p_jpeg_try_fmt_vid_cap_mplane(struct file *file, void *priv,
> +					   struct v4l2_format *f)
> +{
> +	struct v4l2_format tmp;
> +	int ret;
> +
> +	memset(&tmp, 0, sizeof(tmp));
> +	v4l2_format_pixmp_to_pix(f, &tmp);
> +	ret = s5p_jpeg_try_fmt_vid_cap(file, priv, &tmp);
> +	v4l2_format_pixmp_from_pix(f, &tmp);
> +
> +	return ret;
> +}
> +
> +static int s5p_jpeg_try_fmt_vid_out_mplane(struct file *file, void *priv,
> +					   struct v4l2_format *f)
> +{
> +	struct v4l2_format tmp;
> +	int ret;
> +
> +	memset(&tmp, 0, sizeof(tmp));
> +	v4l2_format_pixmp_to_pix(f, &tmp);
> +	ret = s5p_jpeg_try_fmt_vid_out(file, priv, &tmp);
> +	v4l2_format_pixmp_from_pix(f, &tmp);
> +
> +	return ret;
> +}
> +
> +static int s5p_jpeg_s_fmt_vid_cap_mplane(struct file *file, void *priv,
> +					 struct v4l2_format *f)
> +{
> +	struct v4l2_format tmp;
> +	int ret;
> +
> +	memset(&tmp, 0, sizeof(tmp));
> +	v4l2_format_pixmp_to_pix(f, &tmp);
> +	ret = s5p_jpeg_s_fmt_vid_cap(file, priv, &tmp);
> +	v4l2_format_pixmp_from_pix(f, &tmp);
> +
> +	return ret;
> +}
> +
> +static int s5p_jpeg_s_fmt_vid_out_mplane(struct file *file, void *priv,
> +					 struct v4l2_format *f)
> +{
> +	struct v4l2_format tmp;
> +	int ret;
> +
> +	memset(&tmp, 0, sizeof(tmp));
> +	v4l2_format_pixmp_to_pix(f, &tmp);
> +	ret = s5p_jpeg_s_fmt_vid_out(file, priv, &tmp);
> +	v4l2_format_pixmp_from_pix(f, &tmp);
> +
> +	return ret;
> +}
> +
>  static const struct v4l2_ioctl_ops s5p_jpeg_ioctl_ops = {
>  	.vidioc_querycap		= s5p_jpeg_querycap,
>  
> @@ -2133,6 +2244,18 @@ static const struct v4l2_ioctl_ops s5p_jpeg_ioctl_ops = {
>  	.vidioc_s_fmt_vid_cap		= s5p_jpeg_s_fmt_vid_cap,
>  	.vidioc_s_fmt_vid_out		= s5p_jpeg_s_fmt_vid_out,
>  
> +	.vidioc_enum_fmt_vid_cap_mplane	= s5p_jpeg_enum_fmt_vid_cap,
> +	.vidioc_enum_fmt_vid_out_mplane	= s5p_jpeg_enum_fmt_vid_out,
> +
> +	.vidioc_g_fmt_vid_cap_mplane	= s5p_jpeg_g_fmt_mplane,
> +	.vidioc_g_fmt_vid_out_mplane	= s5p_jpeg_g_fmt_mplane,
> +
> +	.vidioc_try_fmt_vid_cap_mplane	= s5p_jpeg_try_fmt_vid_cap_mplane,
> +	.vidioc_try_fmt_vid_out_mplane	= s5p_jpeg_try_fmt_vid_out_mplane,
> +
> +	.vidioc_s_fmt_vid_cap_mplane	= s5p_jpeg_s_fmt_vid_cap_mplane,
> +	.vidioc_s_fmt_vid_out_mplane	= s5p_jpeg_s_fmt_vid_out_mplane,
> +
>  	.vidioc_reqbufs			= v4l2_m2m_ioctl_reqbufs,
>  	.vidioc_querybuf		= v4l2_m2m_ioctl_querybuf,
>  	.vidioc_qbuf			= v4l2_m2m_ioctl_qbuf,
> @@ -2648,7 +2771,7 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
>  	struct s5p_jpeg_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
>  
>  	if (ctx->mode == S5P_JPEG_DECODE &&
> -	    vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> +	    vb->vb2_queue->type == ctx->out_q.type) {
>  		static const struct v4l2_event ev_src_ch = {
>  			.type = V4L2_EVENT_SOURCE_CHANGE,
>  			.u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
> @@ -2657,8 +2780,7 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
>  		u32 ori_w;
>  		u32 ori_h;
>  
> -		dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> -					 V4L2_BUF_TYPE_VIDEO_CAPTURE);
> +		dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, ctx->cap_q.type);
>  		ori_w = ctx->out_q.w;
>  		ori_h = ctx->out_q.h;
>  
> @@ -2708,7 +2830,7 @@ static void s5p_jpeg_stop_streaming(struct vb2_queue *q)
>  	 * subsampling. Update capture queue when the stream is off.
>  	 */
>  	if (ctx->state == JPEGCTX_RESOLUTION_CHANGE &&
> -	    q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> +	    !V4L2_TYPE_IS_OUTPUT(q->type)) {
>  		s5p_jpeg_set_capture_queue_data(ctx);
>  		ctx->state = JPEGCTX_RUNNING;
>  	}
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.h b/drivers/media/platform/s5p-jpeg/jpeg-core.h
> index 9aa26bd..302a297 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.h
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.h
> @@ -196,6 +196,7 @@ struct s5p_jpeg_marker {
>   * @sof_len:	SOF0 marker's payload length (without length field itself)
>   * @components:	number of image components
>   * @size:	image buffer size in bytes
> + * @type:	buffer type of the queue (enum v4l2_buf_type)
>   */
>  struct s5p_jpeg_q_data {
>  	struct s5p_jpeg_fmt	*fmt;
> @@ -208,6 +209,7 @@ struct s5p_jpeg_q_data {
>  	u32			sof_len;
>  	u32			components;
>  	u32			size;
> +	u32			type;
>  };
>  
>  /**
> 

-- 
Best regards,
Jacek Anaszewski