Re: [PATCH v2 2/2] drm/tests: Add a unit test for range bias allocation

[Matthew Auld <matthew.auld@xxxxxxxxx>]

On 13/05/2024 16:11, Paneer Selvam, Arunpravin wrote:
> Hi Matthew,
>
> On 5/13/2024 1:49 PM, Matthew Auld wrote:
> > On 12/05/2024 08:59, Arunpravin Paneer Selvam wrote:
> > > Allocate cleared blocks in the bias range when the DRM
> > > buddy's clear avail is zero. This will validate the bias
> > > range allocation in scenarios like system boot when no
> > > cleared blocks are available and exercise the fallback
> > > path too. The resulting blocks should always be dirty.
> > >
> > > Signed-off-by: Arunpravin Paneer Selvam 
> > > <Arunpravin.PaneerSelvam@xxxxxxx>
> > > ---
> > >   drivers/gpu/drm/tests/drm_buddy_test.c | 35 ++++++++++++++++++++++++++
> > >   1 file changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c 
> > > b/drivers/gpu/drm/tests/drm_buddy_test.c
> > > index e3b50e240d36..a194f271bc55 100644
> > > --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> > > +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> > > @@ -26,6 +26,8 @@ static void drm_test_buddy_alloc_range_bias(struct 
> > > kunit *test)
> > >       u32 mm_size, ps, bias_size, bias_start, bias_end, bias_rem;
> > >       DRM_RND_STATE(prng, random_seed);
> > >       unsigned int i, count, *order;
> > > +    struct drm_buddy_block *block;
> > > +    unsigned long flags;
> > >       struct drm_buddy mm;
> > >       LIST_HEAD(allocated);
> > >   @@ -222,6 +224,39 @@ static void 
> > > drm_test_buddy_alloc_range_bias(struct kunit *test)
> > >         drm_buddy_free_list(&mm, &allocated, 0);
> > >       drm_buddy_fini(&mm);
> > > +
> > > +    /*
> > > +     * Allocate cleared blocks in the bias range when the DRM 
> > > buddy's clear avail is
> > > +     * zero. This will validate the bias range allocation in 
> > > scenarios like system boot
> > > +     * when no cleared blocks are available and exercise the 
> > > fallback path too. The resulting
> > > +     * blocks should always be dirty.
> > > +     */
> > > +
> > > +    KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_init(&mm, mm_size, ps),
> > > +                   "buddy_init failed\n");
> > > +    mm.clear_avail = 0;
> >
> > Should already be zero, right? Maybe make this an assert instead?
> No, since the mm declared as a local variable in the test case, 
> mm.clear_avail is not zero.

That sounds like a bug IMO. The init() should initialize it, like it 
does for mm.avail and everything else.

> > > +
> > > +    bias_start = round_up(prandom_u32_state(&prng) % (mm_size - ps), 
> > > ps);
> > > +    bias_end = round_up(bias_start + prandom_u32_state(&prng) % 
> > > (mm_size - bias_start), ps);
> > > +    bias_end = max(bias_end, bias_start + ps);
> > > +    bias_rem = bias_end - bias_start;
> > > +
> > > +    flags = DRM_BUDDY_CLEAR_ALLOCATION | DRM_BUDDY_RANGE_ALLOCATION;
> > > +    u32 size = max(round_up(prandom_u32_state(&prng) % bias_rem, 
> > > ps), ps);
> >
> > u32 declaration should be moved to above?
> Sure.
>
> Thanks,
> Arun.
> > Otherwise,
> > Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> >
> > > +
> > > +    KUNIT_ASSERT_FALSE_MSG(test,
> > > +                   drm_buddy_alloc_blocks(&mm, bias_start,
> > > +                              bias_end, size, ps,
> > > +                              &allocated,
> > > +                              flags),
> > > +                   "buddy_alloc failed with bias(%x-%x), size=%u, 
> > > ps=%u\n",
> > > +                   bias_start, bias_end, size, ps);
> > > +
> > > +    list_for_each_entry(block, &allocated, link)
> > > +        KUNIT_EXPECT_EQ(test, drm_buddy_block_is_clear(block), false);
> > > +
> > > +    drm_buddy_free_list(&mm, &allocated, 0);
> > > +    drm_buddy_fini(&mm);
> > >   }
> > >     static void drm_test_buddy_alloc_clear(struct kunit *test)