Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Thierry Reding]

On Tue, Jan 06, 2015 at 10:03:36AM -0500, Ilia Mirkin wrote:
> On Tue, Jan 6, 2015 at 9:50 AM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> > On Tue, Jan 06, 2015 at 09:44:01AM -0500, Ilia Mirkin wrote:
> >> On Tue, Jan 6, 2015 at 9:37 AM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> >> > On Tue, Jan 06, 2015 at 08:27:06PM +0800, Vince Hsu wrote:
> >> >>
> >> >> On 01/06/2015 07:49 PM, Thierry Reding wrote:
> >> >> >* PGP Signed by an unknown key
> >> >> >
> >> >> >On Mon, Jan 05, 2015 at 08:50:18PM +0100, Alexandre Courbot wrote:
> >> >> >>On Mon, Jan 5, 2015 at 4:32 PM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> >> >> >>>On Tue, Dec 30, 2014 at 11:18:34AM +0800, Vince Hsu wrote:
> >> >> >>>>Hi Emil,
> >> >> >>>>
> >> >> >>>>On 12/30/2014 10:34 AM, Emil Velikov wrote:
> >> >> >>>>>On 23/12/14 10:40, Vince Hsu wrote:
> >> >> >>>>>>This patch adds some checks in the suspend/resume functions to distinguish
> >> >> >>>>>>the dGPU and mobile GPU and exports some variables/functions so that the
> >> >> >>>>>>nouveau platform device can reuse them.
> >> >> >>>>>>
> >> >> >>>>>Hi Vince,
> >> >> >>>>>
> >> >> >>>>>Afaiu one needs to export a symbol as it's used by another module or
> >> >> >>>>>subsystem. With the follow up two patches you are not doing either one,
> >> >> >>>>>so I'd assume that you can just omit the EXPORT_* changes.
> >> >> >>>>The nouveau platform device driver is built as another module -
> >> >> >>>>nouveau_platform.ko. :)
> >> >> >>>I'd like to hear the opinion of the nouveau people and Alex, but I'd
> >> >> >>>very much prefer if nouveau_platform.o was simply linked into the
> >> >> >>>nouveau.ko module. I don't see any good reason to keep it separate.
> >> >> >>Yep, I agree. The decision to host platform support in a separate
> >> >> >>module looks misleaded if it results in additional exports that we
> >> >> >>would otherwise avoid. IIUC I did this to be able to use the module
> >> >> >>convenience macros to register the platform driver.
> >> >> >>
> >> >> >>>Something like the attached patch (untested) ought to do it.
> >> >> >>This patch alone won't be enough for the reason I mentioned above.
> >> >> >>However, if Vince doesn't mind handling the platform driver
> >> >> >>registration manually in nouveau_drm_init/nouveau_drm_exit, I agree
> >> >> >>this would be the way to go.
> >> >> >If we do the conversion to generic power domains, the only Tegra-
> >> >> >specific API remaining will be the access to the fuse registers for the
> >> >> >speedo value. At that point we wouldn't need the ARCH_TEGRA dependency
> >> >> >any longer and could always build the platform driver along with the PCI
> >> >> >driver.
> >> >> Do we really want the platform driver always built with the PCI driver even
> >> >> there is no dependency between them. Actually I have some patches to
> >> >> build the platform driver with !CONFIG_PCI and would like to post them maybe
> >> >> later.
> >> >
> >> > I do see some advantage in making Nouveau build with !CONFIG_PCI because
> >> > it allows building the driver for an SoC without PCI. But I think always
> >> > compiling whatever is available won't hurt. Nouveau itself is rather big
> >> > (~1 MiB of text and ~120 KiB of data) whereas the platform code weighs
> >> > in at around 1.5 KiB of text and 108 B of data. I suspect that the PCI
> >> > glue is equally negligible.
> >>
> >> The much bigger reductions in size would be from getting rid of all
> >> the stuff that's not used at all on a GK20A, like all the firmware,
> >> and a whole bunch of logic. I had a series that split things up into
> >> nv04/nv50/nvc0 categories. I believe it was considered undesirable due
> >> to increased build complexity. (It wasn't a nice modularized
> >> separation but rather just a compile-time setting.) Shouldn't be too
> >> hard to regenerate though.
> >
> > Would you happen to remember the amount of savings for enabling only
> > nvc0? I'm not overly concerned with the module being 1 MiB on Tegra,
> > though. Most boards tend to have 2 GiB+ of memory these days and the
> > kernel footprint is pretty big without Nouveau anyway.
> 
> Well, nvc0 is a big one since that's where all the firmware lives,
> which is a big chunk of it, and you don't actually end up using any of
> it. (And even if you did, it'd be just one of the firmware files, not
> all of them).
> 
> Here was the patch: http://permalink.gmane.org/gmane.comp.video.dri.devel/102457
> 
> Looks like nvc0 only was:
> 
> NVC0: 878726 bytes
>    text    data     bss     dec     hex filename
>  360593  140327      80  501000   7a508 drivers/gpu/drm/nouveau/nouveau.ko
> 
> compared to the combo package:
> 
> NV04 + NV50 + NVC0: 1343482 bytes
>    text    data     bss     dec     hex filename
>  579171  184264     480  763915   ba80b drivers/gpu/drm/nouveau/nouveau.ko

That's a pretty significant improvement. And the patch doesn't look too
bad either. But like I said, I'm not a big fan of reducing binary size
at the expense of code complexity, so I won't loose any sleep over it.

Thierry

Attachment: pgpaVkh5VPJgL.pgp
Description: PGP signature