[PATCH v6 0/2] Fixes for colorspace logic in exynos-gsc and s5p-mfc drivers

[PATCH v6 0/2] Fixes for colorspace logic in exynos-gsc and s5p-mfc drivers

[Thibault Saunier]

Hello,

This patchset fixes a few issues on the colorspace logic for the exynos-gsc
and s5p-mfc drivers.

We now handle the colorspace in those drivers, and make sure to respect user setting if
possible.

We also now set the 'v4l2_pix_format:field' if userspace passed ANY, and
replicate users value on the capture side.

This is the sixth version of the patch serie.

Best regards,

Thibault Saunier

Changes in v6:
- Do not ever guess colorspace
- Pass user output field value to the capture as the device is not
  doing any deinterlacing and thus decoded content will still be
  interlaced on the output.

Changes in v5:
- Squash commit to always use output colorspace on the capture side
  inside this one
- Fix typo in commit message
- Just adapt the field and never error out.

Changes in v4:
- Reword commit message to better back our assumptions on specifications

Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review
- Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'
- Do not check values in the g_fmt functions as Andrzej explained in previous review

Changes in v2:
- Fix a silly build error that slipped in while rebasing the patches

Thibault Saunier (2):
  [media] exynos-gsc: Use user configured colorspace if provided
  [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

 drivers/media/platform/exynos-gsc/gsc-core.c    | 9 ++++-----
 drivers/media/platform/exynos-gsc/gsc-core.h    | 1 +
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
 4 files changed, 12 insertions(+), 6 deletions(-)

-- 
2.11.1

[PATCH v6 0/2] Fixes for colorspace logic in exynos-gsc and s5p-mfc drivers

[Thibault Saunier]

Hello,

This patchset fixes a few issues on the colorspace logic for the exynos-gsc
and s5p-mfc drivers.

We now handle the colorspace in those drivers, and make sure to respect user setting if
possible.

We also now set the 'v4l2_pix_format:field' if userspace passed ANY, and
replicate users value on the capture side.

This is the sixth version of the patch serie.

Best regards,

Thibault Saunier

Changes in v6:
- Do not ever guess colorspace
- Pass user output field value to the capture as the device is not
  doing any deinterlacing and thus decoded content will still be
  interlaced on the output.

Changes in v5:
- Squash commit to always use output colorspace on the capture side
  inside this one
- Fix typo in commit message
- Just adapt the field and never error out.

Changes in v4:
- Reword commit message to better back our assumptions on specifications

Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review
- Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'
- Do not check values in the g_fmt functions as Andrzej explained in previous review

Changes in v2:
- Fix a silly build error that slipped in while rebasing the patches

Thibault Saunier (2):
  [media] exynos-gsc: Use user configured colorspace if provided
  [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

 drivers/media/platform/exynos-gsc/gsc-core.c    | 9 ++++-----
 drivers/media/platform/exynos-gsc/gsc-core.h    | 1 +
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
 4 files changed, 12 insertions(+), 6 deletions(-)

-- 
2.11.1

[PATCH v6 0/2] Fixes for colorspace logic in exynos-gsc and s5p-mfc drivers

[Thibault Saunier]

Hello,

This patchset fixes a few issues on the colorspace logic for the exynos-gsc
and s5p-mfc drivers.

We now handle the colorspace in those drivers, and make sure to respect user setting if
possible.

We also now set the 'v4l2_pix_format:field' if userspace passed ANY, and
replicate users value on the capture side.

This is the sixth version of the patch serie.

Best regards,

Thibault Saunier

Changes in v6:
- Do not ever guess colorspace
- Pass user output field value to the capture as the device is not
  doing any deinterlacing and thus decoded content will still be
  interlaced on the output.

Changes in v5:
- Squash commit to always use output colorspace on the capture side
  inside this one
- Fix typo in commit message
- Just adapt the field and never error out.

Changes in v4:
- Reword commit message to better back our assumptions on specifications

Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review
- Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'
- Do not check values in the g_fmt functions as Andrzej explained in previous review

Changes in v2:
- Fix a silly build error that slipped in while rebasing the patches

Thibault Saunier (2):
  [media] exynos-gsc: Use user configured colorspace if provided
  [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

 drivers/media/platform/exynos-gsc/gsc-core.c    | 9 ++++-----
 drivers/media/platform/exynos-gsc/gsc-core.h    | 1 +
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
 4 files changed, 12 insertions(+), 6 deletions(-)

-- 
2.11.1

[PATCH v6 1/2] [media] exynos-gsc: Use user configured colorspace if provided

[Thibault Saunier]

Use colorspace provided by the user as we are only doing scaling and
color encoding conversion, we won't be able to transform the colorspace
itself and the colorspace won't mater in that operation.

Also always use output colorspace on the capture side.

If the user does not provide a colorspace do no make it up, we might
later while processing need to figure out the colorspace, which
is possible depending on the frame size but do not ever guess and
leak that guess to the userspace.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

---

Changes in v6:
- Do not ever guess colorspace

Changes in v5:
- Squash commit to always use output colorspace on the capture side
  inside this one
- Fix typo in commit message

Changes in v4:
- Reword commit message to better back our assumptions on specifications

Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review
- Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'

Changes in v2: None

 drivers/media/platform/exynos-gsc/gsc-core.c | 9 ++++-----
 drivers/media/platform/exynos-gsc/gsc-core.h | 1 +
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 59a634201830..0241168c85af 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -454,6 +454,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 	} else {
 		min_w = variant->pix_min->target_rot_dis_w;
 		min_h = variant->pix_min->target_rot_dis_h;
+		pix_mp->colorspace = ctx->out_colorspace;
 	}
 
 	pr_debug("mod_x: %d, mod_y: %d, max_w: %d, max_h = %d",
@@ -472,10 +473,8 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 
 	pix_mp->num_planes = fmt->num_planes;
 
-	if (pix_mp->width >= 1280) /* HD */
-		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
-	else /* SD */
-		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
+	if (V4L2_TYPE_IS_OUTPUT(f->type))
+		ctx->out_colorspace = pix_mp->colorspace;
 
 	for (i = 0; i < pix_mp->num_planes; ++i) {
 		struct v4l2_plane_pix_format *plane_fmt = &pix_mp->plane_fmt[i];
@@ -519,8 +518,8 @@ int gsc_g_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 	pix_mp->height		= frame->f_height;
 	pix_mp->field		= V4L2_FIELD_NONE;
 	pix_mp->pixelformat	= frame->fmt->pixelformat;
-	pix_mp->colorspace	= V4L2_COLORSPACE_REC709;
 	pix_mp->num_planes	= frame->fmt->num_planes;
+	pix_mp->colorspace = ctx->out_colorspace;
 
 	for (i = 0; i < pix_mp->num_planes; ++i) {
 		pix_mp->plane_fmt[i].bytesperline = (frame->f_width *
diff --git a/drivers/media/platform/exynos-gsc/gsc-core.h b/drivers/media/platform/exynos-gsc/gsc-core.h
index 696217e9af66..715d9c9d8d30 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.h
+++ b/drivers/media/platform/exynos-gsc/gsc-core.h
@@ -376,6 +376,7 @@ struct gsc_ctx {
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct gsc_ctrls	gsc_ctrls;
 	bool			ctrls_rdy;
+	enum v4l2_colorspace out_colorspace;
 };
 
 void gsc_set_prefbuf(struct gsc_dev *gsc, struct gsc_frame *frm);
-- 
2.11.1

[PATCH v6 1/2] [media] exynos-gsc: Use user configured colorspace if provided

[Thibault Saunier]

Use colorspace provided by the user as we are only doing scaling and
color encoding conversion, we won't be able to transform the colorspace
itself and the colorspace won't mater in that operation.

Also always use output colorspace on the capture side.

If the user does not provide a colorspace do no make it up, we might
later while processing need to figure out the colorspace, which
is possible depending on the frame size but do not ever guess and
leak that guess to the userspace.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

---

Changes in v6:
- Do not ever guess colorspace

Changes in v5:
- Squash commit to always use output colorspace on the capture side
  inside this one
- Fix typo in commit message

Changes in v4:
- Reword commit message to better back our assumptions on specifications

Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review
- Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'

Changes in v2: None

 drivers/media/platform/exynos-gsc/gsc-core.c | 9 ++++-----
 drivers/media/platform/exynos-gsc/gsc-core.h | 1 +
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 59a634201830..0241168c85af 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -454,6 +454,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 	} else {
 		min_w = variant->pix_min->target_rot_dis_w;
 		min_h = variant->pix_min->target_rot_dis_h;
+		pix_mp->colorspace = ctx->out_colorspace;
 	}
 
 	pr_debug("mod_x: %d, mod_y: %d, max_w: %d, max_h = %d",
@@ -472,10 +473,8 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 
 	pix_mp->num_planes = fmt->num_planes;
 
-	if (pix_mp->width >= 1280) /* HD */
-		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
-	else /* SD */
-		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
+	if (V4L2_TYPE_IS_OUTPUT(f->type))
+		ctx->out_colorspace = pix_mp->colorspace;
 
 	for (i = 0; i < pix_mp->num_planes; ++i) {
 		struct v4l2_plane_pix_format *plane_fmt = &pix_mp->plane_fmt[i];
@@ -519,8 +518,8 @@ int gsc_g_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 	pix_mp->height		= frame->f_height;
 	pix_mp->field		= V4L2_FIELD_NONE;
 	pix_mp->pixelformat	= frame->fmt->pixelformat;
-	pix_mp->colorspace	= V4L2_COLORSPACE_REC709;
 	pix_mp->num_planes	= frame->fmt->num_planes;
+	pix_mp->colorspace = ctx->out_colorspace;
 
 	for (i = 0; i < pix_mp->num_planes; ++i) {
 		pix_mp->plane_fmt[i].bytesperline = (frame->f_width *
diff --git a/drivers/media/platform/exynos-gsc/gsc-core.h b/drivers/media/platform/exynos-gsc/gsc-core.h
index 696217e9af66..715d9c9d8d30 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.h
+++ b/drivers/media/platform/exynos-gsc/gsc-core.h
@@ -376,6 +376,7 @@ struct gsc_ctx {
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct gsc_ctrls	gsc_ctrls;
 	bool			ctrls_rdy;
+	enum v4l2_colorspace out_colorspace;
 };
 
 void gsc_set_prefbuf(struct gsc_dev *gsc, struct gsc_frame *frm);
-- 
2.11.1

[PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Thibault Saunier]

It is required by the standard that the field order is set by the
driver, default to NONE in case any is provided, but we can basically
accept any value provided by the userspace as we will anyway not
be able to do any deinterlacing.

In this patch we also make sure to pass the interlacing mode provided
by userspace from the output to the capture side of the device so
that the information is given back to userspace. This way it can
handle it and potentially deinterlace afterward.

Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>

---

Changes in v6:
- Pass user output field value to the capture as the device is not
  doing any deinterlacing and thus decoded content will still be
  interlaced on the output.

Changes in v5:
- Just adapt the field and never error out.

Changes in v4: None
Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review

Changes in v2:
- Fix a silly build error that slipped in while rebasing the patches

 drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index ab23236aa942..3816a37de4bc 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
 	size_t me_buffer_size;
 	size_t tmv_buffer_size;
 
+	enum v4l2_field field;
+
 	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
 
 	struct list_head ref_queue;
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 367ef8e8dbf0..6e5ca86fb331 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 		   rectangle. */
 		pix_mp->width = ctx->buf_width;
 		pix_mp->height = ctx->buf_height;
-		pix_mp->field = V4L2_FIELD_NONE;
+		pix_mp->field = ctx->field;
 		pix_mp->num_planes = 2;
 		/* Set pixelformat to the format in which MFC
 		   outputs the decoded frame */
@@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 	struct s5p_mfc_dev *dev = video_drvdata(file);
 	struct s5p_mfc_fmt *fmt;
 
+	if (f->fmt.pix.field == V4L2_FIELD_ANY)
+		f->fmt.pix.field = V4L2_FIELD_NONE;
+
 	mfc_debug(2, "Type is %d\n", f->type);
 	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		fmt = find_format(f, MFC_FMT_DEC);
@@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
 		goto out;
 	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		/* src_fmt is validated by call to vidioc_try_fmt */
+		ctx->field = f->fmt.pix.field;
 		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
 		ctx->codec_mode = ctx->src_fmt->codec_mode;
 		mfc_debug(2, "The codec number is: %d\n", ctx->codec_mode);
-- 
2.11.1

[PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Thibault Saunier]

It is required by the standard that the field order is set by the
driver, default to NONE in case any is provided, but we can basically
accept any value provided by the userspace as we will anyway not
be able to do any deinterlacing.

In this patch we also make sure to pass the interlacing mode provided
by userspace from the output to the capture side of the device so
that the information is given back to userspace. This way it can
handle it and potentially deinterlace afterward.

Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>

---

Changes in v6:
- Pass user output field value to the capture as the device is not
  doing any deinterlacing and thus decoded content will still be
  interlaced on the output.

Changes in v5:
- Just adapt the field and never error out.

Changes in v4: None
Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review

Changes in v2:
- Fix a silly build error that slipped in while rebasing the patches

 drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index ab23236aa942..3816a37de4bc 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
 	size_t me_buffer_size;
 	size_t tmv_buffer_size;
 
+	enum v4l2_field field;
+
 	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
 
 	struct list_head ref_queue;
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 367ef8e8dbf0..6e5ca86fb331 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 		   rectangle. */
 		pix_mp->width = ctx->buf_width;
 		pix_mp->height = ctx->buf_height;
-		pix_mp->field = V4L2_FIELD_NONE;
+		pix_mp->field = ctx->field;
 		pix_mp->num_planes = 2;
 		/* Set pixelformat to the format in which MFC
 		   outputs the decoded frame */
@@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 	struct s5p_mfc_dev *dev = video_drvdata(file);
 	struct s5p_mfc_fmt *fmt;
 
+	if (f->fmt.pix.field == V4L2_FIELD_ANY)
+		f->fmt.pix.field = V4L2_FIELD_NONE;
+
 	mfc_debug(2, "Type is %d\n", f->type);
 	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		fmt = find_format(f, MFC_FMT_DEC);
@@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
 		goto out;
 	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		/* src_fmt is validated by call to vidioc_try_fmt */
+		ctx->field = f->fmt.pix.field;
 		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
 		ctx->codec_mode = ctx->src_fmt->codec_mode;
 		mfc_debug(2, "The codec number is: %d\n", ctx->codec_mode);
-- 
2.11.1

[PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Thibault Saunier]

It is required by the standard that the field order is set by the
driver, default to NONE in case any is provided, but we can basically
accept any value provided by the userspace as we will anyway not
be able to do any deinterlacing.

In this patch we also make sure to pass the interlacing mode provided
by userspace from the output to the capture side of the device so
that the information is given back to userspace. This way it can
handle it and potentially deinterlace afterward.

Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>

---

Changes in v6:
- Pass user output field value to the capture as the device is not
  doing any deinterlacing and thus decoded content will still be
  interlaced on the output.

Changes in v5:
- Just adapt the field and never error out.

Changes in v4: None
Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review

Changes in v2:
- Fix a silly build error that slipped in while rebasing the patches

 drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index ab23236aa942..3816a37de4bc 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
 	size_t me_buffer_size;
 	size_t tmv_buffer_size;
 
+	enum v4l2_field field;
+
 	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
 
 	struct list_head ref_queue;
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 367ef8e8dbf0..6e5ca86fb331 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 		   rectangle. */
 		pix_mp->width = ctx->buf_width;
 		pix_mp->height = ctx->buf_height;
-		pix_mp->field = V4L2_FIELD_NONE;
+		pix_mp->field = ctx->field;
 		pix_mp->num_planes = 2;
 		/* Set pixelformat to the format in which MFC
 		   outputs the decoded frame */
@@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 	struct s5p_mfc_dev *dev = video_drvdata(file);
 	struct s5p_mfc_fmt *fmt;
 
+	if (f->fmt.pix.field == V4L2_FIELD_ANY)
+		f->fmt.pix.field = V4L2_FIELD_NONE;
+
 	mfc_debug(2, "Type is %d\n", f->type);
 	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		fmt = find_format(f, MFC_FMT_DEC);
@@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
 		goto out;
 	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		/* src_fmt is validated by call to vidioc_try_fmt */
+		ctx->field = f->fmt.pix.field;
 		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
 		ctx->codec_mode = ctx->src_fmt->codec_mode;
 		mfc_debug(2, "The codec number is: %d\n", ctx->codec_mode);
-- 
2.11.1

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Andrzej Hajda]

On 01.03.2017 12:51, Thibault Saunier wrote:
> It is required by the standard that the field order is set by the
> driver, default to NONE in case any is provided, but we can basically
> accept any value provided by the userspace as we will anyway not
> be able to do any deinterlacing.
>
> In this patch we also make sure to pass the interlacing mode provided
> by userspace from the output to the capture side of the device so
> that the information is given back to userspace. This way it can
> handle it and potentially deinterlace afterward.

As I wrote previously:
- on output side you have encoded bytestream - you cannot say about
interlacing in such case, so the only valid value is NONE,
- on capture side you have decoded frames, and in this case it depends
on the device and driver capabilities, if the driver/device does not
support (de-)interlacing (I suppose this is MFC case), interlace type
field should be filled according to decoded bytestream header (on output
side), but no direct copying from output side!!!

Regards
Andrzej

>
> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>
> ---
>
> Changes in v6:
> - Pass user output field value to the capture as the device is not
>   doing any deinterlacing and thus decoded content will still be
>   interlaced on the output.
>
> Changes in v5:
> - Just adapt the field and never error out.
>
> Changes in v4: None
> Changes in v3:
> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>
> Changes in v2:
> - Fix a silly build error that slipped in while rebasing the patches
>
>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> index ab23236aa942..3816a37de4bc 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
>  	size_t me_buffer_size;
>  	size_t tmv_buffer_size;
>  
> +	enum v4l2_field field;
> +
>  	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
>  
>  	struct list_head ref_queue;
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index 367ef8e8dbf0..6e5ca86fb331 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		   rectangle. */
>  		pix_mp->width = ctx->buf_width;
>  		pix_mp->height = ctx->buf_height;
> -		pix_mp->field = V4L2_FIELD_NONE;
> +		pix_mp->field = ctx->field;
>  		pix_mp->num_planes = 2;
>  		/* Set pixelformat to the format in which MFC
>  		   outputs the decoded frame */
> @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	struct s5p_mfc_dev *dev = video_drvdata(file);
>  	struct s5p_mfc_fmt *fmt;
>  
> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
> +		f->fmt.pix.field = V4L2_FIELD_NONE;
> +
>  	mfc_debug(2, "Type is %d\n", f->type);
>  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		fmt = find_format(f, MFC_FMT_DEC);
> @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		goto out;
>  	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		/* src_fmt is validated by call to vidioc_try_fmt */
> +		ctx->field = f->fmt.pix.field;
>  		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
>  		ctx->codec_mode = ctx->src_fmt->codec_mode;
>  		mfc_debug(2, "The codec number is: %d\n", ctx->codec_mode);

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Andrzej Hajda]

On 01.03.2017 12:51, Thibault Saunier wrote:
> It is required by the standard that the field order is set by the
> driver, default to NONE in case any is provided, but we can basically
> accept any value provided by the userspace as we will anyway not
> be able to do any deinterlacing.
>
> In this patch we also make sure to pass the interlacing mode provided
> by userspace from the output to the capture side of the device so
> that the information is given back to userspace. This way it can
> handle it and potentially deinterlace afterward.

As I wrote previously:
- on output side you have encoded bytestream - you cannot say about
interlacing in such case, so the only valid value is NONE,
- on capture side you have decoded frames, and in this case it depends
on the device and driver capabilities, if the driver/device does not
support (de-)interlacing (I suppose this is MFC case), interlace type
field should be filled according to decoded bytestream header (on output
side), but no direct copying from output side!!!

Regards
Andrzej

>
> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>
> ---
>
> Changes in v6:
> - Pass user output field value to the capture as the device is not
>   doing any deinterlacing and thus decoded content will still be
>   interlaced on the output.
>
> Changes in v5:
> - Just adapt the field and never error out.
>
> Changes in v4: None
> Changes in v3:
> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>
> Changes in v2:
> - Fix a silly build error that slipped in while rebasing the patches
>
>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> index ab23236aa942..3816a37de4bc 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
>  	size_t me_buffer_size;
>  	size_t tmv_buffer_size;
>  
> +	enum v4l2_field field;
> +
>  	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
>  
>  	struct list_head ref_queue;
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index 367ef8e8dbf0..6e5ca86fb331 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		   rectangle. */
>  		pix_mp->width = ctx->buf_width;
>  		pix_mp->height = ctx->buf_height;
> -		pix_mp->field = V4L2_FIELD_NONE;
> +		pix_mp->field = ctx->field;
>  		pix_mp->num_planes = 2;
>  		/* Set pixelformat to the format in which MFC
>  		   outputs the decoded frame */
> @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	struct s5p_mfc_dev *dev = video_drvdata(file);
>  	struct s5p_mfc_fmt *fmt;
>  
> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
> +		f->fmt.pix.field = V4L2_FIELD_NONE;
> +
>  	mfc_debug(2, "Type is %d\n", f->type);
>  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		fmt = find_format(f, MFC_FMT_DEC);
> @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		goto out;
>  	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		/* src_fmt is validated by call to vidioc_try_fmt */
> +		ctx->field = f->fmt.pix.field;
>  		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
>  		ctx->codec_mode = ctx->src_fmt->codec_mode;
>  		mfc_debug(2, "The codec number is: %d\n", ctx->codec_mode);

[PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Andrzej Hajda]

On 01.03.2017 12:51, Thibault Saunier wrote:
> It is required by the standard that the field order is set by the
> driver, default to NONE in case any is provided, but we can basically
> accept any value provided by the userspace as we will anyway not
> be able to do any deinterlacing.
>
> In this patch we also make sure to pass the interlacing mode provided
> by userspace from the output to the capture side of the device so
> that the information is given back to userspace. This way it can
> handle it and potentially deinterlace afterward.

As I wrote previously:
- on output side you have encoded bytestream - you cannot say about
interlacing in such case, so the only valid value is NONE,
- on capture side you have decoded frames, and in this case it depends
on the device and driver capabilities, if the driver/device does not
support (de-)interlacing (I suppose this is MFC case), interlace type
field should be filled according to decoded bytestream header (on output
side), but no direct copying from output side!!!

Regards
Andrzej

>
> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>
> ---
>
> Changes in v6:
> - Pass user output field value to the capture as the device is not
>   doing any deinterlacing and thus decoded content will still be
>   interlaced on the output.
>
> Changes in v5:
> - Just adapt the field and never error out.
>
> Changes in v4: None
> Changes in v3:
> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>
> Changes in v2:
> - Fix a silly build error that slipped in while rebasing the patches
>
>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> index ab23236aa942..3816a37de4bc 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
>  	size_t me_buffer_size;
>  	size_t tmv_buffer_size;
>  
> +	enum v4l2_field field;
> +
>  	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
>  
>  	struct list_head ref_queue;
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index 367ef8e8dbf0..6e5ca86fb331 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		   rectangle. */
>  		pix_mp->width = ctx->buf_width;
>  		pix_mp->height = ctx->buf_height;
> -		pix_mp->field = V4L2_FIELD_NONE;
> +		pix_mp->field = ctx->field;
>  		pix_mp->num_planes = 2;
>  		/* Set pixelformat to the format in which MFC
>  		   outputs the decoded frame */
> @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	struct s5p_mfc_dev *dev = video_drvdata(file);
>  	struct s5p_mfc_fmt *fmt;
>  
> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
> +		f->fmt.pix.field = V4L2_FIELD_NONE;
> +
>  	mfc_debug(2, "Type is %d\n", f->type);
>  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		fmt = find_format(f, MFC_FMT_DEC);
> @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		goto out;
>  	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		/* src_fmt is validated by call to vidioc_try_fmt */
> +		ctx->field = f->fmt.pix.field;
>  		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
>  		ctx->codec_mode = ctx->src_fmt->codec_mode;
>  		mfc_debug(2, "The codec number is: %d\n", ctx->codec_mode);

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Thibault Saunier]

Hello,


On 03/01/2017 10:12 AM, Andrzej Hajda wrote:
> On 01.03.2017 12:51, Thibault Saunier wrote:
>> It is required by the standard that the field order is set by the
>> driver, default to NONE in case any is provided, but we can basically
>> accept any value provided by the userspace as we will anyway not
>> be able to do any deinterlacing.
>>
>> In this patch we also make sure to pass the interlacing mode provided
>> by userspace from the output to the capture side of the device so
>> that the information is given back to userspace. This way it can
>> handle it and potentially deinterlace afterward.
> As I wrote previously:
> - on output side you have encoded bytestream - you cannot say about
> interlacing in such case, so the only valid value is NONE,
Well, the encoded stream contains the info about interlacing and
the most logical thing to do from my point of view is to keep that info
passe it along to the capture side, which is what I am doing.
What makes you think this is not the right way of handling that?
> - on capture side you have decoded frames, and in this case it depends
> on the device and driver capabilities, if the driver/device does not
> support (de-)interlacing (I suppose this is MFC case), interlace type
> field should be filled according to decoded bytestream header (on output
> side), but no direct copying from output side!!!
Well, why? If the userspace has already parsed the headers and passed the
info to the decoder, there is no reason we should pass along that info 
to the
capture side.

Currently the bitstream parser in the decoder does not seem to take of
the interlacing properly so if userspace specified it because it already 
parsed the
stream or the info was in the container not in the codec bitstream, then 
using
it is very sensible I think!!!

Regards,

Thibault
>
> Regards
> Andrzej
>
>> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>>
>> ---
>>
>> Changes in v6:
>> - Pass user output field value to the capture as the device is not
>>    doing any deinterlacing and thus decoded content will still be
>>    interlaced on the output.
>>
>> Changes in v5:
>> - Just adapt the field and never error out.
>>
>> Changes in v4: None
>> Changes in v3:
>> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>>
>> Changes in v2:
>> - Fix a silly build error that slipped in while rebasing the patches
>>
>>   drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
>>   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> index ab23236aa942..3816a37de4bc 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
>>   	size_t me_buffer_size;
>>   	size_t tmv_buffer_size;
>>   
>> +	enum v4l2_field field;
>> +
>>   	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
>>   
>>   	struct list_head ref_queue;
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> index 367ef8e8dbf0..6e5ca86fb331 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   		   rectangle. */
>>   		pix_mp->width = ctx->buf_width;
>>   		pix_mp->height = ctx->buf_height;
>> -		pix_mp->field = V4L2_FIELD_NONE;
>> +		pix_mp->field = ctx->field;
>>   		pix_mp->num_planes = 2;
>>   		/* Set pixelformat to the format in which MFC
>>   		   outputs the decoded frame */
>> @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   	struct s5p_mfc_dev *dev = video_drvdata(file);
>>   	struct s5p_mfc_fmt *fmt;
>>   
>> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
>> +		f->fmt.pix.field = V4L2_FIELD_NONE;
>> +
>>   	mfc_debug(2, "Type is %d\n", f->type);
>>   	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>   		fmt = find_format(f, MFC_FMT_DEC);
>> @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   		goto out;
>>   	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>   		/* src_fmt is validated by call to vidioc_try_fmt */
>> +		ctx->field = f->fmt.pix.field;
>>   		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
>>   		ctx->codec_mode = ctx->src_fmt->codec_mode;
>>   		mfc_debug(2, "The codec number is: %d\n", ctx->codec_mode);
>

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Thibault Saunier]

Hello,


On 03/01/2017 10:12 AM, Andrzej Hajda wrote:
> On 01.03.2017 12:51, Thibault Saunier wrote:
>> It is required by the standard that the field order is set by the
>> driver, default to NONE in case any is provided, but we can basically
>> accept any value provided by the userspace as we will anyway not
>> be able to do any deinterlacing.
>>
>> In this patch we also make sure to pass the interlacing mode provided
>> by userspace from the output to the capture side of the device so
>> that the information is given back to userspace. This way it can
>> handle it and potentially deinterlace afterward.
> As I wrote previously:
> - on output side you have encoded bytestream - you cannot say about
> interlacing in such case, so the only valid value is NONE,
Well, the encoded stream contains the info about interlacing and
the most logical thing to do from my point of view is to keep that info
passe it along to the capture side, which is what I am doing.
What makes you think this is not the right way of handling that?
> - on capture side you have decoded frames, and in this case it depends
> on the device and driver capabilities, if the driver/device does not
> support (de-)interlacing (I suppose this is MFC case), interlace type
> field should be filled according to decoded bytestream header (on output
> side), but no direct copying from output side!!!
Well, why? If the userspace has already parsed the headers and passed the
info to the decoder, there is no reason we should pass along that info 
to the
capture side.

Currently the bitstream parser in the decoder does not seem to take of
the interlacing properly so if userspace specified it because it already 
parsed the
stream or the info was in the container not in the codec bitstream, then 
using
it is very sensible I think!!!

Regards,

Thibault
>
> Regards
> Andrzej
>
>> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>>
>> ---
>>
>> Changes in v6:
>> - Pass user output field value to the capture as the device is not
>>    doing any deinterlacing and thus decoded content will still be
>>    interlaced on the output.
>>
>> Changes in v5:
>> - Just adapt the field and never error out.
>>
>> Changes in v4: None
>> Changes in v3:
>> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>>
>> Changes in v2:
>> - Fix a silly build error that slipped in while rebasing the patches
>>
>>   drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
>>   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> index ab23236aa942..3816a37de4bc 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
>>   	size_t me_buffer_size;
>>   	size_t tmv_buffer_size;
>>   
>> +	enum v4l2_field field;
>> +
>>   	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
>>   
>>   	struct list_head ref_queue;
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> index 367ef8e8dbf0..6e5ca86fb331 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   		   rectangle. */
>>   		pix_mp->width = ctx->buf_width;
>>   		pix_mp->height = ctx->buf_height;
>> -		pix_mp->field = V4L2_FIELD_NONE;
>> +		pix_mp->field = ctx->field;
>>   		pix_mp->num_planes = 2;
>>   		/* Set pixelformat to the format in which MFC
>>   		   outputs the decoded frame */
>> @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   	struct s5p_mfc_dev *dev = video_drvdata(file);
>>   	struct s5p_mfc_fmt *fmt;
>>   
>> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
>> +		f->fmt.pix.field = V4L2_FIELD_NONE;
>> +
>>   	mfc_debug(2, "Type is %d\n", f->type);
>>   	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>   		fmt = find_format(f, MFC_FMT_DEC);
>> @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   		goto out;
>>   	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>   		/* src_fmt is validated by call to vidioc_try_fmt */
>> +		ctx->field = f->fmt.pix.field;
>>   		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
>>   		ctx->codec_mode = ctx->src_fmt->codec_mode;
>>   		mfc_debug(2, "The codec number is: %d\n", ctx->codec_mode);
>

[PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Thibault Saunier]

Hello,


On 03/01/2017 10:12 AM, Andrzej Hajda wrote:
> On 01.03.2017 12:51, Thibault Saunier wrote:
>> It is required by the standard that the field order is set by the
>> driver, default to NONE in case any is provided, but we can basically
>> accept any value provided by the userspace as we will anyway not
>> be able to do any deinterlacing.
>>
>> In this patch we also make sure to pass the interlacing mode provided
>> by userspace from the output to the capture side of the device so
>> that the information is given back to userspace. This way it can
>> handle it and potentially deinterlace afterward.
> As I wrote previously:
> - on output side you have encoded bytestream - you cannot say about
> interlacing in such case, so the only valid value is NONE,
Well, the encoded stream contains the info about interlacing and
the most logical thing to do from my point of view is to keep that info
passe it along to the capture side, which is what I am doing.
What makes you think this is not the right way of handling that?
> - on capture side you have decoded frames, and in this case it depends
> on the device and driver capabilities, if the driver/device does not
> support (de-)interlacing (I suppose this is MFC case), interlace type
> field should be filled according to decoded bytestream header (on output
> side), but no direct copying from output side!!!
Well, why? If the userspace has already parsed the headers and passed the
info to the decoder, there is no reason we should pass along that info 
to the
capture side.

Currently the bitstream parser in the decoder does not seem to take of
the interlacing properly so if userspace specified it because it already 
parsed the
stream or the info was in the container not in the codec bitstream, then 
using
it is very sensible I think!!!

Regards,

Thibault
>
> Regards
> Andrzej
>
>> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>>
>> ---
>>
>> Changes in v6:
>> - Pass user output field value to the capture as the device is not
>>    doing any deinterlacing and thus decoded content will still be
>>    interlaced on the output.
>>
>> Changes in v5:
>> - Just adapt the field and never error out.
>>
>> Changes in v4: None
>> Changes in v3:
>> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>>
>> Changes in v2:
>> - Fix a silly build error that slipped in while rebasing the patches
>>
>>   drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
>>   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> index ab23236aa942..3816a37de4bc 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
>>   	size_t me_buffer_size;
>>   	size_t tmv_buffer_size;
>>   
>> +	enum v4l2_field field;
>> +
>>   	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
>>   
>>   	struct list_head ref_queue;
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> index 367ef8e8dbf0..6e5ca86fb331 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   		   rectangle. */
>>   		pix_mp->width = ctx->buf_width;
>>   		pix_mp->height = ctx->buf_height;
>> -		pix_mp->field = V4L2_FIELD_NONE;
>> +		pix_mp->field = ctx->field;
>>   		pix_mp->num_planes = 2;
>>   		/* Set pixelformat to the format in which MFC
>>   		   outputs the decoded frame */
>> @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   	struct s5p_mfc_dev *dev = video_drvdata(file);
>>   	struct s5p_mfc_fmt *fmt;
>>   
>> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
>> +		f->fmt.pix.field = V4L2_FIELD_NONE;
>> +
>>   	mfc_debug(2, "Type is %d\n", f->type);
>>   	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>   		fmt = find_format(f, MFC_FMT_DEC);
>> @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   		goto out;
>>   	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>   		/* src_fmt is validated by call to vidioc_try_fmt */
>> +		ctx->field = f->fmt.pix.field;
>>   		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
>>   		ctx->codec_mode = ctx->src_fmt->codec_mode;
>>   		mfc_debug(2, "The codec number is: %d\n", ctx->codec_mode);
>

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 4652 bytes --]

Le mercredi 01 mars 2017 à 14:12 +0100, Andrzej Hajda a écrit :
> On 01.03.2017 12:51, Thibault Saunier wrote:
> > It is required by the standard that the field order is set by the
> > driver, default to NONE in case any is provided, but we can
> > basically
> > accept any value provided by the userspace as we will anyway not
> > be able to do any deinterlacing.
> > 
> > In this patch we also make sure to pass the interlacing mode
> > provided
> > by userspace from the output to the capture side of the device so
> > that the information is given back to userspace. This way it can
> > handle it and potentially deinterlace afterward.
> 
> As I wrote previously:
> - on output side you have encoded bytestream - you cannot say about
> interlacing in such case, so the only valid value is NONE,

Userspace may know. It's important for the driver not to reset it back
to NONE, it would tell the userspace that this encoded format is not
supported when interlaced.

Obviously, when userspace don't know (ANY), it does not matter, it will
fail when we try to set the CAPTURE format. Though, it's quite late in
the process for the userspace, which makes implementing software
fallback difficult.

> - on capture side you have decoded frames, and in this case it
> depends
> on the device and driver capabilities, if the driver/device does not
> support (de-)interlacing (I suppose this is MFC case), interlace type
> field should be filled according to decoded bytestream header (on
> output
> side), but no direct copying from output side!!!

That is exact.

> 
> Regards
> Andrzej
> 
> > 
> > Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
> > 
> > ---
> > 
> > Changes in v6:
> > - Pass user output field value to the capture as the device is not
> >   doing any deinterlacing and thus decoded content will still be
> >   interlaced on the output.
> > 
> > Changes in v5:
> > - Just adapt the field and never error out.
> > 
> > Changes in v4: None
> > Changes in v3:
> > - Do not check values in the g_fmt functions as Andrzej explained
> > in previous review
> > 
> > Changes in v2:
> > - Fix a silly build error that slipped in while rebasing the
> > patches
> > 
> >  drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
> >  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > index ab23236aa942..3816a37de4bc 100644
> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
> >  	size_t me_buffer_size;
> >  	size_t tmv_buffer_size;
> >  
> > +	enum v4l2_field field;
> > +
> >  	enum v4l2_mpeg_mfc51_video_force_frame_type
> > force_frame_type;
> >  
> >  	struct list_head ref_queue;
> > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > index 367ef8e8dbf0..6e5ca86fb331 100644
> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void
> > *priv, struct v4l2_format *f)
> >  		   rectangle. */
> >  		pix_mp->width = ctx->buf_width;
> >  		pix_mp->height = ctx->buf_height;
> > -		pix_mp->field = V4L2_FIELD_NONE;
> > +		pix_mp->field = ctx->field;
> >  		pix_mp->num_planes = 2;
> >  		/* Set pixelformat to the format in which MFC
> >  		   outputs the decoded frame */
> > @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file,
> > void *priv, struct v4l2_format *f)
> >  	struct s5p_mfc_dev *dev = video_drvdata(file);
> >  	struct s5p_mfc_fmt *fmt;
> >  
> > +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
> > +		f->fmt.pix.field = V4L2_FIELD_NONE;
> > +
> >  	mfc_debug(2, "Type is %d\n", f->type);
> >  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> >  		fmt = find_format(f, MFC_FMT_DEC);
> > @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void
> > *priv, struct v4l2_format *f)
> >  		goto out;
> >  	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> >  		/* src_fmt is validated by call to vidioc_try_fmt
> > */
> > +		ctx->field = f->fmt.pix.field;
> >  		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
> >  		ctx->codec_mode = ctx->src_fmt->codec_mode;
> >  		mfc_debug(2, "The codec number is: %d\n", ctx-
> > >codec_mode);
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Nicolas Dufresne]

[-- Attachment #1.1: Type: text/plain, Size: 4652 bytes --]

Le mercredi 01 mars 2017 à 14:12 +0100, Andrzej Hajda a écrit :
> On 01.03.2017 12:51, Thibault Saunier wrote:
> > It is required by the standard that the field order is set by the
> > driver, default to NONE in case any is provided, but we can
> > basically
> > accept any value provided by the userspace as we will anyway not
> > be able to do any deinterlacing.
> > 
> > In this patch we also make sure to pass the interlacing mode
> > provided
> > by userspace from the output to the capture side of the device so
> > that the information is given back to userspace. This way it can
> > handle it and potentially deinterlace afterward.
> 
> As I wrote previously:
> - on output side you have encoded bytestream - you cannot say about
> interlacing in such case, so the only valid value is NONE,

Userspace may know. It's important for the driver not to reset it back
to NONE, it would tell the userspace that this encoded format is not
supported when interlaced.

Obviously, when userspace don't know (ANY), it does not matter, it will
fail when we try to set the CAPTURE format. Though, it's quite late in
the process for the userspace, which makes implementing software
fallback difficult.

> - on capture side you have decoded frames, and in this case it
> depends
> on the device and driver capabilities, if the driver/device does not
> support (de-)interlacing (I suppose this is MFC case), interlace type
> field should be filled according to decoded bytestream header (on
> output
> side), but no direct copying from output side!!!

That is exact.

> 
> Regards
> Andrzej
> 
> > 
> > Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
> > 
> > ---
> > 
> > Changes in v6:
> > - Pass user output field value to the capture as the device is not
> >   doing any deinterlacing and thus decoded content will still be
> >   interlaced on the output.
> > 
> > Changes in v5:
> > - Just adapt the field and never error out.
> > 
> > Changes in v4: None
> > Changes in v3:
> > - Do not check values in the g_fmt functions as Andrzej explained
> > in previous review
> > 
> > Changes in v2:
> > - Fix a silly build error that slipped in while rebasing the
> > patches
> > 
> >  drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
> >  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > index ab23236aa942..3816a37de4bc 100644
> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
> >  	size_t me_buffer_size;
> >  	size_t tmv_buffer_size;
> >  
> > +	enum v4l2_field field;
> > +
> >  	enum v4l2_mpeg_mfc51_video_force_frame_type
> > force_frame_type;
> >  
> >  	struct list_head ref_queue;
> > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > index 367ef8e8dbf0..6e5ca86fb331 100644
> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void
> > *priv, struct v4l2_format *f)
> >  		   rectangle. */
> >  		pix_mp->width = ctx->buf_width;
> >  		pix_mp->height = ctx->buf_height;
> > -		pix_mp->field = V4L2_FIELD_NONE;
> > +		pix_mp->field = ctx->field;
> >  		pix_mp->num_planes = 2;
> >  		/* Set pixelformat to the format in which MFC
> >  		   outputs the decoded frame */
> > @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file,
> > void *priv, struct v4l2_format *f)
> >  	struct s5p_mfc_dev *dev = video_drvdata(file);
> >  	struct s5p_mfc_fmt *fmt;
> >  
> > +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
> > +		f->fmt.pix.field = V4L2_FIELD_NONE;
> > +
> >  	mfc_debug(2, "Type is %d\n", f->type);
> >  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> >  		fmt = find_format(f, MFC_FMT_DEC);
> > @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void
> > *priv, struct v4l2_format *f)
> >  		goto out;
> >  	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> >  		/* src_fmt is validated by call to vidioc_try_fmt
> > */
> > +		ctx->field = f->fmt.pix.field;
> >  		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
> >  		ctx->codec_mode = ctx->src_fmt->codec_mode;
> >  		mfc_debug(2, "The codec number is: %d\n", ctx-
> > >codec_mode);
> 
> 

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Nicolas Dufresne]

Le mercredi 01 mars 2017 ? 14:12 +0100, Andrzej Hajda a ?crit?:
> On 01.03.2017 12:51, Thibault Saunier wrote:
> > It is required by the standard that the field order is set by the
> > driver, default to NONE in case any is provided, but we can
> > basically
> > accept any value provided by the userspace as we will anyway not
> > be able to do any deinterlacing.
> > 
> > In this patch we also make sure to pass the interlacing mode
> > provided
> > by userspace from the output to the capture side of the device so
> > that the information is given back to userspace. This way it can
> > handle it and potentially deinterlace afterward.
> 
> As I wrote previously:
> - on output side you have encoded bytestream - you cannot say about
> interlacing in such case, so the only valid value is NONE,

Userspace may know. It's important for the driver not to reset it back
to NONE, it would tell the userspace that this encoded format is not
supported when interlaced.

Obviously, when userspace don't know (ANY), it does not matter, it will
fail when we try to set the CAPTURE format. Though, it's quite late in
the process for the userspace, which makes implementing software
fallback difficult.

> - on capture side you have decoded frames, and in this case it
> depends
> on the device and driver capabilities, if the driver/device does not
> support (de-)interlacing (I suppose this is MFC case), interlace type
> field should be filled according to decoded bytestream header (on
> output
> side), but no direct copying from output side!!!

That is exact.

> 
> Regards
> Andrzej
> 
> > 
> > Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
> > 
> > ---
> > 
> > Changes in v6:
> > - Pass user output field value to the capture as the device is not
> > ? doing any deinterlacing and thus decoded content will still be
> > ? interlaced on the output.
> > 
> > Changes in v5:
> > - Just adapt the field and never error out.
> > 
> > Changes in v4: None
> > Changes in v3:
> > - Do not check values in the g_fmt functions as Andrzej explained
> > in previous review
> > 
> > Changes in v2:
> > - Fix a silly build error that slipped in while rebasing the
> > patches
> > 
> > ?drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
> > ?drivers/media/platform/s5p-mfc/s5p_mfc_dec.c????| 6 +++++-
> > ?2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > index ab23236aa942..3816a37de4bc 100644
> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
> > ?	size_t me_buffer_size;
> > ?	size_t tmv_buffer_size;
> > ?
> > +	enum v4l2_field field;
> > +
> > ?	enum v4l2_mpeg_mfc51_video_force_frame_type
> > force_frame_type;
> > ?
> > ?	struct list_head ref_queue;
> > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > index 367ef8e8dbf0..6e5ca86fb331 100644
> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void
> > *priv, struct v4l2_format *f)
> > ?		???rectangle. */
> > ?		pix_mp->width = ctx->buf_width;
> > ?		pix_mp->height = ctx->buf_height;
> > -		pix_mp->field = V4L2_FIELD_NONE;
> > +		pix_mp->field = ctx->field;
> > ?		pix_mp->num_planes = 2;
> > ?		/* Set pixelformat to the format in which MFC
> > ?		???outputs the decoded frame */
> > @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file,
> > void *priv, struct v4l2_format *f)
> > ?	struct s5p_mfc_dev *dev = video_drvdata(file);
> > ?	struct s5p_mfc_fmt *fmt;
> > ?
> > +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
> > +		f->fmt.pix.field = V4L2_FIELD_NONE;
> > +
> > ?	mfc_debug(2, "Type is %d\n", f->type);
> > ?	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > ?		fmt = find_format(f, MFC_FMT_DEC);
> > @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void
> > *priv, struct v4l2_format *f)
> > ?		goto out;
> > ?	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > ?		/* src_fmt is validated by call to vidioc_try_fmt
> > */
> > +		ctx->field = f->fmt.pix.field;
> > ?		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
> > ?		ctx->codec_mode = ctx->src_fmt->codec_mode;
> > ?		mfc_debug(2, "The codec number is: %d\n", ctx-
> > >codec_mode);
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170301/5b8317db/attachment-0001.sig>

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Thibault Saunier]

Hello,


On 03/01/2017 11:35 AM, Nicolas Dufresne wrote:
> Le mercredi 01 mars 2017 à 14:12 +0100, Andrzej Hajda a écrit :
>> On 01.03.2017 12:51, Thibault Saunier wrote:
>>> It is required by the standard that the field order is set by the
>>> driver, default to NONE in case any is provided, but we can
>>> basically
>>> accept any value provided by the userspace as we will anyway not
>>> be able to do any deinterlacing.
>>>
>>> In this patch we also make sure to pass the interlacing mode
>>> provided
>>> by userspace from the output to the capture side of the device so
>>> that the information is given back to userspace. This way it can
>>> handle it and potentially deinterlace afterward.
>> As I wrote previously:
>> - on output side you have encoded bytestream - you cannot say about
>> interlacing in such case, so the only valid value is NONE,
> Userspace may know. It's important for the driver not to reset it back
> to NONE, it would tell the userspace that this encoded format is not
> supported when interlaced.
>
> Obviously, when userspace don't know (ANY), it does not matter, it will
> fail when we try to set the CAPTURE format. Though, it's quite late in
> the process for the userspace, which makes implementing software
> fallback difficult.
>
>> - on capture side you have decoded frames, and in this case it
>> depends
>> on the device and driver capabilities, if the driver/device does not
>> support (de-)interlacing (I suppose this is MFC case), interlace type
>> field should be filled according to decoded bytestream header (on
>> output
>> side), but no direct copying from output side!!!
> That is exact.
It is yes, in case the driver is determining it properly, which is not
the case here. So the information we have is what the userspace
figured. That part should be fixed in the driver, but meanwhile it is
better to copy to the output side than just loosing the info as otherwise
the userspace never tries to deinterlace after the decoding because the
driver always says field=NONE now.

Regards,

Thibault


>> Regards
>> Andrzej
>>
>>> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>>>
>>> ---
>>>
>>> Changes in v6:
>>> - Pass user output field value to the capture as the device is not
>>>    doing any deinterlacing and thus decoded content will still be
>>>    interlaced on the output.
>>>
>>> Changes in v5:
>>> - Just adapt the field and never error out.
>>>
>>> Changes in v4: None
>>> Changes in v3:
>>> - Do not check values in the g_fmt functions as Andrzej explained
>>> in previous review
>>>
>>> Changes in v2:
>>> - Fix a silly build error that slipped in while rebasing the
>>> patches
>>>
>>>   drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
>>>   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
>>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> index ab23236aa942..3816a37de4bc 100644
>>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
>>>   	size_t me_buffer_size;
>>>   	size_t tmv_buffer_size;
>>>   
>>> +	enum v4l2_field field;
>>> +
>>>   	enum v4l2_mpeg_mfc51_video_force_frame_type
>>> force_frame_type;
>>>   
>>>   	struct list_head ref_queue;
>>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> index 367ef8e8dbf0..6e5ca86fb331 100644
>>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void
>>> *priv, struct v4l2_format *f)
>>>   		   rectangle. */
>>>   		pix_mp->width = ctx->buf_width;
>>>   		pix_mp->height = ctx->buf_height;
>>> -		pix_mp->field = V4L2_FIELD_NONE;
>>> +		pix_mp->field = ctx->field;
>>>   		pix_mp->num_planes = 2;
>>>   		/* Set pixelformat to the format in which MFC
>>>   		   outputs the decoded frame */
>>> @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file,
>>> void *priv, struct v4l2_format *f)
>>>   	struct s5p_mfc_dev *dev = video_drvdata(file);
>>>   	struct s5p_mfc_fmt *fmt;
>>>   
>>> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
>>> +		f->fmt.pix.field = V4L2_FIELD_NONE;
>>> +
>>>   	mfc_debug(2, "Type is %d\n", f->type);
>>>   	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>>   		fmt = find_format(f, MFC_FMT_DEC);
>>> @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void
>>> *priv, struct v4l2_format *f)
>>>   		goto out;
>>>   	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>>   		/* src_fmt is validated by call to vidioc_try_fmt
>>> */
>>> +		ctx->field = f->fmt.pix.field;
>>>   		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
>>>   		ctx->codec_mode = ctx->src_fmt->codec_mode;
>>>   		mfc_debug(2, "The codec number is: %d\n", ctx-
>>>> codec_mode);

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Thibault Saunier]

Hello,


On 03/01/2017 11:35 AM, Nicolas Dufresne wrote:
> Le mercredi 01 mars 2017 à 14:12 +0100, Andrzej Hajda a écrit :
>> On 01.03.2017 12:51, Thibault Saunier wrote:
>>> It is required by the standard that the field order is set by the
>>> driver, default to NONE in case any is provided, but we can
>>> basically
>>> accept any value provided by the userspace as we will anyway not
>>> be able to do any deinterlacing.
>>>
>>> In this patch we also make sure to pass the interlacing mode
>>> provided
>>> by userspace from the output to the capture side of the device so
>>> that the information is given back to userspace. This way it can
>>> handle it and potentially deinterlace afterward.
>> As I wrote previously:
>> - on output side you have encoded bytestream - you cannot say about
>> interlacing in such case, so the only valid value is NONE,
> Userspace may know. It's important for the driver not to reset it back
> to NONE, it would tell the userspace that this encoded format is not
> supported when interlaced.
>
> Obviously, when userspace don't know (ANY), it does not matter, it will
> fail when we try to set the CAPTURE format. Though, it's quite late in
> the process for the userspace, which makes implementing software
> fallback difficult.
>
>> - on capture side you have decoded frames, and in this case it
>> depends
>> on the device and driver capabilities, if the driver/device does not
>> support (de-)interlacing (I suppose this is MFC case), interlace type
>> field should be filled according to decoded bytestream header (on
>> output
>> side), but no direct copying from output side!!!
> That is exact.
It is yes, in case the driver is determining it properly, which is not
the case here. So the information we have is what the userspace
figured. That part should be fixed in the driver, but meanwhile it is
better to copy to the output side than just loosing the info as otherwise
the userspace never tries to deinterlace after the decoding because the
driver always says field=NONE now.

Regards,

Thibault


>> Regards
>> Andrzej
>>
>>> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>>>
>>> ---
>>>
>>> Changes in v6:
>>> - Pass user output field value to the capture as the device is not
>>>    doing any deinterlacing and thus decoded content will still be
>>>    interlaced on the output.
>>>
>>> Changes in v5:
>>> - Just adapt the field and never error out.
>>>
>>> Changes in v4: None
>>> Changes in v3:
>>> - Do not check values in the g_fmt functions as Andrzej explained
>>> in previous review
>>>
>>> Changes in v2:
>>> - Fix a silly build error that slipped in while rebasing the
>>> patches
>>>
>>>   drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
>>>   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
>>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> index ab23236aa942..3816a37de4bc 100644
>>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
>>>   	size_t me_buffer_size;
>>>   	size_t tmv_buffer_size;
>>>   
>>> +	enum v4l2_field field;
>>> +
>>>   	enum v4l2_mpeg_mfc51_video_force_frame_type
>>> force_frame_type;
>>>   
>>>   	struct list_head ref_queue;
>>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> index 367ef8e8dbf0..6e5ca86fb331 100644
>>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void
>>> *priv, struct v4l2_format *f)
>>>   		   rectangle. */
>>>   		pix_mp->width = ctx->buf_width;
>>>   		pix_mp->height = ctx->buf_height;
>>> -		pix_mp->field = V4L2_FIELD_NONE;
>>> +		pix_mp->field = ctx->field;
>>>   		pix_mp->num_planes = 2;
>>>   		/* Set pixelformat to the format in which MFC
>>>   		   outputs the decoded frame */
>>> @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file,
>>> void *priv, struct v4l2_format *f)
>>>   	struct s5p_mfc_dev *dev = video_drvdata(file);
>>>   	struct s5p_mfc_fmt *fmt;
>>>   
>>> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
>>> +		f->fmt.pix.field = V4L2_FIELD_NONE;
>>> +
>>>   	mfc_debug(2, "Type is %d\n", f->type);
>>>   	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>>   		fmt = find_format(f, MFC_FMT_DEC);
>>> @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void
>>> *priv, struct v4l2_format *f)
>>>   		goto out;
>>>   	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>>   		/* src_fmt is validated by call to vidioc_try_fmt
>>> */
>>> +		ctx->field = f->fmt.pix.field;
>>>   		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
>>>   		ctx->codec_mode = ctx->src_fmt->codec_mode;
>>>   		mfc_debug(2, "The codec number is: %d\n", ctx-
>>>> codec_mode);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Thibault Saunier]

Hello,


On 03/01/2017 11:35 AM, Nicolas Dufresne wrote:
> Le mercredi 01 mars 2017 ? 14:12 +0100, Andrzej Hajda a ?crit :
>> On 01.03.2017 12:51, Thibault Saunier wrote:
>>> It is required by the standard that the field order is set by the
>>> driver, default to NONE in case any is provided, but we can
>>> basically
>>> accept any value provided by the userspace as we will anyway not
>>> be able to do any deinterlacing.
>>>
>>> In this patch we also make sure to pass the interlacing mode
>>> provided
>>> by userspace from the output to the capture side of the device so
>>> that the information is given back to userspace. This way it can
>>> handle it and potentially deinterlace afterward.
>> As I wrote previously:
>> - on output side you have encoded bytestream - you cannot say about
>> interlacing in such case, so the only valid value is NONE,
> Userspace may know. It's important for the driver not to reset it back
> to NONE, it would tell the userspace that this encoded format is not
> supported when interlaced.
>
> Obviously, when userspace don't know (ANY), it does not matter, it will
> fail when we try to set the CAPTURE format. Though, it's quite late in
> the process for the userspace, which makes implementing software
> fallback difficult.
>
>> - on capture side you have decoded frames, and in this case it
>> depends
>> on the device and driver capabilities, if the driver/device does not
>> support (de-)interlacing (I suppose this is MFC case), interlace type
>> field should be filled according to decoded bytestream header (on
>> output
>> side), but no direct copying from output side!!!
> That is exact.
It is yes, in case the driver is determining it properly, which is not
the case here. So the information we have is what the userspace
figured. That part should be fixed in the driver, but meanwhile it is
better to copy to the output side than just loosing the info as otherwise
the userspace never tries to deinterlace after the decoding because the
driver always says field=NONE now.

Regards,

Thibault


>> Regards
>> Andrzej
>>
>>> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>>>
>>> ---
>>>
>>> Changes in v6:
>>> - Pass user output field value to the capture as the device is not
>>>    doing any deinterlacing and thus decoded content will still be
>>>    interlaced on the output.
>>>
>>> Changes in v5:
>>> - Just adapt the field and never error out.
>>>
>>> Changes in v4: None
>>> Changes in v3:
>>> - Do not check values in the g_fmt functions as Andrzej explained
>>> in previous review
>>>
>>> Changes in v2:
>>> - Fix a silly build error that slipped in while rebasing the
>>> patches
>>>
>>>   drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
>>>   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
>>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> index ab23236aa942..3816a37de4bc 100644
>>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>>> @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
>>>   	size_t me_buffer_size;
>>>   	size_t tmv_buffer_size;
>>>   
>>> +	enum v4l2_field field;
>>> +
>>>   	enum v4l2_mpeg_mfc51_video_force_frame_type
>>> force_frame_type;
>>>   
>>>   	struct list_head ref_queue;
>>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> index 367ef8e8dbf0..6e5ca86fb331 100644
>>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void
>>> *priv, struct v4l2_format *f)
>>>   		   rectangle. */
>>>   		pix_mp->width = ctx->buf_width;
>>>   		pix_mp->height = ctx->buf_height;
>>> -		pix_mp->field = V4L2_FIELD_NONE;
>>> +		pix_mp->field = ctx->field;
>>>   		pix_mp->num_planes = 2;
>>>   		/* Set pixelformat to the format in which MFC
>>>   		   outputs the decoded frame */
>>> @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file,
>>> void *priv, struct v4l2_format *f)
>>>   	struct s5p_mfc_dev *dev = video_drvdata(file);
>>>   	struct s5p_mfc_fmt *fmt;
>>>   
>>> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
>>> +		f->fmt.pix.field = V4L2_FIELD_NONE;
>>> +
>>>   	mfc_debug(2, "Type is %d\n", f->type);
>>>   	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>>   		fmt = find_format(f, MFC_FMT_DEC);
>>> @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void
>>> *priv, struct v4l2_format *f)
>>>   		goto out;
>>>   	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>>>   		/* src_fmt is validated by call to vidioc_try_fmt
>>> */
>>> +		ctx->field = f->fmt.pix.field;
>>>   		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
>>>   		ctx->codec_mode = ctx->src_fmt->codec_mode;
>>>   		mfc_debug(2, "The codec number is: %d\n", ctx-
>>>> codec_mode);

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1026 bytes --]

Le mercredi 01 mars 2017 à 14:12 +0100, Andrzej Hajda a écrit :
> - on output side you have encoded bytestream - you cannot say about
> interlacing in such case, so the only valid value is NONE,
> - on capture side you have decoded frames, and in this case it
> depends
> on the device and driver capabilities, if the driver/device does not
> support (de-)interlacing (I suppose this is MFC case), interlace type
> field should be filled according to decoded bytestream header (on
> output
> side), but no direct copying from output side!!!

I think we need some nuance here for this to actually be usable. If the
information is not provided by the driver (yes, hardware is limiting
sometimes), it would make sense to copy over the information that
userspace provided. Setting NONE is just the worst approximation in my
opinion.

About MFC, it will be worth trying to read the DISPLAY_STATUS after the
headers has been processed. It's not clearly stated in the spec if this
will be set or not.

Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Nicolas Dufresne]

[-- Attachment #1.1: Type: text/plain, Size: 1026 bytes --]

Le mercredi 01 mars 2017 à 14:12 +0100, Andrzej Hajda a écrit :
> - on output side you have encoded bytestream - you cannot say about
> interlacing in such case, so the only valid value is NONE,
> - on capture side you have decoded frames, and in this case it
> depends
> on the device and driver capabilities, if the driver/device does not
> support (de-)interlacing (I suppose this is MFC case), interlace type
> field should be filled according to decoded bytestream header (on
> output
> side), but no direct copying from output side!!!

I think we need some nuance here for this to actually be usable. If the
information is not provided by the driver (yes, hardware is limiting
sometimes), it would make sense to copy over the information that
userspace provided. Setting NONE is just the worst approximation in my
opinion.

About MFC, it will be worth trying to read the DISPLAY_STATUS after the
headers has been processed. It's not clearly stated in the spec if this
will be set or not.

Nicolas

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Nicolas Dufresne]

Le mercredi 01 mars 2017 ? 14:12 +0100, Andrzej Hajda a ?crit?:
> - on output side you have encoded bytestream - you cannot say about
> interlacing in such case, so the only valid value is NONE,
> - on capture side you have decoded frames, and in this case it
> depends
> on the device and driver capabilities, if the driver/device does not
> support (de-)interlacing (I suppose this is MFC case), interlace type
> field should be filled according to decoded bytestream header (on
> output
> side), but no direct copying from output side!!!

I think we need some nuance here for this to actually be usable. If the
information is not provided by the driver (yes, hardware is limiting
sometimes), it would make sense to copy over the information that
userspace provided. Setting NONE is just the worst approximation in my
opinion.

About MFC, it will be worth trying to read the DISPLAY_STATUS after the
headers has been processed. It's not clearly stated in the spec if this
will be set or not.

Nicolas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170301/cd10c72b/attachment.sig>

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Andrzej Hajda]

On 01.03.2017 16:21, Nicolas Dufresne wrote:
> Le mercredi 01 mars 2017 à 14:12 +0100, Andrzej Hajda a écrit :
>> - on output side you have encoded bytestream - you cannot say about
>> interlacing in such case, so the only valid value is NONE,
>> - on capture side you have decoded frames, and in this case it
>> depends
>> on the device and driver capabilities, if the driver/device does not
>> support (de-)interlacing (I suppose this is MFC case), interlace type
>> field should be filled according to decoded bytestream header (on
>> output
>> side), but no direct copying from output side!!!
> I think we need some nuance here for this to actually be usable. If the
> information is not provided by the driver (yes, hardware is limiting
> sometimes), it would make sense to copy over the information that
> userspace provided. Setting NONE is just the worst approximation in my
> opinion.

The whole point is that s_fmt on output side is to describe format of
the stream passed to the device, and in case of decoder it is just
mpeg/h.26x/... stream. It does not contain frames, fields, width, height
- it is just raw stream of bytes. We cannot say in such case about field
type, there is not such thing as interlaced byte stream.
Using s_fmt on output to describe things on capture side look for me
unnecessary and abuses V4L2 API IMO.

>
> About MFC, it will be worth trying to read the DISPLAY_STATUS after the
> headers has been processed. It's not clearly stated in the spec if this
> will be set or not.
>
Documentation for MFC6.5 states clearly:

> Note: On SEQ_DONE, INTERLACE_PICTURE will return the picture type to
> be decoded based on the
> sequence header information.

In case of MFC5.1 it is unclear, but I hope HW behaves the same way.

Anyway I agree it will be good to fix it at least for MFC6.5+, any
volunteer?


Regards

Andrzej

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Andrzej Hajda]

On 01.03.2017 16:21, Nicolas Dufresne wrote:
> Le mercredi 01 mars 2017 à 14:12 +0100, Andrzej Hajda a écrit :
>> - on output side you have encoded bytestream - you cannot say about
>> interlacing in such case, so the only valid value is NONE,
>> - on capture side you have decoded frames, and in this case it
>> depends
>> on the device and driver capabilities, if the driver/device does not
>> support (de-)interlacing (I suppose this is MFC case), interlace type
>> field should be filled according to decoded bytestream header (on
>> output
>> side), but no direct copying from output side!!!
> I think we need some nuance here for this to actually be usable. If the
> information is not provided by the driver (yes, hardware is limiting
> sometimes), it would make sense to copy over the information that
> userspace provided. Setting NONE is just the worst approximation in my
> opinion.

The whole point is that s_fmt on output side is to describe format of
the stream passed to the device, and in case of decoder it is just
mpeg/h.26x/... stream. It does not contain frames, fields, width, height
- it is just raw stream of bytes. We cannot say in such case about field
type, there is not such thing as interlaced byte stream.
Using s_fmt on output to describe things on capture side look for me
unnecessary and abuses V4L2 API IMO.

>
> About MFC, it will be worth trying to read the DISPLAY_STATUS after the
> headers has been processed. It's not clearly stated in the spec if this
> will be set or not.
>
Documentation for MFC6.5 states clearly:

> Note: On SEQ_DONE, INTERLACE_PICTURE will return the picture type to
> be decoded based on the
> sequence header information.

In case of MFC5.1 it is unclear, but I hope HW behaves the same way.

Anyway I agree it will be good to fix it at least for MFC6.5+, any
volunteer?


Regards

Andrzej


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Andrzej Hajda]

On 01.03.2017 16:21, Nicolas Dufresne wrote:
> Le mercredi 01 mars 2017 ? 14:12 +0100, Andrzej Hajda a ?crit :
>> - on output side you have encoded bytestream - you cannot say about
>> interlacing in such case, so the only valid value is NONE,
>> - on capture side you have decoded frames, and in this case it
>> depends
>> on the device and driver capabilities, if the driver/device does not
>> support (de-)interlacing (I suppose this is MFC case), interlace type
>> field should be filled according to decoded bytestream header (on
>> output
>> side), but no direct copying from output side!!!
> I think we need some nuance here for this to actually be usable. If the
> information is not provided by the driver (yes, hardware is limiting
> sometimes), it would make sense to copy over the information that
> userspace provided. Setting NONE is just the worst approximation in my
> opinion.

The whole point is that s_fmt on output side is to describe format of
the stream passed to the device, and in case of decoder it is just
mpeg/h.26x/... stream. It does not contain frames, fields, width, height
- it is just raw stream of bytes. We cannot say in such case about field
type, there is not such thing as interlaced byte stream.
Using s_fmt on output to describe things on capture side look for me
unnecessary and abuses V4L2 API IMO.

>
> About MFC, it will be worth trying to read the DISPLAY_STATUS after the
> headers has been processed. It's not clearly stated in the spec if this
> will be set or not.
>
Documentation for MFC6.5 states clearly:

> Note: On SEQ_DONE, INTERLACE_PICTURE will return the picture type to
> be decoded based on the
> sequence header information.

In case of MFC5.1 it is unclear, but I hope HW behaves the same way.

Anyway I agree it will be good to fix it at least for MFC6.5+, any
volunteer?


Regards

Andrzej

Re: [PATCH v6 1/2] [media] exynos-gsc: Use user configured colorspace if provided

[Hans Verkuil]

On 01/03/17 12:51, Thibault Saunier wrote:
> Use colorspace provided by the user as we are only doing scaling and
> color encoding conversion, we won't be able to transform the colorspace
> itself and the colorspace won't mater in that operation.
> 
> Also always use output colorspace on the capture side.
> 
> If the user does not provide a colorspace do no make it up, we might
> later while processing need to figure out the colorspace, which
> is possible depending on the frame size but do not ever guess and
> leak that guess to the userspace.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> ---
> 
> Changes in v6:
> - Do not ever guess colorspace
> 
> Changes in v5:
> - Squash commit to always use output colorspace on the capture side
>   inside this one
> - Fix typo in commit message
> 
> Changes in v4:
> - Reword commit message to better back our assumptions on specifications
> 
> Changes in v3:
> - Do not check values in the g_fmt functions as Andrzej explained in previous review
> - Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'
> 
> Changes in v2: None
> 
>  drivers/media/platform/exynos-gsc/gsc-core.c | 9 ++++-----
>  drivers/media/platform/exynos-gsc/gsc-core.h | 1 +
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 59a634201830..0241168c85af 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -454,6 +454,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>  	} else {
>  		min_w = variant->pix_min->target_rot_dis_w;
>  		min_h = variant->pix_min->target_rot_dis_h;
> +		pix_mp->colorspace = ctx->out_colorspace;
>  	}
>  
>  	pr_debug("mod_x: %d, mod_y: %d, max_w: %d, max_h = %d",
> @@ -472,10 +473,8 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>  
>  	pix_mp->num_planes = fmt->num_planes;
>  
> -	if (pix_mp->width >= 1280) /* HD */
> -		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
> -	else /* SD */
> -		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	if (V4L2_TYPE_IS_OUTPUT(f->type))
> +		ctx->out_colorspace = pix_mp->colorspace;
>  
>  	for (i = 0; i < pix_mp->num_planes; ++i) {
>  		struct v4l2_plane_pix_format *plane_fmt = &pix_mp->plane_fmt[i];
> @@ -519,8 +518,8 @@ int gsc_g_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>  	pix_mp->height		= frame->f_height;
>  	pix_mp->field		= V4L2_FIELD_NONE;
>  	pix_mp->pixelformat	= frame->fmt->pixelformat;
> -	pix_mp->colorspace	= V4L2_COLORSPACE_REC709;
>  	pix_mp->num_planes	= frame->fmt->num_planes;
> +	pix_mp->colorspace = ctx->out_colorspace;

You need to do the same for the ycbcr_enc, xfer_func and quantization fields.
With that in place it is fully 'colorspace compliant' :-)

Regards,

	Hans

>  
>  	for (i = 0; i < pix_mp->num_planes; ++i) {
>  		pix_mp->plane_fmt[i].bytesperline = (frame->f_width *
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.h b/drivers/media/platform/exynos-gsc/gsc-core.h
> index 696217e9af66..715d9c9d8d30 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.h
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.h
> @@ -376,6 +376,7 @@ struct gsc_ctx {
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct gsc_ctrls	gsc_ctrls;
>  	bool			ctrls_rdy;
> +	enum v4l2_colorspace out_colorspace;
>  };
>  
>  void gsc_set_prefbuf(struct gsc_dev *gsc, struct gsc_frame *frm);
> 

[PATCH v6 1/2] [media] exynos-gsc: Use user configured colorspace if provided

[Hans Verkuil]

On 01/03/17 12:51, Thibault Saunier wrote:
> Use colorspace provided by the user as we are only doing scaling and
> color encoding conversion, we won't be able to transform the colorspace
> itself and the colorspace won't mater in that operation.
> 
> Also always use output colorspace on the capture side.
> 
> If the user does not provide a colorspace do no make it up, we might
> later while processing need to figure out the colorspace, which
> is possible depending on the frame size but do not ever guess and
> leak that guess to the userspace.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> ---
> 
> Changes in v6:
> - Do not ever guess colorspace
> 
> Changes in v5:
> - Squash commit to always use output colorspace on the capture side
>   inside this one
> - Fix typo in commit message
> 
> Changes in v4:
> - Reword commit message to better back our assumptions on specifications
> 
> Changes in v3:
> - Do not check values in the g_fmt functions as Andrzej explained in previous review
> - Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'
> 
> Changes in v2: None
> 
>  drivers/media/platform/exynos-gsc/gsc-core.c | 9 ++++-----
>  drivers/media/platform/exynos-gsc/gsc-core.h | 1 +
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 59a634201830..0241168c85af 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -454,6 +454,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>  	} else {
>  		min_w = variant->pix_min->target_rot_dis_w;
>  		min_h = variant->pix_min->target_rot_dis_h;
> +		pix_mp->colorspace = ctx->out_colorspace;
>  	}
>  
>  	pr_debug("mod_x: %d, mod_y: %d, max_w: %d, max_h = %d",
> @@ -472,10 +473,8 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>  
>  	pix_mp->num_planes = fmt->num_planes;
>  
> -	if (pix_mp->width >= 1280) /* HD */
> -		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
> -	else /* SD */
> -		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	if (V4L2_TYPE_IS_OUTPUT(f->type))
> +		ctx->out_colorspace = pix_mp->colorspace;
>  
>  	for (i = 0; i < pix_mp->num_planes; ++i) {
>  		struct v4l2_plane_pix_format *plane_fmt = &pix_mp->plane_fmt[i];
> @@ -519,8 +518,8 @@ int gsc_g_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>  	pix_mp->height		= frame->f_height;
>  	pix_mp->field		= V4L2_FIELD_NONE;
>  	pix_mp->pixelformat	= frame->fmt->pixelformat;
> -	pix_mp->colorspace	= V4L2_COLORSPACE_REC709;
>  	pix_mp->num_planes	= frame->fmt->num_planes;
> +	pix_mp->colorspace = ctx->out_colorspace;

You need to do the same for the ycbcr_enc, xfer_func and quantization fields.
With that in place it is fully 'colorspace compliant' :-)

Regards,

	Hans

>  
>  	for (i = 0; i < pix_mp->num_planes; ++i) {
>  		pix_mp->plane_fmt[i].bytesperline = (frame->f_width *
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.h b/drivers/media/platform/exynos-gsc/gsc-core.h
> index 696217e9af66..715d9c9d8d30 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.h
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.h
> @@ -376,6 +376,7 @@ struct gsc_ctx {
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct gsc_ctrls	gsc_ctrls;
>  	bool			ctrls_rdy;
> +	enum v4l2_colorspace out_colorspace;
>  };
>  
>  void gsc_set_prefbuf(struct gsc_dev *gsc, struct gsc_frame *frm);
> 

Re: [PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Hans Verkuil]

On 01/03/17 12:51, Thibault Saunier wrote:
> It is required by the standard that the field order is set by the
> driver, default to NONE in case any is provided, but we can basically
> accept any value provided by the userspace as we will anyway not
> be able to do any deinterlacing.
> 
> In this patch we also make sure to pass the interlacing mode provided
> by userspace from the output to the capture side of the device so
> that the information is given back to userspace. This way it can
> handle it and potentially deinterlace afterward.
> 
> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
> 
> ---
> 
> Changes in v6:
> - Pass user output field value to the capture as the device is not
>   doing any deinterlacing and thus decoded content will still be
>   interlaced on the output.
> 
> Changes in v5:
> - Just adapt the field and never error out.
> 
> Changes in v4: None
> Changes in v3:
> - Do not check values in the g_fmt functions as Andrzej explained in previous review
> 
> Changes in v2:
> - Fix a silly build error that slipped in while rebasing the patches
> 
>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> index ab23236aa942..3816a37de4bc 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
>  	size_t me_buffer_size;
>  	size_t tmv_buffer_size;
>  
> +	enum v4l2_field field;
> +
>  	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
>  
>  	struct list_head ref_queue;
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index 367ef8e8dbf0..6e5ca86fb331 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		   rectangle. */
>  		pix_mp->width = ctx->buf_width;
>  		pix_mp->height = ctx->buf_height;
> -		pix_mp->field = V4L2_FIELD_NONE;
> +		pix_mp->field = ctx->field;
>  		pix_mp->num_planes = 2;
>  		/* Set pixelformat to the format in which MFC
>  		   outputs the decoded frame */
> @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	struct s5p_mfc_dev *dev = video_drvdata(file);
>  	struct s5p_mfc_fmt *fmt;
>  
> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
> +		f->fmt.pix.field = V4L2_FIELD_NONE;
> +
>  	mfc_debug(2, "Type is %d\n", f->type);
>  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		fmt = find_format(f, MFC_FMT_DEC);
> @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		goto out;
>  	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		/* src_fmt is validated by call to vidioc_try_fmt */
> +		ctx->field = f->fmt.pix.field;

Doing this means that you also allow for V4L2_FIELD_ALTERNATE. If you do that, then there
are additional requirements when you queue up buffers in that the 'field' has to be set to
TOP or BOTTOM in struct v4l2_buffer. I am sure that is a requirement for the encoder, how
this would work for a decoder I am not sure.

Also, values like V4L2_FIELD_SEQ_TB/BT would not behave well if you pass them through a
decoder.

Frankly I think the only 'reasonable' values would be FIELD_NONE, FIELD_INTERLACED and
possibly FIELD_ALTERNATE. I don't know enough about how codecs handle interlaced formats,
so I can't tell how much sense FIELD_ALTERNATE would make.

Properly supporting interlaced formats should only be done if you actually tested it and
know how it works and what exactly is supported.

Regards,

	Hans

>  		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
>  		ctx->codec_mode = ctx->src_fmt->codec_mode;
>  		mfc_debug(2, "The codec number is: %d\n", ctx->codec_mode);
> 

[PATCH v6 2/2] [media] s5p-mfc: Handle 'v4l2_pix_format:field' in try_fmt and g_fmt

[Hans Verkuil]

On 01/03/17 12:51, Thibault Saunier wrote:
> It is required by the standard that the field order is set by the
> driver, default to NONE in case any is provided, but we can basically
> accept any value provided by the userspace as we will anyway not
> be able to do any deinterlacing.
> 
> In this patch we also make sure to pass the interlacing mode provided
> by userspace from the output to the capture side of the device so
> that the information is given back to userspace. This way it can
> handle it and potentially deinterlace afterward.
> 
> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
> 
> ---
> 
> Changes in v6:
> - Pass user output field value to the capture as the device is not
>   doing any deinterlacing and thus decoded content will still be
>   interlaced on the output.
> 
> Changes in v5:
> - Just adapt the field and never error out.
> 
> Changes in v4: None
> Changes in v3:
> - Do not check values in the g_fmt functions as Andrzej explained in previous review
> 
> Changes in v2:
> - Fix a silly build error that slipped in while rebasing the patches
> 
>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> index ab23236aa942..3816a37de4bc 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> @@ -652,6 +652,8 @@ struct s5p_mfc_ctx {
>  	size_t me_buffer_size;
>  	size_t tmv_buffer_size;
>  
> +	enum v4l2_field field;
> +
>  	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
>  
>  	struct list_head ref_queue;
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index 367ef8e8dbf0..6e5ca86fb331 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -345,7 +345,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		   rectangle. */
>  		pix_mp->width = ctx->buf_width;
>  		pix_mp->height = ctx->buf_height;
> -		pix_mp->field = V4L2_FIELD_NONE;
> +		pix_mp->field = ctx->field;
>  		pix_mp->num_planes = 2;
>  		/* Set pixelformat to the format in which MFC
>  		   outputs the decoded frame */
> @@ -380,6 +380,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	struct s5p_mfc_dev *dev = video_drvdata(file);
>  	struct s5p_mfc_fmt *fmt;
>  
> +	if (f->fmt.pix.field == V4L2_FIELD_ANY)
> +		f->fmt.pix.field = V4L2_FIELD_NONE;
> +
>  	mfc_debug(2, "Type is %d\n", f->type);
>  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		fmt = find_format(f, MFC_FMT_DEC);
> @@ -436,6 +439,7 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		goto out;
>  	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		/* src_fmt is validated by call to vidioc_try_fmt */
> +		ctx->field = f->fmt.pix.field;

Doing this means that you also allow for V4L2_FIELD_ALTERNATE. If you do that, then there
are additional requirements when you queue up buffers in that the 'field' has to be set to
TOP or BOTTOM in struct v4l2_buffer. I am sure that is a requirement for the encoder, how
this would work for a decoder I am not sure.

Also, values like V4L2_FIELD_SEQ_TB/BT would not behave well if you pass them through a
decoder.

Frankly I think the only 'reasonable' values would be FIELD_NONE, FIELD_INTERLACED and
possibly FIELD_ALTERNATE. I don't know enough about how codecs handle interlaced formats,
so I can't tell how much sense FIELD_ALTERNATE would make.

Properly supporting interlaced formats should only be done if you actually tested it and
know how it works and what exactly is supported.

Regards,

	Hans

>  		ctx->src_fmt = find_format(f, MFC_FMT_DEC);
>  		ctx->codec_mode = ctx->src_fmt->codec_mode;
>  		mfc_debug(2, "The codec number is: %d\n", ctx->codec_mode);
>