diff mbox series

[RFC,06/10] drm: test-drm_dp_mst_helper: Convert to KUnit

Message ID 20220117232259.180459-7-michal.winiarski@intel.com (mailing list archive)
State New
Headers show
Series drm: selftests: Convert to KUnit | expand

Commit Message

Michał Winiarski Jan. 17, 2022, 11:22 p.m. UTC
igt_dp_mst_calc_pbn_mode was converted one-to-one,
igt_dp_mst_sideband_msg_req_decode was refactored to parameterized test.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/selftests/Makefile            |   3 +-
 .../gpu/drm/selftests/drm_modeset_selftests.h |   2 -
 .../drm/selftests/test-drm_dp_mst_helper.c    | 502 ++++++++++++------
 .../drm/selftests/test-drm_modeset_common.h   |   2 -
 4 files changed, 330 insertions(+), 179 deletions(-)

Comments

Daniel Latypov Jan. 19, 2022, 12:28 a.m. UTC | #1
On Mon, Jan 17, 2022 at 3:24 PM Michał Winiarski
<michal.winiarski@intel.com> wrote:
>
> igt_dp_mst_calc_pbn_mode was converted one-to-one,
> igt_dp_mst_sideband_msg_req_decode was refactored to parameterized test.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/selftests/Makefile            |   3 +-
>  .../gpu/drm/selftests/drm_modeset_selftests.h |   2 -
>  .../drm/selftests/test-drm_dp_mst_helper.c    | 502 ++++++++++++------
>  .../drm/selftests/test-drm_modeset_common.h   |   2 -
>  4 files changed, 330 insertions(+), 179 deletions(-)
>
> diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
> index 35f2f40dbaf3..77e37eebf099 100644
> --- a/drivers/gpu/drm/selftests/Makefile
> +++ b/drivers/gpu/drm/selftests/Makefile
> @@ -1,7 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  test-drm_modeset-$(CONFIG_DRM_DEBUG_SELFTEST) := \
>                       test-drm_modeset_common.o \
> -                     test-drm_dp_mst_helper.o \
>                       test-drm_rect.o
>
>  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o
> @@ -9,4 +8,4 @@ obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o
>  obj-$(CONFIG_DRM_KUNIT_TEST) := \
>         test-drm_cmdline_parser.o test-drm_plane_helper.o \
>         test-drm_format.o test-drm_framebuffer.o \
> -       test-drm_damage_helper.o
> +       test-drm_damage_helper.o test-drm_dp_mst_helper.o
> diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> index b6a6dba66b64..630770d30aba 100644
> --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> @@ -10,5 +10,3 @@ selftest(drm_rect_clip_scaled_div_by_zero, igt_drm_rect_clip_scaled_div_by_zero)
>  selftest(drm_rect_clip_scaled_not_clipped, igt_drm_rect_clip_scaled_not_clipped)
>  selftest(drm_rect_clip_scaled_clipped, igt_drm_rect_clip_scaled_clipped)
>  selftest(drm_rect_clip_scaled_signed_vs_unsigned, igt_drm_rect_clip_scaled_signed_vs_unsigned)
> -selftest(dp_mst_calc_pbn_mode, igt_dp_mst_calc_pbn_mode)
> -selftest(dp_mst_sideband_msg_req_decode, igt_dp_mst_sideband_msg_req_decode)
> diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> index 6b4759ed6bfd..d0719f3c5a42 100644
> --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> @@ -3,54 +3,97 @@
>   * Test cases for for the DRM DP MST helpers
>   */
>
> -#define PREFIX_STR "[drm_dp_mst_helper]"
> -
> +#include <kunit/test.h>
>  #include <linux/random.h>
>
>  #include <drm/drm_dp_mst_helper.h>
>  #include <drm/drm_print.h>
>
>  #include "../drm_dp_mst_topology_internal.h"
> -#include "test-drm_modeset_common.h"
>
> -int igt_dp_mst_calc_pbn_mode(void *ignored)
> +struct dp_mst_calc_pbn_mode_test {
> +       int rate;
> +       int bpp;
> +       int expected;
> +       bool dsc;
> +};
> +
> +static void dp_mst_calc_pbn_mode(struct kunit *test)
>  {
> -       int pbn, i;
> -       const struct {
> -               int rate;
> -               int bpp;
> -               int expected;
> -               bool dsc;
> -       } test_params[] = {
> -               { 154000, 30, 689, false },
> -               { 234000, 30, 1047, false },
> -               { 297000, 24, 1063, false },
> -               { 332880, 24, 50, true },
> -               { 324540, 24, 49, true },
> -       };
> +       const struct dp_mst_calc_pbn_mode_test *params = test->param_value;
> +       int pbn;
>
> -       for (i = 0; i < ARRAY_SIZE(test_params); i++) {
> -               pbn = drm_dp_calc_pbn_mode(test_params[i].rate,
> -                                          test_params[i].bpp,
> -                                          test_params[i].dsc);
> -               FAIL(pbn != test_params[i].expected,
> -                    "Expected PBN %d for clock %d bpp %d, got %d\n",
> -                    test_params[i].expected, test_params[i].rate,
> -                    test_params[i].bpp, pbn);
> -       }
> +       pbn = drm_dp_calc_pbn_mode(params->rate,
> +                                  params->bpp,
> +                                  params->dsc);
> +
> +       KUNIT_EXPECT_EQ(test, pbn, params->expected);
> +}
>
> -       return 0;
> +static const struct dp_mst_calc_pbn_mode_test dp_mst_calc_pbn_mode_tests[] = {
> +       {
> +               .rate = 154000,
> +               .bpp = 30,
> +               .expected = 689,
> +               .dsc = false,
> +       },
> +       {
> +               .rate = 234000,
> +               .bpp = 30,
> +               .expected = 1047,
> +               .dsc = false,
> +       },
> +       {
> +               .rate = 297000,
> +               .bpp = 24,
> +               .expected = 1063,
> +               .dsc = false,
> +       },
> +       {
> +               .rate = 332880,
> +               .bpp = 24,
> +               .expected = 50,
> +               .dsc = true,
> +       },
> +       {
> +               .rate = 324540,
> +               .bpp = 24,
> +               .expected = 49,
> +               .dsc = true,
> +       },
> +};
> +
> +static void dp_mst_calc_pbn_mode_desc(const struct dp_mst_calc_pbn_mode_test *t,
> +                                     char *desc)
> +{
> +       sprintf(desc, "rate = %d, bpp = %d, dsc = %s",
> +               t->rate, t->bpp, t->dsc ? "true" : "false");
>  }
>
> -static bool
> -sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
> -                      const struct drm_dp_sideband_msg_req_body *out)
> +KUNIT_ARRAY_PARAM(dp_mst_calc_pbn_mode, dp_mst_calc_pbn_mode_tests, dp_mst_calc_pbn_mode_desc);
> +
> +static void
> +drm_dp_mst_helper_printfn(struct drm_printer *p, struct va_format *vaf)
> +{
> +       struct kunit *test = p->arg;
> +
> +       kunit_err(test, "%pV", vaf);
> +}
> +
> +static void
> +expect_sideband_msg_req_equal(struct kunit *test,
> +                             const struct drm_dp_sideband_msg_req_body *in,
> +                             const struct drm_dp_sideband_msg_req_body *out)
>  {
>         const struct drm_dp_remote_i2c_read_tx *txin, *txout;
> +       struct drm_printer p = {
> +               .printfn = drm_dp_mst_helper_printfn,
> +               .arg = test
> +       };
>         int i;
>
>         if (in->req_type != out->req_type)
> -               return false;
> +               goto fail;

Briefly skimming over this code, it looks like it'd be simpler to have
this function remain unchanged.
There's only the one caller.
It could take on the responsibility of creating the drm_printer and
redirect the printfn to kunit_err, afaik.

Passing in `test` would be useful if we were generating custom error
messages for each of the `return false` lines, which I assume was the
original motivation for doing so?
But looking at this, I'd agree it seems like too much work.

Tangent:
It might have been easier to do that if the kunit assertions returned pass/fail.
E.g. instead of having to do

if (!<long-condition>) {
  KUNIT_FAIL("<long-condition> not met");
  return;
}

if we could do

if(!KUNIT_EXPECT_TRUE(long-condition))
  return;

or if there was a new macro type

KUNIT_EXPECT_RET_TRUE(long-condition); // like ASSERT, but just return
from this func on failure


>
>         switch (in->req_type) {
>         /*
> @@ -65,7 +108,7 @@ sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
>                     IN.num_transactions != OUT.num_transactions ||
>                     IN.port_number != OUT.port_number ||
>                     IN.read_i2c_device_id != OUT.read_i2c_device_id)
> -                       return false;
> +                       goto fail;
>
>                 for (i = 0; i < IN.num_transactions; i++) {
>                         txin = &IN.transactions[i];
> @@ -76,11 +119,11 @@ sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
>                             txin->num_bytes != txout->num_bytes ||
>                             txin->i2c_transaction_delay !=
>                             txout->i2c_transaction_delay)
> -                               return false;
> +                               goto fail;
>
>                         if (memcmp(txin->bytes, txout->bytes,
>                                    txin->num_bytes) != 0)
> -                               return false;
> +                               goto fail;
>                 }
>                 break;
>  #undef IN
> @@ -92,9 +135,12 @@ sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
>                 if (IN.dpcd_address != OUT.dpcd_address ||
>                     IN.num_bytes != OUT.num_bytes ||
>                     IN.port_number != OUT.port_number)
> -                       return false;
> +                       goto fail;
>
> -               return memcmp(IN.bytes, OUT.bytes, IN.num_bytes) == 0;
> +               if (memcmp(IN.bytes, OUT.bytes, IN.num_bytes) != 0)
> +                       goto fail;
> +
> +               break;
>  #undef IN
>  #undef OUT
>
> @@ -104,55 +150,65 @@ sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
>                 if (IN.port_number != OUT.port_number ||
>                     IN.write_i2c_device_id != OUT.write_i2c_device_id ||
>                     IN.num_bytes != OUT.num_bytes)
> -                       return false;
> +                       goto fail;
>
> -               return memcmp(IN.bytes, OUT.bytes, IN.num_bytes) == 0;
> +               if (memcmp(IN.bytes, OUT.bytes, IN.num_bytes) != 0)
> +                       goto fail;
> +
> +               break;
>  #undef IN
>  #undef OUT
>
>         default:
> -               return memcmp(in, out, sizeof(*in)) == 0;
> +               if (memcmp(in, out, sizeof(*in)) != 0)
> +                       goto fail;
>         }
>
> -       return true;
> +       return;
> +
> +fail:
> +       drm_printf(&p, "Expected:\n");
> +       drm_dp_dump_sideband_msg_req_body(in, 1, &p);
> +       drm_printf(&p, "Got:\n");
> +       drm_dp_dump_sideband_msg_req_body(out, 1, &p);
> +       KUNIT_FAIL(test, "Encode/decode failed");
> +}
> +
> +struct dp_mst_sideband_msg_req_decode_test {
> +       const struct drm_dp_sideband_msg_req_body req;
> +       const struct drm_dp_sideband_msg_req_body
> +               (*f)(const struct drm_dp_sideband_msg_req_body *in);
> +};
> +
> +const struct drm_dp_sideband_msg_req_body
> +param_to_dp_mst_sideband_msg_req_body(const struct dp_mst_sideband_msg_req_decode_test *t)
> +{
> +       if (t->f)
> +               return t->f(&t->req);
> +
> +       return t->req;
>  }
>
> -static bool
> -sideband_msg_req_encode_decode(struct drm_dp_sideband_msg_req_body *in)
> +static void dp_mst_sideband_msg_req_decode(struct kunit *test)
>  {
> +       const struct drm_dp_sideband_msg_req_body in =
> +               param_to_dp_mst_sideband_msg_req_body(test->param_value);
>         struct drm_dp_sideband_msg_req_body *out;
> -       struct drm_printer p = drm_err_printer(PREFIX_STR);
>         struct drm_dp_sideband_msg_tx *txmsg;
> -       int i, ret;
> -       bool result = true;
> -
> -       out = kzalloc(sizeof(*out), GFP_KERNEL);
> -       if (!out)
> -               return false;
> -
> -       txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> -       if (!txmsg)
> -               return false;
> -
> -       drm_dp_encode_sideband_req(in, txmsg);
> -       ret = drm_dp_decode_sideband_req(txmsg, out);
> -       if (ret < 0) {
> -               drm_printf(&p, "Failed to decode sideband request: %d\n",
> -                          ret);
> -               result = false;
> -               goto out;
> -       }
> +       int i;
>
> -       if (!sideband_msg_req_equal(in, out)) {
> -               drm_printf(&p, "Encode/decode failed, expected:\n");
> -               drm_dp_dump_sideband_msg_req_body(in, 1, &p);
> -               drm_printf(&p, "Got:\n");
> -               drm_dp_dump_sideband_msg_req_body(out, 1, &p);
> -               result = false;
> -               goto out;
> -       }
> +       out = kunit_kzalloc(test, sizeof(*out), GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, out);
>
> -       switch (in->req_type) {
> +       txmsg = kunit_kzalloc(test, sizeof(*txmsg), GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, txmsg);
> +
> +       drm_dp_encode_sideband_req(&in, txmsg);
> +       KUNIT_ASSERT_EQ(test, drm_dp_decode_sideband_req(txmsg, out), 0);
> +
> +       expect_sideband_msg_req_equal(test, &in, out);
> +
> +       switch (in.req_type) {
>         case DP_REMOTE_DPCD_WRITE:
>                 kfree(out->u.dpcd_write.bytes);
>                 break;
> @@ -164,110 +220,210 @@ sideband_msg_req_encode_decode(struct drm_dp_sideband_msg_req_body *in)
>                 kfree(out->u.i2c_write.bytes);
>                 break;
>         }
> -
> -       /* Clear everything but the req_type for the input */
> -       memset(&in->u, 0, sizeof(in->u));
> -
> -out:
> -       kfree(out);
> -       kfree(txmsg);
> -       return result;
>  }
>
> -int igt_dp_mst_sideband_msg_req_decode(void *unused)
> +static u8 data[] = { 0xff, 0x0, 0xdd };
> +
> +const struct drm_dp_sideband_msg_req_body
> +random_dp_query_enc_client_id(const struct drm_dp_sideband_msg_req_body *in)
>  {
> -       struct drm_dp_sideband_msg_req_body in = { 0 };
> -       u8 data[] = { 0xff, 0x0, 0xdd };
> -       int i;
> +       struct drm_dp_query_stream_enc_status enc_status = { };
>
> -#define DO_TEST() FAIL_ON(!sideband_msg_req_encode_decode(&in))
> -
> -       in.req_type = DP_ENUM_PATH_RESOURCES;
> -       in.u.port_num.port_number = 5;
> -       DO_TEST();
> -
> -       in.req_type = DP_POWER_UP_PHY;
> -       in.u.port_num.port_number = 5;
> -       DO_TEST();
> -
> -       in.req_type = DP_POWER_DOWN_PHY;
> -       in.u.port_num.port_number = 5;
> -       DO_TEST();
> -
> -       in.req_type = DP_ALLOCATE_PAYLOAD;
> -       in.u.allocate_payload.number_sdp_streams = 3;
> -       for (i = 0; i < in.u.allocate_payload.number_sdp_streams; i++)
> -               in.u.allocate_payload.sdp_stream_sink[i] = i + 1;
> -       DO_TEST();
> -       in.u.allocate_payload.port_number = 0xf;
> -       DO_TEST();
> -       in.u.allocate_payload.vcpi = 0x7f;
> -       DO_TEST();
> -       in.u.allocate_payload.pbn = U16_MAX;
> -       DO_TEST();
> -
> -       in.req_type = DP_QUERY_PAYLOAD;
> -       in.u.query_payload.port_number = 0xf;
> -       DO_TEST();
> -       in.u.query_payload.vcpi = 0x7f;
> -       DO_TEST();
> -
> -       in.req_type = DP_REMOTE_DPCD_READ;
> -       in.u.dpcd_read.port_number = 0xf;
> -       DO_TEST();
> -       in.u.dpcd_read.dpcd_address = 0xfedcb;
> -       DO_TEST();
> -       in.u.dpcd_read.num_bytes = U8_MAX;
> -       DO_TEST();
> -
> -       in.req_type = DP_REMOTE_DPCD_WRITE;
> -       in.u.dpcd_write.port_number = 0xf;
> -       DO_TEST();
> -       in.u.dpcd_write.dpcd_address = 0xfedcb;
> -       DO_TEST();
> -       in.u.dpcd_write.num_bytes = ARRAY_SIZE(data);
> -       in.u.dpcd_write.bytes = data;
> -       DO_TEST();
> -
> -       in.req_type = DP_REMOTE_I2C_READ;
> -       in.u.i2c_read.port_number = 0xf;
> -       DO_TEST();
> -       in.u.i2c_read.read_i2c_device_id = 0x7f;
> -       DO_TEST();
> -       in.u.i2c_read.num_transactions = 3;
> -       in.u.i2c_read.num_bytes_read = ARRAY_SIZE(data) * 3;
> -       for (i = 0; i < in.u.i2c_read.num_transactions; i++) {
> -               in.u.i2c_read.transactions[i].bytes = data;
> -               in.u.i2c_read.transactions[i].num_bytes = ARRAY_SIZE(data);
> -               in.u.i2c_read.transactions[i].i2c_dev_id = 0x7f & ~i;
> -               in.u.i2c_read.transactions[i].i2c_transaction_delay = 0xf & ~i;
> -       }
> -       DO_TEST();
> -
> -       in.req_type = DP_REMOTE_I2C_WRITE;
> -       in.u.i2c_write.port_number = 0xf;
> -       DO_TEST();
> -       in.u.i2c_write.write_i2c_device_id = 0x7f;
> -       DO_TEST();
> -       in.u.i2c_write.num_bytes = ARRAY_SIZE(data);
> -       in.u.i2c_write.bytes = data;
> -       DO_TEST();
> -
> -       in.req_type = DP_QUERY_STREAM_ENC_STATUS;
> -       in.u.enc_status.stream_id = 1;
> -       DO_TEST();
> -       get_random_bytes(in.u.enc_status.client_id,
> -                        sizeof(in.u.enc_status.client_id));
> -       DO_TEST();
> -       in.u.enc_status.stream_event = 3;
> -       DO_TEST();
> -       in.u.enc_status.valid_stream_event = 0;
> -       DO_TEST();
> -       in.u.enc_status.stream_behavior = 3;
> -       DO_TEST();
> -       in.u.enc_status.valid_stream_behavior = 1;
> -       DO_TEST();
> -
> -#undef DO_TEST
> -       return 0;
> +       get_random_bytes(enc_status.client_id, sizeof(enc_status.client_id));
> +
> +       return (const struct drm_dp_sideband_msg_req_body) { .req_type = in->req_type,
> +                                                            .u.enc_status = enc_status
> +       };
>  }
> +
> +static const struct dp_mst_sideband_msg_req_decode_test dp_msg_sideband_msg_req_decode_tests[] = {
> +       {
> +               .req = {
> +                       .req_type = DP_ENUM_PATH_RESOURCES,
> +                       .u.port_num.port_number = 5,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_POWER_UP_PHY,
> +                       .u.port_num.port_number = 5,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_POWER_DOWN_PHY,
> +                       .u.port_num.port_number = 5,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_ALLOCATE_PAYLOAD,
> +                       .u.allocate_payload.number_sdp_streams = 3,
> +                       .u.allocate_payload.sdp_stream_sink = { 1, 2, 3 },
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_ALLOCATE_PAYLOAD,
> +                       .u.allocate_payload.port_number = 0xf,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_ALLOCATE_PAYLOAD,
> +                       .u.allocate_payload.vcpi = 0x7f,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_ALLOCATE_PAYLOAD,
> +                       .u.allocate_payload.pbn = U16_MAX,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_QUERY_PAYLOAD,
> +                       .u.query_payload.port_number = 0xf,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_QUERY_PAYLOAD,
> +                       .u.query_payload.vcpi = 0x7f,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_DPCD_READ,
> +                       .u.dpcd_read.port_number = 0xf,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_DPCD_READ,
> +                       .u.dpcd_read.dpcd_address = 0xfedcb,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_DPCD_READ,
> +                       .u.dpcd_read.num_bytes = U8_MAX,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_DPCD_WRITE,
> +                       .u.dpcd_write.port_number = 0xf,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_DPCD_WRITE,
> +                       .u.dpcd_write.dpcd_address = 0xfedcb,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_DPCD_WRITE,
> +                       .u.dpcd_write.num_bytes = ARRAY_SIZE(data),
> +                       .u.dpcd_write.bytes = data,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_I2C_READ,
> +                       .u.i2c_read.port_number = 0xf,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_I2C_READ,
> +                       .u.i2c_read.read_i2c_device_id = 0x7f,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_I2C_READ,
> +                       .u.i2c_read.num_transactions = 3,
> +                       .u.i2c_read.num_bytes_read = ARRAY_SIZE(data) * 3,
> +                       .u.i2c_read.transactions = {
> +                               { .bytes = data, .num_bytes = ARRAY_SIZE(data),
> +                                 .i2c_dev_id = 0x7f, .i2c_transaction_delay = 0xf, },
> +                               { .bytes = data, .num_bytes = ARRAY_SIZE(data),
> +                                 .i2c_dev_id = 0x7e, .i2c_transaction_delay = 0xe, },
> +                               { .bytes = data, .num_bytes = ARRAY_SIZE(data),
> +                                 .i2c_dev_id = 0x7d, .i2c_transaction_delay = 0xd, },
> +                       },
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_I2C_WRITE,
> +                       .u.i2c_write.port_number = 0xf,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_I2C_WRITE,
> +                       .u.i2c_write.write_i2c_device_id = 0x7f,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_REMOTE_I2C_WRITE,
> +                       .u.i2c_write.num_bytes = ARRAY_SIZE(data),
> +                       .u.i2c_write.bytes = data,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
> +                       .u.enc_status.stream_id = 1,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
> +               },
> +               .f = random_dp_query_enc_client_id,
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
> +                       .u.enc_status.stream_event = 3,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
> +                       .u.enc_status.valid_stream_event = 0,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
> +                       .u.enc_status.stream_behavior = 3,
> +               },
> +       },
> +       {
> +               .req = {
> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
> +                       .u.enc_status.valid_stream_behavior = 1,
> +               },
> +       },
> +};
> +
> +KUNIT_ARRAY_PARAM(dp_mst_sideband_msg_req, dp_msg_sideband_msg_req_decode_tests, NULL);
> +
> +static struct kunit_case drm_dp_mst_helper_tests[] = {
> +       KUNIT_CASE_PARAM(dp_mst_calc_pbn_mode, dp_mst_calc_pbn_mode_gen_params),
> +       KUNIT_CASE_PARAM(dp_mst_sideband_msg_req_decode, dp_mst_sideband_msg_req_gen_params),
> +       {}
> +};
> +
> +static struct kunit_suite drm_dp_mst_helper_test_suite = {
> +       .name = "drm_dp_mst_helper_tests",
> +       .test_cases = drm_dp_mst_helper_tests,
> +};
> +
> +kunit_test_suite(drm_dp_mst_helper_test_suite);
> diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
> index 1501d99aee2f..c7cc5edc65f1 100644
> --- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h
> +++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
> @@ -20,7 +20,5 @@ int igt_drm_rect_clip_scaled_div_by_zero(void *ignored);
>  int igt_drm_rect_clip_scaled_not_clipped(void *ignored);
>  int igt_drm_rect_clip_scaled_clipped(void *ignored);
>  int igt_drm_rect_clip_scaled_signed_vs_unsigned(void *ignored);
> -int igt_dp_mst_calc_pbn_mode(void *ignored);
> -int igt_dp_mst_sideband_msg_req_decode(void *ignored);
>
>  #endif
> --
> 2.34.1
>
Michał Winiarski Jan. 21, 2022, 12:48 a.m. UTC | #2
On 19.01.2022 01:28, Daniel Latypov wrote:
> On Mon, Jan 17, 2022 at 3:24 PM Michał Winiarski
> <michal.winiarski@intel.com> wrote:
>>
>> igt_dp_mst_calc_pbn_mode was converted one-to-one,
>> igt_dp_mst_sideband_msg_req_decode was refactored to parameterized test.
>>
>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>> ---
>>   drivers/gpu/drm/selftests/Makefile            |   3 +-
>>   .../gpu/drm/selftests/drm_modeset_selftests.h |   2 -
>>   .../drm/selftests/test-drm_dp_mst_helper.c    | 502 ++++++++++++------
>>   .../drm/selftests/test-drm_modeset_common.h   |   2 -
>>   4 files changed, 330 insertions(+), 179 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
>> index 35f2f40dbaf3..77e37eebf099 100644
>> --- a/drivers/gpu/drm/selftests/Makefile
>> +++ b/drivers/gpu/drm/selftests/Makefile
>> @@ -1,7 +1,6 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   test-drm_modeset-$(CONFIG_DRM_DEBUG_SELFTEST) := \
>>                        test-drm_modeset_common.o \
>> -                     test-drm_dp_mst_helper.o \
>>                        test-drm_rect.o
>>
>>   obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o
>> @@ -9,4 +8,4 @@ obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o
>>   obj-$(CONFIG_DRM_KUNIT_TEST) := \
>>          test-drm_cmdline_parser.o test-drm_plane_helper.o \
>>          test-drm_format.o test-drm_framebuffer.o \
>> -       test-drm_damage_helper.o
>> +       test-drm_damage_helper.o test-drm_dp_mst_helper.o
>> diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
>> index b6a6dba66b64..630770d30aba 100644
>> --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h
>> +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
>> @@ -10,5 +10,3 @@ selftest(drm_rect_clip_scaled_div_by_zero, igt_drm_rect_clip_scaled_div_by_zero)
>>   selftest(drm_rect_clip_scaled_not_clipped, igt_drm_rect_clip_scaled_not_clipped)
>>   selftest(drm_rect_clip_scaled_clipped, igt_drm_rect_clip_scaled_clipped)
>>   selftest(drm_rect_clip_scaled_signed_vs_unsigned, igt_drm_rect_clip_scaled_signed_vs_unsigned)
>> -selftest(dp_mst_calc_pbn_mode, igt_dp_mst_calc_pbn_mode)
>> -selftest(dp_mst_sideband_msg_req_decode, igt_dp_mst_sideband_msg_req_decode)
>> diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
>> index 6b4759ed6bfd..d0719f3c5a42 100644
>> --- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
>> +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
>> @@ -3,54 +3,97 @@
>>    * Test cases for for the DRM DP MST helpers
>>    */
>>
>> -#define PREFIX_STR "[drm_dp_mst_helper]"
>> -
>> +#include <kunit/test.h>
>>   #include <linux/random.h>
>>
>>   #include <drm/drm_dp_mst_helper.h>
>>   #include <drm/drm_print.h>
>>
>>   #include "../drm_dp_mst_topology_internal.h"
>> -#include "test-drm_modeset_common.h"
>>
>> -int igt_dp_mst_calc_pbn_mode(void *ignored)
>> +struct dp_mst_calc_pbn_mode_test {
>> +       int rate;
>> +       int bpp;
>> +       int expected;
>> +       bool dsc;
>> +};
>> +
>> +static void dp_mst_calc_pbn_mode(struct kunit *test)
>>   {
>> -       int pbn, i;
>> -       const struct {
>> -               int rate;
>> -               int bpp;
>> -               int expected;
>> -               bool dsc;
>> -       } test_params[] = {
>> -               { 154000, 30, 689, false },
>> -               { 234000, 30, 1047, false },
>> -               { 297000, 24, 1063, false },
>> -               { 332880, 24, 50, true },
>> -               { 324540, 24, 49, true },
>> -       };
>> +       const struct dp_mst_calc_pbn_mode_test *params = test->param_value;
>> +       int pbn;
>>
>> -       for (i = 0; i < ARRAY_SIZE(test_params); i++) {
>> -               pbn = drm_dp_calc_pbn_mode(test_params[i].rate,
>> -                                          test_params[i].bpp,
>> -                                          test_params[i].dsc);
>> -               FAIL(pbn != test_params[i].expected,
>> -                    "Expected PBN %d for clock %d bpp %d, got %d\n",
>> -                    test_params[i].expected, test_params[i].rate,
>> -                    test_params[i].bpp, pbn);
>> -       }
>> +       pbn = drm_dp_calc_pbn_mode(params->rate,
>> +                                  params->bpp,
>> +                                  params->dsc);
>> +
>> +       KUNIT_EXPECT_EQ(test, pbn, params->expected);
>> +}
>>
>> -       return 0;
>> +static const struct dp_mst_calc_pbn_mode_test dp_mst_calc_pbn_mode_tests[] = {
>> +       {
>> +               .rate = 154000,
>> +               .bpp = 30,
>> +               .expected = 689,
>> +               .dsc = false,
>> +       },
>> +       {
>> +               .rate = 234000,
>> +               .bpp = 30,
>> +               .expected = 1047,
>> +               .dsc = false,
>> +       },
>> +       {
>> +               .rate = 297000,
>> +               .bpp = 24,
>> +               .expected = 1063,
>> +               .dsc = false,
>> +       },
>> +       {
>> +               .rate = 332880,
>> +               .bpp = 24,
>> +               .expected = 50,
>> +               .dsc = true,
>> +       },
>> +       {
>> +               .rate = 324540,
>> +               .bpp = 24,
>> +               .expected = 49,
>> +               .dsc = true,
>> +       },
>> +};
>> +
>> +static void dp_mst_calc_pbn_mode_desc(const struct dp_mst_calc_pbn_mode_test *t,
>> +                                     char *desc)
>> +{
>> +       sprintf(desc, "rate = %d, bpp = %d, dsc = %s",
>> +               t->rate, t->bpp, t->dsc ? "true" : "false");
>>   }
>>
>> -static bool
>> -sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
>> -                      const struct drm_dp_sideband_msg_req_body *out)
>> +KUNIT_ARRAY_PARAM(dp_mst_calc_pbn_mode, dp_mst_calc_pbn_mode_tests, dp_mst_calc_pbn_mode_desc);
>> +
>> +static void
>> +drm_dp_mst_helper_printfn(struct drm_printer *p, struct va_format *vaf)
>> +{
>> +       struct kunit *test = p->arg;
>> +
>> +       kunit_err(test, "%pV", vaf);
>> +}
>> +
>> +static void
>> +expect_sideband_msg_req_equal(struct kunit *test,
>> +                             const struct drm_dp_sideband_msg_req_body *in,
>> +                             const struct drm_dp_sideband_msg_req_body *out)
>>   {
>>          const struct drm_dp_remote_i2c_read_tx *txin, *txout;
>> +       struct drm_printer p = {
>> +               .printfn = drm_dp_mst_helper_printfn,
>> +               .arg = test
>> +       };
>>          int i;
>>
>>          if (in->req_type != out->req_type)
>> -               return false;
>> +               goto fail;
> 
> Briefly skimming over this code, it looks like it'd be simpler to have
> this function remain unchanged.
> There's only the one caller.
> It could take on the responsibility of creating the drm_printer and
> redirect the printfn to kunit_err, afaik.
> 
> Passing in `test` would be useful if we were generating custom error
> messages for each of the `return false` lines, which I assume was the
> original motivation for doing so?
> But looking at this, I'd agree it seems like too much work.

Yes, that was the original motivation, but eventually I went back to the 
original code, leaving drm_printer behind.
I agree - I'll leave the function intact.

> 
> Tangent:
> It might have been easier to do that if the kunit assertions returned pass/fail.
> E.g. instead of having to do
> 
> if (!<long-condition>) {
>    KUNIT_FAIL("<long-condition> not met");
>    return;
> }
> 
> if we could do
> 
> if(!KUNIT_EXPECT_TRUE(long-condition))
>    return;
> 
> or if there was a new macro type
> 
> KUNIT_EXPECT_RET_TRUE(long-condition); // like ASSERT, but just return
> from this func on failure

This would simplify a bunch of other tests as well.
On the other hand - EXPECT_TRUE returning a value is not something I 
would expect :)

Thanks!
-Michał

> 
> 
>>
>>          switch (in->req_type) {
>>          /*
>> @@ -65,7 +108,7 @@ sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
>>                      IN.num_transactions != OUT.num_transactions ||
>>                      IN.port_number != OUT.port_number ||
>>                      IN.read_i2c_device_id != OUT.read_i2c_device_id)
>> -                       return false;
>> +                       goto fail;
>>
>>                  for (i = 0; i < IN.num_transactions; i++) {
>>                          txin = &IN.transactions[i];
>> @@ -76,11 +119,11 @@ sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
>>                              txin->num_bytes != txout->num_bytes ||
>>                              txin->i2c_transaction_delay !=
>>                              txout->i2c_transaction_delay)
>> -                               return false;
>> +                               goto fail;
>>
>>                          if (memcmp(txin->bytes, txout->bytes,
>>                                     txin->num_bytes) != 0)
>> -                               return false;
>> +                               goto fail;
>>                  }
>>                  break;
>>   #undef IN
>> @@ -92,9 +135,12 @@ sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
>>                  if (IN.dpcd_address != OUT.dpcd_address ||
>>                      IN.num_bytes != OUT.num_bytes ||
>>                      IN.port_number != OUT.port_number)
>> -                       return false;
>> +                       goto fail;
>>
>> -               return memcmp(IN.bytes, OUT.bytes, IN.num_bytes) == 0;
>> +               if (memcmp(IN.bytes, OUT.bytes, IN.num_bytes) != 0)
>> +                       goto fail;
>> +
>> +               break;
>>   #undef IN
>>   #undef OUT
>>
>> @@ -104,55 +150,65 @@ sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
>>                  if (IN.port_number != OUT.port_number ||
>>                      IN.write_i2c_device_id != OUT.write_i2c_device_id ||
>>                      IN.num_bytes != OUT.num_bytes)
>> -                       return false;
>> +                       goto fail;
>>
>> -               return memcmp(IN.bytes, OUT.bytes, IN.num_bytes) == 0;
>> +               if (memcmp(IN.bytes, OUT.bytes, IN.num_bytes) != 0)
>> +                       goto fail;
>> +
>> +               break;
>>   #undef IN
>>   #undef OUT
>>
>>          default:
>> -               return memcmp(in, out, sizeof(*in)) == 0;
>> +               if (memcmp(in, out, sizeof(*in)) != 0)
>> +                       goto fail;
>>          }
>>
>> -       return true;
>> +       return;
>> +
>> +fail:
>> +       drm_printf(&p, "Expected:\n");
>> +       drm_dp_dump_sideband_msg_req_body(in, 1, &p);
>> +       drm_printf(&p, "Got:\n");
>> +       drm_dp_dump_sideband_msg_req_body(out, 1, &p);
>> +       KUNIT_FAIL(test, "Encode/decode failed");
>> +}
>> +
>> +struct dp_mst_sideband_msg_req_decode_test {
>> +       const struct drm_dp_sideband_msg_req_body req;
>> +       const struct drm_dp_sideband_msg_req_body
>> +               (*f)(const struct drm_dp_sideband_msg_req_body *in);
>> +};
>> +
>> +const struct drm_dp_sideband_msg_req_body
>> +param_to_dp_mst_sideband_msg_req_body(const struct dp_mst_sideband_msg_req_decode_test *t)
>> +{
>> +       if (t->f)
>> +               return t->f(&t->req);
>> +
>> +       return t->req;
>>   }
>>
>> -static bool
>> -sideband_msg_req_encode_decode(struct drm_dp_sideband_msg_req_body *in)
>> +static void dp_mst_sideband_msg_req_decode(struct kunit *test)
>>   {
>> +       const struct drm_dp_sideband_msg_req_body in =
>> +               param_to_dp_mst_sideband_msg_req_body(test->param_value);
>>          struct drm_dp_sideband_msg_req_body *out;
>> -       struct drm_printer p = drm_err_printer(PREFIX_STR);
>>          struct drm_dp_sideband_msg_tx *txmsg;
>> -       int i, ret;
>> -       bool result = true;
>> -
>> -       out = kzalloc(sizeof(*out), GFP_KERNEL);
>> -       if (!out)
>> -               return false;
>> -
>> -       txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
>> -       if (!txmsg)
>> -               return false;
>> -
>> -       drm_dp_encode_sideband_req(in, txmsg);
>> -       ret = drm_dp_decode_sideband_req(txmsg, out);
>> -       if (ret < 0) {
>> -               drm_printf(&p, "Failed to decode sideband request: %d\n",
>> -                          ret);
>> -               result = false;
>> -               goto out;
>> -       }
>> +       int i;
>>
>> -       if (!sideband_msg_req_equal(in, out)) {
>> -               drm_printf(&p, "Encode/decode failed, expected:\n");
>> -               drm_dp_dump_sideband_msg_req_body(in, 1, &p);
>> -               drm_printf(&p, "Got:\n");
>> -               drm_dp_dump_sideband_msg_req_body(out, 1, &p);
>> -               result = false;
>> -               goto out;
>> -       }
>> +       out = kunit_kzalloc(test, sizeof(*out), GFP_KERNEL);
>> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, out);
>>
>> -       switch (in->req_type) {
>> +       txmsg = kunit_kzalloc(test, sizeof(*txmsg), GFP_KERNEL);
>> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, txmsg);
>> +
>> +       drm_dp_encode_sideband_req(&in, txmsg);
>> +       KUNIT_ASSERT_EQ(test, drm_dp_decode_sideband_req(txmsg, out), 0);
>> +
>> +       expect_sideband_msg_req_equal(test, &in, out);
>> +
>> +       switch (in.req_type) {
>>          case DP_REMOTE_DPCD_WRITE:
>>                  kfree(out->u.dpcd_write.bytes);
>>                  break;
>> @@ -164,110 +220,210 @@ sideband_msg_req_encode_decode(struct drm_dp_sideband_msg_req_body *in)
>>                  kfree(out->u.i2c_write.bytes);
>>                  break;
>>          }
>> -
>> -       /* Clear everything but the req_type for the input */
>> -       memset(&in->u, 0, sizeof(in->u));
>> -
>> -out:
>> -       kfree(out);
>> -       kfree(txmsg);
>> -       return result;
>>   }
>>
>> -int igt_dp_mst_sideband_msg_req_decode(void *unused)
>> +static u8 data[] = { 0xff, 0x0, 0xdd };
>> +
>> +const struct drm_dp_sideband_msg_req_body
>> +random_dp_query_enc_client_id(const struct drm_dp_sideband_msg_req_body *in)
>>   {
>> -       struct drm_dp_sideband_msg_req_body in = { 0 };
>> -       u8 data[] = { 0xff, 0x0, 0xdd };
>> -       int i;
>> +       struct drm_dp_query_stream_enc_status enc_status = { };
>>
>> -#define DO_TEST() FAIL_ON(!sideband_msg_req_encode_decode(&in))
>> -
>> -       in.req_type = DP_ENUM_PATH_RESOURCES;
>> -       in.u.port_num.port_number = 5;
>> -       DO_TEST();
>> -
>> -       in.req_type = DP_POWER_UP_PHY;
>> -       in.u.port_num.port_number = 5;
>> -       DO_TEST();
>> -
>> -       in.req_type = DP_POWER_DOWN_PHY;
>> -       in.u.port_num.port_number = 5;
>> -       DO_TEST();
>> -
>> -       in.req_type = DP_ALLOCATE_PAYLOAD;
>> -       in.u.allocate_payload.number_sdp_streams = 3;
>> -       for (i = 0; i < in.u.allocate_payload.number_sdp_streams; i++)
>> -               in.u.allocate_payload.sdp_stream_sink[i] = i + 1;
>> -       DO_TEST();
>> -       in.u.allocate_payload.port_number = 0xf;
>> -       DO_TEST();
>> -       in.u.allocate_payload.vcpi = 0x7f;
>> -       DO_TEST();
>> -       in.u.allocate_payload.pbn = U16_MAX;
>> -       DO_TEST();
>> -
>> -       in.req_type = DP_QUERY_PAYLOAD;
>> -       in.u.query_payload.port_number = 0xf;
>> -       DO_TEST();
>> -       in.u.query_payload.vcpi = 0x7f;
>> -       DO_TEST();
>> -
>> -       in.req_type = DP_REMOTE_DPCD_READ;
>> -       in.u.dpcd_read.port_number = 0xf;
>> -       DO_TEST();
>> -       in.u.dpcd_read.dpcd_address = 0xfedcb;
>> -       DO_TEST();
>> -       in.u.dpcd_read.num_bytes = U8_MAX;
>> -       DO_TEST();
>> -
>> -       in.req_type = DP_REMOTE_DPCD_WRITE;
>> -       in.u.dpcd_write.port_number = 0xf;
>> -       DO_TEST();
>> -       in.u.dpcd_write.dpcd_address = 0xfedcb;
>> -       DO_TEST();
>> -       in.u.dpcd_write.num_bytes = ARRAY_SIZE(data);
>> -       in.u.dpcd_write.bytes = data;
>> -       DO_TEST();
>> -
>> -       in.req_type = DP_REMOTE_I2C_READ;
>> -       in.u.i2c_read.port_number = 0xf;
>> -       DO_TEST();
>> -       in.u.i2c_read.read_i2c_device_id = 0x7f;
>> -       DO_TEST();
>> -       in.u.i2c_read.num_transactions = 3;
>> -       in.u.i2c_read.num_bytes_read = ARRAY_SIZE(data) * 3;
>> -       for (i = 0; i < in.u.i2c_read.num_transactions; i++) {
>> -               in.u.i2c_read.transactions[i].bytes = data;
>> -               in.u.i2c_read.transactions[i].num_bytes = ARRAY_SIZE(data);
>> -               in.u.i2c_read.transactions[i].i2c_dev_id = 0x7f & ~i;
>> -               in.u.i2c_read.transactions[i].i2c_transaction_delay = 0xf & ~i;
>> -       }
>> -       DO_TEST();
>> -
>> -       in.req_type = DP_REMOTE_I2C_WRITE;
>> -       in.u.i2c_write.port_number = 0xf;
>> -       DO_TEST();
>> -       in.u.i2c_write.write_i2c_device_id = 0x7f;
>> -       DO_TEST();
>> -       in.u.i2c_write.num_bytes = ARRAY_SIZE(data);
>> -       in.u.i2c_write.bytes = data;
>> -       DO_TEST();
>> -
>> -       in.req_type = DP_QUERY_STREAM_ENC_STATUS;
>> -       in.u.enc_status.stream_id = 1;
>> -       DO_TEST();
>> -       get_random_bytes(in.u.enc_status.client_id,
>> -                        sizeof(in.u.enc_status.client_id));
>> -       DO_TEST();
>> -       in.u.enc_status.stream_event = 3;
>> -       DO_TEST();
>> -       in.u.enc_status.valid_stream_event = 0;
>> -       DO_TEST();
>> -       in.u.enc_status.stream_behavior = 3;
>> -       DO_TEST();
>> -       in.u.enc_status.valid_stream_behavior = 1;
>> -       DO_TEST();
>> -
>> -#undef DO_TEST
>> -       return 0;
>> +       get_random_bytes(enc_status.client_id, sizeof(enc_status.client_id));
>> +
>> +       return (const struct drm_dp_sideband_msg_req_body) { .req_type = in->req_type,
>> +                                                            .u.enc_status = enc_status
>> +       };
>>   }
>> +
>> +static const struct dp_mst_sideband_msg_req_decode_test dp_msg_sideband_msg_req_decode_tests[] = {
>> +       {
>> +               .req = {
>> +                       .req_type = DP_ENUM_PATH_RESOURCES,
>> +                       .u.port_num.port_number = 5,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_POWER_UP_PHY,
>> +                       .u.port_num.port_number = 5,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_POWER_DOWN_PHY,
>> +                       .u.port_num.port_number = 5,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_ALLOCATE_PAYLOAD,
>> +                       .u.allocate_payload.number_sdp_streams = 3,
>> +                       .u.allocate_payload.sdp_stream_sink = { 1, 2, 3 },
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_ALLOCATE_PAYLOAD,
>> +                       .u.allocate_payload.port_number = 0xf,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_ALLOCATE_PAYLOAD,
>> +                       .u.allocate_payload.vcpi = 0x7f,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_ALLOCATE_PAYLOAD,
>> +                       .u.allocate_payload.pbn = U16_MAX,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_QUERY_PAYLOAD,
>> +                       .u.query_payload.port_number = 0xf,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_QUERY_PAYLOAD,
>> +                       .u.query_payload.vcpi = 0x7f,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_DPCD_READ,
>> +                       .u.dpcd_read.port_number = 0xf,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_DPCD_READ,
>> +                       .u.dpcd_read.dpcd_address = 0xfedcb,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_DPCD_READ,
>> +                       .u.dpcd_read.num_bytes = U8_MAX,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_DPCD_WRITE,
>> +                       .u.dpcd_write.port_number = 0xf,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_DPCD_WRITE,
>> +                       .u.dpcd_write.dpcd_address = 0xfedcb,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_DPCD_WRITE,
>> +                       .u.dpcd_write.num_bytes = ARRAY_SIZE(data),
>> +                       .u.dpcd_write.bytes = data,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_I2C_READ,
>> +                       .u.i2c_read.port_number = 0xf,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_I2C_READ,
>> +                       .u.i2c_read.read_i2c_device_id = 0x7f,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_I2C_READ,
>> +                       .u.i2c_read.num_transactions = 3,
>> +                       .u.i2c_read.num_bytes_read = ARRAY_SIZE(data) * 3,
>> +                       .u.i2c_read.transactions = {
>> +                               { .bytes = data, .num_bytes = ARRAY_SIZE(data),
>> +                                 .i2c_dev_id = 0x7f, .i2c_transaction_delay = 0xf, },
>> +                               { .bytes = data, .num_bytes = ARRAY_SIZE(data),
>> +                                 .i2c_dev_id = 0x7e, .i2c_transaction_delay = 0xe, },
>> +                               { .bytes = data, .num_bytes = ARRAY_SIZE(data),
>> +                                 .i2c_dev_id = 0x7d, .i2c_transaction_delay = 0xd, },
>> +                       },
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_I2C_WRITE,
>> +                       .u.i2c_write.port_number = 0xf,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_I2C_WRITE,
>> +                       .u.i2c_write.write_i2c_device_id = 0x7f,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_REMOTE_I2C_WRITE,
>> +                       .u.i2c_write.num_bytes = ARRAY_SIZE(data),
>> +                       .u.i2c_write.bytes = data,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
>> +                       .u.enc_status.stream_id = 1,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
>> +               },
>> +               .f = random_dp_query_enc_client_id,
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
>> +                       .u.enc_status.stream_event = 3,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
>> +                       .u.enc_status.valid_stream_event = 0,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
>> +                       .u.enc_status.stream_behavior = 3,
>> +               },
>> +       },
>> +       {
>> +               .req = {
>> +                       .req_type = DP_QUERY_STREAM_ENC_STATUS,
>> +                       .u.enc_status.valid_stream_behavior = 1,
>> +               },
>> +       },
>> +};
>> +
>> +KUNIT_ARRAY_PARAM(dp_mst_sideband_msg_req, dp_msg_sideband_msg_req_decode_tests, NULL);
>> +
>> +static struct kunit_case drm_dp_mst_helper_tests[] = {
>> +       KUNIT_CASE_PARAM(dp_mst_calc_pbn_mode, dp_mst_calc_pbn_mode_gen_params),
>> +       KUNIT_CASE_PARAM(dp_mst_sideband_msg_req_decode, dp_mst_sideband_msg_req_gen_params),
>> +       {}
>> +};
>> +
>> +static struct kunit_suite drm_dp_mst_helper_test_suite = {
>> +       .name = "drm_dp_mst_helper_tests",
>> +       .test_cases = drm_dp_mst_helper_tests,
>> +};
>> +
>> +kunit_test_suite(drm_dp_mst_helper_test_suite);
>> diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
>> index 1501d99aee2f..c7cc5edc65f1 100644
>> --- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h
>> +++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
>> @@ -20,7 +20,5 @@ int igt_drm_rect_clip_scaled_div_by_zero(void *ignored);
>>   int igt_drm_rect_clip_scaled_not_clipped(void *ignored);
>>   int igt_drm_rect_clip_scaled_clipped(void *ignored);
>>   int igt_drm_rect_clip_scaled_signed_vs_unsigned(void *ignored);
>> -int igt_dp_mst_calc_pbn_mode(void *ignored);
>> -int igt_dp_mst_sideband_msg_req_decode(void *ignored);
>>
>>   #endif
>> --
>> 2.34.1
>>
Daniel Latypov Jan. 21, 2022, 1 a.m. UTC | #3
On Thu, Jan 20, 2022 at 4:49 PM Michał Winiarski
<michal.winiarski@intel.com> wrote:
> > Tangent:
> > It might have been easier to do that if the kunit assertions returned pass/fail.
> > E.g. instead of having to do
> >
> > if (!<long-condition>) {
> >    KUNIT_FAIL("<long-condition> not met");
> >    return;
> > }
> >
> > if we could do
> >
> > if(!KUNIT_EXPECT_TRUE(long-condition))
> >    return;
> >
> > or if there was a new macro type
> >
> > KUNIT_EXPECT_RET_TRUE(long-condition); // like ASSERT, but just return
> > from this func on failure
>
> This would simplify a bunch of other tests as well.
> On the other hand - EXPECT_TRUE returning a value is not something I
> would expect :)

Yeah.
It felt painful to type that out :)

But I felt the same need when looking at converting some other selftests over.
It definitely feels like there's room to make these sorts of helper
functions better.

KTF solved these by allowing asserts to `goto` or `break`, e.g.
https://github.com/oracle/ktf/blob/63c19dead80de9cd654b08120d28a04f24174f4b/kernel/ktf.h#L560

I had floated the idea of KUnit having a
KUNIT_ASSERT_GOTO/KUNIT_ASSERT_RET (return)
macro, but these would add yet another dimension to the macros (EXPECT
vs ASSERT, _MSG vs no _MSG).

But I have some patches out that delete hundreds of lines from the
assert macros along with some others I haven't sent out publicly yet.
Maybe such a thing would be more palatable after those land?

But for now, I think they can either just print enough debug info so
that the failures are obvious (like this does), or they can use
kunit_err() to print out additional info (like you do in other patches
in this series).

>
> Thanks!
> -Michał
diff mbox series

Patch

diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
index 35f2f40dbaf3..77e37eebf099 100644
--- a/drivers/gpu/drm/selftests/Makefile
+++ b/drivers/gpu/drm/selftests/Makefile
@@ -1,7 +1,6 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 test-drm_modeset-$(CONFIG_DRM_DEBUG_SELFTEST) := \
 		      test-drm_modeset_common.o \
-		      test-drm_dp_mst_helper.o \
 		      test-drm_rect.o
 
 obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o
@@ -9,4 +8,4 @@  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o
 obj-$(CONFIG_DRM_KUNIT_TEST) := \
 	test-drm_cmdline_parser.o test-drm_plane_helper.o \
 	test-drm_format.o test-drm_framebuffer.o \
-	test-drm_damage_helper.o
+	test-drm_damage_helper.o test-drm_dp_mst_helper.o
diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
index b6a6dba66b64..630770d30aba 100644
--- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h
+++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
@@ -10,5 +10,3 @@  selftest(drm_rect_clip_scaled_div_by_zero, igt_drm_rect_clip_scaled_div_by_zero)
 selftest(drm_rect_clip_scaled_not_clipped, igt_drm_rect_clip_scaled_not_clipped)
 selftest(drm_rect_clip_scaled_clipped, igt_drm_rect_clip_scaled_clipped)
 selftest(drm_rect_clip_scaled_signed_vs_unsigned, igt_drm_rect_clip_scaled_signed_vs_unsigned)
-selftest(dp_mst_calc_pbn_mode, igt_dp_mst_calc_pbn_mode)
-selftest(dp_mst_sideband_msg_req_decode, igt_dp_mst_sideband_msg_req_decode)
diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
index 6b4759ed6bfd..d0719f3c5a42 100644
--- a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
+++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
@@ -3,54 +3,97 @@ 
  * Test cases for for the DRM DP MST helpers
  */
 
-#define PREFIX_STR "[drm_dp_mst_helper]"
-
+#include <kunit/test.h>
 #include <linux/random.h>
 
 #include <drm/drm_dp_mst_helper.h>
 #include <drm/drm_print.h>
 
 #include "../drm_dp_mst_topology_internal.h"
-#include "test-drm_modeset_common.h"
 
-int igt_dp_mst_calc_pbn_mode(void *ignored)
+struct dp_mst_calc_pbn_mode_test {
+	int rate;
+	int bpp;
+	int expected;
+	bool dsc;
+};
+
+static void dp_mst_calc_pbn_mode(struct kunit *test)
 {
-	int pbn, i;
-	const struct {
-		int rate;
-		int bpp;
-		int expected;
-		bool dsc;
-	} test_params[] = {
-		{ 154000, 30, 689, false },
-		{ 234000, 30, 1047, false },
-		{ 297000, 24, 1063, false },
-		{ 332880, 24, 50, true },
-		{ 324540, 24, 49, true },
-	};
+	const struct dp_mst_calc_pbn_mode_test *params = test->param_value;
+	int pbn;
 
-	for (i = 0; i < ARRAY_SIZE(test_params); i++) {
-		pbn = drm_dp_calc_pbn_mode(test_params[i].rate,
-					   test_params[i].bpp,
-					   test_params[i].dsc);
-		FAIL(pbn != test_params[i].expected,
-		     "Expected PBN %d for clock %d bpp %d, got %d\n",
-		     test_params[i].expected, test_params[i].rate,
-		     test_params[i].bpp, pbn);
-	}
+	pbn = drm_dp_calc_pbn_mode(params->rate,
+				   params->bpp,
+				   params->dsc);
+
+	KUNIT_EXPECT_EQ(test, pbn, params->expected);
+}
 
-	return 0;
+static const struct dp_mst_calc_pbn_mode_test dp_mst_calc_pbn_mode_tests[] = {
+	{
+		.rate = 154000,
+		.bpp = 30,
+		.expected = 689,
+		.dsc = false,
+	},
+	{
+		.rate = 234000,
+		.bpp = 30,
+		.expected = 1047,
+		.dsc = false,
+	},
+	{
+		.rate = 297000,
+		.bpp = 24,
+		.expected = 1063,
+		.dsc = false,
+	},
+	{
+		.rate = 332880,
+		.bpp = 24,
+		.expected = 50,
+		.dsc = true,
+	},
+	{
+		.rate = 324540,
+		.bpp = 24,
+		.expected = 49,
+		.dsc = true,
+	},
+};
+
+static void dp_mst_calc_pbn_mode_desc(const struct dp_mst_calc_pbn_mode_test *t,
+				      char *desc)
+{
+	sprintf(desc, "rate = %d, bpp = %d, dsc = %s",
+		t->rate, t->bpp, t->dsc ? "true" : "false");
 }
 
-static bool
-sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
-		       const struct drm_dp_sideband_msg_req_body *out)
+KUNIT_ARRAY_PARAM(dp_mst_calc_pbn_mode, dp_mst_calc_pbn_mode_tests, dp_mst_calc_pbn_mode_desc);
+
+static void
+drm_dp_mst_helper_printfn(struct drm_printer *p, struct va_format *vaf)
+{
+	struct kunit *test = p->arg;
+
+	kunit_err(test, "%pV", vaf);
+}
+
+static void
+expect_sideband_msg_req_equal(struct kunit *test,
+			      const struct drm_dp_sideband_msg_req_body *in,
+			      const struct drm_dp_sideband_msg_req_body *out)
 {
 	const struct drm_dp_remote_i2c_read_tx *txin, *txout;
+	struct drm_printer p = {
+		.printfn = drm_dp_mst_helper_printfn,
+		.arg = test
+	};
 	int i;
 
 	if (in->req_type != out->req_type)
-		return false;
+		goto fail;
 
 	switch (in->req_type) {
 	/*
@@ -65,7 +108,7 @@  sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
 		    IN.num_transactions != OUT.num_transactions ||
 		    IN.port_number != OUT.port_number ||
 		    IN.read_i2c_device_id != OUT.read_i2c_device_id)
-			return false;
+			goto fail;
 
 		for (i = 0; i < IN.num_transactions; i++) {
 			txin = &IN.transactions[i];
@@ -76,11 +119,11 @@  sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
 			    txin->num_bytes != txout->num_bytes ||
 			    txin->i2c_transaction_delay !=
 			    txout->i2c_transaction_delay)
-				return false;
+				goto fail;
 
 			if (memcmp(txin->bytes, txout->bytes,
 				   txin->num_bytes) != 0)
-				return false;
+				goto fail;
 		}
 		break;
 #undef IN
@@ -92,9 +135,12 @@  sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
 		if (IN.dpcd_address != OUT.dpcd_address ||
 		    IN.num_bytes != OUT.num_bytes ||
 		    IN.port_number != OUT.port_number)
-			return false;
+			goto fail;
 
-		return memcmp(IN.bytes, OUT.bytes, IN.num_bytes) == 0;
+		if (memcmp(IN.bytes, OUT.bytes, IN.num_bytes) != 0)
+			goto fail;
+
+		break;
 #undef IN
 #undef OUT
 
@@ -104,55 +150,65 @@  sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
 		if (IN.port_number != OUT.port_number ||
 		    IN.write_i2c_device_id != OUT.write_i2c_device_id ||
 		    IN.num_bytes != OUT.num_bytes)
-			return false;
+			goto fail;
 
-		return memcmp(IN.bytes, OUT.bytes, IN.num_bytes) == 0;
+		if (memcmp(IN.bytes, OUT.bytes, IN.num_bytes) != 0)
+			goto fail;
+
+		break;
 #undef IN
 #undef OUT
 
 	default:
-		return memcmp(in, out, sizeof(*in)) == 0;
+		if (memcmp(in, out, sizeof(*in)) != 0)
+			goto fail;
 	}
 
-	return true;
+	return;
+
+fail:
+	drm_printf(&p, "Expected:\n");
+	drm_dp_dump_sideband_msg_req_body(in, 1, &p);
+	drm_printf(&p, "Got:\n");
+	drm_dp_dump_sideband_msg_req_body(out, 1, &p);
+	KUNIT_FAIL(test, "Encode/decode failed");
+}
+
+struct dp_mst_sideband_msg_req_decode_test {
+	const struct drm_dp_sideband_msg_req_body req;
+	const struct drm_dp_sideband_msg_req_body
+		(*f)(const struct drm_dp_sideband_msg_req_body *in);
+};
+
+const struct drm_dp_sideband_msg_req_body
+param_to_dp_mst_sideband_msg_req_body(const struct dp_mst_sideband_msg_req_decode_test *t)
+{
+	if (t->f)
+		return t->f(&t->req);
+
+	return t->req;
 }
 
-static bool
-sideband_msg_req_encode_decode(struct drm_dp_sideband_msg_req_body *in)
+static void dp_mst_sideband_msg_req_decode(struct kunit *test)
 {
+	const struct drm_dp_sideband_msg_req_body in =
+		param_to_dp_mst_sideband_msg_req_body(test->param_value);
 	struct drm_dp_sideband_msg_req_body *out;
-	struct drm_printer p = drm_err_printer(PREFIX_STR);
 	struct drm_dp_sideband_msg_tx *txmsg;
-	int i, ret;
-	bool result = true;
-
-	out = kzalloc(sizeof(*out), GFP_KERNEL);
-	if (!out)
-		return false;
-
-	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
-	if (!txmsg)
-		return false;
-
-	drm_dp_encode_sideband_req(in, txmsg);
-	ret = drm_dp_decode_sideband_req(txmsg, out);
-	if (ret < 0) {
-		drm_printf(&p, "Failed to decode sideband request: %d\n",
-			   ret);
-		result = false;
-		goto out;
-	}
+	int i;
 
-	if (!sideband_msg_req_equal(in, out)) {
-		drm_printf(&p, "Encode/decode failed, expected:\n");
-		drm_dp_dump_sideband_msg_req_body(in, 1, &p);
-		drm_printf(&p, "Got:\n");
-		drm_dp_dump_sideband_msg_req_body(out, 1, &p);
-		result = false;
-		goto out;
-	}
+	out = kunit_kzalloc(test, sizeof(*out), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, out);
 
-	switch (in->req_type) {
+	txmsg = kunit_kzalloc(test, sizeof(*txmsg), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, txmsg);
+
+	drm_dp_encode_sideband_req(&in, txmsg);
+	KUNIT_ASSERT_EQ(test, drm_dp_decode_sideband_req(txmsg, out), 0);
+
+	expect_sideband_msg_req_equal(test, &in, out);
+
+	switch (in.req_type) {
 	case DP_REMOTE_DPCD_WRITE:
 		kfree(out->u.dpcd_write.bytes);
 		break;
@@ -164,110 +220,210 @@  sideband_msg_req_encode_decode(struct drm_dp_sideband_msg_req_body *in)
 		kfree(out->u.i2c_write.bytes);
 		break;
 	}
-
-	/* Clear everything but the req_type for the input */
-	memset(&in->u, 0, sizeof(in->u));
-
-out:
-	kfree(out);
-	kfree(txmsg);
-	return result;
 }
 
-int igt_dp_mst_sideband_msg_req_decode(void *unused)
+static u8 data[] = { 0xff, 0x0, 0xdd };
+
+const struct drm_dp_sideband_msg_req_body
+random_dp_query_enc_client_id(const struct drm_dp_sideband_msg_req_body *in)
 {
-	struct drm_dp_sideband_msg_req_body in = { 0 };
-	u8 data[] = { 0xff, 0x0, 0xdd };
-	int i;
+	struct drm_dp_query_stream_enc_status enc_status = { };
 
-#define DO_TEST() FAIL_ON(!sideband_msg_req_encode_decode(&in))
-
-	in.req_type = DP_ENUM_PATH_RESOURCES;
-	in.u.port_num.port_number = 5;
-	DO_TEST();
-
-	in.req_type = DP_POWER_UP_PHY;
-	in.u.port_num.port_number = 5;
-	DO_TEST();
-
-	in.req_type = DP_POWER_DOWN_PHY;
-	in.u.port_num.port_number = 5;
-	DO_TEST();
-
-	in.req_type = DP_ALLOCATE_PAYLOAD;
-	in.u.allocate_payload.number_sdp_streams = 3;
-	for (i = 0; i < in.u.allocate_payload.number_sdp_streams; i++)
-		in.u.allocate_payload.sdp_stream_sink[i] = i + 1;
-	DO_TEST();
-	in.u.allocate_payload.port_number = 0xf;
-	DO_TEST();
-	in.u.allocate_payload.vcpi = 0x7f;
-	DO_TEST();
-	in.u.allocate_payload.pbn = U16_MAX;
-	DO_TEST();
-
-	in.req_type = DP_QUERY_PAYLOAD;
-	in.u.query_payload.port_number = 0xf;
-	DO_TEST();
-	in.u.query_payload.vcpi = 0x7f;
-	DO_TEST();
-
-	in.req_type = DP_REMOTE_DPCD_READ;
-	in.u.dpcd_read.port_number = 0xf;
-	DO_TEST();
-	in.u.dpcd_read.dpcd_address = 0xfedcb;
-	DO_TEST();
-	in.u.dpcd_read.num_bytes = U8_MAX;
-	DO_TEST();
-
-	in.req_type = DP_REMOTE_DPCD_WRITE;
-	in.u.dpcd_write.port_number = 0xf;
-	DO_TEST();
-	in.u.dpcd_write.dpcd_address = 0xfedcb;
-	DO_TEST();
-	in.u.dpcd_write.num_bytes = ARRAY_SIZE(data);
-	in.u.dpcd_write.bytes = data;
-	DO_TEST();
-
-	in.req_type = DP_REMOTE_I2C_READ;
-	in.u.i2c_read.port_number = 0xf;
-	DO_TEST();
-	in.u.i2c_read.read_i2c_device_id = 0x7f;
-	DO_TEST();
-	in.u.i2c_read.num_transactions = 3;
-	in.u.i2c_read.num_bytes_read = ARRAY_SIZE(data) * 3;
-	for (i = 0; i < in.u.i2c_read.num_transactions; i++) {
-		in.u.i2c_read.transactions[i].bytes = data;
-		in.u.i2c_read.transactions[i].num_bytes = ARRAY_SIZE(data);
-		in.u.i2c_read.transactions[i].i2c_dev_id = 0x7f & ~i;
-		in.u.i2c_read.transactions[i].i2c_transaction_delay = 0xf & ~i;
-	}
-	DO_TEST();
-
-	in.req_type = DP_REMOTE_I2C_WRITE;
-	in.u.i2c_write.port_number = 0xf;
-	DO_TEST();
-	in.u.i2c_write.write_i2c_device_id = 0x7f;
-	DO_TEST();
-	in.u.i2c_write.num_bytes = ARRAY_SIZE(data);
-	in.u.i2c_write.bytes = data;
-	DO_TEST();
-
-	in.req_type = DP_QUERY_STREAM_ENC_STATUS;
-	in.u.enc_status.stream_id = 1;
-	DO_TEST();
-	get_random_bytes(in.u.enc_status.client_id,
-			 sizeof(in.u.enc_status.client_id));
-	DO_TEST();
-	in.u.enc_status.stream_event = 3;
-	DO_TEST();
-	in.u.enc_status.valid_stream_event = 0;
-	DO_TEST();
-	in.u.enc_status.stream_behavior = 3;
-	DO_TEST();
-	in.u.enc_status.valid_stream_behavior = 1;
-	DO_TEST();
-
-#undef DO_TEST
-	return 0;
+	get_random_bytes(enc_status.client_id, sizeof(enc_status.client_id));
+
+	return (const struct drm_dp_sideband_msg_req_body) { .req_type = in->req_type,
+							     .u.enc_status = enc_status
+	};
 }
+
+static const struct dp_mst_sideband_msg_req_decode_test dp_msg_sideband_msg_req_decode_tests[] = {
+	{
+		.req = {
+			.req_type = DP_ENUM_PATH_RESOURCES,
+			.u.port_num.port_number = 5,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_POWER_UP_PHY,
+			.u.port_num.port_number = 5,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_POWER_DOWN_PHY,
+			.u.port_num.port_number = 5,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_ALLOCATE_PAYLOAD,
+			.u.allocate_payload.number_sdp_streams = 3,
+			.u.allocate_payload.sdp_stream_sink = { 1, 2, 3 },
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_ALLOCATE_PAYLOAD,
+			.u.allocate_payload.port_number = 0xf,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_ALLOCATE_PAYLOAD,
+			.u.allocate_payload.vcpi = 0x7f,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_ALLOCATE_PAYLOAD,
+			.u.allocate_payload.pbn = U16_MAX,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_QUERY_PAYLOAD,
+			.u.query_payload.port_number = 0xf,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_QUERY_PAYLOAD,
+			.u.query_payload.vcpi = 0x7f,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_DPCD_READ,
+			.u.dpcd_read.port_number = 0xf,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_DPCD_READ,
+			.u.dpcd_read.dpcd_address = 0xfedcb,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_DPCD_READ,
+			.u.dpcd_read.num_bytes = U8_MAX,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_DPCD_WRITE,
+			.u.dpcd_write.port_number = 0xf,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_DPCD_WRITE,
+			.u.dpcd_write.dpcd_address = 0xfedcb,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_DPCD_WRITE,
+			.u.dpcd_write.num_bytes = ARRAY_SIZE(data),
+			.u.dpcd_write.bytes = data,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_I2C_READ,
+			.u.i2c_read.port_number = 0xf,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_I2C_READ,
+			.u.i2c_read.read_i2c_device_id = 0x7f,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_I2C_READ,
+			.u.i2c_read.num_transactions = 3,
+			.u.i2c_read.num_bytes_read = ARRAY_SIZE(data) * 3,
+			.u.i2c_read.transactions = {
+				{ .bytes = data, .num_bytes = ARRAY_SIZE(data),
+				  .i2c_dev_id = 0x7f, .i2c_transaction_delay = 0xf, },
+				{ .bytes = data, .num_bytes = ARRAY_SIZE(data),
+				  .i2c_dev_id = 0x7e, .i2c_transaction_delay = 0xe, },
+				{ .bytes = data, .num_bytes = ARRAY_SIZE(data),
+				  .i2c_dev_id = 0x7d, .i2c_transaction_delay = 0xd, },
+			},
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_I2C_WRITE,
+			.u.i2c_write.port_number = 0xf,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_I2C_WRITE,
+			.u.i2c_write.write_i2c_device_id = 0x7f,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_REMOTE_I2C_WRITE,
+			.u.i2c_write.num_bytes = ARRAY_SIZE(data),
+			.u.i2c_write.bytes = data,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_QUERY_STREAM_ENC_STATUS,
+			.u.enc_status.stream_id = 1,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_QUERY_STREAM_ENC_STATUS,
+		},
+		.f = random_dp_query_enc_client_id,
+	},
+	{
+		.req = {
+			.req_type = DP_QUERY_STREAM_ENC_STATUS,
+			.u.enc_status.stream_event = 3,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_QUERY_STREAM_ENC_STATUS,
+			.u.enc_status.valid_stream_event = 0,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_QUERY_STREAM_ENC_STATUS,
+			.u.enc_status.stream_behavior = 3,
+		},
+	},
+	{
+		.req = {
+			.req_type = DP_QUERY_STREAM_ENC_STATUS,
+			.u.enc_status.valid_stream_behavior = 1,
+		},
+	},
+};
+
+KUNIT_ARRAY_PARAM(dp_mst_sideband_msg_req, dp_msg_sideband_msg_req_decode_tests, NULL);
+
+static struct kunit_case drm_dp_mst_helper_tests[] = {
+	KUNIT_CASE_PARAM(dp_mst_calc_pbn_mode, dp_mst_calc_pbn_mode_gen_params),
+	KUNIT_CASE_PARAM(dp_mst_sideband_msg_req_decode, dp_mst_sideband_msg_req_gen_params),
+	{}
+};
+
+static struct kunit_suite drm_dp_mst_helper_test_suite = {
+	.name = "drm_dp_mst_helper_tests",
+	.test_cases = drm_dp_mst_helper_tests,
+};
+
+kunit_test_suite(drm_dp_mst_helper_test_suite);
diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
index 1501d99aee2f..c7cc5edc65f1 100644
--- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h
+++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
@@ -20,7 +20,5 @@  int igt_drm_rect_clip_scaled_div_by_zero(void *ignored);
 int igt_drm_rect_clip_scaled_not_clipped(void *ignored);
 int igt_drm_rect_clip_scaled_clipped(void *ignored);
 int igt_drm_rect_clip_scaled_signed_vs_unsigned(void *ignored);
-int igt_dp_mst_calc_pbn_mode(void *ignored);
-int igt_dp_mst_sideband_msg_req_decode(void *ignored);
 
 #endif