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

2017-06-20 Thread Thierry Escande 

Hi Andrzej,

On 20/06/2017 12:51, Andrzej Pietrasiewicz wrote:

Hi Thierry,

W dniu 19.06.2017 o 15:50, Thierry Escande pisze:

Hi Andrzej,

On 16/06/2017 17:38, Andrzej Pietrasiewicz wrote:

Hi Thierry,

Thank you for the patch.

Can you give a use case for resolution change event?

Unfortunately, the original commit does not mention any clear use case.
I've asked to the patch author for more information.


Can you please share what you learn about it if the author gets back to 
you?

Now that we don't know why to apply a patch I guess we should not do it.
This event is used in Chromium by the V4L2 jpeg decode accelerator to 
allocate output buffer. Please see:

https://cs.chromium.org/chromium/src/media/gpu/v4l2_jpeg_decode_accelerator.cc?rcl=91793c6ef94f05e93d258db8c7f3cad59819c6b8=585

I'll add a note in the commit message.






@@ -2510,43 +2567,18 @@ static void s5p_jpeg_buf_queue(struct
vb2_buffer *vb)
   return;
   }
-q_data = >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 = >cap_q;
-q_data->w = tmp.w;
-q_data->h = tmp.h;



Why is this part removed?

This has not been removed.
The  s5p_jpeg_q_data struct was passed to s5p_jpeg_parse_hdr() and
then copied field-by-field into ctx->out_q (through q_data pointer).
With this change ctx->out_q is passed to s5p_jpeg_parse_hdr() and this
avoids the copy.


It seems that changing field-by-field copying to passing a pointer
directly to s5p_jpeg_parse_hdr() is an unrelated change and as such
should be in a separate patch.

Will do.

Regards,
 Thierry

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

2017-06-20 Thread Andrzej Pietrasiewicz 

Hi Thierry,

W dniu 19.06.2017 o 15:50, Thierry Escande pisze:

Hi Andrzej,

On 16/06/2017 17:38, Andrzej Pietrasiewicz wrote:

Hi Thierry,

Thank you for the patch.

Can you give a use case for resolution change event?

Unfortunately, the original commit does not mention any clear use case.
I've asked to the patch author for more information.


Can you please share what you learn about it if the author gets back to you?
Now that we don't know why to apply a patch I guess we should not do it.








@@ -2510,43 +2567,18 @@ static void s5p_jpeg_buf_queue(struct
vb2_buffer *vb)
   return;
   }
-q_data = >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 = >cap_q;
-q_data->w = tmp.w;
-q_data->h = tmp.h;



Why is this part removed?

This has not been removed.
The  s5p_jpeg_q_data struct was passed to s5p_jpeg_parse_hdr() and
then copied field-by-field into ctx->out_q (through q_data pointer).
With this change ctx->out_q is passed to s5p_jpeg_parse_hdr() and this
avoids the copy.


It seems that changing field-by-field copying to passing a pointer
directly to s5p_jpeg_parse_hdr() is an unrelated change and as such
should be in a separate patch.

Andrzej

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

2017-06-19 Thread Thierry Escande 

Hi Andrzej,

On 16/06/2017 17:38, Andrzej Pietrasiewicz wrote:

Hi Thierry,

Thank you for the patch.

Can you give a use case for resolution change event?
Unfortunately, the original commit does not mention any clear use case. 
I've asked to the patch author for more information.



Also plase see inline.

W dniu 12.06.2017 o 19:13, Thierry Escande pisze:

From: henryhsu 
@@ -1611,8 +1612,6 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx 
*ct, struct v4l2_format *f)

  FMT_TYPE_OUTPUT : FMT_TYPE_CAPTURE;
  q_data->fmt = s5p_jpeg_find_format(ct, pix->pixelformat, f_type);
-q_data->w = pix->width;
-q_data->h = pix->height;
  if (q_data->fmt->fourcc != V4L2_PIX_FMT_JPEG) {
  /*
   * During encoding Exynos4x12 SoCs access wider memory area
@@ -1620,6 +1619,8 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx 
*ct, struct v4l2_format *f)

   * the JPEG_IMAGE_SIZE register. In order to avoid sysmmu
   * page fault calculate proper buffer size in such a case.
   */
+q_data->w = pix->width;
+q_data->h = pix->height;


Is this change related to what the patch is supposed to be doing?

Yes actually. From the author comments to the same question:
"We want to send a resolution change in the first frame. Without this, 
q_data->w and h will be updated by s_fmt. Then we won't know the 
resolution is changed from (0, 0) to (w, h) in qbuf function."



  static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
  {
  struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
@@ -2499,9 +2545,20 @@ static void s5p_jpeg_buf_queue(struct 
vb2_buffer *vb)

  if (ctx->mode == S5P_JPEG_DECODE &&
  vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
-struct s5p_jpeg_q_data tmp, *q_data;
-
-ctx->hdr_parsed = s5p_jpeg_parse_hdr(,
+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(>out_q,
   (unsigned long)vb2_plane_vaddr(vb, 0),
   min((unsigned long)ctx->out_q.size,
   vb2_get_plane_payload(vb, 0)), ctx);
@@ -2510,43 +2567,18 @@ static void s5p_jpeg_buf_queue(struct 
vb2_buffer *vb)

  return;
  }
-q_data = >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 = >cap_q;
-q_data->w = tmp.w;
-q_data->h = tmp.h;



Why is this part removed?

This has not been removed.
The  s5p_jpeg_q_data struct was passed to s5p_jpeg_parse_hdr() and 
then copied field-by-field into ctx->out_q (through q_data pointer).
With this change ctx->out_q is passed to s5p_jpeg_parse_hdr() and this 
avoids the copy.
Then ctx->cap_q width & height copy is done in 
s5p_jpeg_set_capture_queue_data().


Regards,
 Thierry

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

2017-06-16 Thread Andrzej Pietrasiewicz 

Hi Thierry,

Thank you for the patch.

Can you give a use case for resolution change event?

Also plase see inline.

W dniu 12.06.2017 o 19:13, Thierry Escande pisze:

From: henryhsu 

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 
Signed-off-by: Thierry Escande 
---
  drivers/media/platform/s5p-jpeg/jpeg-core.c | 125 +++-
  drivers/media/platform/s5p-jpeg/jpeg-core.h |   7 ++
  2 files changed, 91 insertions(+), 41 deletions(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c 
b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 7ef7173..3d90a63 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -24,6 +24,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -1611,8 +1612,6 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct 
v4l2_format *f)
FMT_TYPE_OUTPUT : FMT_TYPE_CAPTURE;
  
  	q_data->fmt = s5p_jpeg_find_format(ct, pix->pixelformat, f_type);

-   q_data->w = pix->width;
-   q_data->h = pix->height;
if (q_data->fmt->fourcc != V4L2_PIX_FMT_JPEG) {
/*
 * During encoding Exynos4x12 SoCs access wider memory area
@@ -1620,6 +1619,8 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct 
v4l2_format *f)
 * the JPEG_IMAGE_SIZE register. In order to avoid sysmmu
 * page fault calculate proper buffer size in such a case.
 */
+   q_data->w = pix->width;
+   q_data->h = pix->height;


Is this change related to what the patch is supposed to be doing?


if (ct->jpeg->variant->hw_ex4_compat &&
f_type == FMT_TYPE_OUTPUT && ct->mode == S5P_JPEG_ENCODE)
q_data->size = exynos4_jpeg_get_output_buffer_size(ct,
@@ -1695,6 +1696,15 @@ static int s5p_jpeg_s_fmt_vid_out(struct file *file, 
void *priv,
return s5p_jpeg_s_fmt(fh_to_ctx(priv), f);
  }
  
+static int s5p_jpeg_subscribe_event(struct v4l2_fh *fh,

+   const struct v4l2_event_subscription *sub)
+{
+   if (sub->type == V4L2_EVENT_SOURCE_CHANGE)
+   return v4l2_src_change_event_subscribe(fh, sub);
+
+   return -EINVAL;
+}
+
  static int exynos3250_jpeg_try_downscale(struct s5p_jpeg_ctx *ctx,
   struct v4l2_rect *r)
  {
@@ -2020,6 +2030,9 @@ static const struct v4l2_ioctl_ops s5p_jpeg_ioctl_ops = {
  
  	.vidioc_g_selection		= s5p_jpeg_g_selection,

.vidioc_s_selection = s5p_jpeg_s_selection,
+
+   .vidioc_subscribe_event = s5p_jpeg_subscribe_event,
+   .vidioc_unsubscribe_event   = v4l2_event_unsubscribe,
  };
  
  /*

@@ -2412,8 +2425,17 @@ static int s5p_jpeg_job_ready(void *priv)
  {
struct s5p_jpeg_ctx *ctx = priv;
  
-	if (ctx->mode == S5P_JPEG_DECODE)

+   if (ctx->mode == S5P_JPEG_DECODE) {
+   /*
+* We have only one input buffer and one output buffer. If there
+* is a resolution change event, no need to continue decoding.
+*/
+   if (ctx->state == JPEGCTX_RESOLUTION_CHANGE)
+   return 0;
+
return ctx->hdr_parsed;
+   }
+
return 1;
  }
  
@@ -2492,6 +2514,30 @@ static int s5p_jpeg_buf_prepare(struct vb2_buffer *vb)

return 0;
  }
  
+static void s5p_jpeg_set_capture_queue_data(struct s5p_jpeg_ctx *ctx)

+{
+   struct s5p_jpeg_q_data *q_data = >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, _data->w, S5P_JPEG_MIN_WIDTH,
+  S5P_JPEG_MAX_WIDTH, q_data->fmt->h_align,
+  _data->h, S5P_JPEG_MIN_HEIGHT,
+  S5P_JPEG_MAX_HEIGHT, q_data->fmt->v_align);
+
+   q_data->size = q_data->w * q_data->h * q_data->fmt->depth >> 3;
+}
+
  static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
  {
struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
@@ -2499,9 +2545,20 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
  
  	if (ctx->mode == S5P_JPEG_DECODE

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

2017-06-12 Thread Thierry Escande 
From: henryhsu 

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 
Signed-off-by: Thierry Escande 
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 125 +++-
 drivers/media/platform/s5p-jpeg/jpeg-core.h |   7 ++
 2 files changed, 91 insertions(+), 41 deletions(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c 
b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 7ef7173..3d90a63 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1611,8 +1612,6 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct 
v4l2_format *f)
FMT_TYPE_OUTPUT : FMT_TYPE_CAPTURE;
 
q_data->fmt = s5p_jpeg_find_format(ct, pix->pixelformat, f_type);
-   q_data->w = pix->width;
-   q_data->h = pix->height;
if (q_data->fmt->fourcc != V4L2_PIX_FMT_JPEG) {
/*
 * During encoding Exynos4x12 SoCs access wider memory area
@@ -1620,6 +1619,8 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct 
v4l2_format *f)
 * the JPEG_IMAGE_SIZE register. In order to avoid sysmmu
 * page fault calculate proper buffer size in such a case.
 */
+   q_data->w = pix->width;
+   q_data->h = pix->height;
if (ct->jpeg->variant->hw_ex4_compat &&
f_type == FMT_TYPE_OUTPUT && ct->mode == S5P_JPEG_ENCODE)
q_data->size = exynos4_jpeg_get_output_buffer_size(ct,
@@ -1695,6 +1696,15 @@ static int s5p_jpeg_s_fmt_vid_out(struct file *file, 
void *priv,
return s5p_jpeg_s_fmt(fh_to_ctx(priv), f);
 }
 
+static int s5p_jpeg_subscribe_event(struct v4l2_fh *fh,
+   const struct v4l2_event_subscription *sub)
+{
+   if (sub->type == V4L2_EVENT_SOURCE_CHANGE)
+   return v4l2_src_change_event_subscribe(fh, sub);
+
+   return -EINVAL;
+}
+
 static int exynos3250_jpeg_try_downscale(struct s5p_jpeg_ctx *ctx,
   struct v4l2_rect *r)
 {
@@ -2020,6 +2030,9 @@ static const struct v4l2_ioctl_ops s5p_jpeg_ioctl_ops = {
 
.vidioc_g_selection = s5p_jpeg_g_selection,
.vidioc_s_selection = s5p_jpeg_s_selection,
+
+   .vidioc_subscribe_event = s5p_jpeg_subscribe_event,
+   .vidioc_unsubscribe_event   = v4l2_event_unsubscribe,
 };
 
 /*
@@ -2412,8 +2425,17 @@ static int s5p_jpeg_job_ready(void *priv)
 {
struct s5p_jpeg_ctx *ctx = priv;
 
-   if (ctx->mode == S5P_JPEG_DECODE)
+   if (ctx->mode == S5P_JPEG_DECODE) {
+   /*
+* We have only one input buffer and one output buffer. If there
+* is a resolution change event, no need to continue decoding.
+*/
+   if (ctx->state == JPEGCTX_RESOLUTION_CHANGE)
+   return 0;
+
return ctx->hdr_parsed;
+   }
+
return 1;
 }
 
@@ -2492,6 +2514,30 @@ static int s5p_jpeg_buf_prepare(struct vb2_buffer *vb)
return 0;
 }
 
+static void s5p_jpeg_set_capture_queue_data(struct s5p_jpeg_ctx *ctx)
+{
+   struct s5p_jpeg_q_data *q_data = >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, _data->w, S5P_JPEG_MIN_WIDTH,
+  S5P_JPEG_MAX_WIDTH, q_data->fmt->h_align,
+  _data->h, S5P_JPEG_MIN_HEIGHT,
+  S5P_JPEG_MAX_HEIGHT, q_data->fmt->v_align);
+
+   q_data->size = q_data->w * q_data->h * q_data->fmt->depth >> 3;
+}
+
 static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
 {
struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
@@ -2499,9 +2545,20 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
 
if (ctx->mode == S5P_JPEG_DECODE &&
vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
-   struct s5p_jpeg_q_data tmp, *q_data;
-
-   ctx->hdr_parsed = s5p_jpeg_parse_hdr(,
+   static const struct v4l2_event ev_src_ch = {
+