Hi Nocolas, > -----Original Message----- > From: Nicolas Dufresne > Sent: 06 August 2024 20:08 > To: Aakarsh Jain ; linux-arm- > kernel@lists.infradead.org; linux-media@vger.kernel.org; linux- > kernel@vger.kernel.org > Cc: m.szyprowski@samsung.com; andrzej.hajda@intel.com; > mchehab@kernel.org; hverkuil-cisco@xs4all.nl; > krzysztof.kozlowski+dt@linaro.org; linux-samsung-soc@vger.kernel.org; > gost.dev@samsung.com; aswani.reddy@samsung.com; > pankaj.dubey@samsung.com > Subject: Re: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes > > Hi Jain, > > I haven't dig much, but I have a quick question below. > > Le mardi 06 août 2024 à 17:27 +0530, Aakarsh Jain a écrit : > > 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. > > Have you re-run v4l2 compliance ? (better be safe then sorry). > I ran v4l2-compliance and didn't found any issue wrt to the changes added in this patch. Please find the v4l2-compliance report attached. > > > > Signed-off-by: Aakarsh Jain > > --- > > .../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)); > > These size needs to match the sizes reported through TRY_FMT (and S_FMT) > sizeimage for each planes. Is this code being call withing try_fmt ? Will these > value match or will this change cause the value to miss-match ? > This code is getting called within try_fmt. In MFC driver we are not returning any sizes in TRY_FMT. We are only validating codec and the pixel format. We are setting luma, chroma and stride size in S_FMT to inform user space for further buffer allocation. So, this change is not going to cause any mismatch. > The reason is that correct value is needed for allocating this memory from > the outside (like using a DMAbuf Heap). Perhaps its all right, let me know. > > Nicolas > > > 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); > > break; > > case V4L2_PIX_FMT_YUV420M: > > case V4L2_PIX_FMT_YVU420M: