Message ID | 20201002075620.4157591-1-daniel.vetter@ffwll.ch (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/atomic: Make the kerneldoc a bit clearer | expand |
Hi, 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@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@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@kernel.org> But maybe we should add some lockdep assertion to make sure we can catch someone actually doing this?
On Fri, Oct 2, 2020 at 2:31 PM Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > 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@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@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@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 :-) -Daniel
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@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@intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Maxime Ripard <mripard@kernel.org> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > Cc: David Airlie <airlied@linux.ie> > > > Cc: Daniel Vetter <daniel@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@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
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) {
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@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/drm_atomic.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)