Re: [PATCH 0/6] accel/ivpu: Changes for 6.15 2025-02-04

[Thomas Zimmermann <tzimmermann@suse.de>]

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)