Re: [PATCH v7 2/9] drm/panic: Add a drm panic handler

From: Daniel Vetter <daniel@ffwll.ch>
To: Jocelyn Falempe <jfalempe@redhat.com>
Cc: bluescreen_avenger@verizon.net, Daniel Vetter <daniel@ffwll.ch>,
	javierm@redhat.com, mripard@kernel.org, gpiccoli@igalia.com,
	noralf@tronnes.org, dri-devel@lists.freedesktop.org,
	tzimmermann@suse.de, airlied@redhat.com
Subject: Re: [PATCH v7 2/9] drm/panic: Add a drm panic handler
Date: Thu, 18 Jan 2024 14:36:55 +0100	[thread overview]
Message-ID: <Zakpd3Q-OM-PlVGz@phenom.ffwll.local> (raw)
In-Reply-To: <31d0f162-421e-4b42-8b3f-bc3e6c852eb0@redhat.com>

On Tue, Jan 16, 2024 at 11:54:42AM +0100, Jocelyn Falempe wrote:
> On 12/01/2024 14:31, Daniel Vetter wrote:
> > You need to tie these nice kerneldocs into the overall documentation tree,
> > or they're not getting built. Please then also check that all the links
> > and formatting works correctly.
> 
> oh yes, I need to add it to the Documentation/drm/xx.rst.
> I'm not sure where drm panic fits there.
> If I move get_scanout_buffer() pointer to the drm_mode_config struct, adding
> drm_panic docs to drm-kms.rst makes sense ?

Yeah I think a section in the modeset docs (not the modeset helper docs,
this is fairly core concept imo) makes the most sense.

> > The locking here is busted. Which is why the atomic notifier chain is
> > special and uses atomic semantics - I would just avoid this issue and
> > directly register each drm_panic_device instead of trying to maintain a
> > drm-local list and getting the locking rules wrong.
> 
> It's a bit complex to get the drm_device pointer in the callback.
> I think I can add a "struct notifier_block" in the struct drm_mode_config,
> and use the container_of macro, to retrieve it.
> This also makes it easy to unregister.

Yeah if you register each drm_device separately you also need to embedded
the notifier chain into every single one. drm_mode_config sounds ok for
that to me.

I didn't realize that the kmsg_dumper api doesn't have some kind of void*
argument but you need to embed the notifier in the right place, in case
some of my other comments confused you.
-Sima

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch