On Mon, Mar 11, 2019 at 11:40:36PM +0100, Noralf Trønnes wrote:
Den 11.03.2019 20.29, skrev Daniel Vetter:
On Mon, Mar 11, 2019 at 08:23:38PM +0100, Daniel Vetter wrote:
On Mon, Mar 11, 2019 at 06:42:16PM +0100, Noralf Trønnes wrote:
This adds support for outputting kernel messages on panic(). A kernel message dumper is used to dump the log. The dumper iterates over each DRM device and it's crtc's to find suitable framebuffers.
All the other dumpers are run before this one except mtdoops. Only atomic drivers are supported.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
<snip>
+static void drm_panic_try_dev(struct drm_device *dev, struct kmsg_dumper *dumper) +{
- struct drm_framebuffer *fb;
- struct drm_plane *plane;
- struct drm_crtc *crtc;
- if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
return;- if (dumper->max_reason == KMSG_DUMP_OOPS)
pr_info("%s: %s on minor %d\n", __func__, dev->driver->name,dev->primary->index);- drm_for_each_crtc(crtc, dev) {
if (!ww_mutex_trylock(&crtc->mutex.mutex))continue;if (!crtc->enabled || !crtc->primary)goto crtc_unlock;if (!crtc->state || !crtc->state->active)goto crtc_unlock;plane = crtc->primary;if (!ww_mutex_trylock(&plane->mutex.mutex))goto crtc_unlock;/** TODO: Should we check plane_state->visible?* It is not set on vc4I think we should. We should also check whether that primary is connected to the crtc (some hw you can reassign planes freely between crtc, and the crtc->primary pointer is only used for compat with legacy ioctl).
if (!plane->state || !plane->state->visible)*/if (!plane->state)goto plane_unlock;fb = plane->state->fb;if (!fb || !fb->funcs->panic_vmap)Docs says this is optional, doesn't seem to be all that optional. I'd check for this or a driver-specific ->panic_draw_xy instead.
goto plane_unlock;/** fbcon puts out panic messages so stay away to avoid jumbled* output. If vc->vc_mode != KD_TEXT fbcon won't put out* messages (see vt_console_print).*/if (dev->fb_helper && dev->fb_helper->fb == fb)This is a bit a layering violation. Could we instead tell fbcon that it shouldn't do panic handling for drm drivers? I think that would resolve this overlap here in a much cleaner way. drm fbdev helpers already have lots of oops_in_progress checks all over to make sure fbcon doesn't do anything bad. That only leaves the actual rendering, which I think we can stop too with a simple flag.
Ofc only for atomic drivers which have this panic handling mode here implemented.
There used to be a fbdev flag FBINFO_CAN_FORCE_OUTPUT that controlled vc->vc_panic_force_write, but it's gone now I see.
I killed that :-)
Looking at those patches again, it's not what we wanted. It was used to force panic display even when not in KD_TEXT mode.
What we want instead here is a flag to tell fbcon/vt to _never_ write dmesg to our console, not even when the console is in KD_TEXT mode. Because we have a separate panic handler to display it. Heck maybe that QR code thing could be resurrected eventually again.
Totally untested snippet below is what I'm thinking of:
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 7f4c22a65f63..b08c63286ed0 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -282,6 +282,9 @@ static inline int fbcon_is_inactive(struct vc_data *vc, struct fb_info *info) { struct fbcon_ops *ops = info->fbcon_par;
+ if (oops_in_progress && info->flags & NO_OOPS_OUTPUT) + return false; + return (info->state != FBINFO_STATE_RUNNING || vc->vc_mode != KD_TEXT || ops->graphics); }
Plus then unconditionally rendering the oops output on the drm side, even if the current fb is the fbcon one. -Daniel