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]

Date:   Tue, 26 Sep 2017 02:45:23 +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 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?

>> +- clocks : Must contain one entry, for the module clock.
>> +  See ../clocks/clock-bindings.txt for details.
>> +- resets : Must contain an entry for each entry in reset-names.
>> +  See ../reset/reset.txt for details.
>> +- reset-names : Must include the following entries:
>> +  - vde
> 
> Let's require a clock-names property too.

Okay, I'll add this property to the binding.

-- 
Dmitry