[PATCH] [media] s5p-mfc: Align stream buffer and CPB buffer to 512
From: Andrzej Hajda <hidden>
Date: 2017-01-18 14:38:58
Also in:
linux-media, lkml
Hi Smitha, On 18.01.2017 10:37, Smitha T Murthy wrote:
quoted
From MFCv6 onwards encoder stream buffer and decoder CPB buffer
Unexpected char at the beginning.
need to be aligned with 512.
Patch below adds checks only if buffer size is multiple of 512, am I right? If yes, please precise the subject, for example "...CPB buffer size need to be...".
quoted hunk ↗ jump to hunk
Signed-off-by: Smitha T Murthy <redacted> --- drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 9 +++++++++ drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h | 3 +++ 2 files changed, 12 insertions(+), 0 deletions(-)diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c index d6f207e..57da798 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c@@ -408,8 +408,15 @@ static int s5p_mfc_set_dec_stream_buffer_v6(struct s5p_mfc_ctx *ctx, struct s5p_mfc_dev *dev = ctx->dev; const struct s5p_mfc_regs *mfc_regs = dev->mfc_regs; struct s5p_mfc_buf_size *buf_size = dev->variant->buf_size; + size_t cpb_buf_size; mfc_debug_enter(); + cpb_buf_size = ALIGN(buf_size->cpb, CPB_ALIGN);
Since buf_size->cpb is constant of know size there is no need to align it here.
+ if (strm_size >= set_strm_size_max(cpb_buf_size)) {
+ mfc_debug(2, "Decrease strm_size : %u -> %zu, gap : %d\n",
+ strm_size, set_strm_size_max(cpb_buf_size), CPB_ALIGN);
+ strm_size = set_strm_size_max(cpb_buf_size);
+ }As I understand strm_size here is a size of buffer to be decoded, why it cannot be equal to buf_size->cpb? Commit message says nothing about it.
quoted hunk ↗ jump to hunk
mfc_debug(2, "inst_no: %d, buf_addr: 0x%08x,\n" "buf_size: 0x%08x (%d)\n", ctx->inst_no, buf_addr, strm_size, strm_size);@@ -519,6 +526,8 @@ static int s5p_mfc_set_enc_stream_buffer_v6(struct s5p_mfc_ctx *ctx, struct s5p_mfc_dev *dev = ctx->dev; const struct s5p_mfc_regs *mfc_regs = dev->mfc_regs; + size = ALIGN(size, 512); +
Shouldn't be CPB_ALIGN instead of 512? And more importantly size is a length of buffer for encoded stream, by up-aligning you tell MFC that it can write beyond the buffer, it could potentially overwrite random memory? Am I right?
quoted hunk ↗ jump to hunk
writel(addr, mfc_regs->e_stream_buffer_addr); /* 16B align */ writel(size, mfc_regs->e_stream_buffer_size);diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h index 8055848..16a7b1d 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h@@ -40,6 +40,9 @@ #define FRAME_DELTA_H264_H263 1 #define TIGHT_CBR_MAX 10 +#define CPB_ALIGN 512 +#define set_strm_size_max(cpb_max) ((cpb_max) - CPB_ALIGN)
Name of the macro is misleading. Regards Andrzej
+ struct s5p_mfc_hw_ops *s5p_mfc_init_hw_ops_v6(void); const struct s5p_mfc_regs *s5p_mfc_init_regs_v6_plus(struct s5p_mfc_dev *dev); #endif /* S5P_MFC_OPR_V6_H_ */