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

[Ruhl, Michael J]

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf
>Of Chris Wilson
>Sent: Thursday, October 3, 2019 3:08 AM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>; dri-
>devel at lists.freedesktop.org
>Cc: intel-gfx at lists.freedesktop.org
>Subject: RE: [PATCH 2/2] drm/mm: Pack allocated/scanned boolean into a
>bitfield
>
>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).

Got it. 

Thanks for the clarification.

Mike


>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 at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel