Hi Suraj, 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. -- Regards, Laurent Pinchart
