[PATCH 7/8] Documentation/gpu: Add an explanation about the DC weekly patches

[Jani Nikula]

On Fri, 20 Oct 2023, Rodrigo Siqueira <Rodrigo.Siqueira at amd.com> wrote:
> Sharing code with other OSes is confusing and raises some questions.
> This patch introduces some explanation about our upstream process with
> the shared code.

Thanks for writing this! It does help with the transparency.

Please find a comment inline.

>
> Cc: Mario Limonciello <mario.limonciello at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Harry Wentland <Harry.Wentland at amd.com>
> Cc: Hamza Mahfooz <hamza.mahfooz at amd.com>
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
> ---
>  Documentation/gpu/amdgpu/display/index.rst | 111 ++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/gpu/amdgpu/display/index.rst b/Documentation/gpu/amdgpu/display/index.rst
> index b09d1434754d..9d53a42c5339 100644
> --- a/Documentation/gpu/amdgpu/display/index.rst
> +++ b/Documentation/gpu/amdgpu/display/index.rst
> @@ -10,7 +10,114 @@ reason, our Display Core Driver is divided into two pieces:
>  1. **Display Core (DC)** contains the OS-agnostic components. Things like
>     hardware programming and resource management are handled here.
>  2. **Display Manager (DM)** contains the OS-dependent components. Hooks to the
> -   amdgpu base driver and DRM are implemented here.
> +   amdgpu base driver and DRM are implemented here. For example, you can check
> +   display/amdgpu_dm/ folder.
> +
> +--------------------
> +How AMD shares code?
> +--------------------
> +
> +Maintaining the same code-base across multiple OSes requires a lot of
> +synchronization effort between repositories. In the DC case, we maintain a
> +central repository where everyone who works from other OSes can put their
> +change in this centralized repository. In a simple way, this shared repository
> +is identical to all code that you can see in the display folder. The shared
> +repo has integration tests with our Linux CI farm, and we run an exhaustive set
> +of IGT tests in various AMD GPUs/APUs. Our CI also checks ARM64/32, PPC64/32,
> +and x86_64/32 compilation with DCN enabled and disabled. After all tests pass
> +and the developer gets reviewed by someone else, the change gets merged into
> +the shared repository.
> +
> +To maintain this shared code working properly, we run two activities every
> +week:
> +
> +1. **Weekly backport**: We bring changes from Linux to the other shared
> +   repositories. This work gets massive support from our CI tools, which can
> +   detect new changes and send them to internal maintainers.
> +2. **Weekly promotion**: Every week, we get changes from other teams in the
> +   shared repo that have yet to be made public. For this reason, at the
> +   beginning of each week, a developer will review that internal repo and
> +   prepare a series of patches that can be sent to the public upstream
> +   (promotion).
> +
> +For the context of this documentation, promotion is the essential part that
> +deserves a good elaboration here.
> +
> +Weekly promotion
> +----------------
> +
> +As described in the previous sections, the display folder has its equivalent as
> +an internal repository shared with multiple teams. The promotion activity is
> +the task of 'promoting' those internal changes to the upstream; this is
> +possible thanks to numerous tools that help us manage the code-sharing
> +challenges. The weekly promotion usually takes one week, sliced like this:
> +
> +1. Extract all merged patches from the previous week that can be sent to the
> +   upstream. In other words, we check the week's time frame.
> +2. Evaluate if any potential new patches make sense to the upstream.
> +3. Create a branch candidate with the latest amd-staging-drm-next code together
> +   with the new patches. At this step, we must ensure that every patch compiles
> +   and the entire series pass our set of IGT test in different hardware (i.e.,
> +   it has to pass to our CI).
> +4. Send the new candidate branch for an internal quality test and extra CI
> +   validation.
> +5. Send patches to amd-gfx for reviews. We wait a few days for community
> +   feedback after sending a series to the public mailing list.

So we've debated this one before. :)

Again, I applaud the transparency in writing the document, but I can't
help feeling the weekly promotions are code drops that will generally be
merged unchanged, with no comments. They have all been reviewed
internally, get posted with Reviewed-by tags pre-filled, we have no
visibility to the review. Since the code has already been merged
internally and the batch has passed CI, feels like the bar for changing
anything at this point is pretty high.

Just my two cents.


BR,
Jani.


(Side note, there should be a \n before 6.)

6. If there is
> +   an error, we debug as fast as possible; usually, a simple bisect in the
> +   weekly promotion patches points to a bad change, and we can take two
> +   possible actions: fix the issue or drop the patch. If we cannot identify the
> +   problem in the week interval, we drop the promotion and start over the
> +   following week; in this case, the following promotion will have the previous
> +   patches plus the new ones.
> +
> +We usually rotate the above process with many display developers to keep the
> +workload manageable for everybody. It is good to highlight that the test phase
> +is something that we take extremely seriously, and we never merge anything that
> +fails our validation. Just to give an overview:
> +
> +1. Manual test
> + - Multiple Hotplugs with DP and HDMI.
> + - Stress test with multiple display configuration changes via the user
> +   interface.
> + - Validate VRR behaviour.
> + - Check PSR.
> + - Validate MPO when playing video.
> + - Test more than two displays connected at the same time.
> + - Check suspend/resume.
> +2. Automated test
> + - IGT tests in a farm with GPUs and APUs that support DCN and DCE.
> + - Compilation validation with the latest GCC and Clang from LTS distro.
> + - Cross-compilation for PowerPC 64/32, ARM 64/32, and x86 32.
> +
> +Notice that all of the above tests happen in various AMD devices.
> +
> +Contributions to the weekly promotion
> +-------------------------------------
> +
> +If you have a patch and are unsure if it can cause regressions in other ASICs
> +and want some validation, you can ask us to include your patches in the weekly
> +promotion for validation. Just keep in mind that your patch will be included in
> +the next promotion cycle and re-submitted on your behalf (without changing your
> +authorship) by some of the display developers.
> +
> +The weekly promotion process is a very organic initiative that has changed
> +significantly over the years, thanks to numerous feedbacks. We are all ears if
> +you have any suggestions on how we can improve this process; just keep in mind
> +that this is a very challenging task, and implementing some ideas can take time
> +if possible.
> +
> +DC Workflow for a new feature
> +-----------------------------
> +
> +When we enable a new feature in the DC, the entire development workflow happens
> +on the amd-gfx mailing list. For example, when we enabled the PSR or the Replay
> +feature, all the development happened on amd-gfx. When enabling a new feature,
> +we just use promotion for extra validation in the latest patches by asking the
> +quality team to test the current promotion together with the new patches.
> +
> +--------------
> +DC Information
> +--------------
>  
>  The display pipe is responsible for "scanning out" a rendered frame from the
>  GPU memory (also called VRAM, FrameBuffer, etc.) to a display. In other words,
> @@ -26,8 +133,8 @@ table of content:
>  .. toctree::
>  
>     display-manager.rst
> -   dc-debug.rst
>     dcn-overview.rst
>     dcn-blocks.rst
>     mpo-overview.rst
> +   dc-debug.rst
>     dc-glossary.rst

-- 
Jani Nikula, Intel