For starters, add the specific guidelines in their own files, starting with drm-intel, with the intention of consolidating and pulling in common guides in the top level file in the long term.
Signed-off-by: Jani Nikula <[email protected]> --- committer-drm-intel.rst | 271 +++++++++++++++++++++++++++++++++++++++++++++++ committer-guidelines.rst | 12 +++ drm-intel.rst | 270 +--------------------------------------------- index.rst | 1 + 4 files changed, 288 insertions(+), 266 deletions(-) create mode 100644 committer-drm-intel.rst create mode 100644 committer-guidelines.rst diff --git a/committer-drm-intel.rst b/committer-drm-intel.rst new file mode 100644 index 000000000000..11ad3f4cadb5 --- /dev/null +++ b/committer-drm-intel.rst @@ -0,0 +1,271 @@ +.. _committer-drm-intel: + +================================ + drm-intel Committer Guidelines +================================ + +This document describes the detailed merge criteria and committer guidelines for +drm-intel. The same criteria and guidelines apply equally to both committers and +maintainers. + +High Level Guidelines +===================== + +This section describes the guidelines for pushing patches. Strict rules covering +all cases are impossible to write and follow. We put a lot of trust in the sound +judgement of the people with commit access, and that only develops with +experience. These guidelines are primarily for the committers to aid in making +the right decisions, and for others to set their the expectations right. + +The short list: + +* Only push patches changing `drivers/gpu/drm/i915`. + +* Only push patches to `drm-intel-next-queued` branch. + +* Ensure certain details are covered, see separate list below. + +* You have confidence in the patches you push, proportionate to the complexity + and impact they have. The confidence must be explicitly documented with + special tags (Reviewed-by, Acked-by, Tested-by, Bugzilla, etc.) in the commit + message. This is also your insurance, and you want to have it when the commit + comes back haunting you. The complexity and impact are properties of the patch + that must be justified in the commit message. + +* Last but not least, especially when getting started, you can't go wrong with + asking or deferring to the maintainers. If you don't feel comfortable pushing + a patch for any reason (technical concerns, unresolved or conflicting + feedback, management or peer pressure, or anything really) it's best to defer + to the maintainers. This is what the maintainers are there for. + +* After pushing, please reply to the list that you've done so. + +Detail Check List +================= + +An inexhaustive list of details to check: + +* The patch conforms to `Documentation/process/submitting-patches + <https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_ + +* The commit message is sensible, and includes adequate details and references. + +* Bug fixes are clearly indicated as such. + +* IGT test cases, if applicable, must be complete and reviewed. Please see + `details on testing requirements + <http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html>`_. + +* The patch series has passed CI pre-merge testing. See CI details below. + +* An open source userspace, reviewed and ready for merging by the upstream + project, must be available for new kernel ABI. Please see `details on + upstreaming requirements + <http://blog.ffwll.ch/2015/05/gfx-kernel-upstreaming-requirements.html>`_. + +* Relevant documentation must be updated as part of the patch or series. + +* Patch series builds and is expected to boot every step of the way, i.e. is + bisectable. + +* Each patch is of a sensible size. A good rule of thumb metric is, would you + (or the author) stand a chance to fairly quickly understand what goes wrong if + the commit is reported to cause a regression? + +* When pushing someone else's patch you must add your own signed-off per + http://developercertificate.org/. dim apply-branch should do this + automatically for you. + +* For patches that move around lots of code (file rename or extraction) please + coordinate with maintainers to avoid unnecessary pain with conflicts. Usually + some explicit merges are needed to avoid git getting lost. + +* As a general rule, do not modify the patches while applying, apart from the + commit message. If the patch conflicts, or needs to be changed due to review, + have the author rebase, update and resend. Any change at this stage is a + potential issue bypassing CI. + + At most, minor comment and whitespace tweaks are acceptable. + +* Please notify maintainers (IRC is fine) of new merge conflicts during drm-tip + rebuild, so that they can do backmerges as needed. + +On Confidence, Complexity, and Transparency +=========================================== + +* Reviewed-by/Acked-by/Tested-by must include the name and email of a real + person for transparency. Anyone can give these, and therefore you have to + value them according to the merits of the person. Quality matters, not + quantity. Be suspicious of rubber stamps. + +* Reviewed-by/Acked-by/Tested-by can be asked for and given informally (on the + list, IRC, in person, in a meeting) but must be added to the commit. + +* Reviewed-by. All patches must be reviewed, no exceptions. Please see + "Reviewer's statement of oversight" in `Documentation/process/submitting-patches + <https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_ + and `review training + <http://blog.ffwll.ch/2014/08/review-training-slides.html>`_. + +* Acked-by. Indicates acceptance. No reply is not a sign of acceptance, unless + you've involved the person (added Cc:, explicitly included in discussion). + +* Tested-by. Please solicit separate Tested-by especially from the bug + reporters, or people with specific hardware that you or the author do not + have. + +* There must not be open issues or unresolved or conflicting feedback from + anyone. Clear them up first. Defer to maintainers as needed. + +* For patches that are simple, isolated, non-functional, not visible to + userspace, and/or are in author or reviewer's domain of expertise, one + reviewer is enough. Author with commit access can push after review, or + reviewer with commit access can push after review. + +* The more complicated the patch gets, the greater the impact, involves ABI, + touches several areas or platforms, is outside of author's domain of + expertise, the more eyeballs are needed. For example, several reviewers, or + separate author, reviewer and committer. Make sure you have experienced + reviewers. Involve the domain expert(s). Possibly involve people across + team/group boundaries. Possibly involve the maintainers. Give people more time + to voice their concerns. Aim for what's described below in more detail as + "rough consensus". + +* Most patches fall somewhere in between. You have to be the judge, and ensure + you have involved enough people to feel comfortable if the justification for + the commit is questioned afterwards. + +On Rough Consensus +================== + +For really big features with a lot of impact on the code, new cross-driver ABI +(like new atomic properties), or features that will take a few patch series (and +maybe a few kernel releases) aim for rough consensus among a wide group of +stakeholders. There's three components for that: + +* Identify stakeholders beyond just the patch author and reviewers. This + includes contributors with experience in code ancillary to your feature, + domain experts, maybe maintainers. Also include maintainers and reviewers of + the userspace component for new ABI, which often means non-Intel people. In + case of doubt ask maintainers for a reasonable list of people. Make sure you + gather their input actively, don't expect them to deliver it on their own - + most are really busy. + +* Have agreement among all these stakeholders what the code should look like in + the end. Generally disagreement on the end state is considered a blocker, and + overruling such disagreement by maintainers is done only in exceptional cases. + Generally what happens is that maintainers instead do all the work of + soliciting input, which doesn't scale and should be the patch author's and + reviewer's duty. Try to document this consensus somewhere, either in a summary + email, interface patch to describe the data structures and vfuncs. Or maybe + even a more formal design spec, although in the past that didn't work out when + non-Intel people are involved, and the actually merged interface differed from + the spec a lot. Just assumed consensus on IRC tends to result in + misunderstandings. + +* Have a concrete plan how to get to the agreed end state in the codebase. Often + it makes sense to have an intermediate patch set merged first, and then polish + it in tree. Or merge new ABI in stages. Usually this means that a new feature + or ABI is disabled by default at first. For the actual plan some unhappiness + from stakeholders about the execution is acceptable, as long as the overall + stability and other ongoing work isn't negatively affected. As a rule of thumb + new ABI or features should never be in a partial/limbo stage for more than 2-3 + kernel releases. If that seems unlikely, more work should happen pre-merge. + + +Try to reach rough consensus before spending months writing code you might need +to throw away or at least entirely rewrite again. Also make sure that all +discussions happen in public forums, and make sure there's a searchable +permanent record of any discussions for later reference. This means that for +most things internal meetings are not the most suitable venue. + +Continuous Integration and Pre-Merge Testing +============================================ + +The requirements for CI_ pre-merge testing are: + +* ``checkpatch.pl`` does not complain. (Some of the more subjective warnings may + be ignored at the committer's discretion.) + +* The patch does not introduce new ``sparse`` warnings. + +* Patch series must pass IGT Basic Acceptance Tests (BAT) on all the CI machines + without causing regressions. + +* Patch series must pass full IGT tests on CI shard machines without causing + regressions. + +* Patch series must pass GPU piglit tests on all CI machines without causing + regressions. + +The CI bots will send results to the patch author and intel-gfx for any patches +tracked by patchwork. The results are also available on patchwork_ and the CI_ +site. + +Check CI failures and make sure any sporadic failures are a) pre-existing, +and b) tracked in bugzilla. If there's anything dubious that you can't track +down to pre-existing and tracked issues please don't push, but instead figure +out what's going on. + +.. _CI: https://intel-gfx-ci.01.org/ + +.. _patchwork: https://patchwork.freedesktop.org/project/intel-gfx/series/ + +Labeling Fixes Before Pushing +============================= + +To label fixes that should be cherry-picked to the current -rc development +kernel or drm-next, the commit message should have the 'Fixes:' tag. + +If the fix is relevant for a released kernel please also + + Cc: [email protected] + +'Fixes:' tag needs to added to any patch fixing a regression or an incorrect +behavior from previous patches. This tag help maintainers and tools to decide +where to cherry-pick a patch to. This also extremely useful for +product groups to know which bugfixes they must include. To follow the +recommended format please generate the Fixes: line using :: + + $ dim fixes $regressing_commit + +This will also add the correct Cc: line if one is needed. + +If the Cc: or Fixes: was forgotten, you can still reply to the list with that, +just like any other tags, and they should be picked up by whoever pushes the +patch. + +The maintainers will cherry-pick labeled patches from drm-intel-next-queued to +the appropriate branches. + +'Fixes:' tag is described in +`Documentation/process/submitting-patches +<https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_. + +Requesting Fixes Cherry-Pick Afterwards +======================================= + +It's not uncommon for a patch to have been committed before it's identified as a +fix needing to be backported. + +If the patch is already in Linus' tree, please follow `stable kernel rules +<https://01.org/linuxgraphics/gfx-docs/drm/process/stable-kernel-rules.html>`_. + +Otherwise, send an email to [email protected] containing the subject of the patch, the commit id, why you think it should be applied, and what branch you wish it to be applied to. + +Replying to the original patch is also fine, but please mention the commit id. + +Alternatively, if the cherry-pick has conflicts, please send a patch to [email protected] with +subject prefix "drm-intel-fixes PATCH" or "drm-intel-next-fixes PATCH" depending +on the branch. Please add 'git cherry-pick -x' style annotation above your +Signed-off-by: line in the commit message: + + (cherry picked from commit 0bff4858653312a10c83709e0009c3adb87e6f1e) + +Tooling +======= + +drm-intel git repositories are managed with dim_. + +.. _dim: dim.html diff --git a/committer-guidelines.rst b/committer-guidelines.rst new file mode 100644 index 000000000000..c0ed6d942aa7 --- /dev/null +++ b/committer-guidelines.rst @@ -0,0 +1,12 @@ +.. _committer-guidelines: + +====================== + Committer Guidelines +====================== + +This document gathers together committer guidelines. + +.. toctree:: + :maxdepth: 2 + + committer-drm-intel diff --git a/drm-intel.rst b/drm-intel.rst index 087402cf985d..4587396452f2 100644 --- a/drm-intel.rst +++ b/drm-intel.rst @@ -44,7 +44,7 @@ Features -------- Features are picked up and pushed to drm-intel-next-queued by committers and -maintainers. See committer guidelines below for details. +maintainers. See :ref:`committer-drm-intel` for details. Fixes ----- @@ -55,61 +55,9 @@ a timely manner. Fixes that are relevant for stable, current development kernels, or drm-next, will be cherry-picked by maintainers to drm-intel-fixes or drm-intel-next-fixes. -To make this work, patches should be labeled as fixes (see below), and extra -care should be put into making fixes the first patches in series, not depending -on preparatory work or cleanup. - -Labeling Fixes Before Pushing -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -To label fixes that should be cherry-picked to the current -rc development -kernel or drm-next, the commit message should have the 'Fixes:' tag. - -If the fix is relevant for a released kernel please also - - Cc: [email protected] - -'Fixes:' tag needs to added to any patch fixing a regression or an incorrect -behavior from previous patches. This tag help maintainers and tools to decide -where to cherry-pick a patch to. This also extremely useful for -product groups to know which bugfixes they must include. To follow the -recommended format please generate the Fixes: line using :: - - $ dim fixes $regressing_commit - -This will also add the correct Cc: line if one is needed. - -If the Cc: or Fixes: was forgotten, you can still reply to the list with that, -just like any other tags, and they should be picked up by whoever pushes the -patch. - -The maintainers will cherry-pick labeled patches from drm-intel-next-queued to -the appropriate branches. - -'Fixes:' tag is described in -`Documentation/process/submitting-patches -<https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_. - -Requesting Fixes Cherry-Pick Afterwards -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -It's not uncommon for a patch to have been committed before it's identified as a -fix needing to be backported. - -If the patch is already in Linus' tree, please follow `stable kernel rules -<https://01.org/linuxgraphics/gfx-docs/drm/process/stable-kernel-rules.html>`_. - -Otherwise, send an email to [email protected] containing the subject of the patch, the commit id, why you think it should be applied, and what branch you wish it to be applied to. - -Replying to the original patch is also fine, but please mention the commit id. - -Alternatively, if the cherry-pick has conflicts, please send a patch to [email protected] with -subject prefix "drm-intel-fixes PATCH" or "drm-intel-next-fixes PATCH" depending -on the branch. Please add 'git cherry-pick -x' style annotation above your -Signed-off-by: line in the commit message: - - (cherry picked from commit 0bff4858653312a10c83709e0009c3adb87e6f1e) +To make this work, patches should be labeled as fixes (see XXX), and extra care +should be put into making fixes the first patches in series, not depending on +preparatory work or cleanup. Merge Timeline ============== @@ -123,213 +71,3 @@ release. There are no fast paths. For predictions on the future merge windows and releases, see http://phb-crystal-ball.org/. - -Committer Guidelines -==================== - -This section describes the guidelines for pushing patches. Strict rules covering -all cases are impossible to write and follow. We put a lot of trust in the sound -judgement of the people with commit access, and that only develops with -experience. These guidelines are primarily for the committers to aid in making -the right decisions, and for others to set their the expectations right. - -The short list: - -* Only push patches changing `drivers/gpu/drm/i915`. - -* Only push patches to `drm-intel-next-queued` branch. - -* Ensure certain details are covered, see separate list below. - -* You have confidence in the patches you push, proportionate to the complexity - and impact they have. The confidence must be explicitly documented with - special tags (Reviewed-by, Acked-by, Tested-by, Bugzilla, etc.) in the commit - message. This is also your insurance, and you want to have it when the commit - comes back haunting you. The complexity and impact are properties of the patch - that must be justified in the commit message. - -* Last but not least, especially when getting started, you can't go wrong with - asking or deferring to the maintainers. If you don't feel comfortable pushing - a patch for any reason (technical concerns, unresolved or conflicting - feedback, management or peer pressure, or anything really) it's best to defer - to the maintainers. This is what the maintainers are there for. - -* After pushing, please reply to the list that you've done so. - -Detail Check List ------------------ - -An inexhaustive list of details to check: - -* The patch conforms to `Documentation/process/submitting-patches - <https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_ - -* The commit message is sensible, and includes adequate details and references. - -* Bug fixes are clearly indicated as such. - -* IGT test cases, if applicable, must be complete and reviewed. Please see - `details on testing requirements - <http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html>`_. - -* The patch series has passed CI pre-merge testing. See CI details below. - -* An open source userspace, reviewed and ready for merging by the upstream - project, must be available for new kernel ABI. Please see `details on - upstreaming requirements - <http://blog.ffwll.ch/2015/05/gfx-kernel-upstreaming-requirements.html>`_. - -* Relevant documentation must be updated as part of the patch or series. - -* Patch series builds and is expected to boot every step of the way, i.e. is - bisectable. - -* Each patch is of a sensible size. A good rule of thumb metric is, would you - (or the author) stand a chance to fairly quickly understand what goes wrong if - the commit is reported to cause a regression? - -* When pushing someone else's patch you must add your own signed-off per - http://developercertificate.org/. dim apply-branch should do this - automatically for you. - -* For patches that move around lots of code (file rename or extraction) please - coordinate with maintainers to avoid unnecessary pain with conflicts. Usually - some explicit merges are needed to avoid git getting lost. - -* As a general rule, do not modify the patches while applying, apart from the - commit message. If the patch conflicts, or needs to be changed due to review, - have the author rebase, update and resend. Any change at this stage is a - potential issue bypassing CI. - - At most, minor comment and whitespace tweaks are acceptable. - -* Please notify maintainers (IRC is fine) of new merge conflicts during drm-tip - rebuild, so that they can do backmerges as needed. - -On Confidence, Complexity, and Transparency -------------------------------------------- - -* Reviewed-by/Acked-by/Tested-by must include the name and email of a real - person for transparency. Anyone can give these, and therefore you have to - value them according to the merits of the person. Quality matters, not - quantity. Be suspicious of rubber stamps. - -* Reviewed-by/Acked-by/Tested-by can be asked for and given informally (on the - list, IRC, in person, in a meeting) but must be added to the commit. - -* Reviewed-by. All patches must be reviewed, no exceptions. Please see - "Reviewer's statement of oversight" in `Documentation/process/submitting-patches - <https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_ - and `review training - <http://blog.ffwll.ch/2014/08/review-training-slides.html>`_. - -* Acked-by. Indicates acceptance. No reply is not a sign of acceptance, unless - you've involved the person (added Cc:, explicitly included in discussion). - -* Tested-by. Please solicit separate Tested-by especially from the bug - reporters, or people with specific hardware that you or the author do not - have. - -* There must not be open issues or unresolved or conflicting feedback from - anyone. Clear them up first. Defer to maintainers as needed. - -* For patches that are simple, isolated, non-functional, not visible to - userspace, and/or are in author or reviewer's domain of expertise, one - reviewer is enough. Author with commit access can push after review, or - reviewer with commit access can push after review. - -* The more complicated the patch gets, the greater the impact, involves ABI, - touches several areas or platforms, is outside of author's domain of - expertise, the more eyeballs are needed. For example, several reviewers, or - separate author, reviewer and committer. Make sure you have experienced - reviewers. Involve the domain expert(s). Possibly involve people across - team/group boundaries. Possibly involve the maintainers. Give people more time - to voice their concerns. Aim for what's described below in more detail as - "rough consensus". - -* Most patches fall somewhere in between. You have to be the judge, and ensure - you have involved enough people to feel comfortable if the justification for - the commit is questioned afterwards. - -On Rough Consensus ------------------- - -For really big features with a lot of impact on the code, new cross-driver ABI -(like new atomic properties), or features that will take a few patch series (and -maybe a few kernel releases) aim for rough consensus among a wide group of -stakeholders. There's three components for that: - -* Identify stakeholders beyond just the patch author and reviewers. This - includes contributors with experience in code ancillary to your feature, - domain experts, maybe maintainers. Also include maintainers and reviewers of - the userspace component for new ABI, which often means non-Intel people. In - case of doubt ask maintainers for a reasonable list of people. Make sure you - gather their input actively, don't expect them to deliver it on their own - - most are really busy. - -* Have agreement among all these stakeholders what the code should look like in - the end. Generally disagreement on the end state is considered a blocker, and - overruling such disagreement by maintainers is done only in exceptional cases. - Generally what happens is that maintainers instead do all the work of - soliciting input, which doesn't scale and should be the patch author's and - reviewer's duty. Try to document this consensus somewhere, either in a summary - email, interface patch to describe the data structures and vfuncs. Or maybe - even a more formal design spec, although in the past that didn't work out when - non-Intel people are involved, and the actually merged interface differed from - the spec a lot. Just assumed consensus on IRC tends to result in - misunderstandings. - -* Have a concrete plan how to get to the agreed end state in the codebase. Often - it makes sense to have an intermediate patch set merged first, and then polish - it in tree. Or merge new ABI in stages. Usually this means that a new feature - or ABI is disabled by default at first. For the actual plan some unhappiness - from stakeholders about the execution is acceptable, as long as the overall - stability and other ongoing work isn't negatively affected. As a rule of thumb - new ABI or features should never be in a partial/limbo stage for more than 2-3 - kernel releases. If that seems unlikely, more work should happen pre-merge. - - -Try to reach rough consensus before spending months writing code you might need -to throw away or at least entirely rewrite again. Also make sure that all -discussions happen in public forums, and make sure there's a searchable -permanent record of any discussions for later reference. This means that for -most things internal meetings are not the most suitable venue. - -Continuous Integration and Pre-Merge Testing --------------------------------------------- - -The requirements for CI_ pre-merge testing are: - -* ``checkpatch.pl`` does not complain. (Some of the more subjective warnings may - be ignored at the committer's discretion.) - -* The patch does not introduce new ``sparse`` warnings. - -* Patch series must pass IGT Basic Acceptance Tests (BAT) on all the CI machines - without causing regressions. - -* Patch series must pass full IGT tests on CI shard machines without causing - regressions. - -* Patch series must pass GPU piglit tests on all CI machines without causing - regressions. - -The CI bots will send results to the patch author and intel-gfx for any patches -tracked by patchwork. The results are also available on patchwork_ and the CI_ -site. - -Check CI failures and make sure any sporadic failures are a) pre-existing, -and b) tracked in bugzilla. If there's anything dubious that you can't track -down to pre-existing and tracked issues please don't push, but instead figure -out what's going on. - -.. _CI: https://intel-gfx-ci.01.org/ - -.. _patchwork: https://patchwork.freedesktop.org/project/intel-gfx/series/ - -Tooling -======= - -drm-intel git repositories are managed with dim_: - -.. _dim: dim.html diff --git a/index.rst b/index.rst index 2a73f5de5bed..88b49edbeece 100644 --- a/index.rst +++ b/index.rst @@ -26,6 +26,7 @@ Contents: drm-tip drm-misc drm-intel + committer-guidelines commit-access getting-started dim -- 2.11.0 _______________________________________________ dim-tools mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dim-tools
