Re: [PATCH 2/2] Thinkpad Suspend Powersave: Add D2 power saving code for Thinkpads with Radeon video chipsets

Re: [PATCH 2/2] Thinkpad Suspend Powersave: Add D2 power saving code for Thinkpads with Radeon video chipsets

[Benjamin Herrenschmidt]

On Wed, 2005-03-16 at 10:16 -0500, Theodore Y. Ts'o wrote:
> Force the Radeon video chipset on IBM Thinkpads to use the D2 state when
> suspending in order to save a much greater amount of power.
> 
> This patch is based on an earlier version by Volker Braun, but instead
> of using an explicit whitelist that would have to contain hundreds of
> entries, instead we enable going to the D2 state for IBM Thinkpads if a
> CONFIG_EXPERIMENTAL option (CONFIG_FB_RADEON_THINKPAD_PM) is enabled and
> use a black-list if necessary.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

You probably want to remove the bit that does

	OUTREG(TV_DAC_CNTL, INREG(TV_DAC_CNTL) | 0x07000000);

Or you'll lose TV output :)

Ben.

> 
> Index: src/drivers/video/Kconfig
> ===================================================================
> --- src.orig/drivers/video/Kconfig	2005-03-14 12:40:48.000000000 -0500
> +++ src/drivers/video/Kconfig	2005-03-16 00:40:21.000000000 -0500
> @@ -711,6 +711,15 @@
>  	help
>  	  Say Y here if you want DDC/I2C support for your Radeon board. 
>  
> +config FB_RADEON_THINKPAD_PM
> +	bool "Video Power Management for Thinkpads (EXPERIMENTAL)"
> +	depends on PM && FB_RADEON && X86 && EXPERIMENTAL
> +	default n
> +	help
> +	  Say Y here if you want to force the Radeon video chipset on 
> +	  IBM Thinkpads to use the D2 state when suspending in order to
> +	  save a much greater amount of power.
> +
>  config FB_RADEON_DEBUG
>  	bool "Lots of debug output from Radeon driver"
>  	depends on FB_RADEON
> Index: src/drivers/video/aty/radeon_base.c
> ===================================================================
> --- src.orig/drivers/video/aty/radeon_base.c	2005-03-14 12:40:48.000000000 -0500
> +++ src/drivers/video/aty/radeon_base.c	2005-03-14 12:40:48.000000000 -0500
> @@ -273,6 +273,9 @@
>  #ifdef CONFIG_MTRR
>  static int nomtrr = 0;
>  #endif
> +#ifdef CONFIG_FB_RADEON_THINKPAD_PM
> +int radeon_force_sleep = 0;
> +#endif
>  
>  /*
>   * prototypes
> @@ -2535,6 +2538,10 @@
>  			force_measure_pll = 1;
>  		} else if (!strncmp(this_opt, "ignore_edid", 11)) {
>  			ignore_edid = 1;
> +#ifdef CONFIG_FB_RADEON_THINKPAD_PM
> +		} else if (!strncmp(this_opt, "force_sleep", 11)) {
> +			radeon_force_sleep = 1;
> +#endif
>  		} else
>  			mode_option = this_opt;
>  	}
> @@ -2574,3 +2581,7 @@
>  MODULE_PARM_DESC(panel_yres, "int: set panel yres");
>  module_param(mode_option, charp, 0);
>  MODULE_PARM_DESC(mode_option, "Specify resolution as \"<xres>x<yres>[-<bpp>][@<refresh>]\" ");
> +#ifdef CONFIG_FB_RADEON_THINKPAD_PM
> +module_param(radeon_force_sleep, int, 0);
> +MODULE_PARM_DESC(radeon_force_sleep, "bool: force ACPI sleep mode on untested machines");
> +#endif
> Index: src/drivers/video/aty/radeon_pm.c
> ===================================================================
> --- src.orig/drivers/video/aty/radeon_pm.c	2005-03-14 12:40:48.000000000 -0500
> +++ src/drivers/video/aty/radeon_pm.c	2005-03-14 12:40:48.000000000 -0500
> @@ -27,6 +27,27 @@
>  
>  #include "ati_ids.h"
>  
> +#ifdef CONFIG_FB_RADEON_THINKPAD_PM
> +#include <linux/dmi.h>
> +
> +static struct dmi_system_id __devinitdata radeonfb_dmi_table[] = {
> +	{
> +		.ident = "IBM ThinkPad",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "IBM"),
> +			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad"),
> +		},
> +	},
> +	{ },
> +};
> +
> +static struct dmi_system_id __devinitdata radeonfb_dmi_blacklist[] = {
> +	{ },
> +};
> +
> +extern int radeon_force_sleep;
> +#endif
> +
>  void radeon_pm_disable_dynamic_mode(struct radeonfb_info *rinfo)
>  {
>  	u32 tmp;
> @@ -2750,6 +2771,30 @@
>  #endif
>  	}
>  #endif /* defined(CONFIG_PM) && defined(CONFIG_PPC_OF) */
> +
> +	/* The PM code also seems to work on many IBM ThinkPad models, 
> +	 * but of course Your Mileage May Vary.
> +	 */
> +#ifdef CONFIG_FB_RADEON_THINKPAD_PM
> +	if (radeon_force_sleep || 
> +	    (rinfo->is_mobility && rinfo->pm_reg &&
> +	     (rinfo->family <= CHIP_FAMILY_RV250) &&
> +	     dmi_check_system(radeonfb_dmi_table) && 
> +	     !dmi_check_system(radeonfb_dmi_blacklist))) {
> +		if (radeon_force_sleep)
> +			printk("radeonfb: forcefully enabling sleep mode\n");
> +		else
> +			printk("radeonfb: enabling sleep mode\n");
> +
> +		rinfo->pm_mode |= radeon_pm_d2;
> +
> +		/* Power down TV DAC, that saves a significant amount of power,
> +		 * we'll have something better once we actually have some TVOut
> +		 * support
> +		 */
> +		OUTREG(TV_DAC_CNTL, INREG(TV_DAC_CNTL) | 0x07000000);
> +	}
> +#endif /* CONFIG_FB_RADEON_THINKPAD_PM */
>  }
>  
>  void radeonfb_pm_exit(struct radeonfb_info *rinfo)
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>

Re: [PATCH 2/2] Thinkpad Suspend Powersave: Add D2 power saving code for Thinkpads with Radeon video chipsets

[Theodore Ts'o]

On Thu, Mar 17, 2005 at 10:19:04AM +1100, Benjamin Herrenschmidt wrote:
> You probably want to remove the bit that does
> 
> 	OUTREG(TV_DAC_CNTL, INREG(TV_DAC_CNTL) | 0x07000000);
> 
> Or you'll lose TV output :)

I'm not using TV output, and the original patch stated:

> > +		/* Power down TV DAC, that saves a significant amount of power,
> > +		 * we'll have something better once we actually have some TVOut
> > +		 * support
> > +		 */

I suppose I should renable the TV DAC and see how much power it
actually consumes if I enable it.  It would seem to me that we should
have a way that we can power down whatever parts of the video chipset
that we're not using.  (For example if I don't have anything connected
to the VGA output, it would be good if we could power that down too...)

						- Ted

Re: [PATCH 2/2] Thinkpad Suspend Powersave: Add D2 power saving code for Thinkpads with Radeon video chipsets

[Benjamin Herrenschmidt]

On Thu, 2005-03-17 at 22:39 -0500, Theodore Ts'o wrote:
> On Thu, Mar 17, 2005 at 10:19:04AM +1100, Benjamin Herrenschmidt wrote:
> > You probably want to remove the bit that does
> > 
> > 	OUTREG(TV_DAC_CNTL, INREG(TV_DAC_CNTL) | 0x07000000);
> > 
> > Or you'll lose TV output :)
> 
> I'm not using TV output, and the original patch stated:
> 
> > > +		/* Power down TV DAC, that saves a significant amount of power,
> > > +		 * we'll have something better once we actually have some TVOut
> > > +		 * support
> > > +		 */

Yup, I know, I wrote this bit :)

> I suppose I should renable the TV DAC and see how much power it
> actually consumes if I enable it.  It would seem to me that we should
> have a way that we can power down whatever parts of the video chipset
> that we're not using.  (For example if I don't have anything connected
> to the VGA output, it would be good if we could power that down too...)

We can power down the internal DAC too, yes, and the TMDS transmitter
when no DVI is plugged, etc.. and we can also lower the chip clock :) I
do intend to do these things. The problem right now is
that the above will break some users who have a BIOS that can set
TV-Out. Maybe some sysfs attribute ? At least until I can properly
probe all ports including the TV Out (I'm working on that). Ultimately,
the driver should be able to properly detect everything that is
connected.

Ben.