Bug 76321

Created attachment 136331 [details]
possible fix

Does this patch help?

Here is output from sensors with your patch (when card is turned off):

radeon-pci-0100
Adapter: PCI adapter
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1:            N/A  (crit =  +0.0°C, hyst =  +0.0°C)

Values crit & hyst worked fine (as it is not runtime value, but static).

And also your patch disabling showing/changing dpm/pm profile. I do not know if there is some problem to use that sysfs files when card is turned off, but if not I think there is no reason to disable it.

So in my opinion only this part of patch is needed:

@@ -538,8 +570,13 @@ static ssize_t radeon_hwmon_show_temp(struct device *dev,
 				      char *buf)
 {
 	struct radeon_device *rdev = dev_get_drvdata(dev);
+	struct drm_device *ddev = rdev->ddev;
 	int temp;
 
+	if  ((rdev->flags & RADEON_IS_PX) &&
+	     (ddev->switch_power_state != DRM_SWITCH_POWER_ON))
+		return -EINVAL;
+
 	if (rdev->asic->pm.get_temperature)
 		temp = radeon_get_temperature(rdev);
 	else

Or is really needed other hunks too?

Created attachment 136431 [details]
possible fix v2

You can't change any hardware state when the card is powered down.  This patch is a little less strict however.

With v2 patch now sensors does not report any error (when card is turned off):

$ sensors
radeon-pci-0100
Adapter: PCI adapter
temp1:            N/A  (crit = +120.0°C, hyst = +90.0°C)

This looks ok.

And for power_dpm_state & power_dpm_force_performance_level: I understand that it cannot be changed when card is turned off (I see that it also disappear from PCI bus space), but I would like to see option to set "initial" dpm state/level which will be used when card is turned om again. This can be usefull for scripts which setting powersave options depending on hostplugging AC adapter. What do you think about using same sysfs entries for it?

(In reply to Pali Rohár from comment #4)
> 
> And for power_dpm_state & power_dpm_force_performance_level: I understand
> that it cannot be changed when card is turned off (I see that it also
> disappear from PCI bus space), but I would like to see option to set
> "initial" dpm state/level which will be used when card is turned om again.
> This can be usefull for scripts which setting powersave options depending on
> hostplugging AC adapter. What do you think about using same sysfs entries
> for it?

That's fine if someone wants to implement it.  It's not a high priority for me at the moment.

Ok, I will look at it and will try to implemenent it. So can you commit radeon_hwmon_show_temp() part of patch?

Yes, I already sent attachment 136431 [details] upstream.

Created attachment 146011 [details]
patch for get/set dpm state

(In reply to Alex Deucher from comment #5)
> (In reply to Pali Rohár from comment #4)
> > 
> > And for power_dpm_state & power_dpm_force_performance_level: I understand
> > that it cannot be changed when card is turned off (I see that it also
> > disappear from PCI bus space), but I would like to see option to set
> > "initial" dpm state/level which will be used when card is turned om again.
> > This can be usefull for scripts which setting powersave options depending
> on
> > hostplugging AC adapter. What do you think about using same sysfs entries
> > for it?
> 
> That's fine if someone wants to implement it.  It's not a high priority for
> me at the moment.

It is implemented in attached patch. I tested it and it working fine on my system. My scripts automacitally change dpm state to battery when power adapter is unplugged. And when radeon card is powered on it initialize in battery state (even if last state was performance before turning card off).

(In reply to Pali Rohár from comment #8)
> Created attachment 146011 [details]
> patch for get/set dpm state
> 
> 
> It is implemented in attached patch. I tested it and it working fine on my
> system. My scripts automacitally change dpm state to battery when power
> adapter is unplugged. And when radeon card is powered on it initialize in
> battery state (even if last state was performance before turning card off).

Patch looks fine wth respect to the power state change.  You can drop the changes to radeon_[get|set]_dpm_forced_performance_level() however.  That won't work without changes to the current force_performance_level interfaces.  If you can clean up the patch as requested and generate a proper git patch, I'll apply it.

Created attachment 146241 [details]
patch v2 for get/set dpm state

New patch v2 without changes to radeon_[get|set]_dpm_forced_performance_level().

Applied to my tree.  thanks.

Alex, can you send that patch to stable trees like that in attachment 136431 [details]? Because before these patches sysfs reported battery/performance dpm state when card was turned off and some script used that information...

Yes, I've cc'ed stable.