[PATCH] drm radeon: Return -EINVAL on wrong pm sysfs access
On Saturday 26 March 2011 16:38:22 Rafa? Mi?ecki wrote:
> 2011/3/23 Thomas Renninger :
> > drm radeon: Return -EINVAL on wrong pm sysfs access
> >
> > Throw an error if someone tries to fill this with
> > wrong data, instead of simply ignoring the input.
> > Now you get:
> >
> > echo hello >/sys/../power_method
> > -bash: echo: write error: Invalid argument
> >
> > Signed-off-by: Thomas Renninger
> > CC: Alexander.Deucher at amd.com
> > CC: dri-devel at lists.freedesktop.org
> >
> > ---
> > drivers/gpu/drm/radeon/radeon_pm.c |8 +---
> > 1 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_pm.c
> > b/drivers/gpu/drm/radeon/radeon_pm.c
> > index 2aed03b..08de669 100644
> > --- a/drivers/gpu/drm/radeon/radeon_pm.c
> > +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> > @@ -365,12 +365,14 @@ static ssize_t radeon_set_pm_profile(struct device
> > *dev,
> >else if (strncmp("high", buf, strlen("high")) == 0)
> >rdev->pm.profile = PM_PROFILE_HIGH;
> >else {
> > - DRM_ERROR("invalid power profile!\n");
> > + count = -EINVAL;
>
> I think this does not match kernel coding style (braces).
Can you be more specific, please.
I can't see style issues, ./scripts/checkpatch.pl is also happy.
Thomas
Re: [PATCH] drm radeon: Return -EINVAL on wrong pm sysfs access
On Saturday 26 March 2011 16:38:22 Rafał Miłecki wrote:
2011/3/23 Thomas Renninger tr...@suse.de:
drm radeon: Return -EINVAL on wrong pm sysfs access
Throw an error if someone tries to fill this with
wrong data, instead of simply ignoring the input.
Now you get:
echo hello /sys/../power_method
-bash: echo: write error: Invalid argument
Signed-off-by: Thomas Renninger tr...@suse.de
CC: alexander.deuc...@amd.com
CC: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/radeon/radeon_pm.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c
b/drivers/gpu/drm/radeon/radeon_pm.c
index 2aed03b..08de669 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -365,12 +365,14 @@ static ssize_t radeon_set_pm_profile(struct device
*dev,
else if (strncmp(high, buf, strlen(high)) == 0)
rdev-pm.profile = PM_PROFILE_HIGH;
else {
- DRM_ERROR(invalid power profile!\n);
+ count = -EINVAL;
I think this does not match kernel coding style (braces).
Can you be more specific, please.
I can't see style issues, ./scripts/checkpatch.pl is also happy.
Thomas
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm radeon: Return -EINVAL on wrong pm sysfs access
drm radeon: Return -EINVAL on wrong pm sysfs access
Throw an error if someone tries to fill this with
wrong data, instead of simply ignoring the input.
Now you get:
echo hello >/sys/../power_method
-bash: echo: write error: Invalid argument
Signed-off-by: Thomas Renninger
CC: Alexander.Deucher at amd.com
CC: dri-devel at lists.freedesktop.org
---
drivers/gpu/drm/radeon/radeon_pm.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c
b/drivers/gpu/drm/radeon/radeon_pm.c
index 2aed03b..08de669 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -365,12 +365,14 @@ static ssize_t radeon_set_pm_profile(struct device *dev,
else if (strncmp("high", buf, strlen("high")) == 0)
rdev->pm.profile = PM_PROFILE_HIGH;
else {
- DRM_ERROR("invalid power profile!\n");
+ count = -EINVAL;
goto fail;
}
radeon_pm_update_profile(rdev);
radeon_pm_set_clocks(rdev);
- }
+ } else
+ count = -EINVAL;
+
fail:
mutex_unlock(>pm.mutex);
@@ -413,7 +415,7 @@ static ssize_t radeon_set_pm_method(struct device *dev,
mutex_unlock(>pm.mutex);
cancel_delayed_work_sync(>pm.dynpm_idle_work);
} else {
- DRM_ERROR("invalid power method!\n");
+ count = -EINVAL;
goto fail;
}
radeon_pm_compute_clocks(rdev);
[PATCH] drm radeon: Return -EINVAL on wrong pm sysfs access
drm radeon: Return -EINVAL on wrong pm sysfs access
Throw an error if someone tries to fill this with
wrong data, instead of simply ignoring the input.
Now you get:
echo hello /sys/../power_method
-bash: echo: write error: Invalid argument
Signed-off-by: Thomas Renninger tr...@suse.de
CC: alexander.deuc...@amd.com
CC: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/radeon/radeon_pm.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c
b/drivers/gpu/drm/radeon/radeon_pm.c
index 2aed03b..08de669 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -365,12 +365,14 @@ static ssize_t radeon_set_pm_profile(struct device *dev,
else if (strncmp(high, buf, strlen(high)) == 0)
rdev-pm.profile = PM_PROFILE_HIGH;
else {
- DRM_ERROR(invalid power profile!\n);
+ count = -EINVAL;
goto fail;
}
radeon_pm_update_profile(rdev);
radeon_pm_set_clocks(rdev);
- }
+ } else
+ count = -EINVAL;
+
fail:
mutex_unlock(rdev-pm.mutex);
@@ -413,7 +415,7 @@ static ssize_t radeon_set_pm_method(struct device *dev,
mutex_unlock(rdev-pm.mutex);
cancel_delayed_work_sync(rdev-pm.dynpm_idle_work);
} else {
- DRM_ERROR(invalid power method!\n);
+ count = -EINVAL;
goto fail;
}
radeon_pm_compute_clocks(rdev);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[ANNOUNCE] cpupowerutils - cpufrequtils extended with quite some features
> cpupowerutils is based on the well known cpufrequtils project. Forgot to mention: Big thanks to Dominik Brodowski who already gave this some tests! Outcome (heavily shortened): "You can add that you have my Signed-off-by / Acked-by to all these patches" Thomas
[ANNOUNCE] cpupowerutils - cpufrequtils extended with quite some features
Hi, cpupowerutils is based on the well known cpufrequtils project. Where do I find it? --- A git repository is hosted on gitorious: git://gitorious.org/cpupowerutils/cpupowerutils.git Be careful, it's not the default, but the cpupowerutils branch! You can also directly download a tarball of the cpupowerutils branch: wget http://gitorious.org/cpupowerutils/cpupowerutils/archive- tarball/cpupowerutils cpupowerutils.tar.gz How to make it run -- You need the pcitutils package (or whatever provides libpci) at runtime and pcitutils-devel package (or whatever provides /usr/include/pci/pci.h) at compile time. Also a gcc version that provides cpuid.h is needed, but it's in there for some time already afaik. Don't forget to use the right branch if you use the git repo: git branch --track cpupowerutils origin/cpupowerutils git checkout cpupowerutils make # There is nothing for choosing lib vs lib64, default is # /usr/lib,therefore you might need: libdir=/usr/lib64 make install-lib ldconfig ./cpupower There is one known compile warning in get_cpu_topology, it's on the ToDo list. Why is there need for another tool? --- CPU power consumption vs performance tuning is not about CPU frequency switching anymore for quite some time. Deep sleep states, traditional dynamic frequency scaling and hidden turbo/boost frequencies are tight close together and depend on each other. The first two exist on different architectures like PPC, Itanium and ARM the latter only on X86. On X86 the APU (CPU+GPU) will only run most efficiently if CPU and GPU has proper power management in place. Users and Developers want to have *one* tool to get an overview what their system supports and to monitor and debug CPU power management in detail. The tool should compile and work on as much architectures as possible. What is this tool doing? It provides all features cpufrequtils does. It got enhanced with cpuidle and turbo/boost mode (on X86) statistics. On AMD the exact amount of supported boost states and their frequencies are shown. On Intel only turbo/boost support is shown. It got enhanced with a generic HW monitor tool (cpupower monitor). The generic HW monitor tool is the most powerful enhancement. It is a framework to monitor kernel or HW power statistics. It's easy to extend with additional, architecture or processor model specific counters. It's based on turbostat which got merged into the kernel recently: tools/power/x86/turbostat In fact turbostat functionality is integrated as three separate monitors implementing the cpupower monitor API: - Nehalem - SandyBridge - Mperf While Nehalem and SandyBridge HW sleep counters are Intel specific, the mperf functionality is now available on other HW than Intel, supporting the needed registers (Functionality includes: average frequency including turbo/boost frequency, C0 vs Cx idle count). Additionally there is a monitor to collect kernel idle statistics and display them (separate or together) in the same format. This works on all architectures using the cpuidle kernel framework including different ARM architectures and there were patches for powerpc (not in the mainline kernel yet). This allows to compare kernel and HW statistics on specific workloads and figure out how the HW performs compared to OS behavior. Additionally there is an AMD Liano (fam 12h) and Ontario (fam 14h) family specific monitor. This one shows different Package Core (!PC0, PC1, PC7) sleep state statistics directly read out from HW, similar to Nehalem and SandyBridge coutners. The registers are accessed via PCI and therefore can still be read out while cores have been offlined. The Liano/Ontario monitor has one special counter: NBP1 (North Bridge P1). This one always returns 0 or 1, depending on whether the North Bridge P1 power state got entered at least once during measure time. Being able to enter NBP1 state also depends on graphics power management. Therefore this counter can be used to verify whether the graphics' driver power management is working as expected. (E.g. this counter proves that radeon KMS graphics drivers are missing functionality and NBP1 will only be entered when using the fglrx driver). Some examples - On a somewhat older Intel machine where turbostat complaints about: /archteam/trenn/packages/turbostat/turbostat No invariant TSC You still get mperf statistics (here core 1 is 100% utilized): /archteam/trenn/git/latest_cpupowerutils/cpufrequtils/cpupower monitor |Mperf || Idle_Stats CPU | C0 | Cx | Freq || POLL | C1 | C2 | C3 0| 3.71| 96.29| 2833|| 0.00| 0.00| 0.02| 96.32 1| 100.0| -0.00| 2833|| 0.00| 0.00| 0.00| 0.00 2| 9.06| 90.94| 1983|| 0.00| 7.69| 6.98| 76.45 3| 7.43| 92.57| 2039|| 0.00| 2.60| 12.62| 77.52 Hm, mperf (C0 vs Cx) implementation also depends on a correct working TSC, but shows
Re: [ANNOUNCE] cpupowerutils - cpufrequtils extended with quite some features
cpupowerutils is based on the well known cpufrequtils project. Forgot to mention: Big thanks to Dominik Brodowski who already gave this some tests! Outcome (heavily shortened): You can add that you have my Signed-off-by / Acked-by to all these patches Thomas ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[ANNOUNCE] cpupowerutils - cpufrequtils extended with quite some features
Hi, cpupowerutils is based on the well known cpufrequtils project. Where do I find it? --- A git repository is hosted on gitorious: git://gitorious.org/cpupowerutils/cpupowerutils.git Be careful, it's not the default, but the cpupowerutils branch! You can also directly download a tarball of the cpupowerutils branch: wget http://gitorious.org/cpupowerutils/cpupowerutils/archive- tarball/cpupowerutils cpupowerutils.tar.gz How to make it run -- You need the pcitutils package (or whatever provides libpci) at runtime and pcitutils-devel package (or whatever provides /usr/include/pci/pci.h) at compile time. Also a gcc version that provides cpuid.h is needed, but it's in there for some time already afaik. Don't forget to use the right branch if you use the git repo: git branch --track cpupowerutils origin/cpupowerutils git checkout cpupowerutils make # There is nothing for choosing lib vs lib64, default is # /usr/lib,therefore you might need: libdir=/usr/lib64 make install-lib ldconfig ./cpupower There is one known compile warning in get_cpu_topology, it's on the ToDo list. Why is there need for another tool? --- CPU power consumption vs performance tuning is not about CPU frequency switching anymore for quite some time. Deep sleep states, traditional dynamic frequency scaling and hidden turbo/boost frequencies are tight close together and depend on each other. The first two exist on different architectures like PPC, Itanium and ARM the latter only on X86. On X86 the APU (CPU+GPU) will only run most efficiently if CPU and GPU has proper power management in place. Users and Developers want to have *one* tool to get an overview what their system supports and to monitor and debug CPU power management in detail. The tool should compile and work on as much architectures as possible. What is this tool doing? It provides all features cpufrequtils does. It got enhanced with cpuidle and turbo/boost mode (on X86) statistics. On AMD the exact amount of supported boost states and their frequencies are shown. On Intel only turbo/boost support is shown. It got enhanced with a generic HW monitor tool (cpupower monitor). The generic HW monitor tool is the most powerful enhancement. It is a framework to monitor kernel or HW power statistics. It's easy to extend with additional, architecture or processor model specific counters. It's based on turbostat which got merged into the kernel recently: tools/power/x86/turbostat In fact turbostat functionality is integrated as three separate monitors implementing the cpupower monitor API: - Nehalem - SandyBridge - Mperf While Nehalem and SandyBridge HW sleep counters are Intel specific, the mperf functionality is now available on other HW than Intel, supporting the needed registers (Functionality includes: average frequency including turbo/boost frequency, C0 vs Cx idle count). Additionally there is a monitor to collect kernel idle statistics and display them (separate or together) in the same format. This works on all architectures using the cpuidle kernel framework including different ARM architectures and there were patches for powerpc (not in the mainline kernel yet). This allows to compare kernel and HW statistics on specific workloads and figure out how the HW performs compared to OS behavior. Additionally there is an AMD Liano (fam 12h) and Ontario (fam 14h) family specific monitor. This one shows different Package Core (!PC0, PC1, PC7) sleep state statistics directly read out from HW, similar to Nehalem and SandyBridge coutners. The registers are accessed via PCI and therefore can still be read out while cores have been offlined. The Liano/Ontario monitor has one special counter: NBP1 (North Bridge P1). This one always returns 0 or 1, depending on whether the North Bridge P1 power state got entered at least once during measure time. Being able to enter NBP1 state also depends on graphics power management. Therefore this counter can be used to verify whether the graphics' driver power management is working as expected. (E.g. this counter proves that radeon KMS graphics drivers are missing functionality and NBP1 will only be entered when using the fglrx driver). Some examples - On a somewhat older Intel machine where turbostat complaints about: /archteam/trenn/packages/turbostat/turbostat No invariant TSC You still get mperf statistics (here core 1 is 100% utilized): /archteam/trenn/git/latest_cpupowerutils/cpufrequtils/cpupower monitor |Mperf || Idle_Stats CPU | C0 | Cx | Freq || POLL | C1 | C2 | C3 0| 3.71| 96.29| 2833|| 0.00| 0.00| 0.02| 96.32 1| 100.0| -0.00| 2833|| 0.00| 0.00| 0.00| 0.00 2| 9.06| 90.94| 1983|| 0.00| 7.69| 6.98| 76.45 3| 7.43| 92.57| 2039|| 0.00| 2.60| 12.62| 77.52 Hm, mperf (C0 vs Cx) implementation also depends on a correct working TSC, but shows
