[PATCH 3/4] drm/tegra: Add VIC support

[Arto Merilainen]

Hi Thierry,

Thank you for your thorough analysis - and sorry for a bunch of very 
silly mistakes.

I am skipping most trivial parts and focus on the trickier comments and 
questions.

On 05/22/2015 02:47 PM, Thierry Reding wrote:
>> +
>> +struct tegra_bo *ucode_bo;
>> +bool ucode_valid;
>> +void *ucode_vaddr;
>> +
>> +bool booted;
>
> There are a bunch of other drivers that use a Falcon and they will all
> need to use similar data to this to deal with the firmware and all. I
> would like to see that code to be made into a Falcon library so that it
> can be reused in a meaningful way.
>
> Roughly this would look like this:
>
>   struct falcon {
>   struct device *dev;
>   ...
>   };
>
>   int falcon_init(struct falcon *falcon, struct device *dev,
>   void __iomem *regs);
>   int falcon_load_firmware(struct falcon *falcon, const char *filename);
>   int falcon_exit(struct falcon *falcon);
>   int falcon_boot(struct falcon *falcon);
>

There are two issues in above scheme..:
- Memory allocation. Despite I have explicitly mentioned that the series 
has been tested only with iommu disabled, I would prefer trying to keep 
an option to use it later. Depending how well we want to isolate the 
falcon library from other parts of tegradrm, the library may not have 
ability to map anything to the tegradrm iommu domain.
- The firmware images may not hold only Falcon firmware. Already in VIC 
case we have two firmwares: One for Falcon, another for FCE.

To overcome the above issues, I would prefer dropping 
falcon_load_firmware() and keeping firmware image specifics inside the 
client driver and giving data related to Falcon as a parameter for 
falcon_boot(). Would this be ok?

>> +
>> +/* for firewall - this determines if method 1 should be regarded
>> + * as an address register */
>> +bool method_data_is_addr_reg;
>> +};
>
> I think it'd be best to incorporate that functionality into the firewall
> so that we can deal with it more centrally rather that duplicate this in
> all drivers.
>

Do you have a concrete suggestion how this should be done? Firewall has 
no access to the driver specifics and in VIC case the VIC methods 
themselves define whether the METHOD1 includes address or not.

>> +static int vic_dma_pa_to_internal_256b(struct vic *vic, phys_addr_t pa,
>> +   u32 internal_offset, bool imem)
>
> The name is confusing. Without looking at the code I'd expect this to
> perform some kind of conversion from a physical address to some internal
> address, but if I understand correctly this actually copies code into
> the Falcon instruction or data memories. A better name I think would be:
>
>   static int falcon_load_chunk(struct falcon *falcon, phys_addr_t phys,
>unsigned long offset, enum falcon_memory 
> target);
>
> Note that this is now part of the Falcon library because I've seen
> identical code used in several other drivers. Also the bool argument is
> now an enumeration, which makes it much easier to read. Compare:
>
>   err = vic_dma_pa_to_internal_256b(vic, phys, offset, true);
>
> and
>
>   err = falcon_load_chunk(>falcon, phys, offset, FALCON_MEMORY_IMEM);
>

Sounds ok.

> Is there a specific term in Falcon-speak for these 256 byte blocks?
> "chunk" is a little awkward.
>

Unfortunately I am not aware that there would be - maybe "block"?

>> +if (ucode.bin_header->bin_ver != 1) {
>> +dev_err(vic->dev, "unsupported firmware version");
>> +return -ENOENT;
>
> Why not -EINVAL here, too?
>

We can interpret the issue two ways..: The given firmware is invalid 
(-EINVAL) or that there was no supported firmware entry available (-ENOENT).

I do not have strong opinions and will change this to -EINVAL.

>> +vic->ucode_bo = tegra_bo_create(dev, ucode_fw->size, 0);
>> +if (IS_ERR(vic->ucode_bo)) {
>> +dev_err(vic->dev, "dma memory allocation failed");
>> +err = PTR_ERR(vic->ucode_bo);
>> +goto err_alloc_iova;
>> +}
>
> Erm... no. Please don't use tegra_bo_create() to allocate these buffers.
> tegra_bo_create() creates GEM objects and firmware doesn't qualify as a
> GEM object.
>
> Can't you use the DMA API here?
>

The firmware must be mapped to the IOMMU domain into which VIC is 
attached - and I would prefer keeping the door open for enabling iommu 
on VIC. This was the simplest way to get a buffer that is allocated to 
the tegradrm iommu domain.

Should I add a function for allocating memory without making a gem 
object or should I keep memory allocation here and simply add functions 
for mapping it into tegradrm domain?

>> +static int vic_boot(struct device *dev)
>> +{
>> +struct vic *vic = dev_get_drvdata(dev);
>> +u32 offset;
>> +int err = 0;
>> +
>> +if (vic->booted)
>> +return 0;
>> +
>> +if (!vic->ucode_valid) {
>> +

[PATCH 3/4] drm/tegra: Add VIC support

[Arto Merilainen]

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(>dev, "cannot get reset\n");
>>> +   return PTR_ERR(vic->rst);
>>> +   }
>>> +
>>> +   platform_set_drvdata(pdev, vic);
>>> +
>>> +   INIT_LIST_HEAD(>client.base.list);
>>> +   vic->client.base.ops = _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(>client.list);
>>> +   vic->client.ops = _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(>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

[PATCH 3/4] drm/tegra: Add VIC support

[Thierry Reding]

On Thu, May 21, 2015 at 04:20:24PM +0300, Arto Merilainen wrote:
> This patch adds support for Video Image Compositor engine which
> can be used for 2d operations.
> 
> The engine has a microcontroller (Falcon) that acts as a frontend
> for the rest of the unit. In order to properly utilize the engine,
> the frontend must be booted before pushing any commands.
> 
> Signed-off-by: Andrew Chew 
> Signed-off-by: Arto Merilainen 
> ---
>  drivers/gpu/drm/tegra/Makefile |   3 +-
>  drivers/gpu/drm/tegra/drm.c|   9 +-
>  drivers/gpu/drm/tegra/drm.h|   1 +
>  drivers/gpu/drm/tegra/vic.c| 593 
> +
>  drivers/gpu/drm/tegra/vic.h| 116 
>  include/linux/host1x.h |   1 +
>  6 files changed, 721 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/tegra/vic.c
>  create mode 100644 drivers/gpu/drm/tegra/vic.h

I just reviewed, thoroughly, similar patches for ChromeOS. Unfortunately
these comments aren't publicly available, so I guess I'll have to
reproduce them here...

> diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
[...]
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

This include is misplaced. It should go into a separate section, that is
separated from the  includes above and the "*.h" includes
below by a blank line.

> +#include 
> +
> +#include "drm.h"
> +#include "gem.h"
> +#include "vic.h"
> +
> +#define VIC_IDLE_TIMEOUT_DEFAULT 1   /* 10 milliseconds */

The _DEFAULT suffix here isn't very useful since you never override the
default. So this is really just the timeout value.

As was mentioned in the ChromeOS review this should also have a _US
suffix to indicate the unit.

> +#define VIC_IDLE_CHECK_PERIOD10  /* 10 usec */

_US

> +struct vic;
> +
> +struct vic_config {
> + /* firmware name */
> + char *ucode_name;

const?

> + /* class id */
> + u32 class_id;
> +
> + /* powergate id */
> + int powergate_id;

I don't think it's very likely that these will ever change, so please
drop them.

> +struct vic {
> + struct {
> + u32 bin_data_offset;
> + u32 data_offset;
> + u32 data_size;
> + u32 code_offset;
> + u32 size;
> + } os, fce;

This is unintuitive. At least these should be commented, but it looks
like these binary firmware images have code and data sections (and the
code to upload to either instruction or data memory corroborates that)
so perhaps it'd be better to structure this more explicitly and hence
do away with the prefixes. Also the types should be non-sized since
these don't represent direct register values nor fields in a binary
file. Perhaps something like:

struct falcon_firmware_section {
unsigned long offset;
size_t size;
};

struct falcon_firmware {
struct falcon_firmware_section code;
struct falcon_firmware_section data;

dma_addr_t phys;
void *virt;
size_t size;
};

> +
> + struct tegra_bo *ucode_bo;
> + bool ucode_valid;
> + void *ucode_vaddr;
> +
> + bool booted;

There are a bunch of other drivers that use a Falcon and they will all
need to use similar data to this to deal with the firmware and all. I
would like to see that code to be made into a Falcon library so that it
can be reused in a meaningful way.

Roughly this would look like this:

struct falcon {
struct device *dev;
...
};

int falcon_init(struct falcon *falcon, struct device *dev,
void __iomem *regs);
int falcon_load_firmware(struct falcon *falcon, const char *filename);
int falcon_exit(struct falcon *falcon);
int falcon_boot(struct falcon *falcon);

etc. Drivers that need to boot a Falcon would then use it somewhat like
this:

struct vic_soc {
const char *firmware;
};

struct vic {
struct falcon *falcon;
void __iomem *regs;
...
};

static int vic_init(struct host1x_client *client)
{
struct vic *vic;
...
err = falcon_boot(>falcon);
...
}

static int vic_probe(struct platform_device *pdev)
{
struct falcon_firmware *firmware;
const struct vic_soc *soc;
struct vic *vic;
...
soc = (const struct vic_soc *)match->data;
...
err = falcon_init(>falcon, >dev,
  vic->regs + VIC_FALCON_OFFSET);
..
err = falcon_load_firmware(>dev, soc->firmware);
...
}

> + void __iomem 

[PATCH 3/4] drm/tegra: Add VIC support

[Arto Merilainen]

Hi Thierry,

On 05/22/2015 01:02 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, May 21, 2015 at 06:44:08PM +0300, Mikko Perttunen wrote:
>> On 05/21/2015 06:10 PM, Arto Merilainen wrote:
>>> ...
> +
> +vic->rst = devm_reset_control_get(dev, "vic03");

 I might prefer just "vic" as the clock/reset name. The name is often
 used as a sort of "role" for the clock/reset for the device, not
 necessarily the raw name of the "correct" clock/reset.

>>>
>>> I considered that - but I then noticed that
>>> drivers/clk/tegra/clk-tegra124.c was already using vic03 variant. I can
>>> write a patch for changing that too.
>>
>> Well, the two can be different; the clock-name in device tree kind of means
>> "string that i use to refer to a clock that powers the VIC unit". It's not
>> really a big deal though, both ways are used in DT bindings.
>
> I'll insist on calling this vic in the clock-names property. The 03 is
> as far as I can tell an encoding of the version number, so if you want
> to call this vic04 in some future version we'll have to needlessly
> patch the driver.

I agree, this is better without 04 postfix and I will remove it in the 
next version.

- Arto

[PATCH 3/4] drm/tegra: Add VIC support

[Arto Merilainen]

Hi Thierry,

On 05/22/2015 01:02 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, May 21, 2015 at 06:44:08PM +0300, Mikko Perttunen wrote:
>> On 05/21/2015 06:10 PM, Arto Merilainen wrote:
>>> ...
> +
> +vic->rst = devm_reset_control_get(dev, "vic03");

 I might prefer just "vic" as the clock/reset name. The name is often
 used as a sort of "role" for the clock/reset for the device, not
 necessarily the raw name of the "correct" clock/reset.

>>>
>>> I considered that - but I then noticed that
>>> drivers/clk/tegra/clk-tegra124.c was already using vic03 variant. I can
>>> write a patch for changing that too.
>>
>> Well, the two can be different; the clock-name in device tree kind of means
>> "string that i use to refer to a clock that powers the VIC unit". It's not
>> really a big deal though, both ways are used in DT bindings.
>
> I'll insist on calling this vic in the clock-names property. The 03 is
> as far as I can tell an encoding of the version number, so if you want
> to call this vic04 in some future version we'll have to needlessly
> patch the driver.

I agree, this is better without 03 postfix will fix this in the next 
version.

- Arto

[PATCH 3/4] drm/tegra: Add VIC support

[Thierry Reding]

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.

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?

> > +   if (IS_ERR(vic->rst)) {
> > +   dev_err(>dev, "cannot get reset\n");
> > +   return PTR_ERR(vic->rst);
> > +   }
> > +
> > +   platform_set_drvdata(pdev, vic);
> > +
> > +   INIT_LIST_HEAD(>client.base.list);
> > +   vic->client.base.ops = _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(>client.list);
> > +   vic->client.ops = _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(>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.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[PATCH 3/4] drm/tegra: Add VIC support

[Thierry Reding]

On Thu, May 21, 2015 at 06:44:08PM +0300, Mikko Perttunen wrote:
> On 05/21/2015 06:10 PM, Arto Merilainen wrote:
> >...
> >>>+
> >>>+vic->rst = devm_reset_control_get(dev, "vic03");
> >>
> >>I might prefer just "vic" as the clock/reset name. The name is often
> >>used as a sort of "role" for the clock/reset for the device, not
> >>necessarily the raw name of the "correct" clock/reset.
> >>
> >
> >I considered that - but I then noticed that
> >drivers/clk/tegra/clk-tegra124.c was already using vic03 variant. I can
> >write a patch for changing that too.
> 
> Well, the two can be different; the clock-name in device tree kind of means
> "string that i use to refer to a clock that powers the VIC unit". It's not
> really a big deal though, both ways are used in DT bindings.

I'll insist on calling this vic in the clock-names property. The 03 is
as far as I can tell an encoding of the version number, so if you want
to call this vic04 in some future version we'll have to needlessly
patch the driver.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[PATCH 3/4] drm/tegra: Add VIC support

[Mikko Perttunen]

On 05/21/2015 06:10 PM, Arto Merilainen wrote:
> ...
>>> +
>>> +vic->rst = devm_reset_control_get(dev, "vic03");
>>
>> I might prefer just "vic" as the clock/reset name. The name is often
>> used as a sort of "role" for the clock/reset for the device, not
>> necessarily the raw name of the "correct" clock/reset.
>>
>
> I considered that - but I then noticed that
> drivers/clk/tegra/clk-tegra124.c was already using vic03 variant. I can
> write a patch for changing that too.

Well, the two can be different; the clock-name in device tree kind of 
means "string that i use to refer to a clock that powers the VIC unit". 
It's not really a big deal though, both ways are used in DT bindings.

Mikko

[PATCH 3/4] drm/tegra: Add VIC support

[Arto Merilainen]

Thank you Mikko for your comments!

Please see my answers inline.

- Arto

On 05/21/2015 05:40 PM, Mikko Perttunen wrote:
> Hi, very good patch!
>
> Here are a few small comments. Aside those, you should also add a
> section to
> Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt in a
> separate patch.

Will do.

> On 05/21/2015 04:20 PM, Arto Merilainen wrote:
>> This patch adds support for Video Image Compositor engine which
>> can be used for 2d operations.
>>
>> The engine has a microcontroller (Falcon) that acts as a frontend
>> for the rest of the unit. In order to properly utilize the engine,
>> the frontend must be booted before pushing any commands.
>>
>> Signed-off-by: Andrew Chew 
>> Signed-off-by: Arto Merilainen 
>> ---
>>   drivers/gpu/drm/tegra/Makefile |   3 +-
>>   drivers/gpu/drm/tegra/drm.c|   9 +-
>>   drivers/gpu/drm/tegra/drm.h|   1 +
>>   drivers/gpu/drm/tegra/vic.c| 593 
>> +
>>   drivers/gpu/drm/tegra/vic.h| 116 
>>   include/linux/host1x.h |   1 +
>>   6 files changed, 721 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/gpu/drm/tegra/vic.c
>>   create mode 100644 drivers/gpu/drm/tegra/vic.h
>>
>> diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile
>> index 2c66a8db9da4..3bc3566e00b6 100644
>> --- a/drivers/gpu/drm/tegra/Makefile
>> +++ b/drivers/gpu/drm/tegra/Makefile
>> @@ -13,6 +13,7 @@ tegra-drm-y := \
>>  sor.o \
>>  dpaux.o \
>>  gr2d.o \
>> -gr3d.o
>> +gr3d.o \
>> +vic.o
>>
>>   obj-$(CONFIG_DRM_TEGRA) += tegra-drm.o
>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>> index bfad15a913a0..d947f5f4d801 100644
>> --- a/drivers/gpu/drm/tegra/drm.c
>> +++ b/drivers/gpu/drm/tegra/drm.c
>> @@ -1,6 +1,6 @@
>>   /*
>>* Copyright (C) 2012 Avionic Design GmbH
>> - * Copyright (C) 2012-2013 NVIDIA CORPORATION.  All rights reserved.
>> + * Copyright (C) 2012-2015 NVIDIA CORPORATION.  All rights reserved.
>>*
>>* This program is free software; you can redistribute it and/or modify
>>* it under the terms of the GNU General Public License version 2 as
>> @@ -1048,6 +1048,7 @@ static const struct of_device_id host1x_drm_subdevs[] 
>> = {
>>  { .compatible = "nvidia,tegra124-dc", },
>>  { .compatible = "nvidia,tegra124-sor", },
>>  { .compatible = "nvidia,tegra124-hdmi", },
>> +{ .compatible = "nvidia,tegra124-vic", },
>>  { /* sentinel */ }
>>   };
>>
>> @@ -1097,8 +1098,14 @@ static int __init host1x_drm_init(void)
>>  if (err < 0)
>>  goto unregister_gr2d;
>>
>> +err = platform_driver_register(_vic_driver);
>> +if (err < 0)
>> +goto unregister_gr3d;
>> +
>>  return 0;
>>
>> +unregister_gr3d:
>> +platform_driver_unregister(_gr3d_driver);
>>   unregister_gr2d:
>>  platform_driver_unregister(_gr2d_driver);
>>   unregister_dpaux:
>> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
>> index 0e7756e720c5..a9c02a80d6bf 100644
>> --- a/drivers/gpu/drm/tegra/drm.h
>> +++ b/drivers/gpu/drm/tegra/drm.h
>> @@ -275,5 +275,6 @@ extern struct platform_driver tegra_hdmi_driver;
>>   extern struct platform_driver tegra_dpaux_driver;
>>   extern struct platform_driver tegra_gr2d_driver;
>>   extern struct platform_driver tegra_gr3d_driver;
>> +extern struct platform_driver tegra_vic_driver;
>>
>>   #endif /* HOST1X_DRM_H */
>> diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
>> new file mode 100644
>> index ..b7c5a96697ed
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tegra/vic.c
>> @@ -0,0 +1,593 @@
>> +/*
>> + * Copyright (c) 2015, NVIDIA Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "drm.h"
>> +#include "gem.h"
>> +#include "vic.h"
>> +
>> +#define VIC_IDLE_TIMEOUT_DEFAULT1   /* 10 milliseconds */
>> +#define VIC_IDLE_CHECK_PERIOD   10  /* 10 usec */
>> +
>> +struct vic;
>
> This doesn't seem to be needed here.
>
>> +
>> +struct vic_config {
>> +/* firmware name */
>> +char *ucode_name;
>> +
>> +/* class id */
>> +u32 class_id;
>> +
>> +/* powergate id */
>> +int powergate_id;
>> +};
>> +
>> +struct vic {
>> +struct {
>> +u32 bin_data_offset;
>> +u32 data_offset;
>> +u32 data_size;
>> +u32 code_offset;
>> +u32 size;
>> +} os, fce;
>> +
>> +struct tegra_bo *ucode_bo;
>> +bool ucode_valid;
>> +void *ucode_vaddr;
>> +
>> +bool booted;
>> +
>> +void __iomem *regs;
>> +struct tegra_drm_client client;
>> +struct 

[PATCH 3/4] drm/tegra: Add VIC support

[Mikko Perttunen]

Hi, very good patch!

Here are a few small comments. Aside those, you should also add a
section to
Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt in a
separate patch.

Thanks,
Mikko.

On 05/21/2015 04:20 PM, Arto Merilainen wrote:
> This patch adds support for Video Image Compositor engine which
> can be used for 2d operations.
> 
> The engine has a microcontroller (Falcon) that acts as a frontend
> for the rest of the unit. In order to properly utilize the engine,
> the frontend must be booted before pushing any commands.
> 
> Signed-off-by: Andrew Chew 
> Signed-off-by: Arto Merilainen 
> ---
>  drivers/gpu/drm/tegra/Makefile |   3 +-
>  drivers/gpu/drm/tegra/drm.c|   9 +-
>  drivers/gpu/drm/tegra/drm.h|   1 +
>  drivers/gpu/drm/tegra/vic.c| 593 
> +
>  drivers/gpu/drm/tegra/vic.h| 116 
>  include/linux/host1x.h |   1 +
>  6 files changed, 721 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/tegra/vic.c
>  create mode 100644 drivers/gpu/drm/tegra/vic.h
> 
> diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile
> index 2c66a8db9da4..3bc3566e00b6 100644
> --- a/drivers/gpu/drm/tegra/Makefile
> +++ b/drivers/gpu/drm/tegra/Makefile
> @@ -13,6 +13,7 @@ tegra-drm-y := \
>   sor.o \
>   dpaux.o \
>   gr2d.o \
> - gr3d.o
> + gr3d.o \
> + vic.o
>  
>  obj-$(CONFIG_DRM_TEGRA) += tegra-drm.o
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index bfad15a913a0..d947f5f4d801 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1,6 +1,6 @@
>  /*
>   * Copyright (C) 2012 Avionic Design GmbH
> - * Copyright (C) 2012-2013 NVIDIA CORPORATION.  All rights reserved.
> + * Copyright (C) 2012-2015 NVIDIA CORPORATION.  All rights reserved.
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -1048,6 +1048,7 @@ static const struct of_device_id host1x_drm_subdevs[] = 
> {
>   { .compatible = "nvidia,tegra124-dc", },
>   { .compatible = "nvidia,tegra124-sor", },
>   { .compatible = "nvidia,tegra124-hdmi", },
> + { .compatible = "nvidia,tegra124-vic", },
>   { /* sentinel */ }
>  };
>  
> @@ -1097,8 +1098,14 @@ static int __init host1x_drm_init(void)
>   if (err < 0)
>   goto unregister_gr2d;
>  
> + err = platform_driver_register(_vic_driver);
> + if (err < 0)
> + goto unregister_gr3d;
> +
>   return 0;
>  
> +unregister_gr3d:
> + platform_driver_unregister(_gr3d_driver);
>  unregister_gr2d:
>   platform_driver_unregister(_gr2d_driver);
>  unregister_dpaux:
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 0e7756e720c5..a9c02a80d6bf 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -275,5 +275,6 @@ extern struct platform_driver tegra_hdmi_driver;
>  extern struct platform_driver tegra_dpaux_driver;
>  extern struct platform_driver tegra_gr2d_driver;
>  extern struct platform_driver tegra_gr3d_driver;
> +extern struct platform_driver tegra_vic_driver;
>  
>  #endif /* HOST1X_DRM_H */
> diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
> new file mode 100644
> index ..b7c5a96697ed
> --- /dev/null
> +++ b/drivers/gpu/drm/tegra/vic.c
> @@ -0,0 +1,593 @@
> +/*
> + * Copyright (c) 2015, NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "drm.h"
> +#include "gem.h"
> +#include "vic.h"
> +
> +#define VIC_IDLE_TIMEOUT_DEFAULT 1   /* 10 milliseconds */
> +#define VIC_IDLE_CHECK_PERIOD10  /* 10 usec */
> +
> +struct vic;

This doesn't seem to be needed here.

> +
> +struct vic_config {
> + /* firmware name */
> + char *ucode_name;
> +
> + /* class id */
> + u32 class_id;
> +
> + /* powergate id */
> + int powergate_id;
> +};
> +
> +struct vic {
> + struct {
> + u32 bin_data_offset;
> + u32 data_offset;
> + u32 data_size;
> + u32 code_offset;
> + u32 size;
> + } os, fce;
> +
> + struct tegra_bo *ucode_bo;
> + bool ucode_valid;
> + void *ucode_vaddr;
> +
> + bool booted;
> +
> + void __iomem *regs;
> + struct tegra_drm_client client;
> + struct host1x_channel *channel;
> + struct iommu_domain *domain;
> + struct device *dev;
> + struct clk *clk;
> + struct reset_control *rst;
> +
> + /* Platform configuration */
> + struct vic_config *config;
> +
> + /* for 

[PATCH 3/4] drm/tegra: Add VIC support

[Arto Merilainen]

This patch adds support for Video Image Compositor engine which
can be used for 2d operations.

The engine has a microcontroller (Falcon) that acts as a frontend
for the rest of the unit. In order to properly utilize the engine,
the frontend must be booted before pushing any commands.

Signed-off-by: Andrew Chew 
Signed-off-by: Arto Merilainen 
---
 drivers/gpu/drm/tegra/Makefile |   3 +-
 drivers/gpu/drm/tegra/drm.c|   9 +-
 drivers/gpu/drm/tegra/drm.h|   1 +
 drivers/gpu/drm/tegra/vic.c| 593 +
 drivers/gpu/drm/tegra/vic.h| 116 
 include/linux/host1x.h |   1 +
 6 files changed, 721 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/tegra/vic.c
 create mode 100644 drivers/gpu/drm/tegra/vic.h

diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile
index 2c66a8db9da4..3bc3566e00b6 100644
--- a/drivers/gpu/drm/tegra/Makefile
+++ b/drivers/gpu/drm/tegra/Makefile
@@ -13,6 +13,7 @@ tegra-drm-y := \
sor.o \
dpaux.o \
gr2d.o \
-   gr3d.o
+   gr3d.o \
+   vic.o

 obj-$(CONFIG_DRM_TEGRA) += tegra-drm.o
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index bfad15a913a0..d947f5f4d801 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2012 Avionic Design GmbH
- * Copyright (C) 2012-2013 NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (C) 2012-2015 NVIDIA CORPORATION.  All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -1048,6 +1048,7 @@ static const struct of_device_id host1x_drm_subdevs[] = {
{ .compatible = "nvidia,tegra124-dc", },
{ .compatible = "nvidia,tegra124-sor", },
{ .compatible = "nvidia,tegra124-hdmi", },
+   { .compatible = "nvidia,tegra124-vic", },
{ /* sentinel */ }
 };

@@ -1097,8 +1098,14 @@ static int __init host1x_drm_init(void)
if (err < 0)
goto unregister_gr2d;

+   err = platform_driver_register(_vic_driver);
+   if (err < 0)
+   goto unregister_gr3d;
+
return 0;

+unregister_gr3d:
+   platform_driver_unregister(_gr3d_driver);
 unregister_gr2d:
platform_driver_unregister(_gr2d_driver);
 unregister_dpaux:
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 0e7756e720c5..a9c02a80d6bf 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -275,5 +275,6 @@ extern struct platform_driver tegra_hdmi_driver;
 extern struct platform_driver tegra_dpaux_driver;
 extern struct platform_driver tegra_gr2d_driver;
 extern struct platform_driver tegra_gr3d_driver;
+extern struct platform_driver tegra_vic_driver;

 #endif /* HOST1X_DRM_H */
diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
new file mode 100644
index ..b7c5a96697ed
--- /dev/null
+++ b/drivers/gpu/drm/tegra/vic.c
@@ -0,0 +1,593 @@
+/*
+ * Copyright (c) 2015, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "drm.h"
+#include "gem.h"
+#include "vic.h"
+
+#define VIC_IDLE_TIMEOUT_DEFAULT   1   /* 10 milliseconds */
+#define VIC_IDLE_CHECK_PERIOD  10  /* 10 usec */
+
+struct vic;
+
+struct vic_config {
+   /* firmware name */
+   char *ucode_name;
+
+   /* class id */
+   u32 class_id;
+
+   /* powergate id */
+   int powergate_id;
+};
+
+struct vic {
+   struct {
+   u32 bin_data_offset;
+   u32 data_offset;
+   u32 data_size;
+   u32 code_offset;
+   u32 size;
+   } os, fce;
+
+   struct tegra_bo *ucode_bo;
+   bool ucode_valid;
+   void *ucode_vaddr;
+
+   bool booted;
+
+   void __iomem *regs;
+   struct tegra_drm_client client;
+   struct host1x_channel *channel;
+   struct iommu_domain *domain;
+   struct device *dev;
+   struct clk *clk;
+   struct reset_control *rst;
+
+   /* Platform configuration */
+   struct vic_config *config;
+
+   /* for firewall - this determines if method 1 should be regarded
+* as an address register */
+   bool method_data_is_addr_reg;
+};
+
+static inline struct vic *to_vic(struct tegra_drm_client *client)
+{
+   return container_of(client, struct vic, client);
+}
+
+void vic_writel(struct vic *vic, u32 v, u32 r)
+{
+   writel(v, vic->regs + r);
+}
+
+u32 vic_readl(struct vic *vic, u32 r)
+{
+   return readl(vic->regs + r);
+}
+
+static int vic_wait_idle(struct vic *vic)
+{
+   u32 timeout =