[PATCH 0/6] accel/ivpu: Changes for 6.15 2025-02-04
Thomas Zimmermann
tzimmermann at suse.de
Thu Feb 13 14:17:08 UTC 2025
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)
More information about the dri-devel
mailing list