| Message ID | 20231129221412.1180549-3-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/managed: Add drmm_release_action | expand |
Hi, Thanks for creating a test for that, that's really appreciated :) On Wed, Nov 29, 2023 at 11:14:12PM +0100, Michał Winiarski wrote: > Add a simple test that checks whether the action is indeed called right > away and that it is not called on the final drm_dev_put(). > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > --- > drivers/gpu/drm/tests/drm_managed_test.c | 65 ++++++++++++++++++------ > 1 file changed, 50 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c > index 1652dca11d30c..a645ea42aee56 100644 > --- a/drivers/gpu/drm/tests/drm_managed_test.c > +++ b/drivers/gpu/drm/tests/drm_managed_test.c > @@ -12,6 +12,8 @@ > #define TEST_TIMEOUT_MS 100 > > struct managed_test_priv { > + struct drm_device *drm; > + struct device *dev; > bool action_done; > wait_queue_head_t action_wq; > }; > @@ -26,42 +28,75 @@ static void drm_action(struct drm_device *drm, void *ptr) > > static void drm_test_managed_run_action(struct kunit *test) > { > - struct managed_test_priv *priv; > - struct drm_device *drm; > - struct device *dev; > + struct managed_test_priv *priv = test->priv; > int ret; > > - priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL); > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); > - init_waitqueue_head(&priv->action_wq); > + ret = drmm_add_action_or_reset(priv->drm, drm_action, priv); > + KUNIT_EXPECT_EQ(test, ret, 0); > > - dev = drm_kunit_helper_alloc_device(test); > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); > + ret = drm_dev_register(priv->drm, 0); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + drm_dev_unregister(priv->drm); > + drm_kunit_helper_free_device(test, priv->dev); I think we'll need two patches here, one to convert to having an init function, and one to actually add the new test, it's pretty confusing as it is. > > - drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0, DRIVER_MODESET); > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm); > + ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done, > + msecs_to_jiffies(TEST_TIMEOUT_MS)); > + KUNIT_EXPECT_GT(test, ret, 0); > +} > > - ret = drmm_add_action_or_reset(drm, drm_action, priv); > +static void drm_test_managed_release_action(struct kunit *test) > +{ > + struct managed_test_priv *priv = test->priv; > + int ret; > + > + ret = drmm_add_action_or_reset(priv->drm, drm_action, priv); > KUNIT_EXPECT_EQ(test, ret, 0); > > - ret = drm_dev_register(drm, 0); > + ret = drm_dev_register(priv->drm, 0); > KUNIT_ASSERT_EQ(test, ret, 0); > > - drm_dev_unregister(drm); > - drm_kunit_helper_free_device(test, dev); > + drmm_release_action(priv->drm, drm_action, priv); > + KUNIT_EXPECT_TRUE(test, priv->action_done); > + priv->action_done = false; > + > + drm_dev_unregister(priv->drm); > + drm_kunit_helper_free_device(test, priv->dev); > > ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done, > msecs_to_jiffies(TEST_TIMEOUT_MS)); > - KUNIT_EXPECT_GT(test, ret, 0); > + KUNIT_EXPECT_EQ(test, ret, 0); > +} > + > +static int drm_managed_test_init(struct kunit *test) > +{ > + struct managed_test_priv *priv; > + > + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); > + init_waitqueue_head(&priv->action_wq); Also, I know that it was there before, but I'm not sure it's valid from a lifetime point of view. Or at least, we have to think hard enough about it to just remove that construct > + priv->dev = drm_kunit_helper_alloc_device(test); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev); > + > + priv->drm = __drm_kunit_helper_alloc_drm_device(test, priv->dev, sizeof(*priv->drm), > + 0, DRIVER_MODESET); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm); For example by storing the drm_device struct in the priv structure directly, and thus everything will just work out. Maxime
On Thu, Nov 30, 2023 at 09:38:35AM +0100, Maxime Ripard wrote: > Hi, > > Thanks for creating a test for that, that's really appreciated :) > > On Wed, Nov 29, 2023 at 11:14:12PM +0100, Michał Winiarski wrote: > > Add a simple test that checks whether the action is indeed called right > > away and that it is not called on the final drm_dev_put(). > > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > --- > > drivers/gpu/drm/tests/drm_managed_test.c | 65 ++++++++++++++++++------ > > 1 file changed, 50 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c > > index 1652dca11d30c..a645ea42aee56 100644 > > --- a/drivers/gpu/drm/tests/drm_managed_test.c > > +++ b/drivers/gpu/drm/tests/drm_managed_test.c > > @@ -12,6 +12,8 @@ > > #define TEST_TIMEOUT_MS 100 > > > > struct managed_test_priv { > > + struct drm_device *drm; > > + struct device *dev; > > bool action_done; > > wait_queue_head_t action_wq; > > }; > > @@ -26,42 +28,75 @@ static void drm_action(struct drm_device *drm, void *ptr) > > > > static void drm_test_managed_run_action(struct kunit *test) > > { > > - struct managed_test_priv *priv; > > - struct drm_device *drm; > > - struct device *dev; > > + struct managed_test_priv *priv = test->priv; > > int ret; > > > > - priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL); > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); > > - init_waitqueue_head(&priv->action_wq); > > + ret = drmm_add_action_or_reset(priv->drm, drm_action, priv); > > + KUNIT_EXPECT_EQ(test, ret, 0); > > > > - dev = drm_kunit_helper_alloc_device(test); > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); > > + ret = drm_dev_register(priv->drm, 0); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + drm_dev_unregister(priv->drm); > > + drm_kunit_helper_free_device(test, priv->dev); > > I think we'll need two patches here, one to convert to having an init > function, and one to actually add the new test, it's pretty confusing as > it is. > > > > > - drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0, DRIVER_MODESET); > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm); > > + ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done, > > + msecs_to_jiffies(TEST_TIMEOUT_MS)); > > + KUNIT_EXPECT_GT(test, ret, 0); > > +} > > > > - ret = drmm_add_action_or_reset(drm, drm_action, priv); > > +static void drm_test_managed_release_action(struct kunit *test) > > +{ > > + struct managed_test_priv *priv = test->priv; > > + int ret; > > + > > + ret = drmm_add_action_or_reset(priv->drm, drm_action, priv); > > KUNIT_EXPECT_EQ(test, ret, 0); > > > > - ret = drm_dev_register(drm, 0); > > + ret = drm_dev_register(priv->drm, 0); > > KUNIT_ASSERT_EQ(test, ret, 0); > > > > - drm_dev_unregister(drm); > > - drm_kunit_helper_free_device(test, dev); > > + drmm_release_action(priv->drm, drm_action, priv); > > + KUNIT_EXPECT_TRUE(test, priv->action_done); > > + priv->action_done = false; > > + > > + drm_dev_unregister(priv->drm); > > + drm_kunit_helper_free_device(test, priv->dev); > > > > ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done, > > msecs_to_jiffies(TEST_TIMEOUT_MS)); > > - KUNIT_EXPECT_GT(test, ret, 0); > > + KUNIT_EXPECT_EQ(test, ret, 0); > > +} > > + > > +static int drm_managed_test_init(struct kunit *test) > > +{ > > + struct managed_test_priv *priv; > > + > > + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); > > + init_waitqueue_head(&priv->action_wq); > > Also, I know that it was there before, but I'm not sure it's valid from > a lifetime point of view. Or at least, we have to think hard enough > about it to just remove that construct > > > + priv->dev = drm_kunit_helper_alloc_device(test); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev); > > + > > + priv->drm = __drm_kunit_helper_alloc_drm_device(test, priv->dev, sizeof(*priv->drm), > > + 0, DRIVER_MODESET); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm); > > For example by storing the drm_device struct in the priv structure > directly, and thus everything will just work out. Sure, makes sense, I'll include it in the patch that moves device alloc to .init(). Thanks, -Michał > > Maxime
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c index 1652dca11d30c..a645ea42aee56 100644 --- a/drivers/gpu/drm/tests/drm_managed_test.c +++ b/drivers/gpu/drm/tests/drm_managed_test.c @@ -12,6 +12,8 @@ #define TEST_TIMEOUT_MS 100 struct managed_test_priv { + struct drm_device *drm; + struct device *dev; bool action_done; wait_queue_head_t action_wq; }; @@ -26,42 +28,75 @@ static void drm_action(struct drm_device *drm, void *ptr) static void drm_test_managed_run_action(struct kunit *test) { - struct managed_test_priv *priv; - struct drm_device *drm; - struct device *dev; + struct managed_test_priv *priv = test->priv; int ret; - priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL); - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); - init_waitqueue_head(&priv->action_wq); + ret = drmm_add_action_or_reset(priv->drm, drm_action, priv); + KUNIT_EXPECT_EQ(test, ret, 0); - dev = drm_kunit_helper_alloc_device(test); - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); + ret = drm_dev_register(priv->drm, 0); + KUNIT_ASSERT_EQ(test, ret, 0); + + drm_dev_unregister(priv->drm); + drm_kunit_helper_free_device(test, priv->dev); - drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0, DRIVER_MODESET); - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm); + ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done, + msecs_to_jiffies(TEST_TIMEOUT_MS)); + KUNIT_EXPECT_GT(test, ret, 0); +} - ret = drmm_add_action_or_reset(drm, drm_action, priv); +static void drm_test_managed_release_action(struct kunit *test) +{ + struct managed_test_priv *priv = test->priv; + int ret; + + ret = drmm_add_action_or_reset(priv->drm, drm_action, priv); KUNIT_EXPECT_EQ(test, ret, 0); - ret = drm_dev_register(drm, 0); + ret = drm_dev_register(priv->drm, 0); KUNIT_ASSERT_EQ(test, ret, 0); - drm_dev_unregister(drm); - drm_kunit_helper_free_device(test, dev); + drmm_release_action(priv->drm, drm_action, priv); + KUNIT_EXPECT_TRUE(test, priv->action_done); + priv->action_done = false; + + drm_dev_unregister(priv->drm); + drm_kunit_helper_free_device(test, priv->dev); ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done, msecs_to_jiffies(TEST_TIMEOUT_MS)); - KUNIT_EXPECT_GT(test, ret, 0); + KUNIT_EXPECT_EQ(test, ret, 0); +} + +static int drm_managed_test_init(struct kunit *test) +{ + struct managed_test_priv *priv; + + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); + init_waitqueue_head(&priv->action_wq); + + priv->dev = drm_kunit_helper_alloc_device(test); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev); + + priv->drm = __drm_kunit_helper_alloc_drm_device(test, priv->dev, sizeof(*priv->drm), + 0, DRIVER_MODESET); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm); + + test->priv = priv; + + return 0; } static struct kunit_case drm_managed_tests[] = { KUNIT_CASE(drm_test_managed_run_action), + KUNIT_CASE(drm_test_managed_release_action), {} }; static struct kunit_suite drm_managed_test_suite = { .name = "drm-test-managed", + .init = drm_managed_test_init, .test_cases = drm_managed_tests };
Add a simple test that checks whether the action is indeed called right away and that it is not called on the final drm_dev_put(). Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> --- drivers/gpu/drm/tests/drm_managed_test.c | 65 ++++++++++++++++++------ 1 file changed, 50 insertions(+), 15 deletions(-)