On Thu, Aug 11, 2022 at 9:38 PM Maíra Canal <mairacanal@xxxxxxxxxx> wrote:
>
>
>
> On 8/11/22 04:37, David Gow wrote:
> > On Thu, Aug 11, 2022 at 8:41 AM Tales Aparecida
> > <tales.aparecida@xxxxxxxxx> wrote:
> >>
> >> From: Maíra Canal <mairacanal@xxxxxxxxxx>
> >>
> >> Add unit test to the SubVP feature in order to avoid possible
> >> regressions and assure the code robustness.
> >>
> >> Signed-off-by: Maíra Canal <mairacanal@xxxxxxxxxx>
> >> Signed-off-by: Tales Aparecida <tales.aparecida@xxxxxxxxx>
> >> ---
> >
> > FYI: This seems to have a dependency issue. See below.
> >
> > Otherwise, I haven't had a chance to review it properly yet, but I'll
> > try to take a closer look over the next few days.
> >
> > Cheers,
> > -- David
> >
> >> drivers/gpu/drm/amd/display/dc/dc_dmub_srv.c | 4 +
> >> .../amd/display/tests/dc/dc_dmub_srv_test.c | 285 ++++++++++++++++++
> >> 2 files changed, 289 insertions(+)
> >> create mode 100644 drivers/gpu/drm/amd/display/tests/dc/dc_dmub_srv_test.c
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/dc/dc_dmub_srv.c b/drivers/gpu/drm/amd/display/dc/dc_dmub_srv.c
> >> index 2d61c2a91cee..f5dd1f69840e 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/dc_dmub_srv.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/dc_dmub_srv.c
> >> @@ -809,3 +809,7 @@ void dc_dmub_srv_log_diagnostic_data(struct dc_dmub_srv *dc_dmub_srv)
> >> diag_data.is_cw0_enabled,
> >> diag_data.is_cw6_enabled);
> >> }
> >> +
> >> +#if IS_ENABLED(CONFIG_AMD_DC_BASICS_KUNIT_TEST)
> >> +#include "../tests/dc/dc_dmub_srv_test.c"
> >> +#endif
> >> diff --git a/drivers/gpu/drm/amd/display/tests/dc/dc_dmub_srv_test.c b/drivers/gpu/drm/amd/display/tests/dc/dc_dmub_srv_test.c
> >> new file mode 100644
> >> index 000000000000..051079cbf65e
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/amd/display/tests/dc/dc_dmub_srv_test.c
> >> @@ -0,0 +1,285 @@
> >> +// SPDX-License-Identifier: MIT
> >> +/*
> >> + * KUnit tests for dc_dmub_srv.c
> >> + *
> >> + * Copyright (C) 2022, Maíra Canal <mairacanal@xxxxxxxxxx>
> >> + */
> >> +
> >> +#include <kunit/test.h>
> >> +#include "dc_dmub_srv.h"
> >> +
> >> +struct populate_subvp_cmd_drr_info_test_case {
> >> + const char *desc;
> >> + struct dc *dc;
> >> + struct pipe_ctx *subvp_pipe;
> >> + struct pipe_ctx *vblank_pipe;
> >> + const u8 drr_in_use;
> >> + const u8 drr_window_size_ms;
> >> + const u16 min_vtotal_supported;
> >> + const u16 max_vtotal_supported;
> >> + const u8 use_ramping;
> >> +};
> >> +
> >> +struct populate_subvp_cmd_drr_info_test_case populate_subvp_cmd_drr_info_cases[] = {
> >> + {
> >> + .desc = "Same Clock Frequency",
> >> + .dc = &(struct dc) {
> >> + .caps = {
> >> + .subvp_prefetch_end_to_mall_start_us = 0,
> >> + }
> >> + },
> >> + .subvp_pipe = &(struct pipe_ctx) {
> >> + .stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 2784,
> >> + .v_addressable = 1080,
> >> + .pix_clk_100hz = 1855800,
> >> + },
> >> + .mall_stream_config = {
> >> + .paired_stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 3600,
> >> + .v_total = 1111,
> >> + .v_addressable = 1080,
> >> + .v_front_porch = 3,
> >> + .pix_clk_100hz = 1855800,
> >> + },
> >> + },
> >> + },
> >> + },
> >> + },
> >> + .vblank_pipe = &(struct pipe_ctx) {
> >> + .stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 2784,
> >> + .v_total = 1111,
> >> + .v_addressable = 1080,
> >> + .pix_clk_100hz = 1855800,
> >> + },
> >> + },
> >> + },
> >> + .drr_in_use = true,
> >> + .use_ramping = false,
> >> + .drr_window_size_ms = 4,
> >> + .min_vtotal_supported = 2540,
> >> + .max_vtotal_supported = 2619,
> >> + },
> >> + {
> >> + .desc = "Same Clock Frequency with Prefetch End to Mall Start",
> >> + .dc = &(struct dc) {
> >> + .caps = {
> >> + .subvp_prefetch_end_to_mall_start_us = 500,
> >> + }
> >> + },
> >> + .subvp_pipe = &(struct pipe_ctx) {
> >> + .stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 2784,
> >> + .v_addressable = 1080,
> >> + .pix_clk_100hz = 1855800,
> >> + },
> >> + .mall_stream_config = {
> >> + .paired_stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 3600,
> >> + .v_total = 1111,
> >> + .v_addressable = 1080,
> >> + .v_front_porch = 3,
> >> + .pix_clk_100hz = 1855800,
> >> + },
> >> + },
> >> + },
> >> + },
> >> + },
> >> + .vblank_pipe = &(struct pipe_ctx) {
> >> + .stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 2784,
> >> + .v_total = 1111,
> >> + .v_addressable = 1080,
> >> + .pix_clk_100hz = 1855800,
> >> + },
> >> + },
> >> + },
> >> + .drr_in_use = true,
> >> + .use_ramping = false,
> >> + .drr_window_size_ms = 4,
> >> + .min_vtotal_supported = 2540,
> >> + .max_vtotal_supported = 2619,
> >> + },
> >> + {
> >> + .desc = "Same Clock Frequency Not Multiple of 2",
> >> + .dc = &(struct dc) {
> >> + .caps = {
> >> + .subvp_prefetch_end_to_mall_start_us = 0,
> >> + }
> >> + },
> >> + .subvp_pipe = &(struct pipe_ctx) {
> >> + .stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 2784,
> >> + .v_addressable = 1080,
> >> + .pix_clk_100hz = 1866743,
> >> + },
> >> + .mall_stream_config = {
> >> + .paired_stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 3600,
> >> + .v_total = 2400,
> >> + .v_addressable = 2360,
> >> + .v_front_porch = 4,
> >> + .pix_clk_100hz = 1866743,
> >> + },
> >> + },
> >> + },
> >> + },
> >> + },
> >> + .vblank_pipe = &(struct pipe_ctx) {
> >> + .stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 3600,
> >> + .v_total = 2400,
> >> + .v_addressable = 2360,
> >> + .pix_clk_100hz = 1866743,
> >> + },
> >> + },
> >> + },
> >> + .drr_in_use = true,
> >> + .use_ramping = false,
> >> + .drr_window_size_ms = 4,
> >> + .min_vtotal_supported = 1387,
> >> + .max_vtotal_supported = 2875,
> >> + },
> >> + {
> >> + .desc = "Different Clock Frequency for smaller h_total and v_total",
> >> + .dc = &(struct dc) {
> >> + .caps = {
> >> + .subvp_prefetch_end_to_mall_start_us = 300,
> >> + }
> >> + },
> >> + .subvp_pipe = &(struct pipe_ctx) {
> >> + .stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 1280,
> >> + .v_addressable = 600,
> >> + .pix_clk_100hz = 1855800,
> >> + },
> >> + .mall_stream_config = {
> >> + .paired_stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 1280,
> >> + .v_total = 720,
> >> + .v_addressable = 600,
> >> + .v_front_porch = 4,
> >> + .pix_clk_100hz = 1866743,
> >> + },
> >> + },
> >> + },
> >> + },
> >> + },
> >> + .vblank_pipe = &(struct pipe_ctx) {
> >> + .stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 1280,
> >> + .v_total = 720,
> >> + .v_addressable = 600,
> >> + .pix_clk_100hz = 2100800,
> >> + },
> >> + },
> >> + },
> >> + .drr_in_use = true,
> >> + .use_ramping = false,
> >> + .drr_window_size_ms = 4,
> >> + .min_vtotal_supported = 1477,
> >> + .max_vtotal_supported = 9954,
> >> + },
> >> + {
> >> + .desc = "Different Clock Frequency for approximately 1920x1080",
> >> + .dc = &(struct dc) {
> >> + .caps = {
> >> + .subvp_prefetch_end_to_mall_start_us = 0,
> >> + }
> >> + },
> >> + .subvp_pipe = &(struct pipe_ctx) {
> >> + .stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 1920,
> >> + .v_addressable = 1000,
> >> + .pix_clk_100hz = 1855800,
> >> + },
> >> + .mall_stream_config = {
> >> + .paired_stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 1911,
> >> + .v_total = 1080,
> >> + .v_addressable = 1000,
> >> + .v_front_porch = 7,
> >> + .pix_clk_100hz = 1866743,
> >> + },
> >> + },
> >> + },
> >> + },
> >> + },
> >> + .vblank_pipe = &(struct pipe_ctx) {
> >> + .stream = &(struct dc_stream_state) {
> >> + .timing = {
> >> + .h_total = 1280,
> >> + .v_total = 720,
> >> + .v_addressable = 600,
> >> + .pix_clk_100hz = 2100800,
> >> + },
> >> + },
> >> + },
> >> + .drr_in_use = true,
> >> + .use_ramping = false,
> >> + .drr_window_size_ms = 4,
> >> + .min_vtotal_supported = 2482,
> >> + .max_vtotal_supported = 8971,
> >> + },
> >> +};
> >> +
> >> +static void populate_subvp_cmd_drr_info_test_to_desc(struct
> >> + populate_subvp_cmd_drr_info_test_case * t, char *desc)
> >> +{
> >> + strcpy(desc, t->desc);
> >> +}
> >> +
> >> +KUNIT_ARRAY_PARAM(populate_subvp_cmd_drr_info, populate_subvp_cmd_drr_info_cases,
> >> + populate_subvp_cmd_drr_info_test_to_desc);
> >> +
> >> +static void populate_subvp_cmd_drr_info_test(struct kunit *test)
> >> +{
> >> + const struct populate_subvp_cmd_drr_info_test_case *test_param =
> >> + test->param_value;
> >> + struct dmub_cmd_fw_assisted_mclk_switch_pipe_data_v2 *pipe_data;
> >> +
> >> + pipe_data = kunit_kzalloc(test,
> >> + sizeof(struct dmub_cmd_fw_assisted_mclk_switch_pipe_data_v2),
> >> + GFP_KERNEL);
> >> +
> >> + populate_subvp_cmd_drr_info(test_param->dc, test_param->subvp_pipe,
> >> + test_param->vblank_pipe, pipe_data);
> >
> > Should this be hidden behind an #ifdef CONFIG_DRM_AMD_DC_DCN
> >
> > Otherwise, there are build issues under UML:
> > ../drivers/gpu/drm/amd/amdgpu/../display/dc/../tests/dc/dc_dmub_srv_test.c:
> > In function ‘populate_subvp_cmd_drr_info_test’:
> > ../drivers/gpu/drm/amd/amdgpu/../display/dc/../tests/dc/dc_dmub_srv_test.c:260:9:
> > error: implicit declaration of function ‘populate_subvp_cmd_drr_info’;
> > did you mean ‘populate_subvp_cmd_drr_info_test’? [-Werror
> > =implicit-function-declaration]
> > 260 | populate_subvp_cmd_drr_info(test_param->dc,
> > test_param->subvp_pipe,
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > | populate_subvp_cmd_drr_info_test
> > cc1: some warnings being treated as errors
> > make[5]: *** [../scripts/Makefile.build:249:
> > drivers/gpu/drm/amd/amdgpu/../display/dc/dc_dmub_srv.o] Error 1
> >
> > Making the test skip itself if this isn't enabled worked fine here.
> >
> >
>
> Currently, we don't support UML as there are some build problems on
> AMDGPU. You can apply the patch that I sent previously to run the tests
> with UML, and this warning will not happen anymore, but for now, the
> tests will work properly only on x86_64.
>
Fair enough: I definitely don't expect these all to work perfectly on
UML (particularly in v1).
> Moreover, I don't know if it is worth adding an #ifdef
> CONFIG_DRM_AMD_DC_DCN on all the functions, as most of the functions
> that we are testing are built under CONFIG_DRM_AMD_DC_DCN.
>
Even if it's not worth explicitly #ifdef-ing around individual tests
which use this, it's definitely important to make sure this test code
is behind some sort of dependency on CONFIG_DRM_AMD_DC_DCN. It
shouldn't be possible to create a valid .config which will cause a
compile error here. (It breaks, for example, randconfig builds.)
Could we at least have a "depends on" in the Kconfig, so that it's not
possible to create a broken config?
Cheers,
-- David
