Hi Dmitry,
On Mon, Feb 28, 2022 at 11:07:41AM +0300, Dmitry Baryshkov wrote:
On Mon, 28 Feb 2022 at 11:00, Laurent Pinchart wrote:
On Sat, Feb 26, 2022 at 05:10:06AM +0000, Kandpal, Suraj wrote:
Hi Abhinav,
Based on the discussion in this thread [1] , it seems like having drm_encoder as a pointer seems to have merits for both of us and also in agreement with the folks on this thread and has a better chance of an ack.
However drm_connector is not.
I had a brief look at your implementation. Any reason you need to go through intel_connector here and not drm_writeback_connector directly?
The reason I ask is that I see you are not using prepare_writeback_job hook. If you use that, you can use the drm_writeback_connector passed on from there instead of looping through your list like you are doing in intel_find_writeback_connector.
Also, none of the other entries of struct intel_connector are being used for the writeback implementation. So does the drm_writeback_connector in your implementation need to be an intel_connector when its anyway not using other fields? Why cant it be just stored as a drm_writeback_connector itself in your struct intel_wd.
The reason we can't do that is i915 driver always assumes that all connectors present in device list is an intel connector and since drm_writeback_connector even though a faux connector in it's initialization calls drm_connector_init() and gets added to the drm device list and hence the i915 driver also expects a corresponding intel connector to go with it. In case I do try to make writeback connector standalone a lot of checks, a lot will have to be added all around the driver as there could be a chance that one of the drm_connector in the drm device list may not be an intel_connector. Yes right now not all fields of intel_connector are being used but we will be working on filling them as we add more functionality to writeback for example introducing content protection. So even if I do float the patch series with just drm_encoder as pointer it won't help us with our implementation if drm_connector isn't a pointer too.
This is a direct consequence of the decision to use connectors for writeback in the userspace API. This disrupts any code that assumes that a connector is a connector. The problem isn't limited to kernelspace, userspace has the same exact problem, which resulted in a hack to avoid breaking everything. Userspace software that needs to deal with writeback needs to set the DRM_CLIENT_CAP_WRITEBACK_CONNECTORS capability to get the writeback connectors exposed by the kernel, but more than that, a large refactoring is then often needed to chase all code paths that assume a connector is a connector.
I'm afraid the same applies to the kernel. Drivers that don't use writeback are largely unaffected. Drievrs that want to use writeback need to be refactored to properly handle the fact that writeback connectors are special. i915 will need to go that way.
Laurent, you have frown upon the idea of separating the connector from the drm_writeback_connector struct. What about the encoder? The msm code in question can be found at the patchwork: https://patchwork.freedesktop.org/series/99724/. This series depends on Intel's patch, but should give you the overall feeling of the code being shared between to-the-display and writeback pipelines.
I'm not too fond of separating the encoder either as explained separately in this mail thread, but I won't block that as it's even more difficult to avoid today. drm_encoder is a bit of a historical mistake that we need to keep around because it's exposed to userspace. With drivers more and more reliant on drm_bridge, drm_encoder is less meaningful than it used to be. I would like to see the subsystem continuing in that direction, with drm_encoder becoming an empty shell. Drivers should decouple the CRTC outputs from the drm_encoder object, likely creating driver-specific structures to model a CRTC output (which is largely what the driver-specific subclasses of drm_encoder do today), and create drm_encoder instances only for the purpose of exposing the display topology to userspace.
Longer term I can even imagine having a different way to expose the display topology to userspace, without drm_encoder but with objects that will be allowed to support more complex topologies that the CRTC + encoder + connector abstraction can't model. Later :-)