[PATCH] drm/atomic: Make the kerneldoc a bit clearer

[Maxime Ripard]

On Fri, Oct 02, 2020 at 02:37:25PM +0200, Daniel Vetter wrote:
> On Fri, Oct 2, 2020 at 2:31 PM Maxime Ripard <maxime at cerno.tech> wrote:
> > On Fri, Oct 02, 2020 at 09:56:20AM +0200, Daniel Vetter wrote:
> > > Crank up the warning a notch and point at the right set of locking
> > > functions for atomic drivers.
> > >
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > Cc: Maxime Ripard <mripard at kernel.org>
> > > Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > > Cc: David Airlie <airlied at linux.ie>
> > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index aac9122f1da2..b2d20eb6c807 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1642,11 +1642,11 @@ static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p,
> > >   * to dmesg in case of error irq's.  (Hint, you probably want to
> > >   * ratelimit this!)
> > >   *
> > > - * The caller must drm_modeset_lock_all(), or if this is called
> > > - * from error irq handler, it should not be enabled by default.
> > > - * (Ie. if you are debugging errors you might not care that this
> > > - * is racey.  But calling this without all modeset locks held is
> > > - * not inherently safe.)
> > > + * The caller must wrap this drm_modeset_lock_all_ctx() and
> > > + * drm_modeset_drop_locks(). If this is called from error irq handler, it should
> > > + * not be enabled by default - if you are debugging errors you might
> > > + * not care that this is racey, but calling this without all modeset locks held
> > > + * is inherently unsafe.
> > >   */
> > >  void drm_state_dump(struct drm_device *dev, struct drm_printer *p)
> > >  {
> >
> > For the comment itself:
> > Acked-by: Maxime Ripard <mripard at kernel.org>
> 
> Thanks for taking a look, will merge.
> 
> > But maybe we should add some lockdep assertion to make sure we can catch
> > someone actually doing this?
> 
> I think it has some use for ad-hoc debugging in random places, or
> maybe as a an opt-in "tains your kernel" debug option. And then you
> really don't want your useful debug output burried in a few pages of
> lockdep splat first :-)

Yeah, but at the same time reducing the discoverability of this locking
expectation for the common case to favor some one-off debugging by
someone that has more chance to know better seems like not the best
trade-off.

Or maybe make the assertion conditional on drm.debug not being set or
something?

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20201002/8ae02654/attachment-0001.sig>