[v2,0/2] Document the new media-committer's model

The media subsystem used to have a multi-commiter's model in the
past, but things didn't go well on that time, and we had to move to
a centralized model.

As the community has evolved, and as there are now new policies in
place like CoC, let's experiment with a multi-committers again.

The model we're using was inspired by the DRM multi-committers
model. Yet, media subsystem is different on several aspects, so the
model is not exactly the same.

The implementation will be in phases. For this phase, the goal is that 
all committers will be people listed at MAINTAINERS.

On this series,:

patch 1: updates the  media maintainer's entry profile and adds the
workflow that will be used with the new model. While here, it also
adds a missing "P:" tag at the MAINTAINERS file, pointing to it;

patch 2: adds a new document focused at the new maintainers
process. Its target is for developers that will be granted with
commit rights at the new media-maintainers.git tree. It also
contains a reference tag addition to kernel.org PGP chain
at process/maintainer-pgp-guide.rst.

---

v2: I tried to address most of the suggestions where there was an agreement
from Laurent's review and further comments. As there were several changes,
on separate threads, I could have missed something.

Mauro Carvalho Chehab (2):
  docs: media: update maintainer-entry-profile for multi-committers
  docs: media: document media multi-committers rules and process

 Documentation/driver-api/media/index.rst      |   1 +
 .../media/maintainer-entry-profile.rst        | 209 ++++++++++---
 .../driver-api/media/media-committer.rst      | 278 ++++++++++++++++++
 .../process/maintainer-pgp-guide.rst          |   2 +
 MAINTAINERS                                   |   1 +
 5 files changed, 446 insertions(+), 45 deletions(-)
 create mode 100644 Documentation/driver-api/media/media-committer.rst

Em Fri, 29 Nov 2024 16:09:41 +0100
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> escreveu:

> Thanks for putting this together.
> 
> I have marked some minor nits here and there. You can put my
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>

Thanks!

> The only thing that is not a nit: is changing responsible with
> contributor. But if we agree on the meaning (and I think that we do)
> we can always improve this doc later.

See the comments below with regards to your nits.

> On Fri, Nov 29, 2024 at 12:15 PM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:

> > +In the media subsystem, there are experienced developers who can push
> > +patches directly to the 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 push changes to the tree that do not affect the core or ABI.  
> 
> Can we say contributor instead of responsible? For me responsible
> means maintainer.

Works for me.

> I would like to land patches that have been properly reviewed to the
> committers tree for areas that I do not maintiain:
> 
> For example:
> - Laurent has reviewed a uvc patch that I want to land asap to avoid
> conflicts with other patchsets that I am working with.
> - I want to land a patch for a ci breakage that has been reviewed by
> another person, it is trivial, and none has a bad comment about it.
> - I want to land a fix for a driver that has been properly reviewed by
> the maintainer and none has a bad comment about it.

Makes sense. Yet, for the first example you would need to coordinate
with the uvc maintainers to avoid conflicts at the trees they would
be using.

> > +Media development workflow
> > +++++++++++++++++++++++++++
> > +
> > +All changes for the media subsystem must be sent first as e-mails to the
> > +media mailing list, following the process documented at
> > +Documentation/process/index.rst.
> > +
> > +It means that patches shall be submitted as plain text only via e-mail to:
> > +
> > +  `https://subspace.kernel.org/vger.kernel.org.html <linux-media@vger.kernel.org>`_  
> 
> nit: Maybe this is a better url? https://lore.kernel.org/linux-media/

As this is focused on upcoming contributors, placing the place that contains
the subscription link sounds better to me. There, it has links for
subscribe, unsubscribe, post and archive (which already links to lore).

IMO, works better for newbies.

> > +
> > +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  
> nit: How does someone know who the committers are? I think sending to
> the ML and to ./get_maintainers.pl is enough

Yes, but that's what it is written...
> 
> 
> > +``scripts/get_maintainers.pl`` to identify whom else needs to be copied.

here ^

> > +Please always copy driver's authors and maintainers.
> > +
> > +Such patches need to be based against a public branch or tag as follows:
> > +
> > +1. Patches for new features need to be based at the ``next`` branch of
> > +   media.git tree;
> > +
> > +2. Fixes against an already released kernel should preferably be against
> > +   the latest released Kernel. If they require a previously-applied
> > +   change at media.git tree, they need to be against its ``fixes`` branch.  
> 
> 2. Fixes against an already released kernel should preferably be against
>    the ``fixes`` branch of the media.git tree;

The better is to have such fixes against the latest released one, as
this would mean that such patch will apply cleanly at least at the
latest -stable.

Usually, conflicts are unlikely on such cases, but, when they happen,
committers can easily solve it.

As stable will be copied on both versions, that hopefully make their
work easier, as they can just use the version without conflicts.

As a notice, usually stable people doesn't solve conflicts, if they
have a high number of patches: they send-emails requesting us and/or
the author to do it.

> > +3. Fixes for issues not present at the latest released kernel should
> > +   preferably be against the latest -rc1 Kernel. If they require a
> > +   previously-applied change at media.git tree, they need to be against
> > +   its ``fixes`` branch.  
> 
> Can we get rid of this third type? It is a bit confusing.  My mental model is:
> - Things for the next kernel version: next
> - Things for this kernel version: fixes
> 
> We will make sure to close the next tree when needed, and that fixes
> and next are upreved accordingly.

Not all people reporting patches to us will be doing against the
media tree for stuff that are on upstream. That's perfectly fine.
Also, it is an usual practice to have patches against -rc kernels.
This is specially true for developers working on distros: they just
test Linus -rc during their rolling release kernels.

See, for instance:
	https://bodhi.fedoraproject.org/updates/?packages=kernel

So, we need to be prepared to receive patches aiming an upcoming
release on the top of a -rc release.

Maybe we can tell, instead:

3. Fixes for issues not present at the latest released kernel shall
   be either against a -rc kernel for an upcoming release or
   against the ``fixes`` branch of the media.git tree.

That's said, it is uncommon to have conflicts there, but sometimes
they happen.

When they happen, they're trivial enough for the committers to
handle it.

> > +Once a patch is submitted, it may follow either one of the following
> > +workflows:
> > +
> > +a. Pull request workflow: patches are handled by subsystem maintainers::
> > +
> > +     +------+   +---------+   +-------+   +-----------------------+   +---------+
> > +     |e-mail|-->|patchwork|-->|pull   |-->|maintainers merge      |-->|media.git|
> > +     +------+   |picks it |   |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  
> 
> I guess you mean a trusted long-time contributor, not a couple. 
> can you send a PR from two people?

"a couple of" means "a few", not "a couple" ;-)

but yeah, "a trusted long-time contributor" works better.

> 
> > +   long-time contributors. If you are not in such group, please don't submit
> > +   pull requests, as they will likely be ignored.  
> s/be ignored/not processed/.
> 
> Sounds a bit better :).

Agreed.

> Maybe you could even say: not processed, and the author notified.

You meant changing it to:

	please don't submit pull requests, as they will
	not be processed, and the author notified.

right? What do you mean by "and the author notified"?
"and the author will not be notified"?

> > +b. Committers' workflow: patches are handled by media committers::
> > +
> > +     +------+   +---------+   +--------------------+   +-----------+   +---------+
> > +     |e-mail|-->|patchwork|-->|committers merge at |-->|maintainers|-->|media.git|
> > +     +------+   |picks it |   |media-committers.git|   |approval   |   +---------+
> > +                +---------+   +--------------------+   +-----------+
> > +
> > +On both workflows, all patches shall be properly reviewed at
> > +linux-media@vger.kernel.org before being merged at media-committers.git.
> > +
> > +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
> > +and send another version of the patch.  
> 
> must fix them, or explain why the errors are false positives.

Ok.

Thanks,
Mauro

On Sat, 30 Nov 2024 at 13:42, Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Fri, 29 Nov 2024 16:09:41 +0100
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> escreveu:
>
> > Thanks for putting this together.
> >
> > I have marked some minor nits here and there. You can put my
> > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
>
> Thanks!
>
> > The only thing that is not a nit: is changing responsible with
> > contributor. But if we agree on the meaning (and I think that we do)
> > we can always improve this doc later.
>
> See the comments below with regards to your nits.
>
> > On Fri, Nov 29, 2024 at 12:15 PM Mauro Carvalho Chehab
> > <mchehab+huawei@kernel.org> wrote:
>
> > > +In the media subsystem, there are experienced developers who can push
> > > +patches directly to the 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 push changes to the tree that do not affect the core or ABI.
> >
> > Can we say contributor instead of responsible? For me responsible
> > means maintainer.
>
> Works for me.
>
> > I would like to land patches that have been properly reviewed to the
> > committers tree for areas that I do not maintiain:
> >
> > For example:
> > - Laurent has reviewed a uvc patch that I want to land asap to avoid
> > conflicts with other patchsets that I am working with.
> > - I want to land a patch for a ci breakage that has been reviewed by
> > another person, it is trivial, and none has a bad comment about it.
> > - I want to land a fix for a driver that has been properly reviewed by
> > the maintainer and none has a bad comment about it.
>
> Makes sense. Yet, for the first example you would need to coordinate
> with the uvc maintainers to avoid conflicts at the trees they would
> be using.

Sure, coordination with the maintainer is expected.

>
> > > +Media development workflow
> > > +++++++++++++++++++++++++++
> > > +
> > > +All changes for the media subsystem must be sent first as e-mails to the
> > > +media mailing list, following the process documented at
> > > +Documentation/process/index.rst.
> > > +
> > > +It means that patches shall be submitted as plain text only via e-mail to:
> > > +
> > > +  `https://subspace.kernel.org/vger.kernel.org.html <linux-media@vger.kernel.org>`_
> >
> > nit: Maybe this is a better url? https://lore.kernel.org/linux-media/
>
> As this is focused on upcoming contributors, placing the place that contains
> the subscription link sounds better to me. There, it has links for
> subscribe, unsubscribe, post and archive (which already links to lore).
>
> IMO, works better for newbies.
>
> > > +
> > > +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
> > nit: How does someone know who the committers are? I think sending to
> > the ML and to ./get_maintainers.pl is enough
>
> Yes, but that's what it is written...
> >
> >
> > > +``scripts/get_maintainers.pl`` to identify whom else needs to be copied.
>
> here ^
>
> > > +Please always copy driver's authors and maintainers.
> > > +
> > > +Such patches need to be based against a public branch or tag as follows:
> > > +
> > > +1. Patches for new features need to be based at the ``next`` branch of
> > > +   media.git tree;
> > > +
> > > +2. Fixes against an already released kernel should preferably be against
> > > +   the latest released Kernel. If they require a previously-applied
> > > +   change at media.git tree, they need to be against its ``fixes`` branch.
> >
> > 2. Fixes against an already released kernel should preferably be against
> >    the ``fixes`` branch of the media.git tree;
>
> The better is to have such fixes against the latest released one, as
> this would mean that such patch will apply cleanly at least at the
> latest -stable.

They will apply cleanly to the latest stable, but not to our tree.
I prefer that the author to fix the conflict in coordination with the
stable team than us. If they do not respond in good time, we can step
in.

>
> Usually, conflicts are unlikely on such cases, but, when they happen,
> committers can easily solve it.
>
> As stable will be copied on both versions, that hopefully make their
> work easier, as they can just use the version without conflicts.
>
> As a notice, usually stable people doesn't solve conflicts, if they
> have a high number of patches: they send-emails requesting us and/or
> the author to do it.
>
> > > +3. Fixes for issues not present at the latest released kernel should
> > > +   preferably be against the latest -rc1 Kernel. If they require a
> > > +   previously-applied change at media.git tree, they need to be against
> > > +   its ``fixes`` branch.
> >
> > Can we get rid of this third type? It is a bit confusing.  My mental model is:
> > - Things for the next kernel version: next
> > - Things for this kernel version: fixes
> >
> > We will make sure to close the next tree when needed, and that fixes
> > and next are upreved accordingly.
>
> Not all people reporting patches to us will be doing against the
> media tree for stuff that are on upstream. That's perfectly fine.
> Also, it is an usual practice to have patches against -rc kernels.
> This is specially true for developers working on distros: they just
> test Linus -rc during their rolling release kernels.
>
> See, for instance:
>         https://bodhi.fedoraproject.org/updates/?packages=kernel
>
> So, we need to be prepared to receive patches aiming an upcoming
> release on the top of a -rc release.
>
> Maybe we can tell, instead:
>
> 3. Fixes for issues not present at the latest released kernel shall
>    be either against a -rc kernel for an upcoming release or
>    against the ``fixes`` branch of the media.git tree.
>
> That's said, it is uncommon to have conflicts there, but sometimes
> they happen.
>
> When they happen, they're trivial enough for the committers to
> handle it.

What about. Assuming Linus would have released 6.13.rc1 today

1) New features (that will land in 6.14) => media.git/next

2) Fixes for 6.13.rcX => media.git/fixes

3) Fixes for <= 6.12  =>  media.git/fixes . If the patch conflicts in
stable, the author will send the patches

Only 1) can be done by committers.
2) and 3) are coordinated via You and Hans.

Note that if we make the fixes branch up to date with the latest rc,
it will make everyone's life easier. Do you see many conflicts when
you uprev it?

If you like this approach I can help with the wording. If you do not
like it, we can discuss it later and add a follow-up patch.

Also I think that providing an example will make the description more
clear... but that could be me :)

>
> > > +Once a patch is submitted, it may follow either one of the following
> > > +workflows:
> > > +
> > > +a. Pull request workflow: patches are handled by subsystem maintainers::
> > > +
> > > +     +------+   +---------+   +-------+   +-----------------------+   +---------+
> > > +     |e-mail|-->|patchwork|-->|pull   |-->|maintainers merge      |-->|media.git|
> > > +     +------+   |picks it |   |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
> >
> > I guess you mean a trusted long-time contributor, not a couple.
> > can you send a PR from two people?
>
> "a couple of" means "a few", not "a couple" ;-)
>
> but yeah, "a trusted long-time contributor" works better.

nit: for this workflow, pull requests can be generated by a committer:
subsystem maintainers or trusted long-time contributors.

(previous committers already belong to  long-time contributors)
I could even suggest removing the word trusted. Whatever you prefer.

>
> >
> > > +   long-time contributors. If you are not in such group, please don't submit
> > > +   pull requests, as they will likely be ignored.
> > s/be ignored/not processed/.
> >
> > Sounds a bit better :).
>
> Agreed.
>
> > Maybe you could even say: not processed, and the author notified.
>
> You meant changing it to:
>
>         please don't submit pull requests, as they will
>         not be processed, and the author notified.
>
> right? What do you mean by "and the author notified"?
> "and the author will not be notified"?

they will not be processed, and the author will be notified.

>
> > > +b. Committers' workflow: patches are handled by media committers::
> > > +
> > > +     +------+   +---------+   +--------------------+   +-----------+   +---------+
> > > +     |e-mail|-->|patchwork|-->|committers merge at |-->|maintainers|-->|media.git|
> > > +     +------+   |picks it |   |media-committers.git|   |approval   |   +---------+
> > > +                +---------+   +--------------------+   +-----------+
> > > +
> > > +On both workflows, all patches shall be properly reviewed at
> > > +linux-media@vger.kernel.org before being merged at media-committers.git.
> > > +
> > > +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
> > > +and send another version of the patch.
> >
> > must fix them, or explain why the errors are false positives.
>
> Ok.
>
> Thanks,
> Mauro

Thanks :)