Re: [PATCH 05/10] drm/tests: Add test for drm_framebuffer_cleanup()

2023-09-11 Thread Maxime Ripard 
On Fri, Sep 08, 2023 at 05:42:07PM -0300, Maira Canal wrote:
> Hi Carlos,
> 
> On 9/4/23 14:22, Carlos wrote:
> > Hi Maíra,
> > 
> > On 8/26/23 11:06, Maíra Canal wrote:
> > > Hi Carlos,
> > > 
> > > On 8/25/23 13:07, Carlos Eduardo Gallo Filho wrote:
> > > > Add a single KUnit test case for the drm_framebuffer_cleanup function.
> > > > 
> > > > Signed-off-by: Carlos Eduardo Gallo Filho 
> > > > ---
> > > >   drivers/gpu/drm/tests/drm_framebuffer_test.c | 49 
> > > >   1 file changed, 49 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c
> > > > b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> > > > index 0e0e8216bbbc..16d9cf4bed88 100644
> > > > --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
> > > > +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> > > > @@ -370,6 +370,9 @@ static int drm_framebuffer_test_init(struct
> > > > kunit *test)
> > > >   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock);
> > > >   dev = >dev;
> > > >   +    mutex_init(>mode_config.fb_lock);
> > > 
> > > What about drmm_mutex_init()?
> > I took a look into it and as far I understand it would be useful if
> > the drm_device was allocated with drmm_kalloc(), sure? As far we
> 
> I'm not sure if we can allocate drm_device with drmm_kzalloc(), as we
> need a DRM device for drmm_kzalloc(). drmm_kzalloc() is just a
> _device managed version of kzalloc().

We should use drm_kunit_helper_alloc_drm_device_with_driver

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 05/10] drm/tests: Add test for drm_framebuffer_cleanup()

2023-09-08 Thread Maira Canal 

Hi Carlos,

On 9/4/23 14:22, Carlos wrote:

Hi Maíra,

On 8/26/23 11:06, Maíra Canal wrote:

Hi Carlos,

On 8/25/23 13:07, Carlos Eduardo Gallo Filho wrote:

Add a single KUnit test case for the drm_framebuffer_cleanup function.

Signed-off-by: Carlos Eduardo Gallo Filho 
---
  drivers/gpu/drm/tests/drm_framebuffer_test.c | 49 
  1 file changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c 
b/drivers/gpu/drm/tests/drm_framebuffer_test.c

index 0e0e8216bbbc..16d9cf4bed88 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -370,6 +370,9 @@ static int drm_framebuffer_test_init(struct kunit 
*test)

  KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock);
  dev = >dev;
  +    mutex_init(>mode_config.fb_lock);


What about drmm_mutex_init()?

I took a look into it and as far I understand it would be useful if
the drm_device was allocated with drmm_kalloc(), sure? As far we


I'm not sure if we can allocate drm_device with drmm_kzalloc(), as we
need a DRM device for drmm_kzalloc(). drmm_kzalloc() is just a
_device managed version of kzalloc().


are using kunit_kalloc here and the drm_device is automatically
deallocated when the test finishes, what would be the better by
using drmm_mutex_init?



Actually, thinking it better now, I think we cannot use
drmm_mutex_init() here, as we are not calling drm_dev_put().

Best Regards,
- Maíra


It isn't that I don't wanna use it, I just didn't understand how
exactly it works and how could I use it in that code. Should I
replace the drm_device allocation to use drmm?

Thanks,
Carlos


Best Regards,
- Maíra


+ INIT_LIST_HEAD(>mode_config.fb_list);
+    dev->mode_config.num_fb = 0;
  dev->mode_config.min_width = MIN_WIDTH;
  dev->mode_config.max_width = MAX_WIDTH;
  dev->mode_config.min_height = MIN_HEIGHT;
@@ -380,6 +383,14 @@ static int drm_framebuffer_test_init(struct 
kunit *test)

  return 0;
  }
  +static void drm_framebuffer_test_exit(struct kunit *test)
+{
+    struct drm_mock *mock = test->priv;
+    struct drm_device *dev = >dev;
+
+    mutex_destroy(>mode_config.fb_lock);
+}
+
  static void drm_test_framebuffer_create(struct kunit *test)
  {
  const struct drm_framebuffer_test *params = test->param_value;
@@ -483,7 +494,44 @@ static void check_src_coords_test_to_desc(const 
struct check_src_coords_case *t,

  KUNIT_ARRAY_PARAM(check_src_coords, check_src_coords_cases,
    check_src_coords_test_to_desc);
  +static void drm_test_framebuffer_cleanup(struct kunit *test)
+{
+    struct drm_mock *mock = test->priv;
+    struct drm_device *dev = >dev;
+    struct list_head *fb_list = >mode_config.fb_list;
+    struct drm_framebuffer fb1 = { .dev = dev };
+    struct drm_framebuffer fb2 = { .dev = dev };
+
+    /* This must result on [fb_list] -> fb1 -> fb2 */
+    list_add_tail(, fb_list);
+    list_add_tail(, fb_list);
+    dev->mode_config.num_fb = 2;
+
+    KUNIT_ASSERT_PTR_EQ(test, fb_list->prev, );
+    KUNIT_ASSERT_PTR_EQ(test, fb_list->next, );
+    KUNIT_ASSERT_PTR_EQ(test, fb1.head.prev, fb_list);
+    KUNIT_ASSERT_PTR_EQ(test, fb1.head.next, );
+    KUNIT_ASSERT_PTR_EQ(test, fb2.head.prev, );
+    KUNIT_ASSERT_PTR_EQ(test, fb2.head.next, fb_list);
+
+    drm_framebuffer_cleanup();
+
+    /* Now [fb_list] -> fb2 */
+    KUNIT_ASSERT_PTR_EQ(test, fb_list->prev, );
+    KUNIT_ASSERT_PTR_EQ(test, fb_list->next, );
+    KUNIT_ASSERT_PTR_EQ(test, fb2.head.prev, fb_list);
+    KUNIT_ASSERT_PTR_EQ(test, fb2.head.next, fb_list);
+    KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 1);
+
+    drm_framebuffer_cleanup();
+
+    /* Now fb_list is empty */
+    KUNIT_ASSERT_TRUE(test, list_empty(fb_list));
+    KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 0);
+}
+
  static struct kunit_case drm_framebuffer_tests[] = {
+    KUNIT_CASE(drm_test_framebuffer_cleanup),
KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported),
  KUNIT_CASE_PARAM(drm_test_framebuffer_check_src_coords, 
check_src_coords_gen_params),
  KUNIT_CASE_PARAM(drm_test_framebuffer_create, 
drm_framebuffer_create_gen_params),

@@ -493,6 +541,7 @@ static struct kunit_case drm_framebuffer_tests[] = {
  static struct kunit_suite drm_framebuffer_test_suite = {
  .name = "drm_framebuffer",
  .init = drm_framebuffer_test_init,
+    .exit = drm_framebuffer_test_exit,
  .test_cases = drm_framebuffer_tests,
  };

Re: [PATCH 05/10] drm/tests: Add test for drm_framebuffer_cleanup()

2023-09-04 Thread Carlos 

Hi Maíra,

On 8/26/23 11:06, Maíra Canal wrote:

Hi Carlos,

On 8/25/23 13:07, Carlos Eduardo Gallo Filho wrote:

Add a single KUnit test case for the drm_framebuffer_cleanup function.

Signed-off-by: Carlos Eduardo Gallo Filho 
---
  drivers/gpu/drm/tests/drm_framebuffer_test.c | 49 
  1 file changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c 
b/drivers/gpu/drm/tests/drm_framebuffer_test.c

index 0e0e8216bbbc..16d9cf4bed88 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -370,6 +370,9 @@ static int drm_framebuffer_test_init(struct kunit 
*test)

  KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock);
  dev = >dev;
  +    mutex_init(>mode_config.fb_lock);


What about drmm_mutex_init()?

I took a look into it and as far I understand it would be useful if
the drm_device was allocated with drmm_kalloc(), sure? As far we
are using kunit_kalloc here and the drm_device is automatically
deallocated when the test finishes, what would be the better by
using drmm_mutex_init?

It isn't that I don't wanna use it, I just didn't understand how
exactly it works and how could I use it in that code. Should I
replace the drm_device allocation to use drmm?

Thanks,
Carlos


Best Regards,
- Maíra


+ INIT_LIST_HEAD(>mode_config.fb_list);
+    dev->mode_config.num_fb = 0;
  dev->mode_config.min_width = MIN_WIDTH;
  dev->mode_config.max_width = MAX_WIDTH;
  dev->mode_config.min_height = MIN_HEIGHT;
@@ -380,6 +383,14 @@ static int drm_framebuffer_test_init(struct 
kunit *test)

  return 0;
  }
  +static void drm_framebuffer_test_exit(struct kunit *test)
+{
+    struct drm_mock *mock = test->priv;
+    struct drm_device *dev = >dev;
+
+    mutex_destroy(>mode_config.fb_lock);
+}
+
  static void drm_test_framebuffer_create(struct kunit *test)
  {
  const struct drm_framebuffer_test *params = test->param_value;
@@ -483,7 +494,44 @@ static void check_src_coords_test_to_desc(const 
struct check_src_coords_case *t,

  KUNIT_ARRAY_PARAM(check_src_coords, check_src_coords_cases,
    check_src_coords_test_to_desc);
  +static void drm_test_framebuffer_cleanup(struct kunit *test)
+{
+    struct drm_mock *mock = test->priv;
+    struct drm_device *dev = >dev;
+    struct list_head *fb_list = >mode_config.fb_list;
+    struct drm_framebuffer fb1 = { .dev = dev };
+    struct drm_framebuffer fb2 = { .dev = dev };
+
+    /* This must result on [fb_list] -> fb1 -> fb2 */
+    list_add_tail(, fb_list);
+    list_add_tail(, fb_list);
+    dev->mode_config.num_fb = 2;
+
+    KUNIT_ASSERT_PTR_EQ(test, fb_list->prev, );
+    KUNIT_ASSERT_PTR_EQ(test, fb_list->next, );
+    KUNIT_ASSERT_PTR_EQ(test, fb1.head.prev, fb_list);
+    KUNIT_ASSERT_PTR_EQ(test, fb1.head.next, );
+    KUNIT_ASSERT_PTR_EQ(test, fb2.head.prev, );
+    KUNIT_ASSERT_PTR_EQ(test, fb2.head.next, fb_list);
+
+    drm_framebuffer_cleanup();
+
+    /* Now [fb_list] -> fb2 */
+    KUNIT_ASSERT_PTR_EQ(test, fb_list->prev, );
+    KUNIT_ASSERT_PTR_EQ(test, fb_list->next, );
+    KUNIT_ASSERT_PTR_EQ(test, fb2.head.prev, fb_list);
+    KUNIT_ASSERT_PTR_EQ(test, fb2.head.next, fb_list);
+    KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 1);
+
+    drm_framebuffer_cleanup();
+
+    /* Now fb_list is empty */
+    KUNIT_ASSERT_TRUE(test, list_empty(fb_list));
+    KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 0);
+}
+
  static struct kunit_case drm_framebuffer_tests[] = {
+    KUNIT_CASE(drm_test_framebuffer_cleanup),
KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported),
  KUNIT_CASE_PARAM(drm_test_framebuffer_check_src_coords, 
check_src_coords_gen_params),
  KUNIT_CASE_PARAM(drm_test_framebuffer_create, 
drm_framebuffer_create_gen_params),

@@ -493,6 +541,7 @@ static struct kunit_case drm_framebuffer_tests[] = {
  static struct kunit_suite drm_framebuffer_test_suite = {
  .name = "drm_framebuffer",
  .init = drm_framebuffer_test_init,
+    .exit = drm_framebuffer_test_exit,
  .test_cases = drm_framebuffer_tests,
  };

Re: [PATCH 05/10] drm/tests: Add test for drm_framebuffer_cleanup()

2023-08-26 Thread Maíra Canal 

Hi Carlos,

On 8/25/23 13:07, Carlos Eduardo Gallo Filho wrote:

Add a single KUnit test case for the drm_framebuffer_cleanup function.

Signed-off-by: Carlos Eduardo Gallo Filho 
---
  drivers/gpu/drm/tests/drm_framebuffer_test.c | 49 
  1 file changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c 
b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index 0e0e8216bbbc..16d9cf4bed88 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -370,6 +370,9 @@ static int drm_framebuffer_test_init(struct kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock);
dev = >dev;
  
+	mutex_init(>mode_config.fb_lock);


What about drmm_mutex_init()?

Best Regards,
- Maíra


+   INIT_LIST_HEAD(>mode_config.fb_list);
+   dev->mode_config.num_fb = 0;
dev->mode_config.min_width = MIN_WIDTH;
dev->mode_config.max_width = MAX_WIDTH;
dev->mode_config.min_height = MIN_HEIGHT;
@@ -380,6 +383,14 @@ static int drm_framebuffer_test_init(struct kunit *test)
return 0;
  }
  
+static void drm_framebuffer_test_exit(struct kunit *test)

+{
+   struct drm_mock *mock = test->priv;
+   struct drm_device *dev = >dev;
+
+   mutex_destroy(>mode_config.fb_lock);
+}
+
  static void drm_test_framebuffer_create(struct kunit *test)
  {
const struct drm_framebuffer_test *params = test->param_value;
@@ -483,7 +494,44 @@ static void check_src_coords_test_to_desc(const struct 
check_src_coords_case *t,
  KUNIT_ARRAY_PARAM(check_src_coords, check_src_coords_cases,
  check_src_coords_test_to_desc);
  
+static void drm_test_framebuffer_cleanup(struct kunit *test)

+{
+   struct drm_mock *mock = test->priv;
+   struct drm_device *dev = >dev;
+   struct list_head *fb_list = >mode_config.fb_list;
+   struct drm_framebuffer fb1 = { .dev = dev };
+   struct drm_framebuffer fb2 = { .dev = dev };
+
+   /* This must result on [fb_list] -> fb1 -> fb2 */
+   list_add_tail(, fb_list);
+   list_add_tail(, fb_list);
+   dev->mode_config.num_fb = 2;
+
+   KUNIT_ASSERT_PTR_EQ(test, fb_list->prev, );
+   KUNIT_ASSERT_PTR_EQ(test, fb_list->next, );
+   KUNIT_ASSERT_PTR_EQ(test, fb1.head.prev, fb_list);
+   KUNIT_ASSERT_PTR_EQ(test, fb1.head.next, );
+   KUNIT_ASSERT_PTR_EQ(test, fb2.head.prev, );
+   KUNIT_ASSERT_PTR_EQ(test, fb2.head.next, fb_list);
+
+   drm_framebuffer_cleanup();
+
+   /* Now [fb_list] -> fb2 */
+   KUNIT_ASSERT_PTR_EQ(test, fb_list->prev, );
+   KUNIT_ASSERT_PTR_EQ(test, fb_list->next, );
+   KUNIT_ASSERT_PTR_EQ(test, fb2.head.prev, fb_list);
+   KUNIT_ASSERT_PTR_EQ(test, fb2.head.next, fb_list);
+   KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 1);
+
+   drm_framebuffer_cleanup();
+
+   /* Now fb_list is empty */
+   KUNIT_ASSERT_TRUE(test, list_empty(fb_list));
+   KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 0);
+}
+
  static struct kunit_case drm_framebuffer_tests[] = {
+   KUNIT_CASE(drm_test_framebuffer_cleanup),
KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported),
KUNIT_CASE_PARAM(drm_test_framebuffer_check_src_coords, 
check_src_coords_gen_params),
KUNIT_CASE_PARAM(drm_test_framebuffer_create, 
drm_framebuffer_create_gen_params),
@@ -493,6 +541,7 @@ static struct kunit_case drm_framebuffer_tests[] = {
  static struct kunit_suite drm_framebuffer_test_suite = {
.name = "drm_framebuffer",
.init = drm_framebuffer_test_init,
+   .exit = drm_framebuffer_test_exit,
.test_cases = drm_framebuffer_tests,
  };

[PATCH 05/10] drm/tests: Add test for drm_framebuffer_cleanup()

2023-08-25 Thread Carlos Eduardo Gallo Filho 
Add a single KUnit test case for the drm_framebuffer_cleanup function.

Signed-off-by: Carlos Eduardo Gallo Filho 
---
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 49 
 1 file changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c 
b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index 0e0e8216bbbc..16d9cf4bed88 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -370,6 +370,9 @@ static int drm_framebuffer_test_init(struct kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock);
dev = >dev;
 
+   mutex_init(>mode_config.fb_lock);
+   INIT_LIST_HEAD(>mode_config.fb_list);
+   dev->mode_config.num_fb = 0;
dev->mode_config.min_width = MIN_WIDTH;
dev->mode_config.max_width = MAX_WIDTH;
dev->mode_config.min_height = MIN_HEIGHT;
@@ -380,6 +383,14 @@ static int drm_framebuffer_test_init(struct kunit *test)
return 0;
 }
 
+static void drm_framebuffer_test_exit(struct kunit *test)
+{
+   struct drm_mock *mock = test->priv;
+   struct drm_device *dev = >dev;
+
+   mutex_destroy(>mode_config.fb_lock);
+}
+
 static void drm_test_framebuffer_create(struct kunit *test)
 {
const struct drm_framebuffer_test *params = test->param_value;
@@ -483,7 +494,44 @@ static void check_src_coords_test_to_desc(const struct 
check_src_coords_case *t,
 KUNIT_ARRAY_PARAM(check_src_coords, check_src_coords_cases,
  check_src_coords_test_to_desc);
 
+static void drm_test_framebuffer_cleanup(struct kunit *test)
+{
+   struct drm_mock *mock = test->priv;
+   struct drm_device *dev = >dev;
+   struct list_head *fb_list = >mode_config.fb_list;
+   struct drm_framebuffer fb1 = { .dev = dev };
+   struct drm_framebuffer fb2 = { .dev = dev };
+
+   /* This must result on [fb_list] -> fb1 -> fb2 */
+   list_add_tail(, fb_list);
+   list_add_tail(, fb_list);
+   dev->mode_config.num_fb = 2;
+
+   KUNIT_ASSERT_PTR_EQ(test, fb_list->prev, );
+   KUNIT_ASSERT_PTR_EQ(test, fb_list->next, );
+   KUNIT_ASSERT_PTR_EQ(test, fb1.head.prev, fb_list);
+   KUNIT_ASSERT_PTR_EQ(test, fb1.head.next, );
+   KUNIT_ASSERT_PTR_EQ(test, fb2.head.prev, );
+   KUNIT_ASSERT_PTR_EQ(test, fb2.head.next, fb_list);
+
+   drm_framebuffer_cleanup();
+
+   /* Now [fb_list] -> fb2 */
+   KUNIT_ASSERT_PTR_EQ(test, fb_list->prev, );
+   KUNIT_ASSERT_PTR_EQ(test, fb_list->next, );
+   KUNIT_ASSERT_PTR_EQ(test, fb2.head.prev, fb_list);
+   KUNIT_ASSERT_PTR_EQ(test, fb2.head.next, fb_list);
+   KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 1);
+
+   drm_framebuffer_cleanup();
+
+   /* Now fb_list is empty */
+   KUNIT_ASSERT_TRUE(test, list_empty(fb_list));
+   KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 0);
+}
+
 static struct kunit_case drm_framebuffer_tests[] = {
+   KUNIT_CASE(drm_test_framebuffer_cleanup),
KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported),
KUNIT_CASE_PARAM(drm_test_framebuffer_check_src_coords, 
check_src_coords_gen_params),
KUNIT_CASE_PARAM(drm_test_framebuffer_create, 
drm_framebuffer_create_gen_params),
@@ -493,6 +541,7 @@ static struct kunit_case drm_framebuffer_tests[] = {
 static struct kunit_suite drm_framebuffer_test_suite = {
.name = "drm_framebuffer",
.init = drm_framebuffer_test_init,
+   .exit = drm_framebuffer_test_exit,
.test_cases = drm_framebuffer_tests,
 };
 
-- 
2.41.0