linux-kernel - RE: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <047401dae8c0$65352ec0$2f9f8c40$@samsung.com>
Date: Wed, 7 Aug 2024 17:23:15 +0530
From: "Aakarsh Jain" <aakarsh.jain@...sung.com>
To: "'Nicolas Dufresne'" <nicolas@...fresne.ca>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-media@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Cc: <m.szyprowski@...sung.com>, <andrzej.hajda@...el.com>,
	<mchehab@...nel.org>, <hverkuil-cisco@...all.nl>,
	<krzysztof.kozlowski+dt@...aro.org>, <linux-samsung-soc@...r.kernel.org>,
	<gost.dev@...sung.com>, <aswani.reddy@...sung.com>,
	<pankaj.dubey@...sung.com>
Subject: RE: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

Hi Nocolas,

> -----Original Message-----
> From: Nicolas Dufresne <nicolas@...fresne.ca>
> Sent: 06 August 2024 20:08
> To: Aakarsh Jain <aakarsh.jain@...sung.com>; linux-arm-
> kernel@...ts.infradead.org; linux-media@...r.kernel.org; linux-
> kernel@...r.kernel.org
> Cc: m.szyprowski@...sung.com; andrzej.hajda@...el.com;
> mchehab@...nel.org; hverkuil-cisco@...all.nl;
> krzysztof.kozlowski+dt@...aro.org; linux-samsung-soc@...r.kernel.org;
> gost.dev@...sung.com; aswani.reddy@...sung.com;
> pankaj.dubey@...sung.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 <aakarsh.jain@...sung.com>
> > ---
> >  .../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:


View attachment "v4l2-compliance.txt" of type "text/plain" (11186 bytes)