From: Daniel Vetter <daniel@ffwll.ch>
To: "Noralf Trønnes" <noralf@tronnes.org>
Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
darwish.07@gmail.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 1/3] drm: Add support for panic message output
Date: Tue, 12 Mar 2019 10:53:20 +0100 [thread overview]
Message-ID: <20190312095320.GX2665@phenom.ffwll.local> (raw)
In-Reply-To: <853268c4-8cb4-8f95-b0c2-6e8226e8186e@tronnes.org>
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 vc4
> >>
> >> I 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-03-12 9:53 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-11 17:42 [PATCH v2 0/3] drm: Add panic handling Noralf Trønnes
2019-03-11 17:42 ` [PATCH v2 1/3] drm: Add support for panic message output Noralf Trønnes
2019-03-11 19:23 ` Daniel Vetter
2019-03-11 19:29 ` Daniel Vetter
2019-03-11 22:40 ` Noralf Trønnes
2019-03-12 9:53 ` Daniel Vetter [this message]
2019-03-12 9:59 ` Daniel Vetter
2019-03-11 22:33 ` Noralf Trønnes
2019-03-12 10:58 ` Daniel Vetter
2019-03-12 13:29 ` Noralf Trønnes
2019-03-13 3:53 ` Ahmed S. Darwish
2019-03-12 22:13 ` Ahmed S. Darwish
2019-03-13 7:49 ` John Ogness
2019-03-13 8:37 ` Daniel Vetter
2019-03-14 2:51 ` Ahmed S. Darwish
2019-03-14 9:32 ` Daniel Vetter
2019-03-14 9:43 ` John Ogness
2019-03-14 9:52 ` John Ogness
2019-03-15 10:56 ` Daniel Vetter
2019-03-13 8:35 ` Daniel Vetter
2019-03-14 4:45 ` Ahmed S. Darwish
2019-03-14 9:35 ` Daniel Vetter
2019-03-13 10:24 ` Noralf Trønnes
2019-03-13 4:05 ` Ahmed S. Darwish
2019-03-11 19:55 ` Sam Ravnborg
2019-03-12 10:47 ` Michel Dänzer
2019-03-12 16:17 ` [Intel-gfx] " Ville Syrjälä
2019-03-12 17:15 ` Noralf Trønnes
2019-03-12 17:25 ` Ville Syrjälä
2019-03-12 17:37 ` Noralf Trønnes
2019-03-12 17:44 ` Noralf Trønnes
2019-03-12 18:02 ` [Intel-gfx] " Ville Syrjälä
2019-03-13 8:29 ` Christian König
2019-03-13 8:43 ` [Intel-gfx] " Daniel Vetter
2019-03-13 9:35 ` Michel Dänzer
2019-03-13 13:31 ` [Intel-gfx] " Ville Syrjälä
2019-03-13 13:37 ` Christian König
2019-03-13 15:38 ` Michel Dänzer
2019-03-13 15:54 ` [Intel-gfx] " Christian König
2019-03-13 16:16 ` Kazlauskas, Nicholas
2019-03-13 17:30 ` Koenig, Christian
2019-03-13 17:33 ` Michel Dänzer
2019-03-13 17:41 ` Kazlauskas, Nicholas
2019-03-14 9:50 ` Daniel Vetter
2019-03-14 12:44 ` Kazlauskas, Nicholas
2019-03-15 10:58 ` [Intel-gfx] " Daniel Vetter
2019-03-13 17:52 ` Koenig, Christian
2019-03-14 9:40 ` [Intel-gfx] " Daniel Vetter
2019-03-11 17:42 ` [PATCH v2 2/3] drm/cma-helper: Add support for panic screen Noralf Trønnes
2019-03-11 17:42 ` [PATCH v2 3/3] drm/vc4: Support " Noralf Trønnes
2019-03-11 18:53 ` [PATCH v2 0/3] drm: Add panic handling Daniel Vetter
2019-03-17 23:06 ` Ahmed S. Darwish
2019-03-25 8:42 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190312095320.GX2665@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=daniel.vetter@ffwll.ch \
--cc=darwish.07@gmail.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=noralf@tronnes.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).