docs: media: document media multi-committers rules and process

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

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::