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 :-) -- Regards, Laurent Pinchart
