Re: [PATCH] drm/atomic: Make the kerneldoc a bit clearer
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
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> Cc: David Airlie
> Cc: Daniel Vetter
> ---
> 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
But maybe we should add some lockdep assertion to make sure we can catch
someone actually doing this?
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Make the kerneldoc a bit clearer
On Fri, Oct 02, 2020 at 02:37:25PM +0200, Daniel Vetter wrote:
> On Fri, Oct 2, 2020 at 2:31 PM Maxime Ripard 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
> > > Cc: Maarten Lankhorst
> > > Cc: Maxime Ripard
> > > Cc: Thomas Zimmermann
> > > Cc: David Airlie
> > > Cc: Daniel Vetter
> > > ---
> > > 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
>
> 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
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Make the kerneldoc a bit clearer
On Fri, Oct 2, 2020 at 2:31 PM Maxime Ripard 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
> > Cc: Maarten Lankhorst
> > Cc: Maxime Ripard
> > Cc: Thomas Zimmermann
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > ---
> > 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
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
--
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
[PATCH] drm/atomic: Make the kerneldoc a bit clearer
Crank up the warning a notch and point at the right set of locking
functions for atomic drivers.
Signed-off-by: Daniel Vetter
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: Thomas Zimmermann
Cc: David Airlie
Cc: Daniel Vetter
---
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)
{
--
2.28.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
