On 06/08/2024 13:57, Aakarsh Jain wrote:
> There is a possibility of getting page fault if the overall
> buffer size is not aligned to 256bytes. Since MFC does read
> operation only and it won't corrupt the data values even if
> it reads the extra bytes.
> Corrected luma and chroma plane sizes for V4L2_PIX_FMT_NV12M
> and V4L2_PIX_FMT_NV21M pixel format.
>
> Signed-off-by: Aakarsh Jain <aakarsh.jain@xxxxxxxxxxx>
> ---
> .../media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> index 73f7af674c01..03c957221fc4 100644
> --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -498,8 +498,8 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct s5p_mfc_ctx *ctx)
> case V4L2_PIX_FMT_NV21M:
> ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
> ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
> - ctx->luma_size = calc_plane(ctx->stride[0], ctx->img_height);
> - ctx->chroma_size = calc_plane(ctx->stride[1], (ctx->img_height / 2));
> + ctx->luma_size = calc_plane(ctx->img_width, ctx->img_height);
> + ctx->chroma_size = calc_plane(ctx->img_width, (ctx->img_height >> 1));
I don't really understand why this is changed. Looking at the implementation of
calc_plane and the various #define values that are used here and in calc_plane,
the number should be the same.
I think the original code makes more sense.
If I missed something, let me know.
> break;
> case V4L2_PIX_FMT_YUV420M:
> case V4L2_PIX_FMT_YVU420M:
> @@ -539,9 +539,11 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct s5p_mfc_ctx *ctx)
> static void s5p_mfc_enc_calc_src_size_v6(struct s5p_mfc_ctx *ctx)
> {
> unsigned int mb_width, mb_height;
> + unsigned int default_size;
>
> mb_width = MB_WIDTH(ctx->img_width);
> mb_height = MB_HEIGHT(ctx->img_height);
> + default_size = (mb_width * mb_height) * 256;
>
> if (IS_MFCV12(ctx->dev)) {
> switch (ctx->src_fmt->fourcc) {
> @@ -549,8 +551,8 @@ static void s5p_mfc_enc_calc_src_size_v6(struct s5p_mfc_ctx *ctx)
> case V4L2_PIX_FMT_NV21M:
> ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
> ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
> - ctx->luma_size = ctx->stride[0] * ALIGN(ctx->img_height, 16);
> - ctx->chroma_size = ctx->stride[0] * ALIGN(ctx->img_height / 2, 16);
> + ctx->luma_size = ALIGN(default_size, 256);
> + ctx->chroma_size = ALIGN(default_size / 2, 256);
Isn't this effectively the same as doing:
ctx->luma_size = ALIGN(ctx->luma_size, 256);
ctx->chroma_size = ALIGN(ctx->chroma_size, 256);
I.e., the bug is that these sizes are not rounded up to a multiple of 256,
so just add that, rather than changing code elsewhere.
I might be wrong, but this seems a much simpler solution.
Regards,
Hans
> break;
> case V4L2_PIX_FMT_YUV420M:
> case V4L2_PIX_FMT_YVU420M:
