* Stephen Warren wrote:
> On 04/11/2012 06:10 AM, Thierry Reding wrote:
> > 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.
> 
> >  .../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.

> > diff --git a/Documentation/devicetree/bindings/gpu/drm/tegra.txt b/Documentation/devicetree/bindings/gpu/drm/tegra.txt
> 
> > +	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.
> 
> > +		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.

> > +			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.
> 
> > diff --git a/arch/arm/mach-tegra/tegra2_clocks.c b/arch/arm/mach-tegra/tegra2_clocks.c
> 
> > -	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>;
		};
	};

> > +static int tegra_drm_parse_dt_mode(struct device *dev,
> ...
> > +	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.

> > +static int tegra_drm_parse_dt(struct platform_device *pdev)
> > +{
> ...
> > +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > +	if (!pdata)
> > +		return -ENOMEM;
> ...
> > +	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