| Message ID | ZrSFpj20b1LbBhCJ@linux (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL] drm-intel-fixes | expand |
On 08/08/2024 10:45, Tvrtko Ursulin wrote: > > Hi Dave, Sima, > > A small bunch of fixes for the weekly cycle: ... > > ---------------------------------------------------------------- > Andi Shyti (2): > drm/i915/gem: Adjust vma offset for framebuffer mmap offset > drm/i915/gem: Fix Virtual Memory mapping boundaries calculation > > David Gow (2): > drm/i915: Allow evicting to use the requested placement > drm/i915: Attempt to get pages without eviction first > > Dnyaneshwar Bhadane (1): > drm/i915/display: correct dual pps handling for MTL_PCH+ Several commits have issues. Look: Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: https://patchwork.freedesktop.org/patch/msgid ... (cherry picked from commit 97b6784753da06d9d40232328efc5c5367e53417) Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> 1. Duplicated committer SoB. You added SoB. No need to add two. It does not get stronger. You do not change the DCO rules by adding two SoBs. You cannot confirm something more or twice. Read DCO one more time... 2. Useless cherry pick SHA. fatal: bad object 97b6784753da06d9d40232328efc5c5367e53417 (Tried with repo having several maintainer repos and the linux-next) Only you have 97b6784753da06d9d40232328efc5c5367e53417. Maybe few other people as well, but all other do not. This does not bring any useful information, rather obfuscates public git history. Best regards, Krzysztof
On 08/08/2024 20:35, Krzysztof Kozlowski wrote: > On 08/08/2024 10:45, Tvrtko Ursulin wrote: >> >> Hi Dave, Sima, >> >> A small bunch of fixes for the weekly cycle: > > ... > >> >> ---------------------------------------------------------------- >> Andi Shyti (2): >> drm/i915/gem: Adjust vma offset for framebuffer mmap offset >> drm/i915/gem: Fix Virtual Memory mapping boundaries calculation >> >> David Gow (2): >> drm/i915: Allow evicting to use the requested placement >> drm/i915: Attempt to get pages without eviction first >> >> Dnyaneshwar Bhadane (1): >> drm/i915/display: correct dual pps handling for MTL_PCH+ > > Several commits have issues. Look: > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Link: https://patchwork.freedesktop.org/patch/msgid ... > (cherry picked from commit 97b6784753da06d9d40232328efc5c5367e53417) > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > 1. Duplicated committer SoB. > You added SoB. No need to add two. It does not get stronger. You do not > change the DCO rules by adding two SoBs. You cannot confirm something > more or twice. Read DCO one more time... > > 2. Useless cherry pick SHA. > fatal: bad object 97b6784753da06d9d40232328efc5c5367e53417 > (Tried with repo having several maintainer repos and the linux-next) > > Only you have 97b6784753da06d9d40232328efc5c5367e53417. Maybe few other > people as well, but all other do not. This does not bring any useful > information, rather obfuscates public git history. ... and in case you claim that 97b6784753da06d9d40232328efc5c5367e53417 is in drm-next, then your workflow is broken because: 1. You will duplicate the same commit. One in drm-fixes and second in drm-next. Just use git features, like branches and merges... First you apply on fixes, then you merge it to next, for example. Or any other sane way. 2. If you rebase drm-next on top of drm-fixes in some time in the future, then that cherry-pick SHA will not work and will be totally useless. so either you create duplicate commits (that's how Intel gets stats?) or you introduce to git history totally bogus SHAs... Best regards, Krzysztof
Hi Krzysztof, You might want to familiarise yourself with the drm tree development procedures before weighing in, and snarky comments like the final one are not appreciated on this list or in this community. The drm next trees are never rebased (only in super rare emergencies), we never rebase next onto fixes, This leads to a number of things that are different from other workflows. I agree the duplicate SoB isn't great, but it's also not harmful, The cherry-pick thing is normal operating procedure unfortunately, we get told if we leave it out it causes problems later, so we add it in, and it resolves itself in the future when the next trees end up in Linus' tree. Thanks, Dave. > > > > 1. Duplicated committer SoB. > > You added SoB. No need to add two. It does not get stronger. You do not > > change the DCO rules by adding two SoBs. You cannot confirm something > > more or twice. Read DCO one more time... > > > > 2. Useless cherry pick SHA. > > fatal: bad object 97b6784753da06d9d40232328efc5c5367e53417 > > (Tried with repo having several maintainer repos and the linux-next) > > > > Only you have 97b6784753da06d9d40232328efc5c5367e53417. Maybe few other > > people as well, but all other do not. This does not bring any useful > > information, rather obfuscates public git history. > > ... and in case you claim that 97b6784753da06d9d40232328efc5c5367e53417 > is in drm-next, then your workflow is broken because: > 1. You will duplicate the same commit. One in drm-fixes and second in > drm-next. Just use git features, like branches and merges... First you > apply on fixes, then you merge it to next, for example. Or any other > sane way. > > 2. If you rebase drm-next on top of drm-fixes in some time in the > future, then that cherry-pick SHA will not work and will be totally useless. > > so either you create duplicate commits (that's how Intel gets stats?) or > you introduce to git history totally bogus SHAs... > > Best regards, > Krzysztof >
On 09/08/2024 09:13, Dave Airlie wrote: > Hi Krzysztof, > > You might want to familiarise yourself with the drm tree development > procedures before weighing in, > and snarky comments like the final one are not appreciated on this > list or in this community. > > The drm next trees are never rebased (only in super rare emergencies), > we never rebase next onto fixes, > > This leads to a number of things that are different from other workflows. > > I agree the duplicate SoB isn't great, but it's also not harmful, > > The cherry-pick thing is normal operating procedure unfortunately, we > get told if we leave it out it causes problems later, > so we add it in, and it resolves itself in the future when the next > trees end up in Linus' tree. So duplicating commits, instead of merging fixes into next branch, is normal approach for drm-next? Surprising, I admit. Thanks for confirming. Best regards, Krzysztof
Quoting Krzysztof Kozlowski (2024-08-08 21:44:39) > On 08/08/2024 20:35, Krzysztof Kozlowski wrote: > > On 08/08/2024 10:45, Tvrtko Ursulin wrote: > >> > >> Hi Dave, Sima, > >> > >> A small bunch of fixes for the weekly cycle: > > > > ... > > > >> > >> ---------------------------------------------------------------- > >> Andi Shyti (2): > >> drm/i915/gem: Adjust vma offset for framebuffer mmap offset > >> drm/i915/gem: Fix Virtual Memory mapping boundaries calculation > >> > >> David Gow (2): > >> drm/i915: Allow evicting to use the requested placement > >> drm/i915: Attempt to get pages without eviction first > >> > >> Dnyaneshwar Bhadane (1): > >> drm/i915/display: correct dual pps handling for MTL_PCH+ > > > > Several commits have issues. Look: > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Link: https://patchwork.freedesktop.org/patch/msgid ... > > (cherry picked from commit 97b6784753da06d9d40232328efc5c5367e53417) > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > > > > 1. Duplicated committer SoB. > > You added SoB. No need to add two. It does not get stronger. You do not > > change the DCO rules by adding two SoBs. You cannot confirm something > > more or twice. Read DCO one more time... Hi Krzysztof, As a self-proclaimed quite active kernel developer (by a quick web search) you might have already ventured to look at the GIT history of the subsytem tree for the patch in question. If you did that, you might have implied that the second S-o-b is added by automation -- and it indeed is. While appreciating the report, maybe not so much the the condescending style of the communication. You now slightly come through as a troll hoping to be fed - I hope that is not the case. Seems like we've had such a double S-o-b regularly generated by DIM (Drm Inglorious Maintainer scripts) at least since 2016 as the first occurrance seems to have been in ccda3a728f70 . So rest of the ecosystem seems to deal with them just fine. Is the double S-o-b issue purely cosmetic for you? Not a lawyer but I don't think there is any legal problem in stating the same thing twice. [1] Or are you maybe running into some more concrete issues with upstream kernel process related scripts or other automation processing of the commit? Regards, Joonas [1] When it comes to the commit rights model in DRM subsystem, stripping the final S-o-b after the cherry-pick would make it less obvious who did the pick. If there are multiple S-o-bs and the cherry-picking person happened to be one of them, that information would be lost. Less predictable outcome for the patch form for no actual gain in my opinion. On the other hand, removing the S-o-b line from above the "(cherry-picked.." line would modify the cherry-picked commit description itself and I would assume everyone agrees that should not be done.
On 09/08/2024 10:35, Joonas Lahtinen wrote: > Quoting Krzysztof Kozlowski (2024-08-08 21:44:39) >> On 08/08/2024 20:35, Krzysztof Kozlowski wrote: >>> On 08/08/2024 10:45, Tvrtko Ursulin wrote: >>>> >>>> Hi Dave, Sima, >>>> >>>> A small bunch of fixes for the weekly cycle: >>> >>> ... >>> >>>> >>>> ---------------------------------------------------------------- >>>> Andi Shyti (2): >>>> drm/i915/gem: Adjust vma offset for framebuffer mmap offset >>>> drm/i915/gem: Fix Virtual Memory mapping boundaries calculation >>>> >>>> David Gow (2): >>>> drm/i915: Allow evicting to use the requested placement >>>> drm/i915: Attempt to get pages without eviction first >>>> >>>> Dnyaneshwar Bhadane (1): >>>> drm/i915/display: correct dual pps handling for MTL_PCH+ >>> >>> Several commits have issues. Look: >>> >>> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> Link: https://patchwork.freedesktop.org/patch/msgid ... >>> (cherry picked from commit 97b6784753da06d9d40232328efc5c5367e53417) >>> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> >>> >>> 1. Duplicated committer SoB. >>> You added SoB. No need to add two. It does not get stronger. You do not >>> change the DCO rules by adding two SoBs. You cannot confirm something >>> more or twice. Read DCO one more time... > > Hi Krzysztof, > > As a self-proclaimed quite active kernel developer (by a quick web search) > you might have already ventured to look at the GIT history of the subsytem > tree for the patch in question. If you did that, you might have implied that > the second S-o-b is added by automation -- and it indeed is. > > While appreciating the report, maybe not so much the the condescending > style of the communication. You now slightly come through as a troll > hoping to be fed - I hope that is not the case. Indeed, sorry for that. It was quite grumpy from my side. > > Seems like we've had such a double S-o-b regularly generated by DIM (Drm > Inglorious Maintainer scripts) at least since 2016 as the first occurrance > seems to have been in ccda3a728f70 . So rest of the ecosystem seems to > deal with them just fine. > > Is the double S-o-b issue purely cosmetic for you? Not a lawyer but I don't > think there is any legal problem in stating the same thing twice. [1] > > Or are you maybe running into some more concrete issues with upstream kernel > process related scripts or other automation processing of the commit? The entire confusion came because, while having latest next trees, the cherry picked commit SHA was nowhere to be found, so I assumed you just moved it between private/work-in-progress branches. Before drm-next hits linux-next (and I know you have different workflows), this confusion will be for everyone who does not have drm-next remote. And yes, processing unknown SHAs is a problem or inconvenience... which could be easily avoided even in your cherry-pick style - first apply to fixes and then cherry-pick (duplicate!) to drm-next. I understand that it is just quirk of DRM development style. While, I still disagree that duplicating commits which have 100% same diff hunks and context (zero difference) is good, that's just personal opinion and people can disagree. AFAIU, the double SoB is incidental, I guess. > > Regards, Joonas > > [1] When it comes to the commit rights model in DRM subsystem, stripping > the final S-o-b after the cherry-pick would make it less obvious who did > the pick. If there are multiple S-o-bs and the cherry-picking person Well, if you start using b4, which does much more, you notice that process does not make information lost. > happened to be one of them, that information would be lost. Less > predictable outcome for the patch form for no actual gain in my opinion. > > On the other hand, removing the S-o-b line from above the "(cherry-picked.." > line would modify the cherry-picked commit description itself and I > would assume everyone agrees that should not be done. Sure. One more time apologies for grumpy response. Best regards, Krzysztof
Hi Dave, Sima, A small bunch of fixes for the weekly cycle: Fix for Meteorlake dual PPS, vma offset calculation and tidy when partial mapping and unbreaking of eviction handling on DG2 small bar systems. Regards, Tvrtko drm-intel-fixes-2024-08-08: - correct dual pps handling for MTL_PCH+ [display] (Dnyaneshwar Bhadane) - Adjust vma offset for framebuffer mmap offset [gem] (Andi Shyti) - Fix Virtual Memory mapping boundaries calculation [gem] (Andi Shyti) - Allow evicting to use the requested placement (David Gow) - Attempt to get pages without eviction first (David Gow) The following changes since commit de9c2c66ad8e787abec7c9d7eff4f8c3cdd28aed: Linux 6.11-rc2 (2024-08-04 13:50:53 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/i915/kernel.git tags/drm-intel-fixes-2024-08-08 for you to fetch changes up to 787db3bb6ed5cee56fc97fecdd61517d89763f0a: drm/i915: Attempt to get pages without eviction first (2024-08-07 11:02:38 +0300) ---------------------------------------------------------------- - correct dual pps handling for MTL_PCH+ [display] (Dnyaneshwar Bhadane) - Adjust vma offset for framebuffer mmap offset [gem] (Andi Shyti) - Fix Virtual Memory mapping boundaries calculation [gem] (Andi Shyti) - Allow evicting to use the requested placement (David Gow) - Attempt to get pages without eviction first (David Gow) ---------------------------------------------------------------- Andi Shyti (2): drm/i915/gem: Adjust vma offset for framebuffer mmap offset drm/i915/gem: Fix Virtual Memory mapping boundaries calculation David Gow (2): drm/i915: Allow evicting to use the requested placement drm/i915: Attempt to get pages without eviction first Dnyaneshwar Bhadane (1): drm/i915/display: correct dual pps handling for MTL_PCH+ drivers/gpu/drm/i915/display/intel_backlight.c | 3 ++ drivers/gpu/drm/i915/display/intel_pps.c | 3 ++ drivers/gpu/drm/i915/gem/i915_gem_mman.c | 55 +++++++++++++++++++++++--- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 13 +++--- 4 files changed, 62 insertions(+), 12 deletions(-)