Message ID | 6a3e19d75e504ebbf9cd9212faad12c005dfdfb8.1732541337.git.mchehab+huawei@kernel.org (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | docs: media: document media multi-committers rules and process | expand |
Hi Mauro and Hans, On Mon, Nov 25, 2024 at 02:28:58PM +0100, Mauro Carvalho Chehab wrote: > As the media subsystem will experiment with a multi-committers model, > update the Maintainer's entry profile to the new rules, and add a file > documenting the process to become a committer and to maintain such > rights. > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > Signed-off-by: Hans Verkuil <hverkuil@xs4ll.nl> > --- > Documentation/driver-api/media/index.rst | 1 + > .../media/maintainer-entry-profile.rst | 193 ++++++++++---- > .../driver-api/media/media-committer.rst | 252 ++++++++++++++++++ > .../process/maintainer-pgp-guide.rst | 2 + > 4 files changed, 398 insertions(+), 50 deletions(-) > create mode 100644 Documentation/driver-api/media/media-committer.rst > > diff --git a/Documentation/driver-api/media/index.rst b/Documentation/driver-api/media/index.rst > index d5593182a3f9..d0c725fcbc67 100644 > --- a/Documentation/driver-api/media/index.rst > +++ b/Documentation/driver-api/media/index.rst > @@ -26,6 +26,7 @@ Documentation/userspace-api/media/index.rst > :numbered: > > maintainer-entry-profile > + media-committer > > v4l2-core > dtv-core > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst > index ffc712a5f632..90c6c0d9cf17 100644 > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst > @@ -27,19 +27,128 @@ It covers, mainly, the contents of those directories: > Both media userspace and Kernel APIs are documented and the documentation > must be kept in sync with the API changes. It means that all patches that > add new features to the subsystem must also bring changes to the > -corresponding API files. > +corresponding API documentation files. I would have split this kind of small changes to a separate patch to make reviews easier, but that's not a big deal. > > -Due to the size and wide scope of the media subsystem, media's > -maintainership model is to have sub-maintainers that have a broad > -knowledge of a specific aspect of the subsystem. It is the sub-maintainers' > -task to review the patches, providing feedback to users if the patches are > +Due to the size and wide scope of the media subsystem, the media's > +maintainership model is to have committers that have a broad knowledge of > +a specific aspect of the subsystem. It is the committers' task to > +review the patches, providing feedback to users if the patches are > following the subsystem rules and are properly using the media kernel and > userspace APIs. This sounds really like a maintainer definition. I won't bikeshed too much on the wording though, we will always be able to adjust it later to reflect the reality of the situation as it evolves. I do like the removal of the "sub-maintainer" term though, as I've always found it demeaning. > > -Patches for the media subsystem must be sent to the media mailing list > -at linux-media@vger.kernel.org as plain text only e-mail. Emails with > -HTML will be automatically rejected by the mail server. It could be wise > -to also copy the sub-maintainer(s). > +Media committers > +---------------- > + > +In the media subsystem, there are experienced developers that can commit s/that/who/ s/commit/push/ to standardize the vocabulary (below you use "upload" to mean the same thing) > +patches directly on a development tree. These developers are called s/on a/to the/ > +Media committers and are divided into the following categories: > + > +- Committers: responsible for one or more drivers within the media subsystem. > + They can upload changes to the tree that do not affect the core or ABI. s/upload/push/ > + > +- Core committers: responsible for part of the media core. They are typically > + responsible for one or more drivers within the media subsystem, but, besides > + that, they can also merge patches that change the code common to multiple > + drivers, including the kernel internal API/ABI. I would write "API" only here. Neither the kernel internal API nor its internal ABI are stable, and given that lack of stability, the ABI concept doesn't really apply within the kernel. > + > +- Subsystem maintainers: responsible for the subsystem as a whole, with > + access to the entire subsystem. > + > + Only subsystem maintainers can change the userspace API/ABI. This can give the impression that only subsystem maintainers are allowed to work on the API. I would write Only subsystem maintainers change push changes that affect the userspace API/ABI. > + > +Media committers shall explicitly agree with the Kernel development process Do we have to capitalize "Kernel" everywhere ? There are way more occurrences of "kernel" than "Kernel" in Documentation/ (even excluding the lower case occurrences in e-mail addresses, file paths, ...). > +as described at Documentation/process/index.rst and to the Kernel > +development rules inside the Kernel documentation, including its code of > +conduct. That's expected of all kernel developers, but I don't object reiterating it here. > + > +More details about media committers can be found here: Maybe "about media commiters' roles and responsibilities" would be more accurate, the document doesn't include information about the committers themselves. s/here:/at/ > +Documentation/driver-api/media/media-committer.rst. Should this be a RST link ? > + > +Media development tree > +---------------------- > + > +The main development tree used by the media subsystem is hosted at LinuxTV.org, > +where we also maintain news about the subsystem, wiki pages and a patchwork > +instance where we track patches though their lifetime. > + > +The main tree used by media developers is at: > + > +https://git.linuxtv.org/media.git/ I think the multi-committers tree should be upgraded to main development tree status, but that's a separate issue that I'm fine discussing separately, and also delaying for a few kernel releases until we iron out the rough edges of the process. > + > +.. _Media development workflow: > + > +Media development workflow > +++++++++++++++++++++++++++ > + > +All changes for the media subsystem must be sent first as e-mails to the > +media mailing list, as plain text only e-mail to: > + > + `https://subspace.kernel.org/vger.kernel.org.html <linux-media@vger.kernel.org>`_ > + > +Emails with HTML will be automatically rejected by the mail server. > +It could be wise to also copy the media committer(s). You should use > +``scripts/get_maintainers.pl`` to identify whom else needs to be copied. > +Please always copy driver's authors and maintainers. Instead of duplicating a tiny part of the patch submission process, I would link to the appropriate documentation. [1] (see below) > + > +Such patches needed to be based against a public branch or tag as follows: s/needed/need/ > + > +1. new Kernel releases: s/new/New/ What does that mean though ? It's not very clear. > + > + Those need to be based at the ``next`` branch of that media.git tree > + > +2. During Kernel release development cycle, patches fixing bugs on a -rc > + kernel should preferably be against the latest -rc1 Kernel. If they > + require a previously-applied change, they need to be against the ``fixes`` > + branch; > + > +3. Patches against an already released kernel should preferably be against > + the latest released Kernel. If they require a previously-applied > + change, they need to be against ``fixes``. If I were a new contributor I think I would have trouble understand this to be honest. I won't push hard for a rework of this section, as I expect it will change after the multi-committer tree becomes the main way to get patches merged. We can then update the documentation. > + > +All patches with fixes shall have: > + - a ``Fixes:`` tag pointing to the first commit that introduced the bug; > + - a ``Cc: stable@vger.kernel.org`` > + > +Patches that were fixing bugs reported by someone else shall have: > + - a ``Reported-by`` tag immediately followed by a ``Closes`` tag. There's been a recent discussion about not including a Reported-by tag without asking permission from the reporter, due to privacy reasons (in particular for bugs reported to https://bugzilla.kernel.org/, as by default the e-mail address of the reporter is not public). As the Reported-by and Closes tags are not specific to the media tree, I would drop this paragraph, otherwise we will have to duplicate a relatively large amount of information related to privacy. You can link to the relevant documentation instead, but I wouldn't even do that as it's really not media-specific. > + > +Patches that change API/ABI shall require patches to update documentation s/require patches to // > +accordingly at the same patch series. s/at the/in the/ > + > +See Documentation/process/index.rst for more details about e-mail submission. This could be moved up to [1]. > + > +Once a patch is submitted, it may follow either one of the workflows > +below: "of the following workflows" > + > +a. Normal workflow: patches are handled by subsystem maintainers:: > + > + +------+ +---------+ +-------+ +-------------------+ +---------+ > + |e-mail|-->|patchwork|-->|pull |-->|maintainers merge |-->|media.git| > + +------+ +---------+ |request| |at media-committers| +---------+ > + +-------+ +-------------------+ This makes it look like the pull request originates from patchwork. s/at /in / I would also add .git to media-committers to make it clear it's a git tree. (I still think the tree name is too long and not very appropriate, but that's a different question) > + > + For this workflow, pull requests can be generated by a committer, > + a previous committer, subsystem maintainers or by a couple of trusted > + long-time contributors. If you are not in such group, please don't submit > + pull requests, as they will likely be ignored. > + > +b. Committers' workflow: patches are handled by media committers:: > + > + +------+ +---------+ +-------------------+ +-----------+ +---------+ > + |e-mail|-->|patchwork|-->|committers merge |-->|maintainers|-->|media.git| > + +------+ +---------+ |at media-committers| |approval | +---------+ > + +-------------------+ +-----------+ > + > +When patches are merged at patchwork and when merged at media-committers, We don't "merge" patches in patchwork. You could write "When patches are picked by patchwork" for instance. > +CI bots will check for errors and may provide e-mail feedback about > +patch problems. When this happens, the e-mail author must fix them s/e-mail author/patch author/ ? Or possibly better, "patch submitter" as that person may not be the author. > +and send another version of the patch. > + > +Patches will only be moved to the next stage in those two workflows if they > +don't fail on CI or if there are false-positives at the CI reports. s/at the/in the/ > + This workflow doesn't apply to patch submitters who are not allowed to send pull requests and who don't have direct commit access. I thought these submitters are the main audience of this document. In that case, I think moving the next section that explains the e-mail workflow before the "Media development workflow" section (which should likely be renamed to make it clear that it is about merging patches, not developing them) would be best. The "Review Cadence" section could also be folded in there, to give a full view of what a submitter can expect. This would also have the advantage of introducing the linuvtv.org patchwork instance, which you reference above. Documents are more readable when they introduce concepts first before using them. > +Failures during e-mail submission > ++++++++++++++++++++++++++++++++++ > > Media's workflow is heavily based on Patchwork, meaning that, once a patch > is submitted, the e-mail will first be accepted by the mailing list > @@ -47,51 +156,36 @@ server, and, after a while, it should appear at: > > - https://patchwork.linuxtv.org/project/linux-media/list/ > > -If it doesn't automatically appear there after a few minutes, then > +If it doesn't automatically appear there after some time [2]_, then > probably something went wrong on your submission. Please check if the > -email is in plain text\ [2]_ only and if your emailer is not mangling > +email is in plain text\ [3]_ only and if your emailer is not mangling > whitespaces before complaining or submitting them again. > > -You can check if the mailing list server accepted your patch, by looking at: > +To troubleshoot problems, you should first check if the mailing list > +server has accepted your patch, by looking at: > > - https://lore.kernel.org/linux-media/ > > -.. [2] If your email contains HTML, the mailing list server will simply > +If the patch is there and not at patchwork, it is likely that your e-mailer > +mangled the patch. Patchwork internally has a logic that checks if the > +received e-mail contain a valid patch. Any whitespace and new line > +breakages mangling the patch won't be recognized by patchwork, thus such > +patch will be rejected. > + > +.. [2] It usually takes a few minutes for the patch to arrive, but > + the e-mail server is busy, so it may take up to a few hours s/is busy/may be busy/ > + for a patch to be handled by the mail server and by the patchwork > + instance. "for a patch to be recorded by patchwork." > + > +.. [3] If your email contains HTML, the mailing list server will simply > drop it, without any further notice. These changes too could have been split to a separate cleanup patch, to make the crux of this patch easier to review. > > +Subsystem maintainers > +--------------------- > > -Media maintainers > -+++++++++++++++++ > - > -At the media subsystem, we have a group of senior developers that > -are responsible for doing the code reviews at the drivers (also known as > -sub-maintainers), and another senior developer responsible for the > -subsystem as a whole. For core changes, whenever possible, multiple > -media maintainers do the review. > - > -The media maintainers that work on specific areas of the subsystem are: > - > -- Remote Controllers (infrared): > - Sean Young <sean@mess.org> > - > -- HDMI CEC: > - Hans Verkuil <hverkuil@xs4all.nl> > - > -- Media controller drivers: > - Laurent Pinchart <laurent.pinchart@ideasonboard.com> > - > -- ISP, v4l2-async, v4l2-fwnode, v4l2-flash-led-class and Sensor drivers: > - Sakari Ailus <sakari.ailus@linux.intel.com> > - > -- V4L2 drivers and core V4L2 frameworks: > - Hans Verkuil <hverkuil@xs4all.nl> We're losing that information, isn't it valuable ? > - > -The subsystem maintainer is: > - Mauro Carvalho Chehab <mchehab@kernel.org> > - > -Media maintainers may delegate a patch to other media maintainers as needed. > -On such case, checkpatch's ``delegate`` field indicates who's currently > -responsible for reviewing a patch. > +The subsystem maintainers are: > + Mauro Carvalho Chehab <mchehab@kernel.org> and > + Hans Verkuil <hverkuil@xs4all.nl> If you don't intend on this being rendered as a list, it should be The subsystem maintainers are Mauro Carvalho Chehab <mchehab@kernel.org> and Hans Verkuil <hverkuil@xs4all.nl>. Otherwise, The subsystem maintainers are: - Mauro Carvalho Chehab <mchehab@kernel.org> - Hans Verkuil <hverkuil@xs4all.nl> Shouldn't MAINTAINERS be updated at the same time ? > > Submit Checklist Addendum > ------------------------- > @@ -108,17 +202,14 @@ implementing the media APIs: > ==================== ======================================================= > Type Tool > ==================== ======================================================= > -V4L2 drivers\ [3]_ ``v4l2-compliance`` > +V4L2 drivers\ [4]_ ``v4l2-compliance`` > V4L2 virtual drivers ``contrib/test/test-media`` > CEC drivers ``cec-compliance`` > ==================== ======================================================= > > -.. [3] The ``v4l2-compliance`` also covers the media controller usage inside > +.. [4] The ``v4l2-compliance`` also covers the media controller usage inside > V4L2 drivers. > > -Other compilance tools are under development to check other parts of the > -subsystem. > - > Those tests need to pass before the patches go upstream. > > Also, please notice that we build the Kernel with:: > @@ -134,6 +225,8 @@ Where the check script is:: > Be sure to not introduce new warnings on your patches without a > very good reason. > > +Please see `Media development workflow`_ for e-mail submission rules. > + These hunks too could have been split to a cleanup patch before the main changes. > Style Cleanup Patches > +++++++++++++++++++++ > > diff --git a/Documentation/driver-api/media/media-committer.rst b/Documentation/driver-api/media/media-committer.rst > new file mode 100644 > index 000000000000..27b85a37fb2b > --- /dev/null > +++ b/Documentation/driver-api/media/media-committer.rst > @@ -0,0 +1,252 @@ > +Media committers > +================ > + > +What is a media committer? > +-------------------------- > + > +A media committer is a developer who can apply patches from other developers Here too we could standardize to "push" instead of "apply". > +and their own patches at the s/at the/to the/ I would simply write "who can push commits to the ...". > +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_ > +tree. > + > +It is a media committer's duty to ensure that their entries at the MAINTAINERS s/at the/in the/ > +file will be kept updated, and that submitted patches for files for which s/will be kept update/are kept up-to-date/ > +they are listed as maintainers are timely reviewed at the mailing list, s/at the/on the/ > +not waiting in patchwork as ``New`` for more than one Kernel merge cycle, > +and, if accepted, applying them at the media committer's tree. I think this goes a bit backward, and mixes things up a bit. On the mixing side, the expectation of timely reviews comes from maintainer status. Having commit rights is orthogonal to that. The goal of direct commit access is to speed up maintenance, to get patches reviewed and merged quicker. Are we saying here that if someone has commit rights they will lose them if they take too long to review code ? That would then slow down maintenance even more, which seems counterproductive. Also, while one can be recognized as a maintainer for multiple drivers or parts of the kernel, there's a single committer status. You can't revoke committer status of a particular driver only. > + > +This privilege is granted with some expectation of responsibility: "Privilege" sounds a bit like lord and serf. > +committers are people who care about the Linux Kernel as a whole and > +about the Linux media subsystem and want to help its development. It > +is also based on a trust relationship between the rest of the committers, > +maintainers and the LinuxTV community. Who is "the LinuxTV community" ? > + > +As such, a media committer is not just someone who is capable of creating > +code, but someone who has demonstrated their ability to collaborate > +with the team, get the most knowledgeable people to review code, > +contribute high-quality code, and follow through to fix issues (in code > +or tests). > + > +.. Note:: > + > + 1. If a patch introduced a regression, then it is the media committer's s/introduced/introduces/ > + responsibility to correct that as soon as possible. Typically the > + patch is either reverted, or an additional patch is committed that > + fixes the regression; > + 2. if patches are fixing bugs against already released Kernels, including > + the reverts above mentioned, the media committer shall add the needed > + tags. Please see :ref:`Media development workflow` for more details. s/ Please/ Please/ > + 3. all patches should be properly reviewed at > + linux-media@vger.kernel.org before being merged at the s/at the/in the/ > + media-committers tree or submitted on pull requests. This is a fundamental rule that belongs to the development workflow in my opinion. > + > +Becoming a media committer > +-------------------------- > + > +The most important aspect of volunteering to be a committer is that you will > +be able to review and approve other people's changes, so we are looking for Everybody is able to review patches (rather, everybody is allowed to review patches, the ability is a different matter). > +whether we think you will be good at doing that. I've been told that "whether" should also come with a "or" clause. You can write "whether or not we think ...". > + > +As such, potential committers must earn enough credibility and trust from the > +LinuxTV community. To do that, developers shall be familiar with the open > +source model and have been active at the Linux Kernel community for some time, s/at the/in the/ > +and, in particular, with the media subsystem. s/with the/in the/ > + > +So, in addition to actually making the code changes, you are basically > +demonstrating your: > + > +- commitment to the project; > +- ability to collaborate with the team and communicate well; > +- understand of how upstream and the LinuxTV community works s/ works/ work/ > + (policies, processes for testing, code review, ...) > +- reasonable knowledge about: > + > + - the Kernel development process: > + Documentation/process/index.rst > + > + - the Media development profile: > + Documentation/driver-api/media/maintainer-entry-profile.rst > + > +- understanding of the projects' code base and coding style; > +- ability to provide feedback to the patch authors; > +- ability to judge when a patch might be ready for review and to submit; > +- ability to write good code (last but certainly not least). > + > +It is also desirable that developers that intend to become committers > +make a best effort to attend the yearly Linux Media Summit, typically > +co-located with another Linux conference. I would say that "are encouraged to attend" instead of "make a best effort to attend". Also, how will this scale when we'll have a few dozen committers ? Typically the media summit is capped to 20 attendees or less. > + > +If you are doing such tasks and have become a valued developer, an > +existing committer can nominate you to the media subsystem maintainers. https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: "Maintainers and committers should encourage contributors to request commit rights, especially junior contributors tend to underestimate their skills." > + > +The ultimate responsibility for accepting a nominated committer is up to > +the subsystem's maintainers. The committers must earn a trust relationship > +with all subsystem maintainers, as, by granting you commit rights, they will > +be delegating part of their maintenance tasks. > + > +Due to that, to become a committer or a core committer, a consensus between > +all subsystem maintainers is required, as they all need to trust a developer > +well enough to be delegated the responsibility to maintain part of the code > +and to properly review patches from third parties, in a timely manner and > +keeping the status of the reviewed code at https://patchwork.linuxtv.org > +updated. > + > +.. Note:: > + > + In order to preserve the developers that could have their commit rights Do you mean "protect" instead of "preserve" ? Or maybe "preserve/protect the privacy of" ? s/that/who/ s/their // (in the granted case, they don't have commit rights yet) > + granted or denied as well as the subsystem maintainers who have the "granted, denied or removed" ? > + task to accept or deny commit rights, all communication related to > + nominating a committer, preserving commit rights or leaving such function > + should happen in private as much as possible. > + > +Media committer's agreement > +--------------------------- > + > +Once a nominated committer is accepted by all subsystem maintainers, > +they will ask if the developer is interested in the nomination and discuss > +what area(s) of the media subsystem the committer will be responsible for. > + > +Once the developer accepts being a committer, the new committer shall > +explicitly accept the Kernel development policies described under its > +Documentation/, and, in particular to the rules on this document, by writing > +an e-mail to media-committers@linuxtv.org, with a declaration of intent > +following the model below:: > + > + I, John Doe, would like to change my status to: Committer > + > + I intend to actively develop the XYZ driver, send fixes to drivers > + that I can test, reviewing patches and merging trivial fixes > + for the subsystem, ... "Merging trivial fixes for the subsystem" bothers me. I don't think it needs to be a requirement for committers. This is a maintainer's responsibility. If people want to help with that that's great, but making it a requirement isn't. Or did you mean this as an example ? We shouldn't expect committers to handle a higher workload than what they do as driver maintainers who submit patches by e-mail or send pull requests. Giving commit rights will lower the effort to get patches in, and I think it's fair to ask for keeping patchwork up-to-date in return, but that's about it. > + > +Followed by a formal declaration of agreement with the Kernel development > +rules, signed with a PGP key cross signed by other Kernel and media s/PGP/GPG/ (same thing in practice, but let's advocate for free software) > +developers. Such declaration shall be:: I find the GPG signature requirement to be borderline ridiculous. The first message you're giving to committers is that you distrust them so much that you want them to sign an agreement with their blood (figuratively speaking). I don't think it's a very good approach to community building, nor does it bring any advantage to anyone. > + > + I hereby declare that I agree with the Kernel development rules described at: > + > + https://www.kernel.org/doc/html/latest/driver-api/media/media-committer.rst > + > + and to the Linux Kernel development process rules. > + > + I agree to the Code of Conduct as documented here: s/here:/in/ > + Documentation/process/code-of-conduct.rst You use an http link for meda-commiter.rst and a file path for code-of-conduct.rst. RST links for both would be more consistent, but if the goal is to paste the text in an e-mail, you can also use http links or file paths for both. > + > + I am aware that I can, at any point of time, retire. In that case, I will > + send an e-mail to notify the subsystem maintainers for them to revoke my > + commit rights. > + > + I am aware that the Kernel development rules change over time. > + By doing a new commit, I understand that I agree with the rules in effect What does "doing a new commit" mean here ? Sending a patch to the list ? Pushing it to the shared tree ? I assume the latter given the text below, it could be clarified here. > + at the time of the commit. > + > +For more details about PGP sign, please read > +Documentation/process/maintainer-pgp-guide.rst and > +:ref:`kernel_org_trust_repository`. > + > +In case the kernel development process changes, by merging new commits at the s/at the/in the/ > +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_, s/media-committers/media-committers tree/ > +the media committer implicitly declares that the agreement with the latest s/that the/their/ > +version of the documented process and to the contents of this file. s/to the contents // ("latest version" applies to both "the documented process" and "this file") This is problematic, as we can't expect people to check for changes in this file every time they push something. Changes to this file should be announced to all committers, with a reasonable review period. > + > +Core committers > +--------------- > + > +As described in Documentation/driver-api/media/maintainer-entry-profile.rst RST link here too. > +a committer may be granted with additional privileges to also be able to s/privileges/rights/ (same below) > +change a core file and/or media subsystem's Kernel API/ABI. The extent of Drop "/ABI" as above. > +the core committer's additional privileges will be detailed by the subsystem > +maintainers when they nominate a core committer. > + > +Existing committers may become core committers and vice versa. Such > +decisions will be taken in consensus between the subsystem maintainers. > + > +Media committers rules > +---------------------- > + > +Media committers shall ensure that merged patches will not break any existing > +drivers. If it breaks, fixup or revert patches shall be merged as soon as How do they ensure that ? I would prefer mentioning a best effort here, as it's practically not possible to ensure this (until we have CI covering all the drivers in the subsystem, which is not a reasonable target). > +possible, aiming to be merged at the same Kernel cycle the bug is reported. > + > +Media committers shall behave accordingly to the permissions granted by s/permissions/rights/ (we never use "permission" anywhere else) > +the subsystem maintainers, specially with regards of the scope of changes > +they may apply directly at the media-committers tree. Such scope can > +change overtime on a mutual greement between media committers and s/overtime/over time/ s/greement/agreement/ > +maintainers. > + > +As described at :ref:`Media development workflow`, there are workflows. s/at /in / > +For the committers' workflow, the following rules apply: > + > +- Each merged patch shall pass CI tests; > + > +- Media committers shall request reviews from other committers were We shouldn't limit this to "other committers". When making changes to a driver that is not maintained by any committer (for instance when making tree-wide changes), the person in the best position to review the changes is the driver maintainer or author. I would extend this to "from third parties", or "from other developers". > + applicable, i.e. because those committers have more knowledge about > + some areas that are changed by a patch; > + > +- No other media committer would be against the proposed changes. https://drm.pages.freedesktop.org/maintainer-tools/committer/committer-drm-misc.html#merge-criteria: "There must not be open issues or unresolved or conflicting feedback from anyone. Clear them up first. Defer to maintainers as needed." That's better than restricting the conflicts to committers. If there are open issues, the patches should not be merged through the shared tree. > + > +Patches that do not fall under the committer's workflow criteria will follow > +the normal workflow as described at :ref:`Media development workflow`. > + > +Only a subsystem maintainer can override such rules. > + > +All media committers shall ensure that patchwork will reflect the current > +status, e.g. patches shall be delegated to the media committer who is > +handling them and the patch status shall be updated according to these rules: > + > +- ``Under review``: Used if the patch requires a second opinion > + or when it is part of a pull request; > +- ``Accepted``: Once a patch is merged at the multi-committer tree. s/at the/in the/ > +- ``Superseded``: There is a newer version of the patch posted in the s/in the/to the/ > + mailing list. > +- ``Duplicated``: There was another patch doing the same thing from someone > + else that was accepted. > +- ``Not Applicable``: Use for patch series that are not merged at media.git > + tree (e.g. drm, dmabuf, upstream merge, etc.) but were cross-posted to the > + linux-media mailing list. > + > +If the committer decides not to merge it, then reply by email to patch > +authors, explaining why it is not merged, and patchwork shall be updated > +accordingly with either: > + > +- ``Changes Requested``: if a new revision was requested; > +- ``Rejected``: if the proposed change won't be merged upstream. There are tools to ease updating the status of a patch, could you document or at least mention them ? > + > +If a media committer decides to retire, it is the committer's duty to > +notify the subsystem maintainers about that decision. > + > +Maintaining media committer status > +---------------------------------- > + > +A community of committers working together to move the Linux Kernel > +forward is essential to creating successful projects that are rewarding > +to work on. If there are problems or disagreements within the community, > +they can usually be solved through healthy discussion and debate. > + > +In the unhappy event that a media committer continues to disregard good > +citizenship (or actively disrupts the project), we may need to revoke That's very, very vague, surprisingly vague even from someone who raised many concerns about the kernel code of conduct being vague. > +that person's status. In such cases, if someone suggests the revocation with > +a good reason, other developers may second the motion. The final decision > +is taken by the subsystem maintainers. As the decision to become a media What does "seconding the motion" bring, if the decision lies solely in maintainers ? > +committer comes from a consensus between subsystem maintainers, a single > +subsystem maintainer not trusting the media committer anymore is enough to > +revoke committer's privileges. > + > +If a committer is inactive for more than a couple of Kernel cycles, > +maintainers will try to reach you via e-mail. If not possible, they may > +revoke your committer privileges and update MAINTAINERS file entries > +accordingly. If you wish to resume contributing later on, then contact > +the subsystem maintainers to ask if your rights can be restored. https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: "Committers are encouraged to request their commit rights get removed when they no longer contribute to the project. Commit rights may be automatically revoked after a year of inactivity (no commits or reviews). Commit rights will be reinstated when they come back to the project." > + > +A previous committer that had his commit rights revoked can keep contributing s/his/their/ > +to the subsystem via the normal e-mail workflow as documented at the > +:ref:`Media development workflow`. > + > +References > +---------- > + > +Much of this was inspired by/copied from the committer policies of: > + > +- `Chromium <https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md>`_; > +- `WebKit <http://www.google.com/url?q=http%3A%2F%2Fwebkit.org%2Fcoding%2Fcommit-review-policy.html&sa=D&sntz=1&usg=AFrqEze4W4Lvbhue4Bywqgbv-N5J66kQgA>`_; Google tracks us enough without using google URLs. > +- `Mozilla <http://www.google.com/url?q=http%3A%2F%2Fwww.mozilla.org%2Fhacking%2Fcommitter%2F&sa=D&sntz=1&usg=AFrqEzecK7iiXqV30jKibNmmMtzHwtYRTg>`_. https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html would also have been a good source of inspiration. That's the only large multi-committer workflow today in the kernel, and it has proven its value. The explicit acceptance criteria in particular are very good. Quoting the document, it says "Commit rights will be granted to anyone who requests them and fulfills the below criteria:" That's how we build an inclusive community, it feels way more welcoming than saying that maintainers will discuss in private and grant privileges to underlings if it pleases them (even if the effect is the same in practice, it's still a maintainer decision). > + > diff --git a/Documentation/process/maintainer-pgp-guide.rst b/Documentation/process/maintainer-pgp-guide.rst > index f5277993b195..795ef8d89271 100644 > --- a/Documentation/process/maintainer-pgp-guide.rst > +++ b/Documentation/process/maintainer-pgp-guide.rst > @@ -903,6 +903,8 @@ the new default in GnuPG v2). To set it, add (or modify) the > > trust-model tofu+pgp > > +.. _kernel_org_trust_repository: > + > Using the kernel.org web of trust repository > -------------------------------------------- >
Em Tue, 26 Nov 2024 17:19:30 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi Mauro and Hans, > > On Mon, Nov 25, 2024 at 02:28:58PM +0100, Mauro Carvalho Chehab wrote: > > As the media subsystem will experiment with a multi-committers model, > > update the Maintainer's entry profile to the new rules, and add a file > > documenting the process to become a committer and to maintain such > > rights. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > Signed-off-by: Hans Verkuil <hverkuil@xs4ll.nl> > > --- > > Documentation/driver-api/media/index.rst | 1 + > > .../media/maintainer-entry-profile.rst | 193 ++++++++++---- > > .../driver-api/media/media-committer.rst | 252 ++++++++++++++++++ > > .../process/maintainer-pgp-guide.rst | 2 + > > 4 files changed, 398 insertions(+), 50 deletions(-) > > create mode 100644 Documentation/driver-api/media/media-committer.rst > > > > diff --git a/Documentation/driver-api/media/index.rst b/Documentation/driver-api/media/index.rst > > index d5593182a3f9..d0c725fcbc67 100644 > > --- a/Documentation/driver-api/media/index.rst > > +++ b/Documentation/driver-api/media/index.rst > > @@ -26,6 +26,7 @@ Documentation/userspace-api/media/index.rst > > :numbered: > > > > maintainer-entry-profile > > + media-committer > > > > v4l2-core > > dtv-core > > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst > > index ffc712a5f632..90c6c0d9cf17 100644 > > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst > > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst > > @@ -27,19 +27,128 @@ It covers, mainly, the contents of those directories: > > Both media userspace and Kernel APIs are documented and the documentation > > must be kept in sync with the API changes. It means that all patches that > > add new features to the subsystem must also bring changes to the > > -corresponding API files. > > +corresponding API documentation files. > > I would have split this kind of small changes to a separate patch to > make reviews easier, but that's not a big deal. > > > > > -Due to the size and wide scope of the media subsystem, media's > > -maintainership model is to have sub-maintainers that have a broad > > -knowledge of a specific aspect of the subsystem. It is the sub-maintainers' > > -task to review the patches, providing feedback to users if the patches are > > +Due to the size and wide scope of the media subsystem, the media's > > +maintainership model is to have committers that have a broad knowledge of > > +a specific aspect of the subsystem. It is the committers' task to > > +review the patches, providing feedback to users if the patches are > > following the subsystem rules and are properly using the media kernel and > > userspace APIs. > > This sounds really like a maintainer definition. I won't bikeshed too > much on the wording though, we will always be able to adjust it later to > reflect the reality of the situation as it evolves. I do like the > removal of the "sub-maintainer" term though, as I've always found it > demeaning. The main goal here was to replace sub-maintainers by committers. Other changes can go later on, or if you have a better way to define the paper of committers, be welcomed to propose it. > > -Patches for the media subsystem must be sent to the media mailing list > > -at linux-media@vger.kernel.org as plain text only e-mail. Emails with > > -HTML will be automatically rejected by the mail server. It could be wise > > -to also copy the sub-maintainer(s). > > +Media committers > > +---------------- > > + > > +In the media subsystem, there are experienced developers that can commit > > s/that/who/ > s/commit/push/ to standardize the vocabulary (below you use "upload" to > mean the same thing) > > > +patches directly on a development tree. These developers are called > > s/on a/to the/ I won't comment here any below about trivial changes like that: I'll just incorporate at v2. I'll focus my reply on your the comments about the text contents itself. > > > +Media committers and are divided into the following categories: > > + > > +- Committers: responsible for one or more drivers within the media subsystem. > > + They can upload changes to the tree that do not affect the core or ABI. > > s/upload/push/ > > > + > > +- Core committers: responsible for part of the media core. They are typically > > + responsible for one or more drivers within the media subsystem, but, besides > > + that, they can also merge patches that change the code common to multiple > > + drivers, including the kernel internal API/ABI. > > I would write "API" only here. Neither the kernel internal API nor its > internal ABI are stable, and given that lack of stability, the ABI > concept doesn't really apply within the kernel. It does for distros, but this is a separate matter ;-) From my side, I'm ok with either API or API/ABI, if ok for Hans and others as well. > > + > > +- Subsystem maintainers: responsible for the subsystem as a whole, with > > + access to the entire subsystem. > > + > > + Only subsystem maintainers can change the userspace API/ABI. > > This can give the impression that only subsystem maintainers are allowed > to work on the API. I would write > > Only subsystem maintainers change push changes that affect the userspace > API/ABI. I guess you meant to say: Only subsystem maintainers *can* push changes that affect the userspace API/ABI. (change -> can) The new text seems OK to me. I'll change it at v2. > > > + > > +Media committers shall explicitly agree with the Kernel development process > > Do we have to capitalize "Kernel" everywhere ? There are way more > occurrences of "kernel" than "Kernel" in Documentation/ (even excluding > the lower case occurrences in e-mail addresses, file paths, ...). "The Kernel" means "the Linux Kernel". e. g. it is not just a random kernel, it is the Linux Kernel. So, it is a proper noun. Proper nouns should always be capitalized in English. > > +as described at Documentation/process/index.rst and to the Kernel > > +development rules inside the Kernel documentation, including its code of > > +conduct. > > That's expected of all kernel developers, but I don't object reiterating > it here. > > > + > > +More details about media committers can be found here: > > Maybe "about media commiters' roles and responsibilities" would be more > accurate, the document doesn't include information about the committers > themselves. > > s/here:/at/ > > > +Documentation/driver-api/media/media-committer.rst. > > Should this be a RST link ? No need: a Sphinx plugin automatically converts it to hyperlink. > > + > > +Media development tree > > +---------------------- > > + > > +The main development tree used by the media subsystem is hosted at LinuxTV.org, > > +where we also maintain news about the subsystem, wiki pages and a patchwork > > +instance where we track patches though their lifetime. > > + > > +The main tree used by media developers is at: > > + > > +https://git.linuxtv.org/media.git/ > > I think the multi-committers tree should be upgraded to main development > tree status, but that's a separate issue that I'm fine discussing > separately, and also delaying for a few kernel releases until we iron > out the rough edges of the process. This is a separate discussion, and it is too early to start discussing it. > > +.. _Media development workflow: > > + > > +Media development workflow > > +++++++++++++++++++++++++++ > > + > > +All changes for the media subsystem must be sent first as e-mails to the > > +media mailing list, as plain text only e-mail to: > > + > > + `https://subspace.kernel.org/vger.kernel.org.html <linux-media@vger.kernel.org>`_ > > + > > +Emails with HTML will be automatically rejected by the mail server. > > +It could be wise to also copy the media committer(s). You should use > > +``scripts/get_maintainers.pl`` to identify whom else needs to be copied. > > +Please always copy driver's authors and maintainers. > > Instead of duplicating a tiny part of the patch submission process, I > would link to the appropriate documentation. The intention here is to bold that driver's authors and maintainers should always be c/c and the way to do it is via get_maintainers.pl. Yet, a link to the process submission makes sense. I'll move the link to here. > > [1] (see below) > > > + > > +Such patches needed to be based against a public branch or tag as follows: > > s/needed/need/ > > > + > > +1. new Kernel releases: > > s/new/New/ > > What does that mean though ? It's not very clear. Maybe "Upcoming Kernel releases"? > > + > > + Those need to be based at the ``next`` branch of that media.git tree > > + > > +2. During Kernel release development cycle, patches fixing bugs on a -rc > > + kernel should preferably be against the latest -rc1 Kernel. If they > > + require a previously-applied change, they need to be against the ``fixes`` > > + branch; > > + > > +3. Patches against an already released kernel should preferably be against > > + the latest released Kernel. If they require a previously-applied > > + change, they need to be against ``fixes``. > > If I were a new contributor I think I would have trouble understand this > to be honest. I won't push hard for a rework of this section, as I > expect it will change after the multi-committer tree becomes the main > way to get patches merged. We can then update the documentation. Basically, the preference is to have patches against a release or against -rc1 when it makes sense, as it helps when backports are needed. I don't think this will ever change. Patches against media.git next branch are ok, specially for new features. > > > + > > +All patches with fixes shall have: > > + - a ``Fixes:`` tag pointing to the first commit that introduced the bug; > > + - a ``Cc: stable@vger.kernel.org`` > > + > > +Patches that were fixing bugs reported by someone else shall have: > > + - a ``Reported-by`` tag immediately followed by a ``Closes`` tag. > > There's been a recent discussion about not including a Reported-by tag > without asking permission from the reporter, due to privacy reasons (in > particular for bugs reported to https://bugzilla.kernel.org/, as by > default the e-mail address of the reporter is not public). As the > Reported-by and Closes tags are not specific to the media tree, I would > drop this paragraph, otherwise we will have to duplicate a relatively > large amount of information related to privacy. You can link to the > relevant documentation instead, but I wouldn't even do that as it's > really not media-specific. Good point, but: 1. I can't remember last time I saw a media patch pointing to a Kernel BZ in the recent days (surely there are, but they're not frequent); 2. We do have a common pattern on media about people adding Reported-by: without Closes:, or with closes at the wrong place. So, we're trying to address those problematic cases. Maybe we could change the above to: Patches with fixes shall have: - a ``Fixes:`` tag pointing to the first commit that introduced the bug; - when applicable, a ``Cc: stable@vger.kernel.org``. Patches that were fixing bugs publicly reported by someone at the media mailing list shall have: - a ``Reported-by`` tag immediately followed by a ``Closes`` tag. > > > + > > +Patches that change API/ABI shall require patches to update documentation > > s/require patches to // > > > +accordingly at the same patch series. > > s/at the/in the/ > > > + > > +See Documentation/process/index.rst for more details about e-mail submission. > > This could be moved up to [1]. > > > + > > +Once a patch is submitted, it may follow either one of the workflows > > +below: > > "of the following workflows" > > > + > > +a. Normal workflow: patches are handled by subsystem maintainers:: > > + > > + +------+ +---------+ +-------+ +-------------------+ +---------+ > > + |e-mail|-->|patchwork|-->|pull |-->|maintainers merge |-->|media.git| > > + +------+ +---------+ |request| |at media-committers| +---------+ > > + +-------+ +-------------------+ > > This makes it look like the pull request originates from patchwork. No, it points that patches from e-mail go to patchwork, and the PRs come from what it is in patchwork(*). (*) Now, it doesn't matter if one picks it manually from the mailing list or picked with b4 or pwclient, the point is that the patch shall be in patchwork before someone does a pull request, as the patch state at patchwork needs to reflect the patch review lifetime. > > s/at /in / > > I would also add .git to media-committers to make it clear it's a git > tree. Ok. > > (I still think the tree name is too long and not very appropriate, but > that's a different question) > > > + > > + For this workflow, pull requests can be generated by a committer, > > + a previous committer, subsystem maintainers or by a couple of trusted > > + long-time contributors. If you are not in such group, please don't submit > > + pull requests, as they will likely be ignored. > > + > > +b. Committers' workflow: patches are handled by media committers:: > > + > > + +------+ +---------+ +-------------------+ +-----------+ +---------+ > > + |e-mail|-->|patchwork|-->|committers merge |-->|maintainers|-->|media.git| > > + +------+ +---------+ |at media-committers| |approval | +---------+ > > + +-------------------+ +-----------+ > > + > > +When patches are merged at patchwork and when merged at media-committers, > > We don't "merge" patches in patchwork. You could write "When patches are > picked by patchwork" for instance. Works for me. I'll change it in v2. > > > +CI bots will check for errors and may provide e-mail feedback about > > +patch problems. When this happens, the e-mail author must fix them > > s/e-mail author/patch author/ ? Or possibly better, "patch submitter" as > that person may not be the author. Ok. > > +and send another version of the patch. > > + > > +Patches will only be moved to the next stage in those two workflows if they > > +don't fail on CI or if there are false-positives at the CI reports. > > s/at the/in the/ > > > + > > This workflow doesn't apply to patch submitters who are not allowed to > send pull requests and who don't have direct commit access. I thought > these submitters are the main audience of this document. In that case, I > think moving the next section that explains the e-mail workflow before > the "Media development workflow" section (which should likely be renamed > to make it clear that it is about merging patches, not developing them) > would be best. The "Review Cadence" section could also be folded in > there, to give a full view of what a submitter can expect. > > This would also have the advantage of introducing the linuvtv.org > patchwork instance, which you reference above. Documents are more > readable when they introduce concepts first before using them. Will try to do such change at v2. > > > +Failures during e-mail submission > > ++++++++++++++++++++++++++++++++++ > > > > Media's workflow is heavily based on Patchwork, meaning that, once a patch > > is submitted, the e-mail will first be accepted by the mailing list > > @@ -47,51 +156,36 @@ server, and, after a while, it should appear at: > > > > - https://patchwork.linuxtv.org/project/linux-media/list/ > > > > -If it doesn't automatically appear there after a few minutes, then > > +If it doesn't automatically appear there after some time [2]_, then > > probably something went wrong on your submission. Please check if the > > -email is in plain text\ [2]_ only and if your emailer is not mangling > > +email is in plain text\ [3]_ only and if your emailer is not mangling > > whitespaces before complaining or submitting them again. > > > > -You can check if the mailing list server accepted your patch, by looking at: > > +To troubleshoot problems, you should first check if the mailing list > > +server has accepted your patch, by looking at: > > > > - https://lore.kernel.org/linux-media/ > > > > -.. [2] If your email contains HTML, the mailing list server will simply > > +If the patch is there and not at patchwork, it is likely that your e-mailer > > +mangled the patch. Patchwork internally has a logic that checks if the > > +received e-mail contain a valid patch. Any whitespace and new line > > +breakages mangling the patch won't be recognized by patchwork, thus such > > +patch will be rejected. > > + > > +.. [2] It usually takes a few minutes for the patch to arrive, but > > + the e-mail server is busy, so it may take up to a few hours > > s/is busy/may be busy/ > > > + for a patch to be handled by the mail server and by the patchwork > > + instance. > > "for a patch to be recorded by patchwork." > > > + > > +.. [3] If your email contains HTML, the mailing list server will simply > > drop it, without any further notice. > > These changes too could have been split to a separate cleanup patch, to > make the crux of this patch easier to review. > > > > > +Subsystem maintainers > > +--------------------- > > > > -Media maintainers > > -+++++++++++++++++ > > - > > -At the media subsystem, we have a group of senior developers that > > -are responsible for doing the code reviews at the drivers (also known as > > -sub-maintainers), and another senior developer responsible for the > > -subsystem as a whole. For core changes, whenever possible, multiple > > -media maintainers do the review. > > - > > -The media maintainers that work on specific areas of the subsystem are: > > - > > -- Remote Controllers (infrared): > > - Sean Young <sean@mess.org> > > - > > -- HDMI CEC: > > - Hans Verkuil <hverkuil@xs4all.nl> > > - > > -- Media controller drivers: > > - Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > - > > -- ISP, v4l2-async, v4l2-fwnode, v4l2-flash-led-class and Sensor drivers: > > - Sakari Ailus <sakari.ailus@linux.intel.com> > > - > > -- V4L2 drivers and core V4L2 frameworks: > > - Hans Verkuil <hverkuil@xs4all.nl> > > We're losing that information, isn't it valuable ? With the media-committers model, there's no concept of "media maintainers" anymore. Also, the above will dynamically change over time. Instead of placing things here, it makes more sense to ensure that the MAINTAINERS entry gets updated accordingly as we add media-committers, as this is what get-maintainers.pl uses. Besides that, the auto-delegation rules at patchwork will help keeping the patches c/c to the right person, in the eventual case someone doesn't run get-maintainers.pl. > > > - > > -The subsystem maintainer is: > > - Mauro Carvalho Chehab <mchehab@kernel.org> > > - > > -Media maintainers may delegate a patch to other media maintainers as needed. > > -On such case, checkpatch's ``delegate`` field indicates who's currently > > -responsible for reviewing a patch. > > +The subsystem maintainers are: > > + Mauro Carvalho Chehab <mchehab@kernel.org> and > > + Hans Verkuil <hverkuil@xs4all.nl> > > If you don't intend on this being rendered as a list, it should be > > The subsystem maintainers are Mauro Carvalho Chehab <mchehab@kernel.org> and > Hans Verkuil <hverkuil@xs4all.nl>. > > Otherwise, > > The subsystem maintainers are: > > - Mauro Carvalho Chehab <mchehab@kernel.org> > - Hans Verkuil <hverkuil@xs4all.nl> > > > Shouldn't MAINTAINERS be updated at the same time ? It should be a separate patch. I would be doing such change once Hans will be about to start sending pull requests, which still require more actions. > > > > Submit Checklist Addendum > > ------------------------- > > @@ -108,17 +202,14 @@ implementing the media APIs: > > ==================== ======================================================= > > Type Tool > > ==================== ======================================================= > > -V4L2 drivers\ [3]_ ``v4l2-compliance`` > > +V4L2 drivers\ [4]_ ``v4l2-compliance`` > > V4L2 virtual drivers ``contrib/test/test-media`` > > CEC drivers ``cec-compliance`` > > ==================== ======================================================= > > > > -.. [3] The ``v4l2-compliance`` also covers the media controller usage inside > > +.. [4] The ``v4l2-compliance`` also covers the media controller usage inside > > V4L2 drivers. > > > > -Other compilance tools are under development to check other parts of the > > -subsystem. > > - > > Those tests need to pass before the patches go upstream. > > > > Also, please notice that we build the Kernel with:: > > @@ -134,6 +225,8 @@ Where the check script is:: > > Be sure to not introduce new warnings on your patches without a > > very good reason. > > > > +Please see `Media development workflow`_ for e-mail submission rules. > > + > > These hunks too could have been split to a cleanup patch before the main > changes. > > > Style Cleanup Patches > > +++++++++++++++++++++ > > > > diff --git a/Documentation/driver-api/media/media-committer.rst b/Documentation/driver-api/media/media-committer.rst > > new file mode 100644 > > index 000000000000..27b85a37fb2b > > --- /dev/null > > +++ b/Documentation/driver-api/media/media-committer.rst > > @@ -0,0 +1,252 @@ > > +Media committers > > +================ > > + > > +What is a media committer? > > +-------------------------- > > + > > +A media committer is a developer who can apply patches from other developers > > Here too we could standardize to "push" instead of "apply". > > > +and their own patches at the > > s/at the/to the/ > > I would simply write "who can push commits to the ...". The main goal of a maintainer, which is partially delegated to committers, is to apply patches from other developers. We'd like to bold it here. > > > +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_ > > +tree. > > + > > +It is a media committer's duty to ensure that their entries at the MAINTAINERS > > s/at the/in the/ > > > +file will be kept updated, and that submitted patches for files for which > > s/will be kept update/are kept up-to-date/ > > > +they are listed as maintainers are timely reviewed at the mailing list, > > s/at the/on the/ > > > +not waiting in patchwork as ``New`` for more than one Kernel merge cycle, > > +and, if accepted, applying them at the media committer's tree. > > I think this goes a bit backward, and mixes things up a bit. On the > mixing side, the expectation of timely reviews comes from maintainer > status. Having commit rights is orthogonal to that. > The goal of direct commit access is to speed up maintenance, to get > patches reviewed and merged quicker. > Are we saying here that if someone > has commit rights they will lose them if they take too long to review > code ? That would then slow down maintenance even more, which seems > counterproductive. > This seems to be a difference between drm-misc criteria and our criteria: it is a requirement for committers to review other peoples patches. It is somewhat similar to drm-intel and drm-xe, where reviews are part of the acceptance criteria to become committers. On other words, committers are people where maintainers will delegate some parts. They need to work as "maintainer's proxy" for the delegated parties, which includes reviewing patches. So, the same way maintainers have duties to do timely reviews (as you mentioned), committers will have it for the parts of the subsystem that were delegated for them to merge code. This includes not only their work as developers, but also patches submitted by third parties. Clearly, nobody will get rights revoked if a couple of patches would take more than one kernel cycle because of technical issues (for instance too complex patches or patches that may have potential to cause regressions). But it is expected that committers will do timely reviews for the code he asked to be a committer. > Also, while one can be recognized as a maintainer for multiple drivers > or parts of the kernel, there's a single committer status. You can't > revoke committer status of a particular driver only. While not explicitly written, if a committer that maintains multiple drivers wants to step down from maintaining one of them, he could simply write an e-mail to media-committers requesting to step down from maintaining such driver. On such case, MAINTAINERS file should be updated accordingly, as usual. > > + > > +This privilege is granted with some expectation of responsibility: > > "Privilege" sounds a bit like lord and serf. This is the term used on SQL databases. Here, it is used in the context of having the same technical meaning as there. We used this term here in the lack of a better word. Do you have a better way to describe it? See, while correct, it is not nice to repeat grant twice like: This grant is granted with some expectation of ... ===== ======= We might use "duties", but it is not quite the same. > > +committers are people who care about the Linux Kernel as a whole and > > +about the Linux media subsystem and want to help its development. It > > +is also based on a trust relationship between the rest of the committers, > > +maintainers and the LinuxTV community. > > Who is "the LinuxTV community" ? See https://linuxtv.org/: The LinuxTV community (linux-media) develops and maintains the Linux Kernel Media Subsystems and several userspace libraries and applications. I'll add a link to linuxtv.org there. > > > + > > +As such, a media committer is not just someone who is capable of creating > > +code, but someone who has demonstrated their ability to collaborate > > +with the team, get the most knowledgeable people to review code, > > +contribute high-quality code, and follow through to fix issues (in code > > +or tests). > > + > > +.. Note:: > > + > > + 1. If a patch introduced a regression, then it is the media committer's > > s/introduced/introduces/ > > > + responsibility to correct that as soon as possible. Typically the > > + patch is either reverted, or an additional patch is committed that > > + fixes the regression; > > + 2. if patches are fixing bugs against already released Kernels, including > > + the reverts above mentioned, the media committer shall add the needed > > + tags. Please see :ref:`Media development workflow` for more details. > > s/ Please/ Please/ > > > + 3. all patches should be properly reviewed at > > + linux-media@vger.kernel.org before being merged at the > > s/at the/in the/ > > > + media-committers tree or submitted on pull requests. > > This is a fundamental rule that belongs to the development workflow in > my opinion. I'll add it there. Yet, I would prefer to keep it here too, as new committers need to be aware that patches need to be reviewed first at the ML. > > > + > > +Becoming a media committer > > +-------------------------- > > + > > +The most important aspect of volunteering to be a committer is that you will > > +be able to review and approve other people's changes, so we are looking for > > Everybody is able to review patches (rather, everybody is allowed to > review patches, the ability is a different matter). Ok. > > > +whether we think you will be good at doing that. > > I've been told that "whether" should also come with a "or" clause. You > can write "whether or not we think ...". > > > + > > +As such, potential committers must earn enough credibility and trust from the > > +LinuxTV community. To do that, developers shall be familiar with the open > > +source model and have been active at the Linux Kernel community for some time, > > s/at the/in the/ > > > +and, in particular, with the media subsystem. > > s/with the/in the/ > > > + > > +So, in addition to actually making the code changes, you are basically > > +demonstrating your: > > + > > +- commitment to the project; > > +- ability to collaborate with the team and communicate well; > > +- understand of how upstream and the LinuxTV community works > > s/ works/ work/ > > > + (policies, processes for testing, code review, ...) > > +- reasonable knowledge about: > > + > > + - the Kernel development process: > > + Documentation/process/index.rst > > + > > + - the Media development profile: > > + Documentation/driver-api/media/maintainer-entry-profile.rst > > + > > +- understanding of the projects' code base and coding style; > > +- ability to provide feedback to the patch authors; > > +- ability to judge when a patch might be ready for review and to submit; > > +- ability to write good code (last but certainly not least). > > + > > +It is also desirable that developers that intend to become committers > > +make a best effort to attend the yearly Linux Media Summit, typically > > +co-located with another Linux conference. > > I would say that "are encouraged to attend" instead of "make a best > effort to attend". Ok. > Also, how will this scale when we'll have a few > dozen committers ? Typically the media summit is capped to 20 attendees > or less. If we have a few dozen committers, I would try to find a bigger room for the Media Summit ;-) > > +If you are doing such tasks and have become a valued developer, an > > +existing committer can nominate you to the media subsystem maintainers. > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: > > "Maintainers and committers should encourage contributors to request > commit rights, especially junior contributors tend to underestimate > their skills." Too early for that. We don't want junior contributors causing merge issues and forcing maintainers to do rebases at media-committers.git. > > + > > +The ultimate responsibility for accepting a nominated committer is up to > > +the subsystem's maintainers. The committers must earn a trust relationship > > +with all subsystem maintainers, as, by granting you commit rights, they will > > +be delegating part of their maintenance tasks. > > + > > +Due to that, to become a committer or a core committer, a consensus between > > +all subsystem maintainers is required, as they all need to trust a developer > > +well enough to be delegated the responsibility to maintain part of the code > > +and to properly review patches from third parties, in a timely manner and > > +keeping the status of the reviewed code at https://patchwork.linuxtv.org > > +updated. > > + > > +.. Note:: > > + > > + In order to preserve the developers that could have their commit rights > > Do you mean "protect" instead of "preserve" ? Or maybe "preserve/protect > the privacy of" ? Good point. I'd say both. > s/that/who/ > s/their // (in the granted case, they don't have commit rights yet) > > > + granted or denied as well as the subsystem maintainers who have the > > "granted, denied or removed" ? Ok. > > + task to accept or deny commit rights, all communication related to > > + nominating a committer, preserving commit rights or leaving such function > > + should happen in private as much as possible. > > + > > +Media committer's agreement > > +--------------------------- > > + > > +Once a nominated committer is accepted by all subsystem maintainers, > > +they will ask if the developer is interested in the nomination and discuss > > +what area(s) of the media subsystem the committer will be responsible for. > > + > > +Once the developer accepts being a committer, the new committer shall > > +explicitly accept the Kernel development policies described under its > > +Documentation/, and, in particular to the rules on this document, by writing > > +an e-mail to media-committers@linuxtv.org, with a declaration of intent > > +following the model below:: > > + > > + I, John Doe, would like to change my status to: Committer > > + > > + I intend to actively develop the XYZ driver, send fixes to drivers > > + that I can test, reviewing patches and merging trivial fixes > > + for the subsystem, ... > > "Merging trivial fixes for the subsystem" bothers me. I don't think it > needs to be a requirement for committers. This is a maintainer's > responsibility. If people want to help with that that's great, but > making it a requirement isn't. Or did you mean this as an example ? This is an example. The scope will be case-by-case, and will be part of the discussions with the maintainers. Yet, if a committer requests rights for driver A, I expect him/her to pick trivial patches for it. The committers may eventually pick trivial patches for other drivers. We need to take some care with that to avoid multiple developers to review the same patches and wasting their time. > We shouldn't expect committers to handle a higher workload than what > they do as driver maintainers who submit patches by e-mail or send pull > requests. Giving commit rights will lower the effort to get patches in, > and I think it's fair to ask for keeping patchwork up-to-date in return, > but that's about it. > > > + > > +Followed by a formal declaration of agreement with the Kernel development > > +rules, signed with a PGP key cross signed by other Kernel and media > > s/PGP/GPG/ (same thing in practice, but let's advocate for free > software) The Kernel docs referred in this section use the term PGP. I don't mind replacing them to GPG, but this has to happen at the already existing docs as well. Feel free to submit such patch later on, once we get this merged upstream. > > > +developers. Such declaration shall be:: > > I find the GPG signature requirement to be borderline ridiculous. The > first message you're giving to committers is that you distrust them so > much that you want them to sign an agreement with their blood > (figuratively speaking). I don't think it's a very good approach to > community building, nor does it bring any advantage to anyone. Please don't spread FUD. The intent is to ensure that the email really came from the committer, and was not impersonated by someone's else. > > + > > + I hereby declare that I agree with the Kernel development rules described at: > > + > > + https://www.kernel.org/doc/html/latest/driver-api/media/media-committer.rst > > + > > + and to the Linux Kernel development process rules. > > + > > + I agree to the Code of Conduct as documented here: > > s/here:/in/ > > > + Documentation/process/code-of-conduct.rst > > You use an http link for meda-commiter.rst and a file path for > code-of-conduct.rst. RST links for both would be more consistent, but if > the goal is to paste the text in an e-mail, you can also use http links > or file paths for both. Good point. Both shall be pointing to https://www.kernel.org/doc/html/latest/, as this is the example of the content of the e-mail. Also, as this is inside a Sphinx code-block, ReST links won't work. > > + I am aware that I can, at any point of time, retire. In that case, I will > > + send an e-mail to notify the subsystem maintainers for them to revoke my > > + commit rights. > > + > > + I am aware that the Kernel development rules change over time. > > + By doing a new commit, I understand that I agree with the rules in effect > > What does "doing a new commit" mean here ? Sending a patch to the list ? > Pushing it to the shared tree ? I assume the latter given the text > below, it could be clarified here. It means a commit at media-committers.git. I'll clarify it. > > + at the time of the commit. > > + > > +For more details about PGP sign, please read > > +Documentation/process/maintainer-pgp-guide.rst and > > +:ref:`kernel_org_trust_repository`. > > + > > +In case the kernel development process changes, by merging new commits at the > > s/at the/in the/ > > > +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_, > > s/media-committers/media-committers tree/ > > > +the media committer implicitly declares that the agreement with the latest > > s/that the/their/ > > > +version of the documented process and to the contents of this file. > > s/to the contents // ("latest version" applies to both "the documented > process" and "this file") > > This is problematic, as we can't expect people to check for changes in > this file every time they push something. Changes to this file should be > announced to all committers, with a reasonable review period. I don't expect that this document will change too much. See for instance how many times we changed the media developers profile. Also, any changes for the media committers document shall be c/c to linux-media and hopefully c/c to media committers. But see: it is not just this file: it is the entire Kernel submission process, and CoC. We can't expect that people will notify us with a reasonable review period every time someone changes a bit at the documentation process, as such discussions typically happen at linux-docs ML. If such changes happen and a committer doesn't notice and gets upset enough to decide retiring, The committer can step down when this is noticed. Yet, we don't want any troubles for the already committed patches, nor we want to cause any harm for the other media committers and maintainers if this ever happens. > > > + > > +Core committers > > +--------------- > > + > > +As described in Documentation/driver-api/media/maintainer-entry-profile.rst > > RST link here too. > > > +a committer may be granted with additional privileges to also be able to > > s/privileges/rights/ > > (same below) > > > +change a core file and/or media subsystem's Kernel API/ABI. The extent of > > Drop "/ABI" as above. > > > +the core committer's additional privileges will be detailed by the subsystem > > +maintainers when they nominate a core committer. > > + > > +Existing committers may become core committers and vice versa. Such > > +decisions will be taken in consensus between the subsystem maintainers. > > + > > +Media committers rules > > +---------------------- > > + > > +Media committers shall ensure that merged patches will not break any existing > > +drivers. If it breaks, fixup or revert patches shall be merged as soon as > > How do they ensure that ? I would prefer mentioning a best effort here, > as it's practically not possible to ensure this (until we have CI > covering all the drivers in the subsystem, which is not a reasonable > target). Makes sense, I'll change it. The important message there relies at the second phrase: "If it breaks, fixup or revert patches shall be merged as soon as possible" > > +possible, aiming to be merged at the same Kernel cycle the bug is reported. > > + > > +Media committers shall behave accordingly to the permissions granted by > > s/permissions/rights/ (we never use "permission" anywhere else) > > > +the subsystem maintainers, specially with regards of the scope of changes > > +they may apply directly at the media-committers tree. Such scope can > > +change overtime on a mutual greement between media committers and > > s/overtime/over time/ > s/greement/agreement/ > > > +maintainers. > > + > > +As described at :ref:`Media development workflow`, there are workflows. > > s/at /in / > > > +For the committers' workflow, the following rules apply: > > + > > +- Each merged patch shall pass CI tests; > > + > > +- Media committers shall request reviews from other committers were > > We shouldn't limit this to "other committers". When making changes to a > driver that is not maintained by any committer (for instance when making > tree-wide changes), the person in the best position to review the > changes is the driver maintainer or author. I would extend this to "from > third parties", or "from other developers". True, but the idea here is to enforce the need of collaboration with other maintainers. So, perhaps: "from other committers and developers" > > > + applicable, i.e. because those committers have more knowledge about > > + some areas that are changed by a patch; > > + > > +- No other media committer would be against the proposed changes. > > https://drm.pages.freedesktop.org/maintainer-tools/committer/committer-drm-misc.html#merge-criteria: > > "There must not be open issues or unresolved or conflicting feedback > from anyone. Clear them up first. Defer to maintainers as needed." > > That's better than restricting the conflicts to committers. If there are > open issues, the patches should not be merged through the shared tree. Works for me. > > + > > +Patches that do not fall under the committer's workflow criteria will follow > > +the normal workflow as described at :ref:`Media development workflow`. > > + > > +Only a subsystem maintainer can override such rules. > > + > > +All media committers shall ensure that patchwork will reflect the current > > +status, e.g. patches shall be delegated to the media committer who is > > +handling them and the patch status shall be updated according to these rules: > > + > > +- ``Under review``: Used if the patch requires a second opinion > > + or when it is part of a pull request; > > +- ``Accepted``: Once a patch is merged at the multi-committer tree. > > s/at the/in the/ > > > +- ``Superseded``: There is a newer version of the patch posted in the > > s/in the/to the/ > > > + mailing list. > > +- ``Duplicated``: There was another patch doing the same thing from someone > > + else that was accepted. > > +- ``Not Applicable``: Use for patch series that are not merged at media.git > > + tree (e.g. drm, dmabuf, upstream merge, etc.) but were cross-posted to the > > + linux-media mailing list. > > + > > +If the committer decides not to merge it, then reply by email to patch > > +authors, explaining why it is not merged, and patchwork shall be updated > > +accordingly with either: > > + > > +- ``Changes Requested``: if a new revision was requested; > > +- ``Rejected``: if the proposed change won't be merged upstream. > > There are tools to ease updating the status of a patch, could you > document or at least mention them ? I'll add a link to: https://patchwork.readthedocs.io/en/latest/usage/clients/ From my side, I only tested pwclient (and the REST API interface which I use on some scripts, like the one at Jenkins to test PRs). > > + > > +If a media committer decides to retire, it is the committer's duty to > > +notify the subsystem maintainers about that decision. > > + > > +Maintaining media committer status > > +---------------------------------- > > + > > +A community of committers working together to move the Linux Kernel > > +forward is essential to creating successful projects that are rewarding > > +to work on. If there are problems or disagreements within the community, > > +they can usually be solved through healthy discussion and debate. > > + > > +In the unhappy event that a media committer continues to disregard good > > +citizenship (or actively disrupts the project), we may need to revoke > > That's very, very vague, surprisingly vague even from someone who raised > many concerns about the kernel code of conduct being vague. This text came from Google Chromium committer document: https://chromium.googlesource.com/playground/chromium-org-site/+/refs/heads/main/getting-involved/become-a-committer.md#maintaining-committer-status It is vague, a revoking commit rights is a discretionary act. It is nearly impossible to describe all conditions that could lead a committer to be distrusted. > > +that person's status. In such cases, if someone suggests the revocation with > > +a good reason, other developers may second the motion. The final decision > > +is taken by the subsystem maintainers. As the decision to become a media > > What does "seconding the motion" bring, if the decision lies solely in > maintainers ? The decision is up to maintainers, but other committers/developers may voice about it. > > +committer comes from a consensus between subsystem maintainers, a single > > +subsystem maintainer not trusting the media committer anymore is enough to > > +revoke committer's privileges. > > + > > +If a committer is inactive for more than a couple of Kernel cycles, > > +maintainers will try to reach you via e-mail. If not possible, they may > > +revoke your committer privileges and update MAINTAINERS file entries > > +accordingly. If you wish to resume contributing later on, then contact > > +the subsystem maintainers to ask if your rights can be restored. > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: > > "Committers are encouraged to request their commit rights get removed > when they no longer contribute to the project. Commit rights may be > automatically revoked after a year of inactivity (no commits or > reviews). I prefer the current version, as it makes clearer that we expect committers to request removal before becoming inactive. > Commit rights will be reinstated when they come back to the > project." This doesn't sound right, as it means that just returning back is an enough condition for having committer grants restored. This should not be automatic: the returned contributors need to opt-in to be committers again, and the maintainers need to be confident that the developers will do their duties before restore committers' grants. > > + > > +A previous committer that had his commit rights revoked can keep contributing > > s/his/their/ > > > +to the subsystem via the normal e-mail workflow as documented at the > > +:ref:`Media development workflow`. > > + > > +References > > +---------- > > + > > +Much of this was inspired by/copied from the committer policies of: > > + > > +- `Chromium <https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md>`_; > > +- `WebKit <http://www.google.com/url?q=http%3A%2F%2Fwebkit.org%2Fcoding%2Fcommit-review-policy.html&sa=D&sntz=1&usg=AFrqEze4W4Lvbhue4Bywqgbv-N5J66kQgA>`_; > > Google tracks us enough without using google URLs. I'll fix. > > > +- `Mozilla <http://www.google.com/url?q=http%3A%2F%2Fwww.mozilla.org%2Fhacking%2Fcommitter%2F&sa=D&sntz=1&usg=AFrqEzecK7iiXqV30jKibNmmMtzHwtYRTg>`_. > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html > would also have been a good source of inspiration. That's the only large > multi-committer workflow today in the kernel, and it has proven its > value. The explicit acceptance criteria in particular are very good. > Quoting the document, it says > > "Commit rights will be granted to anyone who requests them and fulfills > the below criteria:" > > That's how we build an inclusive community, it feels way more welcoming > than saying that maintainers will discuss in private and grant > privileges to underlings if it pleases them (even if the effect is the > same in practice, it's still a maintainer decision). The criteria for drm-misc has a big gap from what we want: "This should not be interpreted as a requirement to review other peoples patches" We do require committers to review other peoples patches. It sounds that the criteria there for xe/i915 partially fixes the issue with drm-misc, as it adds this criteria: "Has reviewed at least 25 patches from other developers to the chosen driver that have already been merged upstream. Again, most of the reviewed patches must be non-trivial." But still it doesn't say that committers shall review other peoples patches(*). (*) Such reviews happen in practice there, but the number of patches from external parties for xe/915 are very small and they usually come from business partners. This is a very different reality than what we have on media. Besides that, in practical terms, we're not ready to accept "anyone". Our current goal is to grant commit rights to the most active developers that have been doing a good work reviewing patches from others. On other words, the focus are on those who has already grants at patchwork to update patches. It doesn't mean that we're limited to them. It is just that we need to implement multi-committers without causing troubles to the subsystem's workflow. Also, we had enough troubles with our past multi-committers model, back in the days we were using Mercurial, because there, we used to grant committer access to "anyone" that were active at the media development. So, it has to be a step-by-step approach. Maybe after a couple of years this will be revisited and we could use a more inclusive text, but first we need to ensure that media-ci, patchwork and the multi-committer's model will work for us on a multi-committers model. Then, we'll likely need a tool similar to `dim` to help with the merge requests and avoid problems that may force maintainers to do rebases. > > > + > > diff --git a/Documentation/process/maintainer-pgp-guide.rst b/Documentation/process/maintainer-pgp-guide.rst > > index f5277993b195..795ef8d89271 100644 > > --- a/Documentation/process/maintainer-pgp-guide.rst > > +++ b/Documentation/process/maintainer-pgp-guide.rst > > @@ -903,6 +903,8 @@ the new default in GnuPG v2). To set it, add (or modify) the > > > > trust-model tofu+pgp > > > > +.. _kernel_org_trust_repository: > > + > > Using the kernel.org web of trust repository > > -------------------------------------------- > > > Thanks, Mauro
On Wed, Nov 27, 2024 at 10:39:48AM +0100, Mauro Carvalho Chehab wrote: > Em Tue, 26 Nov 2024 17:19:30 +0200 Laurent Pinchart escreveu: > > On Mon, Nov 25, 2024 at 02:28:58PM +0100, Mauro Carvalho Chehab wrote: > > > As the media subsystem will experiment with a multi-committers model, > > > update the Maintainer's entry profile to the new rules, and add a file > > > documenting the process to become a committer and to maintain such > > > rights. > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > > Signed-off-by: Hans Verkuil <hverkuil@xs4ll.nl> > > > --- > > > Documentation/driver-api/media/index.rst | 1 + > > > .../media/maintainer-entry-profile.rst | 193 ++++++++++---- > > > .../driver-api/media/media-committer.rst | 252 ++++++++++++++++++ > > > .../process/maintainer-pgp-guide.rst | 2 + > > > 4 files changed, 398 insertions(+), 50 deletions(-) > > > create mode 100644 Documentation/driver-api/media/media-committer.rst > > > > > > diff --git a/Documentation/driver-api/media/index.rst b/Documentation/driver-api/media/index.rst > > > index d5593182a3f9..d0c725fcbc67 100644 > > > --- a/Documentation/driver-api/media/index.rst > > > +++ b/Documentation/driver-api/media/index.rst > > > @@ -26,6 +26,7 @@ Documentation/userspace-api/media/index.rst > > > :numbered: > > > > > > maintainer-entry-profile > > > + media-committer > > > > > > v4l2-core > > > dtv-core > > > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst > > > index ffc712a5f632..90c6c0d9cf17 100644 > > > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst > > > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst > > > @@ -27,19 +27,128 @@ It covers, mainly, the contents of those directories: > > > Both media userspace and Kernel APIs are documented and the documentation > > > must be kept in sync with the API changes. It means that all patches that > > > add new features to the subsystem must also bring changes to the > > > -corresponding API files. > > > +corresponding API documentation files. > > > > I would have split this kind of small changes to a separate patch to > > make reviews easier, but that's not a big deal. > > > > > > > > -Due to the size and wide scope of the media subsystem, media's > > > -maintainership model is to have sub-maintainers that have a broad > > > -knowledge of a specific aspect of the subsystem. It is the sub-maintainers' > > > -task to review the patches, providing feedback to users if the patches are > > > +Due to the size and wide scope of the media subsystem, the media's > > > +maintainership model is to have committers that have a broad knowledge of > > > +a specific aspect of the subsystem. It is the committers' task to > > > +review the patches, providing feedback to users if the patches are > > > following the subsystem rules and are properly using the media kernel and > > > userspace APIs. > > > > This sounds really like a maintainer definition. I won't bikeshed too > > much on the wording though, we will always be able to adjust it later to > > reflect the reality of the situation as it evolves. I do like the > > removal of the "sub-maintainer" term though, as I've always found it > > demeaning. > > The main goal here was to replace sub-maintainers by committers. > > Other changes can go later on, or if you have a better way to define > the paper of committers, be welcomed to propose it. I think we would waste time arguing about this section. It's not the main goal of this patch, so I'm fine handling it later. > > > -Patches for the media subsystem must be sent to the media mailing list > > > -at linux-media@vger.kernel.org as plain text only e-mail. Emails with > > > -HTML will be automatically rejected by the mail server. It could be wise > > > -to also copy the sub-maintainer(s). > > > +Media committers > > > +---------------- > > > + > > > +In the media subsystem, there are experienced developers that can commit > > > > s/that/who/ > > s/commit/push/ to standardize the vocabulary (below you use "upload" to > > mean the same thing) > > > > > +patches directly on a development tree. These developers are called > > > > s/on a/to the/ > > I won't comment here any below about trivial changes like that: I'll > just incorporate at v2. I'll focus my reply on your the comments about > the text contents itself. Fine with me. I actually considered sending the trivial comments as a separate reply. > > > +Media committers and are divided into the following categories: > > > + > > > +- Committers: responsible for one or more drivers within the media subsystem. > > > + They can upload changes to the tree that do not affect the core or ABI. > > > > s/upload/push/ > > > > > + > > > +- Core committers: responsible for part of the media core. They are typically > > > + responsible for one or more drivers within the media subsystem, but, besides > > > + that, they can also merge patches that change the code common to multiple > > > + drivers, including the kernel internal API/ABI. > > > > I would write "API" only here. Neither the kernel internal API nor its > > internal ABI are stable, and given that lack of stability, the ABI > > concept doesn't really apply within the kernel. > > It does for distros, but this is a separate matter ;-) > > From my side, I'm ok with either API or API/ABI, if ok for Hans and others > as well. > > > > + > > > +- Subsystem maintainers: responsible for the subsystem as a whole, with > > > + access to the entire subsystem. > > > + > > > + Only subsystem maintainers can change the userspace API/ABI. > > > > This can give the impression that only subsystem maintainers are allowed > > to work on the API. I would write > > > > Only subsystem maintainers change push changes that affect the userspace > > API/ABI. > > I guess you meant to say: > > Only subsystem maintainers *can* push changes that affect the userspace > API/ABI. > > (change -> can) Yes, sorry, that's what I meant. > The new text seems OK to me. I'll change it at v2. > > > > + > > > +Media committers shall explicitly agree with the Kernel development process > > > > Do we have to capitalize "Kernel" everywhere ? There are way more > > occurrences of "kernel" than "Kernel" in Documentation/ (even excluding > > the lower case occurrences in e-mail addresses, file paths, ...). > > "The Kernel" means "the Linux Kernel". e. g. it is not just a random kernel, > it is the Linux Kernel. So, it is a proper noun. Proper nouns should always > be capitalized in English. I don't think anyone would consider "kernel" to refer to the BSD kernel in this context. I still think we shouldn't capitalize the term here, but usage of "Kernel" in this file predates this patch, so we can address this separately. > > > +as described at Documentation/process/index.rst and to the Kernel > > > +development rules inside the Kernel documentation, including its code of > > > +conduct. > > > > That's expected of all kernel developers, but I don't object reiterating > > it here. > > > > > + > > > +More details about media committers can be found here: > > > > Maybe "about media commiters' roles and responsibilities" would be more > > accurate, the document doesn't include information about the committers > > themselves. > > > > s/here:/at/ > > > > > +Documentation/driver-api/media/media-committer.rst. > > > > Should this be a RST link ? > > No need: a Sphinx plugin automatically converts it to hyperlink. Ah, good to know. > > > + > > > +Media development tree > > > +---------------------- > > > + > > > +The main development tree used by the media subsystem is hosted at LinuxTV.org, > > > +where we also maintain news about the subsystem, wiki pages and a patchwork > > > +instance where we track patches though their lifetime. > > > + > > > +The main tree used by media developers is at: > > > + > > > +https://git.linuxtv.org/media.git/ > > > > I think the multi-committers tree should be upgraded to main development > > tree status, but that's a separate issue that I'm fine discussing > > separately, and also delaying for a few kernel releases until we iron > > out the rough edges of the process. > > This is a separate discussion, and it is too early to start discussing it. Ack. > > > +.. _Media development workflow: > > > + > > > +Media development workflow > > > +++++++++++++++++++++++++++ > > > + > > > +All changes for the media subsystem must be sent first as e-mails to the > > > +media mailing list, as plain text only e-mail to: > > > + > > > + `https://subspace.kernel.org/vger.kernel.org.html <linux-media@vger.kernel.org>`_ > > > + > > > +Emails with HTML will be automatically rejected by the mail server. > > > +It could be wise to also copy the media committer(s). You should use > > > +``scripts/get_maintainers.pl`` to identify whom else needs to be copied. > > > +Please always copy driver's authors and maintainers. > > > > Instead of duplicating a tiny part of the patch submission process, I > > would link to the appropriate documentation. > > The intention here is to bold that driver's authors and maintainers should > always be c/c and the way to do it is via get_maintainers.pl. Yet, a link > to the process submission makes sense. I'll move the link to here. > > > [1] (see below) > > > > > + > > > +Such patches needed to be based against a public branch or tag as follows: > > > > s/needed/need/ > > > > > + > > > +1. new Kernel releases: > > > > s/new/New/ > > > > What does that mean though ? It's not very clear. > > Maybe "Upcoming Kernel releases"? I think the distinction between the three categories is whether the patches are features (i.e. not fixes), fixes for an issue present in an already released kernel, or fixes for an issue not present in any released kernel (i.e. present in -next only). Phrasing it that way would make it easier to understand, compared to the current wording that focusses on when the patch is sent. > > > + > > > + Those need to be based at the ``next`` branch of that media.git tree > > > + > > > +2. During Kernel release development cycle, patches fixing bugs on a -rc > > > + kernel should preferably be against the latest -rc1 Kernel. If they > > > + require a previously-applied change, they need to be against the ``fixes`` > > > + branch; > > > + > > > +3. Patches against an already released kernel should preferably be against > > > + the latest released Kernel. If they require a previously-applied > > > + change, they need to be against ``fixes``. > > > > If I were a new contributor I think I would have trouble understand this > > to be honest. I won't push hard for a rework of this section, as I > > expect it will change after the multi-committer tree becomes the main > > way to get patches merged. We can then update the documentation. > > Basically, the preference is to have patches against a release or against > -rc1 when it makes sense, as it helps when backports are needed. > I don't think this will ever change. > > Patches against media.git next branch are ok, specially for new > features. > > > > + > > > +All patches with fixes shall have: > > > + - a ``Fixes:`` tag pointing to the first commit that introduced the bug; > > > + - a ``Cc: stable@vger.kernel.org`` > > > + > > > +Patches that were fixing bugs reported by someone else shall have: > > > + - a ``Reported-by`` tag immediately followed by a ``Closes`` tag. > > > > There's been a recent discussion about not including a Reported-by tag > > without asking permission from the reporter, due to privacy reasons (in > > particular for bugs reported to https://bugzilla.kernel.org/, as by > > default the e-mail address of the reporter is not public). As the > > Reported-by and Closes tags are not specific to the media tree, I would > > drop this paragraph, otherwise we will have to duplicate a relatively > > large amount of information related to privacy. You can link to the > > relevant documentation instead, but I wouldn't even do that as it's > > really not media-specific. > > Good point, but: > > 1. I can't remember last time I saw a media patch pointing to a Kernel BZ > in the recent days (surely there are, but they're not frequent); > 2. We do have a common pattern on media about people adding Reported-by: > without Closes:, or with closes at the wrong place. > > So, we're trying to address those problematic cases. Maybe we could change > the above to: > > Patches with fixes shall have: > - a ``Fixes:`` tag pointing to the first commit that introduced the bug; > - when applicable, a ``Cc: stable@vger.kernel.org``. > > Patches that were fixing bugs publicly reported by someone > at the media mailing list shall have: > - a ``Reported-by`` tag immediately followed by a ``Closes`` tag. Given that the Reported-by + Closes pattern isn't specific to the media subsystem, that checkpatch.pl should warn about that (unless I'm mistaken) and cause CI to fail, and that we would need to expand the documentation to cover the privacy issue, my preference would be to drop the paragraph related to Reported-by + Closes. I don't think keeping it here would bring much value. The mention of Fixes and Cc: stable is good. > > > + > > > +Patches that change API/ABI shall require patches to update documentation > > > > s/require patches to // > > > > > +accordingly at the same patch series. > > > > s/at the/in the/ > > > > > + > > > +See Documentation/process/index.rst for more details about e-mail submission. > > > > This could be moved up to [1]. > > > > > + > > > +Once a patch is submitted, it may follow either one of the workflows > > > +below: > > > > "of the following workflows" > > > > > + > > > +a. Normal workflow: patches are handled by subsystem maintainers:: > > > + > > > + +------+ +---------+ +-------+ +-------------------+ +---------+ > > > + |e-mail|-->|patchwork|-->|pull |-->|maintainers merge |-->|media.git| > > > + +------+ +---------+ |request| |at media-committers| +---------+ > > > + +-------+ +-------------------+ > > > > This makes it look like the pull request originates from patchwork. > > No, it points that patches from e-mail go to patchwork, and the PRs come > from what it is in patchwork(*). > > (*) Now, it doesn't matter if one picks it manually from the mailing list > or picked with b4 or pwclient, the point is that the patch shall be > in patchwork before someone does a pull request, as the patch state > at patchwork needs to reflect the patch review lifetime. Yes, I understand the workflow, my point is that the diagram could be misunderstood by people who are not already familiar with the process. > > s/at /in / > > > > I would also add .git to media-committers to make it clear it's a git > > tree. > > Ok. > > > (I still think the tree name is too long and not very appropriate, but > > that's a different question) > > > > > + > > > + For this workflow, pull requests can be generated by a committer, > > > + a previous committer, subsystem maintainers or by a couple of trusted > > > + long-time contributors. If you are not in such group, please don't submit > > > + pull requests, as they will likely be ignored. > > > + > > > +b. Committers' workflow: patches are handled by media committers:: > > > + > > > + +------+ +---------+ +-------------------+ +-----------+ +---------+ > > > + |e-mail|-->|patchwork|-->|committers merge |-->|maintainers|-->|media.git| > > > + +------+ +---------+ |at media-committers| |approval | +---------+ > > > + +-------------------+ +-----------+ > > > + > > > +When patches are merged at patchwork and when merged at media-committers, > > > > We don't "merge" patches in patchwork. You could write "When patches are > > picked by patchwork" for instance. > > Works for me. I'll change it in v2. > > > > +CI bots will check for errors and may provide e-mail feedback about > > > +patch problems. When this happens, the e-mail author must fix them > > > > s/e-mail author/patch author/ ? Or possibly better, "patch submitter" as > > that person may not be the author. > > Ok. > > > > +and send another version of the patch. > > > + > > > +Patches will only be moved to the next stage in those two workflows if they > > > +don't fail on CI or if there are false-positives at the CI reports. > > > > s/at the/in the/ > > > > > + > > > > This workflow doesn't apply to patch submitters who are not allowed to > > send pull requests and who don't have direct commit access. I thought > > these submitters are the main audience of this document. In that case, I > > think moving the next section that explains the e-mail workflow before > > the "Media development workflow" section (which should likely be renamed > > to make it clear that it is about merging patches, not developing them) > > would be best. The "Review Cadence" section could also be folded in > > there, to give a full view of what a submitter can expect. > > > > This would also have the advantage of introducing the linuvtv.org > > patchwork instance, which you reference above. Documents are more > > readable when they introduce concepts first before using them. > > Will try to do such change at v2. > > > > > > +Failures during e-mail submission > > > ++++++++++++++++++++++++++++++++++ > > > > > > Media's workflow is heavily based on Patchwork, meaning that, once a patch > > > is submitted, the e-mail will first be accepted by the mailing list > > > @@ -47,51 +156,36 @@ server, and, after a while, it should appear at: > > > > > > - https://patchwork.linuxtv.org/project/linux-media/list/ > > > > > > -If it doesn't automatically appear there after a few minutes, then > > > +If it doesn't automatically appear there after some time [2]_, then > > > probably something went wrong on your submission. Please check if the > > > -email is in plain text\ [2]_ only and if your emailer is not mangling > > > +email is in plain text\ [3]_ only and if your emailer is not mangling > > > whitespaces before complaining or submitting them again. > > > > > > -You can check if the mailing list server accepted your patch, by looking at: > > > +To troubleshoot problems, you should first check if the mailing list > > > +server has accepted your patch, by looking at: > > > > > > - https://lore.kernel.org/linux-media/ > > > > > > -.. [2] If your email contains HTML, the mailing list server will simply > > > +If the patch is there and not at patchwork, it is likely that your e-mailer > > > +mangled the patch. Patchwork internally has a logic that checks if the > > > +received e-mail contain a valid patch. Any whitespace and new line > > > +breakages mangling the patch won't be recognized by patchwork, thus such > > > +patch will be rejected. > > > + > > > +.. [2] It usually takes a few minutes for the patch to arrive, but > > > + the e-mail server is busy, so it may take up to a few hours > > > > s/is busy/may be busy/ > > > > > + for a patch to be handled by the mail server and by the patchwork > > > + instance. > > > > "for a patch to be recorded by patchwork." > > > > > + > > > +.. [3] If your email contains HTML, the mailing list server will simply > > > drop it, without any further notice. > > > > These changes too could have been split to a separate cleanup patch, to > > make the crux of this patch easier to review. > > > > > > > > +Subsystem maintainers > > > +--------------------- > > > > > > -Media maintainers > > > -+++++++++++++++++ > > > - > > > -At the media subsystem, we have a group of senior developers that > > > -are responsible for doing the code reviews at the drivers (also known as > > > -sub-maintainers), and another senior developer responsible for the > > > -subsystem as a whole. For core changes, whenever possible, multiple > > > -media maintainers do the review. > > > - > > > -The media maintainers that work on specific areas of the subsystem are: > > > - > > > -- Remote Controllers (infrared): > > > - Sean Young <sean@mess.org> > > > - > > > -- HDMI CEC: > > > - Hans Verkuil <hverkuil@xs4all.nl> > > > - > > > -- Media controller drivers: > > > - Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > - > > > -- ISP, v4l2-async, v4l2-fwnode, v4l2-flash-led-class and Sensor drivers: > > > - Sakari Ailus <sakari.ailus@linux.intel.com> > > > - > > > -- V4L2 drivers and core V4L2 frameworks: > > > - Hans Verkuil <hverkuil@xs4all.nl> > > > > We're losing that information, isn't it valuable ? > > With the media-committers model, there's no concept of "media > maintainers" anymore. Also, the above will dynamically change > over time. Instead of placing things here, it makes more sense to > ensure that the MAINTAINERS entry gets updated accordingly as we > add media-committers, as this is what get-maintainers.pl uses. OK, I suppose MAINTAINERS is indeed better. > Besides that, the auto-delegation rules at patchwork will help > keeping the patches c/c to the right person, in the eventual > case someone doesn't run get-maintainers.pl. > > > > - > > > -The subsystem maintainer is: > > > - Mauro Carvalho Chehab <mchehab@kernel.org> > > > - > > > -Media maintainers may delegate a patch to other media maintainers as needed. > > > -On such case, checkpatch's ``delegate`` field indicates who's currently > > > -responsible for reviewing a patch. > > > +The subsystem maintainers are: > > > + Mauro Carvalho Chehab <mchehab@kernel.org> and > > > + Hans Verkuil <hverkuil@xs4all.nl> > > > > If you don't intend on this being rendered as a list, it should be > > > > The subsystem maintainers are Mauro Carvalho Chehab <mchehab@kernel.org> and > > Hans Verkuil <hverkuil@xs4all.nl>. > > > > Otherwise, > > > > The subsystem maintainers are: > > > > - Mauro Carvalho Chehab <mchehab@kernel.org> > > - Hans Verkuil <hverkuil@xs4all.nl> > > > > > > Shouldn't MAINTAINERS be updated at the same time ? > > It should be a separate patch. I would be doing such change once > Hans will be about to start sending pull requests, which still > require more actions. In can be a separate patch, but I think having it in the same series would make sense, as you're listing Hans here as a subsystem maintainer. It doesn't mean he will send pull requests behind your back immediately :-) > > > > > > Submit Checklist Addendum > > > ------------------------- > > > @@ -108,17 +202,14 @@ implementing the media APIs: > > > ==================== ======================================================= > > > Type Tool > > > ==================== ======================================================= > > > -V4L2 drivers\ [3]_ ``v4l2-compliance`` > > > +V4L2 drivers\ [4]_ ``v4l2-compliance`` > > > V4L2 virtual drivers ``contrib/test/test-media`` > > > CEC drivers ``cec-compliance`` > > > ==================== ======================================================= > > > > > > -.. [3] The ``v4l2-compliance`` also covers the media controller usage inside > > > +.. [4] The ``v4l2-compliance`` also covers the media controller usage inside > > > V4L2 drivers. > > > > > > -Other compilance tools are under development to check other parts of the > > > -subsystem. > > > - > > > Those tests need to pass before the patches go upstream. > > > > > > Also, please notice that we build the Kernel with:: > > > @@ -134,6 +225,8 @@ Where the check script is:: > > > Be sure to not introduce new warnings on your patches without a > > > very good reason. > > > > > > +Please see `Media development workflow`_ for e-mail submission rules. > > > + > > > > These hunks too could have been split to a cleanup patch before the main > > changes. > > > > > Style Cleanup Patches > > > +++++++++++++++++++++ > > > > > > diff --git a/Documentation/driver-api/media/media-committer.rst b/Documentation/driver-api/media/media-committer.rst > > > new file mode 100644 > > > index 000000000000..27b85a37fb2b > > > --- /dev/null > > > +++ b/Documentation/driver-api/media/media-committer.rst > > > @@ -0,0 +1,252 @@ > > > +Media committers > > > +================ > > > + > > > +What is a media committer? > > > +-------------------------- > > > + > > > +A media committer is a developer who can apply patches from other developers > > > > Here too we could standardize to "push" instead of "apply". > > > > > +and their own patches at the > > > > s/at the/to the/ > > > > I would simply write "who can push commits to the ...". > > The main goal of a maintainer, which is partially delegated to > committers, is to apply patches from other developers. Hence my comment above, where I said the description looked more like maintainer than committer. Bluring the lines is dangerous. Are we talking about committers or maintainers here ? From a committer I would only expect that they would get the rights to push commits for areas they control directly, lowering Hans' and your work load. This has a clear and immediate value to me, and I believe it's a good first step. If we're talking about delegating more maintainer duties, then it's a different conversation, with a stronger commitment that should go hand in hand with more trust and more rights. > We'd like to bold it here. > > > > > > +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_ > > > +tree. > > > + > > > +It is a media committer's duty to ensure that their entries at the MAINTAINERS > > > > s/at the/in the/ > > > > > +file will be kept updated, and that submitted patches for files for which > > > > s/will be kept update/are kept up-to-date/ > > > > > +they are listed as maintainers are timely reviewed at the mailing list, > > > > s/at the/on the/ > > > > > +not waiting in patchwork as ``New`` for more than one Kernel merge cycle, > > > +and, if accepted, applying them at the media committer's tree. > > > > I think this goes a bit backward, and mixes things up a bit. On the > > mixing side, the expectation of timely reviews comes from maintainer > > status. Having commit rights is orthogonal to that. > > The goal of direct commit access is to speed up maintenance, to get > > patches reviewed and merged quicker. > > Are we saying here that if someone > > has commit rights they will lose them if they take too long to review > > code ? That would then slow down maintenance even more, which seems > > counterproductive. > > > > This seems to be a difference between drm-misc criteria and our > criteria: it is a requirement for committers to review other peoples patches. Then we're not talking about committers, but about maintainers, and that's something that should be explicit, including in the name. To be clear, I don't want commit rights if that means my workload will significantly increase as a result, I can't commit at this point to be a subsystem maintainer. I think it would be a shame if there was no committer role. > It is somewhat similar to drm-intel and drm-xe, where reviews are part > of the acceptance criteria to become committers. Those are corporate trees, so it's easier to set such rules. > > On other words, committers are people where maintainers will delegate > some parts. They need to work as "maintainer's proxy" for the delegated > parties, which includes reviewing patches. That sounds like slave labour, being forced to do work without any of the recognition that comes from being a maintainer. > So, the same way maintainers have duties to do timely reviews > (as you mentioned), committers will have it for the parts of the > subsystem that were delegated for them to merge code. This includes > not only their work as developers, but also patches submitted by third > parties. > > Clearly, nobody will get rights revoked if a couple of patches would > take more than one kernel cycle because of technical issues > (for instance too complex patches or patches that may have potential > to cause regressions). But it is expected that committers will do timely > reviews for the code he asked to be a committer. > > > Also, while one can be recognized as a maintainer for multiple drivers > > or parts of the kernel, there's a single committer status. You can't > > revoke committer status of a particular driver only. > > While not explicitly written, if a committer that maintains multiple > drivers wants to step down from maintaining one of them, he could > simply write an e-mail to media-committers requesting to step down > from maintaining such driver. > > On such case, MAINTAINERS file should be updated accordingly, > as usual. > > > > + > > > +This privilege is granted with some expectation of responsibility: > > > > "Privilege" sounds a bit like lord and serf. > > This is the term used on SQL databases. Here, it is used in the context > of having the same technical meaning as there. We used this term here > in the lack of a better word. Do you have a better way to describe it? I think "rights" would sound better. > > See, while correct, it is not nice to repeat grant twice like: > > This grant is granted with some expectation of ... > ===== ======= > > We might use "duties", but it is not quite the same. > > > > +committers are people who care about the Linux Kernel as a whole and > > > +about the Linux media subsystem and want to help its development. It > > > +is also based on a trust relationship between the rest of the committers, > > > +maintainers and the LinuxTV community. > > > > Who is "the LinuxTV community" ? > > See https://linuxtv.org/: > > The LinuxTV community (linux-media) develops and maintains the Linux Kernel Media Subsystems and several userspace libraries and applications. Yes, but who is that ? You list maintainers, committers and the LinuxTV community. Is there a "community" here that wouldn't be part of "maintainers" and "committers" ? > I'll add a link to linuxtv.org there. > > > > + > > > +As such, a media committer is not just someone who is capable of creating > > > +code, but someone who has demonstrated their ability to collaborate > > > +with the team, get the most knowledgeable people to review code, > > > +contribute high-quality code, and follow through to fix issues (in code > > > +or tests). > > > + > > > +.. Note:: > > > + > > > + 1. If a patch introduced a regression, then it is the media committer's > > > > s/introduced/introduces/ > > > > > + responsibility to correct that as soon as possible. Typically the > > > + patch is either reverted, or an additional patch is committed that > > > + fixes the regression; > > > + 2. if patches are fixing bugs against already released Kernels, including > > > + the reverts above mentioned, the media committer shall add the needed > > > + tags. Please see :ref:`Media development workflow` for more details. > > > > s/ Please/ Please/ > > > > > + 3. all patches should be properly reviewed at > > > + linux-media@vger.kernel.org before being merged at the > > > > s/at the/in the/ > > > > > + media-committers tree or submitted on pull requests. > > > > This is a fundamental rule that belongs to the development workflow in > > my opinion. > > I'll add it there. Yet, I would prefer to keep it here too, as new > committers need to be aware that patches need to be reviewed first > at the ML. I think committers can be expected to read the subsystem's maintainer profile, it's even listed explicitly in this document. Duplicating information means they easily get out of sync. > > > + > > > +Becoming a media committer > > > +-------------------------- > > > + > > > +The most important aspect of volunteering to be a committer is that you will > > > +be able to review and approve other people's changes, so we are looking for > > > > Everybody is able to review patches (rather, everybody is allowed to > > review patches, the ability is a different matter). > > Ok. > > > > > > +whether we think you will be good at doing that. > > > > I've been told that "whether" should also come with a "or" clause. You > > can write "whether or not we think ...". > > > > > + > > > +As such, potential committers must earn enough credibility and trust from the > > > +LinuxTV community. To do that, developers shall be familiar with the open > > > +source model and have been active at the Linux Kernel community for some time, > > > > s/at the/in the/ > > > > > +and, in particular, with the media subsystem. > > > > s/with the/in the/ > > > > > + > > > +So, in addition to actually making the code changes, you are basically > > > +demonstrating your: > > > + > > > +- commitment to the project; > > > +- ability to collaborate with the team and communicate well; > > > +- understand of how upstream and the LinuxTV community works > > > > s/ works/ work/ > > > > > + (policies, processes for testing, code review, ...) > > > +- reasonable knowledge about: > > > + > > > + - the Kernel development process: > > > + Documentation/process/index.rst > > > + > > > + - the Media development profile: > > > + Documentation/driver-api/media/maintainer-entry-profile.rst > > > + > > > +- understanding of the projects' code base and coding style; > > > +- ability to provide feedback to the patch authors; > > > +- ability to judge when a patch might be ready for review and to submit; > > > +- ability to write good code (last but certainly not least). > > > + > > > +It is also desirable that developers that intend to become committers > > > +make a best effort to attend the yearly Linux Media Summit, typically > > > +co-located with another Linux conference. > > > > I would say that "are encouraged to attend" instead of "make a best > > effort to attend". > > Ok. > > > Also, how will this scale when we'll have a few > > dozen committers ? Typically the media summit is capped to 20 attendees > > or less. > > If we have a few dozen committers, I would try to find a bigger > room for the Media Summit ;-) It may be due to a difference of opinion on what "committer" means vs. "maintainer". I wouldn't expect "committers" in the drm-misc sense to attend the linux media summits (they can of course do so if they wish), as I would expect many of such "committers" to only handle a small part of the code base. > > > +If you are doing such tasks and have become a valued developer, an > > > +existing committer can nominate you to the media subsystem maintainers. > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: > > > > "Maintainers and committers should encourage contributors to request > > commit rights, especially junior contributors tend to underestimate > > their skills." > > Too early for that. We don't want junior contributors causing > merge issues and forcing maintainers to do rebases at media-committers.git. This doesn't mean we want to give commit rights to everybody. It's about showing inclusiveness and a will to trust people who show they deserve the trust, compared to wording it as being a secret society that can only be joined when co-opted by existing members. > > > + > > > +The ultimate responsibility for accepting a nominated committer is up to > > > +the subsystem's maintainers. The committers must earn a trust relationship > > > +with all subsystem maintainers, as, by granting you commit rights, they will > > > +be delegating part of their maintenance tasks. > > > + > > > +Due to that, to become a committer or a core committer, a consensus between > > > +all subsystem maintainers is required, as they all need to trust a developer > > > +well enough to be delegated the responsibility to maintain part of the code > > > +and to properly review patches from third parties, in a timely manner and > > > +keeping the status of the reviewed code at https://patchwork.linuxtv.org > > > +updated. > > > + > > > +.. Note:: > > > + > > > + In order to preserve the developers that could have their commit rights > > > > Do you mean "protect" instead of "preserve" ? Or maybe "preserve/protect > > the privacy of" ? > > Good point. I'd say both. > > > s/that/who/ > > s/their // (in the granted case, they don't have commit rights yet) > > > > > + granted or denied as well as the subsystem maintainers who have the > > > > "granted, denied or removed" ? > > Ok. > > > > + task to accept or deny commit rights, all communication related to > > > + nominating a committer, preserving commit rights or leaving such function > > > + should happen in private as much as possible. > > > + > > > +Media committer's agreement > > > +--------------------------- > > > + > > > +Once a nominated committer is accepted by all subsystem maintainers, > > > +they will ask if the developer is interested in the nomination and discuss > > > +what area(s) of the media subsystem the committer will be responsible for. > > > + > > > +Once the developer accepts being a committer, the new committer shall > > > +explicitly accept the Kernel development policies described under its > > > +Documentation/, and, in particular to the rules on this document, by writing > > > +an e-mail to media-committers@linuxtv.org, with a declaration of intent > > > +following the model below:: > > > + > > > + I, John Doe, would like to change my status to: Committer > > > + > > > + I intend to actively develop the XYZ driver, send fixes to drivers > > > + that I can test, reviewing patches and merging trivial fixes > > > + for the subsystem, ... > > > > "Merging trivial fixes for the subsystem" bothers me. I don't think it > > needs to be a requirement for committers. This is a maintainer's > > responsibility. If people want to help with that that's great, but > > making it a requirement isn't. Or did you mean this as an example ? > > This is an example. The scope will be case-by-case, and will be > part of the discussions with the maintainers. OK. Then maybe a sentence after this model to state that the list of activities is an example and should be adapted based on the submitter's interests would be good. > Yet, if a committer requests rights for driver A, I expect him/her to > pick trivial patches for it. Generally speaking, I would expect someone who commits patches for a driver to be listed in MAINTAINERS for that driver (possibly along other people), so they would indeed pick third-party patches. The way you phrased it above includes "for the subsystem" which gives the impression that committers have to volunteer to be janitors for the whole subsystem. People may do so, but I think we agree it's not a requirement, so avoiding implying it's required would be nice. > The committers may eventually pick trivial patches for other drivers. We > need to take some care with that to avoid multiple developers to review > the same patches and wasting their time. I'll be happy if we managed to scale up to a point where we get too many reviews :-) > > We shouldn't expect committers to handle a higher workload than what > > they do as driver maintainers who submit patches by e-mail or send pull > > requests. Giving commit rights will lower the effort to get patches in, > > and I think it's fair to ask for keeping patchwork up-to-date in return, > > but that's about it. > > > > > + > > > +Followed by a formal declaration of agreement with the Kernel development > > > +rules, signed with a PGP key cross signed by other Kernel and media > > > > s/PGP/GPG/ (same thing in practice, but let's advocate for free > > software) > > The Kernel docs referred in this section use the term PGP. I don't > mind replacing them to GPG, but this has to happen at the already > existing docs as well. Feel free to submit such patch later on, once > we get this merged upstream. > > > > > > +developers. Such declaration shall be:: > > > > I find the GPG signature requirement to be borderline ridiculous. The > > first message you're giving to committers is that you distrust them so > > much that you want them to sign an agreement with their blood > > (figuratively speaking). I don't think it's a very good approach to > > community building, nor does it bring any advantage to anyone. > > Please don't spread FUD. The intent is to ensure that the email > really came from the committer, and was not impersonated by someone's > else. There's no FUD here, this is my honest perception of the wording that implies, for me, that the relationship starts on mistrust. I think it gives the wrong message, and I don't believe that the risk of uncaught impersonification is real here. > > > + > > > + I hereby declare that I agree with the Kernel development rules described at: > > > + > > > + https://www.kernel.org/doc/html/latest/driver-api/media/media-committer.rst > > > + > > > + and to the Linux Kernel development process rules. > > > + > > > + I agree to the Code of Conduct as documented here: > > > > s/here:/in/ > > > > > + Documentation/process/code-of-conduct.rst > > > > You use an http link for meda-commiter.rst and a file path for > > code-of-conduct.rst. RST links for both would be more consistent, but if > > the goal is to paste the text in an e-mail, you can also use http links > > or file paths for both. > > Good point. Both shall be pointing to https://www.kernel.org/doc/html/latest/, > as this is the example of the content of the e-mail. Also, as this is inside > a Sphinx code-block, ReST links won't work. > > > > + I am aware that I can, at any point of time, retire. In that case, I will > > > + send an e-mail to notify the subsystem maintainers for them to revoke my > > > + commit rights. > > > + > > > + I am aware that the Kernel development rules change over time. > > > + By doing a new commit, I understand that I agree with the rules in effect > > > > What does "doing a new commit" mean here ? Sending a patch to the list ? > > Pushing it to the shared tree ? I assume the latter given the text > > below, it could be clarified here. > > It means a commit at media-committers.git. I'll clarify it. > > > > + at the time of the commit. > > > + > > > +For more details about PGP sign, please read > > > +Documentation/process/maintainer-pgp-guide.rst and > > > +:ref:`kernel_org_trust_repository`. > > > + > > > +In case the kernel development process changes, by merging new commits at the > > > > s/at the/in the/ > > > > > +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_, > > > > s/media-committers/media-committers tree/ > > > > > +the media committer implicitly declares that the agreement with the latest > > > > s/that the/their/ > > > > > +version of the documented process and to the contents of this file. > > > > s/to the contents // ("latest version" applies to both "the documented > > process" and "this file") > > > > This is problematic, as we can't expect people to check for changes in > > this file every time they push something. Changes to this file should be > > announced to all committers, with a reasonable review period. > > I don't expect that this document will change too much. See for instance > how many times we changed the media developers profile. Also, any > changes for the media committers document shall be c/c to linux-media > and hopefully c/c to media committers. More than "hopefully". Can we write in this document that changes *will* be CC'ed to all existing committers, that they will be given a reasonable amount of time to provide feedback (and I'm fine not defining precisely what "reasonable amount of time" is), and that subsystem maintainers will make a best effort to solve any issue throught consensus ? This still gives you full power ultimately, but is more community-building. > > But see: it is not just this file: it is the entire Kernel submission > process, and CoC. We can't expect that people will notify us with a > reasonable review period every time someone changes a bit at the > documentation process, as such discussions typically happen at > linux-docs ML. We can't generally speaking, but we can for areas we control, especially when making significatn changes to this document. I'm not concerned about tree-wide typo fixes that we wouldn't notice. > If such changes happen and a committer doesn't notice and gets > upset enough to decide retiring, The committer can step down when > this is noticed. Yet, we don't want any troubles for the already > committed patches, nor we want to cause any harm for the other > media committers and maintainers if this ever happens. In my experience people will be less upset if they have been given a chance to express their opinion before a decision is made, and if they feel their opinion is taken into consideration (even if it isn't always followed). > > > + > > > +Core committers > > > +--------------- > > > + > > > +As described in Documentation/driver-api/media/maintainer-entry-profile.rst > > > > RST link here too. > > > > > +a committer may be granted with additional privileges to also be able to > > > > s/privileges/rights/ > > > > (same below) > > > > > +change a core file and/or media subsystem's Kernel API/ABI. The extent of > > > > Drop "/ABI" as above. > > > > > +the core committer's additional privileges will be detailed by the subsystem > > > +maintainers when they nominate a core committer. > > > + > > > +Existing committers may become core committers and vice versa. Such > > > +decisions will be taken in consensus between the subsystem maintainers. > > > + > > > +Media committers rules > > > +---------------------- > > > + > > > +Media committers shall ensure that merged patches will not break any existing > > > +drivers. If it breaks, fixup or revert patches shall be merged as soon as > > > > How do they ensure that ? I would prefer mentioning a best effort here, > > as it's practically not possible to ensure this (until we have CI > > covering all the drivers in the subsystem, which is not a reasonable > > target). > > Makes sense, I'll change it. The important message there relies at the > second phrase: > > "If it breaks, fixup or revert patches shall be merged as soon as possible" > > > > +possible, aiming to be merged at the same Kernel cycle the bug is reported. > > > + > > > +Media committers shall behave accordingly to the permissions granted by > > > > s/permissions/rights/ (we never use "permission" anywhere else) > > > > > +the subsystem maintainers, specially with regards of the scope of changes > > > +they may apply directly at the media-committers tree. Such scope can > > > +change overtime on a mutual greement between media committers and > > > > s/overtime/over time/ > > s/greement/agreement/ > > > > > +maintainers. > > > + > > > +As described at :ref:`Media development workflow`, there are workflows. > > > > s/at /in / > > > > > +For the committers' workflow, the following rules apply: > > > + > > > +- Each merged patch shall pass CI tests; > > > + > > > +- Media committers shall request reviews from other committers were > > > > We shouldn't limit this to "other committers". When making changes to a > > driver that is not maintained by any committer (for instance when making > > tree-wide changes), the person in the best position to review the > > changes is the driver maintainer or author. I would extend this to "from > > third parties", or "from other developers". > > True, but the idea here is to enforce the need of collaboration with > other maintainers. So, perhaps: > > "from other committers and developers" I'm OK with that. > > > + applicable, i.e. because those committers have more knowledge about > > > + some areas that are changed by a patch; > > > + > > > +- No other media committer would be against the proposed changes. > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/committer-drm-misc.html#merge-criteria: > > > > "There must not be open issues or unresolved or conflicting feedback > > from anyone. Clear them up first. Defer to maintainers as needed." > > > > That's better than restricting the conflicts to committers. If there are > > open issues, the patches should not be merged through the shared tree. > > Works for me. > > > > + > > > +Patches that do not fall under the committer's workflow criteria will follow > > > +the normal workflow as described at :ref:`Media development workflow`. > > > + > > > +Only a subsystem maintainer can override such rules. > > > + > > > +All media committers shall ensure that patchwork will reflect the current > > > +status, e.g. patches shall be delegated to the media committer who is > > > +handling them and the patch status shall be updated according to these rules: > > > + > > > +- ``Under review``: Used if the patch requires a second opinion > > > + or when it is part of a pull request; > > > +- ``Accepted``: Once a patch is merged at the multi-committer tree. > > > > s/at the/in the/ > > > > > +- ``Superseded``: There is a newer version of the patch posted in the > > > > s/in the/to the/ > > > > > + mailing list. > > > +- ``Duplicated``: There was another patch doing the same thing from someone > > > + else that was accepted. > > > +- ``Not Applicable``: Use for patch series that are not merged at media.git > > > + tree (e.g. drm, dmabuf, upstream merge, etc.) but were cross-posted to the > > > + linux-media mailing list. > > > + > > > +If the committer decides not to merge it, then reply by email to patch > > > +authors, explaining why it is not merged, and patchwork shall be updated > > > +accordingly with either: > > > + > > > +- ``Changes Requested``: if a new revision was requested; > > > +- ``Rejected``: if the proposed change won't be merged upstream. > > > > There are tools to ease updating the status of a patch, could you > > document or at least mention them ? > > I'll add a link to: > https://patchwork.readthedocs.io/en/latest/usage/clients/ > > From my side, I only tested pwclient (and the REST API interface > which I use on some scripts, like the one at Jenkins to test PRs). Is there a document that explains how to udpate the status of a series on the command line based on the msgid ? That would be very useful to integrate it in mail client macros. > > > + > > > +If a media committer decides to retire, it is the committer's duty to > > > +notify the subsystem maintainers about that decision. > > > + > > > +Maintaining media committer status > > > +---------------------------------- > > > + > > > +A community of committers working together to move the Linux Kernel > > > +forward is essential to creating successful projects that are rewarding > > > +to work on. If there are problems or disagreements within the community, > > > +they can usually be solved through healthy discussion and debate. > > > + > > > +In the unhappy event that a media committer continues to disregard good > > > +citizenship (or actively disrupts the project), we may need to revoke > > > > That's very, very vague, surprisingly vague even from someone who raised > > many concerns about the kernel code of conduct being vague. > > This text came from Google Chromium committer document: > https://chromium.googlesource.com/playground/chromium-org-site/+/refs/heads/main/getting-involved/become-a-committer.md#maintaining-committer-status > > It is vague, a revoking commit rights is a discretionary act. It is nearly > impossible to describe all conditions that could lead a committer to be > distrusted. Indeed, but we can be more precise regarding the process. For instance we could mention that affected committers will be given an opportunity to defend themselves before access is revoked when the revocation isn't urgent. > > > +that person's status. In such cases, if someone suggests the revocation with > > > +a good reason, other developers may second the motion. The final decision > > > +is taken by the subsystem maintainers. As the decision to become a media > > > > What does "seconding the motion" bring, if the decision lies solely in > > maintainers ? > > The decision is up to maintainers, but other committers/developers may > voice about it. Yes, but what's the rationale for that, if the decision is made solely by you and Hans ? What could be worth saying on this topic is that subsystem maintainers will try to gather feedback from other committers before making a final decision. I used the word "try" on purpose as sometimes urgent revocation of commit rights could be needed, but I really don't expect that to be common, or to even ever happen in practice (yes, I know you've had problems with a couple of people in the past, but the probability of this happening again within the committers group is still very low). > > > +committer comes from a consensus between subsystem maintainers, a single > > > +subsystem maintainer not trusting the media committer anymore is enough to > > > +revoke committer's privileges. > > > + > > > +If a committer is inactive for more than a couple of Kernel cycles, > > > +maintainers will try to reach you via e-mail. If not possible, they may s/you/them/ (for consistency, the rest of this section addresses the reader with the 3rd person singular) > > > +revoke your committer privileges and update MAINTAINERS file entries s/your/their/ > > > +accordingly. If you wish to resume contributing later on, then contact s/you swish/the commmitter wishes/ s/then/then they can/ > > > +the subsystem maintainers to ask if your rights can be restored. "to ask for their rights to be restored." > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: > > > > "Committers are encouraged to request their commit rights get removed > > when they no longer contribute to the project. Commit rights may be > > automatically revoked after a year of inactivity (no commits or > > reviews). > > I prefer the current version, as it makes clearer that we expect > committers to request removal before becoming inactive. > > > Commit rights will be reinstated when they come back to the > > project." > > This doesn't sound right, as it means that just returning back > is an enough condition for having committer grants restored. > > This should not be automatic: the returned contributors need to > opt-in to be committers again, and the maintainers need to be > confident that the developers will do their duties before restore > committers' grants. I didn't push for this to be automatic. I'm OK keeping the current wording (with the 2nd person replaced by the 3rd for consistency). > > > + > > > +A previous committer that had his commit rights revoked can keep contributing > > > > s/his/their/ > > > > > +to the subsystem via the normal e-mail workflow as documented at the > > > +:ref:`Media development workflow`. > > > + > > > +References > > > +---------- > > > + > > > +Much of this was inspired by/copied from the committer policies of: > > > + > > > +- `Chromium <https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md>`_; > > > +- `WebKit <http://www.google.com/url?q=http%3A%2F%2Fwebkit.org%2Fcoding%2Fcommit-review-policy.html&sa=D&sntz=1&usg=AFrqEze4W4Lvbhue4Bywqgbv-N5J66kQgA>`_; > > > > Google tracks us enough without using google URLs. > > I'll fix. > > > > > > +- `Mozilla <http://www.google.com/url?q=http%3A%2F%2Fwww.mozilla.org%2Fhacking%2Fcommitter%2F&sa=D&sntz=1&usg=AFrqEzecK7iiXqV30jKibNmmMtzHwtYRTg>`_. > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html > > would also have been a good source of inspiration. That's the only large > > multi-committer workflow today in the kernel, and it has proven its > > value. The explicit acceptance criteria in particular are very good. > > Quoting the document, it says > > > > "Commit rights will be granted to anyone who requests them and fulfills > > the below criteria:" > > > > That's how we build an inclusive community, it feels way more welcoming > > than saying that maintainers will discuss in private and grant > > privileges to underlings if it pleases them (even if the effect is the > > same in practice, it's still a maintainer decision). > > The criteria for drm-misc has a big gap from what we want: > > "This should not be interpreted as a requirement to review other > peoples patches" > > We do require committers to review other peoples patches. It sounds I don't want to require that. We can encourage it, we can try to get people to expand their role and help more with overall subsystem maintenance to try and scale, but voicing that as a requirement is wrong for me. The carrot will work better than the stick. > that the criteria there for xe/i915 partially fixes the issue with > drm-misc, as it adds this criteria: > > "Has reviewed at least 25 patches from other developers to the > chosen driver that have already been merged upstream. Again, > most of the reviewed patches must be non-trivial." > > But still it doesn't say that committers shall review other peoples > patches(*). > > (*) Such reviews happen in practice there, but the number of patches > from external parties for xe/915 are very small and they usually come > from business partners. This is a very different reality than what we > have on media. > > Besides that, in practical terms, we're not ready to accept "anyone". "Anyone who requests them *and* fulfills the criteria". That's very far from anyone. Again, it's a matter of being inclusive in the wording. Wording matters, we'll have a much bigger change of success if the community appears inviting rather than suspicious. > Our current goal is to grant commit rights to the most active developers > that have been doing a good work reviewing patches from others. On other > words, the focus are on those who has already grants at patchwork to > update patches. It doesn't mean that we're limited to them. It is just > that we need to implement multi-committers without causing troubles to > the subsystem's workflow. Absolutely no disagreement on the fact that we'll start with a smaller pool of the most experienced contributors, and slowly expand that over time. > Also, we had enough troubles with our past multi-committers model, > back in the days we were using Mercurial, because there, we used > to grant committer access to "anyone" that were active at the > media development. > > So, it has to be a step-by-step approach. > > Maybe after a couple of years this will be revisited and we could use > a more inclusive text, but first we need to ensure that media-ci, The text should be inclusive from the beginning. It's absolutely fine to start small, and tell people they will have to wait as we don't want to expand faster than we can manage. Please rest assured I won't pressure you to grant committer access to dozens of people from day one, or even to anyone before I get to try the process and grow enough confidence that it can scale. > patchwork and the multi-committer's model will work for us on a > multi-committers model. Then, we'll likely need a tool similar to `dim` > to help with the merge requests and avoid problems that may > force maintainers to do rebases. > > > > + > > > diff --git a/Documentation/process/maintainer-pgp-guide.rst b/Documentation/process/maintainer-pgp-guide.rst > > > index f5277993b195..795ef8d89271 100644 > > > --- a/Documentation/process/maintainer-pgp-guide.rst > > > +++ b/Documentation/process/maintainer-pgp-guide.rst > > > @@ -903,6 +903,8 @@ the new default in GnuPG v2). To set it, add (or modify) the > > > > > > trust-model tofu+pgp > > > > > > +.. _kernel_org_trust_repository: > > > + > > > Using the kernel.org web of trust repository > > > -------------------------------------------- > > >
Em Wed, 27 Nov 2024 10:39:48 +0100 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > > This workflow doesn't apply to patch submitters who are not allowed to > > send pull requests and who don't have direct commit access. I thought > > these submitters are the main audience of this document. In that case, I > > think moving the next section that explains the e-mail workflow before > > the "Media development workflow" section (which should likely be renamed > > to make it clear that it is about merging patches, not developing them) > > would be best. The "Review Cadence" section could also be folded in > > there, to give a full view of what a submitter can expect. > > > > This would also have the advantage of introducing the linuvtv.org > > patchwork instance, which you reference above. Documents are more > > readable when they introduce concepts first before using them. > > Will try to do such change at v2. Actually, both workflows (a) and (b) apply to the ones that can't send pull requests or push at media-committers.git: --- a. Normal workflow: patches are handled by subsystem maintainers:: +------+ +---------+ +-------+ +-----------------------+ +---------+ |e-mail|-->|patchwork|-->|pull |-->|maintainers merge |-->|media.git| +------+ +---------+ |request| |in media-committers.git| +---------+ +-------+ +-----------------------+ For this workflow, pull requests can be generated by a committer, a previous committer, subsystem maintainers or by a couple of trusted long-time contributors. If you are not in such group, please don't submit pull requests, as they will likely be ignored. b. Committers' workflow: patches are handled by media committers:: +------+ +---------+ +--------------------+ +-----------+ +---------+ |e-mail|-->|patchwork|-->|committers merge at |-->|maintainers|-->|media.git| +------+ +---------+ |media-committers.git| |approval | +---------+ +--------------------+ +-----------+ --- No matter who sent an e-mail, this will be picked by patchwork and either be part of a PR or a MR, depending on who picked it. Thanks, Mauro
Hi Laurent, Thank you for your constructive feedback! In my reply I didn't comment on your trivial changes like grammar/spelling, unless otherwise noted I'm OK with those. On 26/11/2024 16:19, Laurent Pinchart wrote: > Hi Mauro and Hans, > > On Mon, Nov 25, 2024 at 02:28:58PM +0100, Mauro Carvalho Chehab wrote: >> As the media subsystem will experiment with a multi-committers model, >> update the Maintainer's entry profile to the new rules, and add a file >> documenting the process to become a committer and to maintain such >> rights. >> >> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> >> Signed-off-by: Hans Verkuil <hverkuil@xs4ll.nl> >> --- >> Documentation/driver-api/media/index.rst | 1 + >> .../media/maintainer-entry-profile.rst | 193 ++++++++++---- >> .../driver-api/media/media-committer.rst | 252 ++++++++++++++++++ >> .../process/maintainer-pgp-guide.rst | 2 + >> 4 files changed, 398 insertions(+), 50 deletions(-) >> create mode 100644 Documentation/driver-api/media/media-committer.rst >> >> diff --git a/Documentation/driver-api/media/index.rst b/Documentation/driver-api/media/index.rst >> index d5593182a3f9..d0c725fcbc67 100644 >> --- a/Documentation/driver-api/media/index.rst >> +++ b/Documentation/driver-api/media/index.rst >> @@ -26,6 +26,7 @@ Documentation/userspace-api/media/index.rst >> :numbered: >> >> maintainer-entry-profile >> + media-committer >> >> v4l2-core >> dtv-core >> diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst >> index ffc712a5f632..90c6c0d9cf17 100644 >> --- a/Documentation/driver-api/media/maintainer-entry-profile.rst >> +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst >> @@ -27,19 +27,128 @@ It covers, mainly, the contents of those directories: >> Both media userspace and Kernel APIs are documented and the documentation >> must be kept in sync with the API changes. It means that all patches that >> add new features to the subsystem must also bring changes to the >> -corresponding API files. >> +corresponding API documentation files. > > I would have split this kind of small changes to a separate patch to > make reviews easier, but that's not a big deal. > >> >> -Due to the size and wide scope of the media subsystem, media's >> -maintainership model is to have sub-maintainers that have a broad >> -knowledge of a specific aspect of the subsystem. It is the sub-maintainers' >> -task to review the patches, providing feedback to users if the patches are >> +Due to the size and wide scope of the media subsystem, the media's >> +maintainership model is to have committers that have a broad knowledge of >> +a specific aspect of the subsystem. It is the committers' task to >> +review the patches, providing feedback to users if the patches are >> following the subsystem rules and are properly using the media kernel and >> userspace APIs. > > This sounds really like a maintainer definition. I won't bikeshed too > much on the wording though, we will always be able to adjust it later to > reflect the reality of the situation as it evolves. I do like the > removal of the "sub-maintainer" term though, as I've always found it > demeaning. Yeah, that was never a great name. > >> >> -Patches for the media subsystem must be sent to the media mailing list >> -at linux-media@vger.kernel.org as plain text only e-mail. Emails with >> -HTML will be automatically rejected by the mail server. It could be wise >> -to also copy the sub-maintainer(s). >> +Media committers >> +---------------- >> + >> +In the media subsystem, there are experienced developers that can commit > > s/that/who/ > s/commit/push/ to standardize the vocabulary (below you use "upload" to > mean the same thing) > >> +patches directly on a development tree. These developers are called > > s/on a/to the/ > >> +Media committers and are divided into the following categories: >> + >> +- Committers: responsible for one or more drivers within the media subsystem. >> + They can upload changes to the tree that do not affect the core or ABI. > > s/upload/push/ > >> + >> +- Core committers: responsible for part of the media core. They are typically >> + responsible for one or more drivers within the media subsystem, but, besides >> + that, they can also merge patches that change the code common to multiple >> + drivers, including the kernel internal API/ABI. > > I would write "API" only here. Neither the kernel internal API nor its > internal ABI are stable, and given that lack of stability, the ABI > concept doesn't really apply within the kernel. I agree. > >> + >> +- Subsystem maintainers: responsible for the subsystem as a whole, with >> + access to the entire subsystem. >> + >> + Only subsystem maintainers can change the userspace API/ABI. > > This can give the impression that only subsystem maintainers are allowed > to work on the API. I would write > > Only subsystem maintainers change push changes that affect the userspace > API/ABI. change push changes -> can push changes > >> + >> +Media committers shall explicitly agree with the Kernel development process > > Do we have to capitalize "Kernel" everywhere ? There are way more > occurrences of "kernel" than "Kernel" in Documentation/ (even excluding > the lower case occurrences in e-mail addresses, file paths, ...). Personally I prefer lower case as well. Not a big deal for me, but it is certainly more consistent with how it is commonly written. > >> +as described at Documentation/process/index.rst and to the Kernel >> +development rules inside the Kernel documentation, including its code of >> +conduct. > > That's expected of all kernel developers, but I don't object reiterating > it here. > >> + >> +More details about media committers can be found here: > > Maybe "about media commiters' roles and responsibilities" would be more > accurate, the document doesn't include information about the committers > themselves. That's indeed better. > > s/here:/at/ > >> +Documentation/driver-api/media/media-committer.rst. > > Should this be a RST link ? > >> + >> +Media development tree >> +---------------------- >> + >> +The main development tree used by the media subsystem is hosted at LinuxTV.org, >> +where we also maintain news about the subsystem, wiki pages and a patchwork >> +instance where we track patches though their lifetime. >> + >> +The main tree used by media developers is at: >> + >> +https://git.linuxtv.org/media.git/ > > I think the multi-committers tree should be upgraded to main development > tree status, but that's a separate issue that I'm fine discussing > separately, and also delaying for a few kernel releases until we iron > out the rough edges of the process. Something to discuss in a year or so. > >> + >> +.. _Media development workflow: >> + >> +Media development workflow >> +++++++++++++++++++++++++++ >> + >> +All changes for the media subsystem must be sent first as e-mails to the >> +media mailing list, as plain text only e-mail to: >> + >> + `https://subspace.kernel.org/vger.kernel.org.html <linux-media@vger.kernel.org>`_ >> + >> +Emails with HTML will be automatically rejected by the mail server. >> +It could be wise to also copy the media committer(s). You should use >> +``scripts/get_maintainers.pl`` to identify whom else needs to be copied. >> +Please always copy driver's authors and maintainers. > > Instead of duplicating a tiny part of the patch submission process, I > would link to the appropriate documentation. Makes sense. > > [1] (see below) > >> + >> +Such patches needed to be based against a public branch or tag as follows: > > s/needed/need/ > >> + >> +1. new Kernel releases: > > s/new/New/ > > What does that mean though ? It's not very clear. > >> + >> + Those need to be based at the ``next`` branch of that media.git tree >> + >> +2. During Kernel release development cycle, patches fixing bugs on a -rc >> + kernel should preferably be against the latest -rc1 Kernel. If they >> + require a previously-applied change, they need to be against the ``fixes`` >> + branch; >> + >> +3. Patches against an already released kernel should preferably be against >> + the latest released Kernel. If they require a previously-applied >> + change, they need to be against ``fixes``. > > If I were a new contributor I think I would have trouble understand this > to be honest. I won't push hard for a rework of this section, as I > expect it will change after the multi-committer tree becomes the main > way to get patches merged. We can then update the documentation. I wonder if this shouldn't be rewritten completely, e.g. something like this: 1) By default patches are against the ``next`` branch of that media.git tree. 2) Patches that need to fix bugs in the -rcX kernel should preferably be against the latest -rc kernel. Is there really anything else? These two cases are all I use in practice. > >> + >> +All patches with fixes shall have: >> + - a ``Fixes:`` tag pointing to the first commit that introduced the bug; >> + - a ``Cc: stable@vger.kernel.org`` >> + >> +Patches that were fixing bugs reported by someone else shall have: >> + - a ``Reported-by`` tag immediately followed by a ``Closes`` tag. > > There's been a recent discussion about not including a Reported-by tag > without asking permission from the reporter, due to privacy reasons (in > particular for bugs reported to https://bugzilla.kernel.org/, as by > default the e-mail address of the reporter is not public). As the > Reported-by and Closes tags are not specific to the media tree, I would > drop this paragraph, otherwise we will have to duplicate a relatively > large amount of information related to privacy. You can link to the > relevant documentation instead, but I wouldn't even do that as it's > really not media-specific. I agree. Note that I tend to ask first for permission, unless I know the reporter is a kernel contributor already, or is otherwise active in the open source world and so the email is public already. > >> + >> +Patches that change API/ABI shall require patches to update documentation > > s/require patches to // > >> +accordingly at the same patch series. > > s/at the/in the/ > >> + >> +See Documentation/process/index.rst for more details about e-mail submission. > > This could be moved up to [1]. > >> + >> +Once a patch is submitted, it may follow either one of the workflows >> +below: > > "of the following workflows" > >> + >> +a. Normal workflow: patches are handled by subsystem maintainers:: >> + >> + +------+ +---------+ +-------+ +-------------------+ +---------+ >> + |e-mail|-->|patchwork|-->|pull |-->|maintainers merge |-->|media.git| >> + +------+ +---------+ |request| |at media-committers| +---------+ >> + +-------+ +-------------------+ > > This makes it look like the pull request originates from patchwork. > > s/at /in / > > I would also add .git to media-committers to make it clear it's a git > tree. > > (I still think the tree name is too long and not very appropriate, but > that's a different question) > >> + >> + For this workflow, pull requests can be generated by a committer, >> + a previous committer, subsystem maintainers or by a couple of trusted >> + long-time contributors. If you are not in such group, please don't submit >> + pull requests, as they will likely be ignored. >> + >> +b. Committers' workflow: patches are handled by media committers:: >> + >> + +------+ +---------+ +-------------------+ +-----------+ +---------+ >> + |e-mail|-->|patchwork|-->|committers merge |-->|maintainers|-->|media.git| >> + +------+ +---------+ |at media-committers| |approval | +---------+ >> + +-------------------+ +-----------+ >> + >> +When patches are merged at patchwork and when merged at media-committers, > > We don't "merge" patches in patchwork. You could write "When patches are > picked by patchwork" for instance. > >> +CI bots will check for errors and may provide e-mail feedback about >> +patch problems. When this happens, the e-mail author must fix them > > s/e-mail author/patch author/ ? Or possibly better, "patch submitter" as > that person may not be the author. > >> +and send another version of the patch. >> + >> +Patches will only be moved to the next stage in those two workflows if they >> +don't fail on CI or if there are false-positives at the CI reports. > > s/at the/in the/ > >> + > > This workflow doesn't apply to patch submitters who are not allowed to > send pull requests and who don't have direct commit access. I thought > these submitters are the main audience of this document. In that case, I > think moving the next section that explains the e-mail workflow before > the "Media development workflow" section (which should likely be renamed > to make it clear that it is about merging patches, not developing them) > would be best. The "Review Cadence" section could also be folded in > there, to give a full view of what a submitter can expect. > > This would also have the advantage of introducing the linuvtv.org > patchwork instance, which you reference above. Documents are more > readable when they introduce concepts first before using them. I think that's a good idea. > >> +Failures during e-mail submission >> ++++++++++++++++++++++++++++++++++ >> >> Media's workflow is heavily based on Patchwork, meaning that, once a patch >> is submitted, the e-mail will first be accepted by the mailing list >> @@ -47,51 +156,36 @@ server, and, after a while, it should appear at: >> >> - https://patchwork.linuxtv.org/project/linux-media/list/ >> >> -If it doesn't automatically appear there after a few minutes, then >> +If it doesn't automatically appear there after some time [2]_, then >> probably something went wrong on your submission. Please check if the >> -email is in plain text\ [2]_ only and if your emailer is not mangling >> +email is in plain text\ [3]_ only and if your emailer is not mangling >> whitespaces before complaining or submitting them again. >> >> -You can check if the mailing list server accepted your patch, by looking at: >> +To troubleshoot problems, you should first check if the mailing list >> +server has accepted your patch, by looking at: >> >> - https://lore.kernel.org/linux-media/ >> >> -.. [2] If your email contains HTML, the mailing list server will simply >> +If the patch is there and not at patchwork, it is likely that your e-mailer >> +mangled the patch. Patchwork internally has a logic that checks if the >> +received e-mail contain a valid patch. Any whitespace and new line >> +breakages mangling the patch won't be recognized by patchwork, thus such >> +patch will be rejected. >> + >> +.. [2] It usually takes a few minutes for the patch to arrive, but >> + the e-mail server is busy, so it may take up to a few hours > > s/is busy/may be busy/ > >> + for a patch to be handled by the mail server and by the patchwork >> + instance. > > "for a patch to be recorded by patchwork." > >> + >> +.. [3] If your email contains HTML, the mailing list server will simply >> drop it, without any further notice. > > These changes too could have been split to a separate cleanup patch, to > make the crux of this patch easier to review. > >> >> +Subsystem maintainers >> +--------------------- >> >> -Media maintainers >> -+++++++++++++++++ >> - >> -At the media subsystem, we have a group of senior developers that >> -are responsible for doing the code reviews at the drivers (also known as >> -sub-maintainers), and another senior developer responsible for the >> -subsystem as a whole. For core changes, whenever possible, multiple >> -media maintainers do the review. >> - >> -The media maintainers that work on specific areas of the subsystem are: >> - >> -- Remote Controllers (infrared): >> - Sean Young <sean@mess.org> >> - >> -- HDMI CEC: >> - Hans Verkuil <hverkuil@xs4all.nl> >> - >> -- Media controller drivers: >> - Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> - >> -- ISP, v4l2-async, v4l2-fwnode, v4l2-flash-led-class and Sensor drivers: >> - Sakari Ailus <sakari.ailus@linux.intel.com> >> - >> -- V4L2 drivers and core V4L2 frameworks: >> - Hans Verkuil <hverkuil@xs4all.nl> > > We're losing that information, isn't it valuable ? That's a good point. I think we still want to keep this information. Although this list should probably be moved to... > >> - >> -The subsystem maintainer is: >> - Mauro Carvalho Chehab <mchehab@kernel.org> >> - >> -Media maintainers may delegate a patch to other media maintainers as needed. >> -On such case, checkpatch's ``delegate`` field indicates who's currently >> -responsible for reviewing a patch. >> +The subsystem maintainers are: >> + Mauro Carvalho Chehab <mchehab@kernel.org> and >> + Hans Verkuil <hverkuil@xs4all.nl> ...here. > > If you don't intend on this being rendered as a list, it should be > > The subsystem maintainers are Mauro Carvalho Chehab <mchehab@kernel.org> and > Hans Verkuil <hverkuil@xs4all.nl>. > > Otherwise, > > The subsystem maintainers are: > > - Mauro Carvalho Chehab <mchehab@kernel.org> > - Hans Verkuil <hverkuil@xs4all.nl> > > > Shouldn't MAINTAINERS be updated at the same time ? Good point, I posted a patch. > >> >> Submit Checklist Addendum >> ------------------------- >> @@ -108,17 +202,14 @@ implementing the media APIs: >> ==================== ======================================================= >> Type Tool >> ==================== ======================================================= >> -V4L2 drivers\ [3]_ ``v4l2-compliance`` >> +V4L2 drivers\ [4]_ ``v4l2-compliance`` >> V4L2 virtual drivers ``contrib/test/test-media`` >> CEC drivers ``cec-compliance`` >> ==================== ======================================================= >> >> -.. [3] The ``v4l2-compliance`` also covers the media controller usage inside >> +.. [4] The ``v4l2-compliance`` also covers the media controller usage inside >> V4L2 drivers. >> >> -Other compilance tools are under development to check other parts of the >> -subsystem. >> - >> Those tests need to pass before the patches go upstream. >> >> Also, please notice that we build the Kernel with:: >> @@ -134,6 +225,8 @@ Where the check script is:: >> Be sure to not introduce new warnings on your patches without a >> very good reason. >> >> +Please see `Media development workflow`_ for e-mail submission rules. >> + > > These hunks too could have been split to a cleanup patch before the main > changes. > >> Style Cleanup Patches >> +++++++++++++++++++++ >> >> diff --git a/Documentation/driver-api/media/media-committer.rst b/Documentation/driver-api/media/media-committer.rst >> new file mode 100644 >> index 000000000000..27b85a37fb2b >> --- /dev/null >> +++ b/Documentation/driver-api/media/media-committer.rst >> @@ -0,0 +1,252 @@ >> +Media committers >> +================ >> + >> +What is a media committer? >> +-------------------------- >> + >> +A media committer is a developer who can apply patches from other developers > > Here too we could standardize to "push" instead of "apply". > >> +and their own patches at the > > s/at the/to the/ > > I would simply write "who can push commits to the ...". > >> +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_ >> +tree. >> + >> +It is a media committer's duty to ensure that their entries at the MAINTAINERS > > s/at the/in the/ > >> +file will be kept updated, and that submitted patches for files for which > > s/will be kept update/are kept up-to-date/ > >> +they are listed as maintainers are timely reviewed at the mailing list, > > s/at the/on the/ > >> +not waiting in patchwork as ``New`` for more than one Kernel merge cycle, >> +and, if accepted, applying them at the media committer's tree. > > I think this goes a bit backward, and mixes things up a bit. On the > mixing side, the expectation of timely reviews comes from maintainer > status. Having commit rights is orthogonal to that. > > The goal of direct commit access is to speed up maintenance, to get > patches reviewed and merged quicker. Are we saying here that if someone > has commit rights they will lose them if they take too long to review > code ? That would then slow down maintenance even more, which seems > counterproductive. Someone with commit rights is also a maintainer, since that's how you gain the trust to get those rights. If you do a poor job of reviewing patches relevant for you as maintainer, then you loose that trust. And if you simply don't have the time anymore for that, then perhaps you need to look for a co-maintainer or just stop being a maintainer for that area. A good example of that is actually the uvc driver. In that case the solution was adding a co-maintainer. > Also, while one can be recognized as a maintainer for multiple drivers > or parts of the kernel, there's a single committer status. You can't > revoke committer status of a particular driver only. The committer status is a sign that you are trusted. Part of that is timely patch review. Or admit that you won't have the time/resources to do that job and look for a co-maintainer or even give up maintainership of some areas. That said, perhaps the text should change a bit: "not waiting in patchwork as ``New`` for more than one Kernel merge cycle" -> "ideally not waiting in patchwork as ``New`` for more than one Kernel merge cycle" We all have patches in patchwork that are much older than that, for one reason or another, but if this happens all the time, then you have a problem. > >> + >> +This privilege is granted with some expectation of responsibility: > > "Privilege" sounds a bit like lord and serf. How about 'These commit rights are granted'? > >> +committers are people who care about the Linux Kernel as a whole and >> +about the Linux media subsystem and want to help its development. It >> +is also based on a trust relationship between the rest of the committers, >> +maintainers and the LinuxTV community. > > Who is "the LinuxTV community" ? "linux kernel media community"? > >> + >> +As such, a media committer is not just someone who is capable of creating >> +code, but someone who has demonstrated their ability to collaborate >> +with the team, get the most knowledgeable people to review code, >> +contribute high-quality code, and follow through to fix issues (in code >> +or tests). >> + >> +.. Note:: >> + >> + 1. If a patch introduced a regression, then it is the media committer's > > s/introduced/introduces/ > >> + responsibility to correct that as soon as possible. Typically the >> + patch is either reverted, or an additional patch is committed that >> + fixes the regression; >> + 2. if patches are fixing bugs against already released Kernels, including >> + the reverts above mentioned, the media committer shall add the needed >> + tags. Please see :ref:`Media development workflow` for more details. > > s/ Please/ Please/ > >> + 3. all patches should be properly reviewed at >> + linux-media@vger.kernel.org before being merged at the > > s/at the/in the/ > >> + media-committers tree or submitted on pull requests. > > This is a fundamental rule that belongs to the development workflow in > my opinion. > >> + >> +Becoming a media committer >> +-------------------------- >> + >> +The most important aspect of volunteering to be a committer is that you will >> +be able to review and approve other people's changes, so we are looking for > > Everybody is able to review patches (rather, everybody is allowed to > review patches, the ability is a different matter). > >> +whether we think you will be good at doing that. > > I've been told that "whether" should also come with a "or" clause. You > can write "whether or not we think ...". How about this: "The most important aspect of volunteering to be a committer is that you have demonstrated the ability to give good code reviews." > >> + >> +As such, potential committers must earn enough credibility and trust from the >> +LinuxTV community. To do that, developers shall be familiar with the open We should probably search for "LinuxTV community" and replace it with "linux kernel media community". "LinuxTV" is fairly meaningless to me, it's just the name of our internet domain, but otherwise it doesn't feel like a community. An alternative name might be "media developers"? >> +source model and have been active at the Linux Kernel community for some time, > > s/at the/in the/ > >> +and, in particular, with the media subsystem. > > s/with the/in the/ > >> + >> +So, in addition to actually making the code changes, you are basically >> +demonstrating your: >> + >> +- commitment to the project; >> +- ability to collaborate with the team and communicate well; >> +- understand of how upstream and the LinuxTV community works > > s/ works/ work/ > >> + (policies, processes for testing, code review, ...) >> +- reasonable knowledge about: >> + >> + - the Kernel development process: >> + Documentation/process/index.rst >> + >> + - the Media development profile: >> + Documentation/driver-api/media/maintainer-entry-profile.rst >> + >> +- understanding of the projects' code base and coding style; >> +- ability to provide feedback to the patch authors; >> +- ability to judge when a patch might be ready for review and to submit; >> +- ability to write good code (last but certainly not least). >> + >> +It is also desirable that developers that intend to become committers >> +make a best effort to attend the yearly Linux Media Summit, typically >> +co-located with another Linux conference. > > I would say that "are encouraged to attend" instead of "make a best > effort to attend". Also, how will this scale when we'll have a few > dozen committers ? Typically the media summit is capped to 20 attendees > or less. If we have that many committers, then we can afford a larger room and we probably would have to start charging some contributions as well. But that would be a luxury problem :-) It's a bridge we can cross when we get there. Note that I am fine with "are encouraged to attend". I think that's a good phrase. > >> + >> +If you are doing such tasks and have become a valued developer, an >> +existing committer can nominate you to the media subsystem maintainers. > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: > > "Maintainers and committers should encourage contributors to request > commit rights, especially junior contributors tend to underestimate > their skills." In drm is it the contributors that request commit rights? Or is it those who already have commit rights that invite others? Currently the plan for the media subsystem is the second method. Although that might change in the future, of course. So since existing committers invite others, I'm not sure this is needed. > >> + >> +The ultimate responsibility for accepting a nominated committer is up to >> +the subsystem's maintainers. The committers must earn a trust relationship >> +with all subsystem maintainers, as, by granting you commit rights, they will >> +be delegating part of their maintenance tasks. >> + >> +Due to that, to become a committer or a core committer, a consensus between >> +all subsystem maintainers is required, as they all need to trust a developer >> +well enough to be delegated the responsibility to maintain part of the code >> +and to properly review patches from third parties, in a timely manner and >> +keeping the status of the reviewed code at https://patchwork.linuxtv.org >> +updated. >> + >> +.. Note:: >> + >> + In order to preserve the developers that could have their commit rights > > Do you mean "protect" instead of "preserve" ? Or maybe "preserve/protect > the privacy of" ? That last one is a good phrase, I like it. > > s/that/who/ > s/their // (in the granted case, they don't have commit rights yet) > >> + granted or denied as well as the subsystem maintainers who have the > > "granted, denied or removed" ? Yes. > >> + task to accept or deny commit rights, all communication related to Same here. >> + nominating a committer, preserving commit rights or leaving such function >> + should happen in private as much as possible. >> + >> +Media committer's agreement >> +--------------------------- >> + >> +Once a nominated committer is accepted by all subsystem maintainers, >> +they will ask if the developer is interested in the nomination and discuss >> +what area(s) of the media subsystem the committer will be responsible for. >> + >> +Once the developer accepts being a committer, the new committer shall >> +explicitly accept the Kernel development policies described under its >> +Documentation/, and, in particular to the rules on this document, by writing >> +an e-mail to media-committers@linuxtv.org, with a declaration of intent >> +following the model below:: >> + >> + I, John Doe, would like to change my status to: Committer >> + >> + I intend to actively develop the XYZ driver, send fixes to drivers >> + that I can test, reviewing patches and merging trivial fixes >> + for the subsystem, ... > > "Merging trivial fixes for the subsystem" bothers me. I don't think it > needs to be a requirement for committers. This is a maintainer's > responsibility. If people want to help with that that's great, but > making it a requirement isn't. Or did you mean this as an example ? > > We shouldn't expect committers to handle a higher workload than what > they do as driver maintainers who submit patches by e-mail or send pull > requests. Giving commit rights will lower the effort to get patches in, > and I think it's fair to ask for keeping patchwork up-to-date in return, > but that's about it. The idea was to make it explicit that they can review and merge trivial fixes for the subsystem as a whole (so outside the direct area that they maintain), but that is certainly optional. How about: ", and optionally reviewing patches and merging trivial fixes in other areas of the subsystem," Still a bit clunky, though. Suggestions are welcome. > >> + >> +Followed by a formal declaration of agreement with the Kernel development >> +rules, signed with a PGP key cross signed by other Kernel and media > > s/PGP/GPG/ (same thing in practice, but let's advocate for free > software) > >> +developers. Such declaration shall be:: > > I find the GPG signature requirement to be borderline ridiculous. The > first message you're giving to committers is that you distrust them so > much that you want them to sign an agreement with their blood > (figuratively speaking). I don't think it's a very good approach to > community building, nor does it bring any advantage to anyone. I kind of agree with Laurent here. Is the media-committers mailinglist publicly archived somewhere? I think it is sufficient if this is posted to a publicly archived mailinglist. That could be linux-media, I would be fine with that. But media-committers would be more appropriate, but only if it is archived somewhere. If we want a GPG key, what would we do with it anyway? > >> + >> + I hereby declare that I agree with the Kernel development rules described at: >> + >> + https://www.kernel.org/doc/html/latest/driver-api/media/media-committer.rst >> + >> + and to the Linux Kernel development process rules. >> + >> + I agree to the Code of Conduct as documented here: > > s/here:/in/ > >> + Documentation/process/code-of-conduct.rst > > You use an http link for meda-commiter.rst and a file path for > code-of-conduct.rst. RST links for both would be more consistent, but if > the goal is to paste the text in an e-mail, you can also use http links > or file paths for both. > >> + >> + I am aware that I can, at any point of time, retire. In that case, I will >> + send an e-mail to notify the subsystem maintainers for them to revoke my >> + commit rights. >> + >> + I am aware that the Kernel development rules change over time. >> + By doing a new commit, I understand that I agree with the rules in effect > > What does "doing a new commit" mean here ? Sending a patch to the list ? > Pushing it to the shared tree ? I assume the latter given the text > below, it could be clarified here. > >> + at the time of the commit. >> + >> +For more details about PGP sign, please read >> +Documentation/process/maintainer-pgp-guide.rst and >> +:ref:`kernel_org_trust_repository`. >> + >> +In case the kernel development process changes, by merging new commits at the Should this be "kernel media development"? > > s/at the/in the/ > >> +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_, > > s/media-committers/media-committers tree/ > >> +the media committer implicitly declares that the agreement with the latest > > s/that the/their/ > >> +version of the documented process and to the contents of this file. > > s/to the contents // ("latest version" applies to both "the documented > process" and "this file") > > This is problematic, as we can't expect people to check for changes in > this file every time they push something. Changes to this file should be > announced to all committers, with a reasonable review period. "Any changes to the kernel media development process will be announced in the media-committers mailinglist with a reasonable review period. All committers are automatically subscribed to that mailinglist." > >> + >> +Core committers >> +--------------- >> + >> +As described in Documentation/driver-api/media/maintainer-entry-profile.rst > > RST link here too. > >> +a committer may be granted with additional privileges to also be able to > > s/privileges/rights/ > > (same below) > >> +change a core file and/or media subsystem's Kernel API/ABI. The extent of > > Drop "/ABI" as above. > >> +the core committer's additional privileges will be detailed by the subsystem >> +maintainers when they nominate a core committer. >> + >> +Existing committers may become core committers and vice versa. Such >> +decisions will be taken in consensus between the subsystem maintainers. >> + >> +Media committers rules >> +---------------------- >> + >> +Media committers shall ensure that merged patches will not break any existing >> +drivers. If it breaks, fixup or revert patches shall be merged as soon as > > How do they ensure that ? I would prefer mentioning a best effort here, > as it's practically not possible to ensure this (until we have CI > covering all the drivers in the subsystem, which is not a reasonable > target). Good point. > >> +possible, aiming to be merged at the same Kernel cycle the bug is reported. >> + >> +Media committers shall behave accordingly to the permissions granted by > > s/permissions/rights/ (we never use "permission" anywhere else) > >> +the subsystem maintainers, specially with regards of the scope of changes >> +they may apply directly at the media-committers tree. Such scope can >> +change overtime on a mutual greement between media committers and > > s/overtime/over time/ > s/greement/agreement/ > >> +maintainers. >> + >> +As described at :ref:`Media development workflow`, there are workflows. > > s/at /in / > >> +For the committers' workflow, the following rules apply: >> + >> +- Each merged patch shall pass CI tests; >> + >> +- Media committers shall request reviews from other committers were > > We shouldn't limit this to "other committers". When making changes to a > driver that is not maintained by any committer (for instance when making > tree-wide changes), the person in the best position to review the > changes is the driver maintainer or author. I would extend this to "from > third parties", or "from other developers". Yes, 'developers' is better. > >> + applicable, i.e. because those committers have more knowledge about Same here. >> + some areas that are changed by a patch; >> + >> +- No other media committer would be against the proposed changes. > > https://drm.pages.freedesktop.org/maintainer-tools/committer/committer-drm-misc.html#merge-criteria: > > "There must not be open issues or unresolved or conflicting feedback > from anyone. Clear them up first. Defer to maintainers as needed." > > That's better than restricting the conflicts to committers. If there are > open issues, the patches should not be merged through the shared tree. Nice, good wordsmithing! > >> + >> +Patches that do not fall under the committer's workflow criteria will follow >> +the normal workflow as described at :ref:`Media development workflow`. >> + >> +Only a subsystem maintainer can override such rules. >> + >> +All media committers shall ensure that patchwork will reflect the current >> +status, e.g. patches shall be delegated to the media committer who is >> +handling them and the patch status shall be updated according to these rules: >> + >> +- ``Under review``: Used if the patch requires a second opinion >> + or when it is part of a pull request; >> +- ``Accepted``: Once a patch is merged at the multi-committer tree. > > s/at the/in the/ > >> +- ``Superseded``: There is a newer version of the patch posted in the > > s/in the/to the/ > >> + mailing list. >> +- ``Duplicated``: There was another patch doing the same thing from someone >> + else that was accepted. >> +- ``Not Applicable``: Use for patch series that are not merged at media.git >> + tree (e.g. drm, dmabuf, upstream merge, etc.) but were cross-posted to the >> + linux-media mailing list. >> + >> +If the committer decides not to merge it, then reply by email to patch >> +authors, explaining why it is not merged, and patchwork shall be updated >> +accordingly with either: >> + >> +- ``Changes Requested``: if a new revision was requested; >> +- ``Rejected``: if the proposed change won't be merged upstream. > > There are tools to ease updating the status of a patch, could you > document or at least mention them ? I think that is out-of-scope. It certainly could be added as a follow-up patch. > >> + >> +If a media committer decides to retire, it is the committer's duty to >> +notify the subsystem maintainers about that decision. >> + >> +Maintaining media committer status >> +---------------------------------- >> + >> +A community of committers working together to move the Linux Kernel >> +forward is essential to creating successful projects that are rewarding >> +to work on. If there are problems or disagreements within the community, >> +they can usually be solved through healthy discussion and debate. >> + >> +In the unhappy event that a media committer continues to disregard good >> +citizenship (or actively disrupts the project), we may need to revoke > > That's very, very vague, surprisingly vague even from someone who raised > many concerns about the kernel code of conduct being vague. I suspect that this phrasing is copied from another project. Mauro, can you confirm that? I think it is extremely difficult to give explicit guidance here. > >> +that person's status. In such cases, if someone suggests the revocation with >> +a good reason, other developers may second the motion. The final decision >> +is taken by the subsystem maintainers. As the decision to become a media > > What does "seconding the motion" bring, if the decision lies solely in > maintainers ? I think the intent here is that, other than in extreme circumstances, it shouldn't be a unilateral decision from the subsystem maintainers. Multiple media committers should agree with it. But perhaps it would be better to replace this with: "In such cases, if someone suggests the revocation with a good reason, then after discussing this among the media committers, the final decision is taken by the subsystem maintainers." I really hope we will never end up in a situation like this, since that's going to be painful regardless of what procedure you choose. > >> +committer comes from a consensus between subsystem maintainers, a single >> +subsystem maintainer not trusting the media committer anymore is enough to >> +revoke committer's privileges. >> + >> +If a committer is inactive for more than a couple of Kernel cycles, >> +maintainers will try to reach you via e-mail. If not possible, they may >> +revoke your committer privileges and update MAINTAINERS file entries >> +accordingly. If you wish to resume contributing later on, then contact >> +the subsystem maintainers to ask if your rights can be restored. > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: > > "Committers are encouraged to request their commit rights get removed > when they no longer contribute to the project. Commit rights may be > automatically revoked after a year of inactivity (no commits or > reviews). Commit rights will be reinstated when they come back to the > project." Yes, that's better. I also realized that the mention of updating the MAINTAINERS makes no sense since that does not contain media committer status. > >> + >> +A previous committer that had his commit rights revoked can keep contributing > > s/his/their/ > >> +to the subsystem via the normal e-mail workflow as documented at the >> +:ref:`Media development workflow`. >> + >> +References >> +---------- >> + >> +Much of this was inspired by/copied from the committer policies of: >> + >> +- `Chromium <https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md>`_; >> +- `WebKit <http://www.google.com/url?q=http%3A%2F%2Fwebkit.org%2Fcoding%2Fcommit-review-policy.html&sa=D&sntz=1&usg=AFrqEze4W4Lvbhue4Bywqgbv-N5J66kQgA>`_; > > Google tracks us enough without using google URLs. > >> +- `Mozilla <http://www.google.com/url?q=http%3A%2F%2Fwww.mozilla.org%2Fhacking%2Fcommitter%2F&sa=D&sntz=1&usg=AFrqEzecK7iiXqV30jKibNmmMtzHwtYRTg>`_. > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html > would also have been a good source of inspiration. That's the only large > multi-committer workflow today in the kernel, and it has proven its > value. The explicit acceptance criteria in particular are very good. > Quoting the document, it says > > "Commit rights will be granted to anyone who requests them and fulfills > the below criteria:" > > That's how we build an inclusive community, it feels way more welcoming > than saying that maintainers will discuss in private and grant > privileges to underlings if it pleases them (even if the effect is the > same in practice, it's still a maintainer decision). The main difference here is that in drm developers can ask for commit rights, whereas for the media subsystem they are invited by existing media committers. The drm model is absolutely more inclusive, and I hope we can end up there eventually. But for now I think we need more work on both the procedures and the media-ci workflow. Even with just two sub-maintainers committing patches it took quite a long time to find and fix all the bugs/issues we encountered. At this point we are definitely not ready to implement a drm model. This document just starts this process, it will change and be improved over time, but we need this in place before we can start adding more committers. Regards, Hans > >> + >> diff --git a/Documentation/process/maintainer-pgp-guide.rst b/Documentation/process/maintainer-pgp-guide.rst >> index f5277993b195..795ef8d89271 100644 >> --- a/Documentation/process/maintainer-pgp-guide.rst >> +++ b/Documentation/process/maintainer-pgp-guide.rst >> @@ -903,6 +903,8 @@ the new default in GnuPG v2). To set it, add (or modify) the >> >> trust-model tofu+pgp >> >> +.. _kernel_org_trust_repository: >> + >> Using the kernel.org web of trust repository >> -------------------------------------------- >> >
Hi Hans, On Wed, Nov 27, 2024 at 12:59:58PM +0100, Hans Verkuil wrote: > Hi Laurent, > > Thank you for your constructive feedback! In my reply I didn't comment on your > trivial changes like grammar/spelling, unless otherwise noted I'm OK with those. Some of the points you raised are also discussed in the other part of the mail thread. I'll elaborate here on the ones that are unique, and just mention for the other ones that we can discuss them in my reply to Mauro to avoid splitting the discussion. > On 26/11/2024 16:19, Laurent Pinchart wrote: > > Hi Mauro and Hans, > > > > On Mon, Nov 25, 2024 at 02:28:58PM +0100, Mauro Carvalho Chehab wrote: > >> As the media subsystem will experiment with a multi-committers model, > >> update the Maintainer's entry profile to the new rules, and add a file > >> documenting the process to become a committer and to maintain such > >> rights. > >> > >> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > >> Signed-off-by: Hans Verkuil <hverkuil@xs4ll.nl> > >> --- > >> Documentation/driver-api/media/index.rst | 1 + > >> .../media/maintainer-entry-profile.rst | 193 ++++++++++---- > >> .../driver-api/media/media-committer.rst | 252 ++++++++++++++++++ > >> .../process/maintainer-pgp-guide.rst | 2 + > >> 4 files changed, 398 insertions(+), 50 deletions(-) > >> create mode 100644 Documentation/driver-api/media/media-committer.rst > >> > >> diff --git a/Documentation/driver-api/media/index.rst b/Documentation/driver-api/media/index.rst > >> index d5593182a3f9..d0c725fcbc67 100644 > >> --- a/Documentation/driver-api/media/index.rst > >> +++ b/Documentation/driver-api/media/index.rst > >> @@ -26,6 +26,7 @@ Documentation/userspace-api/media/index.rst > >> :numbered: > >> > >> maintainer-entry-profile > >> + media-committer > >> > >> v4l2-core > >> dtv-core > >> diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst > >> index ffc712a5f632..90c6c0d9cf17 100644 > >> --- a/Documentation/driver-api/media/maintainer-entry-profile.rst > >> +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst > >> @@ -27,19 +27,128 @@ It covers, mainly, the contents of those directories: > >> Both media userspace and Kernel APIs are documented and the documentation > >> must be kept in sync with the API changes. It means that all patches that > >> add new features to the subsystem must also bring changes to the > >> -corresponding API files. > >> +corresponding API documentation files. > > > > I would have split this kind of small changes to a separate patch to > > make reviews easier, but that's not a big deal. > > > >> > >> -Due to the size and wide scope of the media subsystem, media's > >> -maintainership model is to have sub-maintainers that have a broad > >> -knowledge of a specific aspect of the subsystem. It is the sub-maintainers' > >> -task to review the patches, providing feedback to users if the patches are > >> +Due to the size and wide scope of the media subsystem, the media's > >> +maintainership model is to have committers that have a broad knowledge of > >> +a specific aspect of the subsystem. It is the committers' task to > >> +review the patches, providing feedback to users if the patches are > >> following the subsystem rules and are properly using the media kernel and > >> userspace APIs. > > > > This sounds really like a maintainer definition. I won't bikeshed too > > much on the wording though, we will always be able to adjust it later to > > reflect the reality of the situation as it evolves. I do like the > > removal of the "sub-maintainer" term though, as I've always found it > > demeaning. > > Yeah, that was never a great name. > > >> > >> -Patches for the media subsystem must be sent to the media mailing list > >> -at linux-media@vger.kernel.org as plain text only e-mail. Emails with > >> -HTML will be automatically rejected by the mail server. It could be wise > >> -to also copy the sub-maintainer(s). > >> +Media committers > >> +---------------- > >> + > >> +In the media subsystem, there are experienced developers that can commit > > > > s/that/who/ > > s/commit/push/ to standardize the vocabulary (below you use "upload" to > > mean the same thing) > > > >> +patches directly on a development tree. These developers are called > > > > s/on a/to the/ > > > >> +Media committers and are divided into the following categories: > >> + > >> +- Committers: responsible for one or more drivers within the media subsystem. > >> + They can upload changes to the tree that do not affect the core or ABI. > > > > s/upload/push/ > > > >> + > >> +- Core committers: responsible for part of the media core. They are typically > >> + responsible for one or more drivers within the media subsystem, but, besides > >> + that, they can also merge patches that change the code common to multiple > >> + drivers, including the kernel internal API/ABI. > > > > I would write "API" only here. Neither the kernel internal API nor its > > internal ABI are stable, and given that lack of stability, the ABI > > concept doesn't really apply within the kernel. > > I agree. > > >> + > >> +- Subsystem maintainers: responsible for the subsystem as a whole, with > >> + access to the entire subsystem. > >> + > >> + Only subsystem maintainers can change the userspace API/ABI. > > > > This can give the impression that only subsystem maintainers are allowed > > to work on the API. I would write > > > > Only subsystem maintainers change push changes that affect the userspace > > API/ABI. > > change push changes -> can push changes > > >> + > >> +Media committers shall explicitly agree with the Kernel development process > > > > Do we have to capitalize "Kernel" everywhere ? There are way more > > occurrences of "kernel" than "Kernel" in Documentation/ (even excluding > > the lower case occurrences in e-mail addresses, file paths, ...). > > Personally I prefer lower case as well. Not a big deal for me, but it is > certainly more consistent with how it is commonly written. > > >> +as described at Documentation/process/index.rst and to the Kernel > >> +development rules inside the Kernel documentation, including its code of > >> +conduct. > > > > That's expected of all kernel developers, but I don't object reiterating > > it here. > > > >> + > >> +More details about media committers can be found here: > > > > Maybe "about media commiters' roles and responsibilities" would be more > > accurate, the document doesn't include information about the committers > > themselves. > > That's indeed better. > > > > > s/here:/at/ > > > >> +Documentation/driver-api/media/media-committer.rst. > > > > Should this be a RST link ? > > > >> + > >> +Media development tree > >> +---------------------- > >> + > >> +The main development tree used by the media subsystem is hosted at LinuxTV.org, > >> +where we also maintain news about the subsystem, wiki pages and a patchwork > >> +instance where we track patches though their lifetime. > >> + > >> +The main tree used by media developers is at: > >> + > >> +https://git.linuxtv.org/media.git/ > > > > I think the multi-committers tree should be upgraded to main development > > tree status, but that's a separate issue that I'm fine discussing > > separately, and also delaying for a few kernel releases until we iron > > out the rough edges of the process. > > Something to discuss in a year or so. > > >> + > >> +.. _Media development workflow: > >> + > >> +Media development workflow > >> +++++++++++++++++++++++++++ > >> + > >> +All changes for the media subsystem must be sent first as e-mails to the > >> +media mailing list, as plain text only e-mail to: > >> + > >> + `https://subspace.kernel.org/vger.kernel.org.html <linux-media@vger.kernel.org>`_ > >> + > >> +Emails with HTML will be automatically rejected by the mail server. > >> +It could be wise to also copy the media committer(s). You should use > >> +``scripts/get_maintainers.pl`` to identify whom else needs to be copied. > >> +Please always copy driver's authors and maintainers. > > > > Instead of duplicating a tiny part of the patch submission process, I > > would link to the appropriate documentation. > > Makes sense. > > > > > [1] (see below) > > > >> + > >> +Such patches needed to be based against a public branch or tag as follows: > > > > s/needed/need/ > > > >> + > >> +1. new Kernel releases: > > > > s/new/New/ > > > > What does that mean though ? It's not very clear. > > > >> + > >> + Those need to be based at the ``next`` branch of that media.git tree > >> + > >> +2. During Kernel release development cycle, patches fixing bugs on a -rc > >> + kernel should preferably be against the latest -rc1 Kernel. If they > >> + require a previously-applied change, they need to be against the ``fixes`` > >> + branch; > >> + > >> +3. Patches against an already released kernel should preferably be against > >> + the latest released Kernel. If they require a previously-applied > >> + change, they need to be against ``fixes``. > > > > If I were a new contributor I think I would have trouble understand this > > to be honest. I won't push hard for a rework of this section, as I > > expect it will change after the multi-committer tree becomes the main > > way to get patches merged. We can then update the documentation. > > I wonder if this shouldn't be rewritten completely, e.g. something like this: > > 1) By default patches are against the ``next`` branch of that media.git tree. > > 2) Patches that need to fix bugs in the -rcX kernel should preferably be > against the latest -rc kernel. > > Is there really anything else? These two cases are all I use in practice. I've replied to Mauro on this particular point, and I think it would be clearer to split patches as features (not fixes), fixes for a released kernel, fixes for an unreleased kernel. Let's discuss that there. > >> + > >> +All patches with fixes shall have: > >> + - a ``Fixes:`` tag pointing to the first commit that introduced the bug; > >> + - a ``Cc: stable@vger.kernel.org`` > >> + > >> +Patches that were fixing bugs reported by someone else shall have: > >> + - a ``Reported-by`` tag immediately followed by a ``Closes`` tag. > > > > There's been a recent discussion about not including a Reported-by tag > > without asking permission from the reporter, due to privacy reasons (in > > particular for bugs reported to https://bugzilla.kernel.org/, as by > > default the e-mail address of the reporter is not public). As the > > Reported-by and Closes tags are not specific to the media tree, I would > > drop this paragraph, otherwise we will have to duplicate a relatively > > large amount of information related to privacy. You can link to the > > relevant documentation instead, but I wouldn't even do that as it's > > really not media-specific. > > I agree. Note that I tend to ask first for permission, unless I know the > reporter is a kernel contributor already, or is otherwise active in the open > source world and so the email is public already. > > >> + > >> +Patches that change API/ABI shall require patches to update documentation > > > > s/require patches to // > > > >> +accordingly at the same patch series. > > > > s/at the/in the/ > > > >> + > >> +See Documentation/process/index.rst for more details about e-mail submission. > > > > This could be moved up to [1]. > > > >> + > >> +Once a patch is submitted, it may follow either one of the workflows > >> +below: > > > > "of the following workflows" > > > >> + > >> +a. Normal workflow: patches are handled by subsystem maintainers:: > >> + > >> + +------+ +---------+ +-------+ +-------------------+ +---------+ > >> + |e-mail|-->|patchwork|-->|pull |-->|maintainers merge |-->|media.git| > >> + +------+ +---------+ |request| |at media-committers| +---------+ > >> + +-------+ +-------------------+ > > > > This makes it look like the pull request originates from patchwork. > > > > s/at /in / > > > > I would also add .git to media-committers to make it clear it's a git > > tree. > > > > (I still think the tree name is too long and not very appropriate, but > > that's a different question) > > > >> + > >> + For this workflow, pull requests can be generated by a committer, > >> + a previous committer, subsystem maintainers or by a couple of trusted > >> + long-time contributors. If you are not in such group, please don't submit > >> + pull requests, as they will likely be ignored. > >> + > >> +b. Committers' workflow: patches are handled by media committers:: > >> + > >> + +------+ +---------+ +-------------------+ +-----------+ +---------+ > >> + |e-mail|-->|patchwork|-->|committers merge |-->|maintainers|-->|media.git| > >> + +------+ +---------+ |at media-committers| |approval | +---------+ > >> + +-------------------+ +-----------+ > >> + > >> +When patches are merged at patchwork and when merged at media-committers, > > > > We don't "merge" patches in patchwork. You could write "When patches are > > picked by patchwork" for instance. > > > >> +CI bots will check for errors and may provide e-mail feedback about > >> +patch problems. When this happens, the e-mail author must fix them > > > > s/e-mail author/patch author/ ? Or possibly better, "patch submitter" as > > that person may not be the author. > > > >> +and send another version of the patch. > >> + > >> +Patches will only be moved to the next stage in those two workflows if they > >> +don't fail on CI or if there are false-positives at the CI reports. > > > > s/at the/in the/ > > > >> + > > > > This workflow doesn't apply to patch submitters who are not allowed to > > send pull requests and who don't have direct commit access. I thought > > these submitters are the main audience of this document. In that case, I > > think moving the next section that explains the e-mail workflow before > > the "Media development workflow" section (which should likely be renamed > > to make it clear that it is about merging patches, not developing them) > > would be best. The "Review Cadence" section could also be folded in > > there, to give a full view of what a submitter can expect. > > > > This would also have the advantage of introducing the linuvtv.org > > patchwork instance, which you reference above. Documents are more > > readable when they introduce concepts first before using them. > > I think that's a good idea. > > >> +Failures during e-mail submission > >> ++++++++++++++++++++++++++++++++++ > >> > >> Media's workflow is heavily based on Patchwork, meaning that, once a patch > >> is submitted, the e-mail will first be accepted by the mailing list > >> @@ -47,51 +156,36 @@ server, and, after a while, it should appear at: > >> > >> - https://patchwork.linuxtv.org/project/linux-media/list/ > >> > >> -If it doesn't automatically appear there after a few minutes, then > >> +If it doesn't automatically appear there after some time [2]_, then > >> probably something went wrong on your submission. Please check if the > >> -email is in plain text\ [2]_ only and if your emailer is not mangling > >> +email is in plain text\ [3]_ only and if your emailer is not mangling > >> whitespaces before complaining or submitting them again. > >> > >> -You can check if the mailing list server accepted your patch, by looking at: > >> +To troubleshoot problems, you should first check if the mailing list > >> +server has accepted your patch, by looking at: > >> > >> - https://lore.kernel.org/linux-media/ > >> > >> -.. [2] If your email contains HTML, the mailing list server will simply > >> +If the patch is there and not at patchwork, it is likely that your e-mailer > >> +mangled the patch. Patchwork internally has a logic that checks if the > >> +received e-mail contain a valid patch. Any whitespace and new line > >> +breakages mangling the patch won't be recognized by patchwork, thus such > >> +patch will be rejected. > >> + > >> +.. [2] It usually takes a few minutes for the patch to arrive, but > >> + the e-mail server is busy, so it may take up to a few hours > > > > s/is busy/may be busy/ > > > >> + for a patch to be handled by the mail server and by the patchwork > >> + instance. > > > > "for a patch to be recorded by patchwork." > > > >> + > >> +.. [3] If your email contains HTML, the mailing list server will simply > >> drop it, without any further notice. > > > > These changes too could have been split to a separate cleanup patch, to > > make the crux of this patch easier to review. > > > >> > >> +Subsystem maintainers > >> +--------------------- > >> > >> -Media maintainers > >> -+++++++++++++++++ > >> - > >> -At the media subsystem, we have a group of senior developers that > >> -are responsible for doing the code reviews at the drivers (also known as > >> -sub-maintainers), and another senior developer responsible for the > >> -subsystem as a whole. For core changes, whenever possible, multiple > >> -media maintainers do the review. > >> - > >> -The media maintainers that work on specific areas of the subsystem are: > >> - > >> -- Remote Controllers (infrared): > >> - Sean Young <sean@mess.org> > >> - > >> -- HDMI CEC: > >> - Hans Verkuil <hverkuil@xs4all.nl> > >> - > >> -- Media controller drivers: > >> - Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> - > >> -- ISP, v4l2-async, v4l2-fwnode, v4l2-flash-led-class and Sensor drivers: > >> - Sakari Ailus <sakari.ailus@linux.intel.com> > >> - > >> -- V4L2 drivers and core V4L2 frameworks: > >> - Hans Verkuil <hverkuil@xs4all.nl> > > > > We're losing that information, isn't it valuable ? > > That's a good point. I think we still want to keep this information. Although > this list should probably be moved to... As replied to Mauro, I'm also fine with this information being conveyed through MAINTAINERS. > >> - > >> -The subsystem maintainer is: > >> - Mauro Carvalho Chehab <mchehab@kernel.org> > >> - > >> -Media maintainers may delegate a patch to other media maintainers as needed. > >> -On such case, checkpatch's ``delegate`` field indicates who's currently > >> -responsible for reviewing a patch. > >> +The subsystem maintainers are: > >> + Mauro Carvalho Chehab <mchehab@kernel.org> and > >> + Hans Verkuil <hverkuil@xs4all.nl> > > ...here. > > > > > If you don't intend on this being rendered as a list, it should be > > > > The subsystem maintainers are Mauro Carvalho Chehab <mchehab@kernel.org> and > > Hans Verkuil <hverkuil@xs4all.nl>. > > > > Otherwise, > > > > The subsystem maintainers are: > > > > - Mauro Carvalho Chehab <mchehab@kernel.org> > > - Hans Verkuil <hverkuil@xs4all.nl> > > > > > > Shouldn't MAINTAINERS be updated at the same time ? > > Good point, I posted a patch. > > >> > >> Submit Checklist Addendum > >> ------------------------- > >> @@ -108,17 +202,14 @@ implementing the media APIs: > >> ==================== ======================================================= > >> Type Tool > >> ==================== ======================================================= > >> -V4L2 drivers\ [3]_ ``v4l2-compliance`` > >> +V4L2 drivers\ [4]_ ``v4l2-compliance`` > >> V4L2 virtual drivers ``contrib/test/test-media`` > >> CEC drivers ``cec-compliance`` > >> ==================== ======================================================= > >> > >> -.. [3] The ``v4l2-compliance`` also covers the media controller usage inside > >> +.. [4] The ``v4l2-compliance`` also covers the media controller usage inside > >> V4L2 drivers. > >> > >> -Other compilance tools are under development to check other parts of the > >> -subsystem. > >> - > >> Those tests need to pass before the patches go upstream. > >> > >> Also, please notice that we build the Kernel with:: > >> @@ -134,6 +225,8 @@ Where the check script is:: > >> Be sure to not introduce new warnings on your patches without a > >> very good reason. > >> > >> +Please see `Media development workflow`_ for e-mail submission rules. > >> + > > > > These hunks too could have been split to a cleanup patch before the main > > changes. > > > >> Style Cleanup Patches > >> +++++++++++++++++++++ > >> > >> diff --git a/Documentation/driver-api/media/media-committer.rst b/Documentation/driver-api/media/media-committer.rst > >> new file mode 100644 > >> index 000000000000..27b85a37fb2b > >> --- /dev/null > >> +++ b/Documentation/driver-api/media/media-committer.rst > >> @@ -0,0 +1,252 @@ > >> +Media committers > >> +================ > >> + > >> +What is a media committer? > >> +-------------------------- > >> + > >> +A media committer is a developer who can apply patches from other developers > > > > Here too we could standardize to "push" instead of "apply". > > > >> +and their own patches at the > > > > s/at the/to the/ > > > > I would simply write "who can push commits to the ...". > > > >> +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_ > >> +tree. > >> + > >> +It is a media committer's duty to ensure that their entries at the MAINTAINERS > > > > s/at the/in the/ > > > >> +file will be kept updated, and that submitted patches for files for which > > > > s/will be kept update/are kept up-to-date/ > > > >> +they are listed as maintainers are timely reviewed at the mailing list, > > > > s/at the/on the/ > > > >> +not waiting in patchwork as ``New`` for more than one Kernel merge cycle, > >> +and, if accepted, applying them at the media committer's tree. > > > > I think this goes a bit backward, and mixes things up a bit. On the > > mixing side, the expectation of timely reviews comes from maintainer > > status. Having commit rights is orthogonal to that. > > > > The goal of direct commit access is to speed up maintenance, to get > > patches reviewed and merged quicker. Are we saying here that if someone > > has commit rights they will lose them if they take too long to review > > code ? That would then slow down maintenance even more, which seems > > counterproductive. > > Someone with commit rights is also a maintainer, since that's how you > gain the trust to get those rights. If you do a poor job of reviewing > patches relevant for you as maintainer, then you loose that trust. This is I think the point where our expectations are the least aligned. I'm considering "committer" based on what is done in drm-misc. A committer is essentially a developer who has demonstrated they can follow a documented process to push their own patches. They are given push access as a shortcut, which frees time for the subsystem maintainers who don't have to pick patches manually from the list (or handle pull requests). That's the official side of it. The barrier to entry is intentionally kept very low to ensure that committers won't decide to use the legacy workflow due to expectations of additional work load. Committers are not required or even asked to take any extra work. It's still a win-win situation: subsystem maintainers have less work, and committers can get their patches upstream more easily. Then there's the other "secret" goal: through handing out committer rights, the maintainers hoped that a subset of the committers would become more involved, grow more knowledge about the subsystem, pick up third party patches, review or cross-review code, ... And that worked, DRM has grown an active community of developers who go beyond their personal needs and help with maintenance more broadly. Dave and Sima deliberately decided to favour the carrot over the stick, and I think the events that followed proved it was the right decision. This is what I would like to see replicated in the media subsystem. Even if a committer only handles the single driver they're interested in and push their own patches, it's still a win for everybody involved. By making the barrier to entry low, we will make it possible for people who would have been scared of volunteering to become part of the community, and over time handle more responsibilities. Setting a higher barrier to entry will scare those people away. Even myself, if I'm expected to do more than what I do today to get commit rights, I won't request them. Everybody will lose, I will have to keep sending pull requests, and you will have to keep handling them. Both of us will lose time that we could otherwise use for reviews or other tasks beneficial to the subsystem. More importantly than the exact wording, it's the core principle of the committers model that we need to agree on. If we don't have the same expectations it will clearly not work. > And if you simply don't have the time anymore for that, then perhaps > you need to look for a co-maintainer or just stop being a maintainer for > that area. A good example of that is actually the uvc driver. In that case > the solution was adding a co-maintainer. > > > Also, while one can be recognized as a maintainer for multiple drivers > > or parts of the kernel, there's a single committer status. You can't > > revoke committer status of a particular driver only. > > The committer status is a sign that you are trusted. Part of that is timely > patch review. Or admit that you won't have the time/resources to do that job > and look for a co-maintainer or even give up maintainership of some areas. > > That said, perhaps the text should change a bit: > > "not waiting in patchwork as ``New`` for more than one Kernel merge cycle" -> > "ideally not waiting in patchwork as ``New`` for more than one Kernel merge cycle" > > We all have patches in patchwork that are much older than that, for one reason > or another, but if this happens all the time, then you have a problem. > > >> + > >> +This privilege is granted with some expectation of responsibility: > > > > "Privilege" sounds a bit like lord and serf. > > How about 'These commit rights are granted'? I recommended "rights" to Mauro as well, sounds good. > >> +committers are people who care about the Linux Kernel as a whole and > >> +about the Linux media subsystem and want to help its development. It > >> +is also based on a trust relationship between the rest of the committers, > >> +maintainers and the LinuxTV community. > > > > Who is "the LinuxTV community" ? > > "linux kernel media community"? Also discussed in the other part of the thread. > >> + > >> +As such, a media committer is not just someone who is capable of creating > >> +code, but someone who has demonstrated their ability to collaborate > >> +with the team, get the most knowledgeable people to review code, > >> +contribute high-quality code, and follow through to fix issues (in code > >> +or tests). > >> + > >> +.. Note:: > >> + > >> + 1. If a patch introduced a regression, then it is the media committer's > > > > s/introduced/introduces/ > > > >> + responsibility to correct that as soon as possible. Typically the > >> + patch is either reverted, or an additional patch is committed that > >> + fixes the regression; > >> + 2. if patches are fixing bugs against already released Kernels, including > >> + the reverts above mentioned, the media committer shall add the needed > >> + tags. Please see :ref:`Media development workflow` for more details. > > > > s/ Please/ Please/ > > > >> + 3. all patches should be properly reviewed at > >> + linux-media@vger.kernel.org before being merged at the > > > > s/at the/in the/ > > > >> + media-committers tree or submitted on pull requests. > > > > This is a fundamental rule that belongs to the development workflow in > > my opinion. > > > >> + > >> +Becoming a media committer > >> +-------------------------- > >> + > >> +The most important aspect of volunteering to be a committer is that you will > >> +be able to review and approve other people's changes, so we are looking for > > > > Everybody is able to review patches (rather, everybody is allowed to > > review patches, the ability is a different matter). > > > >> +whether we think you will be good at doing that. > > > > I've been told that "whether" should also come with a "or" clause. You > > can write "whether or not we think ...". > > How about this: > > "The most important aspect of volunteering to be a committer is that you have > demonstrated the ability to give good code reviews." Grammatically speaking that sounds correct, but I don't think it's the most important part :-) It depends on how we define the committers model, as discussed above. > >> + > >> +As such, potential committers must earn enough credibility and trust from the > >> +LinuxTV community. To do that, developers shall be familiar with the open > > We should probably search for "LinuxTV community" and replace it with "linux kernel > media community". "LinuxTV" is fairly meaningless to me, it's just the name of our > internet domain, but otherwise it doesn't feel like a community. I think it would indeed make sense to focus on the subsystem name in the kernel documentation. > An alternative name might be "media developers"? > > >> +source model and have been active at the Linux Kernel community for some time, > > > > s/at the/in the/ > > > >> +and, in particular, with the media subsystem. > > > > s/with the/in the/ > > > >> + > >> +So, in addition to actually making the code changes, you are basically > >> +demonstrating your: > >> + > >> +- commitment to the project; > >> +- ability to collaborate with the team and communicate well; > >> +- understand of how upstream and the LinuxTV community works > > > > s/ works/ work/ > > > >> + (policies, processes for testing, code review, ...) > >> +- reasonable knowledge about: > >> + > >> + - the Kernel development process: > >> + Documentation/process/index.rst > >> + > >> + - the Media development profile: > >> + Documentation/driver-api/media/maintainer-entry-profile.rst > >> + > >> +- understanding of the projects' code base and coding style; > >> +- ability to provide feedback to the patch authors; > >> +- ability to judge when a patch might be ready for review and to submit; > >> +- ability to write good code (last but certainly not least). > >> + > >> +It is also desirable that developers that intend to become committers > >> +make a best effort to attend the yearly Linux Media Summit, typically > >> +co-located with another Linux conference. > > > > I would say that "are encouraged to attend" instead of "make a best > > effort to attend". Also, how will this scale when we'll have a few > > dozen committers ? Typically the media summit is capped to 20 attendees > > or less. > > If we have that many committers, then we can afford a larger room and we > probably would have to start charging some contributions as well. But > that would be a luxury problem :-) It's a bridge we can cross when we > get there. Also tied to how we define committers, and discussed in the other part of the thread. > Note that I am fine with "are encouraged to attend". I think that's a good > phrase. > > >> + > >> +If you are doing such tasks and have become a valued developer, an > >> +existing committer can nominate you to the media subsystem maintainers. > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: > > > > "Maintainers and committers should encourage contributors to request > > commit rights, especially junior contributors tend to underestimate > > their skills." > > In drm is it the contributors that request commit rights? Or is it those > who already have commit rights that invite others? Currently the plan for > the media subsystem is the second method. Although that might change in the > future, of course. The process is documented in https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request. It requires explicit action from the candidate, as they have to create a gitlab.fdo account, and request commit access by fiing an issue in gitlab. You can see the issue template at https://gitlab.freedesktop.org/drm/misc/kernel/-/issues/new?issue[title]=Request%20for%20Commit%20Rights&issuable_template=commit_access, which is roughly speaking the equivalent of the mail template in this document. In practice, as mentioned in the documentation, people often underestimate their skills and lack confidence to ask for committer access. That's why the documentation states that maintainers and committers should encourage contributors to request access. I like that model because it requires an explicit action from the contributor to show that they have an interest, and it also makes it possible for people to request access without having been nominated. It doesn't mean that access will be automatically granted, there are still acceptance criteria, and it's a maintainer decision at the end of the day. Stating as done in this patch that an existing committer can nominate someone implies that contributors have to wait until they get notified they can join The Chosen Few. It's not very welcoming, and given how busy everybody is, valuable contributors may need to wait for longer than they should before someone thinks about nominating them. I wouldn't expect a change of wording to result in any practical change in the process, it is only about being more inclusive and welcoming in the document. If we want to create a vibrant community, people should feel not just welcome, but also desired and valued. > So since existing committers invite others, I'm not sure this is needed. > > >> + > >> +The ultimate responsibility for accepting a nominated committer is up to > >> +the subsystem's maintainers. The committers must earn a trust relationship > >> +with all subsystem maintainers, as, by granting you commit rights, they will > >> +be delegating part of their maintenance tasks. > >> + > >> +Due to that, to become a committer or a core committer, a consensus between > >> +all subsystem maintainers is required, as they all need to trust a developer > >> +well enough to be delegated the responsibility to maintain part of the code > >> +and to properly review patches from third parties, in a timely manner and > >> +keeping the status of the reviewed code at https://patchwork.linuxtv.org > >> +updated. > >> + > >> +.. Note:: > >> + > >> + In order to preserve the developers that could have their commit rights > > > > Do you mean "protect" instead of "preserve" ? Or maybe "preserve/protect > > the privacy of" ? > > That last one is a good phrase, I like it. > > > s/that/who/ > > s/their // (in the granted case, they don't have commit rights yet) > > > >> + granted or denied as well as the subsystem maintainers who have the > > > > "granted, denied or removed" ? > > Yes. > > >> + task to accept or deny commit rights, all communication related to > > Same here. > > >> + nominating a committer, preserving commit rights or leaving such function > >> + should happen in private as much as possible. > >> + > >> +Media committer's agreement > >> +--------------------------- > >> + > >> +Once a nominated committer is accepted by all subsystem maintainers, > >> +they will ask if the developer is interested in the nomination and discuss > >> +what area(s) of the media subsystem the committer will be responsible for. > >> + > >> +Once the developer accepts being a committer, the new committer shall > >> +explicitly accept the Kernel development policies described under its > >> +Documentation/, and, in particular to the rules on this document, by writing > >> +an e-mail to media-committers@linuxtv.org, with a declaration of intent > >> +following the model below:: > >> + > >> + I, John Doe, would like to change my status to: Committer > >> + > >> + I intend to actively develop the XYZ driver, send fixes to drivers > >> + that I can test, reviewing patches and merging trivial fixes > >> + for the subsystem, ... > > > > "Merging trivial fixes for the subsystem" bothers me. I don't think it > > needs to be a requirement for committers. This is a maintainer's > > responsibility. If people want to help with that that's great, but > > making it a requirement isn't. Or did you mean this as an example ? > > > > We shouldn't expect committers to handle a higher workload than what > > they do as driver maintainers who submit patches by e-mail or send pull > > requests. Giving commit rights will lower the effort to get patches in, > > and I think it's fair to ask for keeping patchwork up-to-date in return, > > but that's about it. > > The idea was to make it explicit that they can review and merge trivial > fixes for the subsystem as a whole (so outside the direct area that they > maintain), but that is certainly optional. > > How about: > > ", and optionally reviewing patches and merging trivial fixes in other > areas of the subsystem," > > Still a bit clunky, though. Suggestions are welcome. This is also discussed in the other part of the mail thread. I think it's now clear that this particular wording was just an example, stating so explicitly would address my concern. > >> + > >> +Followed by a formal declaration of agreement with the Kernel development > >> +rules, signed with a PGP key cross signed by other Kernel and media > > > > s/PGP/GPG/ (same thing in practice, but let's advocate for free > > software) > > > >> +developers. Such declaration shall be:: > > > > I find the GPG signature requirement to be borderline ridiculous. The > > first message you're giving to committers is that you distrust them so > > much that you want them to sign an agreement with their blood > > (figuratively speaking). I don't think it's a very good approach to > > community building, nor does it bring any advantage to anyone. > > I kind of agree with Laurent here. Is the media-committers mailinglist > publicly archived somewhere? I think it is sufficient if this is posted > to a publicly archived mailinglist. That could be linux-media, I would be > fine with that. But media-committers would be more appropriate, but only > if it is archived somewhere. > > If we want a GPG key, what would we do with it anyway? > > >> + > >> + I hereby declare that I agree with the Kernel development rules described at: > >> + > >> + https://www.kernel.org/doc/html/latest/driver-api/media/media-committer.rst > >> + > >> + and to the Linux Kernel development process rules. > >> + > >> + I agree to the Code of Conduct as documented here: > > > > s/here:/in/ > > > >> + Documentation/process/code-of-conduct.rst > > > > You use an http link for meda-commiter.rst and a file path for > > code-of-conduct.rst. RST links for both would be more consistent, but if > > the goal is to paste the text in an e-mail, you can also use http links > > or file paths for both. > > > >> + > >> + I am aware that I can, at any point of time, retire. In that case, I will > >> + send an e-mail to notify the subsystem maintainers for them to revoke my > >> + commit rights. > >> + > >> + I am aware that the Kernel development rules change over time. > >> + By doing a new commit, I understand that I agree with the rules in effect > > > > What does "doing a new commit" mean here ? Sending a patch to the list ? > > Pushing it to the shared tree ? I assume the latter given the text > > below, it could be clarified here. > > > >> + at the time of the commit. > >> + > >> +For more details about PGP sign, please read > >> +Documentation/process/maintainer-pgp-guide.rst and > >> +:ref:`kernel_org_trust_repository`. > >> + > >> +In case the kernel development process changes, by merging new commits at the > > Should this be "kernel media development"? > > > > > s/at the/in the/ > > > >> +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_, > > > > s/media-committers/media-committers tree/ > > > >> +the media committer implicitly declares that the agreement with the latest > > > > s/that the/their/ > > > >> +version of the documented process and to the contents of this file. > > > > s/to the contents // ("latest version" applies to both "the documented > > process" and "this file") > > > > This is problematic, as we can't expect people to check for changes in > > this file every time they push something. Changes to this file should be > > announced to all committers, with a reasonable review period. > > "Any changes to the kernel media development process will be announced in > the media-committers mailinglist with a reasonable review period. All > committers are automatically subscribed to that mailinglist." Good improvement. Automatic subscription to the mailing list for announcements is a good idea. We should try to keep the list relatively low volume then, it shouldn't be used to discuss technical issues for instance, but only to manage the process. I've added more comments in the other part of the mail thread, let's discuss it there. > >> + > >> +Core committers > >> +--------------- > >> + > >> +As described in Documentation/driver-api/media/maintainer-entry-profile.rst > > > > RST link here too. > > > >> +a committer may be granted with additional privileges to also be able to > > > > s/privileges/rights/ > > > > (same below) > > > >> +change a core file and/or media subsystem's Kernel API/ABI. The extent of > > > > Drop "/ABI" as above. > > > >> +the core committer's additional privileges will be detailed by the subsystem > >> +maintainers when they nominate a core committer. > >> + > >> +Existing committers may become core committers and vice versa. Such > >> +decisions will be taken in consensus between the subsystem maintainers. > >> + > >> +Media committers rules > >> +---------------------- > >> + > >> +Media committers shall ensure that merged patches will not break any existing > >> +drivers. If it breaks, fixup or revert patches shall be merged as soon as > > > > How do they ensure that ? I would prefer mentioning a best effort here, > > as it's practically not possible to ensure this (until we have CI > > covering all the drivers in the subsystem, which is not a reasonable > > target). > > Good point. > > >> +possible, aiming to be merged at the same Kernel cycle the bug is reported. > >> + > >> +Media committers shall behave accordingly to the permissions granted by > > > > s/permissions/rights/ (we never use "permission" anywhere else) > > > >> +the subsystem maintainers, specially with regards of the scope of changes > >> +they may apply directly at the media-committers tree. Such scope can > >> +change overtime on a mutual greement between media committers and > > > > s/overtime/over time/ > > s/greement/agreement/ > > > >> +maintainers. > >> + > >> +As described at :ref:`Media development workflow`, there are workflows. > > > > s/at /in / > > > >> +For the committers' workflow, the following rules apply: > >> + > >> +- Each merged patch shall pass CI tests; > >> + > >> +- Media committers shall request reviews from other committers were > > > > We shouldn't limit this to "other committers". When making changes to a > > driver that is not maintained by any committer (for instance when making > > tree-wide changes), the person in the best position to review the > > changes is the driver maintainer or author. I would extend this to "from > > third parties", or "from other developers". > > Yes, 'developers' is better. > > > > >> + applicable, i.e. because those committers have more knowledge about > > Same here. > > >> + some areas that are changed by a patch; > >> + > >> +- No other media committer would be against the proposed changes. > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/committer-drm-misc.html#merge-criteria: > > > > "There must not be open issues or unresolved or conflicting feedback > > from anyone. Clear them up first. Defer to maintainers as needed." > > > > That's better than restricting the conflicts to committers. If there are > > open issues, the patches should not be merged through the shared tree. > > Nice, good wordsmithing! I'll pass the praise to the drm-misc architects :-) > >> + > >> +Patches that do not fall under the committer's workflow criteria will follow > >> +the normal workflow as described at :ref:`Media development workflow`. > >> + > >> +Only a subsystem maintainer can override such rules. > >> + > >> +All media committers shall ensure that patchwork will reflect the current > >> +status, e.g. patches shall be delegated to the media committer who is > >> +handling them and the patch status shall be updated according to these rules: > >> + > >> +- ``Under review``: Used if the patch requires a second opinion > >> + or when it is part of a pull request; > >> +- ``Accepted``: Once a patch is merged at the multi-committer tree. > > > > s/at the/in the/ > > > >> +- ``Superseded``: There is a newer version of the patch posted in the > > > > s/in the/to the/ > > > >> + mailing list. > >> +- ``Duplicated``: There was another patch doing the same thing from someone > >> + else that was accepted. > >> +- ``Not Applicable``: Use for patch series that are not merged at media.git > >> + tree (e.g. drm, dmabuf, upstream merge, etc.) but were cross-posted to the > >> + linux-media mailing list. > >> + > >> +If the committer decides not to merge it, then reply by email to patch > >> +authors, explaining why it is not merged, and patchwork shall be updated > >> +accordingly with either: > >> + > >> +- ``Changes Requested``: if a new revision was requested; > >> +- ``Rejected``: if the proposed change won't be merged upstream. > > > > There are tools to ease updating the status of a patch, could you > > document or at least mention them ? > > I think that is out-of-scope. It certainly could be added as a follow-up > patch. It doesn't necessarily have to be done here, but if we expect committers to keep patchwork up-to-date (and I'm fine with that expectation), then I think we need to point every new committer to resources that will help them with that task. I'd like for instance to create a macro in my mail client (mutt) to update the status of a series, which would require using a patchwork client that can locate patchwork entries based on msgid. I expect something to exist, and it would be nice if I didn't have to do the research (unless I'm the first one to want this of course). > >> + > >> +If a media committer decides to retire, it is the committer's duty to > >> +notify the subsystem maintainers about that decision. > >> + > >> +Maintaining media committer status > >> +---------------------------------- > >> + > >> +A community of committers working together to move the Linux Kernel > >> +forward is essential to creating successful projects that are rewarding > >> +to work on. If there are problems or disagreements within the community, > >> +they can usually be solved through healthy discussion and debate. > >> + > >> +In the unhappy event that a media committer continues to disregard good > >> +citizenship (or actively disrupts the project), we may need to revoke > > > > That's very, very vague, surprisingly vague even from someone who raised > > many concerns about the kernel code of conduct being vague. > > I suspect that this phrasing is copied from another project. Mauro, can you > confirm that? > > I think it is extremely difficult to give explicit guidance here. > > >> +that person's status. In such cases, if someone suggests the revocation with > >> +a good reason, other developers may second the motion. The final decision > >> +is taken by the subsystem maintainers. As the decision to become a media > > > > What does "seconding the motion" bring, if the decision lies solely in > > maintainers ? > > I think the intent here is that, other than in extreme circumstances, it shouldn't > be a unilateral decision from the subsystem maintainers. Multiple media committers > should agree with it. > > But perhaps it would be better to replace this with: > > "In such cases, if someone suggests the revocation with a good reason, then after > discussing this among the media committers, the final decision is taken by the > subsystem maintainers." Something like that sounds better. I've also mentioned other options for improvements in the other part of the mail thread. > I really hope we will never end up in a situation like this, since that's going > to be painful regardless of what procedure you choose. I'm quite confident we won't. Nevertheless, it's a good idea to be prepared and establish rules already. > >> +committer comes from a consensus between subsystem maintainers, a single > >> +subsystem maintainer not trusting the media committer anymore is enough to > >> +revoke committer's privileges. > >> + > >> +If a committer is inactive for more than a couple of Kernel cycles, > >> +maintainers will try to reach you via e-mail. If not possible, they may > >> +revoke your committer privileges and update MAINTAINERS file entries > >> +accordingly. If you wish to resume contributing later on, then contact > >> +the subsystem maintainers to ask if your rights can be restored. > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: > > > > "Committers are encouraged to request their commit rights get removed > > when they no longer contribute to the project. Commit rights may be > > automatically revoked after a year of inactivity (no commits or > > reviews). Commit rights will be reinstated when they come back to the > > project." > > Yes, that's better. I also realized that the mention of updating the MAINTAINERS > makes no sense since that does not contain media committer status. Mauro preferred the original version, so I'll let the two of you debate this :-) > >> + > >> +A previous committer that had his commit rights revoked can keep contributing > > > > s/his/their/ > > > >> +to the subsystem via the normal e-mail workflow as documented at the > >> +:ref:`Media development workflow`. > >> + > >> +References > >> +---------- > >> + > >> +Much of this was inspired by/copied from the committer policies of: > >> + > >> +- `Chromium <https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md>`_; > >> +- `WebKit <http://www.google.com/url?q=http%3A%2F%2Fwebkit.org%2Fcoding%2Fcommit-review-policy.html&sa=D&sntz=1&usg=AFrqEze4W4Lvbhue4Bywqgbv-N5J66kQgA>`_; > > > > Google tracks us enough without using google URLs. > > > >> +- `Mozilla <http://www.google.com/url?q=http%3A%2F%2Fwww.mozilla.org%2Fhacking%2Fcommitter%2F&sa=D&sntz=1&usg=AFrqEzecK7iiXqV30jKibNmmMtzHwtYRTg>`_. > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html > > would also have been a good source of inspiration. That's the only large > > multi-committer workflow today in the kernel, and it has proven its > > value. The explicit acceptance criteria in particular are very good. > > Quoting the document, it says > > > > "Commit rights will be granted to anyone who requests them and fulfills > > the below criteria:" > > > > That's how we build an inclusive community, it feels way more welcoming > > than saying that maintainers will discuss in private and grant > > privileges to underlings if it pleases them (even if the effect is the > > same in practice, it's still a maintainer decision). > > The main difference here is that in drm developers can ask for commit rights, > whereas for the media subsystem they are invited by existing media committers. > > The drm model is absolutely more inclusive, and I hope we can end up there > eventually. But for now I think we need more work on both the procedures and > the media-ci workflow. As mentioned on the other part of the mail thread, I think it's absolutely fine to start small and grow at a pace we are comfortable with. I also think it's important for this document to be inclusive from the very beginning in its wording to give the image of a welcoming community. It's not about any fundamental change to the rules, I think we mostly agree on them (with the open question of a committer's duties, but that's orthogonal to the process of becoming a committer). > Even with just two sub-maintainers committing patches it took quite a long time > to find and fix all the bugs/issues we encountered. At this point we are > definitely not ready to implement a drm model. > > This document just starts this process, it will change and be improved over time, > but we need this in place before we can start adding more committers. I'm quite confident we can reach a consensus in a reasonable amount of time. Not in time for -rc1 (I haven't seen feedback from Sean, Sakari, Sebastian or Ricardo yet, and I'd like to know their opinions), but possibly still in time to grant commit rights before the next linux-next freeze period. > >> + > >> diff --git a/Documentation/process/maintainer-pgp-guide.rst b/Documentation/process/maintainer-pgp-guide.rst > >> index f5277993b195..795ef8d89271 100644 > >> --- a/Documentation/process/maintainer-pgp-guide.rst > >> +++ b/Documentation/process/maintainer-pgp-guide.rst > >> @@ -903,6 +903,8 @@ the new default in GnuPG v2). To set it, add (or modify) the > >> > >> trust-model tofu+pgp > >> > >> +.. _kernel_org_trust_repository: > >> + > >> Using the kernel.org web of trust repository > >> -------------------------------------------- > >>
On Wed, Nov 27, 2024 at 12:54:15PM +0100, Mauro Carvalho Chehab wrote: > Em Wed, 27 Nov 2024 10:39:48 +0100 Mauro Carvalho Chehab escreveu: > > > > This workflow doesn't apply to patch submitters who are not allowed to > > > send pull requests and who don't have direct commit access. I thought > > > these submitters are the main audience of this document. In that case, I > > > think moving the next section that explains the e-mail workflow before > > > the "Media development workflow" section (which should likely be renamed > > > to make it clear that it is about merging patches, not developing them) > > > would be best. The "Review Cadence" section could also be folded in > > > there, to give a full view of what a submitter can expect. > > > > > > This would also have the advantage of introducing the linuvtv.org > > > patchwork instance, which you reference above. Documents are more > > > readable when they introduce concepts first before using them. > > > > Will try to do such change at v2. > > Actually, both workflows (a) and (b) apply to the ones that can't > send pull requests or push at media-committers.git: > > --- > > a. Normal workflow: patches are handled by subsystem maintainers:: > > +------+ +---------+ +-------+ +-----------------------+ +---------+ > |e-mail|-->|patchwork|-->|pull |-->|maintainers merge |-->|media.git| > +------+ +---------+ |request| |in media-committers.git| +---------+ > +-------+ +-----------------------+ > > For this workflow, pull requests can be generated by a committer, > a previous committer, subsystem maintainers or by a couple of trusted > long-time contributors. If you are not in such group, please don't submit > pull requests, as they will likely be ignored. > > b. Committers' workflow: patches are handled by media committers:: > > +------+ +---------+ +--------------------+ +-----------+ +---------+ > |e-mail|-->|patchwork|-->|committers merge at |-->|maintainers|-->|media.git| > +------+ +---------+ |media-committers.git| |approval | +---------+ > +--------------------+ +-----------+ > > --- > > No matter who sent an e-mail, this will be picked by patchwork and either > be part of a PR or a MR, depending on who picked it. Today the "normal" workflow for contributors who don't send pull requests is that you or Hans will pick their patches from the list. That's why I mentioned that neither of the above workflows apply there. Now, if we consider that you and Hans will keep doing that for some patches, and merge them using the committers workflow (where you would handle both steps of merging in the shared tree and giving the maintainer approval), it's true that the normal workflow would be one of the two above. Looking at the pull requests sent to the list over the past twelve months, we have 32 Sakari Ailus 24 Hans Verkuil 22 Laurent Pinchart 21 Sebastian Fricke 7 Sean Young 7 Hans de Goede 4 Stanimir Varbanov 1 Shuah Khan I expect people in that list to get commit rights either from the very beginning or very soon after. The committer workflow (if we consider it as including how you and Hans will continue picking patches from the list) will be the new norm. how about flipping things and listing it as a), and then name b) the "Pull request workflow" instead of the "Normal workflow" ? I would even go as far as proposing documenting the pull request workflow as legacy.
Jumping in the middle here with some clarifications. On Wed, 27 Nov 2024 at 12:19, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wed, Nov 27, 2024 at 10:39:48AM +0100, Mauro Carvalho Chehab wrote: > > It is somewhat similar to drm-intel and drm-xe, where reviews are part > > of the acceptance criteria to become committers. > > Those are corporate trees, so it's easier to set such rules. Imo it's the other way round, because it's corporate you need stricter rules and spell them all out clearly - managers just love to apply pressure on their engineers too much otherwise "because it's our own tree". Totally forgetting that it's still part of the overall upstream, and that they don't own upstream. So that's why the corporate trees are stricter than drm-misc, but the goals are still exactly the same: - peer review is required in a tit-for-tat market, but not more. - committers push their own stuff, that's all. Senior committers often also push other people's work, like for smaller work they just reviewed or of people they mentor, but it's not required at all. - maintainership duties, like sending around pr, making sure patches dont get lost and things like that, is separate from commit rights. In my opinion, if you tie commit rights to maintainership you're doing something else than drm and I'd more call it a group maintainership model, not a commit rights model for landing patches. Anyway just figured I'll clarify what we do over at drm. I haven't looked at all the details of this proposal here and the already lengthy discussion, plus it's really not on me to chime in since I'm not involved. Cheers, Sima
Em Wed, 27 Nov 2024 15:39:38 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > On Wed, Nov 27, 2024 at 12:54:15PM +0100, Mauro Carvalho Chehab wrote: > > Em Wed, 27 Nov 2024 10:39:48 +0100 Mauro Carvalho Chehab escreveu: > > > > > > This workflow doesn't apply to patch submitters who are not allowed to > > > > send pull requests and who don't have direct commit access. I thought > > > > these submitters are the main audience of this document. In that case, I > > > > think moving the next section that explains the e-mail workflow before > > > > the "Media development workflow" section (which should likely be renamed > > > > to make it clear that it is about merging patches, not developing them) > > > > would be best. The "Review Cadence" section could also be folded in > > > > there, to give a full view of what a submitter can expect. > > > > > > > > This would also have the advantage of introducing the linuvtv.org > > > > patchwork instance, which you reference above. Documents are more > > > > readable when they introduce concepts first before using them. > > > > > > Will try to do such change at v2. > > > > Actually, both workflows (a) and (b) apply to the ones that can't > > send pull requests or push at media-committers.git: > > > > --- > > > > a. Normal workflow: patches are handled by subsystem maintainers:: > > > > +------+ +---------+ +-------+ +-----------------------+ +---------+ > > |e-mail|-->|patchwork|-->|pull |-->|maintainers merge |-->|media.git| > > +------+ +---------+ |request| |in media-committers.git| +---------+ > > +-------+ +-----------------------+ > > > > For this workflow, pull requests can be generated by a committer, > > a previous committer, subsystem maintainers or by a couple of trusted > > long-time contributors. If you are not in such group, please don't submit > > pull requests, as they will likely be ignored. > > > > b. Committers' workflow: patches are handled by media committers:: > > > > +------+ +---------+ +--------------------+ +-----------+ +---------+ > > |e-mail|-->|patchwork|-->|committers merge at |-->|maintainers|-->|media.git| > > +------+ +---------+ |media-committers.git| |approval | +---------+ > > +--------------------+ +-----------+ > > > > --- > > > > No matter who sent an e-mail, this will be picked by patchwork and either > > be part of a PR or a MR, depending on who picked it. > > Today the "normal" workflow for contributors who don't send pull > requests is that you or Hans will pick their patches from the list. True, but we've been following process (b) since the last merge window: we are generating merges at the media-committers.git. As we're maintainers, the "maintainers approval" step is also handled by us, by the one that submitted the MR, after checking the media-ci results. > That's why I mentioned that neither of the above workflows apply there. > Now, if we consider that you and Hans will keep doing that for some > patches, and merge them using the committers workflow (where you would > handle both steps of merging in the shared tree and giving the > maintainer approval), it's true that the normal workflow would be one of > the two above. Yes, that's the case. > Looking at the pull requests sent to the list over the past twelve > months, we have > > 32 Sakari Ailus > 24 Hans Verkuil > 22 Laurent Pinchart > 21 Sebastian Fricke > 7 Sean Young > 7 Hans de Goede > 4 Stanimir Varbanov > 1 Shuah Khan > > I expect people in that list to get commit rights either from the very > beginning or very soon after. The committer workflow (if we consider it > as including how you and Hans will continue picking patches from the > list) will be the new norm. how about flipping things and listing it as > a), and then name b) the "Pull request workflow" instead of the "Normal > workflow" ? I would even go as far as proposing documenting the pull > request workflow as legacy. Renaming from Normal work flow to Pull request workflow makes sense. The pull request workflow won't be legacy. Even with major contributors using the new workflow for "normal work", pull requests will still be generated for API changes. Regards, Mauro
On Wed, Nov 27, 2024 at 04:09:23PM +0100, Mauro Carvalho Chehab wrote: > Em Wed, 27 Nov 2024 15:39:38 +0200 Laurent Pinchart escreveu: > > On Wed, Nov 27, 2024 at 12:54:15PM +0100, Mauro Carvalho Chehab wrote: > > > Em Wed, 27 Nov 2024 10:39:48 +0100 Mauro Carvalho Chehab escreveu: > > > > > > > > This workflow doesn't apply to patch submitters who are not allowed to > > > > > send pull requests and who don't have direct commit access. I thought > > > > > these submitters are the main audience of this document. In that case, I > > > > > think moving the next section that explains the e-mail workflow before > > > > > the "Media development workflow" section (which should likely be renamed > > > > > to make it clear that it is about merging patches, not developing them) > > > > > would be best. The "Review Cadence" section could also be folded in > > > > > there, to give a full view of what a submitter can expect. > > > > > > > > > > This would also have the advantage of introducing the linuvtv.org > > > > > patchwork instance, which you reference above. Documents are more > > > > > readable when they introduce concepts first before using them. > > > > > > > > Will try to do such change at v2. > > > > > > Actually, both workflows (a) and (b) apply to the ones that can't > > > send pull requests or push at media-committers.git: > > > > > > --- > > > > > > a. Normal workflow: patches are handled by subsystem maintainers:: > > > > > > +------+ +---------+ +-------+ +-----------------------+ +---------+ > > > |e-mail|-->|patchwork|-->|pull |-->|maintainers merge |-->|media.git| > > > +------+ +---------+ |request| |in media-committers.git| +---------+ > > > +-------+ +-----------------------+ > > > > > > For this workflow, pull requests can be generated by a committer, > > > a previous committer, subsystem maintainers or by a couple of trusted > > > long-time contributors. If you are not in such group, please don't submit > > > pull requests, as they will likely be ignored. > > > > > > b. Committers' workflow: patches are handled by media committers:: > > > > > > +------+ +---------+ +--------------------+ +-----------+ +---------+ > > > |e-mail|-->|patchwork|-->|committers merge at |-->|maintainers|-->|media.git| > > > +------+ +---------+ |media-committers.git| |approval | +---------+ > > > +--------------------+ +-----------+ > > > > > > --- > > > > > > No matter who sent an e-mail, this will be picked by patchwork and either > > > be part of a PR or a MR, depending on who picked it. > > > > Today the "normal" workflow for contributors who don't send pull > > requests is that you or Hans will pick their patches from the list. > > True, but we've been following process (b) since the last merge window: we > are generating merges at the media-committers.git. As we're maintainers, > the "maintainers approval" step is also handled by us, by the one that > submitted the MR, after checking the media-ci results. > > > That's why I mentioned that neither of the above workflows apply there. > > Now, if we consider that you and Hans will keep doing that for some > > patches, and merge them using the committers workflow (where you would > > handle both steps of merging in the shared tree and giving the > > maintainer approval), it's true that the normal workflow would be one of > > the two above. > > Yes, that's the case. > > > Looking at the pull requests sent to the list over the past twelve > > months, we have > > > > 32 Sakari Ailus > > 24 Hans Verkuil > > 22 Laurent Pinchart > > 21 Sebastian Fricke > > 7 Sean Young > > 7 Hans de Goede > > 4 Stanimir Varbanov > > 1 Shuah Khan > > > > I expect people in that list to get commit rights either from the very > > beginning or very soon after. The committer workflow (if we consider it > > as including how you and Hans will continue picking patches from the > > list) will be the new norm. how about flipping things and listing it as > > a), and then name b) the "Pull request workflow" instead of the "Normal > > workflow" ? I would even go as far as proposing documenting the pull > > request workflow as legacy. > > Renaming from Normal work flow to Pull request workflow makes sense. > > The pull request workflow won't be legacy. Even with major contributors > using the new workflow for "normal work", pull requests will still be > generated for API changes. OK, let's not mark it as deprecated, we can just rename it to "Pull request workflow". I'd still prefer to list it as b) but won't make that a casus belli.
Em Wed, 27 Nov 2024 12:59:58 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > I find the GPG signature requirement to be borderline ridiculous. The > > first message you're giving to committers is that you distrust them so > > much that you want them to sign an agreement with their blood > > (figuratively speaking). I don't think it's a very good approach to > > community building, nor does it bring any advantage to anyone. > > I kind of agree with Laurent here. Is the media-committers mailinglist > publicly archived somewhere? I think it is sufficient if this is posted > to a publicly archived mailinglist. That could be linux-media, I would be > fine with that. But media-committers would be more appropriate, but only > if it is archived somewhere. > > If we want a GPG key, what would we do with it anyway? Every time I send pull requests upstream, I sign the PR tag with my GPG key: https://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media.git/tag/?h=media/v6.13-2 This is a requirement from the top maintainer. Requiring it is pretty much standard at the Kernel community, and wasn't anything similar "to sign with my blood" (using your words). It is not just a random GPG key: it is a trusted key as stated at this patch: "a PGP key cross signed by other Kernel and media developers" ... For more details about PGP sign, please read Documentation/process/maintainer-pgp-guide.rst and :ref:`kernel_org_trust_repository`." If you see the last link, we're talking about a GPG signature inside kernel.org web of trust. Heh, all PRs we receive are signed with GPG keys that we trust, including PRs from you. We need to keep doing it with the new workflow. That reminds that there are still a gap there: the e-mail from the newcoming committer shall contain something like: "I'll be using this username to commit patches at media-committers: https://gitlab.freedesktop.org/<username>" I'll add it to the next version. Thanks, Mauro
On Thu, Nov 28, 2024 at 09:19:59AM +0100, Mauro Carvalho Chehab wrote: > Em Wed, 27 Nov 2024 12:59:58 +0100 Hans Verkuil escreveu: > > > > I find the GPG signature requirement to be borderline ridiculous. The > > > first message you're giving to committers is that you distrust them so > > > much that you want them to sign an agreement with their blood > > > (figuratively speaking). I don't think it's a very good approach to > > > community building, nor does it bring any advantage to anyone. > > > > I kind of agree with Laurent here. Is the media-committers mailinglist > > publicly archived somewhere? I think it is sufficient if this is posted > > to a publicly archived mailinglist. That could be linux-media, I would be > > fine with that. But media-committers would be more appropriate, but only > > if it is archived somewhere. > > > > If we want a GPG key, what would we do with it anyway? > > Every time I send pull requests upstream, I sign the PR tag with my GPG > key: > > https://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media.git/tag/?h=media/v6.13-2 > > This is a requirement from the top maintainer. Requiring it is pretty much > standard at the Kernel community, and wasn't anything similar "to sign with > my blood" (using your words). > > It is not just a random GPG key: it is a trusted key as stated at this patch: > > "a PGP key cross signed by other Kernel and media developers" > ... > For more details about PGP sign, please read > Documentation/process/maintainer-pgp-guide.rst and > :ref:`kernel_org_trust_repository`." > > If you see the last link, we're talking about a GPG signature inside > kernel.org web of trust. > > Heh, all PRs we receive are signed with GPG keys that we trust, including > PRs from you. We need to keep doing it with the new workflow. > > That reminds that there are still a gap there: the e-mail from the > newcoming committer shall contain something like: > > "I'll be using this username to commit patches at media-committers: > https://gitlab.freedesktop.org/<username>" > > I'll add it to the next version. I don't mind much either way, but as we're using gitlab for the shared tree, we could also do the same as drm-misc and handle this through a gitlab issue instead of an e-mail. That advantage is that we'll ensure the person has a gitlab account.
On Wed, 27 Nov 2024, Simona Vetter <simona.vetter@ffwll.ch> wrote: > Jumping in the middle here with some clarifications. > > On Wed, 27 Nov 2024 at 12:19, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> On Wed, Nov 27, 2024 at 10:39:48AM +0100, Mauro Carvalho Chehab wrote: >> > It is somewhat similar to drm-intel and drm-xe, where reviews are part >> > of the acceptance criteria to become committers. >> >> Those are corporate trees, so it's easier to set such rules. > > Imo it's the other way round, because it's corporate you need stricter > rules and spell them all out clearly - managers just love to apply > pressure on their engineers too much otherwise "because it's our own > tree". Totally forgetting that it's still part of the overall upstream, > and that they don't own upstream. The required commits and reviews to become a committer may sound somewhat artificial and arbitrary, but it's a sort of compromise. The goal is to have a relatively low bar for entry, while ensuring the committers have just enough experience to judge whether a patch passes merge criteria (more on that below). It's also the same for everyone, and something to strive for. Frankly, I'm not fond of the invite-only model. You need to be careful to not lose transparency. It can be scary to have a lot of committers. You put a lot of trust in them. But at the same time, you do monitor what's going on, and can revert commits - and commit rights, if needed. > So that's why the corporate trees are stricter than drm-misc, but the > goals are still exactly the same: > > - peer review is required in a tit-for-tat market, but not more. > > - committers push their own stuff, that's all. Senior committers often > also push other people's work, like for smaller work they just reviewed > or of people they mentor, but it's not required at all. I think it's also important to define merge criteria. A set of rules by which a committer can commit. And it's not really about technical checkboxes. For example, in drm it really boils down to two things: at least two people have been involved, and there are no open issues. (And have those people recorded in git trailers with Sob/Rb/Ab, with tooling to ensure that's the case. There are very few commis in drm-misc/drm-intel without either 2xSob, Sob+Rb, or Sob+Ab.) > - maintainership duties, like sending around pr, making sure patches dont > get lost and things like that, is separate from commit rights. In my > opinion, if you tie commit rights to maintainership you're doing > something else than drm and I'd more call it a group maintainership > model, not a commit rights model for landing patches. Agreed. Personally, I like the committer/maintainer model, because it's a low barrier to entry, with limited responsibilities. Not everyone wants to become even a committer, and the more loaded it becomes, even less so. Yet the committers help maintainers immensely, and you grow a pool of people who can become maintainers. > Anyway just figured I'll clarify what we do over at drm. I haven't looked > at all the details of this proposal here and the already lengthy > discussion, plus it's really not on me to chime in since I'm not involved. To be honest, IMO the length is one of the shortcomings of the proposal. Lots of up front process, which will inevitably have to be ironed out as you gain experience. You just won't be able to figure it all out in advance. All that said, I commend all efforts to move towards shared maintainership models, whether it's group maintainership or committer/maintainer model or something in between. Just offering my views here, which you're obviously free to completely ignore to your benefit or detriment. ;) BR, Jani.
Em Wed, 27 Nov 2024 12:59:58 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > If I were a new contributor I think I would have trouble understand this > > to be honest. I won't push hard for a rework of this section, as I > > expect it will change after the multi-committer tree becomes the main > > way to get patches merged. We can then update the documentation. > > I wonder if this shouldn't be rewritten completely, e.g. something like this: > > 1) By default patches are against the ``next`` branch of that media.git tree. > > 2) Patches that need to fix bugs in the -rcX kernel should preferably be > against the latest -rc kernel. > > Is there really anything else? These two cases are all I use in practice. Yes, there are three situations: - patches against released kernels; - patches against next kernel (-rc1 being prefered, IMO, against a random -rc); - new features. I modified the text following Laurent's suggestion. > > > > >> + > >> +All patches with fixes shall have: > >> + - a ``Fixes:`` tag pointing to the first commit that introduced the bug; > >> + - a ``Cc: stable@vger.kernel.org`` > >> + > >> +Patches that were fixing bugs reported by someone else shall have: > >> + - a ``Reported-by`` tag immediately followed by a ``Closes`` tag. > > > > There's been a recent discussion about not including a Reported-by tag > > without asking permission from the reporter, due to privacy reasons (in > > particular for bugs reported to https://bugzilla.kernel.org/, as by > > default the e-mail address of the reporter is not public). As the > > Reported-by and Closes tags are not specific to the media tree, I would > > drop this paragraph, otherwise we will have to duplicate a relatively > > large amount of information related to privacy. You can link to the > > relevant documentation instead, but I wouldn't even do that as it's > > really not media-specific. > > I agree. Note that I tend to ask first for permission, unless I know the > reporter is a kernel contributor already, or is otherwise active in the open > source world and so the email is public already. I'm opting to use this word on v2: Patches that were fixing bugs publicly reported by someone at the linux-media@vger.kernel.org mailing list shall have: - a ``Reported-by:`` tag immediately followed by a ``Closes:`` tag. This makes clear that we expect to have Reported-by/Closes if someone publicly reports via e-mail to our development ML. This is a subset of the cases where permission is not required, and likely covers 99% of the cases where this is needed. > >> -The media maintainers that work on specific areas of the subsystem are: > >> - > >> -- Remote Controllers (infrared): > >> - Sean Young <sean@mess.org> > >> - > >> -- HDMI CEC: > >> - Hans Verkuil <hverkuil@xs4all.nl> > >> - > >> -- Media controller drivers: > >> - Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> - > >> -- ISP, v4l2-async, v4l2-fwnode, v4l2-flash-led-class and Sensor drivers: > >> - Sakari Ailus <sakari.ailus@linux.intel.com> > >> - > >> -- V4L2 drivers and core V4L2 frameworks: > >> - Hans Verkuil <hverkuil@xs4all.nl> > > > > We're losing that information, isn't it valuable ? > > That's a good point. I think we still want to keep this information. Although > this list should probably be moved to... As discussed at the other thread, things like that need to be at MAINTAINERS, as this will warrant that the right people will be c/c. While not explicitly stated at the document, when we mention MAINTAINERS there, it means that media committer records there need to be up-to-dated when changes happen, in special the status (S:) and mail fields (M:) need to reflect the committers responsibilities. So, for instance, let's suppose we have an entry like this: RADIO GAGA MEDIA DRIVER M: My User <my@user.domain> L: linux-media@vger.kernel.org S: Odd Fixes W: https://linuxtv.org T: git git://linuxtv.org/media.git F: drivers/media/radio/radio-gaga* if a new committer decides to step up to maintain the driver, such entry will be changed to: RADIO GAGA MEDIA DRIVER M: Queen Fanboy <fanboy@queen.fanclub> L: linux-media@vger.kernel.org S: Maintained W: https://linuxtv.org T: git git://linuxtv.org/media.git F: drivers/media/radio/radio-gaga* (either dropping "My User" if he has no time to maintain it anymore or adding a second M:) If, in the future, it decides to not maintain/commit patches for it anymore, because he has no time or interest to keep maintaining it, the status should be changed to: RADIO GAGA MEDIA DRIVER L: linux-media@vger.kernel.org S: Orphan W: https://linuxtv.org T: git git://linuxtv.org/media.git F: drivers/media/radio/radio-gaga* If nobody steps in to keep maintaining it. That's not different from the current Kernel practices. > > I think this goes a bit backward, and mixes things up a bit. On the > > mixing side, the expectation of timely reviews comes from maintainer > > status. Having commit rights is orthogonal to that. > > > > The goal of direct commit access is to speed up maintenance, to get > > patches reviewed and merged quicker. Are we saying here that if someone > > has commit rights they will lose them if they take too long to review > > code ? That would then slow down maintenance even more, which seems > > counterproductive. > > Someone with commit rights is also a maintainer, since that's how you > gain the trust to get those rights. If you do a poor job of reviewing > patches relevant for you as maintainer, then you loose that trust. > > And if you simply don't have the time anymore for that, then perhaps > you need to look for a co-maintainer or just stop being a maintainer for > that area. A good example of that is actually the uvc driver. In that case > the solution was adding a co-maintainer. > > > Also, while one can be recognized as a maintainer for multiple drivers > > or parts of the kernel, there's a single committer status. You can't > > revoke committer status of a particular driver only. > > The committer status is a sign that you are trusted. Part of that is timely > patch review. Or admit that you won't have the time/resources to do that job > and look for a co-maintainer or even give up maintainership of some areas. Exactly. > That said, perhaps the text should change a bit: > > "not waiting in patchwork as ``New`` for more than one Kernel merge cycle" -> > "ideally not waiting in patchwork as ``New`` for more than one Kernel merge cycle" > > We all have patches in patchwork that are much older than that, for one reason > or another, but if this happens all the time, then you have a problem. I'll do the suggested changes. > >> +This privilege is granted with some expectation of responsibility: > > > > "Privilege" sounds a bit like lord and serf. > > How about 'These commit rights are granted'? Ok. > >> +committers are people who care about the Linux Kernel as a whole and > >> +about the Linux media subsystem and want to help its development. It > >> +is also based on a trust relationship between the rest of the committers, > >> +maintainers and the LinuxTV community. > > > > Who is "the LinuxTV community" ? > > "linux kernel media community"? I opted to add both. > >> +Becoming a media committer > >> +-------------------------- > >> + > >> +The most important aspect of volunteering to be a committer is that you will > >> +be able to review and approve other people's changes, so we are looking for > > > > Everybody is able to review patches (rather, everybody is allowed to > > review patches, the ability is a different matter). > > > >> +whether we think you will be good at doing that. > > > > I've been told that "whether" should also come with a "or" clause. You > > can write "whether or not we think ...". > > How about this: > > "The most important aspect of volunteering to be a committer is that you have > demonstrated the ability to give good code reviews." Ok. > >> +It is also desirable that developers that intend to become committers > >> +make a best effort to attend the yearly Linux Media Summit, typically > >> +co-located with another Linux conference. > > > > I would say that "are encouraged to attend" instead of "make a best > > effort to attend". Also, how will this scale when we'll have a few > > dozen committers ? Typically the media summit is capped to 20 attendees > > or less. > > If we have that many committers, then we can afford a larger room and we > probably would have to start charging some contributions as well. But > that would be a luxury problem :-) It's a bridge we can cross when we > get there. Agreed. > Note that I am fine with "are encouraged to attend". I think that's a good > phrase. Ok. > >> + I, John Doe, would like to change my status to: Committer > >> + > >> + I intend to actively develop the XYZ driver, send fixes to drivers > >> + that I can test, reviewing patches and merging trivial fixes > >> + for the subsystem, ... > > > > "Merging trivial fixes for the subsystem" bothers me. I don't think it > > needs to be a requirement for committers. This is a maintainer's > > responsibility. If people want to help with that that's great, but > > making it a requirement isn't. Or did you mean this as an example ? > > > > We shouldn't expect committers to handle a higher workload than what > > they do as driver maintainers who submit patches by e-mail or send pull > > requests. Giving commit rights will lower the effort to get patches in, > > and I think it's fair to ask for keeping patchwork up-to-date in return, > > but that's about it. > > The idea was to make it explicit that they can review and merge trivial > fixes for the subsystem as a whole (so outside the direct area that they > maintain), but that is certainly optional. > > How about: > > ", and optionally reviewing patches and merging trivial fixes in other > areas of the subsystem," Sounds OK to me. Will use that. > > I find the GPG signature requirement to be borderline ridiculous. The > > first message you're giving to committers is that you distrust them so > > much that you want them to sign an agreement with their blood > > (figuratively speaking). I don't think it's a very good approach to > > community building, nor does it bring any advantage to anyone. > > I kind of agree with Laurent here. Is the media-committers mailinglist > publicly archived somewhere? I think it is sufficient if this is posted > to a publicly archived mailinglist. That could be linux-media, I would be > fine with that. But media-committers would be more appropriate, but only > if it is archived somewhere. > > If we want a GPG key, what would we do with it anyway? With PRs, the authenticity was ensured by signed git tags. With MRs, we need to ensure when we're giving grants, which happens after the committer sends us an e-mail agreeing to be a committer. I'm adding to the declaration of intent a text with the username that will be used by the new committer: "I, John Doe, would like to change my status to: Committer I intend to actively develop the XYZ driver, send fixes to drivers that I can test, optionally reviewing patches and merging trivial fixes in other areas of the subsystem, ... For the purpose of committing patches to the media-committer's tree, I'll be using my user https://gitlab.freedesktop.org/users/<username>. I'm also replacing the part that mentions PGP with: Such e-mail shall be signed with a PGP key cross signed by other Kernel and media developers. As described at :ref:`media-developers-gpg`_, the PGP signature, together with the gitlab user security are fundamental components that ensure the authentity of the merge requests that will happen at the media-committer.git tree. Finally, the reference there (media-developers-gpg) will be part of the media maintainer profile entry as: Authentication for pull and merge requests ++++++++++++++++++++++++++++++++++++++++++ The authenticity of developers submitting pull requests and merge requests shall be validated by using PGP sign, via the :ref:`kernel_org_trust_repository`. With the pull request workflow, pull requests shall use a GPG-signed tag. With the committers' workflow, this is ensured at the time merge request rights will be granted to the gitlab instance used by media-committers.git tree, after receiving the e-mail documented at :ref:`media-committer-agreement`. For more details about PGP sign, please read Documentation/process/maintainer-pgp-guide.rst. > > > > This is problematic, as we can't expect people to check for changes in > > this file every time they push something. Changes to this file should be > > announced to all committers, with a reasonable review period. > > "Any changes to the kernel media development process will be announced in > the media-committers mailinglist with a reasonable review period. All > committers are automatically subscribed to that mailinglist." I added a variation of that: In case the kernel development process changes, by merging new commits in the `media-committer tree <https://gitlab.freedesktop.org/linux-media/media-committers>`_, the media committer implicitly declares their agreement with the latest version of the documented process including the contents of this file. .. note:: 1. Changes to the kernel media development process should be announced in the media-committers mailinglist with a reasonable review period. All committers are automatically subscribed to that mailinglist; 2. Due to the distributed nature of the Kernel development, it is possible that kernel development process changes may end being reviewed/merged at the linux-docs mailing list, specially for the contents under Documentation/process and for trivial typo fixes. Since we're talking about the kernel development process as a whole, I added a notice that other parts of the process may change any time (like the recent changes to CoC), and such changes may not be c/c to linux-media. > > There are tools to ease updating the status of a patch, could you > > document or at least mention them ? > > I think that is out-of-scope. It certainly could be added as a follow-up > patch. I ended adding a link, as it doesn't hurt to have it there. > >> +In the unhappy event that a media committer continues to disregard good > >> +citizenship (or actively disrupts the project), we may need to revoke > > > > That's very, very vague, surprisingly vague even from someone who raised > > many concerns about the kernel code of conduct being vague. > > I suspect that this phrasing is copied from another project. Mauro, can you > confirm that? Yes: it came from Chromium. > I think it is extremely difficult to give explicit guidance here. Agreed. > >> +that person's status. In such cases, if someone suggests the revocation with > >> +a good reason, other developers may second the motion. The final decision > >> +is taken by the subsystem maintainers. As the decision to become a media > > > > What does "seconding the motion" bring, if the decision lies solely in > > maintainers ? > > I think the intent here is that, other than in extreme circumstances, it shouldn't > be a unilateral decision from the subsystem maintainers. Multiple media committers > should agree with it. > > But perhaps it would be better to replace this with: > > "In such cases, if someone suggests the revocation with a good reason, then after > discussing this among the media committers, the final decision is taken by the > subsystem maintainers." Changed. > I really hope we will never end up in a situation like this, since that's going > to be painful regardless of what procedure you choose. Yes. Things like that are painful and stressful. > >> +committer comes from a consensus between subsystem maintainers, a single > >> +subsystem maintainer not trusting the media committer anymore is enough to > >> +revoke committer's privileges. > >> + > >> +If a committer is inactive for more than a couple of Kernel cycles, > >> +maintainers will try to reach you via e-mail. If not possible, they may > >> +revoke your committer privileges and update MAINTAINERS file entries > >> +accordingly. If you wish to resume contributing later on, then contact > >> +the subsystem maintainers to ask if your rights can be restored. > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: > > > > "Committers are encouraged to request their commit rights get removed > > when they no longer contribute to the project. Commit rights may be > > automatically revoked after a year of inactivity (no commits or > > reviews). Commit rights will be reinstated when they come back to the > > project." > > Yes, that's better. I also realized that the mention of updating the MAINTAINERS > makes no sense since that does not contain media committer status. See my comments above (with regards to MAINTAINERS) and a separate thread. > >> + > >> +A previous committer that had his commit rights revoked can keep contributing > > > > s/his/their/ > > > >> +to the subsystem via the normal e-mail workflow as documented at the > >> +:ref:`Media development workflow`. > >> + > >> +References > >> +---------- > >> + > >> +Much of this was inspired by/copied from the committer policies of: > >> + > >> +- `Chromium <https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md>`_; > >> +- `WebKit <http://www.google.com/url?q=http%3A%2F%2Fwebkit.org%2Fcoding%2Fcommit-review-policy.html&sa=D&sntz=1&usg=AFrqEze4W4Lvbhue4Bywqgbv-N5J66kQgA>`_; > > > > Google tracks us enough without using google URLs. > > > >> +- `Mozilla <http://www.google.com/url?q=http%3A%2F%2Fwww.mozilla.org%2Fhacking%2Fcommitter%2F&sa=D&sntz=1&usg=AFrqEzecK7iiXqV30jKibNmmMtzHwtYRTg>`_. > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html > > would also have been a good source of inspiration. That's the only large > > multi-committer workflow today in the kernel, and it has proven its > > value. The explicit acceptance criteria in particular are very good. > > Quoting the document, it says > > > > "Commit rights will be granted to anyone who requests them and fulfills > > the below criteria:" > > > > That's how we build an inclusive community, it feels way more welcoming > > than saying that maintainers will discuss in private and grant > > privileges to underlings if it pleases them (even if the effect is the > > same in practice, it's still a maintainer decision). > > The main difference here is that in drm developers can ask for commit rights, > whereas for the media subsystem they are invited by existing media committers. > > The drm model is absolutely more inclusive, and I hope we can end up there > eventually. But for now I think we need more work on both the procedures and > the media-ci workflow. > > Even with just two sub-maintainers committing patches it took quite a long time > to find and fix all the bugs/issues we encountered. At this point we are > definitely not ready to implement a drm model. > > This document just starts this process, it will change and be improved over time, > but we need this in place before we can start adding more committers. Agreed. Thanks, Mauro
Em Wed, 27 Nov 2024 15:25:15 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > > > I think this goes a bit backward, and mixes things up a bit. On the > > > mixing side, the expectation of timely reviews comes from maintainer > > > status. Having commit rights is orthogonal to that. > > > > > > The goal of direct commit access is to speed up maintenance, to get > > > patches reviewed and merged quicker. Are we saying here that if someone > > > has commit rights they will lose them if they take too long to review > > > code ? That would then slow down maintenance even more, which seems > > > counterproductive. > > > > Someone with commit rights is also a maintainer, since that's how you > > gain the trust to get those rights. If you do a poor job of reviewing > > patches relevant for you as maintainer, then you loose that trust. > > This is I think the point where our expectations are the least aligned. > I'm considering "committer" based on what is done in drm-misc. A > committer is essentially a developer who has demonstrated they can > follow a documented process to push their own patches. They are given > push access as a shortcut, which frees time for the subsystem > maintainers who don't have to pick patches manually from the list (or > handle pull requests). That's the official side of it. The barrier to > entry is intentionally kept very low to ensure that committers won't > decide to use the legacy workflow due to expectations of additional work > load. Committers are not required or even asked to take any extra work. > It's still a win-win situation: subsystem maintainers have less work, > and committers can get their patches upstream more easily. > > Then there's the other "secret" goal: through handing out committer > rights, the maintainers hoped that a subset of the committers would > become more involved, grow more knowledge about the subsystem, pick up > third party patches, review or cross-review code, ... And that worked, > DRM has grown an active community of developers who go beyond their > personal needs and help with maintenance more broadly. Dave and Sima > deliberately decided to favour the carrot over the stick, and I think > the events that followed proved it was the right decision. > > This is what I would like to see replicated in the media subsystem. Even > if a committer only handles the single driver they're interested in and > push their own patches, it's still a win for everybody involved. By > making the barrier to entry low, we will make it possible for people who > would have been scared of volunteering to become part of the community, > and over time handle more responsibilities. Setting a higher barrier to > entry will scare those people away. Even myself, if I'm expected to do > more than what I do today to get commit rights, I won't request them. > Everybody will lose, I will have to keep sending pull requests, and you > will have to keep handling them. Both of us will lose time that we could > otherwise use for reviews or other tasks beneficial to the subsystem. > > More importantly than the exact wording, it's the core principle of the > committers model that we need to agree on. If we don't have the same > expectations it will clearly not work. The reality on media is *very* different from DRM. With DRM, most drivers have multiple developers working on it, and the more important drivers typically have dozens of committers. The vast majority of such committers aren't listed at MAINTAINERS file for the drm drivers they commit patches. On media, there's usually just one person that maintains the driver who will become a committer if they want. Right now, my expectation is that *all* committers will also be a maintainer, e. g. they'll all be listed at MAINTAINERS file, being responsible by one or more driver. Besides that, the multi-committers will replace the current sub-maintainers workflow. We also need to do a slow start to ensure that media-ci, patchwork, CI integration with patchwork, etc will work properly. With that in mind, every committer has duties of reviewing other developer's patches submitted for the drivers that they're listed as a maintainer at the MAINTAINERS file entries. > > >> +If you are doing such tasks and have become a valued developer, an > > >> +existing committer can nominate you to the media subsystem maintainers. > > > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: > > > > > > "Maintainers and committers should encourage contributors to request > > > commit rights, especially junior contributors tend to underestimate > > > their skills." > > > > In drm is it the contributors that request commit rights? Or is it those > > who already have commit rights that invite others? Currently the plan for > > the media subsystem is the second method. Although that might change in the > > future, of course. > > The process is documented in > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request. > It requires explicit action from the candidate, as they have to create a > gitlab.fdo account, and request commit access by fiing an issue in > gitlab. You can see the issue template at > https://gitlab.freedesktop.org/drm/misc/kernel/-/issues/new?issue[title]=Request%20for%20Commit%20Rights&issuable_template=commit_access, > which is roughly speaking the equivalent of the mail template in this > document. In practice, as mentioned in the documentation, people often > underestimate their skills and lack confidence to ask for committer > access. That's why the documentation states that maintainers and > committers should encourage contributors to request access. > > I like that model because it requires an explicit action from the > contributor to show that they have an interest, and it also makes it > possible for people to request access without having been nominated. It > doesn't mean that access will be automatically granted, there are still > acceptance criteria, and it's a maintainer decision at the end of the > day. > > Stating as done in this patch that an existing committer can nominate > someone implies that contributors have to wait until they get notified > they can join The Chosen Few. It's not very welcoming, and given how > busy everybody is, valuable contributors may need to wait for longer > than they should before someone thinks about nominating them. > > I wouldn't expect a change of wording to result in any practical change > in the process, it is only about being more inclusive and welcoming in > the document. If we want to create a vibrant community, people should > feel not just welcome, but also desired and valued. The model we're implementing is that the action of becoming a committer will also have a step where the contributor will show that they have an interest. Yet, right now the goal is to implement the model starting with active media maintainers. Once we get there, and after a couple of kernel releases to test that everything is working as expected, we may aim to carefully expand it. Thanks, Mauro
Em Wed, 27 Nov 2024 15:48:10 +0100 Simona Vetter <simona.vetter@ffwll.ch> escreveu: > Jumping in the middle here with some clarifications. > > On Wed, 27 Nov 2024 at 12:19, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Wed, Nov 27, 2024 at 10:39:48AM +0100, Mauro Carvalho Chehab wrote: > > > It is somewhat similar to drm-intel and drm-xe, where reviews are part > > > of the acceptance criteria to become committers. > > > > Those are corporate trees, so it's easier to set such rules. > > Imo it's the other way round, because it's corporate you need stricter > rules and spell them all out clearly - managers just love to apply > pressure on their engineers too much otherwise "because it's our own > tree". Totally forgetting that it's still part of the overall upstream, > and that they don't own upstream. > > So that's why the corporate trees are stricter than drm-misc, but the > goals are still exactly the same: > > - peer review is required in a tit-for-tat market, but not more. > > - committers push their own stuff, that's all. Senior committers often > also push other people's work, like for smaller work they just reviewed > or of people they mentor, but it's not required at all. > > - maintainership duties, like sending around pr, making sure patches dont > get lost and things like that, is separate from commit rights. In my > opinion, if you tie commit rights to maintainership you're doing > something else than drm and I'd more call it a group maintainership > model, not a commit rights model for landing patches. Right now, our focus is for driver maintainers to become committers, so they all have maintainership duties as well. The requirement we're adding is to ensure that they're doing a good work as committers/maintainers, reviewing patches from others, as otherwise nobody will do that. Now, once we start getting drivers with lots of developers working on them without maintainership status, we can start including them, but this is not our reality, as usually, there is usually only one or, at most a couple of developers per driver. > Anyway just figured I'll clarify what we do over at drm. I haven't looked > at all the details of this proposal here and the already lengthy > discussion, plus it's really not on me to chime in since I'm not involved. Thanks, Mauro
Em Thu, 28 Nov 2024 13:24:04 +0200 Jani Nikula <jani.nikula@intel.com> escreveu: > On Wed, 27 Nov 2024, Simona Vetter <simona.vetter@ffwll.ch> wrote: > > Jumping in the middle here with some clarifications. > > > > On Wed, 27 Nov 2024 at 12:19, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > >> On Wed, Nov 27, 2024 at 10:39:48AM +0100, Mauro Carvalho Chehab wrote: > >> > It is somewhat similar to drm-intel and drm-xe, where reviews are part > >> > of the acceptance criteria to become committers. > >> > >> Those are corporate trees, so it's easier to set such rules. > > > > Imo it's the other way round, because it's corporate you need stricter > > rules and spell them all out clearly - managers just love to apply > > pressure on their engineers too much otherwise "because it's our own > > tree". Totally forgetting that it's still part of the overall upstream, > > and that they don't own upstream. > > The required commits and reviews to become a committer may sound > somewhat artificial and arbitrary, but it's a sort of compromise. The > goal is to have a relatively low bar for entry, while ensuring the > committers have just enough experience to judge whether a patch passes > merge criteria (more on that below). It's also the same for everyone, > and something to strive for. We used to have a low bar for entry on our past multi-committers model (back in 2005-2007). It was a disaster, as one of the committer did very evil things. He was blocked, but that didn't prevent some of us to be threatened with physical violence - and some people even reported death threats. So, let's start slow to ensure that things like that won't ever happen again. > Frankly, I'm not fond of the invite-only model. You need to be careful > to not lose transparency. The intent is to be as transparent as possible without violating regulations like GPDR. > It can be scary to have a lot of committers. You put a lot of trust in > them. But at the same time, you do monitor what's going on, and can > revert commits - and commit rights, if needed. The scariest part is to receive threats like what happened in the past. Some were publicly documented; others happened via private talks and e-mails. > > So that's why the corporate trees are stricter than drm-misc, but the > > goals are still exactly the same: > > > > - peer review is required in a tit-for-tat market, but not more. > > > > - committers push their own stuff, that's all. Senior committers often > > also push other people's work, like for smaller work they just reviewed > > or of people they mentor, but it's not required at all. > > I think it's also important to define merge criteria. A set of rules by > which a committer can commit. And it's not really about technical > checkboxes. For example, in drm it really boils down to two things: at > least two people have been involved, and there are no open issues. That's the same criteria we're aiming for. We'll start without two people reviewing, as there won't be enough committers at the beginning for that, but maintainers may revert/rebase the tree in case they don't agree with changes. > (And have those people recorded in git trailers with Sob/Rb/Ab, with > tooling to ensure that's the case. There are very few commis in > drm-misc/drm-intel without either 2xSob, Sob+Rb, or Sob+Ab.) We added a CI engine to check this and other issues. If CI fails, commit will be automatically be blocked. > > - maintainership duties, like sending around pr, making sure patches dont > > get lost and things like that, is separate from commit rights. In my > > opinion, if you tie commit rights to maintainership you're doing > > something else than drm and I'd more call it a group maintainership > > model, not a commit rights model for landing patches. > > Agreed. Personally, I like the committer/maintainer model, because it's > a low barrier to entry, with limited responsibilities. Not everyone > wants to become even a committer, and the more loaded it becomes, even > less so. Yet the committers help maintainers immensely, and you grow a > pool of people who can become maintainers. Currently, for most of the drivers, the number of committers per driver is equal to the number of maintainers for the same driver. So, on this stage, we're aiming on get maintainers commit rights, starting with the ones that are long time contributors and regularly participate at the media summits. Once the "slow start" phase finishes, we can review the process and start thinking on getting more developers and committers. > > Anyway just figured I'll clarify what we do over at drm. I haven't looked > > at all the details of this proposal here and the already lengthy > > discussion, plus it's really not on me to chime in since I'm not involved. > > To be honest, IMO the length is one of the shortcomings of the > proposal. Lots of up front process, which will inevitably have to be > ironed out as you gain experience. You just won't be able to figure it > all out in advance. Agreed. > All that said, I commend all efforts to move towards shared > maintainership models, whether it's group maintainership or > committer/maintainer model or something in between. Just offering my > views here, which you're obviously free to completely ignore to your > benefit or detriment. ;) Thank you for you valuable feedback! Best regards, Mauro
On Thu, Nov 28, 2024 at 07:15:43PM +0100, Mauro Carvalho Chehab wrote: > Em Wed, 27 Nov 2024 15:25:15 +0200 Laurent Pinchart escreveu: > > > > > I think this goes a bit backward, and mixes things up a bit. On the > > > > mixing side, the expectation of timely reviews comes from maintainer > > > > status. Having commit rights is orthogonal to that. > > > > > > > > The goal of direct commit access is to speed up maintenance, to get > > > > patches reviewed and merged quicker. Are we saying here that if someone > > > > has commit rights they will lose them if they take too long to review > > > > code ? That would then slow down maintenance even more, which seems > > > > counterproductive. > > > > > > Someone with commit rights is also a maintainer, since that's how you > > > gain the trust to get those rights. If you do a poor job of reviewing > > > patches relevant for you as maintainer, then you loose that trust. > > > > This is I think the point where our expectations are the least aligned. > > I'm considering "committer" based on what is done in drm-misc. A > > committer is essentially a developer who has demonstrated they can > > follow a documented process to push their own patches. They are given > > push access as a shortcut, which frees time for the subsystem > > maintainers who don't have to pick patches manually from the list (or > > handle pull requests). That's the official side of it. The barrier to > > entry is intentionally kept very low to ensure that committers won't > > decide to use the legacy workflow due to expectations of additional work > > load. Committers are not required or even asked to take any extra work. > > It's still a win-win situation: subsystem maintainers have less work, > > and committers can get their patches upstream more easily. > > > > Then there's the other "secret" goal: through handing out committer > > rights, the maintainers hoped that a subset of the committers would > > become more involved, grow more knowledge about the subsystem, pick up > > third party patches, review or cross-review code, ... And that worked, > > DRM has grown an active community of developers who go beyond their > > personal needs and help with maintenance more broadly. Dave and Sima > > deliberately decided to favour the carrot over the stick, and I think > > the events that followed proved it was the right decision. > > > > This is what I would like to see replicated in the media subsystem. Even > > if a committer only handles the single driver they're interested in and > > push their own patches, it's still a win for everybody involved. By > > making the barrier to entry low, we will make it possible for people who > > would have been scared of volunteering to become part of the community, > > and over time handle more responsibilities. Setting a higher barrier to > > entry will scare those people away. Even myself, if I'm expected to do > > more than what I do today to get commit rights, I won't request them. > > Everybody will lose, I will have to keep sending pull requests, and you > > will have to keep handling them. Both of us will lose time that we could > > otherwise use for reviews or other tasks beneficial to the subsystem. > > > > More importantly than the exact wording, it's the core principle of the > > committers model that we need to agree on. If we don't have the same > > expectations it will clearly not work. > > The reality on media is *very* different from DRM. With DRM, most We're designing a process for the future, it's up to us to design what we want to achieve. > drivers have multiple developers working on it, and the more important > drivers typically have dozens of committers. The vast majority of such There are a few corporate-backed drivers that have bigger teams, but apart from that, it's not as well staffed as you seem to imply. > committers aren't listed at MAINTAINERS file for the drm drivers they > commit patches. > > On media, there's usually just one person that maintains the driver > who will become a committer if they want. > > Right now, my expectation is that *all* committers will also be > a maintainer, e. g. they'll all be listed at MAINTAINERS file, > being responsible by one or more driver. > > Besides that, the multi-committers will replace the current > sub-maintainers workflow. > > We also need to do a slow start to ensure that media-ci, patchwork, > CI integration with patchwork, etc will work properly. > > With that in mind, every committer has duties of reviewing other > developer's patches submitted for the drivers that they're listed as > a maintainer at the MAINTAINERS file entries. I'm sorry but that's not a multi-committer model, it's a co-maintenance model. If that's what you really want we can reopen the discussion and start anew, but I don't think it's a good idea. As I said before, if it increases my work load, I don't want commit rights. I'll keep sending pull requests, you'll have to keep processing them, and patches will be merged slower. It will be a lose-lose situation for everybody, you, me, contributors and users. Starting with a situation where we are understaffed and trying to solve it by putting more work on the few people who currently keep the subsystem alive doesn't sound like a winning strategy. > > > >> +If you are doing such tasks and have become a valued developer, an > > > >> +existing committer can nominate you to the media subsystem maintainers. > > > > > > > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request: > > > > > > > > "Maintainers and committers should encourage contributors to request > > > > commit rights, especially junior contributors tend to underestimate > > > > their skills." > > > > > > In drm is it the contributors that request commit rights? Or is it those > > > who already have commit rights that invite others? Currently the plan for > > > the media subsystem is the second method. Although that might change in the > > > future, of course. > > > > The process is documented in > > https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html#access-request. > > It requires explicit action from the candidate, as they have to create a > > gitlab.fdo account, and request commit access by fiing an issue in > > gitlab. You can see the issue template at > > https://gitlab.freedesktop.org/drm/misc/kernel/-/issues/new?issue[title]=Request%20for%20Commit%20Rights&issuable_template=commit_access, > > which is roughly speaking the equivalent of the mail template in this > > document. In practice, as mentioned in the documentation, people often > > underestimate their skills and lack confidence to ask for committer > > access. That's why the documentation states that maintainers and > > committers should encourage contributors to request access. > > > > I like that model because it requires an explicit action from the > > contributor to show that they have an interest, and it also makes it > > possible for people to request access without having been nominated. It > > doesn't mean that access will be automatically granted, there are still > > acceptance criteria, and it's a maintainer decision at the end of the > > day. > > > > Stating as done in this patch that an existing committer can nominate > > someone implies that contributors have to wait until they get notified > > they can join The Chosen Few. It's not very welcoming, and given how > > busy everybody is, valuable contributors may need to wait for longer > > than they should before someone thinks about nominating them. > > > > I wouldn't expect a change of wording to result in any practical change > > in the process, it is only about being more inclusive and welcoming in > > the document. If we want to create a vibrant community, people should > > feel not just welcome, but also desired and valued. > > The model we're implementing is that the action of becoming a > committer will also have a step where the contributor will show > that they have an interest. > > Yet, right now the goal is to implement the model starting with > active media maintainers. > > Once we get there, and after a couple of kernel releases to test > that everything is working as expected, we may aim to carefully > expand it.
On Thu, Nov 28, 2024 at 07:28:42PM +0100, Mauro Carvalho Chehab wrote: > Em Wed, 27 Nov 2024 15:48:10 +0100 Simona Vetter escreveu: > > > Jumping in the middle here with some clarifications. > > > > On Wed, 27 Nov 2024 at 12:19, Laurent Pinchart wrote: > > > On Wed, Nov 27, 2024 at 10:39:48AM +0100, Mauro Carvalho Chehab wrote: > > > > It is somewhat similar to drm-intel and drm-xe, where reviews are part > > > > of the acceptance criteria to become committers. > > > > > > Those are corporate trees, so it's easier to set such rules. > > > > Imo it's the other way round, because it's corporate you need stricter > > rules and spell them all out clearly - managers just love to apply > > pressure on their engineers too much otherwise "because it's our own > > tree". Totally forgetting that it's still part of the overall upstream, > > and that they don't own upstream. > > > > So that's why the corporate trees are stricter than drm-misc, but the > > goals are still exactly the same: > > > > - peer review is required in a tit-for-tat market, but not more. > > > > - committers push their own stuff, that's all. Senior committers often > > also push other people's work, like for smaller work they just reviewed > > or of people they mentor, but it's not required at all. > > > > - maintainership duties, like sending around pr, making sure patches dont > > get lost and things like that, is separate from commit rights. In my > > opinion, if you tie commit rights to maintainership you're doing > > something else than drm and I'd more call it a group maintainership > > model, not a commit rights model for landing patches. > > Right now, our focus is for driver maintainers to become committers, > so they all have maintainership duties as well. Mauro, that may be your focus, but it's not "ours". > The requirement we're adding is to ensure that they're doing a > good work as committers/maintainers, reviewing patches from others, > as otherwise nobody will do that. > > Now, once we start getting drivers with lots of developers working > on them without maintainership status, we can start including > them, but this is not our reality, as usually, there is usually > only one or, at most a couple of developers per driver. > > > Anyway just figured I'll clarify what we do over at drm. I haven't looked > > at all the details of this proposal here and the already lengthy > > discussion, plus it's really not on me to chime in since I'm not involved.
On Thu, 28 Nov 2024, Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > We used to have a low bar for entry on our past multi-committers > model (back in 2005-2007). It was a disaster, as one of the > committer did very evil things. He was blocked, but that didn't > prevent some of us to be threatened with physical violence - and > some people even reported death threats. While I understand the hesitation, I don't think it's fair towards potential future collaborators to distrust them based on a bad actor from nearly 20 years ago. >> Frankly, I'm not fond of the invite-only model. You need to be careful >> to not lose transparency. > > The intent is to be as transparent as possible without violating regulations > like GPDR. I have no idea how GDPR would be relevant here. We don't collect data, other than what's in git. >> I think it's also important to define merge criteria. A set of rules by >> which a committer can commit. And it's not really about technical >> checkboxes. For example, in drm it really boils down to two things: at >> least two people have been involved, and there are no open issues. > > That's the same criteria we're aiming for. We'll start without > two people reviewing, as there won't be enough committers at the It's not two reviewers for us either; it's typically author+reviewer and either author or reviewer commits. Two sets of eyeballs in total. > beginning for that, but maintainers may revert/rebase the tree in > case they don't agree with changes. Not sure if you really mean it, but saying it like that doesn't really breed trust, IMO. Sure, there have been patches merged to i915 that I didn't "agree" with. But bad enough to warrant a revert? Very few and far between, and always for clear and concrete reasons rather than anything subjective. Side note, we don't do rebases in the development branches. > Currently, for most of the drivers, the number of committers per driver > is equal to the number of maintainers for the same driver. FWIW, I think that pretty much matches how it was for most drivers in drm before the committer model. > So, on this stage, we're aiming on get maintainers commit rights, > starting with the ones that are long time contributors and regularly > participate at the media summits. > > Once the "slow start" phase finishes, we can review the process and > start thinking on getting more developers and committers. Just saying, it's easier to convince people to become committers with no strings attached than (co-)maintainers with a bunch of responsibilities, such as review or travel obligations. BR, Jani.
On Thu, 28 Nov 2024 at 22:27, Jani Nikula <jani.nikula@intel.com> wrote: > On Thu, 28 Nov 2024, Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > We used to have a low bar for entry on our past multi-committers > > model (back in 2005-2007). It was a disaster, as one of the > > committer did very evil things. He was blocked, but that didn't > > prevent some of us to be threatened with physical violence - and > > some people even reported death threats. > > While I understand the hesitation, I don't think it's fair towards > potential future collaborators to distrust them based on a bad actor > from nearly 20 years ago. Yeah this sounds a lot more like a CoC issue (which of course could result in a very swift removal of commit rights and suspend from all access to gitlab and mailing lists). Aside from reference the CoC we've left these things out of scope of the commit rights processes and merge criteria. My key takeaway over the last decade more of maintainering is that assuming that people want to do the right thing and building a process optimized for that works really well. And then handle the toxic people entirely separately through solid conduct enforcement. > >> Frankly, I'm not fond of the invite-only model. You need to be careful > >> to not lose transparency. > > > > The intent is to be as transparent as possible without violating regulations > > like GPDR. > > I have no idea how GDPR would be relevant here. We don't collect data, > other than what's in git. Yeah I don't see the GDPR connection either. > >> I think it's also important to define merge criteria. A set of rules by > >> which a committer can commit. And it's not really about technical > >> checkboxes. For example, in drm it really boils down to two things: at > >> least two people have been involved, and there are no open issues. > > > > That's the same criteria we're aiming for. We'll start without > > two people reviewing, as there won't be enough committers at the > > It's not two reviewers for us either; it's typically author+reviewer and > either author or reviewer commits. Two sets of eyeballs in total. > > > beginning for that, but maintainers may revert/rebase the tree in > > case they don't agree with changes. > > Not sure if you really mean it, but saying it like that doesn't really > breed trust, IMO. Sure, there have been patches merged to i915 that I > didn't "agree" with. But bad enough to warrant a revert? Very few and > far between, and always for clear and concrete reasons rather than > anything subjective. > > Side note, we don't do rebases in the development branches. Yeah, if I don't forget anything I remember a grand total of three rebases by maintainers, and this over 8 years or so of doing this: - Someone pushed their entire development tree by accident. We obviously had to back that out and improved the tooling to catch these better. - Someone who pushed an entire pile of work (I think 30 patches or so) that missed the merge window into -fixes for a late -rc1. - Someone who lost trust with upstream maintainers because they refused to listen for way too long to engineering direction and consensus. The last big push of development work was backed out again. There might have been some other things, but I think those were more maintainers screwing things up than committers pushing stuff, and on trees that are handled with the more classic group maintainer model. It's really an extremely rare event that we rebase out patches. Reverts are usually handled like any other patches with the usual test and review flow, just hopefully a bit accelerated. Cheers, Sima > > Currently, for most of the drivers, the number of committers per driver > > is equal to the number of maintainers for the same driver. > > FWIW, I think that pretty much matches how it was for most drivers in > drm before the committer model. > > > So, on this stage, we're aiming on get maintainers commit rights, > > starting with the ones that are long time contributors and regularly > > participate at the media summits. > > > > Once the "slow start" phase finishes, we can review the process and > > start thinking on getting more developers and committers. > > Just saying, it's easier to convince people to become committers with no > strings attached than (co-)maintainers with a bunch of responsibilities, > such as review or travel obligations.
Em Thu, 28 Nov 2024 23:27:04 +0200 Jani Nikula <jani.nikula@intel.com> escreveu: > On Thu, 28 Nov 2024, Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > We used to have a low bar for entry on our past multi-committers > > model (back in 2005-2007). It was a disaster, as one of the > > committer did very evil things. He was blocked, but that didn't > > prevent some of us to be threatened with physical violence - and > > some people even reported death threats. > > While I understand the hesitation, I don't think it's fair towards > potential future collaborators to distrust them based on a bad actor > from nearly 20 years ago. It is not about distrust; it is a matter of start slow and on a more controlled way. > >> Frankly, I'm not fond of the invite-only model. You need to be careful > >> to not lose transparency. > > > > The intent is to be as transparent as possible without violating regulations > > like GPDR. > > I have no idea how GDPR would be relevant here. We don't collect data, > other than what's in git. People may opt to not discuss publicly about denied requests or about losing commit rights. > >> I think it's also important to define merge criteria. A set of rules by > >> which a committer can commit. And it's not really about technical > >> checkboxes. For example, in drm it really boils down to two things: at > >> least two people have been involved, and there are no open issues. > > > > That's the same criteria we're aiming for. We'll start without > > two people reviewing, as there won't be enough committers at the > > It's not two reviewers for us either; it's typically author+reviewer and > either author or reviewer commits. Two sets of eyeballs in total. Ah, OK. Yeah, that's the model we're implementing. > > beginning for that, but maintainers may revert/rebase the tree in > > case they don't agree with changes. > > Not sure if you really mean it, but saying it like that doesn't really > breed trust, IMO. Sure, there have been patches merged to i915 that I > didn't "agree" with. But bad enough to warrant a revert? Very few and > far between, and always for clear and concrete reasons rather than > anything subjective. > > Side note, we don't do rebases in the development branches. On our model, the development tree is at: https://linuxtv.org/git/media.git We won't be rebasing it (except in case of emergencies). Yet, at least while we're experiencing with the new model, we reserve the right of doing rebases at the media-committer.git tree in case something bad happens with the tree. > > Currently, for most of the drivers, the number of committers per driver > > is equal to the number of maintainers for the same driver. > > FWIW, I think that pretty much matches how it was for most drivers in > drm before the committer model. > > > So, on this stage, we're aiming on get maintainers commit rights, > > starting with the ones that are long time contributors and regularly > > participate at the media summits. > > > > Once the "slow start" phase finishes, we can review the process and > > start thinking on getting more developers and committers. > > Just saying, it's easier to convince people to become committers with no > strings attached than (co-)maintainers with a bunch of responsibilities, > such as review or travel obligations. We know. Thanks, Mauro
Em Thu, 28 Nov 2024 22:52:38 +0100 Simona Vetter <simona.vetter@ffwll.ch> escreveu: > On Thu, 28 Nov 2024 at 22:27, Jani Nikula <jani.nikula@intel.com> wrote: > > On Thu, 28 Nov 2024, Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > We used to have a low bar for entry on our past multi-committers > > > model (back in 2005-2007). It was a disaster, as one of the > > > committer did very evil things. He was blocked, but that didn't > > > prevent some of us to be threatened with physical violence - and > > > some people even reported death threats. > > > > While I understand the hesitation, I don't think it's fair towards > > potential future collaborators to distrust them based on a bad actor > > from nearly 20 years ago. > > Yeah this sounds a lot more like a CoC issue (which of course could > result in a very swift removal of commit rights and suspend from all > access to gitlab and mailing lists). Aside from reference the CoC > we've left these things out of scope of the commit rights processes > and merge criteria. > > My key takeaway over the last decade more of maintainering is that > assuming that people want to do the right thing and building a process > optimized for that works really well. And then handle the toxic people > entirely separately through solid conduct enforcement. > Community has evolved and CoC may help, but it is still it is dozen times more painful to remove grants than to not add rights for people that aren't ready yet to become committers. The migration to the new model is already complex enough with experienced people having troubles with the new CI engine and new process. With just to people testing the new process, basically every time we committed something, we discovered one or more issues with the CI that would end denying merges and cause frustration and more work to maintainers. > > >> I think it's also important to define merge criteria. A set of rules by > > >> which a committer can commit. And it's not really about technical > > >> checkboxes. For example, in drm it really boils down to two things: at > > >> least two people have been involved, and there are no open issues. > > > > > > That's the same criteria we're aiming for. We'll start without > > > two people reviewing, as there won't be enough committers at the > > > > It's not two reviewers for us either; it's typically author+reviewer and > > either author or reviewer commits. Two sets of eyeballs in total. > > > > > beginning for that, but maintainers may revert/rebase the tree in > > > case they don't agree with changes. > > > > Not sure if you really mean it, but saying it like that doesn't really > > breed trust, IMO. Sure, there have been patches merged to i915 that I > > didn't "agree" with. But bad enough to warrant a revert? Very few and > > far between, and always for clear and concrete reasons rather than > > anything subjective. > > > > Side note, we don't do rebases in the development branches. > > Yeah, if I don't forget anything I remember a grand total of three > rebases by maintainers, and this over 8 years or so of doing this: > > - Someone pushed their entire development tree by accident. We > obviously had to back that out and improved the tooling to catch these > better. > - Someone who pushed an entire pile of work (I think 30 patches or so) > that missed the merge window into -fixes for a late -rc1. > - Someone who lost trust with upstream maintainers because they > refused to listen for way too long to engineering direction and > consensus. The last big push of development work was backed out again. > > There might have been some other things, but I think those were more > maintainers screwing things up than committers pushing stuff, and on > trees that are handled with the more classic group maintainer model. > > It's really an extremely rare event that we rebase out patches. Rebases should be rare, and we do avoid doing that, but it depends on what happens and how the merged tree is tested. We hope that the workflow we're implementing with CI testing everything will prevent them, but we need to run it for a few kernel cycles to be sure that what is there is good enough. Regards, Mauro
Em Thu, 28 Nov 2024 21:07:07 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > > With that in mind, every committer has duties of reviewing other > > developer's patches submitted for the drivers that they're listed as > > a maintainer at the MAINTAINERS file entries. > > I'm sorry but that's not a multi-committer model, it's a co-maintenance > model. If that's what you really want we can reopen the discussion and > start anew, but I don't think it's a good idea. > > As I said before, if it increases my work load, I don't want commit > rights. I'll keep sending pull requests, you'll have to keep processing > them, and patches will be merged slower. It will be a lose-lose > situation for everybody, you, me, contributors and users. > > Starting with a situation where we are understaffed and trying to solve > it by putting more work on the few people who currently keep the > subsystem alive doesn't sound like a winning strategy. After sleeping over it, I agree that you're partially right on this. Doing timely reviews is orthogonal of being a committer. What defines if you need to do timely reviews is if you're listed or not at the MAINTANERS file as "M:" - e.g. if the developer is a maintainer (on its broader sense) or not. This applies for both PR and MR workflows. Still, if one is not fulfilling its duty as maintainer, he may end losing maintainership status and the corresponding committer rights. I wrote a separate patch to make it clear. See below. Thanks, Mauro --- [PATCH] docs: media: profile: make it clearer about maintainership duties During the review of the media committes profile, it was noticed that the responsibility for timely review patches was not clear: such review is expected that all developers listed at MAINTAINERS with the "M:" tag (e.g. "maintainers" on its broad sense). This is orthogonal of being a media committer or not. Such duty is implied at: Documentation/admin-guide/reporting-issues.rst and at the MAINTAINERS header, when it says that even when the status is "odd fixes", the patches will flow in. So, let make it explicit at the maintainer-entry-profile that maintainers need to do timely reviews. Also, while right now our focus is on granting committer rights to maintainers, the media-committer model may evolve in the future to accept other committers that don't have such duties. So, make it clear at the media-committer.rst that the duties related to reviewing patches from others are for the drivers they are maintainers as well. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst index 650803c30c41..6daf71bc72c1 100644 --- a/Documentation/driver-api/media/maintainer-entry-profile.rst +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst @@ -147,6 +147,11 @@ b. Committers' workflow: patches are handled by media committers:: On both workflows, all patches shall be properly reviewed at linux-media@vger.kernel.org before being merged at media-committers.git. +Such patches will be timely-reviewed by developers listed as maintainers at +the MAINTAINERS file. Such maintainers will follow one of the above +workflows, e. g. they will either send a pull request or merge patches +directly at the media-committers tree. + When patches are picked by patchwork and when merged at media-committers, CI bots will check for errors and may provide e-mail feedback about patch problems. When this happens, the patch submitter must fix them diff --git a/Documentation/driver-api/media/media-committer.rst b/Documentation/driver-api/media/media-committer.rst index 1756a7af6353..a873ef84fbca 100644 --- a/Documentation/driver-api/media/media-committer.rst +++ b/Documentation/driver-api/media/media-committer.rst @@ -87,9 +87,9 @@ be delegating part of their maintenance tasks. Due to that, to become a committer or a core committer, a consensus between all subsystem maintainers is required, as they all need to trust a developer well enough to be delegated the responsibility to maintain part of the code -and to properly review patches from third parties, in a timely manner and -keeping the status of the reviewed code at https://patchwork.linuxtv.org -updated. +and to properly review patches from third parties for the drivers they are +maintainers in a timely manner and keeping the status of the reviewed code +at https://patchwork.linuxtv.org updated. .. Note::
Hi Mauro, On Fri, Nov 29, 2024 at 11:29:52AM +0100, Mauro Carvalho Chehab wrote: > Em Thu, 28 Nov 2024 21:07:07 +0200 Laurent Pinchart escreveu: > > > > With that in mind, every committer has duties of reviewing other > > > developer's patches submitted for the drivers that they're listed as > > > a maintainer at the MAINTAINERS file entries. > > > > I'm sorry but that's not a multi-committer model, it's a co-maintenance > > model. If that's what you really want we can reopen the discussion and > > start anew, but I don't think it's a good idea. > > > > As I said before, if it increases my work load, I don't want commit > > rights. I'll keep sending pull requests, you'll have to keep processing > > them, and patches will be merged slower. It will be a lose-lose > > situation for everybody, you, me, contributors and users. > > > > Starting with a situation where we are understaffed and trying to solve > > it by putting more work on the few people who currently keep the > > subsystem alive doesn't sound like a winning strategy. > > After sleeping over it, I agree that you're partially right on this. \o/ You should sleep more often :-D > Doing timely reviews is orthogonal of being a committer. What defines > if you need to do timely reviews is if you're listed or not at the > MAINTANERS file as "M:" - e.g. if the developer is a maintainer > (on its broader sense) or not. This applies for both PR and MR workflows. > > Still, if one is not fulfilling its duty as maintainer, he may end > losing maintainership status and the corresponding committer rights. > > I wrote a separate patch to make it clear. See below. > > Thanks, > Mauro > > --- > > [PATCH] docs: media: profile: make it clearer about maintainership duties > > During the review of the media committes profile, it was noticed > that the responsibility for timely review patches was not clear: > such review is expected that all developers listed at MAINTAINERS > with the "M:" tag (e.g. "maintainers" on its broad sense). > > This is orthogonal of being a media committer or not. Such duty > is implied at: > > Documentation/admin-guide/reporting-issues.rst > > and at the MAINTAINERS header, when it says that even when the > status is "odd fixes", the patches will flow in. > > So, let make it explicit at the maintainer-entry-profile that > maintainers need to do timely reviews. > > Also, while right now our focus is on granting committer rights to > maintainers, the media-committer model may evolve in the future to > accept other committers that don't have such duties. > > So, make it clear at the media-committer.rst that the duties > related to reviewing patches from others are for the drivers > they are maintainers as well. > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> I'll comment on this on v3 of the series. > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst > index 650803c30c41..6daf71bc72c1 100644 > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst > @@ -147,6 +147,11 @@ b. Committers' workflow: patches are handled by media committers:: > On both workflows, all patches shall be properly reviewed at > linux-media@vger.kernel.org before being merged at media-committers.git. > > +Such patches will be timely-reviewed by developers listed as maintainers at > +the MAINTAINERS file. Such maintainers will follow one of the above > +workflows, e. g. they will either send a pull request or merge patches > +directly at the media-committers tree. > + > When patches are picked by patchwork and when merged at media-committers, > CI bots will check for errors and may provide e-mail feedback about > patch problems. When this happens, the patch submitter must fix them > diff --git a/Documentation/driver-api/media/media-committer.rst b/Documentation/driver-api/media/media-committer.rst > index 1756a7af6353..a873ef84fbca 100644 > --- a/Documentation/driver-api/media/media-committer.rst > +++ b/Documentation/driver-api/media/media-committer.rst > @@ -87,9 +87,9 @@ be delegating part of their maintenance tasks. > Due to that, to become a committer or a core committer, a consensus between > all subsystem maintainers is required, as they all need to trust a developer > well enough to be delegated the responsibility to maintain part of the code > -and to properly review patches from third parties, in a timely manner and > -keeping the status of the reviewed code at https://patchwork.linuxtv.org > -updated. > +and to properly review patches from third parties for the drivers they are > +maintainers in a timely manner and keeping the status of the reviewed code > +at https://patchwork.linuxtv.org updated. > > .. Note:: >
diff --git a/Documentation/driver-api/media/index.rst b/Documentation/driver-api/media/index.rst index d5593182a3f9..d0c725fcbc67 100644 --- a/Documentation/driver-api/media/index.rst +++ b/Documentation/driver-api/media/index.rst @@ -26,6 +26,7 @@ Documentation/userspace-api/media/index.rst :numbered: maintainer-entry-profile + media-committer v4l2-core dtv-core diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst index ffc712a5f632..90c6c0d9cf17 100644 --- a/Documentation/driver-api/media/maintainer-entry-profile.rst +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst @@ -27,19 +27,128 @@ It covers, mainly, the contents of those directories: Both media userspace and Kernel APIs are documented and the documentation must be kept in sync with the API changes. It means that all patches that add new features to the subsystem must also bring changes to the -corresponding API files. +corresponding API documentation files. -Due to the size and wide scope of the media subsystem, media's -maintainership model is to have sub-maintainers that have a broad -knowledge of a specific aspect of the subsystem. It is the sub-maintainers' -task to review the patches, providing feedback to users if the patches are +Due to the size and wide scope of the media subsystem, the media's +maintainership model is to have committers that have a broad knowledge of +a specific aspect of the subsystem. It is the committers' task to +review the patches, providing feedback to users if the patches are following the subsystem rules and are properly using the media kernel and userspace APIs. -Patches for the media subsystem must be sent to the media mailing list -at linux-media@vger.kernel.org as plain text only e-mail. Emails with -HTML will be automatically rejected by the mail server. It could be wise -to also copy the sub-maintainer(s). +Media committers +---------------- + +In the media subsystem, there are experienced developers that can commit +patches directly on a development tree. These developers are called +Media committers and are divided into the following categories: + +- Committers: responsible for one or more drivers within the media subsystem. + They can upload changes to the tree that do not affect the core or ABI. + +- Core committers: responsible for part of the media core. They are typically + responsible for one or more drivers within the media subsystem, but, besides + that, they can also merge patches that change the code common to multiple + drivers, including the kernel internal API/ABI. + +- Subsystem maintainers: responsible for the subsystem as a whole, with + access to the entire subsystem. + + Only subsystem maintainers can change the userspace API/ABI. + +Media committers shall explicitly agree with the Kernel development process +as described at Documentation/process/index.rst and to the Kernel +development rules inside the Kernel documentation, including its code of +conduct. + +More details about media committers can be found here: +Documentation/driver-api/media/media-committer.rst. + +Media development tree +---------------------- + +The main development tree used by the media subsystem is hosted at LinuxTV.org, +where we also maintain news about the subsystem, wiki pages and a patchwork +instance where we track patches though their lifetime. + +The main tree used by media developers is at: + +https://git.linuxtv.org/media.git/ + +.. _Media development workflow: + +Media development workflow +++++++++++++++++++++++++++ + +All changes for the media subsystem must be sent first as e-mails to the +media mailing list, as plain text only e-mail to: + + `https://subspace.kernel.org/vger.kernel.org.html <linux-media@vger.kernel.org>`_ + +Emails with HTML will be automatically rejected by the mail server. +It could be wise to also copy the media committer(s). You should use +``scripts/get_maintainers.pl`` to identify whom else needs to be copied. +Please always copy driver's authors and maintainers. + +Such patches needed to be based against a public branch or tag as follows: + +1. new Kernel releases: + + Those need to be based at the ``next`` branch of that media.git tree + +2. During Kernel release development cycle, patches fixing bugs on a -rc + kernel should preferably be against the latest -rc1 Kernel. If they + require a previously-applied change, they need to be against the ``fixes`` + branch; + +3. Patches against an already released kernel should preferably be against + the latest released Kernel. If they require a previously-applied + change, they need to be against ``fixes``. + +All patches with fixes shall have: + - a ``Fixes:`` tag pointing to the first commit that introduced the bug; + - a ``Cc: stable@vger.kernel.org`` + +Patches that were fixing bugs reported by someone else shall have: + - a ``Reported-by`` tag immediately followed by a ``Closes`` tag. + +Patches that change API/ABI shall require patches to update documentation +accordingly at the same patch series. + +See Documentation/process/index.rst for more details about e-mail submission. + +Once a patch is submitted, it may follow either one of the workflows +below: + +a. Normal workflow: patches are handled by subsystem maintainers:: + + +------+ +---------+ +-------+ +-------------------+ +---------+ + |e-mail|-->|patchwork|-->|pull |-->|maintainers merge |-->|media.git| + +------+ +---------+ |request| |at media-committers| +---------+ + +-------+ +-------------------+ + + For this workflow, pull requests can be generated by a committer, + a previous committer, subsystem maintainers or by a couple of trusted + long-time contributors. If you are not in such group, please don't submit + pull requests, as they will likely be ignored. + +b. Committers' workflow: patches are handled by media committers:: + + +------+ +---------+ +-------------------+ +-----------+ +---------+ + |e-mail|-->|patchwork|-->|committers merge |-->|maintainers|-->|media.git| + +------+ +---------+ |at media-committers| |approval | +---------+ + +-------------------+ +-----------+ + +When patches are merged at patchwork and when merged at media-committers, +CI bots will check for errors and may provide e-mail feedback about +patch problems. When this happens, the e-mail author must fix them +and send another version of the patch. + +Patches will only be moved to the next stage in those two workflows if they +don't fail on CI or if there are false-positives at the CI reports. + +Failures during e-mail submission ++++++++++++++++++++++++++++++++++ Media's workflow is heavily based on Patchwork, meaning that, once a patch is submitted, the e-mail will first be accepted by the mailing list @@ -47,51 +156,36 @@ server, and, after a while, it should appear at: - https://patchwork.linuxtv.org/project/linux-media/list/ -If it doesn't automatically appear there after a few minutes, then +If it doesn't automatically appear there after some time [2]_, then probably something went wrong on your submission. Please check if the -email is in plain text\ [2]_ only and if your emailer is not mangling +email is in plain text\ [3]_ only and if your emailer is not mangling whitespaces before complaining or submitting them again. -You can check if the mailing list server accepted your patch, by looking at: +To troubleshoot problems, you should first check if the mailing list +server has accepted your patch, by looking at: - https://lore.kernel.org/linux-media/ -.. [2] If your email contains HTML, the mailing list server will simply +If the patch is there and not at patchwork, it is likely that your e-mailer +mangled the patch. Patchwork internally has a logic that checks if the +received e-mail contain a valid patch. Any whitespace and new line +breakages mangling the patch won't be recognized by patchwork, thus such +patch will be rejected. + +.. [2] It usually takes a few minutes for the patch to arrive, but + the e-mail server is busy, so it may take up to a few hours + for a patch to be handled by the mail server and by the patchwork + instance. + +.. [3] If your email contains HTML, the mailing list server will simply drop it, without any further notice. +Subsystem maintainers +--------------------- -Media maintainers -+++++++++++++++++ - -At the media subsystem, we have a group of senior developers that -are responsible for doing the code reviews at the drivers (also known as -sub-maintainers), and another senior developer responsible for the -subsystem as a whole. For core changes, whenever possible, multiple -media maintainers do the review. - -The media maintainers that work on specific areas of the subsystem are: - -- Remote Controllers (infrared): - Sean Young <sean@mess.org> - -- HDMI CEC: - Hans Verkuil <hverkuil@xs4all.nl> - -- Media controller drivers: - Laurent Pinchart <laurent.pinchart@ideasonboard.com> - -- ISP, v4l2-async, v4l2-fwnode, v4l2-flash-led-class and Sensor drivers: - Sakari Ailus <sakari.ailus@linux.intel.com> - -- V4L2 drivers and core V4L2 frameworks: - Hans Verkuil <hverkuil@xs4all.nl> - -The subsystem maintainer is: - Mauro Carvalho Chehab <mchehab@kernel.org> - -Media maintainers may delegate a patch to other media maintainers as needed. -On such case, checkpatch's ``delegate`` field indicates who's currently -responsible for reviewing a patch. +The subsystem maintainers are: + Mauro Carvalho Chehab <mchehab@kernel.org> and + Hans Verkuil <hverkuil@xs4all.nl> Submit Checklist Addendum ------------------------- @@ -108,17 +202,14 @@ implementing the media APIs: ==================== ======================================================= Type Tool ==================== ======================================================= -V4L2 drivers\ [3]_ ``v4l2-compliance`` +V4L2 drivers\ [4]_ ``v4l2-compliance`` V4L2 virtual drivers ``contrib/test/test-media`` CEC drivers ``cec-compliance`` ==================== ======================================================= -.. [3] The ``v4l2-compliance`` also covers the media controller usage inside +.. [4] The ``v4l2-compliance`` also covers the media controller usage inside V4L2 drivers. -Other compilance tools are under development to check other parts of the -subsystem. - Those tests need to pass before the patches go upstream. Also, please notice that we build the Kernel with:: @@ -134,6 +225,8 @@ Where the check script is:: Be sure to not introduce new warnings on your patches without a very good reason. +Please see `Media development workflow`_ for e-mail submission rules. + Style Cleanup Patches +++++++++++++++++++++ diff --git a/Documentation/driver-api/media/media-committer.rst b/Documentation/driver-api/media/media-committer.rst new file mode 100644 index 000000000000..27b85a37fb2b --- /dev/null +++ b/Documentation/driver-api/media/media-committer.rst @@ -0,0 +1,252 @@ +Media committers +================ + +What is a media committer? +-------------------------- + +A media committer is a developer who can apply patches from other developers +and their own patches at the +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_ +tree. + +It is a media committer's duty to ensure that their entries at the MAINTAINERS +file will be kept updated, and that submitted patches for files for which +they are listed as maintainers are timely reviewed at the mailing list, +not waiting in patchwork as ``New`` for more than one Kernel merge cycle, +and, if accepted, applying them at the media committer's tree. + +This privilege is granted with some expectation of responsibility: +committers are people who care about the Linux Kernel as a whole and +about the Linux media subsystem and want to help its development. It +is also based on a trust relationship between the rest of the committers, +maintainers and the LinuxTV community. + +As such, a media committer is not just someone who is capable of creating +code, but someone who has demonstrated their ability to collaborate +with the team, get the most knowledgeable people to review code, +contribute high-quality code, and follow through to fix issues (in code +or tests). + +.. Note:: + + 1. If a patch introduced a regression, then it is the media committer's + responsibility to correct that as soon as possible. Typically the + patch is either reverted, or an additional patch is committed that + fixes the regression; + 2. if patches are fixing bugs against already released Kernels, including + the reverts above mentioned, the media committer shall add the needed + tags. Please see :ref:`Media development workflow` for more details. + 3. all patches should be properly reviewed at + linux-media@vger.kernel.org before being merged at the + media-committers tree or submitted on pull requests. + +Becoming a media committer +-------------------------- + +The most important aspect of volunteering to be a committer is that you will +be able to review and approve other people's changes, so we are looking for +whether we think you will be good at doing that. + +As such, potential committers must earn enough credibility and trust from the +LinuxTV community. To do that, developers shall be familiar with the open +source model and have been active at the Linux Kernel community for some time, +and, in particular, with the media subsystem. + +So, in addition to actually making the code changes, you are basically +demonstrating your: + +- commitment to the project; +- ability to collaborate with the team and communicate well; +- understand of how upstream and the LinuxTV community works + (policies, processes for testing, code review, ...) +- reasonable knowledge about: + + - the Kernel development process: + Documentation/process/index.rst + + - the Media development profile: + Documentation/driver-api/media/maintainer-entry-profile.rst + +- understanding of the projects' code base and coding style; +- ability to provide feedback to the patch authors; +- ability to judge when a patch might be ready for review and to submit; +- ability to write good code (last but certainly not least). + +It is also desirable that developers that intend to become committers +make a best effort to attend the yearly Linux Media Summit, typically +co-located with another Linux conference. + +If you are doing such tasks and have become a valued developer, an +existing committer can nominate you to the media subsystem maintainers. + +The ultimate responsibility for accepting a nominated committer is up to +the subsystem's maintainers. The committers must earn a trust relationship +with all subsystem maintainers, as, by granting you commit rights, they will +be delegating part of their maintenance tasks. + +Due to that, to become a committer or a core committer, a consensus between +all subsystem maintainers is required, as they all need to trust a developer +well enough to be delegated the responsibility to maintain part of the code +and to properly review patches from third parties, in a timely manner and +keeping the status of the reviewed code at https://patchwork.linuxtv.org +updated. + +.. Note:: + + In order to preserve the developers that could have their commit rights + granted or denied as well as the subsystem maintainers who have the + task to accept or deny commit rights, all communication related to + nominating a committer, preserving commit rights or leaving such function + should happen in private as much as possible. + +Media committer's agreement +--------------------------- + +Once a nominated committer is accepted by all subsystem maintainers, +they will ask if the developer is interested in the nomination and discuss +what area(s) of the media subsystem the committer will be responsible for. + +Once the developer accepts being a committer, the new committer shall +explicitly accept the Kernel development policies described under its +Documentation/, and, in particular to the rules on this document, by writing +an e-mail to media-committers@linuxtv.org, with a declaration of intent +following the model below:: + + I, John Doe, would like to change my status to: Committer + + I intend to actively develop the XYZ driver, send fixes to drivers + that I can test, reviewing patches and merging trivial fixes + for the subsystem, ... + +Followed by a formal declaration of agreement with the Kernel development +rules, signed with a PGP key cross signed by other Kernel and media +developers. Such declaration shall be:: + + I hereby declare that I agree with the Kernel development rules described at: + + https://www.kernel.org/doc/html/latest/driver-api/media/media-committer.rst + + and to the Linux Kernel development process rules. + + I agree to the Code of Conduct as documented here: + Documentation/process/code-of-conduct.rst + + I am aware that I can, at any point of time, retire. In that case, I will + send an e-mail to notify the subsystem maintainers for them to revoke my + commit rights. + + I am aware that the Kernel development rules change over time. + By doing a new commit, I understand that I agree with the rules in effect + at the time of the commit. + +For more details about PGP sign, please read +Documentation/process/maintainer-pgp-guide.rst and +:ref:`kernel_org_trust_repository`. + +In case the kernel development process changes, by merging new commits at the +`media-committers <https://gitlab.freedesktop.org/linux-media/media-committers>`_, +the media committer implicitly declares that the agreement with the latest +version of the documented process and to the contents of this file. + +Core committers +--------------- + +As described in Documentation/driver-api/media/maintainer-entry-profile.rst +a committer may be granted with additional privileges to also be able to +change a core file and/or media subsystem's Kernel API/ABI. The extent of +the core committer's additional privileges will be detailed by the subsystem +maintainers when they nominate a core committer. + +Existing committers may become core committers and vice versa. Such +decisions will be taken in consensus between the subsystem maintainers. + +Media committers rules +---------------------- + +Media committers shall ensure that merged patches will not break any existing +drivers. If it breaks, fixup or revert patches shall be merged as soon as +possible, aiming to be merged at the same Kernel cycle the bug is reported. + +Media committers shall behave accordingly to the permissions granted by +the subsystem maintainers, specially with regards of the scope of changes +they may apply directly at the media-committers tree. Such scope can +change overtime on a mutual greement between media committers and +maintainers. + +As described at :ref:`Media development workflow`, there are workflows. +For the committers' workflow, the following rules apply: + +- Each merged patch shall pass CI tests; + +- Media committers shall request reviews from other committers were + applicable, i.e. because those committers have more knowledge about + some areas that are changed by a patch; + +- No other media committer would be against the proposed changes. + +Patches that do not fall under the committer's workflow criteria will follow +the normal workflow as described at :ref:`Media development workflow`. + +Only a subsystem maintainer can override such rules. + +All media committers shall ensure that patchwork will reflect the current +status, e.g. patches shall be delegated to the media committer who is +handling them and the patch status shall be updated according to these rules: + +- ``Under review``: Used if the patch requires a second opinion + or when it is part of a pull request; +- ``Accepted``: Once a patch is merged at the multi-committer tree. +- ``Superseded``: There is a newer version of the patch posted in the + mailing list. +- ``Duplicated``: There was another patch doing the same thing from someone + else that was accepted. +- ``Not Applicable``: Use for patch series that are not merged at media.git + tree (e.g. drm, dmabuf, upstream merge, etc.) but were cross-posted to the + linux-media mailing list. + +If the committer decides not to merge it, then reply by email to patch +authors, explaining why it is not merged, and patchwork shall be updated +accordingly with either: + +- ``Changes Requested``: if a new revision was requested; +- ``Rejected``: if the proposed change won't be merged upstream. + +If a media committer decides to retire, it is the committer's duty to +notify the subsystem maintainers about that decision. + +Maintaining media committer status +---------------------------------- + +A community of committers working together to move the Linux Kernel +forward is essential to creating successful projects that are rewarding +to work on. If there are problems or disagreements within the community, +they can usually be solved through healthy discussion and debate. + +In the unhappy event that a media committer continues to disregard good +citizenship (or actively disrupts the project), we may need to revoke +that person's status. In such cases, if someone suggests the revocation with +a good reason, other developers may second the motion. The final decision +is taken by the subsystem maintainers. As the decision to become a media +committer comes from a consensus between subsystem maintainers, a single +subsystem maintainer not trusting the media committer anymore is enough to +revoke committer's privileges. + +If a committer is inactive for more than a couple of Kernel cycles, +maintainers will try to reach you via e-mail. If not possible, they may +revoke your committer privileges and update MAINTAINERS file entries +accordingly. If you wish to resume contributing later on, then contact +the subsystem maintainers to ask if your rights can be restored. + +A previous committer that had his commit rights revoked can keep contributing +to the subsystem via the normal e-mail workflow as documented at the +:ref:`Media development workflow`. + +References +---------- + +Much of this was inspired by/copied from the committer policies of: + +- `Chromium <https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md>`_; +- `WebKit <http://www.google.com/url?q=http%3A%2F%2Fwebkit.org%2Fcoding%2Fcommit-review-policy.html&sa=D&sntz=1&usg=AFrqEze4W4Lvbhue4Bywqgbv-N5J66kQgA>`_; +- `Mozilla <http://www.google.com/url?q=http%3A%2F%2Fwww.mozilla.org%2Fhacking%2Fcommitter%2F&sa=D&sntz=1&usg=AFrqEzecK7iiXqV30jKibNmmMtzHwtYRTg>`_. + diff --git a/Documentation/process/maintainer-pgp-guide.rst b/Documentation/process/maintainer-pgp-guide.rst index f5277993b195..795ef8d89271 100644 --- a/Documentation/process/maintainer-pgp-guide.rst +++ b/Documentation/process/maintainer-pgp-guide.rst @@ -903,6 +903,8 @@ the new default in GnuPG v2). To set it, add (or modify) the trust-model tofu+pgp +.. _kernel_org_trust_repository: + Using the kernel.org web of trust repository --------------------------------------------