s5p-mfc encoder fixes

PATCH In review Andrzej Hajda 2 participants 8 messages May 28, 2013 Media (V4L/DVB)

Share & tools

Andrzej Hajda

author

2013-05-28 07:26 UTC #0 lore raw

cover s5p-mfc encoder fixes

Those three independent patches are fixes for s5p-mfc encoder. The first one is a serious bug fix - some controls were not working properly. The latter two fixes minor issues.

Regards
Andrzej Hajda

Andrzej Hajda (3):
s5p-mfc: separate encoder parameters for h264 and mpeg4
s5p-mfc: v4l2 controls setup routine moved to initialization code
s5p-mfc: added missing end-of-lines in debug messages

 drivers/media/platform/s5p-mfc/s5p_mfc.c        |  2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |  2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_debug.h  |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    | 18 +++++++++---------
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 14 +++++++-------
 drivers/media/platform/s5p-mfc/s5p_mfc_pm.c     |  4 ++--
 7 files changed, 24 insertions(+), 24 deletions(-)

Signature · 1 line

1.8.1.2

Andrzej Hajda

author

2013-05-28 07:26 UTC #1 lore raw

PATCH 1/3 s5p-mfc: separate encoder parameters for h264 and mpeg4

This patch fixes a bug which caused overwriting h264 codec parameters by mpeg4 parameters during V4L2 control setting.

Signed-off-by Andrzej Hajda <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/media/platform/s5p-mfc/s5p_mfc_common.h +1 -1

index 202d1d7..098459e 100644
@@ -438,7 +438,7 @@ struct s5p_mfc_enc_params {
 	u32 rc_framerate_num;
 	u32 rc_framerate_denom;
 
-	union {
+	struct {
 		struct s5p_mfc_h264_enc_params h264;
 		struct s5p_mfc_mpeg4_enc_params mpeg4;
 	} codec;

Signature · 1 line

1.8.1.2

Sachin Kamat

2013-05-28 08:31 UTC #2 lore raw

Re: [PATCH 1/3] s5p-mfc: separate encoder parameters for h264 and mpeg4

Hi Andrzej,

Quoted 2 lines · Andrzej Hajda

This patch fixes a bug which caused overwriting h264 codec parameters by mpeg4 parameters during V4L2 control setting.

Just curious, what was the use case that triggered this issue?

Signature · 2 lines

With warm regards,
Sachin

Andrzej Hajda

author

2013-05-28 09:11 UTC #3 lore raw

Re: [PATCH 1/3] s5p-mfc: separate encoder parameters for h264 and mpeg4

Quoted 7 lines · Sachin Kamat

Hi Andrzej,
Quoted 2 lines · Andrzej Hajda
This patch fixes a bug which caused overwriting h264 codec parameters by mpeg4 parameters during V4L2 control setting.
Just curious, what was the use case that triggered this issue?

For example it was not possible to set h264 profile and level - they were overwritten by "struct s5p_mfc_mpeg4_enc_params" fields.

In general all 'union' fields of s5p_mfc_h264_enc_params were overwritten by
s5p_mfc_mpeg4_enc_params and vice versa, the control which was set later was 'the winner'.
Furthermore during stream start v4l2_ctrl_handler_setup was called so all controls
were refreshed, so the final winners order was determined by controls definition order.

Regards
Andrzej

Andrzej Hajda

author

2013-05-28 07:26 UTC #4 lore raw

PATCH 2/3 s5p-mfc: v4l2 controls setup routine moved to initialization code

In reply to Andrzej Hajda

Callback .start_streaming is called once for every queue, so v4l2_ctrl_handler_setup was called twice during stream start. Moving v4l2_ctrl_handler_setup to context initialization reduces numbers of calls and seems to be more consistent with API.

Signed-off-by Andrzej Hajda <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/media/platform/s5p-mfc/s5p_mfc_enc.c +1 -1

index 4f6b553..64bb31e 100644
@@ -1760,7 +1760,6 @@ static int s5p_mfc_start_streaming(struct vb2_queue *q, unsigned int count)
 	struct s5p_mfc_ctx *ctx = fh_to_ctx(q->drv_priv);
 	struct s5p_mfc_dev *dev = ctx->dev;
 
-	v4l2_ctrl_handler_setup(&ctx->ctrl_handler);
 	/* If context is ready then dev = work->data;schedule it to run */
 	if (s5p_mfc_ctx_ready(ctx))
 		set_work_bit_irqsave(ctx);

@@ -1920,6 +1919,7 @@ int s5p_mfc_enc_ctrls_setup(struct s5p_mfc_ctx *ctx)
 		if (controls[i].is_volatile && ctx->ctrls[i])
 			ctx->ctrls[i]->flags |= V4L2_CTRL_FLAG_VOLATILE;
 	}
+	v4l2_ctrl_handler_setup(&ctx->ctrl_handler);
 	return 0;
 }

Signature · 1 line

1.8.1.2

Andrzej Hajda

author

2013-05-28 07:26 UTC #5 lore raw

PATCH 3/3 s5p-mfc: added missing end-of-lines in debug messages

In reply to Andrzej Hajda

Many debug messages missed end-of-line.

Signed-off-by Andrzej Hajda <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/media/platform/s5p-mfc/s5p_mfc.c        |  2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_debug.h  |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    | 16 ++++++++--------
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 ++++++++--------
 drivers/media/platform/s5p-mfc/s5p_mfc_pm.c     |  4 ++--
 6 files changed, 23 insertions(+), 23 deletions(-)

drivers/media/platform/s5p-mfc/s5p_mfc.c +1 -1

index 01f9ae0..e04d97f 100644
@@ -562,7 +562,7 @@ static void s5p_mfc_handle_stream_complete(struct s5p_mfc_ctx *ctx,
 	struct s5p_mfc_dev *dev = ctx->dev;
 	struct s5p_mfc_buf *mb_entry;
 
-	mfc_debug(2, "Stream completed");
+	mfc_debug(2, "Stream completed\n");
 
 	s5p_mfc_clear_int_flags(dev);
 	ctx->int_type = reason;

drivers/media/platform/s5p-mfc/s5p_mfc_debug.h +2 -2

index bd5cd4a..8e608f5 100644
@@ -30,8 +30,8 @@ extern int debug;
 #define mfc_debug(level, fmt, args...)
 #endif
 
-#define mfc_debug_enter() mfc_debug(5, "enter")
-#define mfc_debug_leave() mfc_debug(5, "leave")
+#define mfc_debug_enter() mfc_debug(5, "enter\n")
+#define mfc_debug_leave() mfc_debug(5, "leave\n")
 
 #define mfc_err(fmt, args...)				\
 	do {						\

drivers/media/platform/s5p-mfc/s5p_mfc_enc.c +8 -8

index 64bb31e..f6eb28d 100644
@@ -717,9 +717,9 @@ static int enc_post_frame_start(struct s5p_mfc_ctx *ctx)
 
 	slice_type = s5p_mfc_hw_call(dev->mfc_ops, get_enc_slice_type, dev);
 	strm_size = s5p_mfc_hw_call(dev->mfc_ops, get_enc_strm_size, dev);
-	mfc_debug(2, "Encoded slice type: %d", slice_type);
-	mfc_debug(2, "Encoded stream size: %d", strm_size);
-	mfc_debug(2, "Display order: %d",
+	mfc_debug(2, "Encoded slice type: %d\n", slice_type);
+	mfc_debug(2, "Encoded stream size: %d\n", strm_size);
+	mfc_debug(2, "Display order: %d\n",
 		  mfc_read(dev, S5P_FIMV_ENC_SI_PIC_CNT));
 	spin_lock_irqsave(&dev->irqlock, flags);
 	if (slice_type >= 0) {

@@ -1533,14 +1533,14 @@ int vidioc_encoder_cmd(struct file *file, void *priv,
 
 		spin_lock_irqsave(&dev->irqlock, flags);
 		if (list_empty(&ctx->src_queue)) {
-			mfc_debug(2, "EOS: empty src queue, entering finishing state");
+			mfc_debug(2, "EOS: empty src queue, entering finishing state\n");
 			ctx->state = MFCINST_FINISHING;
 			if (s5p_mfc_ctx_ready(ctx))
 				set_work_bit_irqsave(ctx);
 			spin_unlock_irqrestore(&dev->irqlock, flags);
 			s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
 		} else {
-			mfc_debug(2, "EOS: marking last buffer of stream");
+			mfc_debug(2, "EOS: marking last buffer of stream\n");
 			buf = list_entry(ctx->src_queue.prev,
 						struct s5p_mfc_buf, list);
 			if (buf->flags & MFC_BUF_FLAG_USED)

@@ -1609,9 +1609,9 @@ static int check_vb_with_fmt(struct s5p_mfc_fmt *fmt, struct vb2_buffer *vb)
 			mfc_err("failed to get plane cookie\n");
 			return -EINVAL;
 		}
-		mfc_debug(2, "index: %d, plane[%d] cookie: 0x%08zx",
-				vb->v4l2_buf.index, i,
-				vb2_dma_contig_plane_dma_addr(vb, i));
+		mfc_debug(2, "index: %d, plane[%d] cookie: 0x%08zx\n",
+			  vb->v4l2_buf.index, i,
+			  vb2_dma_contig_plane_dma_addr(vb, i));
 	}
 	return 0;
 }

drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c +2 -2

index 0af05a2..368582b 100644
@@ -1275,8 +1275,8 @@ static int s5p_mfc_run_enc_frame(struct s5p_mfc_ctx *ctx)
 	spin_unlock_irqrestore(&dev->irqlock, flags);
 	dev->curr_ctx = ctx->num;
 	s5p_mfc_clean_ctx_int_flags(ctx);
-	mfc_debug(2, "encoding buffer with index=%d state=%d",
-			src_mb ? src_mb->b->v4l2_buf.index : -1, ctx->state);
+	mfc_debug(2, "encoding buffer with index=%d state=%d\n",
+		  src_mb ? src_mb->b->v4l2_buf.index : -1, ctx->state);
 	s5p_mfc_encode_one_frame_v5(ctx);
 	return 0;
 }

drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c +8 -8

index 7e76fce..2799132 100644
@@ -449,8 +449,8 @@ static int s5p_mfc_set_enc_stream_buffer_v6(struct s5p_mfc_ctx *ctx,
 	WRITEL(addr, S5P_FIMV_E_STREAM_BUFFER_ADDR_V6); /* 16B align */
 	WRITEL(size, S5P_FIMV_E_STREAM_BUFFER_SIZE_V6);
 
-	mfc_debug(2, "stream buf addr: 0x%08lx, size: 0x%d",
-		addr, size);
+	mfc_debug(2, "stream buf addr: 0x%08lx, size: 0x%d\n",
+		  addr, size);
 
 	return 0;
 }

@@ -463,8 +463,8 @@ static void s5p_mfc_set_enc_frame_buffer_v6(struct s5p_mfc_ctx *ctx,
 	WRITEL(y_addr, S5P_FIMV_E_SOURCE_LUMA_ADDR_V6); /* 256B align */
 	WRITEL(c_addr, S5P_FIMV_E_SOURCE_CHROMA_ADDR_V6);
 
-	mfc_debug(2, "enc src y buf addr: 0x%08lx", y_addr);
-	mfc_debug(2, "enc src c buf addr: 0x%08lx", c_addr);
+	mfc_debug(2, "enc src y buf addr: 0x%08lx\n", y_addr);
+	mfc_debug(2, "enc src c buf addr: 0x%08lx\n", c_addr);
 }
 
 static void s5p_mfc_get_enc_frame_buffer_v6(struct s5p_mfc_ctx *ctx,

@@ -479,8 +479,8 @@ static void s5p_mfc_get_enc_frame_buffer_v6(struct s5p_mfc_ctx *ctx,
 	enc_recon_y_addr = READL(S5P_FIMV_E_RECON_LUMA_DPB_ADDR_V6);
 	enc_recon_c_addr = READL(S5P_FIMV_E_RECON_CHROMA_DPB_ADDR_V6);
 
-	mfc_debug(2, "recon y addr: 0x%08lx", enc_recon_y_addr);
-	mfc_debug(2, "recon c addr: 0x%08lx", enc_recon_c_addr);
+	mfc_debug(2, "recon y addr: 0x%08lx\n", enc_recon_y_addr);
+	mfc_debug(2, "recon c addr: 0x%08lx\n", enc_recon_c_addr);
 }
 
 /* Set encoding ref & codec buffer */

@@ -1431,8 +1431,8 @@ static inline int s5p_mfc_run_enc_frame(struct s5p_mfc_ctx *ctx)
 	src_y_addr = vb2_dma_contig_plane_dma_addr(src_mb->b, 0);
 	src_c_addr = vb2_dma_contig_plane_dma_addr(src_mb->b, 1);
 
-	mfc_debug(2, "enc src y addr: 0x%08lx", src_y_addr);
-	mfc_debug(2, "enc src c addr: 0x%08lx", src_c_addr);
+	mfc_debug(2, "enc src y addr: 0x%08lx\n", src_y_addr);
+	mfc_debug(2, "enc src c addr: 0x%08lx\n", src_c_addr);
 
 	s5p_mfc_set_enc_frame_buffer_v6(ctx, src_y_addr, src_c_addr);

drivers/media/platform/s5p-mfc/s5p_mfc_pm.c +2 -2

index 6aa38a5..95e7f6d 100644
@@ -98,7 +98,7 @@ int s5p_mfc_clock_on(void)
 	int ret;
 #ifdef CLK_DEBUG
 	atomic_inc(&clk_ref);
-	mfc_debug(3, "+ %d", atomic_read(&clk_ref));
+	mfc_debug(3, "+ %d\n", atomic_read(&clk_ref));
 #endif
 	ret = clk_enable(pm->clock_gate);
 	return ret;

@@ -108,7 +108,7 @@ void s5p_mfc_clock_off(void)
 {
 #ifdef CLK_DEBUG
 	atomic_dec(&clk_ref);
-	mfc_debug(3, "- %d", atomic_read(&clk_ref));
+	mfc_debug(3, "- %d\n", atomic_read(&clk_ref));
 #endif
 	clk_disable(pm->clock_gate);
 }

Signature · 1 line

1.8.1.2

Sachin Kamat

2013-05-28 08:42 UTC #6 lore raw

Re: [PATCH 3/3] s5p-mfc: added missing end-of-lines in debug messages

Hi Andrzej,

Quoted 4 lines · Andrzej Hajda

Many debug messages missed end-of-line.
Signed-off-by Andrzej Hajda <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>

Quoted 8 lines

---
 drivers/media/platform/s5p-mfc/s5p_mfc.c        |  2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_debug.h  |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    | 16 ++++++++--------
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 ++++++++--------
 drivers/media/platform/s5p-mfc/s5p_mfc_pm.c     |  4 ++--
 6 files changed, 23 insertions(+), 23 deletions(-)

Instead of changing in so many places, can't we add it in the macro itself, something like this?

 #define mfc_debug(level, fmt, args...)                         \
        do {                                                    \
                if (debug >= level)                             \
-                       printk(KERN_DEBUG "%s:%d: " fmt,        \
+                       printk(KERN_DEBUG "%s:%d: " fmt "\n",   \
                                __func__, __LINE__, ##args);    \
        } while (0)

Signature · 2 lines

With warm regards,
Sachin

Andrzej Hajda

author

2013-05-28 09:38 UTC #7 lore raw

Re: [PATCH 3/3] s5p-mfc: added missing end-of-lines in debug messages

Hi Sachin,

Thanks for comment.

Quoted 20 lines · Sachin Kamat

Hi Andrzej,
Quoted 4 lines · Andrzej Hajda
Many debug messages missed end-of-line.
Signed-off-by Andrzej Hajda <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
Quoted 8 lines
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c        |  2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_debug.h  |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    | 16 ++++++++--------
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 ++++++++--------
 drivers/media/platform/s5p-mfc/s5p_mfc_pm.c     |  4 ++--
 6 files changed, 23 insertions(+), 23 deletions(-)
Instead of changing in so many places, can't we add it in the macro itself, something like this?
 #define mfc_debug(level, fmt, args...)                         \
        do {                                                    \
                if (debug >= level)                             \

Review

Quoted 4 lines

(quoted diff) +1 -1

-                       printk(KERN_DEBUG "%s:%d: " fmt,        \
+                       printk(KERN_DEBUG "%s:%d: " fmt "\n",   \
                                __func__, __LINE__, ##args);    \
        } while (0)

Enforcing EOL in mfc_debug will result in removing EOL from above 120 places where it is currently used :) Also similar change probably should be made with
mfc_err to make it consistent.
Anyway such change seems to be not consistent with other printk related functions.

Regards
Andrzej