linux-kernel - Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder 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: <113675da-6b85-ef2f-03a9-84e5cb93c31b@gmail.com>
Date:   Tue, 26 Sep 2017 15:02:44 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Stephen Warren <swarren@...dotorg.org>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>, linux-tegra@...r.kernel.org,
        devel@...verdev.osuosl.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder
 driver

On 26.09.2017 08:11, Stephen Warren wrote:
> On 09/25/2017 05:45 PM, Dmitry Osipenko wrote:
>> On 26.09.2017 02:01, Stephen Warren wrote:
>>> On 09/25/2017 04:15 PM, Dmitry Osipenko wrote:
>>>> Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of
>>>> video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports
>>>> decoding of CAVLC H.264 only.
>>>
>>> Note: I don't know anything much about video decoding on Tegra (just NV desktop
>>> GPUs, and that was a while ago), but I had a couple small comments on the DT
>>> binding:
>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt
>>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt
>>>
>>>> +NVIDIA Tegra Video Decoder Engine
>>>> +
>>>> +Required properties:
>>>> +- compatible : "nvidia,tegra20-vde"
>>>> +- reg : Must contain 2 register ranges: registers and IRAM area.
>>>> +- reg-names : Must include the following entries:
>>>> +  - regs
>>>> +  - iram
>>>
>>> I think the IRAM region needs more explanation: What is the region used for and
>>> by what? Can it be moved, and if so does the move need to be co-ordinated with
>>> any other piece of SW?
>>
>> IRAM region is used by Video Decoder HW for internal use and some of decoding
>> parameters are supplied via IRAM, like frames order list. AFAIK IRAM addresses
>> are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure.
>> Should it be explained in the binding?
> 
> I think this should be briefly mentioned, yes. Otherwise at least people
> who don't know the VDE HW well (like me) will wonder why on earth VDE
> interacts with IRAM at all. I would have assumed all parameters were
> supplied via registers or via descriptors in DRAM.
> 
> Thanks.
> 

I also forgot to mention that VDE scrubs that IRAM region on HW reset. So yeah,
it's definitely a part of HW definition. I'll add a brief explanation to the
binding.

-- 
Dmitry