On Wed, Sep 2, 2020 at 9:55 PM Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Sep 2, 2020 at 9:17 PM Alex Deucher alexdeucher@gmail.com wrote:
On Wed, Sep 2, 2020 at 3:04 PM Luben Tuikov luben.tuikov@amd.com wrote:
On 2020-09-02 11:51 a.m., Daniel Stone wrote:
Hi Luben,
On Wed, 2 Sep 2020 at 16:16, Luben Tuikov luben.tuikov@amd.com wrote:
Not sure how I can do this when someone doesn't want to read up on the kref infrastructure. Can you help?
When someone starts off with "My understanding of ..." (as in the OP) you know you're in trouble and in for a rough times.
Such is the nature of world-wide open-to-everyone mailing lists where anyone can put forth an argument, regardless of their level of understanding. The more obfuscated an argument, the more uncertainty.
If one knows the kref infrastructure, it just clicks, no explanation necessary.
Evidently there are more points of view than yours. Evidently your method of persuasion is also not working, because this thread is now getting quite long and not converging on your point of view (which you are holding to be absolutely objectively correct).
I think you need to re-evaluate the way in which you speak to people, considering that it costs nothing to be polite and considerate, and also takes effort to be rude and dismissive.
Not sure how to help this:
My understanding of the drm core code is like something below. struct B { strcut A } we initialize A firstly and initialize B in the end. But destroy B firstly and destory A in the end.
Luben, please tone it down a bit. You are coming across very harshly. You do make a good point though. What is the point of having the drm release callback if it's ostensibly useless? We should either use it as intended to release the structures allocated by the driver or the drm core should handle it all. With the managed resources there is an incongruity between allocation and freeing which leads to confusion. Even with the proposed updated documentation, it's not clear to me who should use the managed resources or not. My understanding was that it was optional for drivers that wanted it.
In an ideal world this would all be perfectly clean. In reality we have huge existing drivers which, if at all feasible, can only be converted over step by step.
So with that there's a few ways we can make this happen:
- drmm resources are cleaned up before ->release is called. This means
doing auto-cleanup of the final steps like cleanup up drm_device resources is gated on all drivers first being converted completely over to drmm, which is never going to happen. And it's holding up removing all the fairly simple cleanup code from small driver, which is where managed resources (whether drmm or devm) have the most benefit, often they completely eliminate the need for any explicit teardown code.
- start in the middle. That just oopses because the unwind order isn't
the inverse of the setup order anymore, and generally that's required.
- start at the end. Unfortunately this means that the drm_device
cannot be freed in the driver's ->release callback, therefore for transition purposes I had to sprinkle drmm_add_final_kfree all over the place. But if you use devm_drm_dev_alloc (like the updated docs recommend) that's not needed anymore, so really not an eyesore for driver developers.
Yes there's mildly tricky code in the core as a result, but given that you guys wont volunteer to fix up the entire subsystem either we just have to live with that I think. Also, the commit adding the drm_managed stuff does explain these implementation details and the reasons why.
Also note that tons of stuff in drm doesn't yet provide drmm_ versions, teardown-less drivers really only works for really simple ones. So completely getting rid of the ->release callback will also need lots of core work, like the currently in-flight series to add more drmm_ helpers for kms objects:
https://lore.kernel.org/dri-devel/20200827160545.1146-1-p.zabel@pengutronix....
Help obviously very much welcome.
Cheers, Daniel