linux-kernel - Re: [PATCH 3/4] drm/tegra: Add VIC support

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:	Mon, 25 May 2015 10:11:28 +0300
From:	Arto Merilainen <amerilainen@...dia.com>
To:	Thierry Reding <thierry.reding@...il.com>,
	Mikko Perttunen <mperttunen@...dia.com>
CC:	<linux-tegra@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
	<linux-kernel@...r.kernel.org>, <achew@...dia.com>,
	<srasal@...dia.com>, <dnibade@...dia.com>
Subject: Re: [PATCH 3/4] drm/tegra: Add VIC support

On 05/22/2015 01:25 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, May 21, 2015 at 05:40:31PM +0300, Mikko Perttunen wrote:
>> On 05/21/2015 04:20 PM, Arto Merilainen wrote:
> [...]
>>> +static int vic_is_addr_reg(struct device *dev, u32 class, u32 offset, u32 val)
>>> +{
>>> +	struct vic *vic = dev_get_drvdata(dev);
>>> +
>>> +	/* handle host class */
>>> +	if (class == HOST1X_CLASS_HOST1X) {
>>> +		if (offset == 0x2b)
>>> +			return true;
>>> +		return false;
>>
>> "return (offset == 0x2b);" perhaps?
>
> I think this should really be extracted into a separate helper. If we
> ever need to take into account additional offsets we would otherwise
> have to extend every driver rather than just the helper.

I agree, that would be better.

>
> Also I think the 0x2b should be replaced by some symbolic name.
> According to the TRM 0x2b is the host1x class method named
> NV_CLASS_HOST_INDCTRL_0. Oddly enough that doesn't seem to be an address
> register. Instead the address seems to be in the INDOFF2 and INDOFF
> methods (0x2c and 0x2d). I also can't tell from the TRM what exactly
> these are supposed to do.
>
> Arto, can you clarify?

This looks like an unfortunate mistake that got reproduced from gr2d and 
gr3d.

The INDCTRL method is used for indirect register accessing and it allows 
Host1x to read registers of an engine - or write data directly to 
memory. It allow implementing context switch for the clients whose state 
should be not change between jobs from the same application.

>
>>> +	if (IS_ERR(vic->rst)) {
>>> +		dev_err(&pdev->dev, "cannot get reset\n");
>>> +		return PTR_ERR(vic->rst);
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, vic);
>>> +
>>> +	INIT_LIST_HEAD(&vic->client.base.list);
>>> +	vic->client.base.ops = &vic_client_ops;
>>> +	vic->client.base.dev = dev;
>>> +	vic->client.base.class = vic_config->class_id;
>>> +	vic->client.base.syncpts = syncpts;
>>> +	vic->client.base.num_syncpts = 1;
>>> +	vic->dev = dev;
>>> +	vic->config = vic_config;
>>> +
>>> +	INIT_LIST_HEAD(&vic->client.list);
>>> +	vic->client.ops = &vic_ops;
>>> +
>>> +	err = tegra_powergate_sequence_power_up(vic->config->powergate_id,
>>> +						vic->clk, vic->rst);
>>> +	if (err) {
>>> +		dev_err(dev, "cannot turn on the device\n");
>>> +		return err;
>>> +	}
>>> +
>>> +	err = host1x_client_register(&vic->client.base);
>>> +	if (err < 0) {
>>
>> You used 'if (err) {' previously, so maybe also here.
>
> For consistency with other Tegra DRM code these checks should use (at
> least where possible) the (err < 0) notation.
>

Will fix.

- Arto
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/