Quoting Ruhl, Michael J (2019-09-16 20:45:14) > >-----Original Message----- > >From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
