Subject: Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder
 driver
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Thierry Reding <thierry.reding@gmail.com>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>, linux-tegra@vger.kernel.org,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
References: <cover.1506377430.git.digetx@gmail.com>
 <5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>
 <20170927094557.yr2fl5arodjnh637@mwanda>
 <2b77bfa7-1272-3d11-9190-326fa0f85f22@gmail.com>
 <20170928072308.s7npizjfnhkxh6zb@mwanda>
From: Dmitry Osipenko <digetx@gmail.com>
Message-ID: <69a019f1-8ce5-6b8b-8eb9-2ce9b8c2759d@gmail.com>
Date: Thu, 28 Sep 2017 14:37:03 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <20170928072308.s7npizjfnhkxh6zb@mwanda>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1965
Lines: 54

On 28.09.2017 10:23, Dan Carpenter wrote:
> On Thu, Sep 28, 2017 at 02:28:04AM +0300, Dmitry Osipenko wrote:
>>>> +	if (is_baseline_profile)
>>>> +		frame->aux_paddr = 0xF4DEAD00;
>>>
>>> The handling of is_baseline_profile is strange to me.  It feels like we
>>> should always check it before we use ->aux_paddr but we don't ever do
>>> that.
>>>
>>
>> In a case of baseline profile, aux buffer isn't needed, HW should't use it. Aux
>> phys address is set to a predefined and invalid address, so that in a case of
>> VDE trying to use it, its invalid memory accesses would be reported in KMSG by
>> memory controller driver and the reported invalid addresses would be known to be
>> associated with the aux buffer. I'm not sure what you are meaning.
> 
> It's not used perhaps, but we do write it to the hardware, right?
> 

That's right. I'm pretty sure HW won't use the aux in a case of baseline
profile, haven't seen an evidence of it. But in order to be on a safe side, the
addresses are initialized to an invalid value, so HW won't have a chance to
silently fetch/trash 'arbitrary' main memory and we will know if it tries to do it.

	if (baseline_profile)
		frame->aux_paddr = 0xF4DEAD00;
	else {

So here ^ all *used* frame entries are being initialized.

> 	tegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr);
> 
> It's just strange.
> 

There up to 16 reference video frames (already decoded) that could be used for
decoding of a video frame. Addresses of reference frames that shouldn't be used
for decoding of the current frame are set to an invalid address. Userspace my
supply wrong frames list or frames list setup code may contain an obscure bug
and we will know about it.

	} else {
		aux_paddr = 0xFADEAD00;
		value = 0;
	}

Here ^ all *unused* frame entries are being initialized.

	tegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr);

And here ^ these entries are written to the tables that are read by HW.

-- 
Dmitry