On Wed, Feb 12, 2025 at 11:20:46AM +0100, Thomas Zimmermann wrote: > Hi, > > here's a complaint about the lack of process and documentation in accel/, > and ivpu specifically. I came across this series while preparing the weekly > PR for drm-misc-next and found myself unable to extract much useful > information to report. This is a problem for a development process that > relies on transparency, accountability and collaboration. Other problematic > examples are at [1] and [2]. IIRC I had similar issues in previous > development cycles. > > I cannot assess the quality of the code itself, but the process and > documentation involved does not meet the requirements. > > - 'Changes for <version>' is not an meaningful description for a patch > series. It's not the submitter (or anyone else) deciding that this series > gets merged into version so-and-so. The series gets merged when it is ready > to be merged. > > - Apparently this series contains 3 different things (buffer imports, > locking, debugging); so it should be 3 series with each addressing one of > these topics. > > - The series' description just restates the patch descriptions briefly. It > should rather give some indication of the problem being solved by the > contained patches, and context on why this is worth solving. (I know that > this is often complicated to state clearly to outsiders.) > > - Review should be public. I understand that it's often only one dev team > working on a specific driver, discussing issues internally. Still it makes > sense to do the code reviews in public, so that others can follow what is > going on in the driver. Public code reviews are also necessary to establish > consent and institutional knowledge within the wider developer community. > You miss that with internal reviews. > > - These patches come with R-b tags pre-applied. Even for trivial changes, > R-b tags should given in public. If the R-bs have been given elsewhere, > please include a reference to that location. The tags (R-b, A-b, T-b, etc) > are not just for verifying the code itself. They also establish trust in the > development process involving each patch; and in the developers involved in > that process. This needs to happen in public to be effective. > > - The kernel's (or any FOSS') development is organized around individuals, > not organizations. Having each developer send their changes individually > would likely resolve most of the current problems. > > I understand that accel is not graphics and can feel somewhat detached from > the rest of DRM. Yet it is part of the DRM subsystem. This development > cycles' ivpu series' made me go to IRC and ask for accel/ to be removed from > the drm-misc tree. Luckily the other maintainer were more charitable. So I > make these remarks in good faith and hope that we can improve the processes > within accel/. I'd also want to emphatize that the last three items at least are applicable to the entire kernel, so even if they had a different tree, it would still not be ok. Maxime
Attachment:
signature.asc
Description: PGP signature
