diff --git a/services/gam/src/lib.rs b/services/gam/src/lib.rs index 501a391e9..2eac5f859 100644 --- a/services/gam/src/lib.rs +++ b/services/gam/src/lib.rs @@ -83,6 +83,9 @@ impl Gam { /// the actual screen update is allowed pub fn post_textview(&self, tv: &mut TextView) -> Result<(), xous::Error> { tv.set_op(TextOp::Render); + // force the clip_rect to none, in case a stale value from a previous bounds computation was hanging out + // the bounds should /always/ come from the GAM canvas when doing a "live fire" redraw + tv.clip_rect = None; let mut buf = Buffer::into_buf(tv.clone()).or(Err(xous::Error::InternalError))?; buf.lend_mut(self.conn, Opcode::RenderTextView.to_u32().unwrap()).or(Err(xous::Error::InternalError))?; @@ -99,6 +102,9 @@ impl Gam { tv.set_op(TextOp::Nop); Ok(()) } + /// Bounds computation does no checks on security since it's a non-drawing operation. While normal drawing always + /// takes the bounds from the canvas, the caller can specify a clip_rect in this tv, instead of drawing the + /// clip_rect from the Canvas associated with the tv. pub fn bounds_compute_textview(&self, tv: &mut TextView) -> Result<(), xous::Error> { tv.set_op(TextOp::ComputeBounds); let mut buf = Buffer::into_buf(tv.clone()).or(Err(xous::Error::InternalError))?; diff --git a/services/gam/src/main.rs b/services/gam/src/main.rs index 8a90d8444..51f92e846 100644 --- a/services/gam/src/main.rs +++ b/services/gam/src/main.rs @@ -432,6 +432,7 @@ impl ContextManager { } // run the defacement before we redraw all the canvases deface(gfx, &self.trng, canvases); + log::trace!("activate redraw"); self.redraw().expect("couldn't redraw the currently focused app"); } } @@ -448,6 +449,7 @@ impl ContextManager { for maybe_context in self.contexts.iter() { if let Some(context) = maybe_context { if token == context.app_token { + log::trace!("redraw msg to {}, id {}", context.listener, context.redraw_id); return xous::send_message(context.listener, xous::Message::new_scalar(context.redraw_id as usize, 0, 0, 0, 0) ).map(|_| ()) @@ -716,6 +718,7 @@ fn xmain() -> ! { // we keep this here because it's a fail-safe in case prior routines missed an edge case. shoot out a warning noting the issue. log::warn!("canvases were not defaced in order. running a defacement, but this could result in drawing optimizations failing."); // try to redraw the trusted foreground apps after a defacement + log::trace!("deface redraw"); context_mgr.redraw().expect("couldn't redraw after defacement"); } log::trace!("flushing..."); @@ -733,7 +736,7 @@ fn xmain() -> ! { log::trace!("rendertextview {:?}", tv); match tv.get_op() { TextOp::Nop => (), - TextOp::Render | TextOp::ComputeBounds => { + TextOp::Render => { if tv.invert & tv.token.is_some() { // an inverted text can only be made by secure processes. check that it has a valid token. if !context_mgr.is_token_valid(tv.token.unwrap()) { @@ -743,11 +746,7 @@ fn xmain() -> ! { } log::trace!("render request for {:?}", tv); - if tv.get_op() == TextOp::ComputeBounds { - tv.set_dry_run(true); - } else { - tv.set_dry_run(false); - } + tv.set_dry_run(false); if let Some(canvas) = canvases.get_mut(&tv.get_canvas_gid()) { // if we're requesting inverted text, this better be a "trusted canvas" @@ -758,12 +757,9 @@ fn xmain() -> ! { continue; } // first, figure out if we should even be drawing to this canvas. - if canvas.is_drawable() || tv.dry_run() { // dry runs should not move any pixels so they are OK to go through in any case + if canvas.is_drawable() { // dry runs should not move any pixels so they are OK to go through in any case // set the clip rectangle according to the canvas' location - let mut base_clip_rect = canvas.clip_rect(); - if tv.dry_run() { - base_clip_rect.normalize(); - } + let base_clip_rect = canvas.clip_rect(); tv.clip_rect = Some(base_clip_rect.into()); // you have to clone the tv object, because if you don't the same block of @@ -774,17 +770,12 @@ fn xmain() -> ! { // issue the draw command gfx.draw_textview(&mut tv_clone).expect("text view draw could not complete."); // copy back the fields that we want to be mutable - if tv.dry_run() { - log::trace!("got computed cursor of {:?}, bounds {:?}", tv_clone.cursor, tv_clone.bounds_computed); - } tv.cursor = tv_clone.cursor; tv.bounds_computed = tv_clone.bounds_computed; let ret = api::Return::RenderReturn(tv); buffer.replace(ret).unwrap(); - if !tv.dry_run() { - canvas.do_drawn().expect("couldn't set canvas to drawn"); - } + canvas.do_drawn().expect("couldn't set canvas to drawn"); } else { log::debug!("attempt to draw TextView on non-drawable canvas. Not fatal, but request ignored. {:?}", tv); let ret = api::Return::NotCurrentlyDrawable; @@ -795,6 +786,34 @@ fn xmain() -> ! { // silently fail if a bogus Gid is given??? } }, + TextOp::ComputeBounds => { + log::trace!("render request for {:?}", tv); + tv.set_dry_run(true); + + if tv.clip_rect.is_none() { + // fill in the clip rect from the canvas + if let Some(canvas) = canvases.get_mut(&tv.get_canvas_gid()) { + // set the clip rectangle according to the canvas' location + let mut base_clip_rect = canvas.clip_rect(); + base_clip_rect.normalize(); + tv.clip_rect = Some(base_clip_rect.into()); + } else { + info!("bogus GID {:?} in TextView {}, not doing anything in response to draw request.", tv.get_canvas_gid(), tv.text); + // silently fail if a bogus Gid is given??? + continue; + } + } + let mut tv_clone = tv.clone(); + // issue the draw command + gfx.draw_textview(&mut tv_clone).expect("text view draw could not complete."); + // copy back the fields that we want to be mutable + log::trace!("got computed cursor of {:?}, bounds {:?}", tv_clone.cursor, tv_clone.bounds_computed); + tv.cursor = tv_clone.cursor; + tv.bounds_computed = tv_clone.bounds_computed; + + let ret = api::Return::RenderReturn(tv); + buffer.replace(ret).unwrap(); + } }; } Some(Opcode::SetCanvasBounds) => { @@ -811,6 +830,7 @@ fn xmain() -> ! { // recompute the canvas orders based on the new layout let recomp_canvases = recompute_canvases(&canvases, Rectangle::new(Point::new(0, 0), screensize)); canvases = recomp_canvases; + log::trace!("canvas bounds redraw"); context_mgr.redraw().expect("can't redraw after new canvas bounds"); } cb.granted = granted; diff --git a/services/gam/src/modal.rs b/services/gam/src/modal.rs index d000148ad..5d1296e2f 100644 --- a/services/gam/src/modal.rs +++ b/services/gam/src/modal.rs @@ -1094,14 +1094,6 @@ pub enum ModalOpcode { // if changes are made here, also update MenuOpcode fn recompute_canvas(modal: &mut Modal, action: ActionType, top_text: Option<&str>, bot_text: Option<&str>, style: GlyphStyle) { // we need to set a "max" size to our modal box, so that the text computations don't fail later on let current_bounds = modal.gam.get_canvas_bounds(modal.canvas).expect("couldn't get current bounds"); - let mut new_bounds = SetCanvasBoundsRequest { - requested: Point::new(current_bounds.x, crate::api::MODAL_Y_MAX), - granted: None, - token_type: TokenType::App, - token: modal.authtoken, - }; - log::debug!("applying recomputed bounds of {:?}", new_bounds); - modal.gam.set_canvas_bounds_request(&mut new_bounds).expect("couldn't call set bounds"); // method: // - we assume the GAM gives us an initial modal with a "maximum" height setting @@ -1125,6 +1117,9 @@ fn recompute_canvas(modal: &mut Modal, action: ActionType, top_text: Option<&str top_tv.margin = Point::new(0, 0,); // all margin already accounted for in the raw bounds of the text drawing top_tv.ellipsis = false; top_tv.invert = modal.inverted; + // specify a clip rect that's the biggest possible allowed. If we don't do this, the current canvas + // bounds are used, and the operation will fail if the text has to get bigger. + top_tv.clip_rect = Some(Rectangle::new(Point::new(0, 0), Point::new(current_bounds.x, crate::api::MODAL_Y_MAX))); write!(top_tv.text, "{}", top_str).unwrap(); log::trace!("posting top tv: {:?}", top_tv); @@ -1157,6 +1152,9 @@ fn recompute_canvas(modal: &mut Modal, action: ActionType, top_text: Option<&str bot_tv.margin = Point::new(0, 0,); // all margin already accounted for in the raw bounds of the text drawing bot_tv.ellipsis = false; bot_tv.invert = modal.inverted; + // specify a clip rect that's the biggest possible allowed. If we don't do this, the current canvas + // bounds are used, and the operation will fail if the text has to get bigger. + bot_tv.clip_rect = Some(Rectangle::new(Point::new(0, 0), Point::new(current_bounds.x, crate::api::MODAL_Y_MAX))); write!(bot_tv.text, "{}", bot_str).unwrap(); log::trace!("posting bot tv: {:?}", bot_tv); @@ -1179,8 +1177,12 @@ fn recompute_canvas(modal: &mut Modal, action: ActionType, top_text: Option<&str token_type: TokenType::App, token: modal.authtoken, }; - log::debug!("applying recomputed bounds of {:?}", new_bounds); - modal.gam.set_canvas_bounds_request(&mut new_bounds).expect("couldn't call set bounds"); + // don't send the request if there is no change in the size of things. This is because the request is expensive -- it will + // result in a redraw of everything, plus defacement, etc. + if new_bounds.requested != current_bounds { + log::debug!("applying recomputed bounds of {:?}", new_bounds); + modal.gam.set_canvas_bounds_request(&mut new_bounds).expect("couldn't call set bounds"); + } } impl<'a> Modal<'a> { @@ -1268,7 +1270,7 @@ impl<'a> Modal<'a> { xous::create_thread_3(crate::forwarding_thread, addr, size, offset).expect("couldn't spawn a helper thread"); } - pub fn redraw(&self) { + pub fn redraw(&mut self) { log::debug!("modal redraw"); let canvas_size = self.gam.get_canvas_bounds(self.canvas).unwrap(); let do_redraw = self.top_dirty || self.bot_dirty; @@ -1454,6 +1456,15 @@ impl<'a, 'b> ProgressBar<'a, 'b> { pub fn update_text(&mut self, text: &str) { self.modal.modify(None, Some(text), false, None, false, None); } + /// There is a significant caveat for performance/stability for this routine. + /// This works well for a modal that is both managed and owned by the same thread so long as + /// one is not resizing the top or bottom text within the modal while changing the progress bar + /// However, if one triggers a redraw/resize of the text, then the system needs to re-run the + /// defacement computations on the underlying canvases. If one or two calls happen, this might + /// not be a big deal, but because the redraw requests that are looped back from the GAM as a + /// result of the defacement operation cannot be processed until the thread managing the progress + /// bar has finished and returned control to the main loop, redraw requests will eventually fill + /// up the server queue and cause a deadlock situation. fn update_ui(&mut self, new_percent: u32) { if new_percent != self.current_progress_percent { log::debug!("progress: {}", new_percent); diff --git a/services/root-keys/src/implementation.rs b/services/root-keys/src/implementation.rs index a6399e5ec..09b2b9671 100644 --- a/services/root-keys/src/implementation.rs +++ b/services/root-keys/src/implementation.rs @@ -752,7 +752,17 @@ impl<'a> RootKeys { rootkeys_modal.activate(); xous::yield_slice(); // give some time to the GAM to render - // capture the progress bar elements in a convenience structure + + // Capture the progress bar elements in a convenience structure. + // NOTE: This is documented in the structure itself, but, it's worth repeating here: + // this routine only works because we don't resize the rootkeys_modal box as we + // advance progress. This allows us to do a local-only redraw without triggering + // a global GAM redraw operation. If we were to resize the dialog box, this would + // trigger the defacing algorithm to send back "redraw" messages into the main + // loop's queue. However, because the main loop is currently stuck running the code + // in this routine, the "redraw" messages never get serviced (even if they are + // effectively NOPs), and eventually, these messages would fill up the queue and can cause + // the system to deadlock once the queue is full. let mut pb = ProgressBar::new(rootkeys_modal, &mut progress_action); // kick the progress bar to indicate we've entered the routine diff --git a/services/root-keys/src/main.rs b/services/root-keys/src/main.rs index 66084398c..c9ffd26ea 100644 --- a/services/root-keys/src/main.rs +++ b/services/root-keys/src/main.rs @@ -97,6 +97,9 @@ mod implementation { pub fn verify_gateware_self_signature(&mut self) -> bool { true } + pub fn test(&mut self, rootkeys_modal: &mut Modal, main_cid: xous::CID) -> Result<(), RootkeyResult> { + Ok(()) + } } } @@ -106,7 +109,7 @@ fn xmain() -> ! { use crate::implementation::RootKeys; log_server::init_wait().unwrap(); - log::set_max_level(log::LevelFilter::Debug); + log::set_max_level(log::LevelFilter::Info); log::info!("my PID is {}", xous::process::id()); let xns = xous_names::XousNames::new().unwrap(); @@ -208,7 +211,7 @@ fn xmain() -> ! { // UX flow opcodes Some(Opcode::UxTryInitKeys) => msg_scalar_unpack!(msg, _, _, _, _, { - if true { // short-circuit for testing subroutines + if false { // short-circuit for testing subroutines let _success = keys.test(&mut rootkeys_modal, main_cid); keys.finish_key_init();