[PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Terje Bergström]

On 21.12.2012 16:36, Thierry Reding wrote:
> On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
>> +static void tegra_drm_close(struct drm_device *drm, struct drm_file *filp)
>> +{
>> +
>> +}
>> +
> 
> This can be removed, right?

Yes, done.

> 
>> +static struct platform_driver tegra_drm_platform_driver = {
>> +.driver = {
>> +.name = "tegradrm",
> 
> This should be "tegra-drm" to match the module name.

Done.

>> -struct host1x_client;
>> +struct tegra_drm_client;
> 
> I don't see the point in renaming this. All of the devices are still
> host1x clients, right? This patch would be a whole shorter if we didn't
> rename these. None of these symbols are exported either so there's not
> much chance for them to clash with anything.

Yep, we renamed it back to make the patch smaller.

>> diff --git a/include/drm/tegra_drm.h b/include/drm/tegra_drm.h
>> new file mode 100644
>> index 000..8632f49
>> --- /dev/null
>> +++ b/include/drm/tegra_drm.h
>> @@ -0,0 +1,20 @@
>> +/*
>> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + */
>> +
>> +#ifndef _TEGRA_DRM_H_
>> +#define _TEGRA_DRM_H_
>> +
>> +#endif
> 
> This can be removed as well.

Removed.

I posted another proposal on how to handle initialization in tegradrm.
It removes a lot of code and relies more on platform_bus keeping track
of devices. Have you had time to look into it?

Terje

Re: [PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Terje Bergström]

On 21.12.2012 16:36, Thierry Reding wrote:
> On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
>> +static void tegra_drm_close(struct drm_device *drm, struct drm_file *filp)
>> +{
>> +
>> +}
>> +
> 
> This can be removed, right?

Yes, done.

> 
>> +static struct platform_driver tegra_drm_platform_driver = {
>> +.driver = {
>> +.name = "tegradrm",
> 
> This should be "tegra-drm" to match the module name.

Done.

>> -struct host1x_client;
>> +struct tegra_drm_client;
> 
> I don't see the point in renaming this. All of the devices are still
> host1x clients, right? This patch would be a whole shorter if we didn't
> rename these. None of these symbols are exported either so there's not
> much chance for them to clash with anything.

Yep, we renamed it back to make the patch smaller.

>> diff --git a/include/drm/tegra_drm.h b/include/drm/tegra_drm.h
>> new file mode 100644
>> index 000..8632f49
>> --- /dev/null
>> +++ b/include/drm/tegra_drm.h
>> @@ -0,0 +1,20 @@
>> +/*
>> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + */
>> +
>> +#ifndef _TEGRA_DRM_H_
>> +#define _TEGRA_DRM_H_
>> +
>> +#endif
> 
> This can be removed as well.

Removed.

I posted another proposal on how to handle initialization in tegradrm.
It removes a lot of code and relies more on platform_bus keeping track
of devices. Have you had time to look into it?

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Terje Bergström]

On 28.12.2012 23:21, Thierry Reding wrote:
> Instead of going over this back and forth, I've decided to rewrite this
> patch from scratch the way I think it should be done. Maybe that'll make
> things clearer. I haven't tested it on real hardware yet because I don't
> have access over the holidays, but I'll post the patch once I've
> verified that it actually works. The code is based on patches 1-4 of
> this series and is meant to replace patch 5.

I'm still not happy that host1x needs to know about drm. That's just a
wrong dependency, and wrong dependencies always end up biting back. We
need to figure out solution that satisfies both mine and your
requirements and reduce complexity.

DC/HDMI/GR2D probes are using the global data only for managing the
lists "drm_clients" and "clients". "clients" list is only required after
tegra_drm_load(). "drm_clients" is required to establish driver load order.

With dummy device, we can determine the registration (and probe) order.
tegra-drm can register the drivers for DC/HDMI/GR2D devices first, and
as last the device and driver for tegra-drm.

tegra-drm probe will allocate the global data, enumerate all drivers and
add them to the clients list. If one driver is not initialized, it'll
return with -EPROBE_DEFER and will be called again later. When all this
is successful, it'll call drm_platform_init().

The advantages:
 * No management of drm_clients list
 * No mucking with drvdata
 * host1x doesn't need to know about drm
 * The global data is allocated and deallocated by tegra-drm probe and
remove, and accessed only via drm_device->dev_private
 * Much less code

Something like the attached patch - not tested, as I don't have access
to hw now, but it shows the idea. It's based on patches 1-5 in the
series, and could replace patch 5.

Terje
-- next part --
A non-text attachment was scrubbed...
Name: 0001-drm-tegra-Postpone-usage-of-global-data.patch
Type: text/x-patch
Size: 11602 bytes
Desc: not available
URL: 

Re: [PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Terje Bergström]

On 28.12.2012 23:21, Thierry Reding wrote:
> Instead of going over this back and forth, I've decided to rewrite this
> patch from scratch the way I think it should be done. Maybe that'll make
> things clearer. I haven't tested it on real hardware yet because I don't
> have access over the holidays, but I'll post the patch once I've
> verified that it actually works. The code is based on patches 1-4 of
> this series and is meant to replace patch 5.

I'm still not happy that host1x needs to know about drm. That's just a
wrong dependency, and wrong dependencies always end up biting back. We
need to figure out solution that satisfies both mine and your
requirements and reduce complexity.

DC/HDMI/GR2D probes are using the global data only for managing the
lists "drm_clients" and "clients". "clients" list is only required after
tegra_drm_load(). "drm_clients" is required to establish driver load order.

With dummy device, we can determine the registration (and probe) order.
tegra-drm can register the drivers for DC/HDMI/GR2D devices first, and
as last the device and driver for tegra-drm.

tegra-drm probe will allocate the global data, enumerate all drivers and
add them to the clients list. If one driver is not initialized, it'll
return with -EPROBE_DEFER and will be called again later. When all this
is successful, it'll call drm_platform_init().

The advantages:
 * No management of drm_clients list
 * No mucking with drvdata
 * host1x doesn't need to know about drm
 * The global data is allocated and deallocated by tegra-drm probe and
remove, and accessed only via drm_device->dev_private
 * Much less code

Something like the attached patch - not tested, as I don't have access
to hw now, but it shows the idea. It's based on patches 1-5 in the
series, and could replace patch 5.

Terje
>From a97d475d65e68ce37c345924171dc57c5e7729ee Mon Sep 17 00:00:00 2001
From: Terje Bergstrom 
Date: Sat, 29 Dec 2012 11:43:54 +0200
Subject: [PATCH] drm: tegra: Postpone usage of global data

Change-Id: Ibfd1d4eeb267ac185de4508a1547fb009b80e93a
Signed-off-by: Terje Bergstrom 
---
 drivers/gpu/drm/tegra/dc.c   |   18 -
 drivers/gpu/drm/tegra/drm.c  |  151 +-
 drivers/gpu/drm/tegra/drm.h  |1 -
 drivers/gpu/drm/tegra/gr2d.c |7 --
 drivers/gpu/drm/tegra/hdmi.c |   18 -
 5 files changed, 47 insertions(+), 148 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 24bcd06..73056b7 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -740,7 +740,6 @@ static int tegra_dc_probe(struct platform_device *pdev)
 	struct resource *regs;
 	struct tegra_dc *dc;
 	int err;
-	struct tegradrm *tegradrm = platform_get_drvdata(pdev);
 
 	dc = devm_kzalloc(&pdev->dev, sizeof(*dc), GFP_KERNEL);
 	if (!dc)
@@ -780,7 +779,6 @@ static int tegra_dc_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&dc->client.list);
 	dc->client.ops = &dc_client_ops;
 	dc->client.dev = &pdev->dev;
-	dc->client.tegradrm = tegradrm;
 
 	err = tegra_dc_rgb_probe(dc);
 	if (err < 0 && err != -ENODEV) {
@@ -788,13 +786,6 @@ static int tegra_dc_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = tegra_drm_register_client(tegradrm, &dc->client);
-	if (err < 0) {
-		dev_err(&pdev->dev, "failed to register tegra drm client: %d\n",
-			err);
-		return err;
-	}
-
 	platform_set_drvdata(pdev, dc);
 
 	return 0;
@@ -803,15 +794,6 @@ static int tegra_dc_probe(struct platform_device *pdev)
 static int tegra_dc_remove(struct platform_device *pdev)
 {
 	struct tegra_dc *dc = platform_get_drvdata(pdev);
-	int err;
-
-	err = tegra_drm_unregister_client(dc->client.tegradrm,
-			&dc->client);
-	if (err < 0) {
-		dev_err(&pdev->dev, "failed to unregister tegra_drm client: %d\n",
-			err);
-		return err;
-	}
 
 	clk_disable_unprepare(dc->clk);
 
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 637b621..520c281 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -25,12 +25,6 @@
 #define DRIVER_MINOR 0
 #define DRIVER_PATCHLEVEL 0
 
-struct tegra_drm_client_entry {
-	struct tegra_drm_client *client;
-	struct device_node *np;
-	struct list_head list;
-};
-
 static int tegra_drm_add_client(struct device *dev, void *data)
 {
 	static const char * const compat[] = {
@@ -42,102 +36,26 @@ static int tegra_drm_add_client(struct device *dev, void *data)
 		"nvidia,tegra30-gr2d"
 	};
 	struct tegradrm *tegradrm = data;
-	struct tegra_drm_client_entry *client;
+	struct tegra_drm_client *client;
 	unsigned int i;
 
+	/* Check if dev is a tegra-drm device, and add to client list */
 	for (i = 0; i < ARRAY_SIZE(compat); i++) {
 		if (of_device_is_compatible(dev->of_node, compat[i]) &&
 		of_device_is_available(dev->of_node)) {
-			client = kzalloc(sizeof(*client), GFP_KERNEL);
+			client = dev_get_drvdata(dev);
 			if (!client)
-return -ENOMEM;
+return -EPROBE_DEFER;
 
 			INIT_LIST_HEAD(&client->list);
-			client->np = 

[PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Thierry Reding]

On Mon, Dec 24, 2012 at 10:25:00PM -0700, Stephen Warren wrote:
> On 12/21/2012 11:50 PM, Terje Bergstr?m wrote:
> > On 21.12.2012 16:36, Thierry Reding wrote:
> >> On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
> >>> +static struct platform_driver tegra_drm_platform_driver = {
> >>> + .driver = {
> >>> + .name = "tegradrm",
> >>
> >> This should be "tegra-drm" to match the module name.
> > 
> > We've actually created two problems.
> > 
> > First is that the device name should match driver name which should
> > match module name. But host1x doesn't know the module name of tegradrm.
> 
> There's no hard requirement for the device/driver name to match the
> module name. It's good thing to do, but nothing will blow up if it don't
> (modules can use MODULE_ALIAS() to declare which drivers they expose).
> 
> But, what's the problem with host1x knowing the driver name; the host1x
> driver and tegradrm driver are both part of the same code-base, so this
> seems trivial to achieve.

Indeed. If we define the name to match the tegra-drm module name, then
just changing the above line is fine. This doesn't need to be automatic.
Making sure that both strings match in both drivers is enough.

> > Second problem is that host1x driver creates tegradrm device even if
> > tegradrm isn't loaded to system.
> 
> That's fine. If there's no driver, the device simply won't be probe()d.
> That's just like a device node existing in device tree, but the driver
> for it not being enabled in the kernel, or the relevant module not being
> inserted.
> 
> > These mean that the device has to be created in tegra-drm module to have
> 
> I definitely disagree here.

Instead of going over this back and forth, I've decided to rewrite this
patch from scratch the way I think it should be done. Maybe that'll make
things clearer. I haven't tested it on real hardware yet because I don't
have access over the holidays, but I'll post the patch once I've
verified that it actually works. The code is based on patches 1-4 of
this series and is meant to replace patch 5.

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

Re: [PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Thierry Reding]

On Mon, Dec 24, 2012 at 10:25:00PM -0700, Stephen Warren wrote:
> On 12/21/2012 11:50 PM, Terje Bergström wrote:
> > On 21.12.2012 16:36, Thierry Reding wrote:
> >> On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
> >>> +static struct platform_driver tegra_drm_platform_driver = {
> >>> + .driver = {
> >>> + .name = "tegradrm",
> >>
> >> This should be "tegra-drm" to match the module name.
> > 
> > We've actually created two problems.
> > 
> > First is that the device name should match driver name which should
> > match module name. But host1x doesn't know the module name of tegradrm.
> 
> There's no hard requirement for the device/driver name to match the
> module name. It's good thing to do, but nothing will blow up if it don't
> (modules can use MODULE_ALIAS() to declare which drivers they expose).
> 
> But, what's the problem with host1x knowing the driver name; the host1x
> driver and tegradrm driver are both part of the same code-base, so this
> seems trivial to achieve.

Indeed. If we define the name to match the tegra-drm module name, then
just changing the above line is fine. This doesn't need to be automatic.
Making sure that both strings match in both drivers is enough.

> > Second problem is that host1x driver creates tegradrm device even if
> > tegradrm isn't loaded to system.
> 
> That's fine. If there's no driver, the device simply won't be probe()d.
> That's just like a device node existing in device tree, but the driver
> for it not being enabled in the kernel, or the relevant module not being
> inserted.
> 
> > These mean that the device has to be created in tegra-drm module to have
> 
> I definitely disagree here.

Instead of going over this back and forth, I've decided to rewrite this
patch from scratch the way I think it should be done. Maybe that'll make
things clearer. I haven't tested it on real hardware yet because I don't
have access over the holidays, but I'll post the patch once I've
verified that it actually works. The code is based on patches 1-4 of
this series and is meant to replace patch 5.

Thierry


pgpEJJ2OuqAhi.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Stephen Warren]

On 12/21/2012 11:50 PM, Terje Bergstr?m wrote:
> On 21.12.2012 16:36, Thierry Reding wrote:
>> On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
>>> +static struct platform_driver tegra_drm_platform_driver = {
>>> +   .driver = {
>>> +   .name = "tegradrm",
>>
>> This should be "tegra-drm" to match the module name.
> 
> We've actually created two problems.
> 
> First is that the device name should match driver name which should
> match module name. But host1x doesn't know the module name of tegradrm.

There's no hard requirement for the device/driver name to match the
module name. It's good thing to do, but nothing will blow up if it don't
(modules can use MODULE_ALIAS() to declare which drivers they expose).

But, what's the problem with host1x knowing the driver name; the host1x
driver and tegradrm driver are both part of the same code-base, so this
seems trivial to achieve.

> Second problem is that host1x driver creates tegradrm device even if
> tegradrm isn't loaded to system.

That's fine. If there's no driver, the device simply won't be probe()d.
That's just like a device node existing in device tree, but the driver
for it not being enabled in the kernel, or the relevant module not being
inserted.

> These mean that the device has to be created in tegra-drm module to have

I definitely disagree here.

Re: [PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Stephen Warren]

On 12/21/2012 11:50 PM, Terje Bergström wrote:
> On 21.12.2012 16:36, Thierry Reding wrote:
>> On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
>>> +static struct platform_driver tegra_drm_platform_driver = {
>>> +   .driver = {
>>> +   .name = "tegradrm",
>>
>> This should be "tegra-drm" to match the module name.
> 
> We've actually created two problems.
> 
> First is that the device name should match driver name which should
> match module name. But host1x doesn't know the module name of tegradrm.

There's no hard requirement for the device/driver name to match the
module name. It's good thing to do, but nothing will blow up if it don't
(modules can use MODULE_ALIAS() to declare which drivers they expose).

But, what's the problem with host1x knowing the driver name; the host1x
driver and tegradrm driver are both part of the same code-base, so this
seems trivial to achieve.

> Second problem is that host1x driver creates tegradrm device even if
> tegradrm isn't loaded to system.

That's fine. If there's no driver, the device simply won't be probe()d.
That's just like a device node existing in device tree, but the driver
for it not being enabled in the kernel, or the relevant module not being
inserted.

> These mean that the device has to be created in tegra-drm module to have

I definitely disagree here.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Terje Bergström]

On 21.12.2012 16:36, Thierry Reding wrote:
> On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
>> +static struct platform_driver tegra_drm_platform_driver = {
>> +.driver = {
>> +.name = "tegradrm",
> 
> This should be "tegra-drm" to match the module name.

We've actually created two problems.

First is that the device name should match driver name which should
match module name. But host1x doesn't know the module name of tegradrm.

Second problem is that host1x driver creates tegradrm device even if
tegradrm isn't loaded to system.

These mean that the device has to be created in tegra-drm module to have
access to the module name. So instead of just getter, we need a getter
and a setter.

Terje

Re: [PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Terje Bergström]

On 21.12.2012 16:36, Thierry Reding wrote:
> On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
>> +static struct platform_driver tegra_drm_platform_driver = {
>> +.driver = {
>> +.name = "tegradrm",
> 
> This should be "tegra-drm" to match the module name.

We've actually created two problems.

First is that the device name should match driver name which should
match module name. But host1x doesn't know the module name of tegradrm.

Second problem is that host1x driver creates tegradrm device even if
tegradrm isn't loaded to system.

These mean that the device has to be created in tegra-drm module to have
access to the module name. So instead of just getter, we need a getter
and a setter.

Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Thierry Reding]

On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
> From: Arto Merilainen 
[...]
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
[...]
> +static int tegra_drm_add_client(struct device *dev, void *data)
> +{
> + static const char * const compat[] = {
> + "nvidia,tegra20-dc",
> + "nvidia,tegra20-hdmi",
> + "nvidia,tegra30-dc",
> + "nvidia,tegra30-hdmi",
> + };
> + struct tegradrm *tegradrm = data;
> + struct tegra_drm_client_entry *client;
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(compat); i++) {
> + if (of_device_is_compatible(dev->of_node, compat[i]) &&
> + of_device_is_available(dev->of_node)) {
> + client = kzalloc(sizeof(*client), GFP_KERNEL);
> + if (!client)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&client->list);
> + client->np = of_node_get(dev->of_node);
> +
> + list_add_tail(&client->list, &tegradrm->drm_clients);
> + dev_set_drvdata(dev, tegradrm);

This should go away now that we have an accessor for this, right?

> +static int tegra_drm_parse_dt(struct tegradrm *tegradrm)
> +{
> + int err;
> + struct device *dev;
> +
> + /* host1x is parent of all devices */
> + dev = bus_find_device_by_name(&platform_bus_type, NULL, "host1x");
> + if (!dev)
> + return -ENODEV;
> +
> + /* find devices that are available and add them into the 'required'
> +  * list */
> + err = device_for_each_child(dev, tegradrm, tegra_drm_add_client);
> +
> + return err;
> +}

I don't see why we can't keep the original code here. The problem with
your approach is that you'll match on a global device host1x, regardless
of it's relationship to tegra-drm. Instead what you should be doing is
use the hierarchical information that you have to make this work. So the
dummy device should be registered as a child device of host1x, because
that allows the host1x device to be obtained from the dummy's parent. In
that way you can use the host1x device's of_node to search through the
devicetree. That's precisely what the existing code does and I see no
reason to change that.

> +static void tegra_drm_close(struct drm_device *drm, struct drm_file *filp)
> +{
> +
> +}
> +

This can be removed, right?

> +static struct platform_driver tegra_drm_platform_driver = {
> + .driver = {
> + .name = "tegradrm",

This should be "tegra-drm" to match the module name.

> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
[...]
> index 3a843a7..34cc3a1 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  struct tegra_framebuffer {
>   struct drm_framebuffer base;
> @@ -28,17 +29,11 @@ static inline struct tegra_framebuffer 
> *to_tegra_fb(struct drm_framebuffer *fb)
>   return container_of(fb, struct tegra_framebuffer, base);
>  }
>  
> -struct host1x {
> - struct drm_device *drm;
> +struct tegradrm {

Similarly, this should be tegra_drm.

> -struct host1x_client;
> +struct tegra_drm_client;

I don't see the point in renaming this. All of the devices are still
host1x clients, right? This patch would be a whole shorter if we didn't
rename these. None of these symbols are exported either so there's not
much chance for them to clash with anything.

>  static int tegra_hdmi_probe(struct platform_device *pdev)
>  {
> - struct host1x *host1x = dev_get_drvdata(pdev->dev.parent);
>   struct tegra_hdmi *hdmi;
>   struct resource *regs;
>   int err;
> + struct tegradrm *tegradrm = platform_get_drvdata(pdev);

I think we all agreed already that you shouldn't be mucking with the
driver private data in this way.

> diff --git a/include/drm/tegra_drm.h b/include/drm/tegra_drm.h
> new file mode 100644
> index 000..8632f49
> --- /dev/null
> +++ b/include/drm/tegra_drm.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#ifndef _TEGRA_DRM_H_
> +#define _TEGRA_DRM_H_
> +
> +#endif

This can be removed as well.

Thierry
-- next part --
A non-text attachment 

[PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Terje Bergstrom]

From: Arto Merilainen 

This patch removes the redundant host1x driver from tegradrm and
makes necessary bindings to the separate host driver.

The infrastructure for drm client lists is merged to drm.c.

The patch simplifies driver initialization; The original driver had
two lists for registered devices (clients and drm_active). The
clients list included references to all registered devices whereas
the drm_active list included only the devices that the tegradrm
driver itself supported. host1x is separated into a driver of its own
and hence there should be no need to support registration of external
drivers.  Therefore, only the drm_active list is reserved. Removal of
the list also simplifies the driver unregistration.

Signed-off-by: Arto Merilainen 
Signed-off-by: Terje Bergstrom 
---
 drivers/gpu/drm/tegra/Kconfig  |2 +-
 drivers/gpu/drm/tegra/Makefile |2 +-
 drivers/gpu/drm/tegra/dc.c |   23 +--
 drivers/gpu/drm/tegra/drm.c|  231 ++--
 drivers/gpu/drm/tegra/drm.h|   43 +++---
 drivers/gpu/drm/tegra/fb.c |   17 ++-
 drivers/gpu/drm/tegra/hdmi.c   |   27 ++--
 drivers/gpu/drm/tegra/host1x.c |  325 
 include/drm/tegra_drm.h|   20 +++
 9 files changed, 298 insertions(+), 392 deletions(-)
 delete mode 100644 drivers/gpu/drm/tegra/host1x.c
 create mode 100644 include/drm/tegra_drm.h

diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
index be1daf7..4a0290e 100644
--- a/drivers/gpu/drm/tegra/Kconfig
+++ b/drivers/gpu/drm/tegra/Kconfig
@@ -1,6 +1,6 @@
 config DRM_TEGRA
tristate "NVIDIA Tegra DRM"
-   depends on DRM && OF && ARCH_TEGRA
+   depends on DRM && OF && ARCH_TEGRA && TEGRA_HOST1X
select DRM_KMS_HELPER
select DRM_GEM_CMA_HELPER
select DRM_KMS_CMA_HELPER
diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile
index 80f73d1..f4c05bb 100644
--- a/drivers/gpu/drm/tegra/Makefile
+++ b/drivers/gpu/drm/tegra/Makefile
@@ -1,7 +1,7 @@
 ccflags-y := -Iinclude/drm
 ccflags-$(CONFIG_DRM_TEGRA_DEBUG) += -DDEBUG

-tegra-drm-y := drm.o fb.o dc.o host1x.o
+tegra-drm-y := drm.o fb.o dc.o
 tegra-drm-y += output.o rgb.o hdmi.o

 obj-$(CONFIG_DRM_TEGRA) += tegra-drm.o
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 0744103..24bcd06 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -673,10 +673,10 @@ static int tegra_dc_debugfs_exit(struct tegra_dc *dc)
return 0;
 }

-static int tegra_dc_drm_init(struct host1x_client *client,
+static int tegra_dc_drm_init(struct tegra_drm_client *client,
 struct drm_device *drm)
 {
-   struct tegra_dc *dc = host1x_client_to_dc(client);
+   struct tegra_dc *dc = tegra_drm_client_to_dc(client);
int err;

dc->pipe = drm->mode_config.num_crtc;
@@ -708,9 +708,9 @@ static int tegra_dc_drm_init(struct host1x_client *client,
return 0;
 }

-static int tegra_dc_drm_exit(struct host1x_client *client)
+static int tegra_dc_drm_exit(struct tegra_drm_client *client)
 {
-   struct tegra_dc *dc = host1x_client_to_dc(client);
+   struct tegra_dc *dc = tegra_drm_client_to_dc(client);
int err;

devm_free_irq(dc->dev, dc->irq, dc);
@@ -730,17 +730,17 @@ static int tegra_dc_drm_exit(struct host1x_client *client)
return 0;
 }

-static const struct host1x_client_ops dc_client_ops = {
+static const struct tegra_drm_client_ops dc_client_ops = {
.drm_init = tegra_dc_drm_init,
.drm_exit = tegra_dc_drm_exit,
 };

 static int tegra_dc_probe(struct platform_device *pdev)
 {
-   struct host1x *host1x = dev_get_drvdata(pdev->dev.parent);
struct resource *regs;
struct tegra_dc *dc;
int err;
+   struct tegradrm *tegradrm = platform_get_drvdata(pdev);

dc = devm_kzalloc(&pdev->dev, sizeof(*dc), GFP_KERNEL);
if (!dc)
@@ -780,6 +780,7 @@ static int tegra_dc_probe(struct platform_device *pdev)
INIT_LIST_HEAD(&dc->client.list);
dc->client.ops = &dc_client_ops;
dc->client.dev = &pdev->dev;
+   dc->client.tegradrm = tegradrm;

err = tegra_dc_rgb_probe(dc);
if (err < 0 && err != -ENODEV) {
@@ -787,9 +788,9 @@ static int tegra_dc_probe(struct platform_device *pdev)
return err;
}

-   err = host1x_register_client(host1x, &dc->client);
+   err = tegra_drm_register_client(tegradrm, &dc->client);
if (err < 0) {
-   dev_err(&pdev->dev, "failed to register host1x client: %d\n",
+   dev_err(&pdev->dev, "failed to register tegra drm client: %d\n",
err);
return err;
}
@@ -801,13 +802,13 @@ static int tegra_dc_probe(struct platform_device *pdev)

 static int tegra_dc_remove(struct platform_device *pdev)
 {
-   struct host1x *host1x = dev_get_drvdata(pdev->dev.parent);
struct tegra_dc *dc

Re: [PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Thierry Reding]

On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
> From: Arto Merilainen 
[...]
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
[...]
> +static int tegra_drm_add_client(struct device *dev, void *data)
> +{
> + static const char * const compat[] = {
> + "nvidia,tegra20-dc",
> + "nvidia,tegra20-hdmi",
> + "nvidia,tegra30-dc",
> + "nvidia,tegra30-hdmi",
> + };
> + struct tegradrm *tegradrm = data;
> + struct tegra_drm_client_entry *client;
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(compat); i++) {
> + if (of_device_is_compatible(dev->of_node, compat[i]) &&
> + of_device_is_available(dev->of_node)) {
> + client = kzalloc(sizeof(*client), GFP_KERNEL);
> + if (!client)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&client->list);
> + client->np = of_node_get(dev->of_node);
> +
> + list_add_tail(&client->list, &tegradrm->drm_clients);
> + dev_set_drvdata(dev, tegradrm);

This should go away now that we have an accessor for this, right?

> +static int tegra_drm_parse_dt(struct tegradrm *tegradrm)
> +{
> + int err;
> + struct device *dev;
> +
> + /* host1x is parent of all devices */
> + dev = bus_find_device_by_name(&platform_bus_type, NULL, "host1x");
> + if (!dev)
> + return -ENODEV;
> +
> + /* find devices that are available and add them into the 'required'
> +  * list */
> + err = device_for_each_child(dev, tegradrm, tegra_drm_add_client);
> +
> + return err;
> +}

I don't see why we can't keep the original code here. The problem with
your approach is that you'll match on a global device host1x, regardless
of it's relationship to tegra-drm. Instead what you should be doing is
use the hierarchical information that you have to make this work. So the
dummy device should be registered as a child device of host1x, because
that allows the host1x device to be obtained from the dummy's parent. In
that way you can use the host1x device's of_node to search through the
devicetree. That's precisely what the existing code does and I see no
reason to change that.

> +static void tegra_drm_close(struct drm_device *drm, struct drm_file *filp)
> +{
> +
> +}
> +

This can be removed, right?

> +static struct platform_driver tegra_drm_platform_driver = {
> + .driver = {
> + .name = "tegradrm",

This should be "tegra-drm" to match the module name.

> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
[...]
> index 3a843a7..34cc3a1 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  struct tegra_framebuffer {
>   struct drm_framebuffer base;
> @@ -28,17 +29,11 @@ static inline struct tegra_framebuffer 
> *to_tegra_fb(struct drm_framebuffer *fb)
>   return container_of(fb, struct tegra_framebuffer, base);
>  }
>  
> -struct host1x {
> - struct drm_device *drm;
> +struct tegradrm {

Similarly, this should be tegra_drm.

> -struct host1x_client;
> +struct tegra_drm_client;

I don't see the point in renaming this. All of the devices are still
host1x clients, right? This patch would be a whole shorter if we didn't
rename these. None of these symbols are exported either so there's not
much chance for them to clash with anything.

>  static int tegra_hdmi_probe(struct platform_device *pdev)
>  {
> - struct host1x *host1x = dev_get_drvdata(pdev->dev.parent);
>   struct tegra_hdmi *hdmi;
>   struct resource *regs;
>   int err;
> + struct tegradrm *tegradrm = platform_get_drvdata(pdev);

I think we all agreed already that you shouldn't be mucking with the
driver private data in this way.

> diff --git a/include/drm/tegra_drm.h b/include/drm/tegra_drm.h
> new file mode 100644
> index 000..8632f49
> --- /dev/null
> +++ b/include/drm/tegra_drm.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#ifndef _TEGRA_DRM_H_
> +#define _TEGRA_DRM_H_
> +
> +#endif

This can be removed as well.

Thierry


pgpbd2X4IuClE.pgp
Description: PGP signature
___

[PATCHv4 5/8] drm: tegra: Remove redundant host1x

[Terje Bergstrom]

From: Arto Merilainen 

This patch removes the redundant host1x driver from tegradrm and
makes necessary bindings to the separate host driver.

The infrastructure for drm client lists is merged to drm.c.

The patch simplifies driver initialization; The original driver had
two lists for registered devices (clients and drm_active). The
clients list included references to all registered devices whereas
the drm_active list included only the devices that the tegradrm
driver itself supported. host1x is separated into a driver of its own
and hence there should be no need to support registration of external
drivers.  Therefore, only the drm_active list is reserved. Removal of
the list also simplifies the driver unregistration.

Signed-off-by: Arto Merilainen 
Signed-off-by: Terje Bergstrom 
---
 drivers/gpu/drm/tegra/Kconfig  |2 +-
 drivers/gpu/drm/tegra/Makefile |2 +-
 drivers/gpu/drm/tegra/dc.c |   23 +--
 drivers/gpu/drm/tegra/drm.c|  231 ++--
 drivers/gpu/drm/tegra/drm.h|   43 +++---
 drivers/gpu/drm/tegra/fb.c |   17 ++-
 drivers/gpu/drm/tegra/hdmi.c   |   27 ++--
 drivers/gpu/drm/tegra/host1x.c |  325 
 include/drm/tegra_drm.h|   20 +++
 9 files changed, 298 insertions(+), 392 deletions(-)
 delete mode 100644 drivers/gpu/drm/tegra/host1x.c
 create mode 100644 include/drm/tegra_drm.h

diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
index be1daf7..4a0290e 100644
--- a/drivers/gpu/drm/tegra/Kconfig
+++ b/drivers/gpu/drm/tegra/Kconfig
@@ -1,6 +1,6 @@
 config DRM_TEGRA
tristate "NVIDIA Tegra DRM"
-   depends on DRM && OF && ARCH_TEGRA
+   depends on DRM && OF && ARCH_TEGRA && TEGRA_HOST1X
select DRM_KMS_HELPER
select DRM_GEM_CMA_HELPER
select DRM_KMS_CMA_HELPER
diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile
index 80f73d1..f4c05bb 100644
--- a/drivers/gpu/drm/tegra/Makefile
+++ b/drivers/gpu/drm/tegra/Makefile
@@ -1,7 +1,7 @@
 ccflags-y := -Iinclude/drm
 ccflags-$(CONFIG_DRM_TEGRA_DEBUG) += -DDEBUG
 
-tegra-drm-y := drm.o fb.o dc.o host1x.o
+tegra-drm-y := drm.o fb.o dc.o
 tegra-drm-y += output.o rgb.o hdmi.o
 
 obj-$(CONFIG_DRM_TEGRA) += tegra-drm.o
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 0744103..24bcd06 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -673,10 +673,10 @@ static int tegra_dc_debugfs_exit(struct tegra_dc *dc)
return 0;
 }
 
-static int tegra_dc_drm_init(struct host1x_client *client,
+static int tegra_dc_drm_init(struct tegra_drm_client *client,
 struct drm_device *drm)
 {
-   struct tegra_dc *dc = host1x_client_to_dc(client);
+   struct tegra_dc *dc = tegra_drm_client_to_dc(client);
int err;
 
dc->pipe = drm->mode_config.num_crtc;
@@ -708,9 +708,9 @@ static int tegra_dc_drm_init(struct host1x_client *client,
return 0;
 }
 
-static int tegra_dc_drm_exit(struct host1x_client *client)
+static int tegra_dc_drm_exit(struct tegra_drm_client *client)
 {
-   struct tegra_dc *dc = host1x_client_to_dc(client);
+   struct tegra_dc *dc = tegra_drm_client_to_dc(client);
int err;
 
devm_free_irq(dc->dev, dc->irq, dc);
@@ -730,17 +730,17 @@ static int tegra_dc_drm_exit(struct host1x_client *client)
return 0;
 }
 
-static const struct host1x_client_ops dc_client_ops = {
+static const struct tegra_drm_client_ops dc_client_ops = {
.drm_init = tegra_dc_drm_init,
.drm_exit = tegra_dc_drm_exit,
 };
 
 static int tegra_dc_probe(struct platform_device *pdev)
 {
-   struct host1x *host1x = dev_get_drvdata(pdev->dev.parent);
struct resource *regs;
struct tegra_dc *dc;
int err;
+   struct tegradrm *tegradrm = platform_get_drvdata(pdev);
 
dc = devm_kzalloc(&pdev->dev, sizeof(*dc), GFP_KERNEL);
if (!dc)
@@ -780,6 +780,7 @@ static int tegra_dc_probe(struct platform_device *pdev)
INIT_LIST_HEAD(&dc->client.list);
dc->client.ops = &dc_client_ops;
dc->client.dev = &pdev->dev;
+   dc->client.tegradrm = tegradrm;
 
err = tegra_dc_rgb_probe(dc);
if (err < 0 && err != -ENODEV) {
@@ -787,9 +788,9 @@ static int tegra_dc_probe(struct platform_device *pdev)
return err;
}
 
-   err = host1x_register_client(host1x, &dc->client);
+   err = tegra_drm_register_client(tegradrm, &dc->client);
if (err < 0) {
-   dev_err(&pdev->dev, "failed to register host1x client: %d\n",
+   dev_err(&pdev->dev, "failed to register tegra drm client: %d\n",
err);
return err;
}
@@ -801,13 +802,13 @@ static int tegra_dc_probe(struct platform_device *pdev)
 
 static int tegra_dc_remove(struct platform_device *pdev)
 {
-   struct host1x *host1x = dev_get_drvdata(pdev->dev.parent);
struct