From b82262c44101caa9b7e8d6feddd4f5f51a534923 Mon Sep 17 00:00:00 2001 From: Karl Simonsen Date: Fri, 17 Jun 2022 14:18:48 -0700 Subject: [PATCH] Perfetto: Add check for presence of a battery Summary: Perfetto: Add check for presence of a battery before allowing perfetto battery profiling. Reviewed By: MarkAndersonIX Differential Revision: D37214651 fbshipit-source-id: 86f0175c815e246fa754be43aa9677d80fe28c14 --- .../platforms/android/android_platform.py | 2 +- benchmarking/profilers/perfetto/perfetto.py | 60 +++++++++++++------ 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/benchmarking/platforms/android/android_platform.py b/benchmarking/platforms/android/android_platform.py index e0ea3aae..6cf5234b 100644 --- a/benchmarking/platforms/android/android_platform.py +++ b/benchmarking/platforms/android/android_platform.py @@ -252,7 +252,7 @@ def runBinaryBenchmark(self, cmd, *args, **kwargs): if enable_profiling: profiler = platform_args["profiling_args"]["profiler"] profiling_types = platform_args["profiling_args"]["types"] - profiler_exception_message = f"An error has occurred when running {profiler} profiler on device {self.device_label}." + profiler_exception_message = f"An error has occurred while running {profiler} profiler on device {self.device_label}." try: if profiler == "simpleperf": if profiling_types != ["cpu"]: diff --git a/benchmarking/profilers/perfetto/perfetto.py b/benchmarking/profilers/perfetto/perfetto.py index 93e0541d..8f65c8fb 100644 --- a/benchmarking/profilers/perfetto/perfetto.py +++ b/benchmarking/profilers/perfetto/perfetto.py @@ -149,7 +149,8 @@ def __init__( # This is a generic path to disconnect battery charing on many Android devices. # Going forward, it may be necessary to override this default mechanism on some devices. - self.battery_state_path = "/sys/class/power_supply/battery/input_suspend" + self.battery_present_path = "/sys/class/power_supply/battery/present" + self.battery_disconnected_path = "/sys/class/power_supply/battery/input_suspend" self.battery_state: BatteryState = BatteryState.connected super(Perfetto, self).__init__(None) @@ -165,11 +166,27 @@ def __exit__(self, type, value, traceback): def _start(self): """Begin Perfetto profiling on platform.""" self.valid = False + + # Validation if self.android_version < 10: raise BenchmarkUnsupportedDeviceException( f"Attempt to run perfetto on {self.platform.type} {self.platform.rel_version} device {self.platform.device_label} ignored." ) + if self.is_rooted_device: + if not self.user_was_root: + self.adb.root() + + if "battery" in self.types: + if self.is_rooted_device: + if not self._hasBattery(): + raise BenchmarkUnsupportedDeviceException( + f"Cannot run perfetto battery profiling on device {self.platform.device_label} without a (supported) battery." + ) + else: + raise BenchmarkUnsupportedDeviceException( + f"Perfetto battery profiling is unsupported on unrooted device {self.platform.device_label}." + ) try: getLogger().info( f"Collect perfetto data on device {self.platform.device_label}." @@ -230,24 +247,34 @@ def _finish(self): shutil.rmtree(self.host_output_dir, ignore_errors=True) self.valid = False # prevent additional calls - def _setBatteryState(self, state: BatteryState, path: str): - cmd_exists = ["ls", path] - cmd_update = ["echo", str(state.value), ">", path] + def _hasBattery(self): + cmd_has_battery = ["cat", self.battery_present_path] + return self.adb.shell(cmd_has_battery, retry=1, silent=True) == ["1"] + + def _setBatteryState(self, state: BatteryState): + cmd_exists = ["ls", self.battery_disconnected_path] + cmd_update = ["echo", str(state.value), ">", self.battery_disconnected_path] try: - if self.adb.shell(cmd_exists, retry=1, silent=True) == [path]: + if self.adb.shell(cmd_exists, retry=1, silent=True) == [ + self.battery_disconnected_path + ]: self.adb.shell(cmd_update, retry=1, silent=True) getLogger().info( f"Battery {state.name} for charging on device {self.platform.device_label}." ) self.battery_state = state else: - getLogger().info( + # this should have been caught by the battery check, but just in case + raise BenchmarkUnsupportedDeviceException( f"Battery disconnect not supported for device {self.platform.device_label}." ) - except Exception as e: - getLogger().warning( - f"Battery not {state.name} for charging on device {self.platform.device_label}, exception {e}." - ) + except Exception: + # alert if we disconnected but cannot now reconnect + error = f"Battery not {state.name} for charging on device {self.platform.device_label}." + if state == BatteryState.connected: + getLogger().critical(error) + else: + getLogger().exception(error) def _upload_config(self, config_file): self.meta = upload_profiling_reports( @@ -279,7 +306,7 @@ def _restoreState(self): """Restore original device state if necessary""" if self.battery_state == BatteryState.disconnected: # restore battery charging - self._setBatteryState(BatteryState.connected, self.battery_state_path) + self._setBatteryState(BatteryState.connected) if self.restoreState: if self.original_SELinux_policy == "enforcing": @@ -340,11 +367,8 @@ def _signalPerfetto(self) -> bool: def _setStateForPerfetto(self): if self.is_rooted_device: - if not self.user_was_root: - self.adb.root() - # Set SELinux to permissive mode if not already - if self.is_rooted_device and self.original_SELinux_policy == "enforcing": + if self.original_SELinux_policy == "enforcing": self.adb.shell( ["setenforce", "0"], timeout=self.DEFAULT_TIMEOUT, @@ -353,10 +377,8 @@ def _setStateForPerfetto(self): self.restoreState = True if "battery" in self.types: - # disable battery charging, if possible - self._setBatteryState( - BatteryState.disconnected, self.battery_state_path - ) + # disable battery charging + self._setBatteryState(BatteryState.disconnected) # Enable Perfetto if not yet enabled. getprop_tracing_enabled = self.adb.getprop(