Hi
Am 12.02.25 um 16:52 schrieb Jeffrey Hugo:
On 2/12/2025 6:27 AM, Jacek Lawrynowicz wrote:
Hi,
Thanks for your detailed feedback and constructive suggestions. I
appreciate this as it is not easy to learn all process details
otherwise.
I echo this. At times, accel feels a bit isolated from DRM.
Agreed, but IDK how to fix that. Although both share common code, there
seems little overlap driver-wise.
On 2/12/2025 11:20 AM, 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.)
We were sometimes using patchsets to bundle patches that were tested
together. We apologize for any confusion this may have caused, as we
were not aware that this approach was not preferred. Moving forward,
we will ensure that patches are split into separate series, each
addressing a specific topic. I hope this will help improve clarity
and make it easier to understand and assess the changes.
- 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.
We value all public comments and typically wait a week for public
reviews before submitting patches, regardless of whether an R-b tag
is pre-applied. I was not aware that pre-applying R-b tags was an
issue. We we will ensure that all R-b tags are added publicly from
now on.
I'll provide a counter point on the pre-applied RBs - Qualcomm has
been told many times in the past decade or so to do this (GregKH comes
to mind although I'm certain he is not the only one). I don't
particularly like it, but we seem to have a reputation for poor
quality in the community, and it would appear that the first step to
mitigating that is to indicate that we have actually done internal
reviews. We've been warned that the next step is requiring a
"community approved" developer to SOB everything. I hope to avoid that.
Personally, I value community given RBs for maillist patches over
internal ones and will typically wait/seek them unless the change is
very trivial. I can't speak for The Intel/AMD/Habana folks although I
suspect they will concur with this but I lurk on IRC and of course you
have my email address. Please feel free to reach out with any
feedback. I would hope that we can learn and improve without annoying
the community to the point that the community feels frustrated and
suggests drastic action.
I'd disagree with GregKH here, but him saying this is like having an
'official' statement for what to do. But I don't think other DRM driver
teams pre-apply R-bs. If a patch got an R-b from an internal review,
maybe briefly mention it in the cover letter. At least it's clear then.
Best regards
Thomas
To Jacek, I'm hoping to be more responsive to reviewing your patches
now that we are out of the holidays and other things have settled down
again. I'm sorry if you've felt ignored.
- 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.
OK, I'll talk to the team about this.
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 appreciate your feedback and would welcome more remarks. Please
keep in mind that all accel drivers are new, and it takes time to
learn all the upstream rules.
The kernel/DRM development process is quite unique, and not
everything is fully documented. I find emails like this to be
incredibly valuable and I am eager to comply with the guidelines.
I just need some patience and guidance as I navigate through this.
Thank you for your understanding and support.
Regards,
Jacek
Best regards
Thomas
[1] https://patchwork.freedesktop.org/series/143182/
[2] https://patchwork.freedesktop.org/series/144101/
Am 04.02.25 um 09:46 schrieb Jacek Lawrynowicz:
Add possibility to import single buffer into multiple contexts,
fix locking when aborting contexts and add some debug features.
Andrzej Kacprowski (2):
accel/ivpu: Add missing locks around mmu queues
accel/ivpu: Prevent runtime suspend during context abort work
Karol Wachowski (3):
ccel/ivpu: Add debugfs interface for setting HWS priority bands
accel/ivpu: Add test modes to toggle clock relinquish disable
accel/ivpu: Implement D0i2 disable test modea
Tomasz Rusinowicz (1):
accel/ivpu: Allow to import single buffer into multiple contexts
drivers/accel/ivpu/ivpu_debugfs.c | 84
+++++++++++++++++++++++++++++++
drivers/accel/ivpu/ivpu_drv.c | 2 +-
drivers/accel/ivpu/ivpu_drv.h | 4 ++
drivers/accel/ivpu/ivpu_fw.c | 4 ++
drivers/accel/ivpu/ivpu_gem.c | 43 ++++++++++++++++
drivers/accel/ivpu/ivpu_gem.h | 1 +
drivers/accel/ivpu/ivpu_hw.c | 31 ++++++++++++
drivers/accel/ivpu/ivpu_hw.h | 5 ++
drivers/accel/ivpu/ivpu_job.c | 10 +++-
drivers/accel/ivpu/ivpu_jsm_msg.c | 29 ++++-------
drivers/accel/ivpu/ivpu_mmu.c | 9 ++++
11 files changed, 202 insertions(+), 20 deletions(-)
--
2.45.1
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
