linux-kernel - Re: [PATCH v5 00/39] i.MX Media Driver

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: <faf90d92-63d8-e5b6-df0f-de0c5a44de33@mentor.com>
Date:   Mon, 13 Mar 2017 16:37:06 -0700
From:   Steve Longerbeam <steve_longerbeam@...tor.com>
To:     Russell King - ARM Linux <linux@...linux.org.uk>,
        Steve Longerbeam <slongerbeam@...il.com>
CC:     <robh+dt@...nel.org>, <mark.rutland@....com>,
        <shawnguo@...nel.org>, <kernel@...gutronix.de>,
        <fabio.estevam@....com>, <mchehab@...nel.org>,
        <hverkuil@...all.nl>, <nick@...anahar.org>,
        <markus.heiser@...marIT.de>, <p.zabel@...gutronix.de>,
        <laurent.pinchart+renesas@...asonboard.com>, <bparrot@...com>,
        <geert@...ux-m68k.org>, <arnd@...db.de>,
        <sudipm.mukherjee@...il.com>, <minghsiu.tsai@...iatek.com>,
        <tiffany.lin@...iatek.com>, <jean-christophe.trotin@...com>,
        <horms+renesas@...ge.net.au>,
        <niklas.soderlund+renesas@...natech.se>, <robert.jarzmik@...e.fr>,
        <songjun.wu@...rochip.com>, <andrew-ct.chen@...iatek.com>,
        <gregkh@...uxfoundation.org>, <shuah@...nel.org>,
        <sakari.ailus@...ux.intel.com>, <pavel@....cz>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-media@...r.kernel.org>, <devel@...verdev.osuosl.org>
Subject: Re: [PATCH v5 00/39] i.MX Media Driver



On 03/13/2017 01:16 AM, Russell King - ARM Linux wrote:
> On Sun, Mar 12, 2017 at 09:26:41PM -0700, Steve Longerbeam wrote:
>> On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:
>>> What I had was this patch for your v3.  I never got to testing your
>>> v4 because of the LP-11 problem.
>>>
>>> In v5, you've changed to propagate the ipu_cpmem_set_image() error
>>> code to avoid the resulting corruption, but that leaves the other bits
>>> of this patch unaddressed, along my "media: imx: smfc: add support
>>> for bayer formats" patch.
>>>
>>> Your driver basically has no support for bayer formats.
>> You added the patches to this driver that adds the bayer support,
>> I don't think there is anything more required of the driver at this
>> point to support bayer, the remaining work needs to happen in the IPUv3
>> driver.
> There is more work, because the way you've merged my changes to
> imx_smfc_setup_channel() into csi_idmac_setup_channel() is wrong with
> respect to the burst size.
>
> You always set it to 8 or 16 depending on the width:
>
> 	burst_size = (image.pix.width & 0xf) ? 8 : 16;
>
> 	ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);
>
> and then you have my switch() statement which assigns burst_size.
> My _tested_ code removed the above, added the switch, which had
> a default case which reflected the above setting:
>
> 	default:
> 		burst_size = (outfmt->width & 0xf) ? 8 : 16;
>
> and then went on to set the burst size _after_ the switch statement:
>
> 	ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size);
>
> The effect is unchanged for non-bayer formats.  For bayer formats, the
> burst size is determined by the bayer data size.
>
> So, even if it's appropriate to fix ipu_cpmem_set_image(), fixing the
> above is still required.

Oops, sorry missed that. I'll fix.

>
> I'm not convinced that fixing ipu_cpmem_set_image() is even the best
> solution - it's not as trivial as it looks on the surface:
>
>          ipu_cpmem_set_resolution(ch, image->rect.width, image->rect.height);
>          ipu_cpmem_set_stride(ch, pix->bytesperline);
>
> this is fine, it doesn't depend on the format.  However, the next line:
>
>          ipu_cpmem_set_fmt(ch, v4l2_pix_fmt_to_drm_fourcc(pix->pixelformat));
>
> does - v4l2_pix_fmt_to_drm_fourcc() is a locally defined function (it
> isn't v4l2 code) that converts a v4l2 pixel format to a DRM fourcc.
> DRM knows nothing about bayer formats, there aren't fourcc codes in
> DRM for it.

right, yeah that's a problem.

>    The result is that v4l2_pix_fmt_to_drm_fourcc() returns
> -EINVAL cast to a u32, which gets passed unchecked into ipu_cpmem_set_fmt().

Ugh.

>
> ipu_cpmem_set_fmt() won't recognise that, and also returns -EINVAL - and
> it's a bug that this is not checked and propagated.  If it is checked and
> propagated, then we need this to support bayer formats, and I don't see
> DRM people wanting bayer format fourcc codes added without there being
> a real DRM driver wanting to use them.

true.

>
> Then there's the business of calculating the top-left offset of the image,
> which for bayer always needs to be an even number of pixels - as this
> function takes the top-left offset, it ought to respect it, but if it
> doesn't meet this criteria, what should it do?  csi_idmac_setup_channel()
> always sets them to zero, but that's not really something that
> ipu_cpmem_set_image() should assume.

Well, I will integrate your patch above. Thanks for doing this
work for me.

We do need to address the issues you brought up in ipu_cpmem at
some point.

Steve