Re: [RFC 4/4] drm: Add NVIDIA Tegra support

* Stephen Warren wrote:

On 04/11/2012 06:10 AM, Thierry Reding wrote:

quoted

This commit adds a very basic DRM driver for NVIDIA Tegra SoCs. It
currently has rudimentary GEM support and can run a console on the
framebuffer as well as X using the xf86-video-modesetting driver.
Only the RGB output is supported. Quite a lot of things still need
to be worked out and there is a lot of room for cleanup.

I'll let Jon Mayo comment on the actual driver implementation, since
he's a lot more familiar with Tegra's display hardware. However, I have
some general comments below.

quoted

 .../devicetree/bindings/gpu/drm/tegra.txt          |   24 +
 arch/arm/mach-tegra/board-dt-tegra20.c             |    3 +
 arch/arm/mach-tegra/tegra2_clocks.c                |    8 +-
 drivers/gpu/drm/Kconfig                            |    2 +
 drivers/gpu/drm/Makefile                           |    1 +
 drivers/gpu/drm/tegra/Kconfig                      |   10 +
 drivers/gpu/drm/tegra/Makefile                     |    5 +
 drivers/gpu/drm/tegra/tegra_drv.c                  | 2241 ++++++++++++++++++++
 drivers/gpu/drm/tegra/tegra_drv.h                  |  184 ++
 include/drm/tegra_drm.h                            |   44 +

Splitting this patch into two, between arch/arm and drivers/gpu would be
a good idea.

I can certainly do that.

quoted

diff --git a/Documentation/devicetree/bindings/gpu/drm/tegra.txt b/Documentation/devicetree/bindings/gpu/drm/tegra.txt

quoted

+	drm@54200000 {
+		compatible = "nvidia,tegra20-drm";

This doesn't seem right; there isn't a "DRM" hardware module on Tegra,
since "DRM" is a Linux/software-specific term.

I'd at least expect to see this compatible flag be renamed to something
more like "nvidia,tegra20-dc" (dc==display controller).

Since Tegra has two display controller modules (I believe identical?),
and numerous other independent(?) blocks, I'd expect to see multiple
nodes in device tree, one per hardware block, such that each block gets
its own device and driver. That said, I'm not familiar enough with
Tegra's display and graphics HW to know if this makes sense. Jon, what's
your take here? The clock change below, and in particular the original
code there that we use downstream, lends weight to my argument.

quoted

+		reg = < 0x54200000 0x00040000    /* display A */
+		        0x54240000 0x00040000    /* display B */
+		        0x58000000 0x02000000 >; /* GART aperture */
+		interrupts = < 0 73 0x04    /* display A */
+		               0 74 0x04 >; /* display B */
+
+		lvds {
+			type = "rgb";

These sub-nodes probably want a "compatible" property rather than a
"type" property.

"compatible" suggests that a driver would bind to it. However the data really
is only passed to the DC driver for configuration.

quoted

+			size = <345 194>;
+
+			default-mode {
+				pixel-clock = <61715000>;
+				vertical-refresh = <50>;
+				resolution = <1366 768>;
+				bits-per-pixel = <16>;
+				horizontal-timings = <4 136 2 36>;
+				vertical-timings = <2 4 21 10>;
+			};
+		};

I imagine that quite a bit of thought needs to be put into the output
part of the binding in order to:

* Model the outputs/connectors separately from display controllers.
* Make sure that the basic infra-structure for representing an output is
general enough to be extensible to all the kinds of outputs we support,
not just the LVDS output.

I haven't played around with HDMI at all yet, so I don't know of the
requirements. I'm pretty sure the above isn't anywhere near complete
though.

* We were wondering about putting an EDID into the DT to represent the
display modes, so that all outputs had EDIDs rather than "real" monitors
having EDIDs, and fixed internal displays having some other
representation of capabilities.

That's an interesting approach. I like it.

I'm hoping that Jon will drive this.

quoted

diff --git a/arch/arm/mach-tegra/tegra2_clocks.c b/arch/arm/mach-tegra/tegra2_clocks.c

quoted

-	PERIPH_CLK("disp1",	"tegradc.0",		NULL,	27,	0x138,	600000000, mux_pllp_plld_pllc_clkm,	MUX), /* scales with voltage and process_id */
-	PERIPH_CLK("disp2",	"tegradc.1",		NULL,	26,	0x13c,	600000000, mux_pllp_plld_pllc_clkm,	MUX), /* scales with voltage and process_id */
+	PERIPH_CLK("disp1",	"tegra-drm",		NULL,	27,	0x138,	600000000, mux_pllp_plld_pllc_clkm,	MUX), /* scales with voltage and process_id */
+	PERIPH_CLK("disp2",	"tegra-drm",		NULL,	26,	0x13c,	600000000, mux_pllp_plld_pllc_clkm,	MUX), /* scales with voltage and process_id */

This doesn't seem right, and couples back to my assertion above that the
two display controller modules probably deserve separate device objects,
named e.g. tegradc.*.

I think I understand where you're going with this. Does the following look
more correct?

	disp1 : dc@54200000 {
		compatible = "nvidia,tegra20-dc";
		reg = <0x54200000, 0x00040000>;
		interrupts = <0 73 0x04>;
	};

	disp2 : dc@54240000 {
		compatible = "nvidia,tegra20-dc";
		reg = <0x54240000, 0x00040000>;
		interrupts = <0 74 0x04>;
	};

	drm {
		compatible = "nvidia,tegra20-drm";

		lvds {
			compatible = "...";
			dc = <&disp1>;
		};

		hdmi {
			compatible = "...";
			dc = <&disp2>;
		};
	};

quoted

+static int tegra_drm_parse_dt_mode(struct device *dev,

...

quoted

+	err = of_property_read_u32(node, "pixel-clock", &value);
+	if (err < 0)
+		return err;

Is it useful to call dev_err() when the DT is present but can't be
parsed, to give some clue what the problem is?

Yes, that might be a good idea.

quoted

+static int tegra_drm_parse_dt(struct platform_device *pdev)
+{

...

quoted

+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;

...

quoted

+	dev->platform_data = pdata;

I don't think you should assign to dev->platform_data. If you do, then I
think the following could happen:

* During first probe, the assignment above happens
* Module is removed, hence device removed, hence dev->platform_data
freed, but not zero'd out

Actually the code does zero out platform_data in tegra_drm_remove(). In fact
I did test module unloading and reloading and it works properly. But it
should probably be zeroed in case drm_platform_init() fails as well.

* Module is re-inserted, finds that dev->platform_data!=NULL and
proceeds to use it.

Instead, the active platform data should probably be stored in a
tegra_drm struct that's stored in the dev's private data.
tegra_drm_probe() might then look more like:

struct tegra_drm *tdev;

tdev = devm_kzalloc();
tdev->pdata = pdev->dev.platform_data;
if (!tdev->pdata)
    tdev->pdata = tegra_drm_parse_dt();
if (!tdev->pdata)
    return -EINVAL;

dev_set_drvdata(dev, tdev);

This is safe, since probe() will never assume that dev_get_drvdata()
might contain something valid before probe() sets it.

I prefer my approach over storing the data in an extra field because the
device platform_data field is where everybody would expect it. Furthermore
this wouldn't be relevant if we decided not to support non-DT setups.

Thierry