[Intel-gfx] [PATCH 2/2] drm/mm: Pack allocated/scanned boolean into a bitfield

[Chris Wilson]

Quoting Ruhl, Michael J (2019-09-16 20:45:14)
> >-----Original Message-----
> >From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf
> >Of Chris Wilson
> >Sent: Sunday, September 15, 2019 2:46 PM
> >@@ -424,9 +424,9 @@ int drm_mm_reserve_node(struct drm_mm *mm,
> >struct drm_mm_node *node)
> >
> >       node->mm = mm;
> >
> >+      __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);
> 
> Maybe a silly question(s), but you appear to be mixing atomic bit ops
> (clear_ and test_) with non-atomic bit ops __xxx_bit().
> 
> Should that be a concern?   Should that be mention in the commit?

Generally yes, but this is inside an allocation function so the new node
cannot be accessed concurrently yet (and manipulation of the drm_mm
itself requires external serialisation).

The concern is with blocks like

> >diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> >index f1f0a7c87771..b00e20f5ce05 100644
> >--- a/drivers/gpu/drm/vc4/vc4_crtc.c
> >+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> >@@ -994,7 +994,7 @@ static void vc4_crtc_destroy_state(struct drm_crtc
> >*crtc,
> >       struct vc4_dev *vc4 = to_vc4_dev(crtc->dev);
> >       struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
> >
> >-      if (vc4_state->mm.allocated) {
> >+      if (drm_mm_node_allocated(&vc4_state->mm)) {
> >               unsigned long flags;
> >
> >               spin_lock_irqsave(&vc4->hvs->mm_lock, flags);

where we are testing the bit prior to taking the lock to serialise
removal before free. To avoid the cost of serialising here we have to be
sure that any other thread has completely stopped using the drm_mm_node
when it is marked as released.
-Chris