On 2023-05-15 at 19:24:07 +0200, Marco Pagani wrote:
>
>
> On 2023-05-13 19:40, Xu Yilun wrote:
> > On 2023-05-11 at 16:19:22 +0200, Marco Pagani wrote:
> >> Introduce initial KUnit tests for the FPGA subsystem. Tests are organized
> >> into three test suites. The first suite tests the FPGA Manager.
> >> The second suite tests the FPGA Bridge. Finally, the last test suite
> >> models a complete FPGA platform and tests static and partial reconfiguration.
> >>
> >> Signed-off-by: Marco Pagani <marpagan@xxxxxxxxxx>
>
> [...]
>
> >> +static void fpga_bridge_test_get_put_list(struct kunit *test)
> >> +{
> >> + struct list_head bridge_list;
> >> + struct fake_fpga_bridge *bridge_0_ctx, *bridge_1_ctx;
> >> + int ret;
> >> +
> >> + bridge_0_ctx = test->priv;
> >> +
> >> + /* Register another bridge for this test */
> >> + bridge_1_ctx = fake_fpga_bridge_register(test, NULL);
> >> + KUNIT_ASSERT_FALSE(test, IS_ERR(bridge_1_ctx));
> >
> > I think bridge_1 could also be initialized in test_init together with
> > bridge_0
>
> I can do it, but it would remain unused in the previous test case.
>
> >> +
> >> + INIT_LIST_HEAD(&bridge_list);
> >> +
> >> + /* Get bridge_0 and add it to the list */
> >> + ret = fpga_bridge_get_to_list(bridge_1_ctx->bridge->dev.parent, NULL,
> >> + &bridge_list);
> >> + KUNIT_EXPECT_EQ(test, ret, 0);
> >> +
> >> + KUNIT_EXPECT_PTR_EQ(test, bridge_1_ctx->bridge,
> >> + list_first_entry_or_null(&bridge_list, struct fpga_bridge, node));
> >
> > Should operate on bridge_0_ctx?
>
> Yes, sorry. Code and comments are reversed. I'll fix it in the next version.
>
> >> +
> >> + /* Get bridge_1 and add it to the list */
> >> + ret = fpga_bridge_get_to_list(bridge_0_ctx->bridge->dev.parent, NULL,
> >> + &bridge_list);
> >> + KUNIT_EXPECT_EQ(test, ret, 0);
> >> +
> >> + KUNIT_EXPECT_PTR_EQ(test, bridge_0_ctx->bridge,
> >> + list_first_entry_or_null(&bridge_list, struct fpga_bridge, node));
> >
> > Should operate on bridge_1_ctx?
>
> Same.
>
> >> +
> >> + /* Disable an then enable both bridges from the list */
> >> + KUNIT_EXPECT_TRUE(test, bridge_0_ctx->stats.enable);
> >
> > Why expect enable without fpga_bridges_enable()?
>
> To check that the bridge is initialized in the correct (enabled) state.
>
> [...]
>
> >> +static void fpga_test_partial_rcfg(struct kunit *test)
> >> +{
> >> + struct fpga_base_ctx *base_ctx;
> >> + struct fake_fpga_region *sub_region_0_ctx, *sub_region_1_ctx;
> >> + struct fake_fpga_bridge *sub_bridge_0_ctx, *sub_bridge_1_ctx;
> >> + struct fpga_image_info *partial_img_info;
> >> + int ret;
> >> +
> >> + base_ctx = test->priv;
> >> +
> >> + /*
> >> + * Add two reconfigurable sub-regions, each controlled by a bridge. The
> >> + * reconfigurable sub-region are children of their bridges which are,
> >> + * in turn, children of the base region. For simplicity, the same image
> >> + * is used to configure reconfigurable regions
> >> + */
> >> + sub_bridge_0_ctx = fake_fpga_bridge_register(test,
> >> + &base_ctx->region_ctx->region->dev);
> >> + KUNIT_ASSERT_FALSE(test, IS_ERR(sub_bridge_0_ctx));
> >> +
> >> + sub_region_0_ctx = fake_fpga_region_register(test, base_ctx->mgr_ctx->mgr,
> >> + &sub_bridge_0_ctx->bridge->dev);
> >> + KUNIT_ASSERT_FALSE(test, IS_ERR(sub_region_0_ctx));
> >> +
> >> + ret = fake_fpga_region_add_bridge(sub_region_0_ctx, sub_bridge_0_ctx->bridge);
> >> + KUNIT_ASSERT_EQ(test, ret, 0);
> >> +
> >> + sub_bridge_1_ctx = fake_fpga_bridge_register(test,
> >> + &base_ctx->region_ctx->region->dev);
> >> + KUNIT_ASSERT_FALSE(test, IS_ERR(sub_bridge_1_ctx));
> >> +
> >> + sub_region_1_ctx = fake_fpga_region_register(test, base_ctx->mgr_ctx->mgr,
> >> + &sub_bridge_1_ctx->bridge->dev);
> >> + KUNIT_ASSERT_FALSE(test, IS_ERR(sub_region_1_ctx));
> >> +
> >> + ret = fake_fpga_region_add_bridge(sub_region_1_ctx, sub_bridge_1_ctx->bridge);
> >> + KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > I'm wondering if we need to construct the topology for partial
> > reconfiguration test. The FPGA core doesn't actually check the topology.
> > It is OK to do partial reconfiguration for a region without parents as
> > long as its associated FPGA manager device has the capability.
> >
> > Thanks,
> > Yilun
>
> I agree with you. Creating a hierarchical layout is rather unnecessary.
>
I assume the following sections have nothing to do with hierarchial
layout, is it?
> Initially, the idea was to test that all components behave as expected
> in a complete setup, e.g., only the bridge of the specific reconfigurable
> region gets disabled during programming and then re-enabled.
>
> However, after some iterations, I'm starting to think that it would be
> better to restructure the whole test code into a set of self-contained
> test modules, one for each core component.
>
> In that way, each module would contain the implementation of the fake/mock
> low-level driver and the related tests. For instance, the manager module
> would contain the implementation of the fake manager and the test_img_load_buf
> and test_img_load_sgt test cases. Similarly, the bridge module would contain
> the fake/mock bridge implementation and the test_toggle and test_get_put_list
> cases.
>
> I think that in this way, the code would be simpler and more adherent to the
> unit testing methodology. The downside is that making tests that need multiple
> components would be more cumbersome and possibly lead to code duplication.
> For instance, testing the region's fpga_region_program_fpga() would require
> implementing additional local mock/fakes for the manager and bridge.
This way is good to me.
>
> What do you think?
>
> Thanks,
> Marco
>
> [...]
>
