<mgonzalez>
Well, I guess after 20 years of submitting kernel patches, I still have no clue what the fuck to write in a commit message. I mean, even one-liners get bike shedded to hell and back. (Or maybe especially one liners)
<mgonzalez>
It seems clear to me to say "this thing fails to work as expected" and maybe provide a log of how it fails.
<mgonzalez>
Like for the venus clock, I wrote "venus is non functional right now" and was told "this is too vague" so I added: it doesn't even init, and this was still too vague, so I added the kernel logs to show that indeed, the init just times out, curls up, and dies
<aka_[m]>
mgonzalez: sounds like someone expects you to explain exactly what is wrong in hardware/software combo for which you have no access to documentation of hardware nor software
<mgonzalez>
aka_[m] OMG this is exactly what I was about to write!
<mgonzalez>
You read my mind
<mgonzalez>
Asking some random joe to reverse engineer a complex platform just to explain why a given change is required is totally unrealistic
<mgonzalez>
I made the change because it unbreaks this stupid platform
<mgonzalez>
I don't know why it works, but it works, yay!
<mgonzalez>
I will say it again: the way msm handles WiFi is completely stupid & insanely complex (unrelated side-rant)
<mgonzalez>
The clock tree is also pretty heinous
<Marijn[m]>
aka_: fortunately this has nothing to do with some complicated undocumented hardware/software combo, but with simple and obvious but probably unwritten "conventions" in DTS files?
<Marijn[m]>
mgonzalez: this is regular people-to-people communication. If making a change, justify to the receivers in some extent what you're doing, why you're making the change, and why you think that's the correct change. 8 words ("adreno_smmu is required, so it must be enabled.") is simply not enough.
<mgonzalez>
I spent 5 hours figuring out why init was silently failing and the gist is: it fails because the driver expects an enabled smmu.
<Marijn[m]>
mgonzalez: in this case the desired context was already spelled out to you in IRC and now even in replies on the mailing list. Should make it easy?
<mgonzalez>
Please don't speak to me as if I am 10 years old. I am 50.
<Marijn[m]>
mgonzalez: this is not a personal attack if you're perceiving that way, merely a reply to "I still have no clue what the ... to write in a commit message"
<aka_[m]>
mgonzalez: well if you don't want to bother i would just drop this patch and enable smmu and adreno in board dts
<Marijn[m]>
mgonzalez: Here I'm just trying to be helpful and providing you answers to the questions that other maintainers might surely ask before taking in this patch
<mgonzalez>
Marijn[m]: sorry, I am probably needlessly belligerent these days.
<Marijn[m]>
mgonzalez: No worries, I understand that it's horrible to spend hours on vague errors caused by such simple mistakes/inconsistencies
<Marijn[m]>
Hence all the more reason to document exactly that in the patch description, so that it's helpful for others and may result in future enhancements going forward
<mgonzalez>
Since the merge window has opened, it is not the best time to request input from maintainers
<mgonzalez>
So thanks for your input
<mgonzalez>
I will try to rewrite the patch before moving on to HDMI
<mgonzalez>
Marijn[m] you have worked on adreno correct?
<Marijn[m]>
mgonzalez: no I haven't touched much on adreno, mostly DPU
<Marijn[m]>
mgonzalez: a patch introducing a "Could not fall back to VRAM carveout" error might be welcome though
<mgonzalez>
weird, I thought your email popped up in get_maintainers.perl
<mgonzalez>
for drivers/gpu/drm/msm
<Marijn[m]>
mgonzalez: oh wait, I'm reading the error message wrong. Yeah it looks like it's not propagating a `PTR_ERR` value in `gpu->aspace`
<Marijn[m]>
*to dmesg
<Marijn[m]>
Maybe it's assuming that create_address_space() would have printed that
<mgonzalez>
failed to bind 5000000.gpu (ops a3xx_ops): -19
<Marijn[m]>
But I don't think anyone would be opposed to adding a DRM_DEV_ERROR in the error case
<mgonzalez>
gpu->funcs->create_address_space() failed with -ENODEV IIRC
<Marijn[m]>
mgonzalez: exactly; that says the GPU bind failed with `-19` but not where exactly inside it; that's simply the error forwarded from `create_address_space()`
<mgonzalez>
right
<Marijn[m]>
For your case it would have been useful to have an explicit error there (and maybe skip it if it is -EPROBE_DEFER
<Marijn[m]>
s/-/`-/, s//`)/
<mgonzalez>
OK I'll write a few paragraphs clarifying the situation
<mgonzalez>
Ah yes I always forget EPROBE_DEFER
<Marijn[m]>
Normally there's dev_err_probe() to help with all of this, but I'm not sure if it makes sense in an _init() function
<Marijn[m]>
It would also set device_set_deferred_probe_reason()
<z3ntu>
mgonzalez: For the adreno_smmu I would just write in the commit message something along the lines "arm64: dts: qcom: msm8998: Don't disable adreno_smmu by default" - "There's no reason why the iommu should be disabled by default, it doesn't need any further configuration. And since also all other qcom dtsi files have adreno_smmu enabled in dtsi, enable it here also." - "This also fixes the GPU loading if the adreno_smmu is not explicitly
<z3ntu>
enabled in device dts"
<z3ntu>
Generally GPU doesn't have much to do with your change directly so I'd limit it to that - you could even skip the last part with GPU but I'd include something to make it clear why you are looking to do this change, not just completely random or because you looked which dtsi files have stuff disabled by default
<Marijn[m]>
Luca Weiss: what's your thought about the Fixes tag? Theoretically it's not wrong for any node to be disabled by default, if anything depending on it is also disabled by default.
<z3ntu>
Marijn: I think I've seen "Fixes" being used for such a thing, I also don't think it's out of place for it.. I also don't expect it to break anything if backported
<z3ntu>
But also not strictly necessary imo
<Marijn[m]>
It sets the precedent that the referenced commit was wrong, which it clearly isn't :)
<Marijn[m]>
It's inconsistent (with other DTSI), but that's it
<mgonzalez>
Marijn[m]: please forgive me, I misspelled your name in my message :(
<Marijn[m]>
mgonzalez: Martin is a friend of mine :P
<mgonzalez>
Then he gets the credit
<Marijn[m]>
He is nor doesn't want to be as picky as me I guess