Re: [PATCH v2 1/3] drm: Add support for panic message output

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Mar 12, 2019 at 10:53:20AM +0100, Daniel Vetter wrote:
> 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.

Hm thinking about this some more, I think this would allow us to replace
all the oops_in_progress checks we have in drm_fb_helper.c with
WARN_ON(oops_in_progress). So worthy cleanup on its own.

Assuming I'm seeing this right ofc, very much possible I'm missing
something :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel