Write tests for "drivers/gpu/drm/amd/display/dc/dml/dcn20/dcn20_fpu.c"

Siqueira sugeriu que escrever um teste sobre https://patchwork.freedesktop.org/patch/488777/?series=104903&rev=1 pode ajudar a vender o projeto

• Evaluate feasibility of writing tests for this file
• Write tests
  • dcn20_populate_dml_writeback_from_context
  • dcn20_fpu_set_wb_arb_params
  • dcn20_calculate_dlg_params
  • dcn20_populate_dml_pipes_from_context
  • dcn20_calculate_wm
  • dcn20_cap_soc_clocks
  • dcn20_update_bounding_box
  • dcn20_patch_bounding_box
  • dcn20_validate_bandwidth_fp
  • dcn20_fpu_set_wm_ranges
  • dcn20_fpu_adjust_dppclk
  • dcn21_populate_dml_pipes_from_context
  • dcn21_validate_bandwidth_fp
  • dcn21_update_bw_bounding_box
  • dcn21_clk_mgr_set_bw_params_wm_table
  • dcn201_populate_dml_writeback_from_context_fpu
  • is_dtbclk_required
  • dcn_zstate_support_state
  • swizzle_to_dml_params
  • dcn20_validate_bandwidth_internal
  • patch_bounding_box
  • calculate_wm_set_for_vlevel
  • dcn21_calculate_wm
  • _vcs_dpi_voltage_scaling_st

assigned to @magali

marked the checklist item Evaluate feasibility of writing tests for this file as completed

marked this issue as related to #7 (closed)

A função dcn20_cap_soc_clocks tem duas partes:

• a primeira basicamente verifica se os limites de clock da bounding box não ultrapassam os limites máximos especificados em max_clocks, e é corrigido para o máximo possível caso o valor seja extrapolado;
• a segunda verifica se há algum estado de clock repetido -- ou seja, verifica se há algum elemento do array clock_limits que seja idêntico a outro. Nessa parte, a verificação é feita com um for (i = bb->num_states - 1; i > 1; i--). Os elementos na posição 0 e 1 não são comparados. Então, caso o estado 0 e 1 sejam duplicados, a função não corrige o número de estados únicos totais...

added AMDGPU label

added InProgress label

changed the description

changed the description

A função dcn21_update_bw_bounding_box modifica variáveis globais[1] e chama outras funções[2].

[1] Acredito que não seria correto, por exemplo, modificar a variável dcn2_1_soc, que fica definida em dml/dcn20/dcn20_fpu.c, diretamente. Então, a ideia é redeclarar essa variável no arquivo de testes, para que possamos ter total controle sobre ela. Se eu faço isso, como esperado, recebo o erro: ERROR:root:ld: drivers/gpu/drm/amd/display/dc/dml/dcn20/dcn20_fpu.o:(.data+0x0): multiple definition of `dcn2_1_soc'; drivers/gpu/drm/amd/display/amdgpu_dm/tests/dcn20/dcn20_fpu_test.o:(.data+0x2c0): first defined here. Há também a modificação da variável dcn2_1_ip.

[2] Outra questão é que essa função chama outras funções. Novamente, acredito que deveríamos ter algum tipo de controle sobre essas funções "externas" a que estamos testando. O que encontrei que parece factível com o KUnit são redirecionamentos (?) de função: https://kunit.dev/mocking#function-redirection. Nesse link há algumas várias formas de fazer isso, com os prós e contras listados. Pelo que eu li e entendi, as opções apresentadas ou precisariam mudar o código original -- o que não desejamos --, ou não se aplica à forma como código original está escrito . Podemos, claro, apenas ignorar o fato de que existem outras funções sendo chamadas, mas não sei se o teste poderia ser classificado como ~unitário.

[2] Do que eu entendo de testes, não são chamadas de outras funções internamente que separa testes unitários de testes de integração, mesmo porque muitas vezes essas chamadas internas serão para funções privadas que costumam ficar fora do escopo dos testes unitários. No meu entendimento a separação está mais relacionada à interação de sistemas...

Mas de qualquer forma, o formalismo de unit x integration testing costuma ser irrelevante, o principal é garantir isolamento entre os casos de teste, por exemplo, garantindo que o valor da variável dcn2_1_soc de um teste não interfira em outro teste, o que nos leva à [1].

Particularmente não vejo problema em usar uma variável global, desde que seu valor esteja devidamente configurado em cada caso de teste (no entanto, isso pode ser proibitivamente complexo).

Então, a minha questão com a variável global é garantir que, ao executar o teste, a variável original não seja modificada. Para isso, eu precisaria fazer um override (?) da variável, ou algo assim. Faz sentido?

Uai, será que realmente precisamos de algo do gênero? Imagino que podemos fazer backup dos valores e restaurá-los após o teste, mas contornar a variável... com qual intenção?

Eu estou confuso pq até onde eu imagino, os testes do IGT são tão destrutivos quanto seria um teste aqui na dcn21_update_bw_bounding_box, isto é, até onde estou entendendo, do momento em que o driver está compilado com qualquer suite de testes kunit, ele vai ser usado exclusivamente para rodar os testes e eles têm que estar preparados pra inicializar todos valores que pretendem avaliar. (Tanto que existe a intenção de deixar o kernel flaggeado/tainted após executar testes)

Minha experiência com mocks é que eles servem mais para facilitar ou até permitir escrever testes, e não para proteger o sistema real rodando paralelamente...

Em outras palavras, acredito que "garantir que, ao executar o teste, a variável original não seja modificada" não é algo necessário, pelo contrário, é garantir que a variável "original" seja afetada como esperado.

Entendi. Obrigada pela explicação.

Vamos trazer essa pauta pra reunião, ver o que os demais pensam :P

Pra ficar registrado, então, Siqueira não vê problemas em não mockarmos essas chamadas por serem simples, até recomendou validarmos o resultado esperado baseado no comportamento delas

Outro patch que corrige bug nela: https://lists.freedesktop.org/archives/amd-gfx/2021-February/059347.html

mentioned in commit a9333bd3

changed title from Write tests for {--}drivers/gpu/drm/amd/display/dc/dml/dcn20/dcn20_fpu.c{--} to Write tests for "drivers/gpu/drm/amd/display/dc/dml/dcn20/dcn20_fpu.c"

unassigned @magali

assigned to @magali

changed the description

marked the checklist item dcn20_cap_soc_clocks as completed